On 05/04/2010 05:07 PM, Eric Blake wrote:
> On 04/30/2010 09:44 AM, Cole Robinson wrote:
>> @@ -1028,6 +1028,7 @@ pciGetDevice(unsigned domain,
>> unsigned function)
>> {
>> pciDevice *dev;
>> + char devdir[PATH_MAX];
>
> Using PATH_MAX as an array size is dangerous; it fails on GNU Hurd where
> there is no minimum size. Also, it wastes a lot of space, given your
> usage...
>
>> char *vendor, *product;
>>
>> if (VIR_ALLOC(dev) < 0) {
>> @@ -1043,8 +1044,17 @@ pciGetDevice(unsigned domain,
>>
>> snprintf(dev->name, sizeof(dev->name), "%.4x:%.2x:%.2x.%.1x",
>> dev->domain, dev->bus, dev->slot, dev->function);
>> + snprintf(devdir, sizeof(devdir),
>> + PCI_SYSFS "devices/%s", dev->name);
>
> ...here, of concatenating two relatively short strings. I'd almost
> rather see a virAsprintf/free.
>
>> snprintf(dev->path, sizeof(dev->path),
>> - PCI_SYSFS "devices/%s/config", dev->name);
>> + "%s/%s/config", devdir, dev->name);
>> +
>> + if (access(devdir, X_OK) != 0) {
>
> Is this ever run in a situation where the effective id might not equal
> the user id (that is, as a setuid script, or as root)? If so, would it
> be better to use faccessat(...,AT_EACCESS) instead of access() to be
> querying the correct permissions? (Gnulib provides faccessat for all
> platforms).
>
Actually, my code was wrong, I just wanted to test for the existence of
the file with F_OK, so sounds like faccessat isn't what I want in that
case. I'm sending an updated patch which is also much simpler, and just
checks dev->path rather than using a new variable.
Thanks,
Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list