Re: [Libguestfs] [PATCH nbdkit] New filter: request-request

On 10/14/21 22:58, Richard W.M. Jones wrote: > +/* This macro encapsulates the logic of retrying. */ > +#define RETRY(code) \ > + do { \ > + unsigned i; \ (1) Just for my own education: doesn't the nbdkit coding style dictate that we should separate this declaration from the following code with an empty line? For me, this is pretty hard to read.

(And yes, I strongly dislike C99-style intermixing of declarations and code :))) > + r = -1; \ > + for (i = 0; r == -1 && i <= retries; ++i) { \ > + if (i > 0) { \ > + nbdkit_debug ("retry %d: waiting %d seconds before retrying", \ > + i, delay); \ (2) My inner pedant raised his hand and said: "i" and "delay" are both unsigned, so they should be printed with "%u", not "%d". Both of these variables can extend up to user-specified "unsigned" values (i.e., up to UINT_MAX), so they can -- in theory at least -- exceed INT_MAX.

(3) A similarly impractical comment (but still): if "retries" (coming from the user) is UINT_MAX, then (i <= UINT_MAX) is constant 1. Simplest workaround is likely to put some arbitrary limits on the user specified inputs in retry_request_config(). > + if (nbdkit_nanosleep (delay, 0) == -1) { \ > + if (*err == 0) *err = errno; \ (4) I find this -- i.e., not putting the dependent assignment on a new line -- borderline unreadable; is this frequent in nbdkit?

> + RETRY ( > + /* Each retry must begin with extents reset to the right beginning. */ > + nbdkit_extents_free (extents2); > + extents2 = nbdkit_extents_new (offset, next->get_size (next)); > + if (extents2 == NULL) { > + *err = errno; > + return -1; /* Not worth a retry after ENOMEM. */ > + } > + r = next->extents (next, count, offset, flags, extents2, err); > + ); (5) So this exemplifies why [*] rubs me the wrong way. This is just too large & confusing as an argument to the RETRY() macro. In particular, the replacement text of this macro invocation will contain *two* semicolons (unlike the other invocations of the macro), because the macro says "code;" and here we finish off with "r = next->extents (...);". I see this as a problem with the macro, really, and not with this call site -- in my opinion, RETRY should not be a macro, but a function, and we should pass it function pointers. Yes, I realize that's many more boilerplate functions; I'd still find that better. (Likely easier to step-through in gdb as well!) For turning RETRY into a function, we might have to define a bunch of "context" structures too (for the individual callbacks to take), so... even more boilerplate. :)

> +static struct nbdkit_filter filter = { Constify this? ... Ah, no, NBDKIT_REGISTER_FILTER requires this to be modifiable. OK.