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

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

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.

>> I suppose we could use helper function to make it more readable: >>

> 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; This looks better yes. Should I prepare a new patch with this? And grep for more readdirs?

On Thu, 10 Apr 2014 15:53:57 -0600 Eric Blake <eblake(a)redhat.com&gt; wrote: > On 04/10/2014 02:52 PM, Natanael Copa wrote: > > >>> I suppose we could use helper function to make it more readable: > >>> > > >> 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; > > or shorter, return !!*ent; > > >> } > >> > >> and used as: > >> > >> while ((ret = virReaddir(dirp, &entry)) > 0) { > >> process entry > >> } > >> if (ret < 0) > >> goto error; > > > > This looks better yes. > > > > Should I prepare a new patch with this? And grep for more readdirs? > > Sure; and while at it, update cfg.mk to add a new syntax check to > enforce this style. We've wrapped other awkward standard functions that > require errno manipulation for correct use (such as strtol and > getpwuid_r), precisely because code is more maintainable when we can > enforce that the awkward functions are always used correctly. I found a fairly nice example of readdir used correctly in vircgroup.c. We could do it like that too. It means less intrusive changes. And we avoid changing the current error messages from "Unable to iterate over TAP/loop/NBD devices" to "unable to read directory". We could alternatively pass another option with the dirname for error messages.

On Thu, 10 Apr 2014 15:53:57 -0600 Eric Blake <eblake(a)redhat.com&gt; wrote: > On 04/10/2014 02:52 PM, Natanael Copa wrote: > > >>> I suppose we could use helper function to make it more readable: > >>> > > >> 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; > > or shorter, return !!*ent; > > >> } > >> > >> and used as: > >> > >> while ((ret = virReaddir(dirp, &entry)) > 0) { > >> process entry > >> } > >> if (ret < 0) > >> goto error; > > > > This looks better yes. > > > > Should I prepare a new patch with this? And grep for more readdirs? > > Sure; and while at it, update cfg.mk to add a new syntax check to > enforce this style. We've wrapped other awkward standard functions that > require errno manipulation for correct use (such as strtol and > getpwuid_r), precisely because code is more maintainable when we can > enforce that the awkward functions are always used correctly. I started with virDirOpen/Read/Close API but it turned out to be a can of worms. Some places we apparently check if closedir() works and logs errors. Some places not. Not big deal since virDirClose wrapper can make sure errors are always logged. But some places we ignore errors for virDirOpen (conf/domain_conf.c). this means that we need the virDirOpen optionally support 'ignore missing dirs' or we simply drop to use virDirOpen in this case. I am starting to think that we should probably just fix the way opendir is used. Its not that hard to do it "right": int rc = -1; for (;;) { struct dirent *ent; errno = 0; ent = readdir(dirp); if (ent == NULL) { if ((rc = -errno)) { /* log error */ } break; } ... } Or do you prefer that I continue bang my head into virDirOpen/Read/Close API? Or should we just keep current use of opendir and closedir and only implement virDirRead?