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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list