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.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
src/util/virfirewall.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index 69521e2b46..a64f8b69c7 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -571,7 +571,6 @@ virFirewallCmdIptablesApply(virFirewall *firewall,
VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK);
bool needRollback = false;
g_autoptr(virCommand) cmd = NULL;
- g_autofree char *cmdStr = NULL;
g_autofree char *error = NULL;
size_t i;
int status;
@@ -607,9 +606,6 @@ virFirewallCmdIptablesApply(virFirewall *firewall,
virCommandAddArg(cmd, fwCmd->args[i]);
}
- cmdStr = virCommandToString(cmd, false);
- VIR_INFO("Running firewall command '%s'", NULLSTR(cmdStr));
-
virCommandSetOutputBuffer(cmd, output);
virCommandSetErrorBuffer(cmd, &error);
@@ -622,6 +618,7 @@ virFirewallCmdIptablesApply(virFirewall *firewall,
VIR_DEBUG("Ignoring error running command");
return 0;
} else {
+ g_autofree char *cmdStr = virCommandToString(cmd, false);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to run firewall command %1$s: %2$s"),
NULLSTR(cmdStr), NULLSTR(error));
@@ -671,7 +668,6 @@ virFirewallCmdNftablesApply(virFirewall *firewall G_GNUC_UNUSED,
size_t cmdIdx = 0;
const char *objectType = NULL;
g_autoptr(virCommand) cmd = NULL;
- g_autofree char *cmdStr = NULL;
g_autofree char *error = NULL;
size_t i;
int status;
@@ -727,9 +723,6 @@ virFirewallCmdNftablesApply(virFirewall *firewall G_GNUC_UNUSED,
for (i = 0; i < fwCmd->argsLen; i++)
virCommandAddArg(cmd, fwCmd->args[i]);
- cmdStr = virCommandToString(cmd, false);
- VIR_INFO("Applying '%s'", NULLSTR(cmdStr));
-
virCommandSetOutputBuffer(cmd, output);
virCommandSetErrorBuffer(cmd, &error);
@@ -745,6 +738,7 @@ virFirewallCmdNftablesApply(virFirewall *firewall G_GNUC_UNUSED,
} else if (fwCmd->ignoreErrors) {
VIR_DEBUG("Ignoring error running command");
} else {
+ g_autofree char *cmdStr = virCommandToString(cmd, false);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to apply firewall command '%1$s': %2$s"),
NULLSTR(cmdStr), NULLSTR(error));
@@ -776,6 +770,7 @@ virFirewallCmdNftablesApply(virFirewall *firewall G_GNUC_UNUSED,
}
if (!handleLen) {
+ g_autofree char *cmdStr = virCommandToString(cmd, false);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("couldn't register rollback command - command '%1$s' had no valid handle in output ('%2$s')"),
NULLSTR(cmdStr), NULLSTR(*output));
--
2.53.0
On 2/11/26 21:20, 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. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > src/util/virfirewall.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
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.
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.
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 :|
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>
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.
Yes, VIR_INFO was not really justifiable IMHO.
> >
> > 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.
Yeah, the only purpose of virCommandToString(Buf) is for debugging,
not the functional execution path. So I don't think we should be
returning early on has_error.
Also the main reason a command argv construction would have triggered
an error historically was OOM, but we abort on OOM these days. So if
the command was constructed successfully without abort, then the call
to virCommandToString should always have a valid string it can return.
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 :|
On Thu, Feb 12, 2026 at 14:51:08 +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>
[...]
> > > @@ -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.
>
> Yeah, the only purpose of virCommandToString(Buf) is for debugging,
> not the functional execution path. So I don't think we should be
> returning early on has_error.
>
> Also the main reason a command argv construction would have triggered
> an error historically was OOM, but we abort on OOM these days. So if
> the command was constructed successfully without abort, then the call
> to virCommandToString should always have a valid string it can return.
Yeah, memory allocation errors are no longer an issue. I don't think we
also care that virCommandToString() propagates failures from wrong usage
of the virCommand object (even when the comment in the function claims
so) so removing that check altoghether should be fine.
© 2016 - 2026 Red Hat, Inc.