Re: [libvirt PATCH 0/6] meson: Improve handling of tests

[Michal Prívozník]

On 10/3/23 16:56, Andrea Bolognani wrote:
> 
> 
> Andrea Bolognani (6):
>   meson: Do less when not building from git
>   meson: Move all handling of test options together
>   meson: Handle -Dtests=enabled with Clang
>   meson: Make -Dexpensive_tests depend on -Dtests
>   meson: Disable all tests when tests are disabled
>   meson: Rename build_tests -> tests_enabled
> 
>  build-aux/meson.build  |  99 ++--
>  meson.build|  75 +--
>  meson_options.txt  |   2 +-
>  src/access/meson.build |  16 ++--
>  src/meson.build| 204 +
>  5 files changed, 207 insertions(+), 189 deletions(-)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCH 00/11] systemd: Further improvements

[Michal Prívozník]

On 10/2/23 10:51, Andrea Bolognani wrote:
> This series addresses two pieces of feedback from my recent systemd
> changes: that some settings, such as LimitNOFILE, where still being
> repeated verbatim in multiple locations, and that only having the
> foo.{service,socket}.extra.in files for some services and not others
> could be confusing.
> 
> Andrea Bolognani (11):
>   systemd: libvirtd doesn't need @sockprefix@
>   systemd: Support merging multiple units
>   systemd: Accept multiple files for service_extra_in/socket_extra_in
>   systemd: Introduce systemd_service_limitnofile_extra
>   systemd: Introduce systemd_service_taskmax_extra
>   systemd: Introduce systemd_service_limitmemlock_extra
>   systemd: Introduce systemd_service_oomscoreadjust_extra
>   systemd: Allow comments at the top of units
>   systemd: Set service_extra_in/socket_extra_in everywhere
>   systemd: Make service_extra_in/socket_extra_in required
>   systemd: Tweak service definitions
> 

>  56 files changed, 248 insertions(+), 112 deletions(-)


Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCH 1/1] meson: Fix XDR check for GNU/Hurd

[Michal Prívozník]

On 10/24/23 18:54, Andrea Bolognani wrote:
> The situation is the same as Linux: since glibc no
> longer includes the RPC functionality, libtirpc must
> be used to complement it.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH libvirt v1 3/3] Improve `virsh create --console` behavior

[Michal Prívozník]

On 9/28/23 17:37, Marc Hartmayer wrote:
> When starting a guest via libvirt (`virsh create --console`), early
> console output was missed because the guest was started first and then
> the console was attached. This patch changes this to the following
> sequence:
> 
> 1. create a paused transient guest
> 2. attach the console
> 3. resume the guest
> 
> Reviewed-by: Boris Fiuczynski 
> Signed-off-by: Marc Hartmayer 
> ---
>  tools/virsh-domain.c | 34 --
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 36670039444c..2f055df0d97d 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -8212,6 +8212,13 @@ static const vshCmdOptDef opts_create[] = {
>  {.name = NULL}
>  };
>  
> +
> +static virshDomain *virDomainCreateXMLHelper(virConnectPtr conn, const char 
> *xmlDesc, unsigned int nfds, int *fds, unsigned int flags) {
> +  if (nfds)
> +  return virDomainCreateXMLWithFiles(conn, xmlDesc, nfds, fds, flags);
> +  return virDomainCreateXML(conn, xmlDesc, flags);
> +}
> +

s/virDomainCreate/virshDomainCreate/

Michal

Re: [PATCH libvirt v1 1/3] virsh: add `console --resume` support

[Michal Prívozník]

On 9/28/23 17:37, Marc Hartmayer wrote:
> This patch adds the command line flag `--resume` to the `virsh console`
> command. This resumes a paused guest after connecting to the console.
> This might be handy since it's a "common" pattern to start a guest
> paused, connect to the console, and then resume it so as not to miss any
> console messages.
> 
> Reviewed-by: Boris Fiuczynski 
> Signed-off-by: Marc Hartmayer 
> ---
>  tools/virsh-console.c |  8 
>  tools/virsh-console.h |  1 +
>  tools/virsh-domain.c  | 14 ++
>  3 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
> index 6bfb44a190ec..e44a070e7045 100644
> --- a/tools/virsh-console.c
> +++ b/tools/virsh-console.c
> @@ -401,6 +401,7 @@ int
>  virshRunConsole(vshControl *ctl,
>  virDomainPtr dom,
>  const char *dev_name,
> +const bool resume_domain,
>  unsigned int flags)
>  {
>  virConsole *con = NULL;
> @@ -476,6 +477,13 @@ virshRunConsole(vshControl *ctl,
>  goto cleanup;
>  }
>  
> +if (resume_domain) {
> +if (virDomainResume(dom) != 0) {
> +vshError(ctl, _("Failed to resume domain '%1$s'"), 
> virDomainGetName(dom));

Long line.

> +goto cleanup;
> +}
> +}
> +
>  while (!con->quit) {
>  if (virCondWait(>cond, >parent.lock) < 0) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> diff --git a/tools/virsh-console.h b/tools/virsh-console.h
> index e89484d24bf4..2d00ed90cf4a 100644
> --- a/tools/virsh-console.h
> +++ b/tools/virsh-console.h
> @@ -27,6 +27,7 @@
>  int virshRunConsole(vshControl *ctl,
>  virDomainPtr dom,
>  const char *dev_name,
> +const bool resume_domain,
>  unsigned int flags);
>  
>  #endif /* !WIN32 */
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 7abafe2ba30c..5c3c6d18aebf 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -3012,6 +3012,10 @@ static const vshCmdOptDef opts_console[] = {
>   .type = VSH_OT_BOOL,
>   .help =  N_("force console connection (disconnect already connected 
> sessions)")
>  },
> +{.name = "resume",
> + .type = VSH_OT_BOOL,
> + .help =  N_("resume a paused guest after connecting to console")
> +},

New options must go hand in hand with manpage update.

Michal

Re: [PATCH libvirt v1 0/3] Ensure full early console access with libvirt

[Michal Prívozník]

On 9/28/23 17:37, Marc Hartmayer wrote:
> Currently, early console output may be lost, e.g. if starting a guest with
> `virsh start --console` guest, which can make debugging of early failures very
> difficult
> (like zipl messages or disabled wait conditions happening early). This is
> because QEMU may emit serial console output before the libvirt console client
> starts to consume data from the pts. This can be prevented by starting the 
> guest
> in paused state, connect to the console and then resume the guest.
> 
> Note: There is still a problem in QEMU itself, see QEMU patch series `[PATCH]
> chardev/char-pty: Avoid losing bytes when the other side just (re-)connected`
> [1]
> 
> Changelog:
> RFCv1->v1:
> + rebased on current master
> + worked in comments from Daniel
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg02725.html
> 
> Marc Hartmayer (3):
>   virsh: add `console --resume` support
>   Improve `virsh start --console` behavior
>   Improve `virsh create --console` behavior
> 
>  tools/virsh-console.c |  8 
>  tools/virsh-console.h |  1 +
>  tools/virsh-domain.c  | 94 ---
>  3 files changed, 80 insertions(+), 23 deletions(-)
> 
> 
> base-commit: dd403f8873cf8de7675b89ed757a4228af7bc05e

All 'issues' I've raised are trivial. I've fixed them and pushed. Sorry
for leaving this to rot this long on the list.

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH libvirt v1 2/3] Improve `virsh start --console` behavior

[Michal Prívozník]

On 9/28/23 17:37, Marc Hartmayer wrote:
> When starting a guest via libvirt (`virsh start --console`), early
> console output was missed because the guest was started first and then
> the console was attached. This patch changes this to the following
> sequence:
> 
> 1. create a paused guest
> 2. attach the console
> 3. resume the guest
> 
> Reviewed-by: Boris Fiuczynski 
> Signed-off-by: Marc Hartmayer 
> ---
>  tools/virsh-domain.c | 50 +++-
>  1 file changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 5c3c6d18aebf..36670039444c 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -4059,12 +4059,27 @@ static const vshCmdOptDef opts_start[] = {
>  {.name = NULL}
>  };
>  
> +static int
> +virDomainCreateHelper(virDomainPtr dom, unsigned int nfds, int *fds,
> +  unsigned int flags)

To make naming less confusing, we prefer 'virsh' prefix in virsh instead
of 'vir'. It makes it obvious whether a function is a public libvirt API
(virDomainCreate...) or a virsh helper (virshDomainCreate...)

> +{
> +/* Prefer older API unless we have to pass a flag.  */
> +if (nfds > 0) {
> +return virDomainCreateWithFiles(dom, nfds, fds, flags);
> +} else if (flags != 0) {
> +return virDomainCreateWithFlags(dom, flags);
> +} else {
> +return virDomainCreate(dom);
> +}
> +}
> +
>  static bool
>  cmdStart(vshControl *ctl, const vshCmd *cmd)
>  {
>  g_autoptr(virshDomain) dom = NULL;
>  #ifndef WIN32
>  bool console = vshCommandOptBool(cmd, "console");
> +bool resume_domain = false;
>  #endif
>  unsigned int flags = VIR_DOMAIN_NONE;
>  int rc;
> @@ -4083,8 +4098,14 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
>  if (virshFetchPassFdsList(ctl, cmd, , ) < 0)
>  return false;
>  
> -if (vshCommandOptBool(cmd, "paused"))
> +if (vshCommandOptBool(cmd, "paused")) {
>  flags |= VIR_DOMAIN_START_PAUSED;
> +#ifndef WIN32
> +} else if (console) {
> +flags |= VIR_DOMAIN_START_PAUSED;
> +resume_domain = true;
> +#endif
> +}
>  if (vshCommandOptBool(cmd, "autodestroy"))
>  flags |= VIR_DOMAIN_START_AUTODESTROY;
>  if (vshCommandOptBool(cmd, "bypass-cache"))
> @@ -4096,12 +4117,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
>  
>  /* We can emulate force boot, even for older servers that reject it.  */
>  if (flags & VIR_DOMAIN_START_FORCE_BOOT) {
> -if (nfds > 0) {
> -rc = virDomainCreateWithFiles(dom, nfds, fds, flags);
> -} else {
> -rc = virDomainCreateWithFlags(dom, flags);
> -}
> -
> +rc = virDomainCreateHelper(dom, nfds, fds, flags);
>  if (rc == 0)
>  goto started;
>  
> @@ -4124,14 +4140,18 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
>  flags &= ~VIR_DOMAIN_START_FORCE_BOOT;
>  }
>  
> -/* Prefer older API unless we have to pass a flag.  */
> -if (nfds > 0) {
> -rc = virDomainCreateWithFiles(dom, nfds, fds, flags);
> -} else if (flags != 0) {
> -rc = virDomainCreateWithFlags(dom, flags);
> -} else {
> -rc = virDomainCreate(dom);
> +rc = virDomainCreateHelper(dom, nfds, fds, flags);
> +#ifndef WIN32
> +/* If the driver does not support the paused flag, let's fallback to the 
> old
> + * behavior without the flag. */
> +if (rc < 0 && resume_domain && last_error && last_error->code == 
> VIR_ERR_INVALID_ARG) {

Long line.

Michal

Re: [libvirt PATCH 1/2] ch: use payload api to send kernel details

[Michal Prívozník]

On 10/10/23 23:42, Praveen K Paladugu wrote:
> Starting with v28.0 cloud-hypervisor requires the use of "payload" api to pass
> kernel, initramfs and cmdline options. Extend ch driver to use the new
> api based on ch version.
> 
> Signed-off-by: Praveen K Paladugu 
> ---
>  src/ch/ch_capabilities.c | 55 
>  src/ch/ch_capabilities.h | 34 +
>  src/ch/ch_conf.h |  6 +
>  src/ch/ch_driver.c   |  3 +++
>  src/ch/ch_monitor.c  | 48 +++
>  src/ch/ch_monitor.h  |  4 ++-
>  src/ch/ch_process.c  |  2 +-
>  src/ch/meson.build   |  2 ++
>  8 files changed, 147 insertions(+), 7 deletions(-)
>  create mode 100644 src/ch/ch_capabilities.c
>  create mode 100644 src/ch/ch_capabilities.h
> 
> diff --git a/src/ch/ch_capabilities.c b/src/ch/ch_capabilities.c
> new file mode 100644
> index 00..b10485820c
> --- /dev/null
> +++ b/src/ch/ch_capabilities.c
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright Microsoft Corp. 2023
> + *
> + * ch_capabilities.h: CH capabilities
> + *
> + * 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 
> +#include "ch_capabilities.h"
> +
> +static void
> +virCHCapsSet(virBitmap *chCaps,
> +   virCHCapsFlags flag)
> +{
> +ignore_value(virBitmapSetBit(chCaps, flag));
> +}
> +
> +/**
> + * virCHCapsInitCHVersionCaps:
> + *
> + * Set all CH capabilities based on version of CH.
> + */
> +virBitmap *
> +virCHCapsInitCHVersionCaps(int version)
> +{
> +g_autoptr(virBitmap) chCaps = NULL;
> +chCaps = virBitmapNew(CH_CAPS_LAST);
> +
> +/* Version 28 deprecated kernel API:
> + * 
> https://github.com/cloud-hypervisor/cloud-hypervisor/releases/tag/v28.0
> + */
> +if (version >= 2800)
> +virCHCapsSet(chCaps, CH_KERNEL_API_DEPRCATED);
> +
> +
> +/* Starting Version 18, serial and console can be used in parallel */
> +if (version >= 1800)
> +virCHCapsSet(chCaps, CH_SERIAL_CONSOLE_IN_PARALLEL);

I wish there was a better way of checking for these facts. Historically,
we used to parse 'qemu -help' output which is a bit better than just
plain version check. OTOH, cloud hypervisor moves fast and probably
doesn't suffer the same problems as qemu, i.e. a lot of backports (even
features) with no version change. So I can live with this.

Michal

Re: [libvirt PATCH 0/2] ch: Use payload api

[Michal Prívozník]

On 10/10/23 23:42, Praveen K Paladugu wrote:
> Introduced virCHCapsInitCHVersionCaps to check and cache capabilities of
> cloud-hypervisor based on its version. Modified ch driver to use "payload" api
> to pass kernel, initramfs and cmdline options to cloud-hypervisor.
> 
> Cloud-hypervisor currently has a single LTS release, v28, that will be 
> supported
> until May 24. All versions older than v28 have reached EOL. Can I bump up the
> min version of cloud-hypervisor to v28 in ch driver?

Our policy is to wait a bit:

https://libvirt.org/platforms.html#linux-freebsd-and-macos

but I guess given that CH driver is 'best effort' for now, we can drop
the support for older releases.

> If I can bump up the min
> supported version in ch driver, I can drop a check in this commit related to
> supporting serial and console devices in parallel.
> 
> Praveen K Paladugu (2):
>   ch: use payload api to send kernel details
>   ch: support serial and console devices in parallel
> 
>  src/ch/ch_capabilities.c | 55 
>  src/ch/ch_capabilities.h | 34 +
>  src/ch/ch_conf.h |  6 +
>  src/ch/ch_domain.c   | 28 
>  src/ch/ch_driver.c   |  3 +++
>  src/ch/ch_monitor.c  | 48 +++
>  src/ch/ch_monitor.h  |  4 ++-
>  src/ch/ch_process.c  |  2 +-
>  src/ch/meson.build   |  2 ++
>  9 files changed, 165 insertions(+), 17 deletions(-)
>  create mode 100644 src/ch/ch_capabilities.c
>  create mode 100644 src/ch/ch_capabilities.h
> 

Reviewed-by: Michal Privoznik 

and pushed.

Michal

Re: [PATCH] util:hostcpu: Report physical address size based on Architecture

[Michal Prívozník]

On 10/4/23 07:58, Narayana Murty N wrote:
> The function virHostCPUGetPhysAddrSize was introduced with commit be1b7d5b18e
> fails on architectures other than x86 and SuperH. The commit 8417c1394cd4d
> fixed the issue only for s390 but the problem is still seen on other
> architectures like ppc which does not report Physical address size in their
> cpuinfo output.
> 
> command:
> systemctl restart libvirtd.service
> Output :
> 
> dnsmasq[2377]: read /var/lib/libvirt/dnsmasq/default.addnhosts - 0
> addresses
> dnsmasq-dhcp[2377]: read /var/lib/libvirt/dnsmasq/default.hostsfile
> libvirtd[3163]: libvirt version: 9.8.0
> libvirtd[3163]: hostname: xx
> libvirtd[3163]: internal error: Missing or invalid CPU address size in
> /proc/cpuinfo
>  libvirtd.service: Deactivated successfully.
>  
> 
> This patch fixes this issue by returning the size=0 for architectures
> other than x86 and SuperH.
> 
> Signed-off-by: Narayana Murty N 
> ---
>  src/util/virarch.h| 3 +++
>  src/util/virhostcpu.c | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 

Whoa, I had no idea that SH is still alive (an well?).

Reviewed-by: Michal Privoznik 

and pushed. Congratulations on your first libvirt contribution.

Michal

Re: [PATCH v2] lxc: fix lxcContainerMountAllFS() DEREF_BEFORE_CHECK

[Michal Prívozník]

On 9/7/23 11:04, Dmitry Frolov wrote:
> vmDef->fss[i]->src->path may be NULL,
> so check is needed before passing it to VIR_DEBUG.
> Also removed checking vmDef->fss[i]->src for NULL, since it may not be NULL.
> 
> Fixes: 57487085dc ("lxc: don't try to reference NULL when mounting 
> filesystems")
> 
> Signed-off-by: Dmitry Frolov 
> ---
>  src/lxc/lxc_container.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 21220661f7..9601061b23 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -1467,12 +1467,12 @@ static int lxcContainerMountAllFS(virDomainDef *vmDef,
>  if (STREQ(vmDef->fss[i]->dst, "/"))
>  continue;
>  
> -VIR_DEBUG("Mounting '%s' -> '%s'", vmDef->fss[i]->src->path, 
> vmDef->fss[i]->dst);
> +VIR_DEBUG("Mounting '%s' -> '%s'", 
> NULLSTR(vmDef->fss[i]->src->path), vmDef->fss[i]->dst);

Nitpick, this is long enough line that can be easily broken into two.

>  
>  if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0)
>  return -1;
>  
> -if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path &&
> +if (!(vmDef->fss[i]->src->path &&
>STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst)) &&
>  lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0)
>  return -1;

Reviewed-by: Michal Privoznik 

and pushed.

Michal

Re: [PATCH] Send event on persistent config modification

[Michal Prívozník]

On 10/19/23 16:20, Fima Shevrin wrote:
> Currently, libvirt doesn't send events when devices are attached,
> detached or updated. Thus, any services that listen to events are
> unaware of the change to persistent config.
> 
> Signed-off-by: Fima Shevrin 
> ---
>  src/qemu/qemu_driver.c | 25 -
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 43d96739d5..86da8da777 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7209,6 +7209,7 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObj *vm,
>  unsigned int flags)
>  {
>  qemuDomainObjPrivate *priv = vm->privateData;
> +virObjectEvent *event = NULL;
>  g_autoptr(virDomainDef) vmdef = NULL;
>  g_autoptr(virQEMUDriverConfig) cfg = NULL;
>  g_autoptr(virDomainDeviceDef) devConf = NULL;
> @@ -7292,6 +7293,12 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObj *vm,
>  return -1;
>  
>  virDomainObjAssignDef(vm, , false, NULL);
> +
> +/* Event sending if persistent config has changed */
> +event = virDomainEventLifecycleNewFromObj(vm,
> +  VIR_DOMAIN_EVENT_DEFINED,
> +  
> VIR_DOMAIN_EVENT_DEFINED_UPDATED);
> +virObjectEventStateQueue(driver->domainEventState, event);

Yeah, this is definitely better, but we have a more specific event:
VIR_DOMAIN_EVENT_ID_DEVICE_ADDED (virDomainEventDeviceAddedNewFromObj()).

Simirarly, we do have VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED
(virDomainEventDeviceRemovedNewFromObj()).

OTOH, both events assume device alias to be present (which is usually
not the case for inactive XML (except for user defined device aliases)).

In the end, your change makes sense.

Reviewed-by: Michal Privoznik 

and pushed. Congratulations on your first libvirt contribution!

Michal

Re: [PATCH 1/1] Checking the value returned by the function

[Michal Prívozník]

On 10/11/23 16:31, Sergey Mironov wrote:
> In version 0.10.0-rc0 
> (https://github.com/libvirt/libvirt/blob/v0.10.0-rc0/src/security/security_selinux.c
>  ) 11 years ago,
> the virSecuritySELinuxSetFileconHelper function appeared, which returned 1 if 
> the optional is true.
> Considering that at the moment the virSecuritySELinuxSetFilecon function (by 
> definition) can only return 0 or -1, I suggest removing the "dead code" in 
> the current patch.
> 
> Co-developed-by: sdl.qemu 
> Signed-off-by: Sergey Mironov 
> ---
>  src/security/security_selinux.c | 11 ---
>  1 file changed, 11 deletions(-)
> 
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 7914aba84d..a7abab9cf8 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1988,17 +1988,6 @@ 
> virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr,
>  ret = virSecuritySELinuxSetFilecon(mgr, path, use_label, remember);
>  }
>  
> -if (ret == 1 && !disk_seclabel) {
> -/* If we failed to set a label, but virt_use_nfs let us
> - * proceed anyway, then we don't need to relabel later.  */
> -disk_seclabel = virSecurityDeviceLabelDefNew(SECURITY_SELINUX_NAME);
> -if (!disk_seclabel)
> -return -1;
> -disk_seclabel->labelskip = true;
> -VIR_APPEND_ELEMENT(src->seclabels, src->nseclabels, disk_seclabel);
> -ret = 0;
> -}
> -
>  return ret;
>  }
>  

At this point, the @ret variable can be removed as well. But I can live
with that. What I can't live with is the commit message. Firstly,
there's no need to link the source file if you mention the actual commit
that introduced the function: 12b187fb956bffbc67a090dcda89e66450503a4e
and to make the hash more readable, just use git-describe (which in this
particular case yields some CVE-* tag, so pass --match=v\* to get
v0.10.0-rc0~108).

Nit-pick - mentioning the commit when a function was introduced is good,
but also, we are probably more interested in what commit turned these
lines into a dead code. Because without that the first piece of
information makes a great sensation (we have a dead code for 11 years!,
which is obviously not true).

Then there are more rigid customs we tend to hold:
1) if you look at 'git log --oneline' you'll see that majority (if not
all) commits are prefixed with something (usually the module they
change). Thus in this case it should have been "selinux: ".

2) Continuing with git log - "selinux: Checking the value ..." does not
really describe what's happening. You are dropping the check, not
introducing it.

I'm sorry if I sound too picky, but these rules help greatly downstream
maintainers when they need to figure out what each commit does.

Anyway, I'm fixing the commit message and pushing.

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH] fix error in printf format string

[Michal Prívozník]

On 10/17/23 08:43, zhujun2 wrote:
> use '%u' to printf unsigned int
> 

The commit subject and the body too are not very descriptive. If I were
to look at git log in a month or two I would have no idea what this
commit does.

> Signed-off-by: zhujun2 
> ---
>  examples/c/misc/event-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/examples/c/misc/event-test.c b/examples/c/misc/event-test.c
> index 813bca2699..07d86500cd 100644
> --- a/examples/c/misc/event-test.c
> +++ b/examples/c/misc/event-test.c
> @@ -969,7 +969,7 @@ myDomainEventMemoryFailureCallback(virConnectPtr conn 
> G_GNUC_UNUSED,
> void *opaque G_GNUC_UNUSED)
>  {
>  printf("%s EVENT: Domain %s(%d) memory failure: recipient '%d', "
> -   "aciont '%d', flags '%d'\n", __func__, virDomainGetName(dom),
> +   "aciont '%d', flags '%u'\n", __func__, virDomainGetName(dom),

We tend to print flags in a hexadecimal format: 0x%x because they are
not really a number rather than bit mask.

> virDomainGetID(dom), recipient, action, flags);
>  return 0;
>  }

I've fixed both commit message and the format and merged.

Reviewed-by: Michal Privoznik 

Congratulations on your first libvirt contribution!

Michal

Re: [libvirt PATCH] logging: lockdown the systemd service configuration

[Michal Prívozník]

On 9/26/23 18:11, Daniel P. Berrangé wrote:
>
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/logging/virtlogd.service.in | 94 +
>  1 file changed, 94 insertions(+)

Reviewed-by: Michal Privoznik 

Michal

ANNOUNCE: Virtualization linting library

[Michal Prívozník]

Users of virtualization may struggle with keeping their guest configuration
optimal. And while libvirt is perfect tool for its job, it merely just do
whatever it is told. For instance, it does not check whether there's enough of
free memory to start a guest. It just starts it and hope for the best.

This led management applications to have some code that performs (more or
less) similar checks. And this is my attempt to unify that code, move it out
of individual projects under this umbrella project. It's named virt-lint.
While it's written in Rust, it has C bindings from which bindings to other
languages can be written (I've written Golang so far).

I've created initial commit here:

https://gitlab.com/MichalPrivoznik/virt-lint

There is README.md which covers all the basic information.

So far, I've written four very basic checks (see src/validators.rs):

1) whether there exists set of NUMA nodes large enough to accommodate given
   domain,
2) whether there exists set of NUMA nodes with enough of free memory to
   accommodate given domain,
3) whether the virtualization node has suitable emulator binary for given
   domain,
4) if domain is of Q35 machine type, whether it has at least one
   pcie-root-port (for hotplugging).

Please keep in mind, these checks are just examples and possibly buggy.
Writing proper checks is part of future work, for now I want to gather
feedback on overall design, APIs, CLI, etc.

I've also built RPMs and copied them over here:

https://mprivozn.fedorapeople.org/rpms/virt-lint/

I appreciate any feedback.

Michal

Re: [PATCH] qemu: Add VIR_FREE in ADD_BITMAP

[Michal Prívozník]

On 9/28/23 13:55, Anastasia Belova wrote:
> virBitmapFormat returns the string that should be freed.
> 
> All strings in three ADD_BITMAP calls in qemuDomainGetGuestVcpusParams
> are contained in tmp. So memory leak is possible here without VIR_FREE.
> 
> Fixes: 3e7db8d3e8 ("Remove backslash alignment attempts")

Nothing in that commit suggests the VIR_FREE() was removed. In fact,
digging in our history it was commit
0108deb944af5ca6f1da350c9d0352c8ed18738b which removed the call.

Nit pick: I dislike short hashes because they are not unique. For
instance, this short has is 10 characters, but as our history grows
it'll need to grow too, otherwise it won't be unique. What is unique
though (and both human and machine readable) is: 'git describe --tags'
and if it's older commit then 'git describe --tags --contains'.

But the purpose of 'Fixes' in our commit messages is to help downstream
maintainers. For instance, when I'd be cherry picking that defect
commit, plain search through git log shows what other commits fix it.
Another reason against short hashes.

> Signed-off-by: Anastasia Belova 
> ---
>  src/qemu/qemu_driver.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9e0f204e44..a70bfb6450 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18420,6 +18420,7 @@ qemuDomainGetGuestVcpusParams(virTypedParameterPtr 
> *params,
>  goto cleanup; \
>  if (virTypedParamsAddString(, , , #name, tmp) < 0) \
>  goto cleanup; \
> +VIR_FREE(tmp); \

Nice catch!

We can use this opportunity to:

1) drop the trailing '\' as we don't really need the macro continue on
the next (empty) line,

2) drop the trailing ';' so that ...

>  
>  ADD_BITMAP(vcpus);
>  ADD_BITMAP(online);

... these calls can retain their semicolons (and in fact enforce them).

Reviewed-by: Michal Privoznik 

and pushed.

Michal

Re: [PATCH] virsh: Account for return values in virNodeGetFreePages

[Michal Prívozník]

On 9/26/23 15:56, Martin Kletzander wrote:
> The function returns how many array items were filled in, but virsh
> never checked for anything other than errors.  Just to make sure this
> does not report invalid data, even though the only possibility would be
> reporting 0 free pages, check the returned data so that possible errors
> are detected.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  tools/virsh-host.c | 28 ++--
>  1 file changed, 22 insertions(+), 6 deletions(-)

Yeah. This should never happen though (at least in real life
conditions), because our virHostMemGetFreePages() either fills
everything or returns an error. But the way our public API is documented
warrants having this in.

Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCH] gitpublish: Add suppresscc option

[Michal Prívozník]

On 9/19/23 11:34, Erik Skultety wrote:
> send-email scans the commit messages to figure out the default set of
> addresses to put into CC, Acked-by/Reviewed-by, etc-by being among
> them. We're quite strict about CC-ing people on libvirt-list, since
> most developers are subscribed to the list anyway. Respect the rule by
> avoiding CCing people solely based on the fact that they've done review
> of any of previous revisions.
> 
> Signed-off-by: Erik Skultety 
> ---
> 
> I noticed this issue when sending 
> https://listman.redhat.com/archives/libvir-list/2023-September/242173.html
> and publish automatically included Dan. I guess I could have overridden the CC
> explicitly to just include my own address and reduce the noise, but was too
> fast to hit send.
> 
>  .gitpublish | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCH 0/2] Add vdpablock and nbdkit to NEWS

[Michal Prívozník]

On 9/19/23 22:47, Jonathon Jongsma wrote:
> 
> 
> Jonathon Jongsma (2):
>   news: document support for vdpa block devices
>   news: document nbdkit support for network disks
> 
>  NEWS.rst | 18 ++
>  1 file changed, 18 insertions(+)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH] test: Fix testNodeGetFreePages

[Michal Prívozník]

On 9/26/23 14:20, Martin Kletzander wrote:
> The function is supposed to return the number of items filled into the
> array and not zero.  Also change the initialization of the "randomness"
> to be based on the startCell so that the values are different for each
> cell even for separate calls.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  src/test/test_driver.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index c962aa74786e..998d102ddc5a 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -4503,7 +4503,7 @@ testNodeGetFreePages(virConnectPtr conn G_GNUC_UNUSED,
>   unsigned int flags)
>  {
>  size_t i = 0, j = 0;
> -int x = 6;
> +int x = startCell * 6;

So startCell is now used within the function and thus its G_GNUC_UNUSED
annotation should be dropped.

>  
>  virCheckFlags(0, -1);
>  
> @@ -4514,7 +4514,7 @@ testNodeGetFreePages(virConnectPtr conn G_GNUC_UNUSED,
>  }
>  }
>  
> -return 0;
> +return cellCount * npages;
>  }
>  
>  static int testDomainCreateWithFlags(virDomainPtr domain, unsigned int flags)

Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCH] qemu: Improve error message for failed firmware autoselection

[Michal Prívozník]

On 9/22/23 15:26, Andrea Bolognani wrote:
> The current message can be misleading, because it seems to suggest
> that no firmware of the requested type is available on the system.
> 
> What actually happens most of the time, however, is that despite
> having multiple firmwares of the right type to choose from, none
> of them is suitable because of lacking some specific feature or
> being incompatible with some setting that the user has explicitly
> enabled.
> 
> Providing an error message that describes exactly the problem is
> not feasible, since we would have to list each candidate along
> with the reason why we rejected it, which would get out of hand
> quickly.
> 
> As a small but hopefully helpful improvement over the current
> situation, reword the error message to make it clearer that the
> culprit is not necessarily the firmware type, but rather the
> overall domain configuration.
> 
> Suggested-by: Michael Kjörling <7d1340278...@ewoof.net>
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_firmware.c| 2 +-
>  .../firmware-auto-efi-loader-path-nonstandard.x86_64-latest.err | 2 +-
>  ...rmware-auto-efi-nvram-template-nonstandard.x86_64-latest.err | 2 +-
>  .../firmware-auto-efi-rw-abi-update.x86_64-latest.err   | 2 +-
>  tests/qemuxml2argvdata/firmware-auto-efi-rw.x86_64-latest.err   | 2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Michal Privoznik 

And maybe we can document what us, libvirt developers, would do to debug
this? I mean, we can put somewhere in a kbase article that we would
enable debug logs and then check log messages where individual reasons
for rejecting each fw are logged.

Michal

Re: [PATCH] libxl: Fix Domain-0 ballooning logic

[Michal Prívozník]

On 9/18/23 19:16, Jim Fehlig wrote:
> When Domain-0 autoballooning is enabled, it's possible that memory may
> need to be ballooned down in Domain-0 to accommodate the needs of another
> virtual machine. libxlDomainFreeMemory handles this task, but due to a
> logic bug is underflowing the variable containing Domain-0 new
> target memory. The resulting huge numbers are filtered by
> libxlSetMemoryTargetWrapper and memory is not changed.
> 
> Under the covers, libxlDomainFreeMemory uses Xen's libxl_set_memory_target
> API, which includes a 'relative' parameter for specifying how to set the
> target. If true, the target is an increment/decrement value over the
> current memory, otherwise target is taken as an absolute value.
> libxlDomainFreeMemory sets 'relative' to true, but never allows for
> negative values by declaring the target memory variable as an unsigned.
> Fix by declaring the variable as signed, which also requried adjusting
> libxlSetMemoryTargetWrapper.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  src/libxl/libxl_api_wrapper.h | 16 ++--
>  src/libxl/libxl_domain.c  |  2 +-
>  2 files changed, 7 insertions(+), 11 deletions(-)

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH 02/16] conf: Add parser logic for nwfilter metadata fields

[Michal Prívozník]

On 9/3/23 17:49, K Shiva Kiran wrote:
> Signed-off-by: K Shiva Kiran 
> ---
>  src/conf/nwfilter_conf.c | 30 ++
>  src/conf/nwfilter_conf.h |  5 +
>  2 files changed, 35 insertions(+)

Here, I'd also expect a test case. Either introduce a new test case to
tests/nwfilterxml2xmltest.c (preferred), or modify an existing one. This
not only verifies the parsing/formatting works, but also that our schema
is valid.

Michal

Re: [PATCH 03/16] nwfilter: Add enum to operate on user defined metadata

[Michal Prívozník]

On 9/3/23 17:49, K Shiva Kiran wrote:
> Adds enum `virNWFilterMetadataType` to choose between ``,
> `` or ``.
> 
> Signed-off-by: K Shiva Kiran 
> ---
>  include/libvirt/libvirt-nwfilter.h | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/libvirt/libvirt-nwfilter.h 
> b/include/libvirt/libvirt-nwfilter.h
> index 33b842b464..a9930a1e66 100644
> --- a/include/libvirt/libvirt-nwfilter.h
> +++ b/include/libvirt/libvirt-nwfilter.h
> @@ -159,4 +159,18 @@ int 
> virNWFilterBindingDelete(virNWFilterBindingPtr binding);
>  int virNWFilterBindingRef(virNWFilterBindingPtr binding);
>  int virNWFilterBindingFree(virNWFilterBindingPtr 
> binding);
>  
> +/**
> + * virNWFilterMetadataType:
> + *
> + * Since: 9.8.0
> + */
> +typedef enum {
> +VIR_NWFILTER_METADATA_DESCRIPTION = 0, /* Operate on  
> (Since: 9.8.0) */
> +VIR_NWFILTER_METADATA_TITLE   = 1, /* Operate on  (Since: 
> 9.8.0) */
> +VIR_NWFILTER_METADATA_ELEMENT = 2, /* Operate on  (Since: 
> 9.8.0) */
> +
> +# ifdef VIR_ENUM_SENTINELS
> +VIR_NWFILTER_METADATA_LAST /* (Since: 9.8.0) */
> +# endif
> +} virNWFilterMetadataType;
>  #endif /* LIBVIRT_NWFILTER_H */

This patch alone makes no sense. Nobody will ever want to backport just
this typedef. Merge it into 05/16.

Michal

Re: [PATCH 04/16] nwfilter: Add error code and message for missing metadata

[Michal Prívozník]

On 9/3/23 17:49, K Shiva Kiran wrote:
> Signed-off-by: K Shiva Kiran 
> ---
>  include/libvirt/virterror.h | 1 +
>  src/util/virerror.c | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
> index 224eddc9e4..ffb47a3242 100644
> --- a/include/libvirt/virterror.h
> +++ b/include/libvirt/virterror.h
> @@ -349,6 +349,7 @@ typedef enum {
>  VIR_ERR_CHECKPOINT_INCONSISTENT = 109, /* checkpoint can't be used 
> (Since: 6.10.0) */
>  VIR_ERR_MULTIPLE_DOMAINS = 110, /* more than one matching domain 
> found (Since: 7.1.0) */
>  VIR_ERR_NO_NETWORK_METADATA = 111,  /* Network metadata is not present 
> (Since: 9.7.0) */
> +VIR_ERR_NO_NWFILTER_METADATA = 112,  /* NWFilter metadata is not present 
> (Since: 9.8.0) */
>  
>  # ifdef VIR_ENUM_SENTINELS
>  VIR_ERR_NUMBER_LAST /* (Since: 5.0.0) */
> diff --git a/src/util/virerror.c b/src/util/virerror.c
> index 227a182417..a1d0d73e5d 100644
> --- a/src/util/virerror.c
> +++ b/src/util/virerror.c
> @@ -1290,6 +1290,9 @@ static const virErrorMsgTuple virErrorMsgStrings[] = {
>  [VIR_ERR_NO_NETWORK_METADATA] = {
>  N_("metadata not found"),
>  N_("metadata not found: %1$s") },
> +[VIR_ERR_NO_NWFILTER_METADATA] = {
> +N_("metadata not found"),
> +N_("metadata not found: %1$s") },
>  };
>  
>  G_STATIC_ASSERT(G_N_ELEMENTS(virErrorMsgStrings) == VIR_ERR_NUMBER_LAST);

Again, this patch alone makes no sense. Merge it into 05/16.

Michal

Re: [PATCH 01/16] xml: Add , and to nwfilter xml schema

[Michal Prívozník]

On 9/3/23 17:49, K Shiva Kiran wrote:
> Signed-off-by: K Shiva Kiran 
> ---
>  docs/formatnwfilter.rst   | 31 +++
>  src/conf/schemas/nwfilter.rng |  9 +
>  2 files changed, 40 insertions(+)

This patch alone does not make any sense. Merge it into 02/16.

Michal

Re: [PATCH 07/16] nwfilter: Implement RPC

[Michal Prívozník]

On 9/3/23 17:49, K Shiva Kiran wrote:
> Signed-off-by: K Shiva Kiran 
> ---
>  src/remote/remote_driver.c   |  2 ++
>  src/remote/remote_protocol.x | 34 +-
>  src/remote_protocol-structs  | 19 +++
>  3 files changed, 54 insertions(+), 1 deletion(-)

You'll need to rebase this as the master has moved.

Michal

Re: [PATCH 06/16] nwfilter: Introduce public API to retrieve user-defined metadata

[Michal Prívozník]

On 9/3/23 17:49, K Shiva Kiran wrote:
> Introduces `virNWFilterGetMetadata()` to retrieve user defined
> metadata fields, i.e ``, `` and ``
> 
> Signed-off-by: K Shiva Kiran 
> ---
>  include/libvirt/libvirt-nwfilter.h |  5 +++
>  src/driver-nwfilter.h  |  6 +++
>  src/libvirt-nwfilter.c | 67 ++
>  src/libvirt_public.syms|  1 +
>  4 files changed, 79 insertions(+)

Alright, introducing setter and getter in two different APIs is a bit of
a stretch but I can live with it. Somebody might want just the setter
and use nwfilter-dumpxml as getter.

But then you implement RPC for both of these APIs in a single patch
(07/16). Therefore, please merge also 05 and 06.

Michal

Re: [PATCH 08/16] virsh: Add helper method to retrieve xml from NWFilter def

[Michal Prívozník]

On 9/3/23 17:49, K Shiva Kiran wrote:
> Signed-off-by: K Shiva Kiran 
> ---
>  tools/virsh-util.c | 25 +
>  tools/virsh-util.h |  9 +
>  2 files changed, 34 insertions(+)
> 


> diff --git a/tools/virsh-util.h b/tools/virsh-util.h
> index 2386847072..4cad3d7eb9 100644
> --- a/tools/virsh-util.h
> +++ b/tools/virsh-util.h
> @@ -152,6 +152,15 @@ virshNetworkGetXMLFromNet(vshControl *ctl,
>  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4)
>  ATTRIBUTE_NONNULL(5) G_GNUC_WARN_UNUSED_RESULT;
>  
> +int
> +virshNWFilterGetXMLFromNWFilter(vshControl *ctl,
> +virNWFilterPtr nwfilter,
> +unsigned int flags,
> +xmlDocPtr *xml,
> +xmlXPathContextPtr *ctxt)
> +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4)
> +ATTRIBUTE_NONNULL(5) G_GNUC_WARN_UNUSED_RESULT;
> +

Looking into the future, there's just one user of this function. Please
consider merging this patch into the next one.

Michal

Re: [PATCH 09/16] virsh: Add new command `nwfilter-desc`

[Michal Prívozník]

On 9/3/23 17:49, K Shiva Kiran wrote:
> This command can be used to view/modify the `` and
> `` fields of the NWFilter object.
> 
> Signed-off-by: K Shiva Kiran 
> ---
>  tools/virsh-nwfilter.c | 209 +
>  1 file changed, 209 insertions(+)

The documentation you're introducing in 12/16 should be introduced here.

Michal

Re: [PATCH 00/16] nwfilter: Add support for user defined metadata

[Michal Prívozník]

On 9/3/23 17:49, K Shiva Kiran wrote:
> This patchset adds support for the following user defined metadata
> fields for network filters.
> 
> - title: A short description of the filter.
> - description: Any documentation that the user wants to store.
> - metadata: Other metadata in XML form.
> 
> K Shiva Kiran (16):
>   xml: Add ,  and  to nwfilter xml schema
>   conf: Add parser logic for nwfilter metadata fields
>   nwfilter: Add enum to operate on user defined metadata
>   nwfilter: Add error code and message for missing metadata
>   nwfilter: Introduce public API to modify user metadata
>   nwfilter: Introduce public API to retrieve user-defined metadata
>   nwfilter: Implement RPC
>   virsh: Add helper method to retrieve xml from NWFilter def
>   virsh: Add new command `nwfilter-desc`
>   virsh: Add new command `nwfilter-metadata`
>   virsh: Add option --title for nwfilter-list
>   docs: Document nwfilter metadata related commands
>   virnwfilterobj: Add virNWFilterObjGetMetadata()
>   virnwfilterobj: Add virNWFilterObjSetMetadata()
>   nwfilter_driver: Add Driver implementation for metadata
>   NEWS: Introduce user-defined metadata fields for NWFilter object
> 
>  NEWS.rst   |  18 ++
>  docs/formatnwfilter.rst|  31 +++
>  docs/manpages/virsh.rst|  98 +++-
>  include/libvirt/libvirt-nwfilter.h |  27 ++
>  include/libvirt/virterror.h|   1 +
>  src/conf/nwfilter_conf.c   |  30 +++
>  src/conf/nwfilter_conf.h   |   5 +
>  src/conf/schemas/nwfilter.rng  |   9 +
>  src/conf/virnwfilterobj.c  | 148 +++
>  src/conf/virnwfilterobj.h  |  13 +
>  src/driver-nwfilter.h  |  15 ++
>  src/libvirt-nwfilter.c | 154 
>  src/libvirt_private.syms   |   2 +
>  src/libvirt_public.syms|   6 +
>  src/nwfilter/nwfilter_driver.c |  61 +
>  src/remote/remote_driver.c |   2 +
>  src/remote/remote_protocol.x   |  34 ++-
>  src/remote_protocol-structs|  19 ++
>  src/util/virerror.c|   3 +
>  tools/virsh-nwfilter.c | 387 -
>  tools/virsh-util.c |  25 ++
>  tools/virsh-util.h |   9 +
>  22 files changed, 1089 insertions(+), 8 deletions(-)
> 

The code looks okay. But the split into patches is a bit awkward. Also,
since I've merged your other series, there's a conflict in RPC
definition file.

Looking forward to v2.

Michal

Re: [PATCH 3/8] Add Event ID, Server side dispatcher and virsh print function

[Michal Prívozník]

On 9/3/23 16:58, K Shiva Kiran wrote:
> Adds the following for Network Metadata change callback events:
> 
> - New Event ID VIR_NETWORK_EVENT_ID_METADATA_CHANGE
> - Server side dispatcher
> 
> virsh:
> - New event type `metadata-change`
> - vshEventMetadataChangePrint() to display the above defined
>   event type in virsh via `net-event`
> 
> Signed-off-by: K Shiva Kiran 
> ---
> I was unable to split this patch due to static assertions (perfomed
> against VIR_NETWORK_EVENT_ID_LAST) in remote_daemon_dispatch.c and
> virsh-network.c
> Please let me know if there is a way to split patches in such cases.
> 
>  include/libvirt/libvirt-network.h   |  1 +
>  src/conf/network_event.c| 12 
>  src/remote/remote_daemon_dispatch.c | 38 
>  src/remote/remote_protocol.x| 15 +-
>  src/remote_protocol-structs |  6 
>  tools/virsh-network.c   | 46 -
>  6 files changed, 116 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-network.h 
> b/include/libvirt/libvirt-network.h
> index 4b121ae0e7..58591be7ac 100644
> --- a/include/libvirt/libvirt-network.h
> +++ b/include/libvirt/libvirt-network.h
> @@ -330,6 +330,7 @@ typedef void 
> (*virConnectNetworkEventLifecycleCallback)(virConnectPtr conn,
>   */
>  typedef enum {
>  VIR_NETWORK_EVENT_ID_LIFECYCLE = 0,   /* 
> virConnectNetworkEventLifecycleCallback (Since: 1.2.1) */
> +VIR_NETWORK_EVENT_ID_METADATA_CHANGE = 1,   /* 
> virConnectNetworkEventMetadataChangeCallback (Since: 9.8.0) */
>  
>  # ifdef VIR_ENUM_SENTINELS
>  VIR_NETWORK_EVENT_ID_LAST
> diff --git a/src/conf/network_event.c b/src/conf/network_event.c
> index 51fa092ffd..d1b3aa5721 100644
> --- a/src/conf/network_event.c
> +++ b/src/conf/network_event.c
> @@ -118,6 +118,18 @@ virNetworkEventDispatchDefaultFunc(virConnectPtr conn,
>  return;
>  }
>  
> +case VIR_NETWORK_EVENT_ID_METADATA_CHANGE:
> +{
> +virNetworkEventMetadataChange *metadataChangeEvent;
> +
> +metadataChangeEvent = (virNetworkEventMetadataChange *)event;
> +((virConnectNetworkEventMetadataChangeCallback)cb)(conn, net,
> +   
> metadataChangeEvent->type,
> +   
> metadataChangeEvent->nsuri,
> +   cbopaque);
> +return;
> +}
> +
>  case VIR_NETWORK_EVENT_ID_LAST:
>  break;
>  }
> diff --git a/src/remote/remote_daemon_dispatch.c 
> b/src/remote/remote_daemon_dispatch.c
> index 2bb9e306a4..7daf503b51 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -1385,8 +1385,46 @@ remoteRelayNetworkEventLifecycle(virConnectPtr conn,
>  return 0;
>  }
>  
> +static int
> +remoteRelayNetworkEventMetadataChange(virConnectPtr conn,
> +  virNetworkPtr net,
> +  int type,
> +  const char *nsuri,
> +  void *opaque)
> +{
> +daemonClientEventCallback *callback = opaque;
> +remote_network_event_callback_metadata_change_msg data;
> +
> +if (callback->callbackID < 0 ||
> +!remoteRelayNetworkEventCheckACL(callback->client, conn, net))
> +return -1;
> +
> +VIR_DEBUG("Relaying network metadata change %s %d %s, callback %d",
> +  net->name, type, NULLSTR(nsuri), callback->callbackID);
> +
> +/* build return data */
> +memset(, 0, sizeof(data));

Declaring the variable with = { 0 } initializer is preferred (see
v9.7.0-rc1~160).

> +
> +data.type = type;
> +if (nsuri) {
> +data.nsuri = g_new0(remote_nonnull_string, 1);
> +*(data.nsuri) = g_strdup(nsuri);
> +}
> +
> +make_nonnull_network(, net);
> +data.callbackID = callback->callbackID;
> +
> +remoteDispatchObjectEventSend(callback->client, callback->program,
> +  
> REMOTE_PROC_NETWORK_EVENT_CALLBACK_METADATA_CHANGE,
> +  
> (xdrproc_t)xdr_remote_network_event_callback_metadata_change_msg,
> +  );
> +return 0;
> +}
> +
> +
>  static virConnectNetworkEventGenericCallback networkEventCallbacks[] = {
>  VIR_NETWORK_EVENT_CALLBACK(remoteRelayNetworkEventLifecycle),
> +VIR_NETWORK_EVENT_CALLBACK(remoteRelayNetworkEventMetadataChange),
>  };
>  
>  G_STATIC_ASSERT(G_N_ELEMENTS(networkEventCallbacks) == 
> VIR_NETWORK_EVENT_ID_LAST);
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index 7ff059e393..e295b0acc3 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -3323,6 +3323,13 @@ struct remote_network_event_lifecycle_msg {
>  int detail;
>  };
>  
> 

Re: [PATCH 4/8] Add methods to create Metadata change events

[Michal Prívozník]

On 9/3/23 16:58, K Shiva Kiran wrote:
> Adds two new private methods to create metadata change events:
> - virNetworkEventMetadataChangeNewFromNet()
> - virNetworkEventMetadataChangeNewFromObj()
> 
> Signed-off-by: K Shiva Kiran 
> ---
>  src/conf/network_event.c | 48 
>  src/conf/network_event.h | 11 +
>  src/libvirt_private.syms |  2 ++
>  3 files changed, 61 insertions(+)
> 
> diff --git a/src/conf/network_event.c b/src/conf/network_event.c
> index d1b3aa5721..5eb9c3d48f 100644
> --- a/src/conf/network_event.c
> +++ b/src/conf/network_event.c
> @@ -267,3 +267,51 @@ virNetworkEventMetadataChangeDispose(void *obj)
>  
>  g_free(event->nsuri);
>  }
> +
> +
> +static virObjectEvent *
> +virNetworkEventMetadataChangeNew(const char *name,
> + unsigned char *uuid,
> + int type,
> + const char *nsuri)
> +{
> +virNetworkEventMetadataChange *event;
> +char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> +if (virNetworkEventsInitialize() < 0)
> +return NULL;
> +
> +virUUIDFormat(uuid, uuidstr);
> +if (!(event = virObjectEventNew(virNetworkEventMetadataChangeClass,
> +virNetworkEventDispatchDefaultFunc,
> +VIR_NETWORK_EVENT_ID_METADATA_CHANGE,
> +0, name, uuid, uuidstr)))
> +return NULL;
> +
> +event->type = type;
> +if (nsuri)
> +event->nsuri = g_strdup(nsuri);

This check if effectively a dead code. The first thing g_strdup() does
is check for NULL. There are a few places in our code where this pattern
occurs, I'll be sending a patch that fixes them.

> +
> +return (virObjectEvent *)event;
> +}
> +

Michal

Re: [PATCH 0/8] network: Add callback for metadata changes

[Michal Prívozník]

On 9/3/23 16:58, K Shiva Kiran wrote:
> This patchset adds support to trigger an event upon changes in the
> content of ``, `` or `` in the network.
> 
> K Shiva Kiran (8):
>   Define Network Metadata change callback function
>   Define Network event struct for Metadata change
>   Add Event ID, Server side dispatcher and virsh print function
>   Add methods to create Metadata change events
>   Implement RPC Client for Network Metadata change callbacks
>   Test driver implementation for Network metadata change callbacks
>   Add Bridge Driver implementation for Network metadata change callbacks
>   examples: Add Testing for metadata change callbacks
> 
>  examples/c/misc/event-test.c| 85 ++---
>  include/libvirt/libvirt-network.h   | 22 
>  src/conf/network_event.c| 84 
>  src/conf/network_event.h| 11 
>  src/libvirt_private.syms|  2 +
>  src/network/bridge_driver.c |  6 ++
>  src/remote/remote_daemon_dispatch.c | 38 +
>  src/remote/remote_driver.c  | 31 +++
>  src/remote/remote_protocol.x| 15 -
>  src/remote_protocol-structs |  6 ++
>  src/test/test_driver.c  |  7 +++
>  tools/virsh-network.c   | 46 +++-
>  12 files changed, 342 insertions(+), 11 deletions(-)
> 

Couple of points:

Patches 1-3 can be squashed together. There is no value in having them
separate (nobody will ever backport just the callback function definition).

Commit message subject usually have a prefix (see git log) respecting
the part of the code they are changing. For instance, instead of:

  Add Bridge Driver implementation for Network metadata change callbacks

we would have:

  network: Implement network metadata change callbacks

or:

  bridge_driver: Implement network metadata change callbacks

This makes it easy to "pattern match" when skimming through git log.


Nevertheless, these are small nits and I'll fix hem before merging.


Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH] fix uint64 underflow

[Michal Prívozník]

On 9/12/23 16:50, Dmitry Frolov wrote:
> According to previous statement,
> 'free_mem' is less than 'needed_mem'.
> So, the subtraction 'free_mem - needed_mem' is negative,
> and will raise uint64 underflow.
> 
> Signed-off-by: Dmitry Frolov 
> ---
>  src/libxl/libxl_domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 6c167df63e..36be042971 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -940,7 +940,7 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config 
> *d_config)
>  if (free_mem >= needed_mem)
>  return 0;
>  
> -target_mem = free_mem - needed_mem;
> +target_mem = needed_mem - free_mem;
>  if (libxlSetMemoryTargetWrapper(ctx, 0, target_mem,
>  /* relative */ 1, 0) < 0)
>  goto error;

Yeah, this fixes the underflow, but I worry about the while semantics a
bit. What the code effectively does:

  libxl_domain_need_memory(_mem);

  do {
libxl_get_free_memory(_mem);

if (free_mem >= needed_mem)
  return 0;

target_mem = needed_mem - free_mem;
libxl_set_memory_target(target_mem);
  } while(...)

Now, if libxl_domain_need_memory() returns how much memory a domain
really needs, then why undergo trouble of getting its free memory? Why
not pass it to set_memory_target() right away?

Or am I misunderstanding something?

Michal

Re: [PATCH] qemuxml2argvtest: Fix tests failing on none x86 host CPUs

[Michal Prívozník]

On 9/14/23 15:15, Boris Fiuczynski wrote:
> Since commit 54257ed51b51 on S390x qemuxml2argvtest fails with the following 
> errors:
> 
> 144) QEMU XML-2-ARGV cpu-kvmclock.x86_64-latest... 
> libvirt: CPU Driver error : the CPU is incompatible with host CPU: Host CPU 
> does not provide required features: monitor
> FAILED
> 
> 2023-09-14 13:01:23.883+: 4113077: info : libvirt version: 9.8.0
> 2023-09-14 13:01:23.883+: 4113077: info : hostname: a46lp61.lnxne.boe
> 2023-09-14 13:01:23.883+: 4113077: error : virCPUx86Compare:1954 : the 
> CPU is incompatible with host CPU: Host CPU does not provide required 
> features: monitor
> 
> 1059) QEMU XML-2-ARGV cpu-check-partial.x86_64-latest   ... 
> libvirt: CPU Driver error : the CPU is incompatible with host CPU: Host CPU 
> does not provide required features: monitor
> FAILED
> 
> 2023-09-14 13:01:23.885+: 4113077: error : virCPUx86Compare:1954 : the 
> CPU is incompatible with host CPU: Host CPU does not provide required 
> features: monitor
> 
> 1064) QEMU XML-2-ARGV cpu-check-default-partial2.x86_64-latest  ... 
> libvirt: CPU Driver error : the CPU is incompatible with host CPU: Host CPU 
> does not provide required features: monitor
> FAILED
> 
> 2023-09-14 13:01:23.885+: 4113077: error : virCPUx86Compare:1954 : the 
> CPU is incompatible with host CPU: Host CPU does not provide required 
> features: monitor
> 
> 3 tests failed.
> 
> Fixes: 54257ed51b5132032cedb7e1e7b8c34b9ae52115
> Signed-off-by: Boris Fiuczynski 
> ---
>  tests/qemuxml2argvtest.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Michal Privoznik 

and pushed.

Michal

Re: [sdl-qemu] [PATCH 0/1] There are no checks, virDomainChrSourceDefNew can return 0

[Michal Prívozník]

On 9/14/23 11:44, Миронов Сергей Владимирович wrote:
> There are no checks, virDomainChrSourceDefNew can return 0.
> 
> 
> Return value of a function 'virDomainChrSourceDefNew'
> 
> is dereferenced at qemu_hotplug.c without checking for NULL,
> 
> but it is usually checked for this function.
> 
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> 
> Fixes: 1f85f0967b ("ci: jobs.sh: Add back '--no-suite syntax-check 
> --print-errorlogs'")
> 
> Signed-off-by: Sergey Mironov 
> 
> ---
> src/qemu/qemu_hotplug.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 177ca87d11..09e16c2c7e 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3207,6 +3207,8 @@ qemuDomainAttachFSDevice(virQEMUDriver *driver,
>  qemuAssignDeviceFSAlias(vm->def, fs);
> 
>  chardev = virDomainChrSourceDefNew(priv->driver->xmlopt);
> +if (chardev == NULL)
> +   goto cleanup;
>  chardev->type = VIR_DOMAIN_CHR_TYPE_UNIX;
>  chardev->data.nix.path = qemuDomainGetVHostUserFSSocketPath(priv, fs);

Apart from Peter's findings, there are more cases like this:
qemuBuildVideoCommandLine(), qemuVirtioFSOpenChardev(),
qemuMonitorJSONTestAttachChardev().

Might be worth fixing them too.

Michal

Re: [libvirt PATCHv1 1/8] qemu: fix indentation

[Michal Prívozník]

On 9/11/23 15:51, Ján Tomko wrote:
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_validate.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 

You could at least mention in which function, because I doubt this is
the only misalignment we have in src/qemu/. Or in that file alone.

Michal

Re: [libvirt PATCHv1 3/8] conf: move idmap parsing earlier

[Michal Prívozník]

On 9/11/23 15:51, Ján Tomko wrote:
> Signed-off-by: Ján Tomko 
> ---
>  src/conf/domain_conf.c | 98 +-
>  1 file changed, 49 insertions(+), 49 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2c8727de54..dd67e7f21b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8581,6 +8581,55 @@ virDomainNetGenerateMAC(virDomainXMLOption *xmlopt,
>  }
>  
>  
> +static int virDomainIdMapEntrySort(const void *a, const void *b)
> +{
> +const virDomainIdMapEntry *entrya = a;
> +const virDomainIdMapEntry *entryb = b;
> +
> +if (entrya->start > entryb->start)
> +return 1;
> +else if (entrya->start < entryb->start)
> +return -1;
> +else
> +return 0;
> +}
> +

Please put an extra empty line here.

> +/* Parse the XML definition for user namespace id map.
> + *
> + * idmap has the form of
> + *
> + *   
> + *   
> + */
> +static virDomainIdMapEntry *
> +virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt,
> +  xmlNodePtr *node,
> +  size_t num)
> +{
> +size_t i;
> +virDomainIdMapEntry *idmap = NULL;
> +VIR_XPATH_NODE_AUTORESTORE(ctxt)
> +
> +idmap = g_new0(virDomainIdMapEntry, num);
> +
> +for (i = 0; i < num; i++) {
> +ctxt->node = node[i];
> +if (virXPathUInt("string(./@start)", ctxt, [i].start) < 0 ||
> +virXPathUInt("string(./@target)", ctxt, [i].target) < 0 ||
> +virXPathUInt("string(./@count)", ctxt, [i].count) < 0) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("invalid idmap start/target/count settings"));
> +VIR_FREE(idmap);
> +return NULL;
> +}
> +}
> +
> +qsort(idmap, num, sizeof(idmap[0]), virDomainIdMapEntrySort);
> +
> +return idmap;
> +}
> +
> +

Michal

Re: [libvirt PATCHv1 6/8] qemu: virtiofs: do not force UID 0

[Michal Prívozník]

On 9/11/23 15:51, Ján Tomko wrote:
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_virtiofs.c | 4 
>  1 file changed, 4 deletions(-)

While I love short commit messages, this one is rather short. I'd
appreciate more insight into why this is no longer needed.

Michal

Re: [libvirt PATCHv1 4/8] conf: add idmap element to filesystem

[Michal Prívozník]

On 9/11/23 15:51, Ján Tomko wrote:
> Let unprivileged virtiofs use user namespace.
> 
> Signed-off-by: Ján Tomko 
> ---
>  docs/formatdomain.rst |  7 +++
>  src/conf/domain_conf.c| 51 +++
>  src/conf/domain_conf.h|  1 +
>  src/conf/schemas/domaincommon.rng |  3 ++
>  .../vhost-user-fs-fd-memory.xml   |  4 ++
>  5 files changed, 66 insertions(+)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index bc469e5f9f..0f10b3043f 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -3501,6 +3501,10 @@ A directory on the host that can be accessed directly 
> from the guest.
>   
>   
>   
> + 
> + 
> + 
> + 
>   
>   
>   
> @@ -3650,6 +3654,9 @@ A directory on the host that can be accessed directly 
> from the guest.
> Where the ``source`` can be accessed in the guest. For most drivers this 
> is
> an automatic mount point, but for QEMU/KVM this is merely an arbitrary 
> string
> tag that is exported to the guest as a hint for where to mount.
> +``idmap``
> +   For ``virtiofs``, an ``idmap`` element can be specified to map IDs in the 
> user
> +   namespace. See the `Container boot`_ section for the syntax of the 
> element.

:since:

>  ``readonly``
> Enables exporting filesystem as a readonly mount for guest, by default
> read-write access is given (currently only works for QEMU/KVM driver).
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index dd67e7f21b..2379a9204f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2585,6 +2585,8 @@ void virDomainFSDefFree(virDomainFSDef *def)
>  virObjectUnref(def->privateData);
>  g_free(def->binary);
>  g_free(def->sock);
> +g_free(def->idmap.uidmap);
> +g_free(def->idmap.gidmap);
>  
>  g_free(def);
>  }
> @@ -8767,6 +8769,8 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt,
>  xmlNodePtr binary_lock_node = virXPathNode("./binary/lock", ctxt);
>  xmlNodePtr binary_cache_node = virXPathNode("./binary/cache", ctxt);
>  xmlNodePtr binary_sandbox_node = virXPathNode("./binary/sandbox", 
> ctxt);
> +ssize_t n;
> +xmlNodePtr *nodes = NULL;
>  
>  if (queue_size && virStrToLong_ull(queue_size, NULL, 10, 
> >queue_size) < 0) {
>  virReportError(VIR_ERR_XML_ERROR,
> @@ -8812,6 +8816,30 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt,
> VIR_XML_PROP_NONZERO,
> >sandbox) < 0)
>  goto error;
> +
> +if ((n = virXPathNodeSet("./idmap/uid", ctxt, )) < 0)
> +return NULL;
> +
> +if (n) {
> +def->idmap.uidmap = virDomainIdmapDefParseXML(ctxt, nodes, n);
> +if (!def->idmap.uidmap)
> +return NULL;

If this return is taken, then ...

> +
> +def->idmap.nuidmap = n;
> +}
> +VIR_FREE(nodes);

... this is never called.

> +
> +if  ((n = virXPathNodeSet("./idmap/gid", ctxt, )) < 0)
> +return NULL;
> +
> +if (n) {
> +def->idmap.gidmap =  virDomainIdmapDefParseXML(ctxt, nodes, n);
> +if (!def->idmap.gidmap)
> +return NULL;

Same here.

> +
> +def->idmap.ngidmap = n;
> +}
> +VIR_FREE(nodes);
>  }
>  
>  if (source == NULL && def->type != VIR_DOMAIN_FS_TYPE_RAM
> @@ -23164,6 +23192,29 @@ virDomainFSDefFormat(virBuffer *buf,
>  virXMLFormatElement(buf, "driver", , );
>  virXMLFormatElement(buf, "binary", , );
>  
> +if (def->idmap.uidmap) {
> +size_t i;
> +
> +virBufferAddLit(buf, "\n");
> +virBufferAdjustIndent(buf, 2);
> +for (i = 0; i < def->idmap.nuidmap; i++) {
> +virBufferAsprintf(buf,
> +  "\n",
> +  def->idmap.uidmap[i].start,
> +  def->idmap.uidmap[i].target,
> +  def->idmap.uidmap[i].count);
> +}
> +for (i = 0; i < def->idmap.ngidmap; i++) {
> +virBufferAsprintf(buf,
> +  "\n",
> +  def->idmap.gidmap[i].start,
> +  def->idmap.gidmap[i].target,
> +  def->idmap.gidmap[i].count);
> +}
> +virBufferAdjustIndent(buf, -2);
> +virBufferAddLit(buf, "\n");
> +}
> +
>  switch (def->type) {
>  case VIR_DOMAIN_FS_TYPE_MOUNT:
>  case VIR_DOMAIN_FS_TYPE_BIND:
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8937968e3b..b84719b01d 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -925,6 +925,7 @@ struct _virDomainFSDef {
>  virTristateSwitch flock;
>  virDomainFSSandboxMode 

Re: [libvirt PATCHv1 0/8] support running virtiofsd in session mode

[Michal Prívozník]

On 9/11/23 15:51, Ján Tomko wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=2034630
> https://gitlab.com/libvirt/libvirt/-/issues/535
> https://gitlab.gnome.org/GNOME/gnome-boxes/-/issues/292
> 
> Ján Tomko (8):
>   qemu: fix indentation
>   conf: move idmap definition earlier
>   conf: move idmap parsing earlier
>   conf: add idmap element to filesystem
>   qemu: format uid/gid map for virtiofs
>   qemu: virtiofs: do not force UID 0
>   qemu: allow running virtiofsd in session mode
>   docs: virtiofs: add section about ID remapping
> 
>  docs/formatdomain.rst |   7 +
>  docs/kbase/virtiofs.rst   |  29 
>  src/conf/domain_conf.c| 149 --
>  src/conf/domain_conf.h|  29 ++--
>  src/conf/schemas/domaincommon.rng |   3 +
>  src/qemu/qemu_validate.c  |  25 ++-
>  src/qemu/qemu_virtiofs.c  |  17 +-
>  .../vhost-user-fs-fd-memory.xml   |   4 +
>  8 files changed, 181 insertions(+), 82 deletions(-)
> 

So how does this idmap end up on virtiofsd cmd line? Maybe you forgot to
send some additional patches?

Michal

Re: [PATCH 00/51] Finish conversion of all test cases in qemuxml2argvtest to real capabilities

[Michal Prívozník]

On 9/5/23 16:54, Peter Krempa wrote:
> On Tue, Sep 05, 2023 at 16:23:27 +0200, Peter Krempa wrote:
>> This series converts all outstanding test cases to use real
>> capabilities.
>>
>> In the process few cases of pointless tests or features which are
>> supported by every version of qemu were identified and addressed.
>>
>> To simplify the bulk of the conversion I've opted to simply set the most
>> common capabilities in DO_TEST and DO_TEST_NOCAPS in addition to what
>> the test asked for. This is done in a handful of patches and allows then
>> a bulk conversion of many tests with no changes in the output files.
>>
>> I've payed special attention to CPU tests were I've ensured that they
>> are semantically testing what they were before.
> 
> 
> Note: As is this breaks 'securityselinuxlabellingtest' as that test is
> skipped on filesystems not supporting xattr and I'm building libvirt in
> tmpfs to save wear on the SSD. I'll update with what changes are
> necessary.
> 

With kernel-6.6 tmpfs gains support for user.* XATTRs too (among other things):

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ecd7db20474c3859d4d01f34aaabf41bd28c7d84

Michal

Re: [PATCH 8/8] build: Decrease maximum stack frame size to 2048

[Michal Prívozník]

On 9/5/23 09:43, Peter Krempa wrote:
> On Mon, Sep 04, 2023 at 16:09:00 +0200, Michal Prívozník wrote:
>> On 8/30/23 13:59, Peter Krempa wrote:
>>> After recent cleanups we can now restrict the maximum stack frame size
>>> to 2k.
>>>
>>> Signed-off-by: Peter Krempa 
>>> ---
>>>  meson.build | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 965ada483b..e45f3e2c39 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -248,7 +248,7 @@ alloc_max = run_command(
>>>  )
>>>
>>>  # sanitizer instrumentation may enlarge stack frames
>>> -stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 32768
>>> +stack_frame_size = get_option('b_sanitize') == 'none' ? 2048 : 32768
>>>
>>>  # array_bounds=2 check triggers false positive on some GCC
>>>  # versions when using sanitizers. Seen on Fedora 34 with
>>
>>
>> I know this is already pushed but to be honest, I don't really
>> understand why this is needed. I mean, we certainly do not want large
>> frames, but IIUC only frames larger than a page are problem (i.e. 4KiB).
>> Can you please point me to some docs where I can learn more?
> 
> The main idea of this is to ensure that we don't have massive stack
> frames which could cause a stack overflow.

I'm failing to see what stack size and buffer overflow have in common.
To overwrite the return address you can has as small buffer as 1 byte
and not check the boundaries (obviously). And yet, you can have 2K
buffer on stack but if you check the boundaries when reading into it, no
exploit is possible.

> 
> But to be honest, it doesn't matter too much whether the actual limit
> size is 4k or 2k. It still allows us to have very deep call stack, and a
> factor of 2 in the size will not make a real difference.

Yeah. If we care about the actual stack usage (e.g. like kernel does -
it has limit of 8KiB for the whole stack of a kernel thread), then
restricting individual functions is not enough since we have recursive
functions.

> 
> 
> At one point I was looking at which functions have massive stack frames
> and tried avoiding such state. Now this last set of patches was what I
> had in my local branch for a long time and decided to finally get rid of
> them. As you can see, there were multiple cases of 2k buffers being
> stack allocated, so the end result IMO made sense.

Indeed and this reason alone is good enough. It made the code cleaner
too. I was just wondering whether there is some deeper sense.

> 
> The decreasing of the actual limit to 2k isn't as important as I've
> noted but similarly to when we add a syntax check after a full-repo
> cleanup it is a way to prevent regressions after a cleanup.
> 

BTW: if you (or anybody else) want to identify functions which have
massive stack usage, I've found clever perl script from kernel.git:

libvirt.git $ objdump -d _build/src/libvirt.so.0.9008.0 | \
kernel.git/scripts/checkstack.pl | \
head -n30

some very interesting functions appear on the list
(virDomainDefFeaturesCheckABIStability()) - but when looking at the
disassembled code directly, it's not just about initial sub 0x68, %rsp;
it's also about how many arguments are passed to functions (esp. with
variable arguments)

Michal

Re: [PATCH 8/8] build: Decrease maximum stack frame size to 2048

[Michal Prívozník]

On 8/30/23 13:59, Peter Krempa wrote:
> After recent cleanups we can now restrict the maximum stack frame size
> to 2k.
> 
> Signed-off-by: Peter Krempa 
> ---
>  meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 965ada483b..e45f3e2c39 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -248,7 +248,7 @@ alloc_max = run_command(
>  )
> 
>  # sanitizer instrumentation may enlarge stack frames
> -stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 32768
> +stack_frame_size = get_option('b_sanitize') == 'none' ? 2048 : 32768
> 
>  # array_bounds=2 check triggers false positive on some GCC
>  # versions when using sanitizers. Seen on Fedora 34 with


I know this is already pushed but to be honest, I don't really
understand why this is needed. I mean, we certainly do not want large
frames, but IIUC only frames larger than a page are problem (i.e. 4KiB).
Can you please point me to some docs where I can learn more?

Michal

Re: [PATCH] conf: Generate MAC address instead of keeping all zeroes

[Michal Prívozník]

On 9/1/23 17:12, Martin Kletzander wrote:
> When we parse  we keep that in memory
> and pass it down to the hypervisor.  However, that MAC address is not
> strictly valid as it is not marked as locally administered (bit 0x02)
> but it is not even globally unique.  It is also used for loopback device
> on Linux, for example.  And QEMU sees such MAC address just as "not
> specified" and generates a new one that libvirt does not even know
> about.  So to make the overall experience better we now generate it if
> the supplied one is all clear.

Please consider s/  / /g

> 
> Resolves: https://issues.redhat.com/browse/RHEL-974
> 
> Signed-off-by: Martin Kletzander 
> ---
>  src/conf/domain_conf.c|  2 +-
>  src/util/virmacaddr.c |  5 +
>  src/util/virmacaddr.h |  1 +
>  .../network-interface-mac-clear.xml   | 21 +++
>  .../network-interface-mac-clear.xml   | 21 +++
>  tests/genericxml2xmltest.c|  4 +++-
>  6 files changed, 52 insertions(+), 2 deletions(-)
>  create mode 100644 tests/genericxml2xmlindata/network-interface-mac-clear.xml
>  create mode 100644 
> tests/genericxml2xmloutdata/network-interface-mac-clear.xml
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index bb4f1fdb948d..652bd09b21b8 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9675,7 +9675,7 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt,
>  return NULL;
>  }
>  
> -if (!macaddr) {
> +if (!macaddr || virMacAddrIsAllClear(>mac)) {
>  virDomainNetGenerateMAC(xmlopt, >mac);
>  def->mac_generated = true;
>  }
> diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
> index 073f298b5b66..e06bb200fc68 100644
> --- a/src/util/virmacaddr.c
> +++ b/src/util/virmacaddr.c
> @@ -246,6 +246,11 @@ virMacAddrIsBroadcastRaw(const unsigned char 
> s[VIR_MAC_BUFLEN])
>  return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
>  }
>  
> +bool virMacAddrIsAllClear(const virMacAddr *addr)
> +{
> +return !virMacAddrCmpRaw(addr, (const unsigned char[VIR_MAC_BUFLEN]){0});
> +}
> +
>  void
>  virMacAddrFree(virMacAddr *addr)
>  {
> diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h
> index f32b58805a61..7b9eb7443bd1 100644
> --- a/src/util/virmacaddr.h
> +++ b/src/util/virmacaddr.h
> @@ -58,6 +58,7 @@ int virMacAddrParseHex(const char* str,
>  bool virMacAddrIsUnicast(const virMacAddr *addr);
>  bool virMacAddrIsMulticast(const virMacAddr *addr);
>  bool virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN]);
> +bool virMacAddrIsAllClear(const virMacAddr *addr);
>  void virMacAddrFree(virMacAddr *addr);
>  
>  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMacAddr, virMacAddrFree);

Please expose the symbol in src/libvirt_private.syms too.

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH] docs, passt: Clarify some niche passt usage

[Michal Prívozník]

On 9/2/23 00:41, Martin Kletzander wrote:
> Change example logfile path and clarify how complicated all things passt
> are.  I chose not to create the non-existing directory because it could
> open a whole new can of worms.
> 
> Also explain missing `dev` attribute of ``
> 
> Resolves: https://issues.redhat.com/browse/RHEL-1833
> 
> Signed-off-by: Martin Kletzander 
> ---
>  docs/formatdomain.rst | 21 +++--
>  1 file changed, 15 insertions(+), 6 deletions(-)

Reviewed-by: Michal Privoznik 

Michal

Re: Error : virHostCPUGetKVMMaxVCPUs:1228 : KVM is not supported on this platform: Function not implemented.

[Michal Prívozník]

On 8/29/23 21:22, Mario Marietto wrote:
> Hello.
> 

> The error that I'm not able to fix is the following one :
> 
> 
> root@chromarietto:~#  virsh domcapabilities --machine virt
> --emulatorbin /usr/local/bin/qemu-system-arm
> 
> 2023-08-29 10:17:59.110+: 1763: error : virHostCPUGetKVMMaxVCPUs:1228 :
> KVM is not supported on this platform: Function not implemented ;
> error: failed to get emulator capabilities
> error: KVM is not supported on this platform: Function not implemented
> 
> 
This is the source code that reports that error:

https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virhostcpu.c?ref_type=heads#L1228

Long story short, you are missing kvm.h header. On my system it's
located under: /usr/include/linux/kvm.h and provided by linux-headers
package.

And if you think about it (and take a look at the other implementation
of the function), it kind of makes sense - libvirt opens /dev/kvm and
calls series of ioctl()-s to learn about KVM capabilities. Libvirt
shouldn't hard code values but rather use the ones provided by kernel
header files.

Happy hacking!

Michal

Re: [PATCH 3/4] vircommand: Make sysconf(_SC_OPEN_MAX) failure non-fatal

[Michal Prívozník]

On 8/29/23 17:20, Daniel P. Berrangé wrote:
> On Tue, Aug 29, 2023 at 05:13:08PM +0200, Michal Privoznik wrote:
>> The point of calling sysconf(_SC_OPEN_MAX) is to allocate big
>> enough bitmap so that subsequent call to
>> virCommandMassCloseGetFDsDir() can just set the bit instead of
>> expanding memory (this code runs in a forked off child and thus
>> using async-signal-unsafe functions like malloc() is a bit
>> tricky).
>>
>> But on some systems the limit for opened FDs is virtually
>> non-existent (typically macOS Ventura started reporting EINVAL).
>>
>> But with both glibc and musl using malloc() after fork() is safe.
>> And with sufficiently new glib too, as it's using malloc() with
>> newer releases instead of their own allocator.
>>
>> Therefore, pick a sufficiently large value (glibc falls back to
>> 256, [1], so 1024 should be good enough) to fall back to and make
>> the error non-fatal.
> 
> That's not suitable for macOS. THey have a constant OPEN_MAX we
> should be using that is way bigger than 1024.

Yeah, it's 2^63-1 per:

https://github.com/eradman/entr/issues/63#issuecomment-776758771

If we really use that it's going to take ages to iterate over all FDs.
OTOH, the next commit switches over to /dev/fd where we can learn about
opened FDs, so it's not that bad. Except we would be allocating
humongous virBitmap (1 EiB). what seems to be the right default then?

Michal

Re: [PATCH 0/5] Refactor few old-style XML node lookup loops

[Michal Prívozník]

On 8/28/23 15:57, Peter Krempa wrote:
> Peter Krempa (5):
>   virNetworkDNSHostDefParseXML: Refactor parsing
>   virsh: domain: Refactor XML handling for disk changes
>   virDomainFeaturesCapabilitiesDefParse: Use virXMLNodeGetSubelementList
>   virDomainFeaturesKVMDefParse: Use virXMLNodeGetSubelementList
>   virDomainFeaturesXENDefParse: Use virXMLNodeGetSubelementList
> 
>  src/conf/domain_conf.c  | 50 +++---
>  src/conf/network_conf.c | 94 -
>  tools/virsh-domain.c| 63 +++
>  3 files changed, 78 insertions(+), 129 deletions(-)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH 0/9] qemu: Rework probing of vCPU hotplug support

[Michal Prívozník]

On 8/28/23 15:11, Peter Krempa wrote:
> Nowadays all qemu's support the command which was used as witness, but
> was gated on machine type's support of vCPU hotplug.  Directly probe the
> machine type.
> 
> 
> Peter Krempa (9):
>   qemu: Rename qemuDomainSupportsNewVcpuHotplug to
> qemuDomainSupportsVcpuHotplug
>   qemu: capabilities: Export functions necessary for probing machine
> types
>   qemu: process: Probe machine type data on reconnect to qemu
>   qemuxml2argvtest: Modernize 'cpu-hotplug-startup' case
>   tests: qemuhotplugtest: Fix arch-specific parts of 'ppc64' test XMLs
>   qemuhotplugtest: Remove 'modern' field for cpu hotplug tests
>   qemuDomainSupportsVcpuHotplug: Base return value on
> virQEMUCapsGetMachineHotplugCpus
>   qemu: capabilities: Retire QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS
>   qemu: capabilities: Remove unused 'virQEMUCapsFilterByMachineType'
> 
>  src/qemu/qemu_capabilities.c  | 53 ++-
>  src/qemu/qemu_capabilities.h  | 18 ---
>  src/qemu/qemu_domain.c| 12 ++---
>  src/qemu/qemu_domain.h|  2 +-
>  src/qemu/qemu_hotplug.c   |  4 +-
>  src/qemu/qemu_process.c   | 41 --
>  .../caps_4.2.0_aarch64.xml|  1 -
>  .../qemucapabilitiesdata/caps_4.2.0_ppc64.xml |  1 -
>  .../qemucapabilitiesdata/caps_4.2.0_s390x.xml |  1 -
>  .../caps_4.2.0_x86_64.xml |  1 -
>  .../caps_5.0.0_aarch64.xml|  1 -
>  .../qemucapabilitiesdata/caps_5.0.0_ppc64.xml |  1 -
>  .../caps_5.0.0_riscv64.xml|  1 -
>  .../caps_5.0.0_x86_64.xml |  1 -
>  .../qemucapabilitiesdata/caps_5.1.0_sparc.xml |  1 -
>  .../caps_5.1.0_x86_64.xml |  1 -
>  .../caps_5.2.0_aarch64.xml|  1 -
>  .../qemucapabilitiesdata/caps_5.2.0_ppc64.xml |  1 -
>  .../caps_5.2.0_riscv64.xml|  1 -
>  .../qemucapabilitiesdata/caps_5.2.0_s390x.xml |  1 -
>  .../caps_5.2.0_x86_64.xml |  1 -
>  .../caps_6.0.0_aarch64.xml|  1 -
>  .../qemucapabilitiesdata/caps_6.0.0_s390x.xml |  1 -
>  .../caps_6.0.0_x86_64.xml |  1 -
>  .../caps_6.1.0_x86_64.xml |  1 -
>  .../caps_6.2.0_aarch64.xml|  1 -
>  .../qemucapabilitiesdata/caps_6.2.0_ppc64.xml |  1 -
>  .../caps_6.2.0_x86_64.xml |  1 -
>  .../caps_7.0.0_aarch64+hvf.xml|  1 -
>  .../caps_7.0.0_aarch64.xml|  1 -
>  .../qemucapabilitiesdata/caps_7.0.0_ppc64.xml |  1 -
>  .../caps_7.0.0_x86_64.xml |  1 -
>  .../qemucapabilitiesdata/caps_7.1.0_ppc64.xml |  1 -
>  .../caps_7.1.0_x86_64.xml |  1 -
>  tests/qemucapabilitiesdata/caps_7.2.0_ppc.xml |  1 -
>  .../caps_7.2.0_x86_64+hvf.xml |  1 -
>  .../caps_7.2.0_x86_64.xml |  1 -
>  .../caps_8.0.0_riscv64.xml|  1 -
>  .../caps_8.0.0_x86_64.xml |  1 -
>  .../qemucapabilitiesdata/caps_8.1.0_s390x.xml |  1 -
>  .../caps_8.1.0_x86_64.xml |  1 -
>  tests/qemuhotplugtest.c   | 40 +-
>  .../ppc64-modern-bulk-domain.xml  |  4 +-
>  .../ppc64-modern-bulk-result-conf.xml | 18 ---
>  .../ppc64-modern-bulk-result-live.xml | 19 +++
>  .../ppc64-modern-individual-domain.xml|  4 +-
>  .../ppc64-modern-individual-result-conf.xml   | 18 ---
>  .../ppc64-modern-individual-result-live.xml   | 19 +++
>  ...=> cpu-hotplug-startup.x86_64-latest.args} | 14 ++---
>  tests/qemuxml2argvtest.c  |  2 +-
>  50 files changed, 126 insertions(+), 177 deletions(-)
>  rename tests/qemuxml2argvdata/{cpu-hotplug-startup.args => 
> cpu-hotplug-startup.x86_64-latest.args} (55%)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH] REFACTOR: Eliminate newlines in messages and fixes a typo

[Michal Prívozník]

On 8/25/23 17:29, K Shiva Kiran wrote:
> Removes newlines of error message strings in metadata unit tests,
> libvirt-domain.c and libvirt-network.c
> Fixes a minor typo in libvirt-domain.c
> 
> Signed-off-by: K Shiva Kiran 
> ---
>  include/libvirt/libvirt-domain.h | 2 +-
>  src/libvirt-domain.c | 3 +--
>  src/libvirt-network.c| 3 +--
>  tests/metadatatest.c | 9 +++--
>  tests/networkmetadatatest.c  | 9 +++--
>  5 files changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index a1902546bb..ea36805aa3 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -5184,7 +5184,7 @@ typedef void 
> (*virConnectDomainEventDeviceRemovalFailedCallback)(virConnectPtr c
>   * virConnectDomainEventMetadataChangeCallback:
>   * @conn: connection object
>   * @dom: domain on which the event occurred
> - * @type: a value from virDomainMetadataTypea
> + * @type: a value from virDomainMetadataType
>   * @nsuri: XML namespace URI
>   * @opaque: application specified data
>   *

ACK to this hunk.

> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index ec42bb9a53..80a554a530 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -8585,8 +8585,7 @@ virDomainSetMetadata(virDomainPtr domain,
>  case VIR_DOMAIN_METADATA_TITLE:
>  if (metadata && strchr(metadata, '\n')) {
>  virReportInvalidArg(metadata, "%s",
> -_("metadata title can't contain "
> -  "newlines"));
> +_("metadata title can't contain newlines"));
>  goto error;
>  }
>  G_GNUC_FALLTHROUGH;
> diff --git a/src/libvirt-network.c b/src/libvirt-network.c
> index c0daea3a60..ef17a8a04d 100644
> --- a/src/libvirt-network.c
> +++ b/src/libvirt-network.c
> @@ -1974,8 +1974,7 @@ virNetworkSetMetadata(virNetworkPtr network,
>  case VIR_NETWORK_METADATA_TITLE:
>  if (metadata && strchr(metadata, '\n')) {
>  virReportInvalidArg(metadata, "%s",
> -_("metadata title can't contain "
> -  "newlines"));
> +_("metadata title can't contain newlines"));
>  goto error;
>  }
>  G_GNUC_FALLTHROUGH;


These two - I've already covered in my other series, that's just waiting
for the release to be pushed:

https://listman.redhat.com/archives/libvir-list/2023-August/241416.html

> diff --git a/tests/metadatatest.c b/tests/metadatatest.c
> index b56428fde5..7136730e6a 100644
> --- a/tests/metadatatest.c
> +++ b/tests/metadatatest.c
> @@ -113,8 +113,7 @@ verifyMetadata(virDomainPtr dom,
>  
>  if (STRNEQ(metadataAPI, expectAPI)) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> -   "XML metadata in API doesn't match expected 
> metadata: "
> -   "expected:\n[%s]\ngot:\n[%s]",
> +   "XML metadata in API doesn't match expected 
> metadata: expected:\n[%s]\ngot:\n[%s]",
> expectAPI, metadataAPI);
>  return false;
>  }

