On Wed, Aug 24, 2022 at 02:49:27PM +0200, Laszlo Ersek wrote:
On 08/24/22 04:53, Eric Blake wrote:
> Add testsuite coverage that exposes the flaw fixed in the previous patch.
>
> ---
> tests/opt-info.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/tests/opt-info.c b/tests/opt-info.c
> index b9739a5..2402a31 100644
> --- a/tests/opt-info.c
> +++ b/tests/opt-info.c
> @@ -1,5 +1,5 @@
> /* NBD client library in userspace
> - * Copyright (C) 2013-2020 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
> @@ -80,15 +80,11 @@ main (int argc, char *argv[])
> exit (EXIT_FAILURE);
> }
>
> - /* info on something not present fails, wipes out prior info */
> + /* changing export wipes out prior info */
> if (nbd_set_export_name (nbd, "a") == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> }
> - if (nbd_opt_info (nbd) != -1) {
> - fprintf (stderr, "expecting error for opt_info\n");
> - exit (EXIT_FAILURE);
> - }
> if (nbd_get_size (nbd) != -1) {
> fprintf (stderr, "expecting error for get_size\n");
> exit (EXIT_FAILURE);
Is this well targeted though?
Here we can't say whether nbd_get_size() fails because export "a" is
different from the previous "" export, or because "a" does not exist.
If
"a" existed and nbd_get_size() still failed, that would be more to the
point.
Good observation. I'll set name to "b" (which does exist, and whereas
export "" has size 0, export "b" has size 1, so it should be very
obvious that returning 0 is wrong, returning 1 is only possible if we
queried the server about the new export which hasn't happened without
an nbd_opt_ call, and therefore -1 is the only sane answer)...
> @@ -102,7 +98,13 @@ main (int argc, char *argv[])
> exit (EXIT_FAILURE);
> }
>
> - /* info for a different export */
> + /* info on something not present fails */
> + if (nbd_opt_info (nbd) != -1) {
> + fprintf (stderr, "expecting error for opt_info\n");
> + exit (EXIT_FAILURE);
> + }
> +
Yes, this makes sense, assuming the previous point is "good enough".
and then add one more nbd_set_export_name("a") before the
expected-failing nbd_opt_info().
> + /* info for a different export; idempotent name change is no-op */
> if (nbd_set_export_name (nbd, "b") == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> @@ -111,6 +113,10 @@ main (int argc, char *argv[])
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> }
> + if (nbd_set_export_name (nbd, "b") == -1) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> if ((r = nbd_get_size (nbd)) != 1) {
> fprintf (stderr, "expecting size of 1, got %" PRId64 "\n",
r);
> exit (EXIT_FAILURE);
>
This looks OK to me.
I need to repost it anyway; the Python, Go, and OCaml bindings all
have the same test (under the name *230*), and can similarly be
changed for multi-lingual coverage of the idiom (if nothing else, to
prove we aren't maintaining any separate language-specific state).
v2 coming up later today, with more patches (the promised
nbd_opt_set_meta_context() among them).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization:
qemu.org |
libvirt.org