Re: [PATCH libdrm v3 2/5] xf86drm: Add USB support

[walter harms]

Am 19.01.2017 11:20, schrieb Thierry Reding:
> Adding back dri-devel@lists.freedesktop.org
> 
> On Wed, Jan 18, 2017 at 12:21:23PM +0100, walter harms wrote:
>>
>>
>> Am 18.01.2017 10:02, schrieb Thierry Reding:
>>> Allow DRM/KMS devices hosted on USB to be detected by the drmDevice
>>> infrastructure.
>>>
>>> v3:
>>> - guard Linux-specific sysfs parsing code with #ifdef __linux__
>>>
>>> v2:
>>> - make sysfs_uevent_get() more flexible using a format string
>>>
>>> Signed-off-by: Thierry Reding 
>>> ---
>>>  xf86drm.c | 175 
>>> ++
>>>  xf86drm.h |  13 +
>>>  2 files changed, 188 insertions(+)
>>>
>>> diff --git a/xf86drm.c b/xf86drm.c
>>> index 7766bfe937db..d83674e638c4 100644
>>> --- a/xf86drm.c
>>> +++ b/xf86drm.c
>>> @@ -2886,6 +2886,50 @@ char *drmGetRenderDeviceNameFromFd(int fd)
>>>  return drmGetMinorNameForFD(fd, DRM_NODE_RENDER);
>>>  }
>>>  
>>> +#ifdef __linux__
>>> +static char * DRM_PRINTFLIKE(2, 3)
>>> +sysfs_uevent_get(const char *path, const char *fmt, ...)
>>> +{
>>> +char filename[PATH_MAX + 1], *key, *line = NULL, *value = NULL;
>>char *filename=NULL, *key, *line = NULL, *value = NULL;
>>> +size_t size = 0, len;
>>> +ssize_t num;
>>> +va_list ap;
>>> +FILE *fp;
>>> +
>>> +va_start(ap, fmt);
>>> +num = vasprintf(, fmt, ap);
>>> +va_end(ap);
>>> +len = num;
>>> +
>>> +snprintf(filename, sizeof(filename), "%s/uevent", path);
>>
>>  since asprintf() is available you could use:
>>
>> asprintf(,"%s/uevent", path);
>>
>>  same could be done for path below.
> 
> I had thought about that, but a stack-allocated string seemed
> advantageous for three reasons:
> 
>   - asprintf() is a GNU extension. That's not much of an issue
> because this is already protected by #ifdef __linux__ and
> pretty much all C libraries I know support asprintf() and
> friends.
> 
>   - PATH_MAX is the maximum length of a filename, so there's no
> need to dynamically allocate, since it should nicely fit into
> the stack pretty much everywhere (I think the largest value I
> have ever seen for PATH_MAX is 4096 on recent Linux systems).
> 
>   - Most of the other code in xf86drm.c already uses PATH_MAX, so
> the code remains consistent.
> 
> Given the added complexity of asprintf() and the need to free the memory
> the advantages of the stack-allocated string seemed to outweigh.
> 
> Thierry

Granted,

I like to use asprintf every time %s is used to stop malicious users in the 
tracks
(do not aks me how this could be done with our function). Freeing the allocated 
buf
is no practical problem. NTL i have no problem if you are happy with the patch, 
making
an informed decision is ok with me.

re,
 wh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm v3 2/5] xf86drm: Add USB support

[Thierry Reding]

Adding back dri-devel@lists.freedesktop.org

On Wed, Jan 18, 2017 at 12:21:23PM +0100, walter harms wrote:
> 
> 
> Am 18.01.2017 10:02, schrieb Thierry Reding:
> > Allow DRM/KMS devices hosted on USB to be detected by the drmDevice
> > infrastructure.
> > 
> > v3:
> > - guard Linux-specific sysfs parsing code with #ifdef __linux__
> > 
> > v2:
> > - make sysfs_uevent_get() more flexible using a format string
> > 
> > Signed-off-by: Thierry Reding 
> > ---
> >  xf86drm.c | 175 
> > ++
> >  xf86drm.h |  13 +
> >  2 files changed, 188 insertions(+)
> > 
> > diff --git a/xf86drm.c b/xf86drm.c
> > index 7766bfe937db..d83674e638c4 100644
> > --- a/xf86drm.c
> > +++ b/xf86drm.c
> > @@ -2886,6 +2886,50 @@ char *drmGetRenderDeviceNameFromFd(int fd)
> >  return drmGetMinorNameForFD(fd, DRM_NODE_RENDER);
> >  }
> >  
> > +#ifdef __linux__
> > +static char * DRM_PRINTFLIKE(2, 3)
> > +sysfs_uevent_get(const char *path, const char *fmt, ...)
> > +{
> > +char filename[PATH_MAX + 1], *key, *line = NULL, *value = NULL;
>char *filename=NULL, *key, *line = NULL, *value = NULL;
> > +size_t size = 0, len;
> > +ssize_t num;
> > +va_list ap;
> > +FILE *fp;
> > +
> > +va_start(ap, fmt);
> > +num = vasprintf(, fmt, ap);
> > +va_end(ap);
> > +len = num;
> > +
> > +snprintf(filename, sizeof(filename), "%s/uevent", path);
> 
>   since asprintf() is available you could use:
> 
> asprintf(,"%s/uevent", path);
> 
>   same could be done for path below.

I had thought about that, but a stack-allocated string seemed
advantageous for three reasons:

- asprintf() is a GNU extension. That's not much of an issue
  because this is already protected by #ifdef __linux__ and
  pretty much all C libraries I know support asprintf() and
  friends.

- PATH_MAX is the maximum length of a filename, so there's no
  need to dynamically allocate, since it should nicely fit into
  the stack pretty much everywhere (I think the largest value I
  have ever seen for PATH_MAX is 4096 on recent Linux systems).

- Most of the other code in xf86drm.c already uses PATH_MAX, so
  the code remains consistent.

Given the added complexity of asprintf() and the need to free the memory
the advantages of the stack-allocated string seemed to outweigh.

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel