Libguestfs - Feb 2022 - [libnbd PATCH v2 2/5] ocaml: tests: Fix error handling

On 02/03/22 21:25, Eric Blake wrote:
> Like a lot of the C examples, the aio copy test ignores read and write
> errors in the completion callback, which can cause silent data
> corruption. The failure in the test is not critical, but this is a bad
> example that may be copied by developers to a real application.
> 
> The test dies with an assertion failure now if completion callback
> fails.  Tested with the temporary patch of:
> 
> | diff --git i/ocaml/tests/test_590_aio_copy.ml
w/ocaml/tests/test_590_aio_copy.ml
> | index a0339c8b..332bf31b 100644
> | --- i/ocaml/tests/test_590_aio_copy.ml
> | +++ w/ocaml/tests/test_590_aio_copy.ml
> | @@ -120,7 +120,8 @@ let
> |    let dst = NBD.create () in
> |    NBD.set_handle_name dst "dst";
> |    NBD.connect_command src ["nbdkit"; "-s";
"--exit-with-parent"; "-r";
> | -                           "pattern"; sprintf
"size=%d" disk_size];
> | +                           "--filter=error";
"pattern"; "error-pread-rate=1";
> | +                           sprintf "size=%d" disk_size];
> |    NBD.connect_command dst ["nbdkit"; "-s";
"--exit-with-parent";
> |                             "memory"; sprintf
"size=%d" disk_size];
> |    asynch_copy src dst
> ---
>  ocaml/tests/test_590_aio_copy.ml | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/ocaml/tests/test_590_aio_copy.ml
b/ocaml/tests/test_590_aio_copy.ml
> index 11d89256..a0339c8b 100644
> --- a/ocaml/tests/test_590_aio_copy.ml
> +++ b/ocaml/tests/test_590_aio_copy.ml
> @@ -1,6 +1,6 @@
>  (* hey emacs, this is OCaml code: -*- tuareg -*- *)
>  (* libnbd OCaml test case
> - * Copyright (C) 2013-2019 Red Hat Inc.
> + * Copyright (C) 2013-2022 Red Hat Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -45,7 +45,8 @@ let
>     * next iteration of the loop.
>     *)
>    let writes = ref [] in
> -  let read_completed buf offset _ > +  let read_completed buf offset
err > +    assert (!err = 0);
>      bytes_read := !bytes_read + NBD.Buffer.size buf;
>      (* Get ready to issue a write command. *)
>      writes := (buf, offset) :: !writes;
> @@ -56,7 +57,8 @@ let
>    (* This callback is called when any pwrite to the destination
>     * has completed.
>     *)
> -  let write_completed buf _ > +  let write_completed buf err > +   
assert (!err = 0);
>      bytes_written := !bytes_written + NBD.Buffer.size buf;
>      (* By returning 1 here we auto-retire the pwrite command. *)
>      1
> 
My (older) OCaml book calls "assert" an "operator"; whereas
Real World
OCaml calls "assert" a "directive":
<https://dev.realworldocaml.org/error-handling.html#exceptions>. Either
way, AIUI it cannot be compiled out, and if the assertin fails,
Assert_failure is raised, which (= throwing an exception) is the proper
way for an OCaml-language completion callback to report an error, IIUC
Rich's explanation.

Reviewed-by: Laszlo Ersek <lersek at redhat.com>

On Fri, Feb 04, 2022 at 09:41:33AM +0100, Laszlo Ersek
wrote:
> On 02/03/22 21:25, Eric Blake wrote:
> > Like a lot of the C examples, the aio copy test ignores read and write
> > errors in the completion callback, which can cause silent data
> > corruption. The failure in the test is not critical, but this is a bad
> > example that may be copied by developers to a real application.
> > 
> > The test dies with an assertion failure now if completion callback
> > fails.  Tested with the temporary patch of:
> > 
> > | diff --git i/ocaml/tests/test_590_aio_copy.ml
w/ocaml/tests/test_590_aio_copy.ml
> > | index a0339c8b..332bf31b 100644
> > | --- i/ocaml/tests/test_590_aio_copy.ml
> > | +++ w/ocaml/tests/test_590_aio_copy.ml
> > | @@ -120,7 +120,8 @@ let
> > |    let dst = NBD.create () in
> > |    NBD.set_handle_name dst "dst";
> > |    NBD.connect_command src ["nbdkit"; "-s";
"--exit-with-parent"; "-r";
> > | -                           "pattern"; sprintf
"size=%d" disk_size];
> > | +                           "--filter=error";
"pattern"; "error-pread-rate=1";
> > | +                           sprintf "size=%d" disk_size];
> > |    NBD.connect_command dst ["nbdkit"; "-s";
"--exit-with-parent";
> > |                             "memory"; sprintf
"size=%d" disk_size];
> > |    asynch_copy src dst
> > ---
> >  ocaml/tests/test_590_aio_copy.ml | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/ocaml/tests/test_590_aio_copy.ml
b/ocaml/tests/test_590_aio_copy.ml
> > index 11d89256..a0339c8b 100644
> > --- a/ocaml/tests/test_590_aio_copy.ml
> > +++ b/ocaml/tests/test_590_aio_copy.ml
> > @@ -1,6 +1,6 @@
> >  (* hey emacs, this is OCaml code: -*- tuareg -*- *)
> >  (* libnbd OCaml test case
> > - * Copyright (C) 2013-2019 Red Hat Inc.
> > + * Copyright (C) 2013-2022 Red Hat Inc.
> >   *
> >   * This library is free software; you can redistribute it and/or
> >   * modify it under the terms of the GNU Lesser General Public
> > @@ -45,7 +45,8 @@ let
> >     * next iteration of the loop.
> >     *)
> >    let writes = ref [] in
> > -  let read_completed buf offset _ > > +  let read_completed buf
offset err > > +    assert (!err = 0);
> >      bytes_read := !bytes_read + NBD.Buffer.size buf;
> >      (* Get ready to issue a write command. *)
> >      writes := (buf, offset) :: !writes;
> > @@ -56,7 +57,8 @@ let
> >    (* This callback is called when any pwrite to the destination
> >     * has completed.
> >     *)
> > -  let write_completed buf _ > > +  let write_completed buf err
> > +    assert (!err = 0);
> >      bytes_written := !bytes_written + NBD.Buffer.size buf;
> >      (* By returning 1 here we auto-retire the pwrite command. *)
> >      1
> > 
> 
> My (older) OCaml book calls "assert" an "operator";
whereas Real World
> OCaml calls "assert" a "directive":
> <https://dev.realworldocaml.org/error-handling.html#exceptions>.
Either
> way, AIUI it cannot be compiled out, and if the assertin fails,
> Assert_failure is raised, which (= throwing an exception) is the proper
> way for an OCaml-language completion callback to report an error, IIUC
> Rich's explanation.
Relevant fact: libnbd OCaml wrapper turns Assert_failure in callbacks
into abort:

https://gitlab.com/nbdkit/libnbd/-/blob/b80780e980275a879c13d27aff1449f91f883ce6/ocaml/helpers.c#L144

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html