[Mesa-dev] [PATCH 2/2] loader: add optional /sys filesystem method for PCI identification.

[Emil Velikov]

On 22/05/14 04:58, Gary Wong wrote:
> On Wed, May 21, 2014 at 07:46:52PM +0100, Emil Velikov wrote:
>> On 21/05/14 02:40, Gary Wong wrote:
>>> Introduce a simple PCI identification method of looking up the answer
>>> the /sys filesystem (available on Linux).  Attempted after libudev, but
>>> before DRM.
>>>
>>> Disabled by default (available only when the --enable-sysfs configure
>>> option is specified).
>>
>> Gary does uevent provide the full device path on your system ?
> 
> No (see below).
> 
>> Can you please keep the code style similar to the rest of the file
>>  - no space around [] and ()
>>  - use sizeof(bla)
>>  - (pedantic) snprintf can fail/truncate.
>>  - empty lines with whitespace
> 
> Thanks... I tried to match, but wasn't aware of a couple of those points
> and some of the others fell through the cracks.
> 
> I'm not sure how to best handle snprintf here... the only conversions
> used are %d, and they cannot possibly overflow the (constant size) buffers
> unless both UINT_MAX and dev_t are very large (on the order of 10^16).  But
> I don't know a good way to express that.  How do you feel about: just plain
> sprintf() with comments about conditions under which it's always safe;
> snprintf() with dynamic buffer allocation; #if/#error tests on UINT_MAX;
> or something else?
> 
The buffer size expressed in hex kind of threw me over a bit. AFAICS things
should be safe as is. Unless things are fundamentally broken.

>>> -PKG_CHECK_MODULES([LIBUDEV], [libudev >= $LIBUDEV_REQUIRED],
>>> -                  have_libudev=yes, have_libudev=no)
>>> +AC_ARG_ENABLE([libudev],
>>> +    [AS_HELP_STRING([--disable-libudev],
>>> +        [disable libudev PCI identification @<:@default=enabled on supported platforms@:>@])],
>>                                                            ^^^^
>> Not strictly correct. I would just go with "auto"
>>
>>> +    [attempt_libudev="$enableval"],
>>> +    [attempt_libudev=yes]
>> [enable_libudev="$enableval"],
>> [enable_libudev=auto]
>>
>> The above is more consistent with the mayhem that our current configure.ac :)
> 
> On further thought, I'll revert that section of the commit.  The
> --disable-libudev option is silly, because the whole point of this patch
> is to carry on working at runtime even if libudev fails (therefore having
> it in cannot hurt, so why have an option not to?).
> 
Fair enough. The less options the better.

>>> +static char *
>>> +sysfs_get_device_name_for_fd(int fd)
>>> +{
>> On my linux system this approach returns "dri/card0" only. Whereas it should
>> produce "/dev/dri/card0".
> 
> Oops, well spotted!  My system does the same.  I assumed libudev would
> also have returned the partial path so misunderstood the intended return.
> Oddly enough Mesa worked perfectly well for me with the short name... I
> can exercise it on only one hardware type, and perhaps in my case it turns
> out not to be critical.  Thanks very much for catching that one!
> 
I'm guessing that you've never actually hit this code. IIRC there are two
use-cases - in the dri2 and gallium backends of the egl driver. If you're
really into it you can attach a debugger and check.

-Emil

> 
> Thank you for the detailed comments... I'll send revised patches in a minute.
> 
> Gary.
>