Re: [Libguestfs] [PATCH nbdkit 1/2] ocaml: Simplify NBDKit.set_error

On Mon, Nov 15, 2021 at 02:00:25PM +0100, Laszlo Ersek wrote: > On 11/15/21 13:14, Richard W.M. Jones wrote: > > (aren't you on PTO?) Eh hem .. >> -# This is somewhat different from the other tests because we have >> -# to build an actual plugin here. > > (1) I think this comment removal belongs to a separate patch. (Not a > "hard requirement", just saying.) Done, with clearer explanation: https://gitlab.com/nbdkit/nbdkit/-/commit/6b821fdc4ced4fc0af3fe2480d3ca43... >> + char *args[] = { >> + "nbdkit", "-s", "--exit-with-parent", >> + "./test-ocaml-errorcodes-plugin.so", NULL >> + }; > > (3) Style (well, at least my personal preference, not sure about nbdkit > standards): this should be > > static const char * const args[] = ... > > or at least (cue <https://libguestfs.org/nbd_connect_command.3.html>) > > static char *args[] = ... > > and in either case it should go to the top of the block (not mixing > declarations with code). > > Feel free to ignore of course if the pattern is already wide-spread in > nbdkit. Thie pattern has been copied around over about a dozen tests. How about attached instead? Compound literals are a fun, little know feature!

>> + assert (nbd_pread (nbd, buf, 512, 1*512, 0) == -1); >> + printf ("%s %d\n", nbd_get_error (), nbd_get_errno ()); >> + fflush (stdout); >> + assert (nbd_get_errno () == EPERM); >> + >> + assert (nbd_pread (nbd, buf, 512, 2*512, 0) == -1); >> + printf ("%s %d\n", nbd_get_error (), nbd_get_errno ()); >> + fflush (stdout); >> + assert (nbd_get_errno () == EIO); >> + >> + assert (nbd_pread (nbd, buf, 512, 3*512, 0) == -1); >> + printf ("%s %d\n", nbd_get_error (), nbd_get_errno ()); >> + fflush (stdout); >> + assert (nbd_get_errno () == ENOMEM); >> + >> + assert (nbd_pread (nbd, buf, 512, 4*512, 0) == -1); >> + printf ("%s %d\n", nbd_get_error (), nbd_get_errno ()); >> + fflush (stdout); >> + assert (nbd_get_errno () == ESHUTDOWN); >> + >> + assert (nbd_pread (nbd, buf, 512, 5*512, 0) == -1); >> + printf ("%s %d\n", nbd_get_error (), nbd_get_errno ()); >> + fflush (stdout); >> + assert (nbd_get_errno () == EINVAL); > > (4) This shouldn't be difficult to turn into a table of { sector, errno > } pairs, and a loop. Not necessarily for the space savings, but for ease > of reading. (I've found it hard to discern the only two differences > between each repetition of the block.) > > (5) The fflush(stdout) looks weird; if that really matters (e.g. for > following the log when it is written to a regular file), then we might > want to consider setvbuf() instead. Doesn't matter much if you implement > (4), because in that case, the loop body will contain only one fflush(). I fixed this all up in v2 which I'll post shortly. Rich.