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

[Richard W.M. Jones]

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

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top