[Libguestfs] [libnbd PATCH 2/2] api: Test previous patch

[Eric Blake]

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