[PATCH 07/24] virCommandSetDryRun: Rework resetting of the dry run data

[Peter Krempa]

On Wed, Apr 07, 2021 at 20:06:30 +0200, Pavel Hrdina wrote:
> On Wed, Apr 07, 2021 at 05:09:30PM +0200, Peter Krempa wrote:
> > While virCommandSetDryRun is used in tests only, there were some cases
> > when error paths would not call the function with NULL arguments to
> > reset the dry run infrastructure.
> > 
> > Introduce VIR_COMMAND_DRY_RUN_TOKEN macro which declares a variable
> > called 'dryRunToken' which must be passed to virCommandSetDryRun.
> > 
> > This way we can use automatic variable cleaning to trigger the cleanup
> > of virCommandSetDryRun parameters and also the use of the token variable
> > ensures that all callers of virCommandSetDryRun clean up after
> > themselves and also that VIR_COMMAND_DRY_RUN_TOKEN isn't left unused in
> > the code.
> > 
> > Signed-off-by: Peter Krempa <pkrempa at redhat.com>
> > ---
> >  src/libvirt_private.syms         |  1 +
> >  src/util/vircommand.c            | 13 ++++++++++-
> >  src/util/vircommandpriv.h        | 10 +++++++-
> >  tests/networkxml2firewalltest.c  |  4 ++--
> >  tests/nodedevmdevctltest.c       |  8 +++----
> >  tests/nwfilterebiptablestest.c   | 28 +++++++++++-----------
> >  tests/nwfilterxml2firewalltest.c |  4 ++--
> >  tests/sysinfotest.c              |  4 ++--
> >  tests/virfirewalltest.c          | 40 ++++++++++++++++----------------
> >  tests/viriscsitest.c             | 12 +++++-----
> >  tests/virkmodtest.c              |  8 +++----
> >  tests/virnetdevbandwidthtest.c   |  4 ++--
> >  12 files changed, 78 insertions(+), 58 deletions(-)
> > 
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index caa0fd18f8..e32088badb 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -1983,6 +1983,7 @@ virCommandAllowCap;
> >  virCommandClearCaps;
> >  virCommandDaemonize;
> >  virCommandDoAsyncIO;
> > +virCommandDryRunResetHelper;
> >  virCommandExec;
> >  virCommandFree;
> >  virCommandGetArgList;
> > diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> > index 8ae5badf0f..7b2588f5c8 100644
> > --- a/src/util/vircommand.c
> > +++ b/src/util/vircommand.c
> > @@ -3132,7 +3132,8 @@ virCommandDoAsyncIO(virCommandPtr cmd)
> >   * To cancel this effect pass NULL for @buf and @callback.
> >   */
> >  void
> > -virCommandSetDryRun(virBufferPtr buf,
> > +virCommandSetDryRun(virCommandDryRunToken tok G_GNUC_UNUSED,
> > +                    virBufferPtr buf,
> >                      virCommandDryRunCallback cb,
> >                      void *opaque)
> >  {
> > @@ -3141,6 +3142,16 @@ virCommandSetDryRun(virBufferPtr buf,
> >      dryRunOpaque = opaque;
> >  }
> > 
> > +
> > +void
> > +virCommandDryRunResetHelper(virCommandDryRunToken *tok G_GNUC_UNUSED)
> > +{
> > +    dryRunBuffer = NULL;
> > +    dryRunCallback = NULL;
> > +    dryRunOpaque = NULL;
> > +}
> > +
> > +
> >  #ifndef WIN32
> >  /**
> >   * virCommandRunRegex:
> > diff --git a/src/util/vircommandpriv.h b/src/util/vircommandpriv.h
> > index 80f1d1376c..8a47a6d5e3 100644
> > --- a/src/util/vircommandpriv.h
> > +++ b/src/util/vircommandpriv.h
> > @@ -35,6 +35,14 @@ typedef void (*virCommandDryRunCallback)(const char *const*args,
> >                                           int *status,
> >                                           void *opaque);
> > 
> > -void virCommandSetDryRun(virBufferPtr buf,
> > +typedef int virCommandDryRunToken;
> > +
> > +void virCommandSetDryRun(virCommandDryRunToken tok,
> > +                         virBufferPtr buf,
> >                           virCommandDryRunCallback cb,
> >                           void *opaque);
> > +
> > +void
> > +virCommandDryRunResetHelper(virCommandDryRunToken *tok);
> > +
> > +#define VIR_COMMAND_DRY_RUN_TOKEN __attribute__((cleanup(virCommandDryRunResetHelper))) virCommandDryRunToken dryRunToken = 0
> 
> How about using this instead:
> 
> G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(virCommandDryRunToken, virCommandDryRunResetHelper);
> 
> > diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c
> > index d358f12897..574b553fa6 100644
> > --- a/tests/networkxml2firewalltest.c
> > +++ b/tests/networkxml2firewalltest.c
> > @@ -96,8 +96,9 @@ static int testCompareXMLToArgvFiles(const char *xml,
> >      virNetworkDefPtr def = NULL;
> >      int ret = -1;
> >      char *actual;
> > +    VIR_COMMAND_DRY_RUN_TOKEN;
> 
> I don't like this at all, specifically the fact that it hides
> declaration of dryRunToken variable.
> 
> Here we could use this:
> 
>     g_auto(virCommandDryRunToken) dryRunToken = 0;
> 
> 
> This would be probably acceptable but I would still prefer if we could
> create a new struct, for example:
> 
> struct _virCommandDryData {
>     virBufferPtr buf;
>     virCommandDryRunCallback callback;
>     void *opaque;
> };
> 
> That way the struct would be used so clang will not complain about
> unused variable and there is no need for the dummy dryRunToken.

I didn't like the VIR_COMMAND_DRY_RUN_TOKEN either but I think the
semantics of usage with the token are good.

IMO a better solution which we could also possibly use instead of the
VIR_XPATH_CTXT_AUTORESTORE macro would be to create a function which
allocates the token (thus shifting responsibility of deallocating it to
the caller/programmer) and provide a autoptr cleanup function for it to
preserve the semantics we have.

While this adds an allocation, the declaration of the variable will be
cleaner.

P.S.: looks like this series had a few conflicts recently with test
changes so if you don't plan on continuing the review right away I'll be
posting another fixed version soon.