NACK to this one and the rest. If message contains '\n' character then
it's okay if it is on multiple lines. The sole reason we want error
messages on one line is: whenever I see a bug report I can select
"random" substring of the error message and pass it to 'git grep'. If a
message is split on multiple lines though I have to play guessing game
and try to find the exact point where the message was split.

For instance, in the previous two hunks, I can do (after my series is
merged):

git grep "metadata title can't contain newlines"
git grep "metadata title can't contain"
git grep "can't contain newlines"

and all would match the same lines. But if I would do that now, the last
one would match nothing.

But with error messages that contain '\n' it's obviously wrong if I'd
pick a substring containing '\n' and pass it to git grep.

Hence, this hunk and the rest is wrong and make code worse. Sorry.

Michal

Re: [RFC] Adding timestamp to guest's serial console log

[Michal Prívozník]

On 8/29/23 09:04, Shaleen Bathla wrote:
> ping
> 
> -Original Message-
> From: Shaleen Bathla  
> Sent: Tuesday, June 20, 2023 2:44 PM
> To: libvir-list@redhat.com
> Subject: [RFC] Adding timestamp to guest's serial console log
> 
> Hi,
> 
> Need some comments regarding the following feature :
> Addition of timestamp support for serial console logs of a guest.
> 
> We can implement it as a configurable attribute in xml.
> For example :
> 
>   
>   
> 
>   
> 
> 
> We can add a timestamp after every '\n' character received from qemu.
> Can I have some comments regarding this change like what I should keep
> in mind while implementing, whether it is a welcome addition or not,
> issues I might face, any qemu changes required.
> 

Yeah, this might be useful. We definitely need additional attribute to
control this behavior, otherwise we might break some scenarios (e.g.
where users transfer binary data via this channel.

What I worry though, is that with stdio_handler="file", i.e. when the
file is passed to QEMU directly, then this knob won't work. So we would
need some changes to QEMU. But then, taking this idea further, if QEMU
is changed, then does it make sense to have the same code in libvirt
(for stdio_handler='logd' case)?

QEMU already has -msg timestamp=on, which controls timestamps for log
messages (/var/log/libvirt/qemu/$dom.log). But that may be orthogonal to
what you are trying to achieve.

Michal

Re: [PATCH 00/27] Move error messages onto a single line

[Michal Prívozník]

On 8/25/23 12:14, Pavel Hrdina wrote:
> On Fri, Aug 25, 2023 at 09:39:57AM +0200, Michal Privoznik wrote:
>> This is inspired by the following discussion:
>>
>> https://listman.redhat.com/archives/libvir-list/2023-August/241361.html
> 
> We have it covered by coding style document so it would be nice to make
> a syntax-check for it as well so there are no new multi-line errors
> introduced in the future. Not sure how difficult it would be so
> definitely as follow up.

Yeah, I was thinking about that, but I'm not that good with RE-s.

> 
> Otherwise looks good and compiles without issues on my machine.
> 
> I would consider pushing it after release is done to make sure it will
> not break anything as it affects basically the whole libvirt code base.

I hear you and I agree. And given how huge the change is I'd rather push
it right after the release to avoid any conflicts.

> 
>> And ideally I'd present a green pipeline but for some reason, I can't:
>>
>> https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/981105262
>>
>> But the problem is not with my code rather than our CI.
> 
> Unless there are objections from others
> 
> Reviewed-by: Pavel Hrdina 

Thank you!

Michal

Re: [libvirt PATCH 0/2] Document basic VFIO variant driver support

[Michal Prívozník]

On 8/25/23 06:23, Laine Stump wrote:
> update the manpage for the virsh nodedev-detach --driver option, and add a 
> blurb to the NEWS file for the upcoming release
> 
> Laine Stump (2):
>   docs: update description of virsh nodedev-detach --driver option
>   NEWS: document support for VFIO variant drivers
> 
>  NEWS.rst| 11 +++
>  docs/manpages/virsh.rst | 25 +
>  2 files changed, 28 insertions(+), 8 deletions(-)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCH v2 4/7] virsh exposure of Network Metadata APIs

[Michal Prívozník]

On 8/16/23 20:47, K Shiva Kiran wrote:
> Adds two new commands and a new option:
> - 'net-desc' to show/modify network title and description.
> - 'net-metadata' to show/modify network metadata.
> - Option '--title' for 'net-list' to print corresponding
>   network titles in an additional column.
> - Documentation for all the above.
> - XML Fallback function `virshNetworkGetXMLFromNet` for title and
>   description for compatibility with hosts running older versions
>   of libvirtd.
> 
> Signed-off-by: K Shiva Kiran 
> ---
>  docs/manpages/virsh.rst |  77 
>  tools/virsh-network.c   | 411 ++--
>  tools/virsh-util.c  |  25 +++
>  tools/virsh-util.h  |   9 +
>  4 files changed, 511 insertions(+), 11 deletions(-)

There's too much going on in a single patch. Two new commands are
introduced, another one grows new argument. It easily could have been
separate patches. The problem with different semantic changes being
tangled in one patch is: it's harder to review as reviewer has to track
multiple things during review. Subsequently, reviewers tend to pick
something better structured and this may then fall through the cracks.

Michal

Re: [libvirt PATCH v2 0/7] Introduce new Metadata fields for Network object with corresponding APIs

[Michal Prívozník]

On 8/16/23 20:47, K Shiva Kiran wrote:
> This commit adds the following:
> - Introduction of  and  fields to the Network Object.
> - Introduction of Get and Set Public APIs for the aforementioned fields.
> - virsh exposure of the aforementioned Public APIs.
> - Adds implementation in test driver along with a testcase.
> - Implementation in bridge driver.
> 
> This is a v2 of:
> https://listman.redhat.com/archives/libvir-list/2023-July/240828.html
> Diff to v1:
> - Corrected placement of structs in remote_protocol-structs.
> - Removed redundant call to virNetworkObjSetDefTransient() in
>   virNetworkConfigChangeSetup().
> - Removed redundant logic in networkUpdate(), substituted by call to
>   newly introduced virNetworkObjUpdateModificationImpact().
> - Added virsh exposure of the APIs.
> - Added bridge driver implementation.
> 
> Signed-off-by: K Shiva Kiran 
> 
> K Shiva Kiran (7):
>   Add  and  for Network Objects
>   Adding Public Get and Set APIs for Network Metadata
>   Implementing Remote Protocol for Network Metadata
>   virsh exposure of Network Metadata APIs
>   Add virNetworkObj Get and Set Methods for Metadata
>   Add Test driver and testcase for Network Metadata change APIs
>   Added bridge driver implementation
> 
>  docs/formatnetwork.rst|  11 +
>  docs/manpages/virsh.rst   |  77 ++
>  include/libvirt/libvirt-network.h |  29 +++
>  include/libvirt/virterror.h   |   1 +
>  src/conf/network_conf.c   |  21 ++
>  src/conf/network_conf.h   |   2 +
>  src/conf/schemas/basictypes.rng   |  15 ++
>  src/conf/schemas/domaincommon.rng |  15 --
>  src/conf/schemas/network.rng  |  10 +
>  src/conf/virnetworkobj.c  | 329 +++-
>  src/conf/virnetworkobj.h  |  21 ++
>  src/driver-network.h  |  16 ++
>  src/libvirt-network.c | 167 
>  src/libvirt_private.syms  |   3 +
>  src/libvirt_public.syms   |   6 +
>  src/network/bridge_driver.c   |  78 +-
>  src/remote/remote_driver.c|   2 +
>  src/remote/remote_protocol.x  |  36 ++-
>  src/remote_protocol-structs   |  19 ++
>  src/test/test_driver.c|  83 +-
>  src/util/virerror.c   |   3 +
>  tests/meson.build |   1 +
>  tests/networkmetadatatest.c   | 297 +
>  tools/virsh-network.c | 411 +-
>  tools/virsh-util.c|  25 ++
>  tools/virsh-util.h|   9 +
>  26 files changed, 1624 insertions(+), 63 deletions(-)
>  create mode 100644 tests/networkmetadatatest.c
> 

Alright. Let me merge these. Vven though they should have been
structured differently, the code looks factually okay.

Reviewed-by: Michal Privoznik 

As we are getting close to the release, please do write a NEWS entry as
this is something that users might find valuable.

Michal

Re: [libvirt PATCH v2 5/7] Add virNetworkObj Get and Set Methods for Metadata

[Michal Prívozník]

On 8/16/23 20:47, K Shiva Kiran wrote:
> - Introduces virNetworkObjGetMetadata() and
>   virNetworkObjSetMetadata().
> - These functions implement common behaviour that can be reused by
>   network drivers.
> - Introduces virNetworkObjUpdateModificationImpact() among other
>   helper functions that resolve the live/persistent state of
>   the network before setting metadata.
> - Eliminates redundant call of virNetworkObjSetDefTransient() in
>   virNetworkConfigChangeSetup() among others.
> - Substituted redundant logic in networkUpdate() with a call to
>   virNetworkObjUpdateModificationImpact().
> 
> Signed-off-by: K Shiva Kiran 
> ---
>  src/conf/virnetworkobj.c| 329 ++--
>  src/conf/virnetworkobj.h|  21 +++
>  src/libvirt_private.syms|   3 +
>  src/network/bridge_driver.c |  14 +-
>  src/test/test_driver.c  |  16 +-
>  5 files changed, 347 insertions(+), 36 deletions(-)

Again, way too much changes, disperse in semantics for one patch. You've
introduced virNetworkObjUpdateModificationImpact(). Perfect! But it
should have been one patch. Then you eliminate redundant call to
virNetworkObjSetDefTransient()? Splendid, but again - it's a different
change and has nothing to do with virNetworkObjSetDefTransient(). You
implement new APIs? Sweet, but what do they have to do with the
redundant call?

Splitting patches per directory is not the same as "one semantic change
per patch". Sometimes it is, e.g. in the series I've posted earlier today:

https://listman.redhat.com/archives/libvir-list/2023-August/241416.html

I know I might be asking too much, but try to put yourself into
reviewers shoes. Libvirt's code base is not exactly the smallest and
reviewing one change (and trying to think of all implications) is hard
enough already. If changes are intertwined into one patch then it's
needlessly harder.

Michal

Re: [PATCH] conf: add virDomainDiskBlockIoCheckABIStability()

[Michal Prívozník]

On 8/24/23 12:57, Kristina Hanicova wrote:
> Add missing ABI stability check for blockio properties for disk
> devices.
> 
> Signed-off-by: Kristina Hanicova 
> ---
>  src/conf/domain_conf.c | 25 +
>  1 file changed, 25 insertions(+)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCH v3 8/8] qemu: turn two multiline log messages into single line

[Michal Prívozník]

On 8/23/23 22:06, Laine Stump wrote:
> On 8/23/23 3:52 AM, Michal Prívozník wrote:
>> On 8/21/23 21:32, Laine Stump wrote:
>>> Normally I wouldn't bother with a change like this, but I was touching
>>> the function anyway, and wanted to leave it looking nice and tidy.
>>>
>>> Signed-off-by: Laine Stump 
>>> ---
>>>   src/qemu/qemu_driver.c | 6 ++
>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index f676744e9e..a60cbf0ed4 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -11411,8 +11411,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
>>>    */
>>>   if (STREQ_NULLABLE(driverName, "kvm")) {
>>>   virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>>> -   _("'legacy KVM' device assignment is no longer "
>>> - "supported on this system"));
>>> +   _("'legacy KVM' device assignment is no
>>> longer supported on this system"));
>>>   return -1;
>>>   }
>>>   @@ -11423,8 +11422,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr
>>> dev,
>>>     if (!qemuHostdevHostSupportsPassthroughVFIO()) {
>>>   virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>>> -   _("VFIO device assignment is currently not "
>>> - "supported on this system"));
>>> +   _("VFIO device assignment is currently not
>>> supported on this system"));
>>>    return -1;
>>>   }
>>>   
>>
>> This got me thinking, whether we should do one huge commit which would
>> fix ALL the error messages in all files and just be done with it for
>> good. Again, future work, you patch is good as is.
> 
> Yep, I had that thought too. I do worry that single giant mega-patches
> like that can create merge conflicts later though (since cherry-picking
> the mega-patch to resolve one conflict in context can create several
> other conflicts), but as you say that can be done (including the
> discussion of its relative merits) at another time.
> 

Yeah, that would be a problem, but also such conflicts would be trivial
to resolve.

Michal

Re: [libvirt PATCH v3 1/8] util: use "stubDriverType" instead of just "stubDriver"

[Michal Prívozník]

On 8/21/23 21:32, Laine Stump wrote:
> In the past we just kept track of the type of the "stub driver" (the
> driver that is bound to a device in order to assign it to a
> guest). The next commit will add a stubDriverName to go along with
> type, so lets use stubDriverType for the existing enum to make it
> easier to keep track of whether we're talking about the name or the
> type.
> 
> Signed-off-by: Laine Stump 
> ---
>  src/hypervisor/domain_driver.c |  4 ++--
>  src/hypervisor/virhostdev.c|  8 
>  src/libvirt_private.syms   |  4 ++--
>  src/util/virnvme.c |  2 +-
>  src/util/virpci.c  | 16 
>  src/util/virpci.h  |  6 +++---
>  tests/virhostdevtest.c |  2 +-
>  tests/virpcitest.c |  8 
>  8 files changed, 25 insertions(+), 25 deletions(-)

There's one more occurrence:

diff --git i/src/util/virpci.c w/src/util/virpci.c
index 88a020fb86..d86a81c2b1 100644
--- i/src/util/virpci.c
+++ w/src/util/virpci.c
@@ -1267,9 +1267,10 @@ virPCIDeviceBindToStub(virPCIDevice *dev)
 /* virPCIDeviceDetach:
  *
  * Detach this device from the host driver, attach it to the stub
- * driver (previously set with virPCIDeviceSetStubDriver(), and add *a
- * copy* of the object to the inactiveDevs list (if provided). This
- * function will *never* consume dev, so the caller should free it.
+ * driver (previously set with virPCIDeviceSetStubDriverType(), and
+ * add *a copy* of the object to the inactiveDevs list (if provided).
+ * This function will *never* consume dev, so the caller should free
+ * it.
  *
  * Returns 0 on success, -1 on failure (will fail if the device is
  * already in the activeDevs list, but will be a NOP if the device is


Michal

Re: [libvirt PATCH v3 4/8] util: permit existing binding to VFIO variant driver

[Michal Prívozník]

On 8/21/23 21:32, Laine Stump wrote:
> Before a PCI device can be assigned to a guest with VFIO, that device
> must be bound to the vfio-pci driver rather than to the device's
> normal host driver. The vfio-pci driver provides APIs that permit QEMU
> to perform all the necessary operations to make the device accessible
> to the guest.
> 
> In the past vfio-pci was the only driver that supplied these APIs, but
> there are now vendor/device-specific "VFIO variant" drivers that
> provide the basic vfio-pci driver functionality/API while adding
> support for device-specific operations (for example these
> device-specific drivers may support live migration of certain
> devices).  All that is needed to make this functionality available is
> to bind the vendor-specific "VFIO variant" driver to the device
> (rather than the generic vfio-pci driver, which will continue to work,
> just without the extra functionality).
> 
> But until now libvirt has required that all PCI devices being assigned
> to a guest with VFIO specifically have the "vfio-pci" driver bound to
> the device. So even if the user manually binds a shiny new
> vendor-specific VFIO variant driver to the device (and puts
> "managed='no'" in the config to prevent libvirt from changing the
> binding), libvirt will just fail during startup of the guest (or
> during hotplug) because the driver bound to the device isn't exactly
> "vfio-pci".
> 
> Beginning with kernel 6.1, it's possible to determine from the sysfs
> directory for a device whether the currently-bound driver is the
> vfio-pci driver or a VFIO variant - the device directory will have a
> subdirectory called "vfio-dev". We can use that to appropriately widen
> the list of drivers that libvirt will allow for VFIO device
> assignment.
> 
> This patch doesn't remove the explicit check for the exact "vfio-pci"
> driver (since that would cause systems with pre-6.1 kernels to behave
> incorrectly), but adds an additional check for the vfio-dev directory,
> so that any VFIO variant driver is acceptable for libvirt to continue
> setting up for VFIO device assignment.
> 
> Signed-off-by: Laine Stump 
> ---
>  src/hypervisor/virhostdev.c | 28 +
>  src/libvirt_private.syms|  1 +
>  src/util/virpci.c   | 78 ++---
>  src/util/virpci.h   |  3 ++
>  4 files changed, 87 insertions(+), 23 deletions(-)


Splendid! We can now turn virPCIDeviceGetCurrentDriverPathAndName() into
a static function is it's only real use is inside of virpci.c. The only
other use outside is in virpcitest.c and it only cares about
${driverName} anyway (so it's okay with calling this new
virPCIDeviceGetCurrentDriverNameAndType()). Feel free to save that work
for a follow up patch.

Michal

Re: [libvirt PATCH v3 2/8] util: add stub driver name to virPCIDevice object

[Michal Prívozník]

On 8/21/23 21:32, Laine Stump wrote:
> There can be many different drivers that are of the type "VFIO", so
> add the driver name to the object and allow getting/setting it.
> 
> Signed-off-by: Laine Stump 
> ---
>  src/libvirt_private.syms |  2 ++
>  src/util/virpci.c| 16 
>  src/util/virpci.h|  3 +++
>  3 files changed, 21 insertions(+)
> 

> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 88a020fb86..103bc4254e 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c

> @@ -1580,6 +1583,19 @@ virPCIDeviceGetStubDriverType(virPCIDevice *dev)
>  return dev->stubDriverType;
>  }
>  
> +void
> +virPCIDeviceSetStubDriverName(virPCIDevice *dev,
> +   const char *driverName)
> +{
> +dev->stubDriverName = g_strdup(driverName);
> +}


I think it's worth freeing dev->stubDriverName before setting another
one. Please consider squashing this in:

diff --git i/src/util/virpci.c w/src/util/virpci.c
index 3f207a24f3..15e53e6749 100644
--- i/src/util/virpci.c
+++ w/src/util/virpci.c
@@ -1586,8 +1586,9 @@ virPCIDeviceGetStubDriverType(virPCIDevice *dev)

 void
 virPCIDeviceSetStubDriverName(virPCIDevice *dev,
-   const char *driverName)
+  const char *driverName)
 {
+g_free(dev->stubDriverName);
 dev->stubDriverName = g_strdup(driverName);
 }


And just a thought (maybe it was covered in earlier discussions, maybe
it'll be covered in next patches) - what about the following scenario:

virPCIDeviceSetStubDriverType(, VIR_PCI_STUB_DRIVER_VFIO);
virPCIDeviceSetStubDriverName(, "blah");

Should the latter reset dev->stubDriverType to _NONE? Similarly, what if
the two are reversed? But I guess we can always fine tune this later.

Michal

Re: [libvirt PATCH v3 7/8] node_device: support binding other drivers with virNodeDeviceDetachFlags()

[Michal Prívozník]

On 8/21/23 21:32, Laine Stump wrote:
> In the past, the only allowable values for the "driver" field of
> virNodeDeviceDetachFlags() were "kvm" or "vfio" for the QEMU driver,
> and "xen" for the libxl driver. Then "kvm" was deprecated and removed,
> so the driver name became essentially irrelevant (because it is always
> called via a particular hypervisor driver, and so the "xen" or "vfio"
> can be (and almost always is) implied.
> 
> With the advent of VFIO variant drivers, the ability to explicitly
> specify a driver name once again becomes useful - it can be used to
> name the exact VFIO driver that we want bound to the device in place
> of vfio-pci, so this patch allows those other names to be passed down
> the call chain, where the code in virpci.c can make use of them.
> 
> The names "vfio", "kvm", and "xen" retain their special meaning, though:
> 
>   1) because there may be some application or configuration that still
>  calls virNodeDeviceDetachFlags() with driverName="vfio", this
>  single value is substituted with the synonym of NULL, which means
>  "bind the default driver for this device and hypervisor". This
>  will currently result in the vfio-pci driver being bound to the
>  device.
> 
>   2) in the case of the libxl driver, "xen" means to use the standard
>  driver used in the case of Xen ("pciback").
> 
>   3) "kvm" as a driver name always results in an error, as legacy KVM
>  device assignment was removed from the kernel around 10 years ago.
> 
> Signed-off-by: Laine Stump 
> ---
>  src/hypervisor/domain_driver.c |  9 -
>  src/hypervisor/domain_driver.h |  2 ++
>  src/libxl/libxl_driver.c   |  3 ++-
>  src/qemu/qemu_driver.c | 33 +++--
>  4 files changed, 27 insertions(+), 20 deletions(-)
> 
> diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
> index a70f75f3ae..429784292a 100644
> --- a/src/hypervisor/domain_driver.c
> +++ b/src/hypervisor/domain_driver.c
> @@ -462,6 +462,7 @@ virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev,
>  int
>  virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev,
>   virHostdevManager *hostdevMgr,
> + virPCIStubDriver driverType,
>   const char *driverName)
>  {
>  g_autoptr(virPCIDevice) pci = NULL;
> @@ -471,7 +472,7 @@ virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev,
>  g_autoptr(virConnect) nodeconn = NULL;
>  g_autoptr(virNodeDevice) nodedev = NULL;
>  
> -if (!driverName)
> +if (driverType == VIR_PCI_STUB_DRIVER_NONE)
>  return -1;

Even though, we are very unlikely to hit this check, the rest of the
function reports an error on error paths. Please virReportError() here too.

Michal

Re: [libvirt PATCH v3 8/8] qemu: turn two multiline log messages into single line

[Michal Prívozník]

On 8/21/23 21:32, Laine Stump wrote:
> Normally I wouldn't bother with a change like this, but I was touching
> the function anyway, and wanted to leave it looking nice and tidy.
> 
> Signed-off-by: Laine Stump 
> ---
>  src/qemu/qemu_driver.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f676744e9e..a60cbf0ed4 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11411,8 +11411,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
>   */
>  if (STREQ_NULLABLE(driverName, "kvm")) {
>  virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> -   _("'legacy KVM' device assignment is no longer "
> - "supported on this system"));
> +   _("'legacy KVM' device assignment is no longer 
> supported on this system"));
>  return -1;
>  }
>  
> @@ -11423,8 +11422,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
>  
>  if (!qemuHostdevHostSupportsPassthroughVFIO()) {
>  virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> -   _("VFIO device assignment is currently not "
> - "supported on this system"));
> +   _("VFIO device assignment is currently not supported 
> on this system"));
>   return -1;
>  }
>  

