Re: [PATCH v3 2/5] drm: Add private data field to trace control block

["Dmitry V. Levin" <ldv@altlinux.org>]

[-- Attachment #1.1: Type: text/plain, Size: 5062 bytes --]

On Wed, Jul 01, 2015 at 02:52:45PM +0200, Patrik Jakobsson wrote:
[...]
> --- a/defs.h
> +++ b/defs.h
> @@ -266,6 +266,13 @@ struct tcb {
>  	int u_error;		/* Error code */
>  	long scno;		/* System call number */
>  	long u_arg[MAX_ARGS];	/* System call arguments */
> +
> +	/*
> +	 * Private data for the decoding functions of the syscall. TCB core does
> +	 * _not_ handle allocation / deallocation of this data.
> +	 */
> +	void *priv_data;
> +

This will result to memory leaks if droptcb() is called before the
syscall parser that allocated memory had a chance to deallocate it.
As this data is no longer relevant after leaving trace_syscall_exiting(),
I suggest to perform deallocation directly from trace_syscall_exiting.

This API could be made more flexible by adding another pointer -
the function to be called to deallocate memory, e.g.
struct tcb {
	...
	void *priv_data;
	void (*free_priv_data)(void *);
	...
};
...
void
free_priv_data(struct tcb *tcp)
{
	if (tcp->priv_data) {
		if (tcp->free_priv_data) {
			tcp->free_priv_data(tcp->priv_data);
			tcp->free_priv_data = NULL;
		}
		tcp->priv_data = NULL;
	}
}
...
droptcb(struct tcb *tcp)
{
	...
	free_priv_data(tcp);
	...
}
...
trace_syscall_exiting(struct tcb *tcp)
{
	...
 ret:
	free_priv_data(tcp);
	...
}

[...]
On Wed, Jul 01, 2015 at 02:52:46PM +0200, Patrik Jakobsson wrote:
> * Makefile.am: Add compilation of drm.c
> * defs.h: Declarations of drm functions
> * drm.c: Utility functions for drm driver detection
> * io.c: Dispatch drm ioctls
> * ioctl.c: Distpatch generic and driver specific ioctls

This is not quite a GNU style changelog entry.  Please have a look at
http://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html
and examples in strace.git history.

[...]
> +#include "defs.h"
> +
> +#include <drm.h>
> +#include <linux/limits.h>

Please include <sys/param.h> instead of <linux/limits.h>.

> +#define DRM_MAX_NAME_LEN 128
> +
> +struct drm_ioctl_priv {
> +	char name[DRM_MAX_NAME_LEN];
> +};
> +
> +inline int drm_is_priv(const unsigned int num)
> +{
> +	return (_IOC_NR(num) >= DRM_COMMAND_BASE &&
> +		_IOC_NR(num) < DRM_COMMAND_END);
> +}
> +
> +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize)
> +{
> +	char path[PATH_MAX];
> +	char link[PATH_MAX];
> +	int ret;
> +
> +	ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
> +		 basename(path));
> +
> +	ret = readlink(link, path, PATH_MAX - 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	path[ret] = '\0';
> +	strncpy(name, basename(path), bufsize);
> +
> +	return 0;
> +}

I think this is getting too complicated.  This function could just return
strdup(basename(path)) or NULL in case of any error:

static char *
drm_get_driver_name(struct tcb *tcp, const char *name)
{
	char path[PATH_MAX];
	char link[PATH_MAX];
	int ret;

	if (getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1) < 0)
		return NULL;

	if (snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
		     basename(path)) >= PATH_MAX)
		return NULL;

	ret = readlink(link, path, PATH_MAX - 1);
	if (ret < 0)
		return NULL;

	path[ret] = '\0';
	return strdup(basename(path));
}

> +
> +int drm_is_driver(struct tcb *tcp, const char *name)
> +{
> +	struct drm_ioctl_priv *priv;
> +	int ret;
> +
> +	/*
> +	 * If no private data is allocated we are detecting the driver name for
> +	 * the first time and must resolve it.
> +	 */
> +	if (tcp->priv_data == NULL) {
> +		tcp->priv_data = xcalloc(1, sizeof(struct drm_ioctl_priv));

xcalloc shouldn't be used if a potential memory allocation error is not
fatal.  In a parser that performs verbose syscall decoding no memory
allocation error is fatal.

> +		priv = tcp->priv_data;
> +
> +		ret = drm_get_driver_name(tcp, priv->name, DRM_MAX_NAME_LEN);
> +		if (ret)
> +			return 0;
> +	}
> +
> +	priv = tcp->priv_data;
> +
> +	return strncmp(name, priv->name, DRM_MAX_NAME_LEN) == 0;

Then with priv_data+free_priv_data interface this would looks smth like
	...
	if (!tcp->priv_data) {
		tcp->priv_data = drm_get_driver_name(tcp, name);
		if (tcp->priv_data) {
			tcp->free_priv_data = free;
		} else {
			tcp->priv_data = (void *) "";
			tcp->free_priv_data = NULL;
		}
	}
	return !strcmp(name, (char *) tcp->priv_data);

> +}
> +
> +int drm_decode_number(struct tcb *tcp, unsigned int arg)

This is an ioctl request code, let's consistently call it "code" to
distinguish from its argument.

[...]
> --- a/ioctl.c
> +++ b/ioctl.c
> @@ -182,7 +182,7 @@ hiddev_decode_number(unsigned int arg)
>  }
>  
>  int
> -ioctl_decode_command_number(unsigned int arg)
> +ioctl_decode_command_number(struct tcb *tcp, unsigned int arg)

I've already changed ioctl_decode_command_number's signature:
struct tcb * is already there and "arg" is now called "code".


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx