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

[Laszlo Ersek]

On 11/17/21 18:57, Richard W.M. Jones wrote:
> 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/6b821fdc4ced4fc0af3fe2480d3ca43cb6476a6d
> 
>>> +  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!

Looks fine to me, thanks!
Laszlo

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