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

On 11/15/21 13:14, Richard W.M. Jones wrote: (aren't you on PTO?)

> -# 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.)

> + 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.

> + 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().