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.
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
