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@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 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list