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

[Daniel P. Berrange]

On Tue, Apr 15, 2014 at 11:27:12AM +0200, Natanael Copa wrote:
> On Thu, 10 Apr 2014 15:53:57 -0600
> Eric Blake <eblake at 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?

I'd only both with virDirRead, since that's the troublesome function


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|