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.
Pavel
Attachment:
signature.asc
Description: PGP signature
