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é <[email protected]> > > > > > > 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é <[email protected]> > > > --- > > > 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 <[email protected]>
