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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
