[PATCH 0/2] Prohibit newlines in VIR_WARN strings

Peter Krempa via Devel posted 2 patches 3 days, 16 hours ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/cover.1778516505.git.pkrempa@redhat.com
build-aux/syntax-check.mk               |  8 +++++
src/bhyve/bhyve_command.c               |  4 +--
src/conf/virdomainjob.c                 |  5 +--
src/cpu/cpu_x86.c                       |  5 ++-
src/esx/esx_vi.c                        |  3 +-
src/hypervisor/virhostdev.c             | 12 +++----
src/interface/interface_backend_netcf.c | 12 +++----
src/libxl/libxl_conf.c                  |  4 +--
src/libxl/xen_xl.c                      |  4 +--
src/lxc/lxc_driver.c                    |  3 +-
src/lxc/lxc_process.c                   |  3 +-
src/network/bridge_driver.c             | 15 +++------
src/nwfilter/nwfilter_dhcpsnoop.c       |  3 +-
src/qemu/qemu_block.c                   |  3 +-
src/qemu/qemu_command.c                 |  3 +-
src/qemu/qemu_conf.c                    |  4 +--
src/qemu/qemu_domain.c                  | 44 +++++++++----------------
src/qemu/qemu_domain_address.c          | 10 ++----
src/qemu/qemu_driver.c                  | 10 +++---
src/qemu/qemu_firmware.c                |  6 ++--
src/qemu/qemu_hotplug.c                 |  3 +-
src/qemu/qemu_migration.c               | 11 +++----
src/qemu/qemu_monitor_json.c            |  4 +--
src/remote/remote_driver.c              |  3 +-
src/rpc/virnetserverclient.c            |  5 ++-
src/security/security_dac.c             |  3 +-
src/security/security_selinux.c         |  7 ++--
src/security/security_stack.c           | 34 +++++++------------
src/storage/storage_backend_rbd.c       |  4 +--
src/storage/storage_backend_zfs.c       |  3 +-
src/storage/storage_util.c              |  4 +--
src/storage_file/storage_file_probe.c   |  3 +-
src/vbox/vbox_common.c                  |  4 +--
src/vmx/vmx.c                           |  6 ++--
src/vz/vz_sdk.c                         |  3 +-
35 files changed, 96 insertions(+), 162 deletions(-)
[PATCH 0/2] Prohibit newlines in VIR_WARN strings
Posted by Peter Krempa via Devel 3 days, 16 hours ago
Make them the same as we do with error messages since they often end up
in logs.

Since they are not translated [1] the existing check didn't catch that.


[1] IMO in some cases they are in fact used as errors, and maybe would
be worth to be translated. But this is for another series


Peter Krempa (2):
  Don't break up strings for VIR_WARN messages
  syntax-check: Enforce no linebreaks in VIR_WARN messages

 build-aux/syntax-check.mk               |  8 +++++
 src/bhyve/bhyve_command.c               |  4 +--
 src/conf/virdomainjob.c                 |  5 +--
 src/cpu/cpu_x86.c                       |  5 ++-
 src/esx/esx_vi.c                        |  3 +-
 src/hypervisor/virhostdev.c             | 12 +++----
 src/interface/interface_backend_netcf.c | 12 +++----
 src/libxl/libxl_conf.c                  |  4 +--
 src/libxl/xen_xl.c                      |  4 +--
 src/lxc/lxc_driver.c                    |  3 +-
 src/lxc/lxc_process.c                   |  3 +-
 src/network/bridge_driver.c             | 15 +++------
 src/nwfilter/nwfilter_dhcpsnoop.c       |  3 +-
 src/qemu/qemu_block.c                   |  3 +-
 src/qemu/qemu_command.c                 |  3 +-
 src/qemu/qemu_conf.c                    |  4 +--
 src/qemu/qemu_domain.c                  | 44 +++++++++----------------
 src/qemu/qemu_domain_address.c          | 10 ++----
 src/qemu/qemu_driver.c                  | 10 +++---
 src/qemu/qemu_firmware.c                |  6 ++--
 src/qemu/qemu_hotplug.c                 |  3 +-
 src/qemu/qemu_migration.c               | 11 +++----
 src/qemu/qemu_monitor_json.c            |  4 +--
 src/remote/remote_driver.c              |  3 +-
 src/rpc/virnetserverclient.c            |  5 ++-
 src/security/security_dac.c             |  3 +-
 src/security/security_selinux.c         |  7 ++--
 src/security/security_stack.c           | 34 +++++++------------
 src/storage/storage_backend_rbd.c       |  4 +--
 src/storage/storage_backend_zfs.c       |  3 +-
 src/storage/storage_util.c              |  4 +--
 src/storage_file/storage_file_probe.c   |  3 +-
 src/vbox/vbox_common.c                  |  4 +--
 src/vmx/vmx.c                           |  6 ++--
 src/vz/vz_sdk.c                         |  3 +-
 35 files changed, 96 insertions(+), 162 deletions(-)

