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@xxxxxxxxxx>
> > ---
> > 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.
