[Intel-gfx] [PATCH v4 3/5] drm: Add dispatcher and driver identification for DRM

[Patrik Jakobsson]

On Tue, Sep 08, 2015 at 03:36:25AM +0300, Dmitry V. Levin wrote:
> On Mon, Aug 24, 2015 at 02:42:48PM +0200, Patrik Jakobsson wrote:
> > * Makefile.am: Add compilation of drm.c.
> > * defs.h: Add extern declaration of drm_ioctl when drm headers are found.
> > * drm.c: New file.
> > * ioctl.c (ioctl_decode): Dispatch drm ioctls when drm headers are found.
> 
> * defs.h (drm_decode_number, drm_ioctl): New prototypes.
> * drm.c: New file.
> * Makefile.am (strace_SOURCES): Add it.
> * ioctl.c (ioctl_decode_command_number, ioctl_decode)
> [HAVE_DRM_H || HAVE_DRM_DRM_H]: Dispatch drm ioctls.

Ugh, thought I had this correct already. Will fix.

> > --- a/defs.h
> > +++ b/defs.h
> > @@ -612,6 +612,9 @@ extern const char *sprint_open_modes(int);
> >  extern void print_seccomp_filter(struct tcb *tcp, unsigned long);
> >  
> >  extern int block_ioctl(struct tcb *, const unsigned int, long);
> > +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> > +extern int drm_ioctl(struct tcb *, const unsigned int, long);
> > +#endif
> 
> I think function these prototypes could be added unconditionally.
> Note that v3 version of this patch also declared drm_decode_number().

Ok. Will move the number decoding pieces back into this patch. Not sure why I
did this in the first place.

> > --- /dev/null
> > +++ b/drm.c
> [...]
> > +#include "defs.h"
> > +
> > +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> > +
> > +#ifdef HAVE_DRM_H
> > +#include <drm.h>
> > +#else
> > +#include <drm/drm.h>
> > +#endif
> > +
> > +#include <sys/param.h>
> 
> Let's include <sys/param.h> before drm stuff.

Ok

> > +#define DRM_MAX_NAME_LEN 128
> > +
> > +inline int drm_is_priv(const unsigned int num)
> > +{
> > +	return (_IOC_NR(num) >= DRM_COMMAND_BASE &&
> > +		_IOC_NR(num) < DRM_COMMAND_END);
> > +}
> 
> This function has to be static, and like other static functions,
> it has to be added along with its first user, otherwise the project
> won't build with --enable-gcc-Werror.

Will move it to the correct patch and make it static.

> > +static char *drm_get_driver_name(struct tcb *tcp)
> > +{
> > +	char path[PATH_MAX];
> > +	char link[PATH_MAX];
> > +	int ret;
> > +
> > +	if (getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1) < 0)
> > +		return NULL;
> > +
> > +	snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
> > +		 basename(path));
> 
> if (snprintf(link, sizeof(link), ...) >= sizeof(link))
> 	return NULL;
> 

According to manpage snprintf never returns more than the specified size - 1.
So the only error we can check for is:

if (snprintf(link, sizeof(link), ...) <
    sizeof("/sys/class/drm/%s/device/driver"))

> > +static void drm_free_priv(void *data)
> > +{
> > +	free(data);
> > +}
> 
> Do we really need a wrapper for free(3)?

No. The only reason I see is for clarity on how to use the tcb priv interface.
But that is ofc debatable.

> > --- a/ioctl.c
> > +++ b/ioctl.c
> > @@ -248,6 +248,10 @@ ioctl_decode(struct tcb *tcp)
> >  	case 0x22:
> >  		return scsi_ioctl(tcp, code, arg);
> >  #endif
> > +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> > +	case 'd':
> > +		return drm_ioctl(tcp, code, arg);
> > +#endif
> >  	case 'L':
> >  		return loop_ioctl(tcp, code, arg);
> >  	case 'M':
> 
> v3 version also patched ioctl_decode_command_number()
> to call drm_decode_number().

Will move it back into this patch.

> 
> 
> -- 
> ldv