Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling

[Peter Xu]

On Fri, May 17, 2024 at 03:01:59PM +0200, Yu Zhang wrote:
> Hello Michael and Peter,

Hi,

> 
> Exactly, not so compelling, as I did it first only on servers widely
> used for production in our data center. The network adapters are
> 
> Ethernet controller: Broadcom Inc. and subsidiaries NetXtreme BCM5720
> 2-port Gigabit Ethernet PCIe

Hmm... I definitely thinks Jinpu's Mellanox ConnectX-6 looks more
reasonable.

https://lore.kernel.org/qemu-devel/camgffen-dkpmz4ta71mjydyemg0zda15wvaqk81vxtkzx-l...@mail.gmail.com/

Appreciate a lot for everyone helping on the testings.

> InfiniBand controller: Mellanox Technologies MT27800 Family [ConnectX-5]
> 
> which doesn't meet our purpose. I can choose RDMA or TCP for VM
> migration. RDMA traffic is through InfiniBand and TCP through Ethernet
> on these two hosts. One is standby while the other is active.
> 
> Now I'll try on a server with more recent Ethernet and InfiniBand
> network adapters. One of them has:
> BCM57414 NetXtreme-E 10Gb/25Gb RDMA Ethernet Controller (rev 01)
> 
> The comparison between RDMA and TCP on the same NIC could make more sense.

It looks to me NICs are powerful now, but again as I mentioned I don't
think it's a reason we need to deprecate rdma, especially if QEMU's rdma
migration has the chance to be refactored using rsocket.

Is there anyone who started looking into that direction?  Would it make
sense we start some PoC now?

Thanks,

-- 
Peter Xu

Re: [PATCH v5 24/30] network: add an nftables backend for network driver's firewall construction

[Laine Stump]

On 5/17/24 1:30 PM, Laine Stump wrote:

+virFirewallAddCmd(fw, layer, "insert", "rule",
+  nftablesLayerTypeToString(layer),
+  VIR_NFTABLES_PRIVATE_TABLE,
+  VIR_NFTABLES_FWD_X_CHAIN,
+  "iifname", iface,
+  "oifname", iface,
+  "counter", "accept",
+  NULL);


I just found out that if we use "iif" and "oif" rather than "iifname" 
and "oifname", the runtime operation will be a direct comparison if the 
ifindex (stored with the packet data) rather than a string comparison 
with the interface name (which needs to be separately retrieved).


