On Fri, Apr 09, 2021 at 02:50:09PM +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 virCommandDryRunToken type which must be allocated via
> virCommandDryRunTokenNew and 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 the token isn't left unused in the code.
>
> Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
> ---
> src/libvirt_private.syms | 2 ++
> src/util/vircommand.c | 44 +++++++++++++++++++++++++++++++-
> src/util/vircommandpriv.h | 9 ++++++-
> tests/networkxml2firewalltest.c | 4 +--
> tests/nodedevmdevctltest.c | 12 ++++-----
> 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, 111 insertions(+), 60 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 7dd3a1ee10..75340f85ae 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1989,6 +1989,8 @@ virCommandAllowCap;
> virCommandClearCaps;
> virCommandDaemonize;
> virCommandDoAsyncIO;
> +virCommandDryRunTokenFree;
> +virCommandDryRunTokenNew;
> virCommandExec;
> virCommandFree;
> virCommandGetArgList;
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index 8ae5badf0f..e816995636 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -3099,8 +3099,45 @@ virCommandDoAsyncIO(virCommandPtr cmd)
> cmd->flags |= VIR_EXEC_ASYNC_IO | VIR_EXEC_NONBLOCK;
> }
>
> +
> +struct _virCommandDryRunToken {
> + int dummy;
> +};
I wanted to complain that having normal struct containing the
dryRunBuffer, dryRunCallback, dryRunOpaque would be better but looking
at this file they are global variables used in other functions as well
so it would be pain to refactor it and definitely not in scope of this
patch or patch series.
> +
> +
> +/**
> + * virCommandDryRunTokenNew:
> + *
> + * Returns a token which is used with virCommandSetDryRun. Freeing the token
> + * with the appropriate automatic cleanup function ensures that the dry run
> + * environment is reset.
> + */
> +virCommandDryRunToken *
> +virCommandDryRunTokenNew(void)
> +{
> + return g_new0(virCommandDryRunToken, 1);
> +}
> +
> +
> +/**
> + * virCommandDryRunTokenFree:
> + *
> + * Helper to free a virCommandDryRunToken. Do not use this function directly,
> + * always declare virCommandDryRunToken as a g_autoptr.
> + */
> +void
> +virCommandDryRunTokenFree(virCommandDryRunToken *tok)
> +{
> + dryRunBuffer = NULL;
> + dryRunCallback = NULL;
> + dryRunOpaque = NULL;
> + g_free(tok);
> +}
> +
> +
> /**
> * virCommandSetDryRun:
> + * @tok: a virCommandDryRunToken obtained from virCommandDryRunTokenNew
> * @buf: buffer to store stringified commands
> * @callback: callback to process input/output/args
> *
> @@ -3132,15 +3169,20 @@ virCommandDoAsyncIO(virCommandPtr cmd)
> * To cancel this effect pass NULL for @buf and @callback.
> */
> void
> -virCommandSetDryRun(virBufferPtr buf,
> +virCommandSetDryRun(virCommandDryRunToken *tok,
> + virBufferPtr buf,
> virCommandDryRunCallback cb,
> void *opaque)
> {
> + if (!tok)
> + abort();
> +
> dryRunBuffer = buf;
> dryRunCallback = cb;
> dryRunOpaque = opaque;
> }
>
> +
Unrelated new line addition.
> #ifndef WIN32
> /**
> * virCommandRunRegex:
Pavel
Attachment:
signature.asc
Description: PGP signature
