Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

From: Jeff King <peff@peff.net>
Date: Dec 4, 2014, 07:59 UTC
Message-ID: <20141204075920.GA27142@peff.net>
In-Reply-To: <20141203213858.GC6527@google.com>

On Wed, Dec 03, 2014 at 01:38:58PM -0800, Jonathan Nieder wrote:

Show 15 quoted lines

> > What about a struct that has an errno-like value _and_ a fixed-size
> > buffer? I'm thinking something like:
> >
> >   struct error {
> > 	int code;
> > 	char msg[1024];
> >   };
> 
> My experience with errno is that it is very hard to anticipate what
> granularity to use with error codes, especially if the error code is
> going to (in some contexts) determine the message printed to the user.
> In practice, it is easier to come up with a message at the error
> detection site and use a generic "something happened" error code
> except in those places where the caller is going to act on specific
> error types that need to be distinguished.

Yeah, I agree. I think error.msg there would be the main use, and error.code is just gravy. I agree it could simply come back through the integer return value, too.

Show 5 quoted lines

> The "< 0 means error" convention gives room to use different exit
> codes for the errors that need to be programmatically distinguished.
> For example, the ref transaction API uses this to distinguish D/F
> conflicts from other errors, while still returning an error message
> through a strbuf output parameter.

Yup. One nice thing about stuffing it into the error struct is that it saves the caller from declaring a separate variable (assuming they need "err" anyway to collect the message). I.e.:

  struct error err;

  if (some_func(fd, &err))
	react_to(err.code);

as opposed to:

  struct error err;
  int ret;

  ret = some_func(fd, &err);
  if (ret)
	react_to(ret);

But I think it's a minority of cases where we care about the specific value anyway, so it's probably not a big deal either way.

Show 7 quoted lines

> > If we changed the strbuf to a fixed-size buffer, that would help the
> > allocation issue. Some messages might be truncated, but it seems
> > unlikely in practice. It still loses readability, though.
> 
> I don't like the idea of having to choose a size in advance and not
> being able to fit more detailed (perhaps language-specific, and
> including user-specified input) messages lest the buffer overflow.

Is that really a problem in practice? Is a message larger than 1024 characters actually readable? It would have to be broken across many lines, and then I think we are no longer dealing with an error message, but some advice (which probably shouldn't go through this mechanism anyway).

> The allocation of a variable-sized buffer is a small overhead that I
> don't mind incurring on error.  In the non-error case, the caller
> doesn't actually have to free the buffer, and if they choose to, the
> overhead incurred is that of free(NULL)'.

I don't care at all about overhead. I care about extra work on the part of the caller to avoid a leak. It turns:

  if (some_func(fd, &err))
	return error("%s", err.msg);

into:

  if (some_func(fd, &err)) {
	error("%s", err.buf);
	strbuf_release(&err);
	return -1;
  }

It may make things a little easier when an intermediate function has to further munge the message. For example, your 04/14 uses strbuf_prefixf. But that can also be expressed in a buffer world as:

  /* "parent_err" is passed to us from caller, "err" is a local buffer */
  if (copy_fd(orig, fd, &err))
	return mkerror(&parent_err, "cannot copy '%s': %s", path, err.msg);

Strbufs make more complicated munging possible, but I'd expect prefixing to be the most common case (except for not munging at all, which I think is even more common).

I dunno. I just hate to see a system where it's very easy for callers to introduce memory leaks by forgetting a strbuf_release, or that bogs callers down with error-handling boilerplate.

-Peff