Re: [GSoC][PATCH 1/3] dir-iterator: add pedantic option to dir_iterator_begin

[Thomas Gummerer <t.gummerer@gmail.com>]

On 02/24, Matheus Tavares Bernardino wrote:
> On Sun, Feb 24, 2019 at 5:35 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> > On Sat, Feb 23, 2019 at 10:37 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > > >  int dir_iterator_advance(struct dir_iterator *dir_iterator)
> > > > @@ -71,6 +78,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
> > > >
> > > >                       level->dir = opendir(iter->base.path.buf);
> > > >                       if (!level->dir && errno != ENOENT) {
> > > > +                             if (iter->pedantic)
> > > > +                                     goto error_out;
> > >
> > > I think we should also print an error here.  The caller doesn't have
> > > any context on what went wrong, and will probably just 'die()' if an
> > > error is encountered.
> 
> To correctly handle the error, I assumed that the caller wouldn't need
> to know which exact function returned an error, as long as it knew it
> was a "failure to fetch the next entry" kind of error, which is the
> "category" of errors caught with the 'pedantic' option. (currently, it
> includes errors on lstat, opendir and readdir). Is this assumption
> valid?
>
> > > I think it would make sense to call
> > > 'error(...)' here before 'goto error_out;' to give a useful error
> > > message here.
> >
> > If we start to give error messages, then we might as well give error
> > messages all the times when we error out. This will avoid the callers
> > wondering if they need to give an error message or not.
> >
> > I am not sure it's necessary here though. And I think if it's useful,
> > it can be added in another patch or another patch series.
> >
>
> I could just copy the warning messages bellow each 'goto error_out'
> and use then at an 'error(...)' call before the goto. But as Christian
> pointed out, I think this would confuse callers wether they should
> print error messages or not. On the other side, it may just be
> different 'layers' of errors... I don't have any strong opinion about
> this.

Right, I think it just comes down to which amount of detail we want to
communicate back to the user.  I thought a bit more detail could be
helpful, but just giving a more generic error should also be okay.

> > > >                               warning("error opening directory %s: %s",
> > > >                                       iter->base.path.buf, strerror(errno));
> > > >                               /* Popping the level is handled below */
> >
> > > > -struct dir_iterator *dir_iterator_begin(const char *path)
> > > > +struct dir_iterator *dir_iterator_begin(const char *path, int pedantic)
> > >
> > > Thinking about the future evolution of this interface, it might make
> > > more sense to have that second parameter be a "struct
> > > dir_iterator_opts".  For now it would just have one member "pedantic",
> > > but in the future we could add additional options there instead of
> > > adding additional parameters.
> >
> > I think it's ok with `int pedantic` for now as improvements can be
> > done when they are really needed. And we will perhaps find out that
> > it's better to just change `int pedantic` to `unsigned flags` instead
> > of `struct dir_iterator_opts`.
> >
> 
> I did thought about using `unsigned flags` instead of `int pedantic`
> for the same reasons Thomas pointed out, but as there would be just
> one flag for now, it seemed to me that `int pedantic` would make more
> sense (following the 'YAGNI' principle). But if it is already known
> that more flags (or options) are coming in a very near future, I may
> change this to `unsigned flags` or `struct dir_iterator_opts` in v3 if
> you think it is needed. Just let me know, please.

Looking at the potential improvements that were suggested in the
initial commit adding dir-iterator, 'unsigned flags' would not be
enough to be able to pass all those options.  That's where my
suggestion for 'struct dir_iterator_opts' comes in.

But I don't feel too strongly about it, and am okay with just an 'int
pedantic' option for now, until we see different usages, if others
feel like that's the better option for now.

> > Thanks,
> > Christian.