This got me thinking, whether we should do one huge commit which would
fix ALL the error messages in all files and just be done with it for
good. Again, future work, you patch is good as is.

Michal

Re: [libvirt PATCH v3 0/8] Support for VFIO variant drivers, Part 1

[Michal Prívozník]

On 8/21/23 21:32, Laine Stump wrote:
> A "VFIO variant" driver is a kernel driver for a device that supports
> all the APIs of the basic vfio-pci driver (which enables assigning a
> host PCI device to a QEMU guest) plus "some extra stuff" (e.g. to
> enable things like saving/restoring device state in order to support
> live migration.)
> 
> Way back last year I posted a couple attempts to support VFIO variant
> drivers; here is V2 (along with a later followup discussion from a
> couple months ago):
> 
> https://listman.redhat.com/archives/libvir-list/2022-August/233661.html
> https://listman.redhat.com/archives/libvir-list/2023-May/240108.html
> 
> The mlx5-vfio-pci driver has now been upstream for quite awhile (and
> even in the downstream Fedora 38 kernel, for example), as are the
> sysfs bits that allow us to determine whether or not a driver is a
> VFIO variant, and I've updated the patch(es) to use this.
> 
> I've also been working on auto-binding to the "best-match" VFIO
> variant driver based on comparing the device's modalias file in sysfs
> to the contents of the kernel's modules.alias file, but that isn't
> quite ready (partly code that isn't yet working, but also partly
> indecision about exactly where in the XML to put the driver name when
> it is specified; I won't take up more space here with that though).
> 
> In the meantime, there are people who want to use the mlx5-vfio-pci
> driver (and Cedric Le Goater also has written vfio-pci-igbvf and
> vfio-pci-e1000e drivers (which area very useful for testing), although
> I don't think he has posted them anywhere yet), so I would like to get
> the basic patches here merged in upstream now while I continue working
> on "Part 2".
> 
> These patches provide two improvements that make testing/using VFIO
> drivers much more convenient:
> 
> 1) The specific driver can be given in the virsh nodedev-detach
> command (or the virNodeDeviceDetachFlags() API call), e.g.:
> 
> virsh nodedev-detach pci__04_11_5 --driver vfio-pci-igbvf
> 
> 2) If the  (or " ... " has
> "managed='no'", then libvirt will recognize any VFIO variant driver
> (rather than the current behavior of rejecting anything that isn't
> exactly "vfio-pci")
> 
> With these two capabilities, it's simple and straightforward to bind a
> device to a VFIO variant driver, and then start a guest that uses that
> device.
> 
> Change in V2:
> 
> * complete remake, more refactoring
> 
> * use existence of "vfio-dev" subdirectory of device directory in
>   sysfs to determine whether the currently-bound driver is a vfio
>   variant.
> 
> * support binding to a user-specified driver during nodedev-detach,
>   rather than only supporting vfio-pci.
> 
> Laine Stump (8):
>   util: use "stubDriverType" instead of just "stubDriver"
>   util: add stub driver name to virPCIDevice object
>   util: rename virPCIDeviceGetDriverPathAndName
>   util: permit existing binding to VFIO variant driver
>   util: probe stub driver from within function that binds to stub driver
>   util: honor stubDriverName when probing/binding stub driver for a
> device
>   node_device: support binding other drivers with
> virNodeDeviceDetachFlags()
>   qemu: turn two multiline log messages into single line
> 
>  src/hypervisor/domain_driver.c |   9 +-
>  src/hypervisor/domain_driver.h |   2 +
>  src/hypervisor/virhostdev.c|  35 +++-
>  src/libvirt_private.syms   |   9 +-
>  src/libxl/libxl_driver.c   |   3 +-
>  src/qemu/qemu_driver.c |  37 
>  src/util/virnvme.c |   2 +-
>  src/util/virpci.c  | 156 +
>  src/util/virpci.h  |  18 ++--
>  tests/virhostdevtest.c |   2 +-
>  tests/virpcitest.c |  10 +--
>  11 files changed, 185 insertions(+), 98 deletions(-)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH] qemuValidateDomainVCpuTopology: Remove misconfiguration warning

[Michal Prívozník]

On 8/22/23 15:09, Peter Krempa wrote:
> Since commit baca59a5384 the NUMA definition is automatically fixed if
> the vCPU count mismatches the NUMA cpu count so that this warning will
> never be triggered.
> 
> Additionally VIR_WARN of a misconfiguration of a VM would not really
> be seen in most cases as it's only simply logged.
> 
> Signed-off-by: Peter Krempa 
> ---
> 
> This was originally in my series reworking qemuxml2xmltest. I've removed
> it but later figured out that it's dead code. This posting has an
> updated commit message.
> 
>  src/qemu/qemu_validate.c | 8 
>  1 file changed, 8 deletions(-)

Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCH] qemu: remove pointless qemuDomainLogContextMode

[Michal Prívozník]

On 8/16/23 14:24, Ján Tomko wrote:
> Since its introduction in 4d1b771fbb610537b7425e649a490143588b8ed3
> it has only been used to differentiate between START and non-START.
> 
> Last use of QEMU_DOMAIN_LOG_CONTEXT_MODE_ATTACH was removed by:
> 
>   commit f709377301b919a9fcbfc366e33057f7848bee28
> qemu: Fix qemuDomainObjTaint with virtlogd
> 
> QEMU_DOMAIN_LOG_CONTEXT_MODE_STOP is unused since:
> 
>   commit cf3ea0769c54a328733bcb0cd27f546e70090c89
> qemu: process: Append the "shutting down" message using the new APIs
> 
> Now, the only caller passes QEMU_DOMAIN_LOG_CONTEXT_MODE_START.
> Assume that's always the case and remove the 'mode' argument.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_domain.c  | 26 +++---
>  src/qemu/qemu_domain.h  |  9 +
>  src/qemu/qemu_process.c |  3 +--
>  3 files changed, 13 insertions(+), 25 deletions(-)

Reviewed-by: Michal Privoznik 

Michal

Sunset libvirt-snmp?

[Michal Prívozník]

It's been a while since libvirt-snmp was actively developed. Now it
receives only libvirt-ci related commits. The code compiles with
net-snmp-5.9.3 but the freshly released net-snmp-5.9.4 [1] breaks
compilation [2]. Now, libvirt-snmp has this crazy architecture, where
some sources are manually generated from src/LIBVIRT-MIB.txt, then
edited (added code to talk to libvirt) and then added to git.

This is labor extensive and since I don't think libvirt-snmp is actually
used I'd like to sunset it. According to repology [3] only Gentoo (and
its clones) has the latest version (released ~5 years ago). And I doubt
it has any real users there.

Thoughts?

1: https://sourceforge.net/projects/net-snmp/files/net-snmp/5.9.4/
2: https://bugs.gentoo.org/show_bug.cgi?id=912582
2: https://repology.org/project/libvirt-snmp/versions

Michal

Re: [libvirt PATCH 00/21] qemu: Various fixes to firmware selection

[Michal Prívozník]

On 8/10/23 18:50, Andrea Bolognani wrote:
> 
> Andrea Bolognani (21):
>   tests: Use DO_TEST_CAPS_*_ABI_UPDATE() for ppc64
>   tests: Switch to firmware autoselection for hvf
>   tests: Use virt-4.0 machine type for aarch64
>   tests: Consistently use /path/to/guest_VARS.fd
>   tests: Turn abi-update.xml into a symlink
>   tests: Rename firmware-auto-efi-nvram-path
>   qemu: Fix return value for qemuFirmwareFillDomainLegacy()
>   qemu: Fix lookup against stateless/combined pflash
>   tests: Add some more DO_TEST*ABI_UPDATE* macros
>   tests: Add more tests for firmware selection
>   tests: Update firmware descriptor files
>   tests: Drop tags from BIOS firmware descriptor
>   tests: Include microvm in firmwaretest
>   qemu: Don't overwrite NVRAM template for legacy firmware
>   qemu: Generate NVRAM path in more cases
>   qemu: Filter firmware based on loader.readonly
>   qemu: Match NVRAM template extension for new domains
>   conf: Don't default to raw format for loader/NVRAM
>   tests: Rename firmware-auto-efi-format-loader-qcow2-nvram-path
>   tests: Reintroduce firmware-auto-efi-format-mismatch
>   NEWS: Mention fixes to firmware selection
> 

>  196 files changed, 655 insertions(+), 491 deletions(-)


Reviewed-by: Michal Privoznik 

Re: [libvirt PATCH 0/2] ci: Introduce Debian 12 target

[Michal Prívozník]

On 8/11/23 14:15, Erik Skultety wrote:
> This miniseries also moves the CI workloads from Debian 11 to 12 and marks 11
> jobs as optional (just like we did for Debian 10 in the past).
> 
> Erik Skultety (2):
>   ci: Add Debian-12 target
>   ci: Move Debian-11 workloads to Debian-12
> 
>  ci/buildenv/debian-12-cross-aarch64.sh| 115 ++
>  ci/buildenv/debian-12-cross-armv6l.sh | 114 +
>  ci/buildenv/debian-12-cross-armv7l.sh | 115 ++
>  ci/buildenv/debian-12-cross-i686.sh   | 114 +
>  ci/buildenv/debian-12-cross-mips64el.sh   | 114 +
>  ci/buildenv/debian-12-cross-mipsel.sh | 114 +
>  ci/buildenv/debian-12-cross-ppc64le.sh| 114 +
>  ci/buildenv/debian-12-cross-s390x.sh  | 114 +
>  ci/buildenv/debian-12.sh  |  97 
>  .../debian-12-cross-aarch64.Dockerfile| 121 ++
>  .../debian-12-cross-armv6l.Dockerfile | 120 ++
>  .../debian-12-cross-armv7l.Dockerfile | 121 ++
>  ci/containers/debian-12-cross-i686.Dockerfile | 120 ++
>  .../debian-12-cross-mips64el.Dockerfile   | 120 ++
>  .../debian-12-cross-mipsel.Dockerfile | 120 ++
>  .../debian-12-cross-ppc64le.Dockerfile| 120 ++
>  .../debian-12-cross-s390x.Dockerfile  | 120 ++
>  ci/containers/debian-12.Dockerfile| 100 
>  ci/gitlab/builds.yml  | 216 ++
>  ci/gitlab/containers.yml  |  72 ++
>  ci/manifest.yml   |  40 
>  21 files changed, 2401 insertions(+)
>  create mode 100644 ci/buildenv/debian-12-cross-aarch64.sh
>  create mode 100644 ci/buildenv/debian-12-cross-armv6l.sh
>  create mode 100644 ci/buildenv/debian-12-cross-armv7l.sh
>  create mode 100644 ci/buildenv/debian-12-cross-i686.sh
>  create mode 100644 ci/buildenv/debian-12-cross-mips64el.sh
>  create mode 100644 ci/buildenv/debian-12-cross-mipsel.sh
>  create mode 100644 ci/buildenv/debian-12-cross-ppc64le.sh
>  create mode 100644 ci/buildenv/debian-12-cross-s390x.sh
>  create mode 100644 ci/buildenv/debian-12.sh
>  create mode 100644 ci/containers/debian-12-cross-aarch64.Dockerfile
>  create mode 100644 ci/containers/debian-12-cross-armv6l.Dockerfile
>  create mode 100644 ci/containers/debian-12-cross-armv7l.Dockerfile
>  create mode 100644 ci/containers/debian-12-cross-i686.Dockerfile
>  create mode 100644 ci/containers/debian-12-cross-mips64el.Dockerfile
>  create mode 100644 ci/containers/debian-12-cross-mipsel.Dockerfile
>  create mode 100644 ci/containers/debian-12-cross-ppc64le.Dockerfile
>  create mode 100644 ci/containers/debian-12-cross-s390x.Dockerfile
>  create mode 100644 ci/containers/debian-12.Dockerfile
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH 3/3] meson: Drop python3_prog

[Michal Prívozník]

On 8/16/23 12:25, Pavel Hrdina wrote:
> On Tue, Aug 15, 2023 at 02:45:37PM +0200, Michal Privoznik wrote:
>> Signed-off-by: Michal Privoznik 
>> ---
>>  docs/meson.build|  7 +++
>>  meson.build |  4 ++--
>>  src/access/meson.build  |  6 +++---
>>  src/meson.build | 43 +++--
>>  src/network/meson.build |  2 +-
>>  src/qemu/meson.build|  2 +-
>>  tests/meson.build   |  2 +-
>>  7 files changed, 31 insertions(+), 35 deletions(-)
> 
> Not sure we can do this. In meson.build we have the following:
> 
> required_programs = [
>   'perl',
>   'python3',
>   'xmllint',
>   'xsltproc',
> ]
> 
> foreach name : required_programs
>   prog = find_program(name, dirs: libvirt_sbin_path)
>   varname = name.underscorify()
>   conf.set_quoted(varname.to_upper(), prog.full_path())
>   set_variable('@0@_prog'.format(varname), prog)
> endforeach
> 
> which will set the python3_prog variable and we use that as our python
> executable.
> 
> I did a quick testing using the following meson.build file:
> 
> ---
> project('mesonpy', 'c')
> 
> required_programs = [
>   'python3',
> ]
> 
> foreach name : required_programs
>   prog = find_program(name)
>   varname = name.underscorify()
>   set_variable('@0@_prog'.format(varname), prog)
> endforeach
> 
> res1 = run_command(python3_prog, 'script.py')
> res2 = run_command('script.py')
> 
> warning(res1.stdout())
> warning(res2.stdout())
> ---
> 
> with the script.py having the following:
> 
> ---
> #!/usr/bin/env python3
> 
> import sys
> 
> print(sys.version)
> ---
> 
> and when I changed PATH to have python3 pointing to python3.12 but my
> system python is python3.11 I've got the following resutl:
> 
> meson.build:16: WARNING: 3.12.0b4 (main, Jul 12 2023, 00:00:00) [GCC 13.1.1 
> 20230614 (Red Hat 13.1.1-4)]
> 
> meson.build:17: WARNING: 3.11.4 (main, Jun  7 2023, 00:00:00) [GCC 13.1.1 
> 20230511 (Red Hat 13.1.1-2)]
> 
> 
> Don't remember the origin of the wrapper but my guess is that this was
> the main reason why we have it.

Right. But this is expected, isn't it? The same way, if you'd change any
other binary invoked from meson (say compiler) and we don't hardcode
paths for them either.

IOW - why should this behavior be problematic?

BTW: we already invoke python scripts WITHOUT pytho3_prog and it didn't
bother us yet: apibuild_prog, hyperv_wmi_generator_prog,
esx_vi_generator_prog to name a few.

Michal

Re: [libvirt PATCH 0/2] Fix the failing FreeBSD 13.1 CI build job

[Michal Prívozník]

On 8/16/23 10:52, Erik Skultety wrote:
> failing CI job reference: https://gitlab.com/libvirt/libvirt/-/jobs/4880861477
> 
> Even though 13.1 has been marked as EOL and we are rightfully permitted to 
> bump
> the version, we can't/shouldn't fix this directly in lcitool just yet, because
> libosinfo hasn't caught up on this fact yet and if we just bumped the cirrus
> version in the lcitool target OS config without updating the libosinfo ID we'd
> create a consistency problem.
> However, thanks to Paolo Bonzini's work on lcitool we have an easy workaround
> fix at hand where we can override many data-related things in lcitool.
> 
> Erik Skultety (2):
>   ci: Introduce a new 'lcitool' data directory
>   ci: Udate FreeBSD-13 image with lcitool manifest
> 
>  ci/gitlab/builds.yml  | 2 +-
>  ci/lcitool/targets/freebsd-13.yml | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>  create mode 100644 ci/lcitool/targets/freebsd-13.yml
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH 01/13] tools: Fix vshControl declaration and initialization

[Michal Prívozník]

On 8/3/23 19:53, Olaf Hering wrote:
> Thu,  3 Aug 2023 12:36:08 +0200 Michal Privoznik :
> 
>> +vshControl _ctl = { 0 };
> 
> I see this often, instead of a simple 'type variable = {};',
> and wonder what that zero is doing here?

That's C23 standard:

https://en.wikipedia.org/wiki/C23_(C_standard_revision)#Syntax

Libvirt requires C99 with GNU extensions which don't have that AFAIK.
OTOH, when turning on `-pedantic` compilation fails on the very first .c
file.

Anyway, it looks like we're using { 0 } everywhere else so that's why I
went with it. But switching to {} should be as easy as:

@@
@@
- { 0 }
+ {}

(untested spatch) Or maybe even a bit of sed trickery can do it.

Michal

Re: [PATCH 08/13] securityselinuxhelper: Use g_new0() instead of malloc()+memset() combo

[Michal Prívozník]

On 8/3/23 13:29, Claudio Fontana wrote:
> On 8/3/23 12:36, Michal Privoznik wrote:
>> Inside of securityselinuxhelper we still use malloc() +
>> memset(.., 0, ...) combo. Convert it to g_new0().
>>
>> Signed-off-by: Michal Privoznik 
> 
> I don't think it is a good idea to mix Glib g_new, g_free etc with malloc, 
> calloc, free.
> 
> If you go with g_new0 here, imo you need to also change the calls to free() 
> to g_free.
> 
> The alternative is to use calloc instead, then you could leave the existing 
> free calls alone.


Ah, good catch! Let me drop this from the series and post a switch to
calloc() then.

Thanks!

Michal

Re: [PATCH 1/3] qemu: Reflect MAC address change in live domain XML

[Michal Prívozník]