The only potential downside would be in the case that an interface was 
deleted and then re-added - it would have a new ifindex, and the ifindex 
in the rule is determined when the rule is installed, so the new ifindex 
would never match. In our case this would never happen though, since all 
the rules for a network are recreating (possibly multiple times just 
after the bridge device is created, then deleted when the bridge device 
is deleted.


I've tried this, and everything works fine. Should I squash that change 
into this patch, or make a separate patch with the change so that this 
patch remains (nearly) identical to what is produced by 
iptables-restore-translate on the old iptables rules?

Re: [RFC PATCH v3 3/6] qemu_interface: Added routine for loading the eBPF objects.

[Andrew Melnichenko]

Hi all,

On Fri, May 17, 2024 at 5:00 PM Michal Prívozník  wrote:
>
> On 5/12/24 21:45, Andrew Melnychenko wrote:
> > Also, added dependencies for libbpf with bpf option.
> >
> > Signed-off-by: Andrew Melnychenko 
> > ---
> >  meson.build   |  7 
> >  meson_options.txt |  1 +
> >  src/qemu/meson.build  |  1 +
> >  src/qemu/qemu_interface.c | 83 +++
> >  src/qemu/qemu_interface.h |  4 ++
> >  5 files changed, 96 insertions(+)
>
>
> libvirt.spec.in should be changed too to either selectively disable this
> feature or enable it and drag in the requirement (preferred).

Yes, I'll "enable it" in the next version.

>
> >
> > diff --git a/meson.build b/meson.build
> > index e8b0094b91..e12a703a4d 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -998,6 +998,12 @@ else
> >libkvm_dep = dependency('', required: false)
> >  endif
> >
> > +libbpf_version = '1.1.0'
> > +libbpf_dep = dependency('libbpf', version: '>=' + libbpf_version, 
> > required: get_option('libbpf'))
> > +if libbpf_dep.found()
> > +conf.set('WITH_BPF', 1)
> > +endif
> > +
> >  libiscsi_version = '1.18.0'
> >  libiscsi_dep = dependency('libiscsi', version: '>=' + libiscsi_version, 
> > required: get_option('libiscsi'))
> >
> > @@ -2283,6 +2289,7 @@ libs_summary = {
> >'dlopen': dlopen_dep.found(),
> >'fuse': fuse_dep.found(),
> >'glusterfs': glusterfs_dep.found(),
> > +  'libbpf': libbpf_dep.found(),
> >'libiscsi': libiscsi_dep.found(),
> >'libkvm': libkvm_dep.found(),
> >'libnbd': libnbd_dep.found(),
> > diff --git a/meson_options.txt b/meson_options.txt
> > index 9d729b3e1f..9b7bd9d1f8 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -48,6 +48,7 @@ option('udev', type: 'feature', value: 'auto', 
> > description: 'udev support')
> >  option('wireshark_dissector', type: 'feature', value: 'auto', description: 
> > 'wireshark support')
> >  option('wireshark_plugindir', type: 'string', value: '', description: 
> > 'wireshark plugins directory for use when installing wireshark plugin')
> >  option('yajl', type: 'feature', value: 'auto', description: 'yajl support')
> > +option('libbpf', type: 'feature', value: 'auto', description: 'qemu libbpf 
> > support')
> >
> >
> >  # build driver options
> > diff --git a/src/qemu/meson.build b/src/qemu/meson.build
> > index 907893d431..de7ae87d5b 100644
> > --- a/src/qemu/meson.build
> > +++ b/src/qemu/meson.build
> > @@ -105,6 +105,7 @@ if conf.has('WITH_QEMU')
> >selinux_dep,
> >src_dep,
> >xdr_dep,
> > +  libbpf_dep,
>
> We tend to keep this kind of lists sorted alphabetically.
>
> >  ],
> >  include_directories: [
> >conf_inc_dir,
> > diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
> > index c2007c7043..ec8cf18d86 100644
> > --- a/src/qemu/qemu_interface.c
> > +++ b/src/qemu/qemu_interface.c
> > @@ -38,6 +38,10 @@
> >  #include 
> >  #include 
> >
> > +#ifdef WITH_BPF
> > +#include 
>
> s/#include/# include/
> if you'd install 'cppi' then a syntax-check rule of ours would have
> warned you about this.
>
> > +#endif
> > +
> >  #define VIR_FROM_THIS VIR_FROM_QEMU
> >
> >  VIR_LOG_INIT("qemu.qemu_interface");
> > @@ -432,3 +436,82 @@ qemuInterfaceOpenVhostNet(virDomainObj *vm,
> >  virDomainAuditNetDevice(vm->def, net, vhostnet_path, vhostfdSize);
> >  return 0;
> >  }
> > +
> > +#ifdef WITH_BPF
> > +
> > +int
> > +qemuInterfaceLoadEbpf(const char *ebpfObject, void **retLibbpfObj, int 
> > *fds, size_t nfds)
> > +{
> > +int err = 0;
> > +size_t i = 0;
> > +struct bpf_program *prog;
> > +struct bpf_map *map;
> > +struct bpf_object *obj;
> > +size_t ebpfSize = 0;
> > +g_autofree void *ebpfRawData = NULL;
> > +
> > +ebpfRawData = g_base64_decode(ebpfObject, );
> > +if (ebpfRawData == NULL) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("can't decode the 
> > eBPF from base64"));
> > +return -1;
> > +}
> > +
> > +obj = bpf_object__open_mem(ebpfRawData, ebpfSize, NULL);
> > +err = libbpf_get_error(obj);
> > +if (err) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("can't open eBPF 
> > object"));
> > +return -1;
> > +}
>
> IIUC, libbpf_get_error() is deprecated and upon failure bpf_object_*()
> APIs return NULL and set errno. Thus this (and the rest) could look
> something like this:
>
>   obj = bpf_object__open_mem(...);
>   if (!obj) {
>   virReportSystemError(errno, "%s", _("can't open eBPF object"));
>   return -1;
>   }
>

Ok. originally I've tied to omit errno and use VIR_ERR* macros.

> > +
> > +
> > +err = bpf_object__load(obj);
> > +if (err) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("can't load eBPF 
> > object"));
> > +return -1;
> > +}
> > +
> > +bpf_object__for_each_program(prog, obj) {
> > +fds[i] = bpf_program__fd(prog);
> > +++i;
> > +

Re: [RFC PATCH v3 4/6] qemu_command: Added "ebpf_rss_fds" support for virtio-net.

[Andrew Melnichenko]

Hi all,

On Fri, May 17, 2024 at 5:00 PM Michal Prívozník  wrote:
>
> On 5/12/24 21:45, Andrew Melnychenko wrote:
> > Added new capability ebpf_rss_fds for QEMU.
> > Added logic for loading the "RSS" eBPF program.
> > eBPF file descriptors passed to the QEMU.
> >
> > Signed-off-by: Andrew Melnychenko 
> > ---
> >  src/qemu/qemu_capabilities.c |  4 +++
> >  src/qemu/qemu_capabilities.h |  3 ++
> >  src/qemu/qemu_command.c  | 61 
> >  src/qemu/qemu_domain.c   |  4 +++
> >  src/qemu/qemu_domain.h   |  3 ++
> >  5 files changed, 75 insertions(+)
> >
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 09bb6ca36e..bb6a587725 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -708,6 +708,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
> >"usb-mtp", /* QEMU_CAPS_DEVICE_USB_MTP */
> >"machine.virt.ras", /* QEMU_CAPS_MACHINE_VIRT_RAS */
> >"virtio-sound", /* QEMU_CAPS_DEVICE_VIRTIO_SOUND */
> > +
> > +  /* 460 */
> > +  "virtio-net.ebpf_rss_fds", /* 
> > QEMU_CAPS_VIRTIO_NET_EBPF_RSS_FDS */
> >  );
> >
> >
> > @@ -1452,6 +1455,7 @@ static struct virQEMUCapsDevicePropsFlags 
> > virQEMUCapsDevicePropsVirtioBlk[] = {
> >  static struct virQEMUCapsDevicePropsFlags 
> > virQEMUCapsDevicePropsVirtioNet[] = {
> >  { "acpi-index", QEMU_CAPS_ACPI_INDEX, NULL },
> >  { "rss", QEMU_CAPS_VIRTIO_NET_RSS, NULL },
> > +{ "ebpf-rss-fds", QEMU_CAPS_VIRTIO_NET_EBPF_RSS_FDS, NULL },
> >  };
> >
> >  static struct virQEMUCapsDevicePropsFlags 
> > virQEMUCapsDevicePropsPCIeRootPort[] = {
> > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> > index 371ea19bd0..e4bb137b21 100644
> > --- a/src/qemu/qemu_capabilities.h
> > +++ b/src/qemu/qemu_capabilities.h
> > @@ -688,6 +688,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
> > syntax-check */
> >  QEMU_CAPS_MACHINE_VIRT_RAS, /* -machine virt,ras= */
> >  QEMU_CAPS_DEVICE_VIRTIO_SOUND, /* -device virtio-sound-* */
> >
> > +/* 460 */
> > +QEMU_CAPS_VIRTIO_NET_EBPF_RSS_FDS, /* virtio-net ebpf_rss_fds feature 
> > */
> > +
> >  QEMU_CAPS_LAST /* this must always be the last item */
> >  } virQEMUCapsFlags;
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 9859ea67a4..77715cf6fe 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -3805,6 +3805,25 @@ qemuBuildLegacyNicStr(virDomainNetDef *net)
> >  }
> >
> >
> > +static virJSONValue *
> > +qemuBuildEbpfRssArg(virDomainNetDef *net)
> > +{
> > +qemuDomainNetworkPrivate *netpriv = QEMU_DOMAIN_NETWORK_PRIVATE(net);
> > +g_autoptr(virJSONValue) props = NULL;
> > +
> > +GSList *n;
> > +
> > +if (netpriv->ebpfrssfds)
> > +props = virJSONValueNewArray();
>
> nitpick: A bit more convoluted than needed be.
>
> if (!netpriv->ebpfrssfds)
>   return NULL;
>
> props = virJSONValueNewArray();
>

I'll refactor it in the next patch.

> > +
> > +for (n = netpriv->ebpfrssfds; n; n = n->next) {
> > +virJSONValueArrayAppendString(props, 
> > qemuFDPassDirectGetPath(n->data));
> > +}
> > +
> > +return g_steal_pointer();
> > +}
> > +
> > +
> >  virJSONValue *
> >  qemuBuildNicDevProps(virDomainDef *def,
> >   virDomainNetDef *net,
> > @@ -3813,6 +3832,7 @@ qemuBuildNicDevProps(virDomainDef *def,
> >  g_autoptr(virJSONValue) props = NULL;
> >  char macaddr[VIR_MAC_STRING_BUFLEN];
> >  g_autofree char *netdev = g_strdup_printf("host%s", net->info.alias);
> > +g_autoptr(virJSONValue) ebpffds = NULL;
>
> We like to declare variables in their smallest possible scope. IOW, this
> should be declared ..
>
> >
> >  if (virDomainNetIsVirtioModel(net)) {
> >  const char *tx = NULL;
>
> .. in this block.

Ok.

> > @@ -3863,6 +3883,8 @@ qemuBuildNicDevProps(virDomainDef *def,
> >  if (!(props = qemuBuildVirtioDevProps(VIR_DOMAIN_DEVICE_NET, net, 
> > qemuCaps)))
> >  return NULL;
> >
> > +ebpffds = qemuBuildEbpfRssArg(net);
> > +
> >  if (virJSONValueObjectAdd(,
> >"S:tx", tx,
> >"T:ioeventfd", 
> > net->driver.virtio.ioeventfd,
> > @@ -3887,6 +3909,7 @@ qemuBuildNicDevProps(virDomainDef *def,
> >"T:hash", 
> > net->driver.virtio.rss_hash_report,
> >"p:host_mtu", net->mtu,
> >"T:failover", failover,
> > +  "A:ebpf-rss-fds", ,
> >NULL) < 0)
> >  return NULL;
> >  } else {
> > @@ -4170,6 +4193,39 @@ qemuBuildWatchdogDevProps(const virDomainDef *def,
> >  }
> >
> >
> > +static void
> > +qemuOpenEbpfRssFds(virDomainNetDef *net, virQEMUCaps *qemuCaps)
> > +{
> > +const char 

Re: [RFC PATCH v3 6/6] tests: Added tests for eBPF blob loading.

[Andrew Melnichenko]

Hi all,

On Fri, May 17, 2024 at 5:00 PM Michal Prívozník  wrote:
>
> On 5/12/24 21:45, Andrew Melnychenko wrote:
> > Added net-virtio-rss-bpf to qemuxmlconf's test.
> > Synthetically modified caps-9.0.0 with a reply.
> > Added mock functions for loading eBPF.
> >
> > Signed-off-by: Andrew Melnychenko 
> > ---
> >  .../caps_9.0.0_x86_64.replies | 199 ++
> >  .../caps_9.0.0_x86_64.xml |   4 +
> >  tests/qemuxml2argvmock.c  |  21 ++
> >  .../net-virtio-rss-bpf.x86_64-latest.args |  37 
> >  .../net-virtio-rss-bpf.x86_64-latest.xml  |  46 
> >  tests/qemuxmlconfdata/net-virtio-rss-bpf.xml  |  46 
> >  tests/qemuxmlconftest.c   |   4 +
> >  7 files changed, 265 insertions(+), 92 deletions(-)
> >  create mode 100644 
> > tests/qemuxmlconfdata/net-virtio-rss-bpf.x86_64-latest.args
> >  create mode 100644 
> > tests/qemuxmlconfdata/net-virtio-rss-bpf.x86_64-latest.xml
> >  create mode 100644 tests/qemuxmlconfdata/net-virtio-rss-bpf.xml
> >
> > diff --git a/tests/qemucapabilitiesdata/caps_9.0.0_x86_64.replies 
> > b/tests/qemucapabilitiesdata/caps_9.0.0_x86_64.replies
> > index 5d36853ce3..b94625904b 100644
> > --- a/tests/qemucapabilitiesdata/caps_9.0.0_x86_64.replies
> > +++ b/tests/qemucapabilitiesdata/caps_9.0.0_x86_64.replies
>
>
> Changes to this file should be done earlier.
>
> > diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
> > index 9cc97199c4..0d1ebbe5b9 100644
> > --- a/tests/qemuxml2argvmock.c
> > +++ b/tests/qemuxml2argvmock.c
> > @@ -292,3 +292,24 @@ virNetDevSetMTU(const char *ifname G_GNUC_UNUSED,
> >  {
> >  return 0;
> >  }
> > +
> > +int
> > +qemuInterfaceLoadEbpf(__attribute__((unused)) const char *ebpfObject,
> > +__attribute__((unused)) void **retLibbpfObj, int *fds, size_t nfds)
> > +{
> > +if (nfds >= 4) {
> > +fds[0] = 0x100;
> > +fds[1] = 0x101;
> > +fds[2] = 0x102;
> > +fds[3] = 0x103;
> > +return 4;
> > +} else {
> > +return -1;
> > +}
> > +}
> > +
> > +void
> > +qemuInterfaceCloseEbpf(__attribute__((unused)) void *libbpfObj)
> > +{
> > +return;
> > +}
> > diff --git a/tests/qemuxmlconfdata/net-virtio-rss-bpf.x86_64-latest.args 
> > b/tests/qemuxmlconfdata/net-virtio-rss-bpf.x86_64-latest.args
> > new file mode 100644
> > index 00..b9497e5a73
> > --- /dev/null
> > +++ b/tests/qemuxmlconfdata/net-virtio-rss-bpf.x86_64-latest.args
> > @@ -0,0 +1,37 @@
> > +LC_ALL=C \
> > +PATH=/bin \
> > +HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1 \
> > +USER=test \
> > +LOGNAME=test \
> > +XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.local/share \
> > +XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.cache \
> > +XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
> > +/usr/bin/qemu-system-x86_64 \
> > +-name guest=QEMUGuest1,debug-threads=on \
> > +-S \
> > +-object 
> > '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-QEMUGuest1/master-key.aes"}'
> >  \
> > +-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \
> > +-accel tcg \
> > +-cpu qemu64 \
> > +-m size=219136k \
> > +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' 
> > \
> > +-overcommit mem-lock=off \
> > +-smp 1,sockets=1,cores=1,threads=1 \
> > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> > +-display none \
> > +-no-user-config \
> > +-nodefaults \
> > +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
> > +-mon chardev=charmonitor,id=monitor,mode=control \
> > +-rtc base=utc \
> > +-no-shutdown \
> > +-boot strict=on \
> > +-device 
> > '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
> > +-blockdev 
> > '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","read-only":false}'
> >  \
> > +-device 
> > '{"driver":"ide-hd","bus":"ide.0","unit":0,"drive":"libvirt-1-storage","id":"ide0-0-0","bootindex":1}'
> >  \
> > +-netdev '{"type":"user","id":"hostnet0"}' \
> > +-device 
> > '{"driver":"virtio-net-pci","rss":true,"ebpf-rss-fds":["256","257","258","259"],"netdev":"hostnet0","id":"net0","mac":"00:11:22:33:44:55","bus":"pci.0","addr":"0x2"}'
> >  \
> > +-audiodev '{"id":"audio1","driver":"none"}' \
> > +-device 
> > '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x5"}'
> >  \
> > +-sandbox 
> > on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
> > +-msg timestamp=on
> > diff --git a/tests/qemuxmlconfdata/net-virtio-rss-bpf.x86_64-latest.xml 
> > b/tests/qemuxmlconfdata/net-virtio-rss-bpf.x86_64-latest.xml
> > new file mode 100644
> > index 00..198540380c
> > --- /dev/null
> > +++ b/tests/qemuxmlconfdata/net-virtio-rss-bpf.x86_64-latest.xml
> > @@ -0,0 +1,46 @@
> > +
> > +  QEMUGuest1
> > +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> > +  219100
> > +  219100
> > +  1
> > 

Re: [RFC PATCH v3 5/6] qemu_conf: Added configuration to optionally disable eBPF loading.

[Andrew Melnichenko]

Hi all,

On Fri, May 17, 2024 at 5:00 PM Michal Prívozník  wrote:
>
> On 5/12/24 21:45, Andrew Melnychenko wrote:
> > Currently, there is no way to control that config through qemu.conf file.
> > This optional is required for future eBPF tests.
> >
> > Signed-off-by: Andrew Melnychenko 
> > ---
> >  src/qemu/qemu_command.c | 8 +---
> >  src/qemu/qemu_conf.c| 2 ++
> >  src/qemu/qemu_conf.h| 2 ++
> >  3 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 77715cf6fe..0d41d34c3b 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -8917,9 +8917,11 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver,
> >  qemuFDPassDirectTransferCommand(netpriv->slirpfd, cmd);
> >  qemuFDPassTransferCommand(netpriv->vdpafd, cmd);
> >
> > -qemuOpenEbpfRssFds(net, qemuCaps);
> > -for (n = netpriv->ebpfrssfds; n; n = n->next)
> > -qemuFDPassDirectTransferCommand(n->data, cmd);
> > +if (cfg->allowEBPF) {
> > +qemuOpenEbpfRssFds(net, qemuCaps);
> > +for (n = netpriv->ebpfrssfds; n; n = n->next)
> > +qemuFDPassDirectTransferCommand(n->data, cmd);
> > +}
> >
> >  if (!(hostnetprops = qemuBuildHostNetProps(vm, net)))
> >  goto cleanup;
> > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> > index 4050a82341..79168c3e54 100644
> > --- a/src/qemu/qemu_conf.c
> > +++ b/src/qemu/qemu_conf.c
> > @@ -287,6 +287,8 @@ virQEMUDriverConfig *virQEMUDriverConfigNew(bool 
> > privileged,
> >  cfg->deprecationBehavior = g_strdup("none");
> >  cfg->storageUseNbdkit = USE_NBDKIT_DEFAULT;
> >
> > +cfg->allowEBPF = true;
> > +
> >  return g_steal_pointer();
> >  }
> >
> > diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> > index 36049b4bfa..778897bd40 100644
> > --- a/src/qemu/qemu_conf.h
> > +++ b/src/qemu/qemu_conf.h
> > @@ -233,6 +233,8 @@ struct _virQEMUDriverConfig {
> >  bool storageUseNbdkit;
> >
> >  virQEMUSchedCore schedCore;
> > +
> > +bool allowEBPF;
> >  };
>
> This structure should reflect knobs that are tunable in qemu.conf. If
> you need a temporary change of behaviour just for tests we use env vars
> for that.

Yes, I need it only for tests. I'll work on it in the next version of patches.


>
> Michal
>

Re: [PATCH] fix virLogCleanerParseFilename logic

[David Negreira]

Hi,

For the reproducer I have enabled the garbage collection and debugging
on virtlogd.conf file:
- grep -v ^# /etc/libvirt/virtlogd.conf
  log_filters="1:logging 4:object 4:json 1:event 1:util"
  log_outputs="1:syslog:virtlogd"
  max_age_days = 1
  log_root = "/var/log/libvirt"
- I have also patched the cleaner timer with the patch attached so
that I could debug it quickly and not have to wait a full day :-)
- Create a couple of VMs so that some logs are created.

- When you enable the above you could see on the logs the message:
  "error : virLogCleanerParseFilename:89 : internal error: Failed to
parse rotated index from ''

Kind regards,
David Negreira


On Tue, May 21, 2024 at 10:42 AM Michal Prívozník  wrote:
>
> On 5/19/24 17:40, David Negreira wrote:
> > We should return the full filename path when we don't have a match on
> > the third group of the regex.
> >
> > Signed-off-by: David Negreira 
> > ---
> >  src/logging/log_cleaner.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/logging/log_cleaner.c b/src/logging/log_cleaner.c
> > index 4ee91843aa..d8e6ce9cdd 100644
> > --- a/src/logging/log_cleaner.c
> > +++ b/src/logging/log_cleaner.c
> > @@ -82,7 +82,7 @@ virLogCleanerParseFilename(const char *path,
> >  *rotated_index = 0;
> >  rotated_index_str = g_match_info_fetch(matchInfo, 3);
> >
> > -if (!rotated_index_str)
> > +if (rotated_index_str)
> >  return chain_prefix;
> >
> >  if (virStrToLong_i(rotated_index_str, NULL, 10, rotated_index) < 0) {
>
> I'm not sure this is the right fix. If rotated_index_str is NOT NULL
> chain_prefix is returned. Fair enough. But when it is NULL then it's
> passed to virStrToLong_i() which does not seem right.
>
> Also, do you have a minimalist reproducer?
>
> Michal
>


-- 
David Negreira
Senior Sustaining Operations Engineer | Canonical
e: david.negre...@canonical.com
w: www.ubuntu.com | Support: support.canonical.com
diff --git a/src/logging/log_cleaner.c b/src/logging/log_cleaner.c
index d8e6ce9cdd..413df54eb9 100644
--- a/src/logging/log_cleaner.c
+++ b/src/logging/log_cleaner.c
@@ -39,7 +39,7 @@ VIR_LOG_INIT("logging.log_cleaner");
 
 /* Cleanup log root (/var/log/libvirt) and all subfolders (e.g. /var/log/libvirt/qemu) */
 #define CLEANER_LOG_DEPTH 1
-#define CLEANER_LOG_TIMEOUT_MS (24 * 3600 * 1000) /* One day */
+#define CLEANER_LOG_TIMEOUT_MS (1 * 60 * 1000) /* 1 MINUTE */
 #define MAX_TIME ((time_t) G_MAXINT64)
 
 static GRegex *log_regex;

Re: [PATCH] qemu: Optimize monitor event name lookup with hash tables

[Peter Krempa]

On Tue, May 21, 2024 at 17:36:48 +0530, Rayhan Faizel wrote:
> Currently, monitor event names are looked up using binary search which has
> O(log(n)) time complexity. This can be optimized even further with a
> compile-time static hash table generated by the gperf tool. As gperf ensures
> perfect hashing, lookup times are guaranteed to be O(1).

As a first point I'd like to ask if you are seeing any specific
performance problems with the existing code and ideally elaborate in
which cases.

This is a non-trivial change and with a justification in this commit
message I'm not really convinced that it's needed.

Unless QEMU would be firing a massive amount of events it is unlikely
that the lookup of the event will even have measurable impact.

It's also very likely that the lookup of the event handler is
overshadowed by the complexity of the actual handler, thus is the impact
even measurable?

> This patch also makes gperf a requirement for compiling libvirt if the QEMU
> driver is enabled.

Based on the above question I'm not persuaded that this is really
justified at this point.

Note that such change will also require update to the .spec. file
libvirt ships in the repo.

> 
> Signed-off-by: Rayhan Faizel 
> ---
>  docs/kbase/internals/qemu-event-handlers.rst |  13 +-
>  meson.build  |   2 +
>  src/qemu/meson.build |   9 +
>  src/qemu/qemu_monitor_event.gperf|  68 
>  src/qemu/qemu_monitor_json.c | 171 +--
>  src/qemu/qemu_monitor_json.h |  45 +
>  6 files changed, 170 insertions(+), 138 deletions(-)
>  create mode 100644 src/qemu/qemu_monitor_event.gperf

[PATCH] qemu: Optimize monitor event name lookup with hash tables

[Rayhan Faizel]

Currently, monitor event names are looked up using binary search which has
O(log(n)) time complexity. This can be optimized even further with a
compile-time static hash table generated by the gperf tool. As gperf ensures
perfect hashing, lookup times are guaranteed to be O(1).

This patch also makes gperf a requirement for compiling libvirt if the QEMU
driver is enabled.

Signed-off-by: Rayhan Faizel 
---
 docs/kbase/internals/qemu-event-handlers.rst |  13 +-
 meson.build  |   2 +
 src/qemu/meson.build |   9 +
 src/qemu/qemu_monitor_event.gperf|  68 
 src/qemu/qemu_monitor_json.c | 171 +--
 src/qemu/qemu_monitor_json.h |  45 +
 6 files changed, 170 insertions(+), 138 deletions(-)
 create mode 100644 src/qemu/qemu_monitor_event.gperf

diff --git a/docs/kbase/internals/qemu-event-handlers.rst 
b/docs/kbase/internals/qemu-event-handlers.rst
index 3589c4c48c..5ec11e28bd 100644
--- a/docs/kbase/internals/qemu-event-handlers.rst
+++ b/docs/kbase/internals/qemu-event-handlers.rst
@@ -23,16 +23,15 @@ QEMU monitor events
 
 Any event emitted by qemu is received by
 ``qemu_monitor_json.c:qemuMonitorJSONIOProcessEvent()``. It looks up the
-event by name in the table ``eventHandlers`` (in the same file), which
-should have an entry like this for each event that libvirt
-understands::
+event by name from a hash-table, generated at compile-time by ``gperf``
+based on a list of entries defined in ``qemu_monitor_event.gperf``.
+``qemu_monitor_event.gperf`` should should have an entry like this
+for each event that libvirt understands::
 
-{ "NIC_RX_FILTER_CHANGED", qemuMonitorJSONHandleNicRxFilterChanged, },
-
-NB: This table is searched with bsearch, so it *must* be alphabetically sorted.
+"NIC_RX_FILTER_CHANGED", qemuMonitorJSONHandleNicRxFilterChanged
 
 ``qemuMonitorJSONIOProcessEvent`` calls the function listed in
-``eventHandlers``, e.g.::
+``qemu_monitor_event.gperf``, e.g.::
 
qemu_monitor_json.c:qemuMonitorJSONHandleNicRxFilterChanged()
 
diff --git a/meson.build b/meson.build
index e8b0094b91..03f1f0d56d 100644
--- a/meson.build
+++ b/meson.build
@@ -1708,6 +1708,8 @@ if not get_option('driver_qemu').disabled()
   qemu_slirp_path = '/usr/bin/slirp-helper'
 endif
 conf.set_quoted('QEMU_SLIRP_HELPER', qemu_slirp_path)
+
+gperf_prog = find_program('gperf')
   endif
 endif
 
diff --git a/src/qemu/meson.build b/src/qemu/meson.build
index 907893d431..2440476a7b 100644
--- a/src/qemu/meson.build
+++ b/src/qemu/meson.build
@@ -88,11 +88,20 @@ qemu_shim_sources = files(
   'qemu_shim.c',
 )
 
+qemu_driver_gperf_sources = []
+
+qemu_driver_gperf_sources += custom_target(
+  'qemu_monitor_event.c',
+  input : 'qemu_monitor_event.gperf',
+  output : 'qemu_monitor_event.c',
+  command : [gperf_prog, '@INPUT@', '--output-file', '@OUTPUT@'])
+
 if conf.has('WITH_QEMU')
   qemu_driver_impl = static_library(
 'virt_driver_qemu_impl',
 [
   qemu_driver_sources,
+  qemu_driver_gperf_sources,
   qemu_dtrace_gen_headers,
 ],
 dependencies: [
diff --git a/src/qemu/qemu_monitor_event.gperf 
b/src/qemu/qemu_monitor_event.gperf
new file mode 100644
index 00..de2f958b9e
--- /dev/null
+++ b/src/qemu/qemu_monitor_event.gperf
@@ -0,0 +1,68 @@
+/*
+ * qemu_monitor_event.gperf: QEMU Monitor Event Lookup Table
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+%{
+#include "qemu_monitor_json.h"
+%}
+qemuEventHandler;
+%null_strings
+%define initializer-suffix ,NULL
+%language=ANSI-C
+%define slot-name type
+%define lookup-function-name qemu_monitor_event_lookup
+%define hash-function-name qemu_monitor_event_hash
+%readonly-tables
+%omit-struct-type
+%struct-type
+%%
+"ACPI_DEVICE_OST", qemuMonitorJSONHandleAcpiOstInfo
+"BALLOON_CHANGE", qemuMonitorJSONHandleBalloonChange
+"BLOCK_IO_ERROR", qemuMonitorJSONHandleIOError
+"BLOCK_WRITE_THRESHOLD", qemuMonitorJSONHandleBlockThreshold
+"DEVICE_DELETED", qemuMonitorJSONHandleDeviceDeleted
+"DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange
+"DEVICE_UNPLUG_GUEST_ERROR", qemuMonitorJSONHandleDeviceUnplugErr
+"DUMP_COMPLETED", qemuMonitorJSONHandleDumpCompleted
+"GUEST_CRASHLOADED", qemuMonitorJSONHandleGuestCrashloaded
+"GUEST_PANICKED", qemuMonitorJSONHandleGuestPanic

Re: [PATCH v2] network: add modify-or-add feature to net-update

[atp exp]

nvm, the command was not working due to a different reason.
The indexes used in delete and modify were still `i` need to
update that.



On Tue, 21 May 2024 at 15:31, atp exp  wrote:

>
> On Wed, 15 May 2024 at 15:43, Abhiram Tilak  wrote:
>
>> The current way of updating a network configuration uses `virsh
>> net-update` to add, delete or modify entries. But with such a mechansim
>> one should know if an entry with current info already exists. Adding
>> modify-or-add option automatically performs either modify or add
>> depending on the current state.
>>
>> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/363
>> Signed-off-by: Abhiram Tilak 
>> ---
>> Changes in v2:
>>   - Removed the modify-or-add functionality for sections
>> where modify is not applicable.
>>   - Changed the existing implementation of `UpdateIPDHCPHost`,
>> to avoid code duplication.
>>   - Changed the implementation of modify-or-delete to reassign
>> the `command` variable instead of using multiple nested conditions.
>>
>>  docs/manpages/virsh.rst   |   5 +-
>>  include/libvirt/libvirt-network.h |  12 +--
>>  src/conf/network_conf.c   | 134 +-
>>  tools/virsh-network.c |   6 +-
>>  4 files changed, 110 insertions(+), 47 deletions(-)
>>
>> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
>> index 115b802c45..0da3827f6b 100644
>> --- a/docs/manpages/virsh.rst
>> +++ b/docs/manpages/virsh.rst
>> @@ -5908,7 +5908,10 @@ changes optionally taking effect immediately,
>> without needing to
>>  destroy and re-start the network.
>>
>>  *command* is one of "add-first", "add-last", "add" (a synonym for
>> -add-last), "delete", or "modify".
>> +add-last), "delete", "modify", "modify-or-add" (modify + add-last), or
>> +"modify-or-add-first". The 'modify-or-add*' commands perform modify or
>> +add operation depending on the given state, and can be useful for
>> +scripting.
>>
>>  *section* is one of "bridge", "domain", "ip", "ip-dhcp-host",
>>  "ip-dhcp-range", "forward", "forward-interface", "forward-pf",
>> diff --git a/include/libvirt/libvirt-network.h
>> b/include/libvirt/libvirt-network.h
>> index 58591be7ac..bb4468b160 100644
>> --- a/include/libvirt/libvirt-network.h
>> +++ b/include/libvirt/libvirt-network.h
>> @@ -176,11 +176,13 @@ int virNetworkUndefine
>> (virNetworkPtr network);
>>   * Since: 0.10.2
>>   */
>>  typedef enum {
>> -VIR_NETWORK_UPDATE_COMMAND_NONE  = 0, /* invalid (Since: 0.10.2)
>> */
>> -VIR_NETWORK_UPDATE_COMMAND_MODIFY= 1, /* modify an existing
>> element (Since: 0.10.2) */
>> -VIR_NETWORK_UPDATE_COMMAND_DELETE= 2, /* delete an existing
>> element (Since: 0.10.2) */
>> -VIR_NETWORK_UPDATE_COMMAND_ADD_LAST  = 3, /* add an element at end
>> of list (Since: 0.10.2) */
>> -VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST = 4, /* add an element at start
>> of list (Since: 0.10.2) */
>> +VIR_NETWORK_UPDATE_COMMAND_NONE = 0, /* invalid (Since:
>> 0.10.2) */
>> +VIR_NETWORK_UPDATE_COMMAND_MODIFY   = 1, /* modify an
>> existing element (Since: 0.10.2) */
>> +VIR_NETWORK_UPDATE_COMMAND_DELETE   = 2, /* delete an
>> existing element (Since: 0.10.2) */
>> +VIR_NETWORK_UPDATE_COMMAND_ADD_LAST = 3, /* add an element
>> at end of list (Since: 0.10.2) */
>> +VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST= 4, /* add an element
>> at start of list (Since: 0.10.2) */
>> +VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_LAST  = 5, /* conditionally
>> modify or add an element at end (Since: 10.4.0) */
>> +VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_FIRST = 6, /* conditionally
>> modify or add an element at start (Since: 10.4.0) */
>>  # ifdef VIR_ENUM_SENTINELS
>>  VIR_NETWORK_UPDATE_COMMAND_LAST /* (Since: 0.10.2) */
>>  # endif
>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>> index cc92ed0b03..a7c3dea163 100644
>> --- a/src/conf/network_conf.c
>> +++ b/src/conf/network_conf.c
>> @@ -2720,6 +2720,7 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDef *def,
>>  virNetworkIPDef *ipdef = virNetworkIPDefByIndex(def, parentIndex);
>>  virNetworkDHCPHostDef host = { 0 };
>>  bool partialOkay = (command == VIR_NETWORK_UPDATE_COMMAND_DELETE);
>> +int foundMatchedEntry = -1, foundExactEntry = -1;
>>
>>  if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "host") < 0)
>>  goto cleanup;
>> @@ -2740,22 +2741,47 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDef *def,
>>  goto cleanup;
>>  }
>>
>> -if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
>> +/* check if the entry already exsits */
>> +for (i = 0; i < ipdef->nhosts; i++) {
>>
>> -/* search for the entry with this (ip|mac|name),
>> - * and update the IP+(mac|name) */
>> -for (i = 0; i < ipdef->nhosts; i++) {
>> -if ((host.mac && ipdef->hosts[i].mac &&
>> - !virMacAddrCompare(host.mac, 

[PATCH v3] network: add modify-or-add feature to net-update

[Abhiram Tilak]

The current way of updating a network configuration uses `virsh
net-update` to add, delete or modify entries. But with such a mechansim
one should know if an entry with current info already exists. Adding
modify-or-add option automatically performs either modify or add
depending on the current state.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/363
Signed-off-by: Abhiram Tilak 
---
Changes in v3:
  - Changed the indexes used to delete or modify instead of `i`.

Changes in v2:
  - Removed the modify-or-add functionality for sections
where modify is not applicable.
  - Changed the existing implementation of `UpdateIPDHCPHost`,
to avoid code duplication.
  - Changed the implementation of modify-or-delete to reassign
the `command` variable instead of using multiple nested conditions.

 docs/manpages/virsh.rst   |   5 +-
 include/libvirt/libvirt-network.h |  12 +--
 src/conf/network_conf.c   | 142 +-
 tools/virsh-network.c |   6 +-
 4 files changed, 114 insertions(+), 51 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index fa038e4547..918e6db30c 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -5906,7 +5906,10 @@ changes optionally taking effect immediately, without 
needing to
 destroy and re-start the network.
 
 *command* is one of "add-first", "add-last", "add" (a synonym for
-add-last), "delete", or "modify".
+add-last), "delete", "modify", "modify-or-add" (modify + add-last), or
+"modify-or-add-first". The 'modify-or-add*' commands perform modify or
+add operation depending on the given state, and can be useful for
+scripting.
 
 *section* is one of "bridge", "domain", "ip", "ip-dhcp-host",
 "ip-dhcp-range", "forward", "forward-interface", "forward-pf",
diff --git a/include/libvirt/libvirt-network.h 
b/include/libvirt/libvirt-network.h
index 58591be7ac..bb4468b160 100644
--- a/include/libvirt/libvirt-network.h
+++ b/include/libvirt/libvirt-network.h
@@ -176,11 +176,13 @@ int virNetworkUndefine  
(virNetworkPtr network);
  * Since: 0.10.2
  */
 typedef enum {
-VIR_NETWORK_UPDATE_COMMAND_NONE  = 0, /* invalid (Since: 0.10.2) */
-VIR_NETWORK_UPDATE_COMMAND_MODIFY= 1, /* modify an existing element 
(Since: 0.10.2) */
-VIR_NETWORK_UPDATE_COMMAND_DELETE= 2, /* delete an existing element 
(Since: 0.10.2) */
-VIR_NETWORK_UPDATE_COMMAND_ADD_LAST  = 3, /* add an element at end of list 
(Since: 0.10.2) */
-VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST = 4, /* add an element at start of 
list (Since: 0.10.2) */
+VIR_NETWORK_UPDATE_COMMAND_NONE = 0, /* invalid (Since: 
0.10.2) */
+VIR_NETWORK_UPDATE_COMMAND_MODIFY   = 1, /* modify an existing 
element (Since: 0.10.2) */
+VIR_NETWORK_UPDATE_COMMAND_DELETE   = 2, /* delete an existing 
element (Since: 0.10.2) */
+VIR_NETWORK_UPDATE_COMMAND_ADD_LAST = 3, /* add an element at end 
of list (Since: 0.10.2) */
+VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST= 4, /* add an element at 
start of list (Since: 0.10.2) */
+VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_LAST  = 5, /* conditionally modify 
or add an element at end (Since: 10.4.0) */
+VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_FIRST = 6, /* conditionally modify 
or add an element at start (Since: 10.4.0) */
 # ifdef VIR_ENUM_SENTINELS
 VIR_NETWORK_UPDATE_COMMAND_LAST /* (Since: 0.10.2) */
 # endif
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index cc92ed0b03..4f76257d06 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2720,6 +2720,7 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDef *def,
 virNetworkIPDef *ipdef = virNetworkIPDefByIndex(def, parentIndex);
 virNetworkDHCPHostDef host = { 0 };
 bool partialOkay = (command == VIR_NETWORK_UPDATE_COMMAND_DELETE);
+int foundMatchedEntry = -1, foundExactEntry = -1;
 
 if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "host") < 0)
 goto cleanup;
@@ -2740,22 +2741,47 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDef *def,
 goto cleanup;
 }
 
-if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
+/* check if the entry already exsits */
+for (i = 0; i < ipdef->nhosts; i++) {
 
-/* search for the entry with this (ip|mac|name),
- * and update the IP+(mac|name) */
-for (i = 0; i < ipdef->nhosts; i++) {
-if ((host.mac && ipdef->hosts[i].mac &&
- !virMacAddrCompare(host.mac, ipdef->hosts[i].mac)) ||
-(VIR_SOCKET_ADDR_VALID() &&
- virSocketAddrEqual(, >hosts[i].ip)) ||
-(host.name &&
- STREQ_NULLABLE(host.name, ipdef->hosts[i].name))) {
-break;
-}
+/* try to match any of (ip|mac|name) attributes */
+if ((host.mac && ipdef->hosts[i].mac &&
+ !virMacAddrCompare(host.mac, ipdef->hosts[i].mac)) ||
+

Re: [PATCH] fix virLogCleanerParseFilename logic

[Michal Prívozník]

On 5/21/24 11:11, David Negreira wrote:
> Hi,
> 
> For the reproducer I have enabled the garbage collection and debugging
> on virtlogd.conf file:
> - grep -v ^# /etc/libvirt/virtlogd.conf
>   log_filters="1:logging 4:object 4:json 1:event 1:util"
>   log_outputs="1:syslog:virtlogd"
>   max_age_days = 1
>   log_root = "/var/log/libvirt"
> - I have also patched the cleaner timer with the patch attached so
> that I could debug it quickly and not have to wait a full day :-)
> - Create a couple of VMs so that some logs are created.
> 
> - When you enable the above you could see on the logs the message:
>   "error : virLogCleanerParseFilename:89 : internal error: Failed to
> parse rotated index from ''


Ah, that helped. Thanks. IMO the proper fix is something among these lines:

diff --git i/src/logging/log_cleaner.c w/src/logging/log_cleaner.c
index 4ee91843aa..9f987b02b5 100644
--- i/src/logging/log_cleaner.c
+++ w/src/logging/log_cleaner.c
@@ -82,10 +82,8 @@ virLogCleanerParseFilename(const char *path,
 *rotated_index = 0;
 rotated_index_str = g_match_info_fetch(matchInfo, 3);
 
-if (!rotated_index_str)
-return chain_prefix;
-
-if (virStrToLong_i(rotated_index_str, NULL, 10, rotated_index) < 0) {
+if (rotated_index_str && STRNEQ(rotated_index_str, "") &&
+virStrToLong_i(rotated_index_str, NULL, 10, rotated_index) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to parse rotated index from '%1$s'"),
rotated_index_str);

If the path lacks ".N" suffix, then the third match group is empty and
g_match_info_fetch() returns an empty string. IOW, we should be parsing
the suffix iff it's a non-empty string.

Alternatively, we might use g_match_info_get_match_count() to find out
how many groups were matched.
What I also experimented with was passing G_REGEX_MATCH_NOTEMPTY flag to
g_regex_new() but that didn't work. Idea was to make
g_match_info_fetch(,3) return NULL, but I have no idea why it didn't
work.

Can you please send v2?

Michal

Re: [PATCH] docs: Fix broken links

[Michal Prívozník]

On 5/21/24 11:07, Han Han wrote:
> For the links of drvinterface, drvnetwork, drvnwfilter, and Nagios-virt,
> there are no alternative docs. Just remove them directly.
> 
> Signed-off-by: Han Han 
> ---
>  docs/apps.rst  | 10 ++
>  docs/kbase/live_full_disk_backup.rst   |  2 +-
>  docs/manpages/virt-sanlock-cleanup.rst |  2 +-
>  docs/manpages/virtinterfaced.rst   |  1 -
>  docs/manpages/virtnetworkd.rst |  1 -
>  docs/manpages/virtnwfilterd.rst|  1 -
>  docs/manpages/virtstoraged.rst |  2 +-
>  docs/manpages/virtvzd.rst  |  2 +-
>  docs/windows.rst   |  2 +-
>  9 files changed, 7 insertions(+), 16 deletions(-)
> 

> diff --git a/docs/manpages/virtstoraged.rst b/docs/manpages/virtstoraged.rst
> index 70863282d1..fe966f3123 100644
> --- a/docs/manpages/virtstoraged.rst
> +++ b/docs/manpages/virtstoraged.rst
> @@ -211,4 +211,4 @@ SEE ALSO
>  
>  virsh(1), libvirtd(8),
>  `https://libvirt.org/daemons.html `_,
> -`https://libvirt.org/drvstorage.html `_
> +`https://libvirt.org/storage.html `_

s/drvstorage/storage/


Reviewed-by: Michal Privoznik 

and merged. Thanks!

Michal

Re: [PATCH] virt-host-validate: Improve translatability of messages printed by 'virHostMsgCheck()'

[Jiri Denemark]

On Mon, May 20, 2024 at 15:00:26 +0200, Peter Krempa wrote:
> Move the word 'Checking' into the appropriate formatting strings and
> mark all outstanding ones for translation.
> 
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/637
> Signed-off-by: Peter Krempa 
> ---
>  tools/virt-host-validate-bhyve.c  |  2 +-
>  tools/virt-host-validate-ch.c |  2 +-
>  tools/virt-host-validate-common.c | 18 +-
>  tools/virt-host-validate-qemu.c   |  4 ++--
>  4 files changed, 13 insertions(+), 13 deletions(-)
> 
...
> diff --git a/tools/virt-host-validate-common.c 
> b/tools/virt-host-validate-common.c
> index c8a23e2e99..58d8dbbaa1 100644
> --- a/tools/virt-host-validate-common.c
> +++ b/tools/virt-host-validate-common.c
> @@ -66,7 +66,7 @@ void virHostMsgCheck(const char *prefix,
>  msg = g_strdup_vprintf(format, args);
>  va_end(args);
> 
> -fprintf(stdout, _("%1$6s: Checking %2$-60s: "), prefix, msg);
> +fprintf(stdout, "%1$6s: %2$-60s: ", prefix, msg);
>  }
> 
>  static bool virHostMsgWantEscape(void)

Shouldn't the formatting string change from %2$-60s to %2$-69s since the
string will now contain the extra "Checking " prefix?

Jirka

Re: [PATCH v2] network: add modify-or-add feature to net-update

[atp exp]

On Wed, 15 May 2024 at 15:43, Abhiram Tilak  wrote:

> The current way of updating a network configuration uses `virsh
> net-update` to add, delete or modify entries. But with such a mechansim
> one should know if an entry with current info already exists. Adding
> modify-or-add option automatically performs either modify or add
> depending on the current state.
>
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/363
> Signed-off-by: Abhiram Tilak 
> ---
> Changes in v2:
>   - Removed the modify-or-add functionality for sections
> where modify is not applicable.
>   - Changed the existing implementation of `UpdateIPDHCPHost`,
> to avoid code duplication.
>   - Changed the implementation of modify-or-delete to reassign
> the `command` variable instead of using multiple nested conditions.
>
>  docs/manpages/virsh.rst   |   5 +-
>  include/libvirt/libvirt-network.h |  12 +--
>  src/conf/network_conf.c   | 134 +-
>  tools/virsh-network.c |   6 +-
>  4 files changed, 110 insertions(+), 47 deletions(-)
>
> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> index 115b802c45..0da3827f6b 100644
> --- a/docs/manpages/virsh.rst
> +++ b/docs/manpages/virsh.rst
> @@ -5908,7 +5908,10 @@ changes optionally taking effect immediately,
> without needing to
>  destroy and re-start the network.
>
>  *command* is one of "add-first", "add-last", "add" (a synonym for
> -add-last), "delete", or "modify".
> +add-last), "delete", "modify", "modify-or-add" (modify + add-last), or
> +"modify-or-add-first". The 'modify-or-add*' commands perform modify or
> +add operation depending on the given state, and can be useful for
> +scripting.
>
>  *section* is one of "bridge", "domain", "ip", "ip-dhcp-host",
>  "ip-dhcp-range", "forward", "forward-interface", "forward-pf",
> diff --git a/include/libvirt/libvirt-network.h
> b/include/libvirt/libvirt-network.h
> index 58591be7ac..bb4468b160 100644
> --- a/include/libvirt/libvirt-network.h
> +++ b/include/libvirt/libvirt-network.h
> @@ -176,11 +176,13 @@ int virNetworkUndefine
> (virNetworkPtr network);
>   * Since: 0.10.2
>   */
>  typedef enum {
> -VIR_NETWORK_UPDATE_COMMAND_NONE  = 0, /* invalid (Since: 0.10.2)
> */
> -VIR_NETWORK_UPDATE_COMMAND_MODIFY= 1, /* modify an existing
> element (Since: 0.10.2) */
> -VIR_NETWORK_UPDATE_COMMAND_DELETE= 2, /* delete an existing
> element (Since: 0.10.2) */
> -VIR_NETWORK_UPDATE_COMMAND_ADD_LAST  = 3, /* add an element at end of
> list (Since: 0.10.2) */
> -VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST = 4, /* add an element at start
> of list (Since: 0.10.2) */
> +VIR_NETWORK_UPDATE_COMMAND_NONE = 0, /* invalid (Since:
> 0.10.2) */
> +VIR_NETWORK_UPDATE_COMMAND_MODIFY   = 1, /* modify an
> existing element (Since: 0.10.2) */
> +VIR_NETWORK_UPDATE_COMMAND_DELETE   = 2, /* delete an
> existing element (Since: 0.10.2) */
> +VIR_NETWORK_UPDATE_COMMAND_ADD_LAST = 3, /* add an element at
> end of list (Since: 0.10.2) */
> +VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST= 4, /* add an element at
> start of list (Since: 0.10.2) */
> +VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_LAST  = 5, /* conditionally
> modify or add an element at end (Since: 10.4.0) */
> +VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_FIRST = 6, /* conditionally
> modify or add an element at start (Since: 10.4.0) */
>  # ifdef VIR_ENUM_SENTINELS
>  VIR_NETWORK_UPDATE_COMMAND_LAST /* (Since: 0.10.2) */
>  # endif
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index cc92ed0b03..a7c3dea163 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -2720,6 +2720,7 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDef *def,
>  virNetworkIPDef *ipdef = virNetworkIPDefByIndex(def, parentIndex);
>  virNetworkDHCPHostDef host = { 0 };
>  bool partialOkay = (command == VIR_NETWORK_UPDATE_COMMAND_DELETE);
> +int foundMatchedEntry = -1, foundExactEntry = -1;
>
>  if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "host") < 0)
>  goto cleanup;
> @@ -2740,22 +2741,47 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDef *def,
>  goto cleanup;
>  }
>
> -if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
> +/* check if the entry already exsits */
> +for (i = 0; i < ipdef->nhosts; i++) {
>
> -/* search for the entry with this (ip|mac|name),
> - * and update the IP+(mac|name) */
> -for (i = 0; i < ipdef->nhosts; i++) {
> -if ((host.mac && ipdef->hosts[i].mac &&
> - !virMacAddrCompare(host.mac, ipdef->hosts[i].mac)) ||
> -(VIR_SOCKET_ADDR_VALID() &&
> - virSocketAddrEqual(, >hosts[i].ip)) ||
> -(host.name &&
> - STREQ_NULLABLE(host.name, ipdef->hosts[i].name))) {
> -break;
> -}
> +/* try to match 

Re: [PATCH] docs: Fix broken links

[Han Han]

On Tue, May 21, 2024 at 5:43 PM Richard W.M. Jones 
wrote:

> On Tue, May 21, 2024 at 05:12:00PM +0800, Han Han wrote:
> > @Richard Jones Hi Richard,
> > I cannot find the valid link for Nagios-virt. However, I saw your ID in
> the
> > broken link for Nagios-virt.
> > Could you help provide the new link for Nagios-virt?
> >
> > -`Nagios-virt `__
> > -   Nagios-virt is a configuration tool to add monitoring of your
> > -   virtualised domains to `Nagios `__.
> You can
> > -   use this tool to either set up a new Nagios installation for
> your Xen
> > -   or QEMU/KVM guests, or to integrate with your existing Nagios
> > -   installation.
>
> I think it's dead, so removing it (as you seem to be doing here)
> is the right thing to do IMHO.
>
> OK. Thanks for your confirmation

> Rich.
>
> --
> Richard Jones, Virtualization Group, Red Hat
> http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> Fedora Windows cross-compiler. Compile Windows programs, test, and
> build Windows installers. Over 100 libraries supported.
> http://fedoraproject.org/wiki/MinGW
>
>

Re: [PATCH] docs: Fix broken links

[Richard W.M. Jones]

On Tue, May 21, 2024 at 05:12:00PM +0800, Han Han wrote:
> @Richard Jones Hi Richard,
> I cannot find the valid link for Nagios-virt. However, I saw your ID in the
> broken link for Nagios-virt.
> Could you help provide the new link for Nagios-virt?
> 
> -`Nagios-virt `__
> -   Nagios-virt is a configuration tool to add monitoring of your
> -   virtualised domains to `Nagios `__. You can
> -   use this tool to either set up a new Nagios installation for your Xen
> -   or QEMU/KVM guests, or to integrate with your existing Nagios
> -   installation.

I think it's dead, so removing it (as you seem to be doing here)
is the right thing to do IMHO.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Re: [PATCH] docs: Fix broken links

[Han Han]

@Richard Jones  Hi Richard,
I cannot find the valid link for Nagios-virt. However, I saw your ID in the
broken link for Nagios-virt.
Could you help provide the new link for Nagios-virt?

On Tue, May 21, 2024 at 5:08 PM Han Han  wrote:

> For the links of drvinterface, drvnetwork, drvnwfilter, and Nagios-virt,
> there are no alternative docs. Just remove them directly.
>
> Signed-off-by: Han Han 
> ---
>  docs/apps.rst  | 10 ++
>  docs/kbase/live_full_disk_backup.rst   |  2 +-
>  docs/manpages/virt-sanlock-cleanup.rst |  2 +-
>  docs/manpages/virtinterfaced.rst   |  1 -
>  docs/manpages/virtnetworkd.rst |  1 -
>  docs/manpages/virtnwfilterd.rst|  1 -
>  docs/manpages/virtstoraged.rst |  2 +-
>  docs/manpages/virtvzd.rst  |  2 +-
>  docs/windows.rst   |  2 +-
>  9 files changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/docs/apps.rst b/docs/apps.rst
> index b7d19e15d5..443c888c4d 100644
> --- a/docs/apps.rst
> +++ b/docs/apps.rst
> @@ -35,7 +35,7 @@ virsh
> machine to be cloned to form a new virtual machine. It automates
> copying of data across to new disk images, and updates the UUID, MAC
> address, and name in the configuration.
> -`virt-df `__
> +`virt-df `__
> Examine the utilization of each filesystem in a virtual machine from
> the comfort of the host machine. This tool peeks into the guest disks
> and determines how much space is used. It can cope with common Linux
> @@ -235,13 +235,7 @@ Monitoring
> The plugins provided by Guido Günther allow to monitor various things
> like network and block I/O with
> `Munin `__.
> -`Nagios-virt `__
> -   Nagios-virt is a configuration tool to add monitoring of your
> -   virtualised domains to `Nagios `__. You can
> -   use this tool to either set up a new Nagios installation for your Xen
> -   or QEMU/KVM guests, or to integrate with your existing Nagios
> -   installation.
> -`PCP `__
> +`PCP `__
> The PCP libvirt PMDA (plugin) is part of the
> `PCP `__ toolkit and provides hypervisor and guest
> information and complete set of guest performance metrics. It
> diff --git a/docs/kbase/live_full_disk_backup.rst
> b/docs/kbase/live_full_disk_backup.rst
> index 562a9e87b0..f20169e314 100644
> --- a/docs/kbase/live_full_disk_backup.rst
> +++ b/docs/kbase/live_full_disk_backup.rst
> @@ -12,7 +12,7 @@ space requirements.  The following outlines an efficient
> method to do
>  that using libvirt's APIs.  This method involves concepts: the notion of
>  `backing chains `_,
>  `QCOW2 overlays
> -<
> https://qemu.readthedocs.io/en/latest/interop/live-block-operations.html#disk-image-backing-chain-notation
> >`_,
> +<
> https://www.qemu.org/docs/master/interop/live-block-operations.html#disk-image-backing-chain-notation
> >`_,
>  and a special operation called "active block-commit", which allows
>  live-merging an overlay disk image into its backing file.
>
> diff --git a/docs/manpages/virt-sanlock-cleanup.rst
> b/docs/manpages/virt-sanlock-cleanup.rst
> index 3ad70ce683..bf3ef6742b 100644
> --- a/docs/manpages/virt-sanlock-cleanup.rst
> +++ b/docs/manpages/virt-sanlock-cleanup.rst
> @@ -75,5 +75,5 @@ PURPOSE
>  SEE ALSO
>  
>
> -virsh(1), `online instructions `_,
> +virsh(1), `online instructions  >`_,
>  `https://libvirt.org/ `_
> diff --git a/docs/manpages/virtinterfaced.rst
> b/docs/manpages/virtinterfaced.rst
> index 247a8c4009..ef63f4c278 100644
> --- a/docs/manpages/virtinterfaced.rst
> +++ b/docs/manpages/virtinterfaced.rst
> @@ -211,4 +211,3 @@ SEE ALSO
>
>  virsh(1), libvirtd(8),
>  `https://libvirt.org/daemons.html `_,
> -`https://libvirt.org/drvinterface.html <
> https://libvirt.org/drvinterface.html>`_
> diff --git a/docs/manpages/virtnetworkd.rst
> b/docs/manpages/virtnetworkd.rst
> index 22b3fc0f2d..3bd1dd3221 100644
> --- a/docs/manpages/virtnetworkd.rst
> +++ b/docs/manpages/virtnetworkd.rst
> @@ -211,4 +211,3 @@ SEE ALSO
>
>  virsh(1), libvirtd(8),
>  `https://libvirt.org/daemons.html `_,
> -`https://libvirt.org/drvnetwork.html  >`_
> diff --git a/docs/manpages/virtnwfilterd.rst
> b/docs/manpages/virtnwfilterd.rst
> index b1fc45e7a2..1ec11c247f 100644
> --- a/docs/manpages/virtnwfilterd.rst
> +++ b/docs/manpages/virtnwfilterd.rst
> @@ -211,4 +211,3 @@ SEE ALSO
>
>  virsh(1), libvirtd(8),
>  `https://libvirt.org/daemons.html `_,

[PATCH] docs: Fix broken links

[Han Han]

For the links of drvinterface, drvnetwork, drvnwfilter, and Nagios-virt,
there are no alternative docs. Just remove them directly.

Signed-off-by: Han Han 
---
 docs/apps.rst  | 10 ++
 docs/kbase/live_full_disk_backup.rst   |  2 +-
 docs/manpages/virt-sanlock-cleanup.rst |  2 +-
 docs/manpages/virtinterfaced.rst   |  1 -
 docs/manpages/virtnetworkd.rst |  1 -
 docs/manpages/virtnwfilterd.rst|  1 -
 docs/manpages/virtstoraged.rst |  2 +-
 docs/manpages/virtvzd.rst  |  2 +-
 docs/windows.rst   |  2 +-
 9 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/docs/apps.rst b/docs/apps.rst
index b7d19e15d5..443c888c4d 100644
--- a/docs/apps.rst
+++ b/docs/apps.rst
@@ -35,7 +35,7 @@ virsh
machine to be cloned to form a new virtual machine. It automates
copying of data across to new disk images, and updates the UUID, MAC
address, and name in the configuration.
-`virt-df `__
+`virt-df `__
Examine the utilization of each filesystem in a virtual machine from
the comfort of the host machine. This tool peeks into the guest disks
and determines how much space is used. It can cope with common Linux
@@ -235,13 +235,7 @@ Monitoring
The plugins provided by Guido G??nther allow to monitor various things
like network and block I/O with
`Munin `__.
-`Nagios-virt `__
-   Nagios-virt is a configuration tool to add monitoring of your
-   virtualised domains to `Nagios `__. You can
-   use this tool to either set up a new Nagios installation for your Xen
-   or QEMU/KVM guests, or to integrate with your existing Nagios
-   installation.
-`PCP `__
+`PCP `__
The PCP libvirt PMDA (plugin) is part of the
`PCP `__ toolkit and provides hypervisor and guest
information and complete set of guest performance metrics. It
diff --git a/docs/kbase/live_full_disk_backup.rst 
b/docs/kbase/live_full_disk_backup.rst
index 562a9e87b0..f20169e314 100644
--- a/docs/kbase/live_full_disk_backup.rst
+++ b/docs/kbase/live_full_disk_backup.rst
@@ -12,7 +12,7 @@ space requirements.  The following outlines an efficient 
method to do
 that using libvirt's APIs.  This method involves concepts: the notion of
 `backing chains `_,
 `QCOW2 overlays
-`_,
+`_,
 and a special operation called "active block-commit", which allows
 live-merging an overlay disk image into its backing file.
 
diff --git a/docs/manpages/virt-sanlock-cleanup.rst 
b/docs/manpages/virt-sanlock-cleanup.rst
index 3ad70ce683..bf3ef6742b 100644
--- a/docs/manpages/virt-sanlock-cleanup.rst
+++ b/docs/manpages/virt-sanlock-cleanup.rst
@@ -75,5 +75,5 @@ PURPOSE
 SEE ALSO
 
 
-virsh(1), `online instructions `_,
+virsh(1), `online instructions `_,
 `https://libvirt.org/ `_
diff --git a/docs/manpages/virtinterfaced.rst b/docs/manpages/virtinterfaced.rst
index 247a8c4009..ef63f4c278 100644
--- a/docs/manpages/virtinterfaced.rst
+++ b/docs/manpages/virtinterfaced.rst
@@ -211,4 +211,3 @@ SEE ALSO
 
 virsh(1), libvirtd(8),
 `https://libvirt.org/daemons.html `_,
-`https://libvirt.org/drvinterface.html 
`_
diff --git a/docs/manpages/virtnetworkd.rst b/docs/manpages/virtnetworkd.rst
index 22b3fc0f2d..3bd1dd3221 100644
--- a/docs/manpages/virtnetworkd.rst
+++ b/docs/manpages/virtnetworkd.rst
@@ -211,4 +211,3 @@ SEE ALSO
 
 virsh(1), libvirtd(8),
 `https://libvirt.org/daemons.html `_,
-`https://libvirt.org/drvnetwork.html `_
diff --git a/docs/manpages/virtnwfilterd.rst b/docs/manpages/virtnwfilterd.rst
index b1fc45e7a2..1ec11c247f 100644
--- a/docs/manpages/virtnwfilterd.rst
+++ b/docs/manpages/virtnwfilterd.rst
@@ -211,4 +211,3 @@ SEE ALSO
 
 virsh(1), libvirtd(8),
 `https://libvirt.org/daemons.html `_,
-`https://libvirt.org/drvnwfilter.html `_
diff --git a/docs/manpages/virtstoraged.rst b/docs/manpages/virtstoraged.rst
index 70863282d1..fe966f3123 100644
--- a/docs/manpages/virtstoraged.rst
+++ b/docs/manpages/virtstoraged.rst
@@ -211,4 +211,4 @@ SEE ALSO
 
 virsh(1), libvirtd(8),
 `https://libvirt.org/daemons.html `_,
-`https://libvirt.org/drvstorage.html 

Re: [PATCH] virt-host-validate: Improve translatability of messages printed by 'virHostMsgCheck()'

[Michal Prívozník]

On 5/20/24 15:00, Peter Krempa wrote:
> Move the word 'Checking' into the appropriate formatting strings and
> mark all outstanding ones for translation.
> 
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/637
> Signed-off-by: Peter Krempa 
> ---
>  tools/virt-host-validate-bhyve.c  |  2 +-
>  tools/virt-host-validate-ch.c |  2 +-
>  tools/virt-host-validate-common.c | 18 +-
>  tools/virt-host-validate-qemu.c   |  4 ++--
>  4 files changed, 13 insertions(+), 13 deletions(-)

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH] fix virLogCleanerParseFilename logic

[Michal Prívozník]

On 5/19/24 17:40, David Negreira wrote:
> We should return the full filename path when we don't have a match on
> the third group of the regex.
> 
> Signed-off-by: David Negreira 
> ---
>  src/logging/log_cleaner.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/logging/log_cleaner.c b/src/logging/log_cleaner.c
> index 4ee91843aa..d8e6ce9cdd 100644
> --- a/src/logging/log_cleaner.c
> +++ b/src/logging/log_cleaner.c
> @@ -82,7 +82,7 @@ virLogCleanerParseFilename(const char *path,
>  *rotated_index = 0;
>  rotated_index_str = g_match_info_fetch(matchInfo, 3);
>  
> -if (!rotated_index_str)
> +if (rotated_index_str)
>  return chain_prefix;
>  
>  if (virStrToLong_i(rotated_index_str, NULL, 10, rotated_index) < 0) {

I'm not sure this is the right fix. If rotated_index_str is NOT NULL
chain_prefix is returned. Fair enough. But when it is NULL then it's
passed to virStrToLong_i() which does not seem right.

Also, do you have a minimalist reproducer?

Michal