[Libguestfs] [libnbd PATCH 0/4] Work towards fixing nbdcopy silent corruption

[Laszlo Ersek]

On 02/02/22 11:55, Richard W.M. Jones wrote:
> On Wed, Feb 02, 2022 at 09:59:04AM +0100, Laszlo Ersek wrote:
>> However, in extent_wrapper() -> extent_wrapper_locked(), there is more
>> confusion for me:
>>
>>   metacontextv = caml_copy_string (metacontext);
>>   offsetv = caml_copy_int64 (offset);
>>   entriesv = nbd_internal_ocaml_alloc_int64_from_uint32_array (entries, nr_entries);
>>   errorv = caml_alloc_tuple (1);
>>   Store_field (errorv, 0, Val_int (*error));
>>   args[0] = metacontextv;
>>   args[1] = offsetv;
>>   args[2] = entriesv;
>>   args[3] = errorv;
>>   rv = caml_callbackN_exn (data->fnv, 4, args);
>>   *error = Int_val (Field (errorv, 0));
>>   if (Is_exception_result (rv)) {
>>     nbd_internal_ocaml_exception_in_wrapper ("extent", rv);
>>     CAMLreturnT (int, -1);
>>   }
>>
>>   r = Int_val (rv);
>>   assert (r >= 0);
>>   CAMLreturnT (int, r);
>>
>> If I understand correctly, if we returned (-1) from the (nameless)
>> extents callback, that would trip the assert(); would it not? Thus, we
>> would not reach the nbd_internal_ocaml_raise_error() in the above-quoted
>> nbd_internal_ocaml_nbd_block_status() function.
> 
> There are five cases:
> 
>  - Callback returns > 1
>    => The program shouldn't do this, but it's treated the same
>       as the next case.
> 
>  - Callback returns 1
>    => For completion callbacks this indicates that the callback was
>       successful and you want to autoretire the command.  For other
>       types of callbacks the program shouldn't do this but it is the
>       same as the next case.
> 
>  - Callback returns 0
>    => Indicates that the callback was successful.
> 
>  - Callback throws an exception
>    => nbd_internal_ocaml_exception_in_wrapper is called
>    => Callback returns -1 indicating (to libnbd) that there was
>       an error.  cmd->error is updated with !err || EPROTO.
> 
>  - Callback returns < 0
>    => assert fails!
> 
> The assert is a bit harsh, but it indicates a bug in the OCaml code.
> It probably means the OCaml code thought that returning -1 indicates
> an error, whereas the OCaml code should throw an exception instead.
> 
> For the history of why that assert was added see:
> 
> https://gitlab.com/nbdkit/libnbd/-/commit/d881d160e1cd9c9964782300a7652ffb4e506c27

Thank you!
Laszlo