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.