Re: [Libguestfs] [libnbd PATCH v2 2/5] ocaml: tests: Fix error handling

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/b80780e980275a879c13d27aff1449f91...