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

[Gary Wong]

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?

> > -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?).

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


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

Gary.
-- 
     Gary Wong         gtw at gnu.org         http://www.cs.utah.edu/~gtw/