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

[Daniel P. Berrange]

On Fri, Apr 11, 2014 at 09:48:41AM +0000, 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 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.

The root problem here is that the readdir() API is incredibly badly
designed. If we continue to use readdir() directly, we're going to
hit this bug over and over and over and over and over again. We
should introduce a wrapper that makes it harder for people to write
incorrect code, so we can solve this bug once now and be done with it.

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