Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors

[David Turner <novalis@novalis.org>]

On Tue, 2016-09-27 at 15:19 -0400, Jeff King wrote:
> On Tue, Sep 27, 2016 at 11:19:34AM -0400, David Turner wrote:
> 
> > >   typedef void (*err_fn)(const char *, ...);
> > > 
> > >   static int decode_tree_entry(struct tree_desc *desc,
> > >                                const char *buf, unsigned long size,
> > > 			       err_fn err)
> > >   {
> > >          ...
> > >          if (size < 23 || buf[size - 21]) {
> > > 	        err("too-short tree object");
> > > 		return -1;
> > > 	 }
> > >   }
> > > 
> > > I dunno. Maybe that is overengineering. I guess we only hit the strbufs
> > > in the error path (which used to die!), so nobody really cares that much
> > > about the extra allocation.
> > 
> > I don't really like err_fn because:
> > (a) without a baton, it's somewhat less general (or less thread-safe)
> > than the strbuf approach and
> > (b) with a baton, it's two arguments instead of one.
> 
> I'm going to ramble for a minute, and I don't think it's worth exploring
> for this patch series in particular, so feel free to ignore me.
> 
> I think this error concept could be extended fairly elegantly with
> something like:
> 
>   typedef void (*err_fn)(void *, const char *fmt, va_list ap)
>   struct error_context {
>         err_fn fn;
>         void *data;
>   };
> 
>   int report_error(struct error_context *err, const char *fmt, ...)
>   {
>         if (err->fn) {
>                 va_list ap;
>                 va_start(ap, fmt);
>                 err->fn(err->data, fmt, ap);
>                 va_end(ap);
>         }
>         return -1;
>   }
> 
> Then low-level functions just take a context and do:
> 
>   return report_error(&err, "some error: %s", foo);
> 
> And then the callers would pick one of a few generic error contexts:
> 
>   - passing NULL silences the errors

Overall, +1.

I guess I would rather have a sentinel value for silencing errors,
because I'm worried that someone might read NULL as "don't handle the
errors, just die".  Of course, code review would hopefully catch this,
but even so, it would be easier to read foo(x, y, silence_errors) than
foo(x, y, null).