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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 10 Apr 2014 15:53:57 -0600
Eric Blake <eblake@xxxxxxxxxx> 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.

> Food for thought: depending on how many clients you find, it may be
> worth a version of virReaddir() that takes a flag argument, for
> auto-filtering '.' and '..' entries.
> 

I found a virDirCreate function so if we still want do the readdir,
then we should probably do: virDirOpen, virDirRead and virDirClose.

-nc

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]