Re: [libvirt] [PATCH] nodeinfo: Make sure we always reset errno before calling readdir
On Thu, 10 Apr 2014 15:53:57 -0600
Eric Blake <eblake(a)redhat.com> 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?
-nc
Attachments:
- signature.asc (application/pgp-signature — 181 bytes)