[libvirt] [PATCH] pci: Give an explicit error if device not found

[Cole Robinson]

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