On 7/26/23 16:45, Martin Kletzander wrote:
> On Wed, Jun 28, 2023 at 12:53:35PM +0200, Michal Privoznik wrote:
>> If a guest changes MAC address on its vNIC, then QEMU emits
>> NIC_RX_FILTER_CHANGED event (the event is emitted in other cases
>> too, but that's not important right now). Now, domain XML allows
>> users to chose whether to trust these events or not:
>>
>>  
>>
>> For the 'no' case no action is performed and the event is
>> ignored. But for the 'yes' case, some host side features of
>> corresponding vNIC (well tap/macvtap device) are tweaked to
>> reflect changed MAC address. But what is missing is reflecting
>> this new MAC address in domain XML.
>>
>> Basically, what happens is: the host sees traffic with new MAC
>> address, all tools inside the guest see the new MAC address
>> (including 'virsh domifaddr --source agent') which makes it
>> harder to match device in the guest with the one in the domain
>> XML.
>>
>> NB, we should relay this event to clients, but that is covered in
>> next commits.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>> src/qemu/qemu_domain.c | 18 ++
>> src/qemu/qemu_driver.c |  2 +-
>> 2 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 94587638c3..5e5789a28c 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -12482,6 +12482,19 @@ syncNicRxFilterMulticast(char *ifname,
>> }
>>
>>
>> +/**
>> + * qemuDomainSyncRxFilter:
>> + * @vm: domain object
>> + * @def: domain interface definition
>> + * @asyncJob: async job type
>> + *
>> + * Fetch new state of RX Filter and set host side of the interface
>> + * accordingly (e.g. reflect MAC address change on macvtap).
>> + *
>> + * Reflect changed MAC address in the domain definition.
>> + *
>> + * Returns: 0 on success, -1 on error.
>> + */
>> int
>> qemuDomainSyncRxFilter(virDomainObj *vm,
>>    virDomainNetDef *def,
>> @@ -12535,6 +12548,11 @@ qemuDomainSyncRxFilter(virDomainObj *vm,
>>     return -1;
>>     }
>>
>> +    /* Reflect changed MAC address in the domain XML. */
>> +    if (virMacAddrCmp(>mac, >mac)) {
>> +    virMacAddrSet(>mac, >mac);
>> +    }
>> +
> 
> If we go with the idea I suggested this needs to be done even when we're
> not updating the filters.

You mean, even when this qemuDomainSyncRxFilter() function is not
called, i.e. update MAC addr from processNicRxFilterChangedEvent()?

We could do that, but this RX_FILTER_CHANGED event is a bit special. To
avoid flooding libvirt with this event, there's a antispam mechanism
implemented - after QEMU emits the event it awaits 'query-rx-filter'
monitor cmd. No other RX_FILTER_CHANGED event is emitted until the
monitor cmd is issued.

IOW, if we want to refresh MAC address on each event, we must
(unconditionally) issue the command and then (based on
trustGuestRxFilters) sync RX filters.

What I can't decide is whether it's better to reflect MAC change in
domain XML iff trustGuestRxFilters is set, or regardless.

Michal

Re: [PATCH 2/3] Introduce NIC_MAC_CHANGE event

[Michal Prívozník]

On 7/26/23 16:47, Martin Kletzander wrote:
> On Wed, Jun 28, 2023 at 12:53:36PM +0200, Michal Privoznik wrote:
>> The aim off this event is to notify management application that
>> guest changed MAC address on one of its vNICs so the app can
>> update its internal records, e.g. for finding match between
>> guest/host view of vNICs.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>> examples/c/misc/event-test.c    | 14 +
>> include/libvirt/libvirt-domain.h    | 28 +
>> src/conf/domain_event.c | 93 +
>> src/conf/domain_event.h | 12 
>> src/libvirt_private.syms    |  2 +
>> src/remote/remote_daemon_dispatch.c | 32 ++
>> src/remote/remote_driver.c  | 34 +++
>> src/remote/remote_protocol.x    | 17 +-
>> tools/virsh-domain-event.c  | 20 +++
>> 9 files changed, 251 insertions(+), 1 deletion(-)
>>
> 
> [...]
> 
>> diff --git a/src/remote/remote_daemon_dispatch.c
>> b/src/remote/remote_daemon_dispatch.c
>> index 7144e9e7ca..f347e7bcce 100644
>> --- a/src/remote/remote_daemon_dispatch.c
>> +++ b/src/remote/remote_daemon_dispatch.c
>> @@ -1357,6 +1357,37 @@
>> remoteRelayDomainEventMemoryDeviceSizeChange(virConnectPtr conn,
>> }
>>
>>
>> +static int
>> +remoteRelayDomainEventNICMACChange(virConnectPtr conn,
>> +   virDomainPtr dom,
>> +   const char *alias,
>> +   const char *oldMAC,
>> +   const char *newMAC,
>> +   void *opaque)
>> +{
>> +    daemonClientEventCallback *callback = opaque;
>> +    remote_domain_event_nic_mac_change_msg data;
>> +
>> +    if (callback->callbackID < 0 ||
>> +    !remoteRelayDomainEventCheckACL(callback->client, conn, dom))
>> +    return -1;
>> +
>> +    /* build return data */
>> +    memset(, 0, sizeof(data));
> 
> Just a nit pick, but instead of this you should be able to do:
> 
>     remote_domain_event_nic_mac_change_msg data = {0};

Yep, you're right. This is what you get when you copy code from an event
introduced earlier :-)

Fixed locally.

Michal

Re: [PATCH] NEWS: Document my contributions for upcoming release

[Michal Prívozník]

On 7/31/23 09:29, Martin Kletzander wrote:
> On Fri, Jul 28, 2023 at 04:40:50PM +0200, Michal Privoznik wrote:
>> Signed-off-by: Michal Privoznik 
>> ---
>> NEWS.rst | 17 +
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/NEWS.rst b/NEWS.rst
>> index 1de8314a61..0dc909c849 100644
>> --- a/NEWS.rst
>> +++ b/NEWS.rst
>> @@ -33,8 +33,25 @@ v9.6.0 (unreleased)
>>     ``/disk/target@bus='scsi'``) supports the ``removable`` attribute at
>>     ``/disk/target@removable```.
>>
>> +  * qemu: Add NUMA node automatically for memory hotplug
>> +
>> +    When enabling memory hotplug, libvirt required at least one guest
>> NUMA to
>> +    be specified in the domain XML. With this release, libvirt adds it
>> +    automatically when needed.
>> +
> 
> I think it sounds better if we say what libvirt does since this release
> rather
> than what used to happen before.  I would suggest something like:
> 
>     Users no longer need to specify guest NUMA node in the domain XML
> during
>     hotplug, libvirt automatically adds one when it is missing.

Good point. But we need better phrasing, because "memory hotplug" is
basically two things: enabling it in domain XML (e.g. by specifying
 or  - as examined by
virDomainDefHasMemoryHotplug()), and then there's actual hotplug whilst
the domain is running. The NUMA node is added only for the first part.
If domain is running we can not add a NUMA node, obviously.

Let me fix that in v2, just like the rest of your (valuable) comments.

Thanks!

Michal

Re: [PATCH 1/3] docs: formatdomain: Mention the QEMU version for igb

[Michal Prívozník]

On 7/28/23 10:09, Han Han wrote:
> Signed-off-by: Han Han 
> ---
>  docs/formatdomain.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index cd9cb02bf8..97ab367cff 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -5446,7 +5446,7 @@ Typical values for QEMU and KVM include: ne2k_isa 
> i82551 i82557b i82559er
>  ne2k_pci pcnet rtl8139 e1000 virtio. :since:`Since 5.2.0` ,
>  ``virtio-transitional`` and ``virtio-non-transitional`` values are supported.
>  See `Virtio transitional devices`_ for more details.
> -:since:`Since 9.3.0` igb is also supported.
> +:since:`Since 9.3.0(QEMU 8.0)` igb is also supported.

Missing space to separate libvirt version and '('. But then I am
wondering how useful this is. We usually don't document hypervisor
version, because especially QEMU is notoriously known to have active
downstream developers who backport even features (that's the whole
reason we have QEMU capabilities and try to avoid version checks as much
as possible).

Looking from another angle, the worst thing that will happen is: QEMU
refuses to start because of unknown NIC model. Well if that doesn't
suggest to the user to upgrade then I don't think our docs would.

Michal

Re: [PATCH 3/3] qemu/qemu_validate: Add capability check for the model igb

[Michal Prívozník]

On 7/28/23 12:59, Peter Krempa wrote:
> On Fri, Jul 28, 2023 at 16:09:38 +0800, Han Han wrote:
>> Signed-off-by: Han Han 
>> ---
>>  src/qemu/qemu_validate.c | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
>> index d5c2b2cd44..a5eb96c9b5 100644
>> --- a/src/qemu/qemu_validate.c
>> +++ b/src/qemu/qemu_validate.c
>> @@ -1858,6 +1858,13 @@ qemuValidateDomainDeviceDefNetwork(const 
>> virDomainNetDef *net,
>>  }
>>  }
>>  
>> +if (net->model == VIR_DOMAIN_NET_MODEL_IGB &&
>> +!virQEMUCapsGet(qemuCaps, QEMU_CAPS_IGB)) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +   _("interface model igb is not supported with this 
>> QEMU binary"));
>> +return -1;
>> +}
> 
> NACK, this was already discussed and is not needed:
> 
> https://listman.redhat.com/archives/libvir-list/2023-April/239345.html
> 

Agreed. And thus patch 2/3 is not necessary either (unless we want to
report supported models in domcapabilities, but that's orthogonal - we
don't do that, yet. And until somebody wants that I'd like to keep it
that way.).

Michal

Re: [PATCH v2] docs: Mention vhostuser for queues and queue_size

[Michal Prívozník]

On 7/20/23 11:00, Han Han wrote:
> These two attributes are supported for vhost-user-blk as well.
> 
> Signed-off-by: Han Han 
> ---
> Update the supported version of the queues attribute of vhost-user-blk
> v1: https://listman.redhat.com/archives/libvir-list/2023-July/240836.html
> 
>  docs/formatdomain.rst | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH v2] NEWS: qemu: Implement QEMU NBD reconnect delay attribute

[Michal Prívozník]

On 7/25/23 08:43, Han Han wrote:
> Signed-off-by: Han Han 
> ---
>  NEWS.rst | 5 +
>  1 file changed, 5 insertions(+)
Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCH 0/6] Sync cpu features with qemu

[Michal Prívozník]

On 7/13/23 13:45, Tim Wiederhake wrote:
> This brings libvirt in sync qith qemu commit
> 6f05a92ddc73ac8aa16cfd6188f907b30b0501e3.
> 
> Tim Wiederhake (6):
>   cpu_map: Add missing feature "mcdt-no"
>   cpu_map: Add missing feature "sbdr-ssdp-no"
>   cpu_map: Add missing feature "fbsdp-no"
>   cpu_map: Add missing feature "psdp-no"
>   cpu_map: Add missing feature "pbrsb-no"
>   sync_qemu_models_i386.py: Add missing features
> 
>  src/cpu_map/sync_qemu_models_i386.py |  7 +++
>  src/cpu_map/x86_features.xml | 16 
>  2 files changed, 23 insertions(+)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH 0/2] fix use-after-free in network IO

[Michal Prívozník]

On 7/4/23 09:10, Oleg Vasilev wrote:
> Found by repeatedly created and destroyed channels with guest agent. More
> details in the patch.
> 
> Oleg Vasilev (2):
>   net: add debug logs
>   remote: fix stream use-after-free
> 
>  src/remote/remote_daemon_stream.c | 13 +++--
>  src/rpc/virnetmessage.c   |  5 +
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 

Reviewed-by: Michal Privoznik 

and pushed.

Michal

Re: [PATCH 1/4] qemu: Generate shorter channel target paths

[Michal Prívozník]

On 7/21/23 14:27, Daniel P. Berrangé wrote:
> On Wed, Jul 12, 2023 at 04:49:53PM +0200, Michal Privoznik wrote:
>> A  device is basically an UNIX socket into guest.
>> Whatever is sent from the host, appears in the guest and vice
>> versa. But because of that, the length of the path to the socket
>> is important (underscored by fact that we derive the path from
>> domain short name). But there are still cases where we might not
>> fit into UNIX_PATH_MAX limit (usually 108 characters), because
>> the path is derived also from other variables, e.g.
>> XDG_CONFIG_HOME for session domains.
>>
>> There is one component though, that's needless: "/target/". Drop
>> it. This is safe to do, because running domains have their path
>> saved in status XML and even though paths are dropped on
>> migration, they are not part of guest ABI and thus we are free to
>> change them.
> 
> This mentions dropping '/target/' which makes sense, but...
> 
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 94587638c3..b4844351ba 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -1780,7 +1780,7 @@ qemuDomainSetPrivatePaths(virQEMUDriver *driver,
>>  priv->libDir = g_strdup_printf("%s/domain-%s", cfg->libDir, 
>> domname);
>>  
>>  if (!priv->channelTargetDir)
>> -priv->channelTargetDir = g_strdup_printf("%s/domain-%s",
>> +priv->channelTargetDir = g_strdup_printf("%s/%s",
>>   cfg->channelTargetDir, 
>> domname);
> 
> ...this is also dropping 'domain-' which also makes sense, but
> is not mentioned in the commit message.
> 
>> @@ -5352,9 +5352,12 @@ qemuDomainDefaultNetModel(const virDomainDef *def,
>>   * libvirt 1.2.19 - 1.3.2:
>>   * {cfg->channelTargetDir}/domain-{dom-name}/{target-name}
>>   *
>> - * libvirt 1.3.3 and newer:
>> + * libvirt 1.3.3 - 9.4.0:
>>   * 
>> {cfg->channelTargetDir}/domain-{dom-id}-{short-dom-name}/{target-name}
>>   *
>> + * libvirt 9.6.0 and newer:
>> + * {cfg->channelTargetDir}/{dom-id}-{short-dom-name}/{target-name}
>> + *
> 
> This doesn't make it clear that 'cfg->channelTargetDir' is actually
> different in those two examples.  Should we change th old ones to
> 
> libvirt 1.2.19 - 1.3.2:
> {cfg->channelTargetDir}/target/domain-{dom-name}/{target-name}
>
> libvirt 1.3.3 - 9.4.0:
> 
> {cfg->channelTargetDir}/target/domain-{dom-id}-{short-dom-name}/{target-name}
> 
> 

Good point, let me fix that.

> 
>>   * The unix socket path was stored in config XML until libvirt 1.3.0.
>>   * If someone specifies the same path as we generate, they shouldn't do it.
>>   *
>> @@ -5379,7 +5382,7 @@ qemuDomainChrDefDropDefaultPath(virDomainChrDef *chr,
>>  cfg = virQEMUDriverGetConfig(driver);
>>  
>>  virBufferEscapeRegex(, "^%s", cfg->channelTargetDir);
>> -virBufferAddLit(, "/([^/]+\\.)|(domain-[^/]+/)");
>> +virBufferAddLit(, 
>> "/(target/)?([^/]+\\.)|(domain-[^/]+/)|([0-9]+-[^/]+/)");
>>  virBufferEscapeRegex(, "%s$", chr->target.name);
> 
>   
>> diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-qemu-agent-detach.xml 
>> b/tests/qemuhotplugtestdevices/qemuhotplug-qemu-agent-detach.xml
>> index 0c3c70a78e..2e5cbfe09c 100644
>> --- a/tests/qemuhotplugtestdevices/qemuhotplug-qemu-agent-detach.xml
>> +++ b/tests/qemuhotplugtestdevices/qemuhotplug-qemu-agent-detach.xml
>> @@ -1,5 +1,5 @@
>>  
>> -  > path='/var/lib/libvirt/qemu/channel/target/domain-7-hotplug/org.qemu.guest_agent.0'/>
>> +  > path='/var/lib/libvirt/qemu/channel/target/7-hotplug/org.qemu.guest_agent.0'/>
> 
> You dropped 'domain-' but didn't drop '/target'. Same in a few other cases 
> below

Yeah, I need to squash in the next patch that handles this. Let me fix
that in v2.

Michal

Re: [PATCH 0/2] Improve the validation for queues and queue_size

[Michal Prívozník]

On 7/21/23 04:36, Han Han wrote:
> 
> Han Han (2):
>   conf/domain_validate.c: Improve the err for queue validation
>   conf/domain_validate: Validate the disk queue_size
> 
>  src/conf/domain_validate.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 

Reviewed-by: Michal Privoznik 

and pushed.

Michal

Re: [PATCH] docs: Mention vhostuser for queues and queue_size

[Michal Prívozník]

On 7/20/23 10:53, Han Han wrote:
> 
> 
> On Thu, Jul 20, 2023 at 2:56 PM Michal Prívozník  <mailto:mpriv...@redhat.com>> wrote:
> 
> On 7/20/23 08:22, Han Han wrote:
> > These two attributes are supported for vhost-user-blk as well.
> >
> > Signed-off-by: Han Han mailto:h...@redhat.com>>
> > ---
> >  docs/formatdomain.rst | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > index 4af0b82569..447ab32c01 100644
> > --- a/docs/formatdomain.rst
> > +++ b/docs/formatdomain.rst
> > @@ -3275,9 +3275,9 @@ paravirtualized driver is specified via the
> ``disk`` element.
> >        "virtio" ``bus`` and "pci" or "ccw" ``address`` types.
> :since:`Since 1.2.8
> >        (QEMU 2.1)`
> >     -  The optional ``queues`` attribute specifies the number of
> virt queues for
> > -      virtio-blk. ( :since:`Since 3.9.0` )
> > +      virtio-blk or vhost-user-blk. ( :since:`Since 3.9.0` )
> 
> This doesn't feel right. The vhost-user-blk disk was introduced fairly
> recently and 3.9.0 is just ancient. Digging into commits, vhost-user-blk
> disk was introduced in v7.1.0 and I didn't check whether it supported
> the attribute from the very beginning.
> 
> Thank you for pointing this out.  It looks libvirt allows the user to
> set the queues for
> all the bus=='virtio': 
> https://gitlab.com/libvirt/libvirt/-/blob/v7.1.0/src/conf/domain_validate.c#L465
>  
> <https://gitlab.com/libvirt/libvirt/-/blob/v7.1.0/src/conf/domain_validate.c#L465>
> 
> So vhost-user-blk could use the queues at the beginning.

Okay, can you post v2 then please? I'll review it.

Michal

Re: [PATCH 0/4] qemu: generate shorter channel target paths

[Michal Prívozník]

On 7/12/23 16:49, Michal Privoznik wrote:
>

Polite ping.

Michal

Re: [PATCH] storage: zfs: Use 'zfs list' to check pool status

[Michal Prívozník]

On 7/17/23 10:58, Peter Krempa wrote:
> On Mon, Jul 03, 2023 at 16:53:28 -0600, Matt Low wrote:
>> The current virtStorageBackendZFSCheckPool checks for the existence of a
>> path under /dev/zvol/ to determine if the pool is active. ZFS does not
>> create a path under /dev/zvol/ if no ZFS volumes have been created under
>> a particular dataset, thus, empty ZFS storage pools are deactivated
>> whenever checkPool is called on them (as noted in referenced issue).
>>
>> This commit changes virStorageBackendZFSCheckPool so that the 'zfs list'
>> command is used to explicitly check for the existence a dataset
>> specified by the pool's def->source.name.
>>
>> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/221
>>
>> Signed-off-by: Matt Low 
>> ---
>>  src/storage/storage_backend_zfs.c | 20 +---
>>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Peter Krempa 
> 
> I'll push it shortly.
> 

Pushed now.

Michal

Re: [PATCH] docs: Mention vhostuser for queues and queue_size

[Michal Prívozník]

On 7/20/23 08:22, Han Han wrote:
> These two attributes are supported for vhost-user-blk as well.
> 
> Signed-off-by: Han Han 
> ---
>  docs/formatdomain.rst | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 4af0b82569..447ab32c01 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -3275,9 +3275,9 @@ paravirtualized driver is specified via the ``disk`` 
> element.
>"virtio" ``bus`` and "pci" or "ccw" ``address`` types. :since:`Since 
> 1.2.8
>(QEMU 2.1)`
> -  The optional ``queues`` attribute specifies the number of virt queues 
> for
> -  virtio-blk. ( :since:`Since 3.9.0` )
> +  virtio-blk or vhost-user-blk. ( :since:`Since 3.9.0` )

This doesn't feel right. The vhost-user-blk disk was introduced fairly
recently and 3.9.0 is just ancient. Digging into commits, vhost-user-blk
disk was introduced in v7.1.0 and I didn't check whether it supported
the attribute from the very beginning.

I think if we want to document the attribute then we should probably do
it like this:

The optional ``queues`` attribute specifies the number of virt queues
for virtio-blk ( :sinnce:`Since 3.9.0) or vhost-user-blk ( :since:`Since
X.Y.Z` ).

> -  The optional ``queue_size`` attribute specifies the size of each virt
> -  queue for virtio-blk. ( :since:`Since 7.8.0` )
> +  queue for virtio-blk or vhost-user-blk. ( :since:`Since 7.8.0` )
> -  For virtio disks, `Virtio-related options`_ can also
>be set. ( :since:`Since 3.5.0` )
> -  The optional ``metadata_cache`` subelement controls aspects related to 
> the

I have not checked this later hunk.

Michal

Re: [PATCH 2/2] tests: Refresh valgrind suppressions

[Michal Prívozník]

On 7/19/23 12:50, Peter Krempa wrote:
> On Tue, Jul 18, 2023 at 14:28:57 +0200, Michal Privoznik wrote:
>> Since nobody is expected to run valgrind over scripts now, we can
>> drop plenty of suppressions. Also, there are some old ones that
>> no longer exist and new ones, that are not covered.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  tests/.valgrind.supp | 188 ++-
>>  1 file changed, 41 insertions(+), 147 deletions(-)
> 
> Is it really worth keeping this file in the repository? I don't think
> we can keep it current.
> 

We have valgrind test setup (add_test_setup() in tests/meson.build) that
passes this file. What we could have is a configure option that
specifies where the file is so that everybody can keep a copy specific
to their system. Would that work?

Michal

Re: [PATCH] qemu: Retire QEMU_CAPS_USB_STORAGE_REMOVABLE

[Michal Prívozník]

