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

[Patrik Jakobsson <patrik.jakobsson@linux.intel.com>]

On Fri, Jul 03, 2015 at 03:33:31AM +0300, Dmitry V. Levin wrote:
> 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:

Yes, that's more robust. I was afraid it would be too intrusive.

> > * 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.
> 
> [...]

I'll get that sorted out.

> > +#include "defs.h"
> > +
> > +#include <drm.h>
> > +#include <linux/limits.h>
> 
> Please include <sys/param.h> instead of <linux/limits.h>.

Yup

> > +#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));
> }
> 

That's nicer

> > +
> > +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.

Ok

> > +		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


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