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

[Pavel Hrdina]

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.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210407/76ed348e/attachment-0001.sig>