On Thu, Feb 12, 2026 at 16:51:58 +0000, Daniel P. Berrangé wrote:
On Thu, Feb 12, 2026 at 03:39:45PM +0100, Peter Krempa wrote:
On Wed, Feb 11, 2026 at 20:20:40 +0000, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
The vircommand.c code will always log the argv about to be run, so logging it again in virfirewall.c is redundant. Removing the dupe avoids the repeated memory allocation from the array -> string conversion.
The virCommand infra does that with VIR_DEBUG, while this was doing it with VIR_INFO priority. I'm okay with removing these but IMO should be mentioned in the commit message.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virfirewall.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
[...]
@@ -622,6 +618,7 @@ virFirewallCmdIptablesApply(virFirewall *firewall, VIR_DEBUG("Ignoring error running command"); return 0; } else { + g_autofree char *cmdStr = virCommandToString(cmd, false);
Many operations on virCommand, including the whole call to virCommandRunAsync() from within virCommandRun assert the 'has_error' flag which in turn will prevent virCommandToString() returning the command at this point.
Since virCommandRunAsync() captures errors from everything including virExec this might actually make the errors useless.
virCommandToStringBuf could theoretically be made to skip the check with an extra flag to avoid this behaviour so that we could be able to get the command after trying to run it.
Looking at this again, I don't think there is actually any problem to solve here. It isn't visible from the patch context, but the code here does
if (virCommandRun(cmd, &status) < 0) return -1;
if (status != 0) { .... g_autofree char *cmdStr = virCommandToString(cmd, false); ... }
IOW, the only scenario in which we call virCommandToString is when the virCommandRun was successful.
The failure scenario here is that the command was spawned successfully but then exited with non-zero status. virCommandToString will always return a valid string in this case.
Ah!, in such case just mention that the patch is downgrading VIR_INFO->VIR_DEBUG and Reviewed-by: Peter Krempa <pkrempa@redhat.com>