[Libguestfs] [libnbd PATCH v2 2/5] ocaml: tests: Fix error handling
Laszlo Ersek
lersek at redhat.com
Fri Feb 4 10:09:18 UTC 2022
On 02/04/22 09:48, Richard W.M. Jones wrote:
> 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
Haha, nice! :)
Laszlo
More information about the Libguestfs
mailing list