Eric Blake
2022-Feb-03 20:25 UTC
[Libguestfs] [libnbd PATCH v2 2/5] ocaml: tests: Fix error handling
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
--
2.34.1
Laszlo Ersek
2022-Feb-04 08:41 UTC
[Libguestfs] [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>