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.
With regards,
Daniel
--
|: https://berrange.com ~~ https://hachyderm.io/@berrange :|
|: https://libvirt.org ~~ https://entangle-photo.org :|
|: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
