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