[libvirt] [PATCH] nodeinfo: Make sure we always reset errno before calling readdir

[Eric Blake]

On 04/10/2014 08:38 AM, Natanael Copa wrote:

>>> -    errno = 0;
>>> -    while ((cpudirent = readdir(cpudir))) {
>>> +    for (errno = 0; (cpudirent = readdir(cpudir)); errno = 0) {
>>>          if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
>>>              continue;
>>
>> Good catch.  However, the code is still buggy even after your fix.  We
>> are missing:
>>
>> if (errno)
>>     break;
>>
>> before attempting the sscanf.
> 
> Why? if readdir fails it should return NULL and the for() loop
> condition should break the loop. If readdir does not return NULL it
> didn't fail and errno value is undefined.

Uggh, you're right.  Yet another reason why I hate the readdir() interface.

> 
> I suppose we could use helper function to make it more readable:
> 
> static struct dirent *virReaddir(DIR *dirp)
> {
>     errno = 0;
>     return readdir(dirp);
> }

Or maybe:

int virReaddir(DIR *dirp, struct dirent **ent)
{
    errno = 0;
    *ent = readdir(dirp);
    if (!*ent && errno) {
        virReportSystemError(errno, _("unable to read directory"))
        return -1;
    }
    return *ent ? 1 : 0;
}

and used as:

while ((ret = virReaddir(dirp, &entry)) > 0) {
    process entry
}
if (ret < 0)
    goto error;


> while ((cpudirent = virReaddir(cpudir))
> 
>> Furthermore, sscanf() is undefined on
>> overflow; while the cpudir is unlikely to be giving us integers that
>> overflow, it would be nicer to not use sscanf in the first place.  It's
>> more than just the improper use of readdir that needs fixing here.
>>
>> I'm debating whether to push this now and fix the rest as a followup, or
>> whether to do a v2 that fixes it all at once.
> 
> I'd say fix the current bug first and do the clean up in separate commit.

Saving sscanf() abuse cleanup for a separate commit is just fine.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140410/eb376cba/attachment-0001.sig>