-- 
2.54.0
Re: [PATCH 0/2] Prohibit newlines in VIR_WARN strings
Posted by Laine Stump via Devel 3 days, 11 hours ago
(I prefer the way you describe it in the actual commit message - "don't 
break up lines in the source" - vs the subject line here - "prohibit 
newlines" - since it's more accurate. Fortunately the actual commit 
message is used as .... the actual commit message, so everything is 
cool! :-))


On 5/11/26 12:23 PM, Peter Krempa via Devel wrote:
> Make them the same as we do with error messages since they often end up
> in logs.
> 
> Since they are not translated [1] the existing check didn't catch that.
> 
> 
> [1] IMO in some cases they are in fact used as errors, and maybe would
> be worth to be translated. But this is for another series

There have been a few times I've wished we were allowed to at least 
*optionally* mark VIR_WARN strings to be translated, for cases where the 
situation is  something like:

"This is *probably* going to lead to an error, and we've just seen 
evidence of it in advance of the place where it's going to actually blow 
up. Right now while we have this context we can tell the user how to fix 
it (and it would be nice to tell them in a language they understand), 
but it would be awkward and wasteful to try and regather that context 
later at the time we actually fail. But also it might *not* be an error, 
so we don't want to just fail right away in case we wouldn't have failed 
later."

or something like that. For example (a recent one), we determine that a 
particular default path for a directory for something isn't accessible 
by the uid of the process running libvirt, but it turns out that in some 
situations that path is never actually used, and so adding an actual 
error when we decide on the path for config/logging/status would cause 
some working setups to "mysteriously" start failing. (On the other hand, 
it's awkward to later regather the context that the reason for the 
open/create failure was because "the log path is inaccessible by this 
process" (the fact that this was the source of the file path is usually 
a few layers up the call stack from where the error is logged).

So yeah, that issue is completely unrelated to your patch, but I thought 
I'd chime in while the topic was brought up.


Reviewed-by: Laine Stump <laine@redhat.com>

for the series.