On 7/19/23 12:43, Peter Krempa wrote:
> On Thu, Jun 29, 2023 at 17:13:42 +0200, Michal Privoznik wrote:
>> Introduced in QEMU commit of v0.14.0-rc0~83^2~1 and not being
>> able to compile the .removable attribute of the "usb-storage"
>> object out, renders our corresponding capability
>> QEMU_CAPS_USB_STORAGE_REMOVABLE useless. Retire it.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_capabilities.c  |   9 +-
>>  src/qemu/qemu_capabilities.h  |   2 +-
>>  src/qemu/qemu_command.c   |  10 +-
>>  src/qemu/qemu_validate.c  |   8 -
>>  .../caps_4.2.0_aarch64.replies| 195 --
>>  .../caps_4.2.0_aarch64.xml|   1 -
>>  .../caps_4.2.0_ppc64.replies  | 179 +++-
>>  .../qemucapabilitiesdata/caps_4.2.0_ppc64.xml |   1 -
>>  .../caps_4.2.0_x86_64.replies | 211 +--
>>  .../caps_4.2.0_x86_64.xml |   1 -
>>  .../caps_5.0.0_aarch64.replies| 211 ---
>>  .../caps_5.0.0_aarch64.xml|   1 -
>>  .../caps_5.0.0_ppc64.replies  | 203 --
>>  .../qemucapabilitiesdata/caps_5.0.0_ppc64.xml |   1 -
>>  .../caps_5.0.0_riscv64.replies| 191 +++--
>>  .../caps_5.0.0_riscv64.xml|   1 -
>>  .../caps_5.0.0_x86_64.replies | 227 +---
>>  .../caps_5.0.0_x86_64.xml |   1 -
>>  .../caps_5.1.0_x86_64.replies | 231 +---
>>  .../caps_5.1.0_x86_64.xml |   1 -
>>  .../caps_5.2.0_aarch64.replies| 219 ---
>>  .../caps_5.2.0_aarch64.xml|   1 -
>>  .../caps_5.2.0_ppc64.replies  | 207 --
>>  .../qemucapabilitiesdata/caps_5.2.0_ppc64.xml |   1 -
>>  .../caps_5.2.0_riscv64.replies| 195 +++---
>>  .../caps_5.2.0_riscv64.xml|   1 -
>>  .../caps_5.2.0_x86_64.replies | 235 +---
>>  .../caps_5.2.0_x86_64.xml |   1 -
>>  .../caps_6.0.0_aarch64.replies| 217 ---
>>  .../caps_6.0.0_aarch64.xml|   1 -
>>  .../caps_6.0.0_x86_64.replies | 233 +---
>>  .../caps_6.0.0_x86_64.xml |   1 -
>>  .../caps_6.1.0_x86_64.replies | 239 +---
>>  .../caps_6.1.0_x86_64.xml |   1 -
>>  .../caps_6.2.0_aarch64.replies| 223 ---
>>  .../caps_6.2.0_aarch64.xml|   1 -
>>  .../caps_6.2.0_ppc64.replies  | 211 ---
>>  .../qemucapabilitiesdata/caps_6.2.0_ppc64.xml |   1 -
>>  .../caps_6.2.0_x86_64.replies | 243 +
>>  .../caps_6.2.0_x86_64.xml |   1 -
>>  .../caps_7.0.0_aarch64+hvf.replies| 227 
>>  .../caps_7.0.0_aarch64+hvf.xml|   1 -
>>  .../caps_7.0.0_aarch64.replies| 227 
>>  .../caps_7.0.0_aarch64.xml|   1 -
>>  .../caps_7.0.0_ppc64.replies  | 211 ---
>>  .../qemucapabilitiesdata/caps_7.0.0_ppc64.xml |   1 -
>>  .../caps_7.0.0_x86_64.replies | 243 +
>>  .../caps_7.0.0_x86_64.xml |   1 -
>>  .../caps_7.1.0_ppc64.replies  | 211 ---
>>  .../qemucapabilitiesdata/caps_7.1.0_ppc64.xml |   1 -
>>  .../caps_7.1.0_x86_64.replies | 243 +
>>  .../caps_7.1.0_x86_64.xml |   1 -
>>  .../caps_7.2.0_ppc.replies| 211 +++
>>  tests/qemucapabilitiesdata/caps_7.2.0_ppc.xml |   1 -
>>  .../caps_7.2.0_x86_64+hvf.replies | 255 +-
>>  .../caps_7.2.0_x86_64+hvf.xml |   1 -
>>  .../caps_7.2.0_x86_64.replies | 255 +-
>>  .../caps_7.2.0_x86_64.xml |   1 -
>>  .../caps_8.0.0_riscv64.replies| 207 +++---
>>  .../caps_8.0.0_riscv64.xml|   1 -
>>  .../caps_8.0.0_x86_64.replies | 255 +-
>>  .../caps_8.0.0_x86_64.xml |   1 -
>>  .../caps_8.1.0_x86_64.replies | 255 +-
>>  .../caps_8.1.0_x86_64.xml |   1 -
>>  64 files changed, 1704 insertions(+), 5025 deletions(-)
> 
> Please split this at least into a patch that retires the capability
> (removes the code), you can include removal of the detection of the
> capability but usually split that separately too.
> 
> For those one/two patches you can use:
> 
> Reviewed-by: Peter Krempa 
> 
> A separate patch then drops the actual probing of the usb-storage qemu
> device.
> 
> Additionally consider querying the 

Re: [PATCH 1/5] virsh: Make cmdVersion() work with split daemon

[Michal Prívozník]

On 7/18/23 17:47, Peter Krempa wrote:
> On Tue, Jul 18, 2023 at 17:27:33 +0200, Michal Privoznik wrote:
>> When virsh connects to a non-hypervisor daemon directly (e.g.
>> "nodedev:///system") and user executes 'version' they are met
>> with an error message. This is because cmdVersion() calls
>> virConnectGetVersion() which fails, hence the error.
>>
>> The reason for virConnectGetVersion() fail is simple - it's
>> documented as:
>>
>>   Get the version level of the Hypervisor running.
>>
>> Well, there's no hypervisor in non-hypervisor daemons and thus it
>> doesn't make sense to provide an implementation in each driver's
>> virConnectDriver.hypervisorDriver table (just like we do for
>> other APIs, e.g. nodeConnectIsSecure()).
>>
>> Given all of this, just make cmdVersion() deal with the error in
>> a non-fatal fashion.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  tools/virsh-host.c | 26 --
>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/virsh-host.c b/tools/virsh-host.c
>> index 0bda327cae..35e6a2eb98 100644
>> --- a/tools/virsh-host.c
>> +++ b/tools/virsh-host.c
>> @@ -1447,21 +1447,19 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd 
>> G_GNUC_UNUSED)
>>  vshPrint(ctl, _("Using API: %1$s %2$d.%3$d.%4$d\n"), hvType,
>>   major, minor, rel);
>>  
>> -if (virConnectGetVersion(priv->conn, ) < 0) {
>> -vshError(ctl, "%s", _("failed to get the hypervisor version"));
>> -return false;
>> -}
>> -if (hvVersion == 0) {
>> -vshPrint(ctl,
>> - _("Cannot extract running %1$s hypervisor version\n"), 
>> hvType);
>> -} else {
>> -major = hvVersion / 100;
>> -hvVersion %= 100;
>> -minor = hvVersion / 1000;
>> -rel = hvVersion % 1000;
>> +if (virConnectGetVersion(priv->conn, ) >= 0) {
>> +if (hvVersion == 0) {
>> +vshPrint(ctl,
>> + _("Cannot extract running %1$s hypervisor version\n"), 
>> hvType);
>> +} else {
>> +major = hvVersion / 100;
>> +hvVersion %= 100;
>> +minor = hvVersion / 1000;
>> +rel = hvVersion % 1000;
>>  
>> -vshPrint(ctl, _("Running hypervisor: %1$s %2$d.%3$d.%4$d\n"),
>> - hvType, major, minor, rel);
>> +vshPrint(ctl, _("Running hypervisor: %1$s %2$d.%3$d.%4$d\n"),
>> + hvType, major, minor, rel);
>> +}
>>  }
> 
> Ideally you'd also add an else branch with 'vshResetLibvirtError(); but
> the other call to virConnectGetLibVersion() doesn't do that so ...
> whatever ... I guess :)

Oh, you're right. In fact we should also ignore VIR_ERR_NO_SUPPORT.
Other errors might be actually a good reason to return early. Consider
this squashed in:

diff --git i/tools/virsh-host.c w/tools/virsh-host.c
index 35e6a2eb98..ad440d5123 100644
--- i/tools/virsh-host.c
+++ w/tools/virsh-host.c
@@ -1447,7 +1447,14 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd 
G_GNUC_UNUSED)
 vshPrint(ctl, _("Using API: %1$s %2$d.%3$d.%4$d\n"), hvType,
  major, minor, rel);
 
-if (virConnectGetVersion(priv->conn, ) >= 0) {
+if (virConnectGetVersion(priv->conn, ) < 0) {
+if (last_error->code == VIR_ERR_NO_SUPPORT) {
+vshResetLibvirtError();
+} else {
+vshError(ctl, "%s", _("failed to get the hypervisor version"));
+return false;
+}
+} else {


> 
>>  
>>  if (vshCommandOptBool(cmd, "daemon")) {
>> -- 
>> 2.41.0
>>
> 
> Reviewed-by: Peter Krempa 
> 

Thanks,

Michal

Re: [PATCH] qemu: Retire QEMU_CAPS_USB_STORAGE_REMOVABLE

[Michal Prívozník]

On 6/29/23 17:13, Michal Privoznik wrote:
>

Polite ping.

Michal

Re: [PATCH 1/2] NEWS: qemu: Support removable attribute for scsi disk

[Michal Prívozník]

On 7/17/23 12:56, Han Han wrote:
> Signed-off-by: Han Han 
> ---
>  NEWS.rst | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/NEWS.rst b/NEWS.rst
> index 42c2c53091..5ee880efcb 100644
> --- a/NEWS.rst
> +++ b/NEWS.rst
> @@ -27,6 +27,11 @@ v9.6.0 (unreleased)
>  ``abstractions/foo`` can be overridden by creating ``local/foo`` and
>  ``abstractions/foo.d`` respectively.
>  
> +  * qemu: Support removable attribute for scsi disk
> +
> +Now the scsi disk device (``/disk@device='disk'`` and 
> ``/disk/target@bus='scsi'``) supports

Long line.

> +the removable attribute at ``/disk/target@removable```.
> +
>  * **Bug fixes**
>  
>  

Michal

Re: [libvirt PATCH 2/4] Adding Public Get and Set APIs for Network Metadata

[Michal Prívozník]

On 7/11/23 08:47, K Shiva Kiran wrote:
> This patch introduces public Get and Set APIs for modifying ,
>  and  elements of the Network object.
> 
> - Added enum to select one of the above elements to operate on.
> - Added error code and messages for missing metadata.
> - Added public API implementation.
> - Added driver support.
> - Defined wire protocol format.
> 
> Signed-off-by: K Shiva Kiran 
> ---
>  include/libvirt/libvirt-network.h |  29 ++
>  include/libvirt/virterror.h   |   1 +
>  src/driver-network.h  |  16 +++
>  src/libvirt-network.c | 167 ++
>  src/libvirt_public.syms   |   6 ++
>  src/remote/remote_driver.c|   2 +
>  src/remote/remote_protocol.x  |  36 ++-
>  src/remote_protocol-structs   |  19 
>  src/util/virerror.c   |   3 +
>  9 files changed, 278 insertions(+), 1 deletion(-)
> 

We usually introduce public APIs in one patch and implement remote
driver in another. But I guess I can live with this.

> diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> index 3c6c230a16..14898a0bc7 100644
> --- a/src/remote_protocol-structs
> +++ b/src/remote_protocol-structs
> @@ -3130,6 +3130,23 @@ struct remote_network_port_delete_args {
>  remote_nonnull_network_port port;
>  u_int  flags;
>  };
> +struct remote_network_set_metadata_args {
> +remote_nonnull_network network;
> +inttype;
> +remote_string  metadata;
> +remote_string  key;
> +remote_string  uri;
> +u_int  flags;
> +};
> +struct remote_network_get_metadata_args {
> +remote_nonnull_network network;
> +inttype;
> +remote_string  uri;
> +u_int  flags;
> +};
> +struct remote_network_get_metadata_ret {
> +remote_nonnull_string  metadata;
> +};

This is misplaced. 

>  struct remote_domain_checkpoint_create_xml_args {
>  remote_nonnull_domain  dom;
>  remote_nonnull_string  xml_desc;
> @@ -3717,4 +3734,6 @@ enum remote_procedure {
>  REMOTE_PROC_DOMAIN_RESTORE_PARAMS = 441,
>  REMOTE_PROC_DOMAIN_ABORT_JOB_FLAGS = 442,
>  REMOTE_PROC_DOMAIN_FD_ASSOCIATE = 443,
> +REMOTE_PROC_NETWORK_SET_METADATA = 444,
> +REMOTE_PROC_NETWORK_GET_METADATA = 445

Here we want the trailing comma.

>  };

Squash in the following:

diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 14898a0bc7..c07e0af1e6 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -2687,6 +2687,23 @@ struct remote_network_event_lifecycle_msg {
 intevent;
 intdetail;
 };
+struct remote_network_set_metadata_args {
+remote_nonnull_network network;
+inttype;
+remote_string  metadata;
+remote_string  key;
+remote_string  uri;
+u_int  flags;
+};
+struct remote_network_get_metadata_args {
+remote_nonnull_network network;
+inttype;
+remote_string  uri;
+u_int  flags;
+};
+struct remote_network_get_metadata_ret {
+remote_nonnull_string  metadata;
+};
 struct remote_connect_storage_pool_event_register_any_args {
 inteventID;
 remote_storage_poolpool;
@@ -3130,23 +3147,6 @@ struct remote_network_port_delete_args {
 remote_nonnull_network_port port;
 u_int  flags;
 };
-struct remote_network_set_metadata_args {
-remote_nonnull_network network;
-inttype;
-remote_string  metadata;
-remote_string  key;
-remote_string  uri;
-u_int  flags;
-};
-struct remote_network_get_metadata_args {
-remote_nonnull_network network;
-inttype;
-remote_string  uri;
-u_int  flags;
-};
-struct remote_network_get_metadata_ret {
-remote_nonnull_string  metadata;
-};
 struct remote_domain_checkpoint_create_xml_args {
 remote_nonnull_domain  dom;
 remote_nonnull_string  xml_desc;
@@ -3735,5 +3735,5 @@ enum remote_procedure {
 REMOTE_PROC_DOMAIN_ABORT_JOB_FLAGS = 442,
 REMOTE_PROC_DOMAIN_FD_ASSOCIATE = 443,
 REMOTE_PROC_NETWORK_SET_METADATA = 444,
-REMOTE_PROC_NETWORK_GET_METADATA = 445
+REMOTE_PROC_NETWORK_GET_METADATA = 445,
 };


Michal

Re: [libvirt PATCH 0/4] Introduce new Metadata fields for Network object with corresponding APIs

[Michal Prívozník]

On 7/11/23 08:47, K Shiva Kiran wrote:
> This commit introduces  and  fields to the 
> XML schema of the Network object.
> It also adds public Get/Set APIs for modifying all metadata fields
> of the same, along with a test program.
> 
> K Shiva Kiran (4):
>   Add  and  for Network Objects
>   Adding Public Get and Set APIs for Network Metadata
>   Add virNetworkObj Get and Set Methods for Metadata
>   Add Test driver and testcase for Network Metadata change APIs
> 
>  docs/formatnetwork.rst|  11 +
>  include/libvirt/libvirt-network.h |  29 +++
>  include/libvirt/virterror.h   |   1 +
>  src/conf/network_conf.c   |  21 ++
>  src/conf/network_conf.h   |   2 +
>  src/conf/schemas/basictypes.rng   |  15 ++
>  src/conf/schemas/domaincommon.rng |  15 --
>  src/conf/schemas/network.rng  |  10 +
>  src/conf/virnetworkobj.c  | 325 ++
>  src/conf/virnetworkobj.h  |  17 ++
>  src/driver-network.h  |  16 ++
>  src/libvirt-network.c | 167 +++
>  src/libvirt_public.syms   |   6 +
>  src/remote/remote_driver.c|   2 +
>  src/remote/remote_protocol.x  |  36 +++-
>  src/remote_protocol-structs   |  19 ++
>  src/test/test_driver.c|  67 ++
>  src/util/virerror.c   |   3 +
>  tests/meson.build |   1 +
>  tests/networkmetadatatest.c   | 297 +++
>  20 files changed, 1044 insertions(+), 16 deletions(-)
>  create mode 100644 tests/networkmetadatatest.c
> 

No virsh exposure of these new APIs? :-(

Michal

Re: [libvirt PATCH 3/4] Add virNetworkObj Get and Set Methods for Metadata

[Michal Prívozník]

On 7/11/23 08:47, K Shiva Kiran wrote:
> - Introduces virNetworkObjGetMetadata() and
>   virNetworkObjSetMetadata().
> - These functions implement common behaviour that can be reused by
>   network drivers that use the virNetworkObj struct.
> - Also adds helper functions that resolve the live/persistent state of
>   the network before setting metadata.
> 
> Signed-off-by: K Shiva Kiran 
> ---
>  src/conf/virnetworkobj.c | 325 +++
>  src/conf/virnetworkobj.h |  17 ++
>  2 files changed, 342 insertions(+)
> 
> diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
> index b8b86da06f..41d0a1d971 100644
> --- a/src/conf/virnetworkobj.c
> +++ b/src/conf/virnetworkobj.c
> @@ -1822,3 +1822,328 @@ virNetworkObjLoadAllPorts(virNetworkObj *net,
>  
>  return 0;
>  }
> +
> +
> +/**
> + * virNetworkObjUpdateModificationImpact:
> + *
> + * @net: network object
> + * @flags: flags to update the modification impact on
> + *
> + * Resolves virNetworkUpdateFlags in @flags so that they correctly
> + * apply to the actual state of @net. @flags may be modified after call to 
> this
> + * function.
> + *
> + * Returns 0 on success if @flags point to a valid combination for @net or 
> -1 on
> + * error.
> + */
> +static int
> +virNetworkObjUpdateModificationImpact(virNetworkObj *net,
> + unsigned int *flags)
> +{
> +bool isActive = virNetworkObjIsActive(net);
> +
> +if ((*flags & (VIR_NETWORK_UPDATE_AFFECT_LIVE | 
> VIR_NETWORK_UPDATE_AFFECT_CONFIG)) ==
> +VIR_NETWORK_UPDATE_AFFECT_CURRENT) {
> +if (isActive)
> +*flags |= VIR_NETWORK_UPDATE_AFFECT_LIVE;
> +else
> +*flags |= VIR_NETWORK_UPDATE_AFFECT_CONFIG;
> +}
> +
> +if (!isActive && (*flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) {
> +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +   _("network is not running"));
> +return -1;
> +}
> +
> +if (!net->persistent && (*flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG)) {
> +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +   _("transient networks do not have any "
> + "persistent config"));

I'm going to mention it here, but it applies to the all of your patches,
and all the new code in fact. We made an exemption for error messages
and thus they don't need to be broken at 80 chars limit. In fact, they
shouldn't. The reason is simple: easier grep as one doesn't have to try
and guess how is the message broken into individual substrings. Of
course, you will find plenty of "bad" examples throughout the code, but
that's because it is an old code. Whenever we rewrite something or
introduce new code this exception applies and the old code is, well,
gradually fixed.

> +return -1;
> +}
> +
> +return 0;


This code is basically the same as in networkUpdate(). The first part
that sets _LIVE or _CONFIG is there verbatim, the second one is hidden
under virNetworkObjConfigChangeSetup(). There's one extra step that the
function does - it calls virNetworkObjSetDefTransient() but I don't
think that's necessary. Either the network is active and
virNetworkObjSetDefTransient() was already called, or is inactive and
thus it's a NOP. IOW, the call to virNetworkObjSetDefTransient() can be
removed.

After this, virNetworkObjUpdateModificationImpact() could be exported
(just like corresponding virDomain* sibling function is) so that it can
be called from both src/conf/virnetworkobj.c and
src/network/bridge_driver.c. Because I think we can get away with the
networkUpdate() doing the following:

networkUpdate() {
  ...
  virNetworkObjUpdateModificationImpact();

  if (isActive && (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) {
/* This is the check that possibly calls
networkRemoveFirewallRules() and sets needFirewallRefresh = true */
  }

  virNetworkObjUpdate();
  ...
}

BTW: when cooking the patch, don't forget the same pattern is copied to
our test driver (src/test/test_driver.c).


Michal

Re: [libvirt PATCH] qemu: require memfd memory for virtio 'blob' support

[Michal Prívozník]

On 7/18/23 16:48, Jonathon Jongsma wrote:
> The virtio-gpu 'blob' support was insufficiently validated. Qemu
> requires a memfd memory backing in order to use udmabuf and enable blob
> support. Example error:
> 
> $ virsh start rhel9
> error: Failed to start domain 'rhel9'
> error: internal error: qemu unexpectedly closed the monitor: 
> 2023-07-18T02:33:57.083178Z qemu-kvm: -device 
> {"driver":"virtio-vga","id":"video0","max_outputs":1,"blob":true,"bus":"pcie.0","addr":"0x1"}:
>  cannot enable blob resources without udmabuf
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  src/qemu/qemu_validate.c | 20 ++--
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 

You forgot to update tests. Squash this in:


diff --git i/tests/qemuxml2argvdata/video-virtio-blob-on.x86_64-latest.args 
w/tests/qemuxml2argvdata/video-virtio-blob-on.x86_64-latest.args
index ef37e32e5e..577422426b 100644
--- i/tests/qemuxml2argvdata/video-virtio-blob-on.x86_64-latest.args
+++ w/tests/qemuxml2argvdata/video-virtio-blob-on.x86_64-latest.args
@@ -14,7 +14,7 @@ 
XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
 -accel tcg \
 -cpu qemu64 \
 -m size=1048576k \
--object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
+-object 
'{"qom-type":"memory-backend-memfd","id":"pc.ram","x-use-canonical-path-for-ramblock-id":false,"size":1073741824}'
 \
 -overcommit mem-lock=off \
 -smp 1,sockets=1,cores=1,threads=1 \
 -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
diff --git i/tests/qemuxml2argvdata/video-virtio-blob-on.xml 
w/tests/qemuxml2argvdata/video-virtio-blob-on.xml
index 2b8a913f49..96ccf13079 100644
--- i/tests/qemuxml2argvdata/video-virtio-blob-on.xml
+++ w/tests/qemuxml2argvdata/video-virtio-blob-on.xml
@@ -3,6 +3,9 @@
   c7a5fdbd-edaf-9455-926a-d65c16db1809
   1048576
   1048576
+  
+
+  
   1
   
 hvm
diff --git i/tests/qemuxml2xmloutdata/video-virtio-blob-on.x86_64-latest.xml 
w/tests/qemuxml2xmloutdata/video-virtio-blob-on.x86_64-latest.xml
index 410db67592..40f40b4132 100644
--- i/tests/qemuxml2xmloutdata/video-virtio-blob-on.x86_64-latest.xml
+++ w/tests/qemuxml2xmloutdata/video-virtio-blob-on.x86_64-latest.xml
@@ -3,6 +3,9 @@
   c7a5fdbd-edaf-9455-926a-d65c16db1809
   1048576
   1048576
+  
+
+  
   1
   
 hvm


Reviewed-by: Michal Privoznik 

Michal