> 
> Peter Krempa (2):
>    Don't break up strings for VIR_WARN messages
>    syntax-check: Enforce no linebreaks in VIR_WARN messages
> 
>   build-aux/syntax-check.mk               |  8 +++++
>   src/bhyve/bhyve_command.c               |  4 +--
>   src/conf/virdomainjob.c                 |  5 +--
>   src/cpu/cpu_x86.c                       |  5 ++-
>   src/esx/esx_vi.c                        |  3 +-
>   src/hypervisor/virhostdev.c             | 12 +++----
>   src/interface/interface_backend_netcf.c | 12 +++----
>   src/libxl/libxl_conf.c                  |  4 +--
>   src/libxl/xen_xl.c                      |  4 +--
>   src/lxc/lxc_driver.c                    |  3 +-
>   src/lxc/lxc_process.c                   |  3 +-
>   src/network/bridge_driver.c             | 15 +++------
>   src/nwfilter/nwfilter_dhcpsnoop.c       |  3 +-
>   src/qemu/qemu_block.c                   |  3 +-
>   src/qemu/qemu_command.c                 |  3 +-
>   src/qemu/qemu_conf.c                    |  4 +--
>   src/qemu/qemu_domain.c                  | 44 +++++++++----------------
>   src/qemu/qemu_domain_address.c          | 10 ++----
>   src/qemu/qemu_driver.c                  | 10 +++---
>   src/qemu/qemu_firmware.c                |  6 ++--
>   src/qemu/qemu_hotplug.c                 |  3 +-
>   src/qemu/qemu_migration.c               | 11 +++----
>   src/qemu/qemu_monitor_json.c            |  4 +--
>   src/remote/remote_driver.c              |  3 +-
>   src/rpc/virnetserverclient.c            |  5 ++-
>   src/security/security_dac.c             |  3 +-
>   src/security/security_selinux.c         |  7 ++--
>   src/security/security_stack.c           | 34 +++++++------------
>   src/storage/storage_backend_rbd.c       |  4 +--
>   src/storage/storage_backend_zfs.c       |  3 +-
>   src/storage/storage_util.c              |  4 +--
>   src/storage_file/storage_file_probe.c   |  3 +-
>   src/vbox/vbox_common.c                  |  4 +--
>   src/vmx/vmx.c                           |  6 ++--
>   src/vz/vz_sdk.c                         |  3 +-
>   35 files changed, 96 insertions(+), 162 deletions(-)
>
Re: [PATCH 0/2] Prohibit newlines in VIR_WARN strings
Posted by Peter Krempa via Devel 3 days, 1 hour ago
On Mon, May 11, 2026 at 17:32:44 -0400, Laine Stump wrote:
> (I prefer the way you describe it in the actual commit message - "don't
> break up lines in the source" - vs the subject line here - "prohibit
> newlines" - since it's more accurate. Fortunately the actual commit message
> is used as .... the actual commit message, so everything is cool! :-))
> 
> 
> On 5/11/26 12:23 PM, Peter Krempa via Devel wrote:
> > Make them the same as we do with error messages since they often end up
> > in logs.
> > 
> > Since they are not translated [1] the existing check didn't catch that.
> > 
> > 
> > [1] IMO in some cases they are in fact used as errors, and maybe would
> > be worth to be translated. But this is for another series
> 
> There have been a few times I've wished we were allowed to at least
> *optionally* mark VIR_WARN strings to be translated, for cases where the
> situation is  something like:

Yeah, at least some of the warnings we emit would benefit from a
translation. Especially since they are logged by default.

> "This is *probably* going to lead to an error, and we've just seen evidence
> of it in advance of the place where it's going to actually blow up. Right
> now while we have this context we can tell the user how to fix it (and it
> would be nice to tell them in a language they understand), but it would be
> awkward and wasteful to try and regather that context later at the time we
> actually fail. But also it might *not* be an error, so we don't want to just
> fail right away in case we wouldn't have failed later."
> 
> or something like that. For example (a recent one), we determine that a
> particular default path for a directory for something isn't accessible by
> the uid of the process running libvirt, but it turns out that in some
> situations that path is never actually used, and so adding an actual error
> when we decide on the path for config/logging/status would cause some
> working setups to "mysteriously" start failing. (On the other hand, it's
> awkward to later regather the context that the reason for the open/create
> failure was because "the log path is inaccessible by this process" (the fact
> that this was the source of the file path is usually a few layers up the
> call stack from where the error is logged).
> 
> So yeah, that issue is completely unrelated to your patch, but I thought I'd
> chime in while the topic was brought up.

Well, this is a first step :). Maybe Jirka still has his script to add
positional substitutions which he used when adding them to
virReportError, so we could also do the second step too :)



> 
> 
> Reviewed-by: Laine Stump <laine@redhat.com>

Thanks!