Re: [PATCH v2 05/22] qemu: support automatic VM managed save in system daemon

[Peter Krempa]

On Wed, Mar 12, 2025 at 17:17:45 +, Daniel P. Berrangé wrote:
> Currently automatic VM managed save is only performed in session
> daemons, on desktop session close, or host OS shutdown request.
> 
> With this change it is possible to control shutdown behaviour for
> all daemons. A recommended setup might be:
> 
>   auto_shutdown_try_save = "persistent"
>   auto_shutdown_try_shutdown = "all"
>   auto_shutdown_poweroff = "all"
> 
> Each setting accepts 'none', 'persistent', 'transient', and 'all'
> to control what types of guest it applies to.
> 
> For historical compatibility, for the system daemon, the settings
> currently default to:
> 
>   auto_shutdown_try_save = "none"
>   auto_shutdown_try_shutdown = "none"
>   auto_shutdown_poweroff = "none"
> 
> while for the session daemon they currently default to
> 
>   auto_shutdown_try_save = "persistent"
>   auto_shutdown_try_shutdown = "none"
>   auto_shutdown_poweroff = "none"
> 
> The system daemon settings should NOT be enabled if the traditional
> libvirt-guests.service is already enabled.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/qemu/libvirtd_qemu.aug |  3 ++
>  src/qemu/qemu.conf.in  | 42 +++
>  src/qemu/qemu_conf.c   | 65 ++
>  src/qemu/qemu_conf.h   |  4 ++
>  src/qemu/qemu_driver.c |  9 ++---
>  src/qemu/test_libvirtd_qemu.aug.in |  3 ++
>  6 files changed, 121 insertions(+), 5 deletions(-)

[...]

> diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
> index 31172303dc..a674e11388 100644
> --- a/src/qemu/qemu.conf.in
> +++ b/src/qemu/qemu.conf.in
> @@ -639,6 +639,48 @@
>  #
>  #auto_start_delay = 0
>  
> +# The settings for auto shutdown actions accept one of
> +# four possible options:
> +#
> +# * "none" - do not try to save any running VMs
> +# * "persistent" - only try to save persistent running VMs
> +# * "transient" - only try to save transient running VMs
> +# * "all" - try to save all running VMs
> +#
I'd suggest you change the commented-out empty line here to uncommented,
to separate the docs for auto_shutdown_try_save from the common section
above.

> +# Whether to perform managed save of running VMs if a host OS
> +# shutdown is requested (system/session daemons), or the desktop
> +# session terminates (session daemon only).
> +#
> +# Defaults to "persistent" for session daemons and "none"
> +# for system daemons. The values "all" and "transient" are
> +# not permitted for this setting, since managed save is not
> +# implemented for transient VMs.
> +#
> +# If 'libvirt-guests.service' is enabled, then this must be
> +# set to 'none' for system daemons to avoid dueling actions
> +#auto_shutdown_try_save = "persistent"
> +

Reviewed-by: Peter Krempa 

Re: [PATCH v2 05/22] qemu: support automatic VM managed save in system daemon

[Pavel Hrdina]

On Wed, Mar 12, 2025 at 05:17:45PM +, Daniel P. Berrangé wrote:
> Currently automatic VM managed save is only performed in session
> daemons, on desktop session close, or host OS shutdown request.
> 
> With this change it is possible to control shutdown behaviour for
> all daemons. A recommended setup might be:
> 
>   auto_shutdown_try_save = "persistent"
>   auto_shutdown_try_shutdown = "all"
>   auto_shutdown_poweroff = "all"
> 
> Each setting accepts 'none', 'persistent', 'transient', and 'all'
> to control what types of guest it applies to.
> 
> For historical compatibility, for the system daemon, the settings
> currently default to:
> 
>   auto_shutdown_try_save = "none"
>   auto_shutdown_try_shutdown = "none"
>   auto_shutdown_poweroff = "none"
> 
> while for the session daemon they currently default to
> 
>   auto_shutdown_try_save = "persistent"
>   auto_shutdown_try_shutdown = "none"
>   auto_shutdown_poweroff = "none"
> 
> The system daemon settings should NOT be enabled if the traditional
> libvirt-guests.service is already enabled.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/qemu/libvirtd_qemu.aug |  3 ++
>  src/qemu/qemu.conf.in  | 42 +++
>  src/qemu/qemu_conf.c   | 65 ++
>  src/qemu/qemu_conf.h   |  4 ++
>  src/qemu/qemu_driver.c |  9 ++---
>  src/qemu/test_libvirtd_qemu.aug.in |  3 ++
>  6 files changed, 121 insertions(+), 5 deletions(-)

[...]

> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index b376841388..8554558201 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c

[...]

> @@ -627,6 +642,8 @@ virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfig *cfg,
>  g_autofree char *savestr = NULL;
>  g_autofree char *dumpstr = NULL;
>  g_autofree char *snapstr = NULL;
> +g_autofree char *autoShutdownScope = NULL;
> +int autoShutdownScopeVal;
>  
>  if (virConfGetValueString(conf, "save_image_format", &savestr) < 0)
>  return -1;
> @@ -663,6 +680,54 @@ virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfig 
> *cfg,
>  return -1;
>  if (virConfGetValueUInt(conf, "auto_start_delay", 
> &cfg->autoStartDelayMS) < 0)
>  return -1;
> +if (virConfGetValueString(conf, "auto_shutdown_try_save", 
> &autoShutdownScope) < 0)
> +return -1;
> +
> +if (autoShutdownScope != NULL) {
> +if ((autoShutdownScopeVal =
> + 
> virDomainDriverAutoShutdownScopeTypeFromString(autoShutdownScope)) < 0) {
> +virReportError(VIR_ERR_INVALID_ARG,
> +   _("unknown auto_shutdown_try_save '%1$s'"),
> +   autoShutdownScope);
> +return -1;
> +}
> +cfg->autoShutdownTrySave = autoShutdownScopeVal;
> +}
> +
> +if (cfg->autoShutdownTrySave == 
> VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL ||
> +cfg->autoShutdownTrySave == 
> VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_TRANSIENT) {
> +virReportError(VIR_ERR_INVALID_ARG, "%s",
> +   _("managed save cannot be requested for transient 
> domains"));
> +return -1;
> +}
> +
> +if (virConfGetValueString(conf, "auto_shutdown_try_shutdown", 
> &autoShutdownScope) < 0)

I wanted to note the we would leak memory here when reusing the same
string but virConfGetValueString frees the value before assigning new
data to it. But it doesn't touch the variable if the config option is
missing so the next check for NULL can be invalid if
auto_shutdown_try_save is set but auto_shutdown_try_shutdown is not set.

> +return -1;
> +
> +if (autoShutdownScope != NULL) {
> +if ((autoShutdownScopeVal =
> + 
> virDomainDriverAutoShutdownScopeTypeFromString(autoShutdownScope)) < 0) {
> +virReportError(VIR_ERR_INVALID_ARG,
> +   _("unknown auto_shutdown_try_shutdown '%1$s'"),
> +   autoShutdownScope);
> +return -1;
> +}
> +cfg->autoShutdownTryShutdown = autoShutdownScopeVal;
> +}
> +
> +if (virConfGetValueString(conf, "auto_shutdown_poweroff", 
> &autoShutdownScope) < 0)
> +return -1;

Same here.

> +if (autoShutdownScope != NULL) {
> +if ((autoShutdownScopeVal =
> + 
> virDomainDriverAutoShutdownScopeTypeFromString(autoShutdownScope)) < 0) {
> +virReportError(VIR_ERR_INVALID_ARG,
> +   _("unknown auto_shutdown_poweroff '%1$s'"),
> +   autoShutdownScope);
> +return -1;
> +}
> +cfg->autoShutdownPoweroff = autoShutdownScopeVal;
> +}
>  
>  return 0;
>  }

Pavel


signature.asc
Description: PGP signature

Re: [PATCH v2 17/22] rpc: move state stop into virNetDaemon class

[Peter Krempa]

On Wed, Mar 12, 2025 at 17:17:57 +, Daniel P. Berrangé wrote:
> Currently the remote daemon code is responsible for calling virStateStop
> in a background thread. The virNetDaemon code wants to synchronize with
> this during shutdown, however, so the virThreadPtr must be passed over.
> 
> Even the limited synchronization done currently, however, is flawed and
> to fix this requires the virNetDaemon code to be responsible for calling
> virStateStop in a thread more directly.
> 
> Thus the logic is moved over into virStateStop via a further callback
> to be registered by the remote daemon.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/libvirt_remote.syms|  2 +-
>  src/remote/remote_daemon.c | 40 ++
>  src/rpc/virnetdaemon.c | 59 +++---
>  src/rpc/virnetdaemon.h | 27 +++--
>  4 files changed, 77 insertions(+), 51 deletions(-)

Reviewed-by: Peter Krempa 

Re: [PATCH v2 21/22] rpc: don't let systemd shutdown daemon while saving VMs

[Peter Krempa]

On Wed, Mar 12, 2025 at 17:18:01 +, Daniel P. Berrangé wrote:
> The service unit "TimeoutStopSec" setting controls how long systemd
> waits for a service to stop before aggressively killing it, defaulting
> to 30 seconds if not set.
> 
> When we're processing shutdown of VMs in response to OS shutdown, we
> very likely need more than 30 seconds to complete this job, and can
> not stop the daemon during this time.
> 
> To avoid being prematurely killed, setup a timer that repeatedly
> extends the "TimeoutStopSec" value while stop of running VMs is
> arranged.
> 
> This does mean if libvirt hangs while stoppping VMs, systemd won't
> get to kill the libvirt daemon, but this is considered less harmful
> that forcefully killing running VMs.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/rpc/virnetdaemon.c | 53 +-
>  1 file changed, 52 insertions(+), 1 deletion(-)

Reviewed-by: Peter Krempa 

Re: [PATCH v2 22/22] hypervisor: emit systemd status & log messages while saving

[Peter Krempa]

On Wed, Mar 12, 2025 at 17:18:02 +, Daniel P. Berrangé wrote:
> Since processing running VMs on OS shutdown can take a while, it is
> beneficial to send systemd status messages about the progress.
> 
> The systemd status is a point-in-time message, with no ability to
> look at the history of received messages. So in the systemd status
> we include the progress information. For the same reason there is
> no benefit in sending failure messages, as they'll disappear as soon
> as a status is sent for the subsequent VM to be processed.
> 
> The libvirt log statements can be viewed as a complete log record
> so don't need progress info, but do include warnings about failures
> (present from earlier commits).
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/hypervisor/domain_driver.c | 22 ++
>  1 file changed, 22 insertions(+)

Reviewed-by: Peter Krempa 

Re: [PATCH 3/6] ch: Don't leak virCHDomainObjPrivate struct members

[Peter Krempa]

On Thu, Mar 13, 2025 at 15:26:52 +0100, Peter Krempa wrote:
> On Thu, Mar 13, 2025 at 14:44:35 +0100, Michal Privoznik wrote:
> > There are some members of the virCHDomainObjPrivate struct that
> > are allocated at various stages of domain lifecycle but then are
> > never freed:
> > 
> > 1) cgroup - allocated in virDomainCgroupSetupCgroup()
> > 2) autoCpuset - this one is actually never allocated (and thus is
> > always NULL, but soon it may be used. Just free
> > it for now, which is a NOP anyways.
> > 3) autoNodeset - same story as 2).
> 
> So wait; was it copied from qemu without being used?
> 
> Will it actually be used soon? If no; I'd prefer to just delete the
> members instead.

Ah I see that it's actually at least attempted to be read. So ignore
this and

Reviewed-by: Peter Krempa 

Re: [PATCH v8 11/18] qemu: block: Support block disk along with throttle filters

[Peter Krempa]

On Wed, Feb 19, 2025 at 22:27:15 +0530, Harikumar Rajkumar wrote:
> From: Chun Feng Wu 
> 
> For hot attaching/detaching
> * Leverage qemuBlockThrottleFiltersData to prepare attaching/detaching
>   throttle filter data for qemuMonitorBlockdevAdd and qemuMonitorBlockdevDel
> * For hot attaching, within qemuDomainAttachDiskGeneric,prepare throttle
>   filters json data, and create corresponding blockdev for QMP request
>   ("blockdev-add" with "driver":"throttle")
> * Each filter has a nodename, and those filters are chained up,
>   create them in sequence, and delete them reversely
> * Delete filters by "qemuBlockThrottleFiltersDetach"("blockdev-del")
>   when detaching device
> 
> For throttle group commandline
> * Add qemuBuildThrottleGroupCommandLine in qemuBuildCommandLine to add
>   "object" of throttle-group
> * Verify throttle group definition when lauching vm
> * Check QEMU_CAPS_OBJECT_JSON before "qemuBuildObjectCommandlineFromJSON",
>   which is to build "-object" option
> 
> For throttle filter commandline
> * Add qemuBuildDiskThrottleFiltersCommandLine in qemuBuildDiskCommandLine
>   to add "blockdev"
> 
> Signed-off-by: Chun Feng Wu 
> 
> * Apply suggested coding style changes.
> * Update of code documentation comments.
> 
> Signed-off-by: Harikumar Rajkumar 
> ---
>  src/qemu/qemu_command.c | 99 +
>  src/qemu/qemu_hotplug.c | 21 +
>  2 files changed, 120 insertions(+)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 750edd4faa..6d9efbe887 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2045,6 +2045,99 @@ 
> qemuBuildBlockStorageSourceAttachDataCommandline(virCommand *cmd,
>  }
>  
>  
> +static inline bool
> +qemuDiskConfigThrottleGroupEnabled(const virDomainThrottleGroupDef *group)
> +{
> +return !!group->group_name &&
> +   virDomainBlockIoTuneInfoHasAny(group);
> +}
> +
> +
> +/**
> + * qemuBuildThrottleGroupCommandLine:
> + * @cmd: the command to modify
> + * @def: domain definition
> + * @qemuCaps: qemu capabilities object
> + *
> + * build throttle group object in json format
> + */
> +static int
> +qemuBuildThrottleGroupCommandLine(virCommand *cmd,
> +  const virDomainDef *def,
> +  virQEMUCaps *qemuCaps)
> +{
> +size_t i;
> +
> +for (i = 0; i < def->nthrottlegroups; i++) {
> +g_autoptr(virJSONValue) props = NULL;
> +g_autoptr(virJSONValue) limits = virJSONValueNewObject();
> +virDomainThrottleGroupDef *group = def->throttlegroups[i];
> +/* prefix group name with "throttle-" in QOM */
> +g_autofree char *prefixed_group_name = 
> g_strdup_printf("throttle-%s", group->group_name);
> +
> +if (!qemuDiskConfigThrottleGroupEnabled(group)) {
> +continue;
> +}
> +
> +if (qemuMonitorThrottleGroupLimits(limits, group) < 0)
> +return -1;
> +
> +if (qemuMonitorCreateObjectProps(&props, "throttle-group", 
> prefixed_group_name,
> + "a:limits", &limits,
> + NULL) < 0)
> +return -1;
> +
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_JSON)) {
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +   _("This QEMU doesn't support throttle group 
> creation"));
> +return -1;

This would be considered too late for this check; Luckily all of it can
be deledet now.

> +}
> +
> +if (qemuBuildObjectCommandlineFromJSON(cmd, props, qemuCaps) < 0)
> +return -1;
> +}
> +
> +return 0;
> +}
> +
> +
> +static int
> +qemuBuildBlockThrottleFilterCommandline(virCommand *cmd,
> +qemuBlockThrottleFilterAttachData 
> *data)
> +{
> +if (data->filterProps) {
> +g_autofree char *tmp = NULL;
> +if (!(tmp = virJSONValueToString(data->filterProps, false)))
> +return -1;
> +
> +virCommandAddArgList(cmd, "-blockdev", tmp, NULL);
> +}
> +
> +return 0;
> +}
> +
> +
> +static int
> +qemuBuildDiskThrottleFiltersCommandLine(virCommand *cmd,
> +virDomainDiskDef *disk)
> +{
> +g_autoptr(qemuBlockThrottleFiltersData) data = NULL;
> +size_t i;
> +
> +data = qemuBuildThrottleFiltersAttachPrepareBlockdev(disk);
> +if (!data)
> +return -1;
> +
> +for (i = 0; i < data->nfilterdata; i++) {
> +if (qemuBuildBlockThrottleFilterCommandline(cmd,
> +data->filterdata[i]) < 0)
> +return -1;
> +}
> +
> +return 0;
> +}
> +
> +
>  static int
>  qemuBuildDiskSourceCommandLine(virCommand *cmd,
> virDomainDiskDef *disk,
> @@ -2102,6 +2195,9 @@ qemuBuildDiskCommandLine(virCommand *cmd,
>  if (qemuBuildDiskSourceCommandLine(cmd,

[libvirt PATCH 05/10] qemu: remove qemuCaps from qemuBuildTLSx509CommandLine

[Ján Tomko]

Also from qemuBuildGraphicsVNCCommandLine

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_command.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4f0a9a517c..ef1ba9269c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1286,8 +1286,7 @@ qemuBuildTLSx509CommandLine(virCommand *cmd,
 bool isListen,
 bool verifypeer,
 const char *certEncSecretAlias,
-const char *alias,
-virQEMUCaps *qemuCaps G_GNUC_UNUSED)
+const char *alias)
 {
 g_autoptr(virJSONValue) props = NULL;
 
@@ -1335,7 +1334,7 @@ qemuBuildChardevCommand(virCommand *cmd,
 dev->data.tcp.listen,
 chrSourcePriv->tlsVerify,
 tlsCertEncSecAlias,
-objalias, qemuCaps) < 0) {
+objalias) < 0) {
 return -1;
 }
 
@@ -8026,7 +8025,6 @@ static int
 qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfig *cfg,
 const virDomainDef *def,
 virCommand *cmd,
-virQEMUCaps *qemuCaps,
 virDomainGraphicsDef *graphics)
 {
 g_autofree char *audioid = qemuGetAudioIDString(def, 
graphics->data.vnc.audioId);
@@ -8107,8 +8105,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfig *cfg,
 true,
 cfg->vncTLSx509verify,
 secretAlias,
-gfxPriv->tlsAlias,
-qemuCaps) < 0)
+gfxPriv->tlsAlias) < 0)
 return -1;
 
 virBufferAsprintf(&opt, ",tls-creds=%s", gfxPriv->tlsAlias);
@@ -8417,7 +8414,7 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfig *cfg,
 break;
 case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
 if (qemuBuildGraphicsVNCCommandLine(cfg, def, cmd,
-qemuCaps, graphics) < 0)
+graphics) < 0)
 return -1;
 
 break;
-- 
2.48.1

[libvirt PATCH 04/10] qemu: remove qemuCaps from qemuBuildObjectSecretCommandLine

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_command.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ae187dac78..4f0a9a517c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1220,8 +1220,7 @@ qemuBuildSecretInfoProps(qemuDomainSecretInfo *secinfo,
  */
 static int
 qemuBuildObjectSecretCommandLine(virCommand *cmd,
- qemuDomainSecretInfo *secinfo,
- virQEMUCaps *qemuCaps G_GNUC_UNUSED)
+ qemuDomainSecretInfo *secinfo)
 {
 g_autoptr(virJSONValue) props = NULL;
 
@@ -1323,8 +1322,7 @@ qemuBuildChardevCommand(virCommand *cmd,
  * functions can just check the config fields */
 if (chrSourcePriv->secinfo) {
 if (qemuBuildObjectSecretCommandLine(cmd,
- chrSourcePriv->secinfo,
- qemuCaps) < 0)
+ chrSourcePriv->secinfo) < 
0)
 return -1;
 
 tlsCertEncSecAlias = chrSourcePriv->secinfo->alias;
@@ -8099,8 +8097,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfig *cfg,
 
 if (gfxPriv->secinfo) {
 if (qemuBuildObjectSecretCommandLine(cmd,
- gfxPriv->secinfo,
- qemuCaps) < 0)
+ gfxPriv->secinfo) < 0)
 return -1;
 secretAlias = gfxPriv->secinfo->alias;
 }
-- 
2.48.1

[libvirt PATCH 02/10] qemu: validate: fs: remove unneeded parameter

[Ján Tomko]

No longer required since we don't require driver->privileged anymore.

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_validate.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 4ef944addb..af334aa96a 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4578,7 +4578,6 @@ qemuValidateDomainDeviceDefGraphics(const 
virDomainGraphicsDef *graphics,
 static int
 qemuValidateDomainDeviceDefFS(virDomainFSDef *fs,
   const virDomainDef *def,
-  virQEMUDriver *driver G_GNUC_UNUSED,
   virQEMUCaps *qemuCaps)
 {
 if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) {
@@ -5544,7 +5543,7 @@ qemuValidateDomainDeviceDef(const virDomainDeviceDef *dev,
 return qemuValidateDomainDeviceDefIOMMU(dev->data.iommu, def, 
qemuCaps);
 
 case VIR_DOMAIN_DEVICE_FS:
-return qemuValidateDomainDeviceDefFS(dev->data.fs, def, driver, 
qemuCaps);
+return qemuValidateDomainDeviceDefFS(dev->data.fs, def, qemuCaps);
 
 case VIR_DOMAIN_DEVICE_NVRAM:
 return qemuValidateDomainDeviceDefNVRAM(dev->data.nvram, def, 
qemuCaps);
-- 
2.48.1

[libvirt PATCH 07/10] qemu: remove qemuCaps from qemuBuildIOThreadCommandLine

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_command.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4b178cac71..456df9ae0e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7296,8 +7296,7 @@ qemuBuildMemCommandLine(virCommand *cmd,
 
 static int
 qemuBuildIOThreadCommandLine(virCommand *cmd,
- const virDomainDef *def,
- virQEMUCaps *qemuCaps G_GNUC_UNUSED)
+ const virDomainDef *def)
 {
 size_t i;
 
@@ -10454,7 +10453,7 @@ qemuBuildCommandLine(virDomainObj *vm,
 if (qemuBuildSmpCommandLine(cmd, def, qemuCaps) < 0)
 return NULL;
 
-if (qemuBuildIOThreadCommandLine(cmd, def, qemuCaps) < 0)
+if (qemuBuildIOThreadCommandLine(cmd, def) < 0)
 return NULL;
 
 if (virDomainNumaGetNodeCount(def->numa) &&
-- 
2.48.1

[libvirt PATCH 03/10] qemu: remove qemuCaps from qemuBuildObjectCommandlineFromJSON

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_command.c | 65 ++---
 1 file changed, 29 insertions(+), 36 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index e2e19743d8..ae187dac78 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -184,8 +184,7 @@ qemuOnOffAuto(virTristateSwitch s)
 
 static int
 qemuBuildObjectCommandlineFromJSON(virCommand *cmd,
-   virJSONValue *props,
-   virQEMUCaps *qemuCaps G_GNUC_UNUSED)
+   virJSONValue *props)
 {
 g_autofree char *arg = NULL;
 
@@ -324,7 +323,7 @@ qemuBuildMasterKeyCommandLine(virCommand *cmd,
  NULL) < 0)
 return -1;
 
-if (qemuBuildObjectCommandlineFromJSON(cmd, props, priv->qemuCaps) < 0)
+if (qemuBuildObjectCommandlineFromJSON(cmd, props) < 0)
 return -1;
 
 return 0;
@@ -1222,14 +1221,14 @@ qemuBuildSecretInfoProps(qemuDomainSecretInfo *secinfo,
 static int
 qemuBuildObjectSecretCommandLine(virCommand *cmd,
  qemuDomainSecretInfo *secinfo,
- virQEMUCaps *qemuCaps)
+ virQEMUCaps *qemuCaps G_GNUC_UNUSED)
 {
 g_autoptr(virJSONValue) props = NULL;
 
 if (qemuBuildSecretInfoProps(secinfo, &props) < 0)
 return -1;
 
-if (qemuBuildObjectCommandlineFromJSON(cmd, props, qemuCaps) < 0)
+if (qemuBuildObjectCommandlineFromJSON(cmd, props) < 0)
 return -1;
 
 return 0;
@@ -1289,7 +1288,7 @@ qemuBuildTLSx509CommandLine(virCommand *cmd,
 bool verifypeer,
 const char *certEncSecretAlias,
 const char *alias,
-virQEMUCaps *qemuCaps)
+virQEMUCaps *qemuCaps G_GNUC_UNUSED)
 {
 g_autoptr(virJSONValue) props = NULL;
 
@@ -1297,7 +1296,7 @@ qemuBuildTLSx509CommandLine(virCommand *cmd,
  certEncSecretAlias, &props) < 0)
 return -1;
 
-if (qemuBuildObjectCommandlineFromJSON(cmd, props, qemuCaps) < 0)
+if (qemuBuildObjectCommandlineFromJSON(cmd, props) < 0)
 return -1;
 
 return 0;
@@ -1969,12 +1968,12 @@ qemuBuildFloppyCommandLineControllerOptions(virCommand 
*cmd,
 static int
 qemuBuildObjectCommandline(virCommand *cmd,
virJSONValue *objProps,
-   virQEMUCaps *qemuCaps)
+   virQEMUCaps *qemuCaps G_GNUC_UNUSED)
 {
 if (!objProps)
 return 0;
 
-if (qemuBuildObjectCommandlineFromJSON(cmd, objProps, qemuCaps) < 0)
+if (qemuBuildObjectCommandlineFromJSON(cmd, objProps) < 0)
 return -1;
 
 return 0;
@@ -3442,11 +3441,10 @@ qemuBuildMemoryDimmBackendStr(virCommand *cmd,
 return -1;
 
 if (tcProps &&
-qemuBuildObjectCommandlineFromJSON(cmd, tcProps,
-   priv->qemuCaps) < 0)
+qemuBuildObjectCommandlineFromJSON(cmd, tcProps) < 0)
 return -1;
 
-if (qemuBuildObjectCommandlineFromJSON(cmd, props, priv->qemuCaps) < 0)
+if (qemuBuildObjectCommandlineFromJSON(cmd, props) < 0)
 return -1;
 
 return 0;
@@ -4247,7 +4245,7 @@ qemuBuildInputCommandLine(virCommand *cmd,
 if (!(props = qemuBuildInputEvdevProps(input)))
 return -1;
 
-if (qemuBuildObjectCommandlineFromJSON(cmd, props, qemuCaps) < 0)
+if (qemuBuildObjectCommandlineFromJSON(cmd, props) < 0)
 return -1;
 } else {
 g_autoptr(virJSONValue) props = NULL;
@@ -5371,7 +5369,7 @@ qemuBuildRNGCommandLine(virCommand *cmd,
 if (qemuBuildRNGBackendProps(rng, &props) < 0)
 return -1;
 
-if (qemuBuildObjectCommandlineFromJSON(cmd, props, qemuCaps) < 0)
+if (qemuBuildObjectCommandlineFromJSON(cmd, props) < 0)
 return -1;
 
 /* add the device */
@@ -7230,11 +7228,10 @@ qemuBuildMemCommandLineMemoryDefaultBackend(virCommand 
*cmd,
 return -1;
 
 if (tcProps &&
-qemuBuildObjectCommandlineFromJSON(cmd, tcProps,
-   priv->qemuCaps) < 0)
+qemuBuildObjectCommandlineFromJSON(cmd, tcProps) < 0)
 return -1;
 
-if (qemuBuildObjectCommandlineFromJSON(cmd, props, priv->qemuCaps) < 0)
+if (qemuBuildObjectCommandlineFromJSON(cmd, props) < 0)
 return -1;
 
 return 0;
@@ -7304,7 +7301,7 @@ qemuBuildMemCommandLine(virCommand *cmd,
 static int
 qemuBuildIOThreadCommandLine(virCommand *cmd,
  const virDomainDef *def,
- virQEMUCaps *qemuCaps)
+ virQEMUCaps *qemuCaps G_GNUC_UNUSED)
 {
 size_t i;
 
@@ -7339,7 +7336,7 @@ qemuBuildIOThreadCommandLine(virCommand *cmd,
 

[libvirt PATCH 10/10] Remove unreachable breaks right after return

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_cgroup.c   | 2 --
 src/qemu/qemu_command.c  | 6 +++---
 src/qemu/qemu_driver.c   | 1 -
 src/qemu/qemu_hotplug.c  | 1 -
 src/qemu/qemu_monitor_json.c | 1 -
 src/security/security_apparmor.c | 1 -
 src/vmx/vmx.c| 2 --
 tools/virsh-domain.c | 1 -
 tools/virsh-host.c   | 2 --
 tools/vsh.c  | 7 ---
 10 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index f3c85d65e8..48af467bf9 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -436,7 +436,6 @@ qemuSetupInputCgroup(virDomainObj *vm,
 case VIR_DOMAIN_INPUT_TYPE_EVDEV:
 return qemuCgroupAllowDevicePath(vm, dev->source.evdev,
  VIR_CGROUP_DEVICE_RW, false);
-break;
 }
 
 return ret;
@@ -457,7 +456,6 @@ qemuTeardownInputCgroup(virDomainObj *vm,
 case VIR_DOMAIN_INPUT_TYPE_EVDEV:
 return qemuCgroupDenyDevicePath(vm, dev->source.evdev,
 VIR_CGROUP_DEVICE_RWM, false);
-break;
 }
 
 return 0;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 92560d381f..ca5a228572 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9706,13 +9706,13 @@ qemuBuildSecCommandLine(virDomainObj *vm, virCommand 
*cmd,
 switch (sec->sectype) {
 case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
 return qemuBuildSEVCommandLine(vm, cmd, &sec->data.sev);
-break;
+
 case VIR_DOMAIN_LAUNCH_SECURITY_SEV_SNP:
 return qemuBuildSEVSNPCommandLine(cmd, &sec->data.sev_snp);
-break;
+
 case VIR_DOMAIN_LAUNCH_SECURITY_PV:
 return qemuBuildPVCommandLine(cmd);
-break;
+
 case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
 case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
 virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index af5445f78d..f0e9681161 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11990,7 +11990,6 @@ qemuDomainGetJobInfoDumpStats(virDomainObj *vm,
_("dump query failed, status=%1$d"),
privJob->stats.dump.status);
 return -1;
-break;
 
 case QEMU_MONITOR_DUMP_STATUS_ACTIVE:
 jobData->status = VIR_DOMAIN_JOB_STATUS_ACTIVE;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 28ca321c5c..b6ef10edf9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -7268,7 +7268,6 @@ qemuDomainChangeMemoryLiveValidateChange(const 
virDomainMemoryDef *oldDef,
_("cannot modify memory of model '%1$s'"),
virDomainMemoryModelTypeToString(oldDef->model));
 return false;
-break;
 }
 
 if (oldDef->model != newDef->model) {
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 3caeb39a1b..9c60807926 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5601,7 +5601,6 @@ int qemuMonitorJSONGetObjectProperty(qemuMonitor *mon,
_("qom-get invalid object property type %1$d"),
prop->type);
 return -1;
-break;
 }
 
 if (ret == -1) {
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 91c51f6395..68ac39611f 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -681,7 +681,6 @@ AppArmorSetInputLabel(virSecurityManager *mgr,
 return -1;
 }
 return reload_profile(mgr, def, input->source.evdev, true);
-break;
 
 case VIR_DOMAIN_INPUT_TYPE_MOUSE:
 case VIR_DOMAIN_INPUT_TYPE_TABLET:
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 8d10b6e9e8..0dd03c1a88 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2250,13 +2250,11 @@ virVMXGenerateDiskTarget(virDomainDiskDef *def,
virDomainDiskBusTypeToString(def->bus),
virDomainDiskDeviceTypeToString(def->device));
 return -1;
-break;
 
 case VIR_DOMAIN_DISK_BUS_NONE:
 case VIR_DOMAIN_DISK_BUS_LAST:
 virReportEnumRangeError(virDomainDiskBus, def->bus);
 return -1;
-break;
 }
 
 def->dst = virIndexToDiskName(idx, prefix);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 93c34c4971..7624cb90fe 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1070,7 +1070,6 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
 vshError(ctl, _("No support for %1$s in command 'attach-interface'"),
  type);
 return false;
-break;
 }
 
 if (target != NULL)
diff --git a/tools/virsh-host.c b/tools/virsh-host.c
index 9e8f542c96..8a92e15b21 100644
--- a/tools/virsh-host.c
+++ b/tools/virsh-host.c
@@ -

[libvirt PATCH 06/10] qemu: remove qemuCaps from qemuBuildObjectCommandline

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_command.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ef1ba9269c..4b178cac71 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1964,8 +1964,7 @@ qemuBuildFloppyCommandLineControllerOptions(virCommand 
*cmd,
 
 static int
 qemuBuildObjectCommandline(virCommand *cmd,
-   virJSONValue *objProps,
-   virQEMUCaps *qemuCaps G_GNUC_UNUSED)
+   virJSONValue *objProps)
 {
 if (!objProps)
 return 0;
@@ -1985,15 +1984,15 @@ 
qemuBuildBlockStorageSourceAttachDataCommandline(virCommand *cmd,
 char *tmp;
 size_t i;
 
-if (qemuBuildObjectCommandline(cmd, data->prmgrProps, qemuCaps) < 0 ||
-qemuBuildObjectCommandline(cmd, data->authsecretProps, qemuCaps) < 0 ||
-qemuBuildObjectCommandline(cmd, data->httpcookiesecretProps, qemuCaps) 
< 0 ||
-qemuBuildObjectCommandline(cmd, data->tlsKeySecretProps, qemuCaps) < 0 
||
-qemuBuildObjectCommandline(cmd, data->tlsProps, qemuCaps) < 0)
+if (qemuBuildObjectCommandline(cmd, data->prmgrProps) < 0 ||
+qemuBuildObjectCommandline(cmd, data->authsecretProps) < 0 ||
+qemuBuildObjectCommandline(cmd, data->httpcookiesecretProps) < 0 ||
+qemuBuildObjectCommandline(cmd, data->tlsKeySecretProps) < 0 ||
+qemuBuildObjectCommandline(cmd, data->tlsProps) < 0)
 return -1;
 
 for (i = 0; i < data->encryptsecretCount; ++i) {
-if (qemuBuildObjectCommandline(cmd, data->encryptsecretProps[i], 
qemuCaps) < 0) {
+if (qemuBuildObjectCommandline(cmd, data->encryptsecretProps[i]) < 0) {
 return -1;
 }
 }
-- 
2.48.1

Re: [PATCH v8 12/18] config: validate: Verify iotune, throttle group and filter

[Peter Krempa]

On Wed, Feb 19, 2025 at 22:27:16 +0530, Harikumar Rajkumar wrote:
> From: Chun Feng Wu 
> 
> Refactor iotune verification, and verify some rules
> 
> * Disk iotune validation can be reused for throttle group validation,
>   refactor it into common method "virDomainDiskIoTuneValidate"
> * Add "virDomainDefValidateThrottleGroups" to validate throttle groups,
>   which in turn calls "virDomainDiskIoTuneValidate"
> * Make sure referenced throttle group exists
> * Use "iotune" and "throttlefilters" exclusively for specific disk
> * Throttle filters cannot be used together with CDROM
> 
> Signed-off-by: Chun Feng Wu 
> 
> * Update of code documentation comments.
> 
> Signed-off-by: Harikumar Rajkumar 
> ---
>  src/conf/domain_conf.c |   8 +++
>  src/conf/domain_validate.c | 118 ++---
>  src/qemu/qemu_driver.c |  13 
>  src/qemu/qemu_hotplug.c|   8 +++
>  4 files changed, 113 insertions(+), 34 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 303aa7d1ae..0cc318d1f9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8008,6 +8008,14 @@ virDomainDiskDefThrottleFiltersParse(virDomainDiskDef 
> *def,
>  if ((n = virXPathNodeSet("./throttlefilters/throttlefilter", ctxt, 
> &nodes)) < 0)
>  return -1;
>  
> +if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
> +if (n > 0) {
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +   _("cdrom device with throttle filters isn't 
> supported"));
> +return -1;
> +}
> +}

Normally adding any form of validation into the parser is not allowed
because it makes VMs vanish. as this is happening in hthe same series
it's technically allowable, but ...


> +
>  if (n)
>  def->throttlefilters = g_new0(virDomainThrottleFilterDef *, n);
>  
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 563558d920..d51682d155 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -685,11 +685,55 @@ virDomainDiskDefValidateStartupPolicy(const 
> virDomainDiskDef *disk)
>  }
>  
>  
> +static int
> +virDomainDiskIoTuneValidate(const virDomainBlockIoTuneInfo blkdeviotune)
> +{
> +if ((blkdeviotune.total_bytes_sec &&
> + blkdeviotune.read_bytes_sec) ||
> +(blkdeviotune.total_bytes_sec &&
> + blkdeviotune.write_bytes_sec)) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("total and read/write bytes_sec cannot be set at 
> the same time"));
> +return -1;
> +}
> +
> +if ((blkdeviotune.total_iops_sec &&
> + blkdeviotune.read_iops_sec) ||
> +(blkdeviotune.total_iops_sec &&
> + blkdeviotune.write_iops_sec)) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("total and read/write iops_sec cannot be set at the 
> same time"));
> +return -1;
> +}
> +
> +if ((blkdeviotune.total_bytes_sec_max &&
> + blkdeviotune.read_bytes_sec_max) ||
> +(blkdeviotune.total_bytes_sec_max &&
> + blkdeviotune.write_bytes_sec_max)) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("total and read/write bytes_sec_max cannot be set 
> at the same time"));
> +return -1;
> +}
> +
> +if ((blkdeviotune.total_iops_sec_max &&
> + blkdeviotune.read_iops_sec_max) ||
> +(blkdeviotune.total_iops_sec_max &&
> + blkdeviotune.write_iops_sec_max)) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("total and read/write iops_sec_max cannot be set at 
> the same time"));
> +return -1;
> +}
> +
> +return 0;
> +}
> +
> +
>  static int
>  virDomainDiskDefValidate(const virDomainDef *def,
>   const virDomainDiskDef *disk)
>  {
>  virStorageSource *next;
> +size_t i;
>  
>  /* disk target is used widely in other code so it must be validated 
> first */
>  if (!disk->dst) {
> @@ -739,41 +783,8 @@ virDomainDiskDefValidate(const virDomainDef *def,
>  }
>  
>  /* Validate IotuneParse */
> -if ((disk->blkdeviotune.total_bytes_sec &&
> - disk->blkdeviotune.read_bytes_sec) ||
> -(disk->blkdeviotune.total_bytes_sec &&
> - disk->blkdeviotune.write_bytes_sec)) {
> -virReportError(VIR_ERR_XML_ERROR, "%s",
> -   _("total and read/write bytes_sec cannot be set at 
> the same time"));
> +if (virDomainDiskIoTuneValidate(disk->blkdeviotune) < 0)
>  return -1;
> -}
> -
> -if ((disk->blkdeviotune.total_iops_sec &&
> - disk->blkdeviotune.read_iops_sec) ||
> -(disk->blkdeviotune.total_iops_sec &&
> - disk->blkdeviotune.write_iops_sec)) {
> -virReportError(VIR_ERR_XML_ERROR, "%s",
> -   _("total and read/write iops_sec cannot be set at the 
> same time"));
> -  

[libvirt PATCH 09/10] qemu: remove unused vm from qemuBuildPVCommandLine

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_command.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0b618e5c88..92560d381f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9681,7 +9681,7 @@ qemuBuildSEVSNPCommandLine(virCommand *cmd,
 
 
 static int
-qemuBuildPVCommandLine(virDomainObj *vm G_GNUC_UNUSED, virCommand *cmd)
+qemuBuildPVCommandLine(virCommand *cmd)
 {
 g_autoptr(virJSONValue) props = NULL;
 
@@ -9711,7 +9711,7 @@ qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd,
 return qemuBuildSEVSNPCommandLine(cmd, &sec->data.sev_snp);
 break;
 case VIR_DOMAIN_LAUNCH_SECURITY_PV:
-return qemuBuildPVCommandLine(vm, cmd);
+return qemuBuildPVCommandLine(cmd);
 break;
 case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
 case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
-- 
2.48.1

Re: [PATCH v2 20/22] admin: add 'daemon-shutdown' command

[Peter Krempa]

On Wed, Mar 12, 2025 at 17:18:00 +, Daniel P. Berrangé wrote:
> The daemons are wired up to shutdown in responsible to UNIX process
> signals, as well as in response to login1 dbus signals, or loss of
> desktop session. The latter two options can optionally preserve state
> (ie running VMs).
> 
> In non-systemd environments, as well as for testing, it would be useful
> to have a way to trigger shutdown with state preservation more directly.
> 
> Thus a new admin protocol API is introduced
> 
>   virAdmConnectDaemonShutdown
> 
> which will trigger a daemon shutdown, and preserve running VMs if the
> VIR_DAEMON_SHUTDOWN_PRESERVE flag is set.
> 
> It has a corresponding 'virt-admin daemon-shutdown [--preserve]' command
> binding.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  docs/manpages/virt-admin.rst| 13 +
>  include/libvirt/libvirt-admin.h | 13 +
>  src/admin/admin_protocol.x  | 11 +++-
>  src/admin/admin_server_dispatch.c   | 13 +
>  src/admin/libvirt-admin.c   | 33 +++
>  src/admin/libvirt_admin_public.syms |  5 
>  tools/virt-admin.c  | 41 +
>  7 files changed, 128 insertions(+), 1 deletion(-)

Reviewed-by: Peter Krempa 

Re: [PATCH v2 11/22] conf: implement support for autostart once feature

[Peter Krempa]

On Wed, Mar 12, 2025 at 17:17:51 +, Daniel P. Berrangé wrote:
> This is maintained in the same way as the autostart flag, using a
> symlink. The difference is that instead of '.xml', the symlink
> suffix is '.xml.once'. The link is also deleted immediately after
> it has been read.

The last sentence is no longer accurate after the changes.

> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/conf/domain_conf.c |  7 ++-
>  src/conf/domain_conf.h |  2 ++
>  src/conf/virdomainobjlist.c|  8 
>  src/hypervisor/domain_driver.c | 14 +++---
>  4 files changed, 27 insertions(+), 4 deletions(-)

Reviewed-by: Peter Krempa 

[libvirt PATCH 00/10] Unused cleanups

[Ján Tomko]

Patches 4-9/10 all depend on patch 3/10
10/10 can be pushed independently

Ján Tomko (10):
  qemu: seccomp sandbox: remove incorect G_GNUC_UNUSED marker
  qemu: validate: fs: remove unneeded parameter
  qemu: remove qemuCaps from qemuBuildObjectCommandlineFromJSON
  qemu: remove qemuCaps from qemuBuildObjectSecretCommandLine
  qemu: remove qemuCaps from qemuBuildTLSx509CommandLine
  qemu: remove qemuCaps from qemuBuildObjectCommandline
  qemu: remove qemuCaps from qemuBuildIOThreadCommandLine
  qemu: remove unused vm from qemuBuildSEVSNPCommandLine
  qemu: remove unused vm from qemuBuildPVCommandLine
  Remove unreachable breaks right after return

 src/qemu/qemu_cgroup.c   |   2 -
 src/qemu/qemu_command.c  | 110 +--
 src/qemu/qemu_driver.c   |   1 -
 src/qemu/qemu_hotplug.c  |   1 -
 src/qemu/qemu_monitor_json.c |   1 -
 src/qemu/qemu_validate.c |   3 +-
 src/security/security_apparmor.c |   1 -
 src/vmx/vmx.c|   2 -
 tools/virsh-domain.c |   1 -
 tools/virsh-host.c   |   2 -
 tools/vsh.c  |   7 --
 11 files changed, 48 insertions(+), 83 deletions(-)

-- 
2.48.1

[libvirt PATCH 01/10] qemu: seccomp sandbox: remove incorect G_GNUC_UNUSED marker

[Ján Tomko]

qemuCaps is obviously used.

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 51e428e017..e2e19743d8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10075,7 +10075,7 @@ qemuBuildCommandLineValidate(virQEMUDriver *driver,
 static int
 qemuBuildSeccompSandboxCommandLine(virCommand *cmd,
virQEMUDriverConfig *cfg,
-   virQEMUCaps *qemuCaps G_GNUC_UNUSED)
+   virQEMUCaps *qemuCaps)
 {
 if (cfg->seccompSandbox == 0) {
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX))
-- 
2.48.1

Re: [PATCH v8 09/18] qemu: Implement qemu driver for throttle API

[Peter Krempa]

On Wed, Feb 19, 2025 at 22:27:13 +0530, Harikumar Rajkumar wrote:
> From: Chun Feng Wu 
> 
> ThrottleGroup lifecycle implementation, note, in QOM, throttlegroup name is 
> prefixed with
> "throttle-" to clearly separate throttle group objects into their own 
> namespace.
> * "qemuDomainSetThrottleGroup", this method is to add("object-add") or 
> update("qom-set")
>   throttlegroup in QOM and update corresponding objects in DOM
> * "qemuDomainGetThrottleGroup", this method queries throttlegroup info by 
> groupname
> * "qemuDomainDelThrottleGroup", this method checks if group is referenced by 
> any throttle
>   in disks and delete it if it's not used anymore
> * Check flag "QEMU_CAPS_OBJECT_JSON" during qemuDomainSetThrottleGroup when 
> vm is active,
>   throttle group feature requries such flag
> * "objectAddNoWrap"("props") check is done by reusing qemuMonitorAddObject in
>   qemuDomainSetThrottleGroup
> 
> Signed-off-by: Chun Feng Wu 
> 
> * Apply suggested coding style changes.
> * cleanup qemu Get ThrottleGroup.
> * Update the version to 11.1.0.
> 
> Signed-off-by: Harikumar Rajkumar 
> ---
>  src/qemu/qemu_driver.c | 243 +
>  1 file changed, 243 insertions(+)

I've removed the check mentioned in my other reply and fixed the version
info.

Reviewed-by: Peter Krempa 

Re: [PATCH v8 15/18] test_driver: Test throttle group lifecycle APIs

[Peter Krempa]

On Wed, Feb 19, 2025 at 22:27:19 +0530, Harikumar Rajkumar wrote:
> From: Chun Feng Wu 
> 
> Test throttle group APIs
> 
> * Extract common methods for both "testDomainSetThrottleGroup" and 
> "testDomainSetBlockIoTune":
>  testDomainValidateBlockIoTune, testDomainSetBlockIoTuneFields,
> testDomainCheckBlockIoTuneMutualExclusion, testDomainCheckBlockIoTuneMax
> * Test "Set": testDomainSetThrottleGroup
> * Test "Get": testDomainGetThrottleGroup
> * Test "Del": testDomainDelThrottleGroup
> 
> Signed-off-by: Chun Feng Wu 
> 
> * Cleanup Test "Get": testDomainGetThrottleGroup
> * Update the version to 11.1.0.
> 
> Signed-off-by: Harikumar Rajkumar 
> ---
>  src/test/test_driver.c | 367 +++--
>  1 file changed, 245 insertions(+), 122 deletions(-)

For the sake of not prolonging the review any more I'm dropping this
patch for now. Please submit it once we're done with the qemu driver
impl.

Re: [libvirt PATCH 04/10] qemu: remove qemuCaps from qemuBuildObjectSecretCommandLine

[Peter Krempa]

On Thu, Mar 13, 2025 at 16:30:18 +0100, Ján Tomko wrote:
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_command.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ae187dac78..4f0a9a517c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1220,8 +1220,7 @@ qemuBuildSecretInfoProps(qemuDomainSecretInfo *secinfo,
>   */

'qemuCaps' is documented in the funcion comment above this block.

Re: [libvirt PATCH 00/10] Unused cleanups

[Martin Kletzander]

On Thu, Mar 13, 2025 at 04:30:14PM +0100, Ján Tomko wrote:

Patches 4-9/10 all depend on patch 3/10
10/10 can be pushed independently

Ján Tomko (10):
 qemu: seccomp sandbox: remove incorect G_GNUC_UNUSED marker
 qemu: validate: fs: remove unneeded parameter
 qemu: remove qemuCaps from qemuBuildObjectCommandlineFromJSON
 qemu: remove qemuCaps from qemuBuildObjectSecretCommandLine
 qemu: remove qemuCaps from qemuBuildTLSx509CommandLine
 qemu: remove qemuCaps from qemuBuildObjectCommandline
 qemu: remove qemuCaps from qemuBuildIOThreadCommandLine
 qemu: remove unused vm from qemuBuildSEVSNPCommandLine
 qemu: remove unused vm from qemuBuildPVCommandLine
 Remove unreachable breaks right after return



I wonder whether there was some tool that got confused by return without
break in a switch statement, but in any case that's a problem of that
tool, if such thing exists.

For the series:

Reviewed-by: Martin Kletzander 


src/qemu/qemu_cgroup.c   |   2 -
src/qemu/qemu_command.c  | 110 +--
src/qemu/qemu_driver.c   |   1 -
src/qemu/qemu_hotplug.c  |   1 -
src/qemu/qemu_monitor_json.c |   1 -
src/qemu/qemu_validate.c |   3 +-
src/security/security_apparmor.c |   1 -
src/vmx/vmx.c|   2 -
tools/virsh-domain.c |   1 -
tools/virsh-host.c   |   2 -
tools/vsh.c  |   7 --
11 files changed, 48 insertions(+), 83 deletions(-)

--
2.48.1



signature.asc
Description: PGP signature

[libvirt PATCH 08/10] qemu: remove unused vm from qemuBuildSEVSNPCommandLine

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_command.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 456df9ae0e..0b618e5c88 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9643,8 +9643,7 @@ qemuBuildSEVCommandLine(virDomainObj *vm, virCommand *cmd,
 
 
 static int
-qemuBuildSEVSNPCommandLine(virDomainObj *vm G_GNUC_UNUSED,
-   virCommand *cmd,
+qemuBuildSEVSNPCommandLine(virCommand *cmd,
virDomainSEVSNPDef *def)
 {
 g_autoptr(virJSONValue) props = NULL;
@@ -9709,7 +9708,7 @@ qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd,
 return qemuBuildSEVCommandLine(vm, cmd, &sec->data.sev);
 break;
 case VIR_DOMAIN_LAUNCH_SECURITY_SEV_SNP:
-return qemuBuildSEVSNPCommandLine(vm, cmd, &sec->data.sev_snp);
+return qemuBuildSEVSNPCommandLine(cmd, &sec->data.sev_snp);
 break;
 case VIR_DOMAIN_LAUNCH_SECURITY_PV:
 return qemuBuildPVCommandLine(vm, cmd);
-- 
2.48.1

[PATCH 2/2] src: add new target for regenerating protocol structs files

[Daniel P . Berrangé]

Introduce a new ninja target

   ninja -C build regen-{PROTO}

eg

   ninja -C build regen-admin_protocol

that will re-create the reference output file based on what the
current pdwtags command emits. A small change is made to squash
whitespace on enum declarations so that introducing a new longer
enum name dosn't trigger re-indent of all existing enum names.

Signed-off-by: Daniel P. Berrangé 
---
 scripts/check-remote-protocol.py | 27 ++-
 src/meson.build  | 14 ++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/scripts/check-remote-protocol.py b/scripts/check-remote-protocol.py
index 40c4b481be..9de0a4a93d 100644
--- a/scripts/check-remote-protocol.py
+++ b/scripts/check-remote-protocol.py
@@ -36,6 +36,9 @@ targetname = sys.argv[2]
 libpath = sys.argv[3]
 pdwtags = sys.argv[4]
 expected = sys.argv[5]
+regen = False
+if len(sys.argv) == 7 and sys.argv[6] == "--regenerate":
+regen = True
 
 builddir = os.path.dirname(libpath)
 libname = os.path.basename(libpath)
@@ -128,7 +131,29 @@ actualstr = "\n".join(actual) + "\n"
 # know our RPC structs are suitably aligned to not need
 # packing, so we can just trim the attribute.
 actualstr = re.sub(r'''} __attribute__\(\(__packed__\)\);''', "};", actualstr)
+actualstr = re.sub(r'''([A-Z0-9]+)\s+=\s+(\d)''', r'''\1 = \2''', actualstr)
 
 diff.communicate(input=actualstr.encode("utf-8"))
 
-sys.exit(diff.returncode)
+if diff.returncode != 0:
+if regen:
+with open(expected, "w") as fh:
+print(actualstr, file=fh, end='')
+
+print("")
+print("WARNING: reference output was re-generated to apply")
+print("WARNING: the above diff. Validate the changes are")
+print("WARNING: expected and correct before committing")
+print("")
+sys.exit(0)
+else:
+print("")
+print("WARNING: validate the above protocol changes are")
+print("WARNING: expected and correct. To re-generate the")
+print("WARNING: reference output invoke")
+print("")
+print("  $ ninja regen-%s" % name)
+
+sys.exit(diff.returncode)
+else:
+sys.exit(0)
diff --git a/src/meson.build b/src/meson.build
index 9413192a55..9a818dab50 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -1088,6 +1088,20 @@ if tests_enabled[0]
 depends: [ lib ],
 suite: 'script'
   )
+
+  run_target(
+  'regen-@0@'.format(proto['name']),
+  command: [python3_prog,
+check_remote_protocol_prog.full_path(),
+proto['name'],
+lib.name(),
+lib.full_path(),
+pdwtags_prog.full_path(),
+files('@0@-structs'.format(proto['name'])),
+'--regenerate',
+   ],
+  env: runutf8,
+  depends: [ lib ])
 endforeach
   endif
 endif
-- 
2.48.1

Re: [libvirt PATCH 00/10] Unused cleanups

[Peter Krempa]

On Thu, Mar 13, 2025 at 16:30:14 +0100, Ján Tomko wrote:
> Patches 4-9/10 all depend on patch 3/10
> 10/10 can be pushed independently
> 
> Ján Tomko (10):
>   qemu: seccomp sandbox: remove incorect G_GNUC_UNUSED marker
>   qemu: validate: fs: remove unneeded parameter
>   qemu: remove qemuCaps from qemuBuildObjectCommandlineFromJSON
>   qemu: remove qemuCaps from qemuBuildObjectSecretCommandLine
>   qemu: remove qemuCaps from qemuBuildTLSx509CommandLine
>   qemu: remove qemuCaps from qemuBuildObjectCommandline
>   qemu: remove qemuCaps from qemuBuildIOThreadCommandLine
>   qemu: remove unused vm from qemuBuildSEVSNPCommandLine
>   qemu: remove unused vm from qemuBuildPVCommandLine
>   Remove unreachable breaks right after return

With the two docs things fixed:

Reviewed-by: Peter Krempa 

[PATCH 0/2] src: make it easier to updat the protocol structs files

[Daniel P . Berrangé]

This introduces a new target:

   ninja -C build  regen-admin_protocol

and equiv for other protocol files

Daniel P. Berrangé (2):
  src: normalize whitespace in protocol structs files
  src: add new target for regenerating protocol structs files

 scripts/check-remote-protocol.py | 27 ++-
 src/admin_protocol-structs   |  2 +-
 src/meson.build  | 14 ++
 src/qemu_protocol-structs|  2 +-
 src/remote_protocol-structs  |  6 +++---
 5 files changed, 45 insertions(+), 6 deletions(-)

-- 
2.48.1

[PATCH 1/2] src: normalize whitespace in protocol structs files

[Daniel P . Berrangé]

This makes the output match what current pdwtags will emit,
modulo some whitespace changes made by the check script
before comparison.

Signed-off-by: Daniel P. Berrangé 
---
 src/admin_protocol-structs  | 2 +-
 src/qemu_protocol-structs   | 2 +-
 src/remote_protocol-structs | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs
index 8caac59824..f6988cf76d 100644
--- a/src/admin_protocol-structs
+++ b/src/admin_protocol-structs
@@ -167,5 +167,5 @@ enum admin_procedure {
 ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16,
 ADMIN_PROC_CONNECT_SET_LOGGING_FILTERS = 17,
 ADMIN_PROC_SERVER_UPDATE_TLS_FILES = 18,
-ADMIN_PROC_CONNECT_SET_DAEMON_TIMEOUT   = 19,
+ADMIN_PROC_CONNECT_SET_DAEMON_TIMEOUT = 19,
 };
diff --git a/src/qemu_protocol-structs b/src/qemu_protocol-structs
index ea0854385f..7c78e5ab5a 100644
--- a/src/qemu_protocol-structs
+++ b/src/qemu_protocol-structs
@@ -62,5 +62,5 @@ enum qemu_procedure {
 QEMU_PROC_CONNECT_DOMAIN_MONITOR_EVENT_REGISTER = 4,
 QEMU_PROC_CONNECT_DOMAIN_MONITOR_EVENT_DEREGISTER = 5,
 QEMU_PROC_DOMAIN_MONITOR_EVENT = 6,
-QEMU_PROC_DOMAIN_MONITOR_COMMAND_WITH_FILES   = 7,
+QEMU_PROC_DOMAIN_MONITOR_COMMAND_WITH_FILES = 7,
 };
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 4d3dc2d249..ed7e2fbcb0 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -569,7 +569,7 @@ struct remote_domain_save_params_args {
 u_int  params_len;
 remote_typed_param * params_val;
 } params;
-u_int  flags;
+u_int  flags;
 };
 struct remote_domain_restore_args {
 remote_nonnull_string  from;
@@ -1115,7 +1115,7 @@ struct remote_network_create_xml_ret {
 };
 struct remote_network_create_xml_flags_args {
 remote_nonnull_string  xml;
-u_intflags;
+u_int  flags;
 };
 struct remote_network_create_xml_flags_ret {
 remote_nonnull_network net;
@@ -1128,7 +1128,7 @@ struct remote_network_define_xml_ret {
 };
 struct remote_network_define_xml_flags_args {
 remote_nonnull_string  xml;
-u_intflags;
+u_int  flags;
 };
 struct remote_network_define_xml_flags_ret {
 remote_nonnull_network net;
-- 
2.48.1

Re: [libvirt PATCH 00/10] Unused cleanups

[Daniel P . Berrangé]

On Thu, Mar 13, 2025 at 04:52:13PM +0100, Martin Kletzander wrote:
> On Thu, Mar 13, 2025 at 04:30:14PM +0100, Ján Tomko wrote:
> > Patches 4-9/10 all depend on patch 3/10
> > 10/10 can be pushed independently
> > 
> > Ján Tomko (10):
> >  qemu: seccomp sandbox: remove incorect G_GNUC_UNUSED marker
> >  qemu: validate: fs: remove unneeded parameter
> >  qemu: remove qemuCaps from qemuBuildObjectCommandlineFromJSON
> >  qemu: remove qemuCaps from qemuBuildObjectSecretCommandLine
> >  qemu: remove qemuCaps from qemuBuildTLSx509CommandLine
> >  qemu: remove qemuCaps from qemuBuildObjectCommandline
> >  qemu: remove qemuCaps from qemuBuildIOThreadCommandLine
> >  qemu: remove unused vm from qemuBuildSEVSNPCommandLine
> >  qemu: remove unused vm from qemuBuildPVCommandLine
> >  Remove unreachable breaks right after return
> > 
> 
> I wonder whether there was some tool that got confused by return without
> break in a switch statement, but in any case that's a problem of that
> tool, if such thing exists.

Probably a case where we did

  ret = 

and then did a simple search & replace for 's/ret = /return/', when we
eliminated the need for a centralize return path thanks to g_auto*




With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 5/6] ch: Unref @cfg in virCHProcessStop()

[Peter Krempa]

On Thu, Mar 13, 2025 at 14:44:37 +0100, Michal Privoznik wrote:
> At the beginning of virCHProcessStop() the ref to driver config
> is obtained (via virCHDriverGetConfig()), but corresponding unref
> call is lacking. Use g_autoptr() to make sure the config is
> unrefed always.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/ch/ch_process.c | 2 +-

Reviewed-by: Peter Krempa 

Re: [PATCH 3/6] ch: Don't leak virCHDomainObjPrivate struct members

[Peter Krempa]

On Thu, Mar 13, 2025 at 14:44:35 +0100, Michal Privoznik wrote:
> There are some members of the virCHDomainObjPrivate struct that
> are allocated at various stages of domain lifecycle but then are
> never freed:
> 
> 1) cgroup - allocated in virDomainCgroupSetupCgroup()
> 2) autoCpuset - this one is actually never allocated (and thus is
> always NULL, but soon it may be used. Just free
> it for now, which is a NOP anyways.
> 3) autoNodeset - same story as 2).

So wait; was it copied from qemu without being used?

Will it actually be used soon? If no; I'd prefer to just delete the
members instead.

> 
> There are two more members, which shouldn't be freed:
> 
> 1) driver - this is just a raw pointer to the CH driver (see
>virCHDomainObjPrivateAlloc()).
> 
> 2) monitor - this member is cleared in virCHProcessStop(), way
>  before control even gets to
>  virCHDomainObjPrivateFree().
> 
> 452 (400 direct, 52 indirect) bytes in 1 blocks are definitely lost in loss 
> record 1,944 of 1,998
>at 0x484CEF3: calloc (vg_replace_malloc.c:1675)
>by 0x4F0E7A9: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.8000.5)
>by 0x49479CE: virCgroupNewFromParent (vircgroup.c:893)
>by 0x49481BA: virCgroupNewDomainPartition (vircgroup.c:1068)
>by 0x494915E: virCgroupNewMachineManual (vircgroup.c:1378)
>by 0x49492FE: virCgroupNewMachine (vircgroup.c:1432)
>by 0x4B5E3DE: virDomainCgroupInitCgroup (domain_cgroup.c:377)
>by 0x4B5E9CD: virDomainCgroupSetupCgroup (domain_cgroup.c:524)
>by 0xB3AC693: virCHProcessStart (ch_process.c:951)
>by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246)
>by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188)
>by 0x168F91: remoteDispatchDomainCreateXML 
> (remote_daemon_dispatch_stubs.h:5186)
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/ch/ch_domain.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
> index 4f5966adce..a08b18c5b9 100644
> --- a/src/ch/ch_domain.c
> +++ b/src/ch/ch_domain.c
> @@ -65,6 +65,9 @@ virCHDomainObjPrivateFree(void *data)
>  
>  virChrdevFree(priv->chrdevs);
>  g_free(priv->machineName);
> +virBitmapFree(priv->autoCpuset);
> +virBitmapFree(priv->autoNodeset);
> +virCgroupFree(priv->cgroup);
>  g_free(priv);
>  }

At least for the cgroup bit:

Reviewed-by: Peter Krempa 

Re: [PATCH 04/17] qemu: monitor: Drop support for extra wrapper for 'object_add'

[Ján Tomko]

On a Wednesday in 2025, Peter Krempa wrote:

The QAPIfication of objects removed the extra warapper object which we


wrapper


were adding in the monitor code to simplify the other callers.

Now that we support only qemus which don't require this we can drop the
support code.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_monitor.c  | 27 +--
src/qemu/qemu_monitor_priv.h |  2 --
2 files changed, 1 insertion(+), 28 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 09/17] qemu: Always assume supprot for QEMU_CAPS_BLOCKDEV_REOPEN

[Ján Tomko]

On a Wednesday in 2025, Peter Krempa wrote:

'blockdev-reopen' is supported since qemu-6.1, thus we can now remove
the interlocks.

Document the change to 'mirror' as this patch removes the last clue why
we overwrite the mirror's readonly state to false unconditionally.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_blockjob.c   | 11 +--
src/qemu/qemu_checkpoint.c |  6 ++
src/qemu/qemu_driver.c | 11 +++
3 files changed, 6 insertions(+), 22 deletions(-)



*support
in the commit summary

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 4/6] ch: Free @iothreads array in virCHProcessSetupIOThreads()

[Peter Krempa]

On Thu, Mar 13, 2025 at 14:44:36 +0100, Michal Privoznik wrote:
> When the CH driver starts a domain virCHProcessSetupIOThreads()
> is called eventually which in turn calls
> virCHMonitorGetIOThreads(). The latter returns an array of
> iothreads which is never freed leading to a memleak:
> 
> 130 (104 direct, 26 indirect) bytes in 1 blocks are definitely lost in loss 
> record 1,804 of 1,998
>at 0x484CEF3: calloc (vg_replace_malloc.c:1675)
>by 0x4F0E7A9: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.8000.5)
>by 0xB3A9359: virCHMonitorGetIOThreads (ch_monitor.c:1183)
>by 0xB3AA5BB: virCHProcessSetupIOThreads (ch_process.c:348)
>by 0xB3AAC59: virCHProcessSetup (ch_process.c:480)
>by 0xB3AC75A: virCHProcessStart (ch_process.c:973)
>by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246)
>by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188)
>by 0x168F91: remoteDispatchDomainCreateXML 
> (remote_daemon_dispatch_stubs.h:5186)
>by 0x168F18: remoteDispatchDomainCreateXMLHelper 
> (remote_daemon_dispatch_stubs.h:5167)
>by 0x4B20066: virNetServerProgramDispatchCall (virnetserverprogram.c:423)
>by 0x4B1FB99: virNetServerProgramDispatch (virnetserverprogram.c:299)
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/ch/ch_process.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

Reviewed-by: Peter Krempa 

Re: [PATCH 2/6] ch: Free retval of curl_slist_append()

[Peter Krempa]

On Thu, Mar 13, 2025 at 14:44:34 +0100, Michal Privoznik wrote:
> There are two places where curl_slist_append() is called but
> corresponding call to curl_slist_free_all() is missing:
> virCHMonitorPutNoContent() and virCHMonitorGet() which leads to
> memleaks:
> 
> 41 (16 direct, 25 indirect) bytes in 1 blocks are definitely lost in loss 
> record 992 of 1,998
>at 0x4845888: malloc (vg_replace_malloc.c:446)
>by 0x5B2F8FE: curl_slist_append (in /usr/lib64/libcurl.so.4.8.0)
>by 0xB3A7B41: virCHMonitorPutNoContent (ch_monitor.c:824)
>by 0xB3A89FF: virCHMonitorBootVM (ch_monitor.c:1030)
>by 0xB3AC6F1: virCHProcessStart (ch_process.c:967)
>by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246)
>by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188)
>by 0x168F91: remoteDispatchDomainCreateXML 
> (remote_daemon_dispatch_stubs.h:5186)
>by 0x168F18: remoteDispatchDomainCreateXMLHelper 
> (remote_daemon_dispatch_stubs.h:5167)
>by 0x4B20066: virNetServerProgramDispatchCall (virnetserverprogram.c:423)
>by 0x4B1FB99: virNetServerProgramDispatch (virnetserverprogram.c:299)
>by 0x4B28B5E: virNetServerProcessMsg (virnetserver.c:135)
> 
> 88 (16 direct, 72 indirect) bytes in 1 blocks are definitely lost in loss 
> record 1,501 of 1,998
>at 0x4845888: malloc (vg_replace_malloc.c:446)
>by 0x5B2F8FE: curl_slist_append (in /usr/lib64/libcurl.so.4.8.0)
>by 0xB3A7E41: virCHMonitorGet (ch_monitor.c:864)
>by 0xB3A92E2: virCHMonitorGetInfo (ch_monitor.c:1157)
>by 0xB3A9CEA: virCHProcessUpdateInfo (ch_process.c:142)
>by 0xB3AAD36: virCHProcessSetup (ch_process.c:492)
>by 0xB3AC75A: virCHProcessStart (ch_process.c:973)
>by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246)
>by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188)
>by 0x168F91: remoteDispatchDomainCreateXML 
> (remote_daemon_dispatch_stubs.h:5186)
>by 0x168F18: remoteDispatchDomainCreateXMLHelper 
> (remote_daemon_dispatch_stubs.h:5167)
>by 0x4B20066: virNetServerProgramDispatchCall (virnetserverprogram.c:423)
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/ch/ch_monitor.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Peter Krempa 

Re: [PATCH 00/17] qemu: Clean up various unneeded capabilities

[Ján Tomko]

On a Wednesday in 2025, Peter Krempa wrote:

The recent bump of minimum qemu version resulted into some capabilities
always being present. I've noticed one while doing a review and from
there I removed a few others, mostly storage related ones as well.

Peter Krempa (17):
 qemuBuildCompatDeprecatedCommandLine: Assume that
   QEMU_CAPS_COMPAT_DEPRECATED is supported
 qemu: capabilities: Retire QEMU_CAPS_COMPAT_DEPRECATED
 qemuBuildObjectCommandlineFromJSON: Assume all qemus support
   QEMU_CAPS_OBJECT_JSON
 qemu: monitor: Drop support for extra wrapper for 'object_add'
 util: Drop 'virQEMUBuildCommandLineJSONArrayBitmap'
 qemu: capabilities: Retire QEMU_CAPS_OBJECT_JSON
 qemu: monitor: Always assume support for
   QEMU_CAPS_QMP_QUERY_NAMED_BLOCK_NODES_FLAT
 qemu: capabilities: Retire QEMU_CAPS_QMP_QUERY_NAMED_BLOCK_NODES_FLAT
 qemu: Always assume supprot for QEMU_CAPS_BLOCKDEV_REOPEN
 qemu: capabilities: Retire QEMU_CAPS_BLOCKDEV_REOPEN
 qemu: Always assume support for
   QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY
 qemu: capabilities: Retire
   QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY
 qemu: Always assume support for QEMU_CAPS_INCREMENTAL_BACKUP
 qemu: capabilites: Retire QEMU_CAPS_INCREMENTAL_BACKUP
 qemu: domain: Remove qemuDomainSupportsCheckpointsBlockjobs
 qemu: migration: Always assume support for
   QEMU_CAPS_MIGRATION_PARAM_BLOCK_BITMAP_MAPPING
 qemu: capabilites: Retire
   QEMU_CAPS_MIGRATION_PARAM_BLOCK_BITMAP_MAPPING



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 6/6] ch: Rework virCHProcessConnectMonitor()

[Peter Krempa]

On Thu, Mar 13, 2025 at 14:44:38 +0100, Michal Privoznik wrote:
> Firstly, let's switch from explicit virCHDriverGetConfig() +
> virObjectUnref() combo to g_autoptr(virCHDriverConfig). This
> leaves us with the @monitor variable which is initialized to NULL
> only to be then set to the retval of virCHMonitorNew() and
> returned instantly. Well, the variable is now useless and can be
> dropped.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/ch/ch_process.c | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)

Reviewed-by: Peter Krempa 

Re: [PATCH v2 00/21] Add qemu RDP server support

[Martin Kletzander]

On Thu, Mar 13, 2025 at 02:08:47PM +0400, Marc-André Lureau wrote:

Hi

On Thu, Mar 13, 2025 at 1:07 PM Martin Kletzander  wrote:


On Thu, Mar 13, 2025 at 11:59:41AM +0400, Marc-André Lureau wrote:
>Hi
>
>On Tue, Mar 11, 2025 at 5:28 PM Martin Kletzander  wrote:
>> So I tried testing it and I found a couple of issues.  Those might be on
>> my side, maybe caused by misunderstanding of it all.  First of all
>>  fails with these patches for me even without
>
>How does it fail? (it works on my end)
>

$ virsh dumpxml fedora39 --xpath //graphics


   
   


$ virsh version
Compiled against library: libvirt 11.1.0
Using library: libvirt 11.1.0
Using API: QEMU 11.1.0
Running hypervisor: QEMU 9.2.2

$ virsh start fedora39
Domain 'fedora39' started

$ virsh destroy fedora39
Domain 'fedora39' destroyed

$ echo 'Start daemon with patches'
Start daemon with patches
$ virsh version
Compiled against library: libvirt 11.1.0
Using library: libvirt 11.1.0
Using API: QEMU 11.1.0
Running hypervisor: QEMU 9.2.2

$ # Same version, but this time with the patch series applied
$ virsh dumpxml fedora39 --xpath //graphics


   
   


$ virsh start fedora39
error: Failed to start domain 'fedora39'
error: operation failed: Failed to connect to dbus-daemon: The connection is 
closed


Hmm, I don't get this issue. libvirt fails to connect to the just
launched bus (or got closed). This is weird, because we checked
earlier the socket exists.

Could you check dbus log? (for some reason it's not in cfg.logDir, but
in cfg.dbusStateDir - I wonder if we can change that now)



There is no log when it fails (but there is an empty file when it works)
and the configurations differ only in the listen socket path.


thanks



signature.asc
Description: PGP signature

Re: [PATCH v2 10/22] src: add new APIs for marking a domain to autostart once

[Peter Krempa]

On Wed, Mar 12, 2025 at 17:17:50 +, Daniel P. Berrangé wrote:
> When a domain is marked for autostart, it will be started on every
> subsequent host OS boot. There may be times when it is desirable to
> mark a domain to be autostarted, on the next boot only.
> 
> Thus we add virDomainSetAutostartOnce / virDomainGetAutostartOnce.
> 
> An alternative would have been to overload the existing
> virDomainSetAutostart method, to accept values '1' or '2' for
> the autostart flag. This was not done because it is expected
> that language bindings will have mapped the current autostart
> flag to a boolean, and thus turning it into an enum would create
> a compatibility problem.
> 
> A further alternative would have been to create a new method
> virDomainSetAutostartFlags, with a VIR_DOMAIN_AUTOSTART_ONCE
> flag defined. This was not done because it is felt desirable
> to clearly separate the two flags. Setting the "once" flag
> should not interfere with existing autostart setting, whether
> it is enabled or disabled currently.
> 
> The 'virsh autostart' command, however, is still overloaded
> by just adding a --once flag, while current state is added
> to 'virsh dominfo'.
> 
> No ability to filter by 'autostart once' status is added to
> the domain list APIs. The most common use of autostart once
> will be to automatically set it on host shutdown, and it be
> cleared on host startup. Thus there would rarely be scenarios
> in which a running app will need to filter on this new flag.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  include/libvirt/libvirt-domain.h |  4 ++
>  src/driver-hypervisor.h  | 10 
>  src/libvirt-domain.c | 87 
>  src/libvirt_public.syms  |  6 +++
>  src/remote/remote_driver.c   |  2 +
>  src/remote/remote_protocol.x | 30 ++-
>  src/remote_protocol-structs  | 12 +
>  src/rpc/gendispatch.pl   |  4 +-
>  tools/virsh-domain-monitor.c |  7 +++
>  tools/virsh-domain.c | 39 ++
>  10 files changed, 189 insertions(+), 12 deletions(-)

Reviewed-by: Peter Krempa 

Re: [PATCH v2 16/22] rpc: rename virNetDaemonSetShutdownCallbacks

[Peter Krempa]

On Wed, Mar 12, 2025 at 17:17:56 +, Daniel P. Berrangé wrote:
> The next patch will be introducing a new callback, so rename the method
> to virNetDaemonSetLifecycleCallbacks to reflect the more general usage.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/libvirt_remote.syms|  2 +-
>  src/remote/remote_daemon.c |  6 +++---
>  src/rpc/virnetdaemon.c | 10 +-
>  src/rpc/virnetdaemon.h |  8 
>  4 files changed, 13 insertions(+), 13 deletions(-)

Reviewed-by: Peter Krempa 

Re: [PATCH v2 15/22] src: clarify semantics of the various virStateNNN methods

[Peter Krempa]

On Wed, Mar 12, 2025 at 17:17:55 +, Daniel P. Berrangé wrote:
> It is not documented what the various virStateNNN methods are each
> responsible for doing and the names give little guidance either.
> Provide some useful documentation comments to explain the intended
> usage of each.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/libvirt.c | 44 +++-
>  1 file changed, 39 insertions(+), 5 deletions(-)

Reviewed-by: Peter Krempa 

Re: [PATCH v2 19/22] rpc: fix shutdown sequence when preserving state

[Peter Krempa]

On Wed, Mar 12, 2025 at 17:17:59 +, Daniel P. Berrangé wrote:
> The preserving of state (ie running VMs) requires a fully functional
> daemon and hypervisor driver. If any part has started shutting down
> then saving state may fail, or worse, hang.
> 
> The current shutdown sequence does not guarantee safe ordering, as
> we synchronize with the state saving thread only after the hypervisor
> driver has had its 'shutdownPrepare' callback invoked. In the case of
> QEMU this means that worker threads processing monitor events may well
> have been stopped.
> 
> This implements a full state machine that has a well defined ordering
> that an earlier commit documented as the desired semantics.
> 
> With this change, nothing will start shutting down if the state saving
> thread is still running.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/rpc/virnetdaemon.c | 107 ++---
>  1 file changed, 80 insertions(+), 27 deletions(-)

Reviewed-by: Peter Krempa 

Re: [PATCH 1/2] src: normalize whitespace in protocol structs files

[Martin Kletzander]

On Thu, Mar 13, 2025 at 03:57:36PM +, Daniel P. Berrangé wrote:

This makes the output match what current pdwtags will emit,
modulo some whitespace changes made by the check script
before comparison.

Signed-off-by: Daniel P. Berrangé 


Reviewed-by: Martin Kletzander 


signature.asc
Description: PGP signature

Re: [PATCH 2/2] src: add new target for regenerating protocol structs files

[Martin Kletzander]

On Thu, Mar 13, 2025 at 03:57:37PM +, Daniel P. Berrangé wrote:

Introduce a new ninja target

  ninja -C build regen-{PROTO}

eg

  ninja -C build regen-admin_protocol

that will re-create the reference output file based on what the
current pdwtags command emits. A small change is made to squash
whitespace on enum declarations so that introducing a new longer
enum name dosn't trigger re-indent of all existing enum names.


s/dos/does/

Reviewed-by: Martin Kletzander 


signature.asc
Description: PGP signature

Re: [libvirt PATCH 00/10] Unused cleanups

[Martin Kletzander]

On Thu, Mar 13, 2025 at 03:59:07PM +, Daniel P. Berrangé wrote:

On Thu, Mar 13, 2025 at 04:52:13PM +0100, Martin Kletzander wrote:

On Thu, Mar 13, 2025 at 04:30:14PM +0100, Ján Tomko wrote:
> Patches 4-9/10 all depend on patch 3/10
> 10/10 can be pushed independently
>
> Ján Tomko (10):
>  qemu: seccomp sandbox: remove incorect G_GNUC_UNUSED marker
>  qemu: validate: fs: remove unneeded parameter
>  qemu: remove qemuCaps from qemuBuildObjectCommandlineFromJSON
>  qemu: remove qemuCaps from qemuBuildObjectSecretCommandLine
>  qemu: remove qemuCaps from qemuBuildTLSx509CommandLine
>  qemu: remove qemuCaps from qemuBuildObjectCommandline
>  qemu: remove qemuCaps from qemuBuildIOThreadCommandLine
>  qemu: remove unused vm from qemuBuildSEVSNPCommandLine
>  qemu: remove unused vm from qemuBuildPVCommandLine
>  Remove unreachable breaks right after return
>

I wonder whether there was some tool that got confused by return without
break in a switch statement, but in any case that's a problem of that
tool, if such thing exists.


Probably a case where we did

 ret = 

and then did a simple search & replace for 's/ret = /return/', when we
eliminated the need for a centralize return path thanks to g_auto*



Yeah, probably that.





With regards,
Daniel
--
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|



signature.asc
Description: PGP signature

[PATCH 0/6] Couple of memleak fixes

[Michal Privoznik]

*** BLURB HERE ***

Michal Prívozník (6):
  network: Free inhibitor in networkStateCleanup()
  ch: Free retval of curl_slist_append()
  ch: Don't leak virCHDomainObjPrivate struct members
  ch: Free @iothreads array in virCHProcessSetupIOThreads()
  ch: Unref @cfg in virCHProcessStop()
  ch: Rework virCHProcessConnectMonitor()

 src/ch/ch_domain.c  |  3 +++
 src/ch/ch_monitor.c |  3 +++
 src/ch/ch_process.c | 22 +-
 src/network/bridge_driver.c |  2 ++
 4 files changed, 21 insertions(+), 9 deletions(-)

-- 
2.48.1

[PATCH 1/6] network: Free inhibitor in networkStateCleanup()

[Michal Privoznik]

The shutdown inhibitor is created in networkStateInitialize() but
corresponding call to virInhibitorFree() is missing in
networkStateCleanup() leading to a memleak:

116 (72 direct, 44 indirect) bytes in 1 blocks are definitely lost in loss 
record 1,769 of 1,998
   at 0x484CEF3: calloc (vg_replace_malloc.c:1675)
   by 0x4F0E7A9: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.8000.5)
   by 0x4993B9B: virInhibitorNew (virinhibitor.c:152)
   by 0x5279394: networkStateInitialize (bridge_driver.c:654)
   by 0x4CC74DC: virStateInitialize (libvirt.c:665)
   by 0x15B719: daemonRunStateInit (remote_daemon.c:613)
   by 0x49F2B44: virThreadHelper (virthread.c:256)
   by 0x5356662: start_thread (in /usr/lib64/libc.so.6)
   by 0x53D7DA3: clone (in /usr/lib64/libc.so.6)

Signed-off-by: Michal Privoznik 
---
 src/network/bridge_driver.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 80d2c3a1d5..2cad1c8cbe 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -802,6 +802,8 @@ networkStateCleanup(void)
   network_driver->lockFD);
 }
 
+virInhibitorFree(network_driver->inhibitor);
+
 virObjectUnref(network_driver->config);
 virObjectUnref(network_driver->dnsmasqCaps);
 
-- 
2.48.1

[PATCH 2/6] ch: Free retval of curl_slist_append()

[Michal Privoznik]

There are two places where curl_slist_append() is called but
corresponding call to curl_slist_free_all() is missing:
virCHMonitorPutNoContent() and virCHMonitorGet() which leads to
memleaks:

41 (16 direct, 25 indirect) bytes in 1 blocks are definitely lost in loss 
record 992 of 1,998
   at 0x4845888: malloc (vg_replace_malloc.c:446)
   by 0x5B2F8FE: curl_slist_append (in /usr/lib64/libcurl.so.4.8.0)
   by 0xB3A7B41: virCHMonitorPutNoContent (ch_monitor.c:824)
   by 0xB3A89FF: virCHMonitorBootVM (ch_monitor.c:1030)
   by 0xB3AC6F1: virCHProcessStart (ch_process.c:967)
   by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246)
   by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188)
   by 0x168F91: remoteDispatchDomainCreateXML 
(remote_daemon_dispatch_stubs.h:5186)
   by 0x168F18: remoteDispatchDomainCreateXMLHelper 
(remote_daemon_dispatch_stubs.h:5167)
   by 0x4B20066: virNetServerProgramDispatchCall (virnetserverprogram.c:423)
   by 0x4B1FB99: virNetServerProgramDispatch (virnetserverprogram.c:299)
   by 0x4B28B5E: virNetServerProcessMsg (virnetserver.c:135)

88 (16 direct, 72 indirect) bytes in 1 blocks are definitely lost in loss 
record 1,501 of 1,998
   at 0x4845888: malloc (vg_replace_malloc.c:446)
   by 0x5B2F8FE: curl_slist_append (in /usr/lib64/libcurl.so.4.8.0)
   by 0xB3A7E41: virCHMonitorGet (ch_monitor.c:864)
   by 0xB3A92E2: virCHMonitorGetInfo (ch_monitor.c:1157)
   by 0xB3A9CEA: virCHProcessUpdateInfo (ch_process.c:142)
   by 0xB3AAD36: virCHProcessSetup (ch_process.c:492)
   by 0xB3AC75A: virCHProcessStart (ch_process.c:973)
   by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246)
   by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188)
   by 0x168F91: remoteDispatchDomainCreateXML 
(remote_daemon_dispatch_stubs.h:5186)
   by 0x168F18: remoteDispatchDomainCreateXMLHelper 
(remote_daemon_dispatch_stubs.h:5167)
   by 0x4B20066: virNetServerProgramDispatchCall (virnetserverprogram.c:423)

Signed-off-by: Michal Privoznik 
---
 src/ch/ch_monitor.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index e0d6b490de..0ba927a194 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -839,6 +839,8 @@ virCHMonitorPutNoContent(virCHMonitor *mon, const char 
*endpoint,
 if (responseCode == 200 || responseCode == 204)
 ret = 0;
 
+curl_slist_free_all(headers);
+
 return ret;
 }
 
@@ -884,6 +886,7 @@ virCHMonitorGet(virCHMonitor *mon, const char *endpoint, 
virJSONValue **response
 
  cleanup:
 g_free(data.content);
+curl_slist_free_all(headers);
 /* reset the libcurl handle to avoid leaking a stack pointer to data */
 curl_easy_reset(mon->handle);
 
-- 
2.48.1

[PATCH 5/6] ch: Unref @cfg in virCHProcessStop()

[Michal Privoznik]

At the beginning of virCHProcessStop() the ref to driver config
is obtained (via virCHDriverGetConfig()), but corresponding unref
call is lacking. Use g_autoptr() to make sure the config is
unrefed always.

Signed-off-by: Michal Privoznik 
---
 src/ch/ch_process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c
index cbf98faaf0..79f5990cc1 100644
--- a/src/ch/ch_process.c
+++ b/src/ch/ch_process.c
@@ -997,11 +997,11 @@ virCHProcessStop(virCHDriver *driver,
  virDomainObj *vm,
  virDomainShutoffReason reason)
 {
+g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver);
 int ret;
 int retries = 0;
 unsigned int hostdev_flags = VIR_HOSTDEV_SP_PCI;
 virCHDomainObjPrivate *priv = vm->privateData;
-virCHDriverConfig *cfg = virCHDriverGetConfig(driver);
 virDomainDef *def = vm->def;
 size_t i;
 
-- 
2.48.1

[PATCH 4/6] ch: Free @iothreads array in virCHProcessSetupIOThreads()

[Michal Privoznik]

When the CH driver starts a domain virCHProcessSetupIOThreads()
is called eventually which in turn calls
virCHMonitorGetIOThreads(). The latter returns an array of
iothreads which is never freed leading to a memleak:

130 (104 direct, 26 indirect) bytes in 1 blocks are definitely lost in loss 
record 1,804 of 1,998
   at 0x484CEF3: calloc (vg_replace_malloc.c:1675)
   by 0x4F0E7A9: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.8000.5)
   by 0xB3A9359: virCHMonitorGetIOThreads (ch_monitor.c:1183)
   by 0xB3AA5BB: virCHProcessSetupIOThreads (ch_process.c:348)
   by 0xB3AAC59: virCHProcessSetup (ch_process.c:480)
   by 0xB3AC75A: virCHProcessStart (ch_process.c:973)
   by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246)
   by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188)
   by 0x168F91: remoteDispatchDomainCreateXML 
(remote_daemon_dispatch_stubs.h:5186)
   by 0x168F18: remoteDispatchDomainCreateXMLHelper 
(remote_daemon_dispatch_stubs.h:5167)
   by 0x4B20066: virNetServerProgramDispatchCall (virnetserverprogram.c:423)
   by 0x4B1FB99: virNetServerProgramDispatch (virnetserverprogram.c:299)

Signed-off-by: Michal Privoznik 
---
 src/ch/ch_process.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c
index 9a85f7869e..cbf98faaf0 100644
--- a/src/ch/ch_process.c
+++ b/src/ch/ch_process.c
@@ -344,6 +344,7 @@ virCHProcessSetupIOThreads(virDomainObj *vm)
 virDomainIOThreadInfo **iothreads = NULL;
 size_t i;
 int niothreads;
+int ret = -1;
 
 if ((niothreads = virCHMonitorGetIOThreads(priv->monitor, &iothreads)) < 0)
 return -1;
@@ -351,9 +352,16 @@ virCHProcessSetupIOThreads(virDomainObj *vm)
 for (i = 0; i < niothreads; i++) {
 VIR_DEBUG("IOThread index = %zu , tid = %d", i, 
iothreads[i]->iothread_id);
 if (virCHProcessSetupIOThread(vm, iothreads[i]) < 0)
-return -1;
+goto cleanup;
 }
-return 0;
+
+ret = 0;
+ cleanup:
+for (i = 0; i < niothreads; i++) {
+virDomainIOThreadInfoFree(iothreads[i]);
+}
+g_free(iothreads);
+return ret;
 }
 
 static int
-- 
2.48.1

[PATCH 6/6] ch: Rework virCHProcessConnectMonitor()

[Michal Privoznik]

Firstly, let's switch from explicit virCHDriverGetConfig() +
virObjectUnref() combo to g_autoptr(virCHDriverConfig). This
leaves us with the @monitor variable which is initialized to NULL
only to be then set to the retval of virCHMonitorNew() and
returned instantly. Well, the variable is now useless and can be
dropped.

Signed-off-by: Michal Privoznik 
---
 src/ch/ch_process.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c
index 79f5990cc1..ee86430e08 100644
--- a/src/ch/ch_process.c
+++ b/src/ch/ch_process.c
@@ -53,13 +53,9 @@ virCHProcessConnectMonitor(virCHDriver *driver,
virDomainObj *vm,
int logfile)
 {
-virCHMonitor *monitor = NULL;
-virCHDriverConfig *cfg = virCHDriverGetConfig(driver);
+g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver);
 
-monitor = virCHMonitorNew(vm, cfg, logfile);
-
-virObjectUnref(cfg);
-return monitor;
+return virCHMonitorNew(vm, cfg, logfile);
 }
 
 static void
-- 
2.48.1

Re: [PATCH 1/6] network: Free inhibitor in networkStateCleanup()

[Peter Krempa]

On Thu, Mar 13, 2025 at 14:44:33 +0100, Michal Privoznik wrote:
> The shutdown inhibitor is created in networkStateInitialize() but
> corresponding call to virInhibitorFree() is missing in
> networkStateCleanup() leading to a memleak:

good that it's a "singleton"

> 
> 116 (72 direct, 44 indirect) bytes in 1 blocks are definitely lost in loss 
> record 1,769 of 1,998
>at 0x484CEF3: calloc (vg_replace_malloc.c:1675)
>by 0x4F0E7A9: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.8000.5)
>by 0x4993B9B: virInhibitorNew (virinhibitor.c:152)
>by 0x5279394: networkStateInitialize (bridge_driver.c:654)
>by 0x4CC74DC: virStateInitialize (libvirt.c:665)
>by 0x15B719: daemonRunStateInit (remote_daemon.c:613)
>by 0x49F2B44: virThreadHelper (virthread.c:256)
>by 0x5356662: start_thread (in /usr/lib64/libc.so.6)
>by 0x53D7DA3: clone (in /usr/lib64/libc.so.6)
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/network/bridge_driver.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Peter Krempa 

Re: [PATCH v8 13/18] qemuxmlconftest: Add 'throttlefilter' tests

[Peter Krempa]

On Wed, Feb 19, 2025 at 22:27:17 +0530, Harikumar Rajkumar wrote:
> From: Chun Feng Wu 
> 
> * Add tests for throttlegroup domain xml processing, including
> groups referenced and not referenced by filters
> * Add tests for throttlefilter domain xml processing, including
> throttle group referenced by different disks
> * Add negative test case to report error when iotune is configured
> together with throttle filters
> 
> Signed-off-by: Chun Feng Wu 
> 
> * Isolate domain xml test
> 
> Signed-off-by: Harikumar Rajkumar 
> ---
>  .../throttlefilter-invalid.x86_64-latest.err  |   1 +
>  .../throttlefilter-invalid.xml|  89 +++
>  .../throttlefilter.x86_64-latest.args |  55 +
>  .../throttlefilter.x86_64-latest.xml  | 105 ++
>  tests/qemuxmlconfdata/throttlefilter.xml  |  95 
>  tests/qemuxmlconftest.c   |   2 +
>  6 files changed, 347 insertions(+)
>  create mode 100644 
> tests/qemuxmlconfdata/throttlefilter-invalid.x86_64-latest.err
>  create mode 100644 tests/qemuxmlconfdata/throttlefilter-invalid.xml
>  create mode 100644 tests/qemuxmlconfdata/throttlefilter.x86_64-latest.args
>  create mode 100644 tests/qemuxmlconfdata/throttlefilter.x86_64-latest.xml
>  create mode 100644 tests/qemuxmlconfdata/throttlefilter.xml

One of the test cases ought to test also the case when copy-on-read is
enabled.

[PATCH 3/6] ch: Don't leak virCHDomainObjPrivate struct members

[Michal Privoznik]

There are some members of the virCHDomainObjPrivate struct that
are allocated at various stages of domain lifecycle but then are
never freed:

1) cgroup - allocated in virDomainCgroupSetupCgroup()
2) autoCpuset - this one is actually never allocated (and thus is
always NULL, but soon it may be used. Just free
it for now, which is a NOP anyways.
3) autoNodeset - same story as 2).

There are two more members, which shouldn't be freed:

1) driver - this is just a raw pointer to the CH driver (see
   virCHDomainObjPrivateAlloc()).

2) monitor - this member is cleared in virCHProcessStop(), way
 before control even gets to
 virCHDomainObjPrivateFree().

452 (400 direct, 52 indirect) bytes in 1 blocks are definitely lost in loss 
record 1,944 of 1,998
   at 0x484CEF3: calloc (vg_replace_malloc.c:1675)
   by 0x4F0E7A9: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.8000.5)
   by 0x49479CE: virCgroupNewFromParent (vircgroup.c:893)
   by 0x49481BA: virCgroupNewDomainPartition (vircgroup.c:1068)
   by 0x494915E: virCgroupNewMachineManual (vircgroup.c:1378)
   by 0x49492FE: virCgroupNewMachine (vircgroup.c:1432)
   by 0x4B5E3DE: virDomainCgroupInitCgroup (domain_cgroup.c:377)
   by 0x4B5E9CD: virDomainCgroupSetupCgroup (domain_cgroup.c:524)
   by 0xB3AC693: virCHProcessStart (ch_process.c:951)
   by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246)
   by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188)
   by 0x168F91: remoteDispatchDomainCreateXML 
(remote_daemon_dispatch_stubs.h:5186)

Signed-off-by: Michal Privoznik 
---
 src/ch/ch_domain.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
index 4f5966adce..a08b18c5b9 100644
--- a/src/ch/ch_domain.c
+++ b/src/ch/ch_domain.c
@@ -65,6 +65,9 @@ virCHDomainObjPrivateFree(void *data)
 
 virChrdevFree(priv->chrdevs);
 g_free(priv->machineName);
+virBitmapFree(priv->autoCpuset);
+virBitmapFree(priv->autoNodeset);
+virCgroupFree(priv->cgroup);
 g_free(priv);
 }
 
-- 
2.48.1

Re: [PATCH] NEWS: Mention new 'image_format' parameter for virDomainSaveParams

[Jim Fehlig via Devel]

Hi All,

Any comments on this addition to NEWS?

Regards,
Jim

On 3/3/25 11:34, Jim Fehlig wrote:

Signed-off-by: Jim Fehlig 
---
  NEWS.rst | 8 
  1 file changed, 8 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 9c940b1a81..d08a18dc02 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -17,6 +17,14 @@ v11.2.0 (unreleased)
  
  * **New features**
  
+  * qemu: Add new 'image_format' parameter to virDomainSaveParams

+
+``virDomainSaveParams`` now supports an ``image_format`` parameter for
+specifying the save image format on a per-domain basis. The parameter
+accepts the same values as the driver-wide ``save_image_format`` setting
+in ``qemu.conf``. An image format specified via ``virDomainSaveParams``
+takes precedence over the driver-wide setting.
+
  * **Improvements**
  
  * **Bug fixes**

Re: [libvirt PATCH 05/10] qemu: remove qemuCaps from qemuBuildTLSx509CommandLine

[Peter Krempa]

On Thu, Mar 13, 2025 at 16:30:19 +0100, Ján Tomko wrote:
> Also from qemuBuildGraphicsVNCCommandLine
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_command.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 4f0a9a517c..ef1ba9269c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1286,8 +1286,7 @@ qemuBuildTLSx509CommandLine(virCommand *cmd,


'qemuCaps' is documented above.

>  bool isListen,
>  bool verifypeer,
>  const char *certEncSecretAlias,
> -const char *alias,
> -virQEMUCaps *qemuCaps G_GNUC_UNUSED)
> +const char *alias)
>  {
>  g_autoptr(virJSONValue) props = NULL;

Re: [PATCH] libxl_conf: Implement hyperv domain flags for Xen

[Jim Fehlig via Devel]

Hi Will,

Thanks for the patch! Looks good. I only have some minor comments.

On 3/4/25 02:11, Will wrote:

Adds support for configuring  flags for domains
running under Xen.

The following flags, making use of QEMU's existing flags, are now
configurable for Xen: vapic, synic, stimer, frequencies, tlbflush and
ipi

Tests have been added validating translation to libxl's viridian flags

Updated docs section on  flags to note support and to specify
which flags work with Xen

Signed-off-by: Will 
---
  NEWS.rst  |   5 +


Please split the NEWS changes to a separate patch.


  docs/formatdomain.rst |  20 +--
  src/libxl/libxl_conf.c|  90 ++
  .../viridian-hvm-full.json| 101 
  .../viridian-hvm-full.xml |  45 +
  .../viridian-hvm-none.json|  89 ++
  .../viridian-hvm-none.xml |  36 
  .../libxlxml2domconfigdata/viridian-hvm.json  |  99 +++
  tests/libxlxml2domconfigdata/viridian-hvm.xml |  42 +
  .../viridian-passthrough.json | 155 ++
  .../viridian-passthrough.xml  |  37 +
  tests/libxlxml2domconfigtest.c|   9 +
  12 files changed, 718 insertions(+), 10 deletions(-)
  create mode 100644 tests/libxlxml2domconfigdata/viridian-hvm-full.json
  create mode 100644 tests/libxlxml2domconfigdata/viridian-hvm-full.xml
  create mode 100644 tests/libxlxml2domconfigdata/viridian-hvm-none.json
  create mode 100644 tests/libxlxml2domconfigdata/viridian-hvm-none.xml
  create mode 100644 tests/libxlxml2domconfigdata/viridian-hvm.json
  create mode 100644 tests/libxlxml2domconfigdata/viridian-hvm.xml
  create mode 100644 tests/libxlxml2domconfigdata/viridian-passthrough.json
  create mode 100644 tests/libxlxml2domconfigdata/viridian-passthrough.xml

diff --git a/NEWS.rst b/NEWS.rst
index 9c940b1a81..d6ad961f56 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -17,6 +17,11 @@ v11.2.0 (unreleased)
  
  * **New features**
  
+  * Support for configuration of  flags for Xen domains.

+
+The following flags are now configurable for Xen: ``vapic``, ``synic``,
+``stimer``, ``frequencies``, ``tlbflush`` and ``ipi``.
+
  * **Improvements**
  
  * **Bug fixes**

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index cbe378e61d..4162ae7930 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -2084,31 +2084,31 @@ are:
 based virtualization drivers, such as LXC.
  ``hyperv``
 Enable various features improving behavior of guests running Microsoft
-   Windows.
+   Windows. :since:`Since 11.2.0` some of these flags are also available for 
Xen domains running Microsoft Windows.


The length of lines should be consistent with the rest of the file. The below 
table is a bit of an exception, and I think your changes there are fine as is.


  
 === ==  

 Feature Description
ValueSince
 === 
== 
 

-   relaxed Relax constraints on timers 
   on, off  :since:`1.0.0 (QEMU 
2.0)`
-   vapic   Enable virtual APIC 
   on, off  :since:`1.1.0 (QEMU 
2.0)`
+   relaxed Relax constraints on timers 
   on, off  :since:`1.0.0 (QEMU 
2.0), 11.2.0 (Xen, always on)`
+   vapic   Enable virtual APIC 
   on, off  :since:`1.1.0 (QEMU 
2.0), 11.2.0 (Xen)`
 spinlocks   Enable spinlock support
on, off; retries - at least 4095 :since:`1.1.0 (QEMU 
2.0)`
-   vpindex Virtual processor index 
   on, off  :since:`1.3.3 (QEMU 
2.5)`
+   vpindex Virtual processor index 
   on, off  :since:`1.3.3 (QEMU 
2.5), 11.2.0 (Xen, always on)`
 runtime Processor time spent on running guest code and on behalf 
of guest code on, off  :since:`1.3.3 (QEMU 
2.5)`
-   synic   Enable Synthetic Interrupt Controller (SynIC)   
  

Re: [PATCH V4 00/18] qemu: support mapped-ram+directio+mulitfd

[Jim Fehlig via Devel]

Hi All,

Does anyone have time to review this series? QEMU has supported mapped-ram for a 
few releases now. Adding support in libvirt allows us to build parallel 
save/restore on top. Would be a nice feature for the next release :-).


Regards,
Jim

On 3/5/25 15:48, Jim Fehlig wrote:

V4 series adding support for QEMU's mapped-ram stream format [1] and
migration capability. The use of mapped-ram is controlled by extending
save_image_format setting in qemu.conf with a new 'sparse' option. The
'raw' format continues to be the default.

Also included are patches that leverage mapped-ram to add support for
parallel save/restore.

Changes in V4:
* Rebased on latest master, including the series
   "qemu: Support specifying save image format"

V1:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/MNBHGQH7PKV4RXQZXLPAGMOTNEVR3JVS/

V2:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/GNJNAXSB77PNUKOZ7Q4D5RVF23SAVMDN/

V3:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/SK2PSRXO4MPKRZLP74GR4J2EUVI5VVHW/

[1] 
https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/migration/mapped-ram.rst?ref_type=heads

Claudio Fontana (3):
   include: Define constants for parallel save/restore
   tools: add parallel parameter to virsh save command
   tools: add parallel parameter to virsh restore command

Jim Fehlig (15):
   lib: virDomain{Save,Restore}Params: Ensure absolute path
   qemu: Add function to get FDPass object from monitor
   qemu: Add function to check capability in migration params
   qemu: Add function to get bool value from migration params
   qemu: Add mapped-ram migration capability
   qemu: Add function to get migration params for save
   qemu_saveimage: add "sparse" to supported save image formats
   qemu: Add helper function for creating save image fd
   qemu: Add support for mapped-ram on save
   qemu: Move creation of qemuProcessIncomingDef struct
   qemu: Apply migration parameters in qemuMigrationDstRun
   qemu: Add support for mapped-ram on restore
   qemu: Support O_DIRECT with mapped-ram on save
   qemu: Support O_DIRECT with mapped-ram on restore
   qemu: Add support for parallel save and restore

  docs/manpages/virsh.rst  |  21 +++-
  include/libvirt/libvirt-domain.h |  11 ++
  src/libvirt-domain.c |  92 +++---
  src/qemu/qemu.conf.in|   9 +-
  src/qemu/qemu_driver.c   |  78 +---
  src/qemu/qemu_fd.c   |  46 +++
  src/qemu/qemu_fd.h   |   4 +
  src/qemu/qemu_migration.c| 204 ++-
  src/qemu/qemu_migration.h|  10 +-
  src/qemu/qemu_migration_params.c |  92 ++
  src/qemu/qemu_migration_params.h |  17 +++
  src/qemu/qemu_monitor.c  |  37 ++
  src/qemu/qemu_monitor.h  |   5 +
  src/qemu/qemu_process.c  | 100 ++-
  src/qemu/qemu_process.h  |  19 ++-
  src/qemu/qemu_saveimage.c| 120 --
  src/qemu/qemu_saveimage.h|   4 +
  src/qemu/qemu_snapshot.c |  15 ++-
  tools/virsh-domain.c |  81 ++--
  19 files changed, 775 insertions(+), 190 deletions(-)

Re: [PATCH v2 00/21] Add qemu RDP server support

[Martin Kletzander]

On Thu, Mar 13, 2025 at 11:59:41AM +0400, Marc-André Lureau wrote:

Hi

On Tue, Mar 11, 2025 at 5:28 PM Martin Kletzander  wrote:

So I tried testing it and I found a couple of issues.  Those might be on
my side, maybe caused by misunderstanding of it all.  First of all
 fails with these patches for me even without


How does it fail? (it works on my end)



$ virsh dumpxml fedora39 --xpath //graphics


  
  


$ virsh version
Compiled against library: libvirt 11.1.0
Using library: libvirt 11.1.0
Using API: QEMU 11.1.0
Running hypervisor: QEMU 9.2.2

$ virsh start fedora39
Domain 'fedora39' started

$ virsh destroy fedora39
Domain 'fedora39' destroyed

$ echo 'Start daemon with patches'
Start daemon with patches
$ virsh version
Compiled against library: libvirt 11.1.0
Using library: libvirt 11.1.0
Using API: QEMU 11.1.0
Running hypervisor: QEMU 9.2.2

$ # Same version, but this time with the patch series applied
$ virsh dumpxml fedora39 --xpath //graphics


  
  


$ virsh start fedora39
error: Failed to start domain 'fedora39'
error: operation failed: Failed to connect to dbus-daemon: The connection is 
closed


.  That might be the source of all other issues,
but since I was not sure whether to use p2p='yes' or
address='unix:path=...' I tried both.  When address is specified qemu
tries to connect to it, so that's not the right approach.  When I
specify p2p='yes' then the VM starts.  If I add the RDP graphics I get a
"confusing" error message:

error: unsupported configuration: qemu-rdp support requires a D-Bus bus
graphics device.

which should probably say "qemu-rdp support requires a non-p2p dbus
graphics device" instead since it is looking for a p2p='no' dbus
graphics.


Not sure what is the best wording, but I understand it is confusing.
It requires a "D-Bus bus" to work. (we could make it work with p2p
too, eventually, but that might make multi-process handling more
complicated in the future - say if vhost-user-gpu exports the dbus
display interface etc)



No matter how, but mentioning that it must not be p2p would help.



If I delay the checking of qemu-rdp running I find out that it fails
right away.  It cannot connect to the unix socket, so I tried just
running qemu VM with dbus graphics and qemu-rdp manually, but that
failed on the missing certificate files.  That could be checked before
blindly passing it down to the helper, too (even though I was running it
manually this time it reminded me of that possibility).


Are you saying that libvirt should check the presence of certificates
before running qemu-rdp and provide an early error?
qemu rdp log is explicit (Error: reading server cert `...`). However,
I admit libvirt error report isn't great, as it fails with "The name
org.QemuDisplay.RDP was not provided by any .service files". I will
look into improving that.



Thanks.  I haven't encountered the certificate issue, as I generated the
certificated before I noticed that we're passing it blindly.  But if the
error gets spat out correctly, then that's fine, I guess.




Ideally some handshake could confirm it is actually running.  If not a
handshake, then maybe waiting whether it listens on where it is supposed
to or some other indication of it being started while checking that the
PID exists, so that it is not a dumb delay.

Even running it manually I cannot verify it works, but that's probably a
client-side issue?

Server output when trying rdesktop:
Waiting for org.qemu...
Starting RDP server, args: ServerArgs { bind_address: 0.0.0.0:3389, cert: None, 
key: None, remotefx: Disable }
Cert: "/home/nert/.config/qemu-rdp/server-cert.pem", Key: 
"/home/nert/.config/qemu-rdp/server-key.pem"
2025-03-11T13:21:00.350891Z ERROR ironrdp_server::server: Connection error 
error=[no credentials while doing credssp] general error
2025-03-11T13:21:00.352401Z ERROR ironrdp_server::server: Connection error 
error=accept_begin failed
Caused by:
 [failed to negotiate security protocol] general error

sdl-freerdp client output (no error on the server side reported):
[14:24:35:046] [449399:0006db7b] [ERROR][com.freerdp.core.transport] - 
[transport_default_write]: BIO_should_retry returned a system error 32: Broken 
pipe

rdesktop client output:
Failed to initialize NLA, do you have correct Kerberos TGT initialized ?
Core(error): rcp_recv(), connection closed by peer



If you run it manually, make sure you set the credentials before
connecting the client, ex:
busctl --user call org.QemuDisplay.RDP /org/qemu_display/rdp
org.QemuDisplay.RDP SetCredentials sss user pass ''



That's not mentioned in https://crates.io/crates/qemu-rdp =D =D =D


===

Anyway, I'm clearly doing something wrong, but there are some issues in
the code as well and from the greater picture it should not be this
complicated to test it out, should it?  I can test out other patches
from you if you want, and I am open to hearing tips on what I'm doing
wrong.



Thanks a lot for testing and the suggestions!



You're welcome, thanks for t

Re: [PATCH v2 03/22] hypervisor: expand available shutdown actions

[Daniel P . Berrangé]

On Thu, Mar 13, 2025 at 09:00:58AM +0100, Peter Krempa wrote:
> On Wed, Mar 12, 2025 at 17:17:43 +, Daniel P. Berrangé wrote:
> > The auto shutdown code can currently only perform managed save,
> > which may fail in some cases, for example when PCI devices are
> > assigned. On failure, shutdown inhibitors remain in place which
> > may be undesirable.
> > 
> > This expands the logic to try a sequence of operations
> > 
> >  * Managed save
> >  * Graceful shutdown
> >  * Forced poweroff
> > 
> > Each of these operations can be enabled or disabled, but they
> > are always applied in this order.
> > 
> > With the shutdown option, a configurable time is allowed for
> > shutdown to complete, defaulting to 30 seconds, before moving
> > onto the forced poweroff phase.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/hypervisor/domain_driver.c | 130 -
> >  src/hypervisor/domain_driver.h |   7 ++
> >  src/qemu/qemu_driver.c |   3 +
> >  3 files changed, 121 insertions(+), 19 deletions(-)
> 
> [...]
> 
> > 
> > @@ -732,31 +748,107 @@ 
> > virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> > 
> > VIR_CONNECT_LIST_DOMAINS_ACTIVE)) < 0)
> >  goto cleanup;
> >  
> > -flags = g_new0(unsigned int, numDomains);
> > +VIR_DEBUG("Auto shutdown with %d running domains", numDomains);
> > +if (cfg->trySave) {
> > +g_autofree unsigned int *flags = g_new0(unsigned int, numDomains);
> > +for (i = 0; i < numDomains; i++) {
> > +int state;
> > +/*
> > + * Pause all VMs to make them stop dirtying pages,
> > + * so save is quicker. We remember if any VMs were
> > + * paused so we can restore that on resume.
> > + */
> > +flags[i] = VIR_DOMAIN_SAVE_RUNNING;
> > +if (virDomainGetState(domains[i], &state, NULL, 0) == 0) {
> > +if (state == VIR_DOMAIN_PAUSED)
> > +flags[i] = VIR_DOMAIN_SAVE_PAUSED;
> > +}
> > +if (flags[i] & VIR_DOMAIN_SAVE_RUNNING)
> > +virDomainSuspend(domains[i]);
> > +}
> > +
> > +for (i = 0; i < numDomains; i++) {
> > +if (virDomainManagedSave(domains[i], flags[i]) < 0) {
> > +VIR_WARN("Unable to perform managed save of '%s': %s",
> > + virDomainGetName(domains[i]),
> 
> virDomainGetName() calls virResetLastError()
> 
> > + virGetLastErrorMessage());
> 
> IIRC argument evaluation order is not defined this might have random
> results.

In fact it is worse than that. If virGetLastErrorMessage runs
second (order as written) it will get no error message. If it
runs first (reverse as written) it will get the error message
which virDomainGetName will cause to be free()d, so it will
be passed freed memory to VIR_WARN.


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Dr. David Alan Gilbert]

* Thomas Huth (th...@redhat.com) wrote:
> On 09/03/2025 19.55, Pierrick Bouvier wrote:
> > On 6/3/24 05:18, Daniel P. Berrangé wrote:
> > > On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
> > > > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
> > > > > We are trying to unify all qemu-system-FOO to a single binary.
> > > > > In order to do that we need to remove QAPI target specific code.
> > > > > 
> > > > > @dump-skeys is only available on qemu-system-s390x. This series
> > > > > rename it as @dump-s390-skey, making it available on other
> > > > > binaries. We take care of backward compatibility via deprecation.
> > > > > 
> > > > > Philippe Mathieu-Daudé (4):
> > > > >     hw/s390x: Introduce the @dump-s390-skeys QMP command
> > > > >     hw/s390x: Introduce the 'dump_s390_skeys' HMP command
> > > > >     hw/s390x: Deprecate the HMP 'dump_skeys' command
> > > > >     hw/s390x: Deprecate the QMP @dump-skeys command
> > > > 
> > > > Why do we have to rename the command? Just for the sake of it? I think
> > > > renaming HMP commands is maybe ok, but breaking the API in QMP is 
> > > > something
> > > > you should consider twice.
> > > 
> > > That was going to be my question too. Seems like its possible to simply
> > > stub out the existing command for other targets.
> > > 
> > > The renaming is just window dressing.
> > > 
> > 
> > Working on single-binary topic means specificities from every qemu
> > binary/ architecture has to be merged together. Despite appearing has a
> > bad thing now, it's definitely a step forward for QEMU, and will allow
> > to enable new usages.
> > 
> > The hard way is to trigger a deep refactoring, involving lengthy
> > conversations where compromises have to be found ("let's implement this
> > for all arch"). The pragmatic way is to eliminate obvious stuff.
> > 
> > This command is specific to an arch, so renaming is a good and obvious
> > strategy. For the backward compatible anxious developer, another
> > strategy would be to simply declare this command if the running target
> > is s390x. But then, you create a precedent to do something that should
> > not have existed in the first place.
> > 
> > +1 for the renaming, and hope that users of this command are able to
> > change a line in their script to adapt to the new command.
> 
> Sorry, but no: We've got plenty of other target specific commands...
> rtc-reset-reinjection , query-sev, query-gic-capabilities, just to name some
> few. So unless you provide a patch series to rename *all* of them and
> deprecate the previous names, I don't see the point why changing just one
> single s390x command is necessary.

I'd probably agree; mind you, it wouldn't be a bad convention to
adopt in general.
For HMP, since there's no need to have a fixed schema for a command,
it would be fine to have a generic command for all architectures
that have a similar idea even if their data is very different.

Dave

>  Thomas
> 
-- 
 -Open up your eyes, open up your mind, open up your code ---   
/ Dr. David Alan Gilbert|   Running GNU/Linux   | Happy  \ 
\dave @ treblig.org |   | In Hex /
 \ _|_ http://www.treblig.org   |___/

Re: [PATCH v4 0/1] add RAPL feature in libvirt

[Igor Mammedov]

On Thu,  6 Mar 2025 13:51:35 +0100
Anthony Harivel  wrote:

> Hi,
> 
> The enhancement in this new version based on last feedback are:
> 
> * add more documentation on limitation of the feature
> * no more double check of the vmsr socket, once is enough
> * virQEMUBuildBufferEscapeComma() is used for user input Path
> 
> Because there is no garantie at the moment that the helper will be one 
> day managed by libvirt, I decided not adding any preparation about this 
> to avoid any confusion but add more documentation.
> 
> If the helper is managed in libvirt, the patch introducing it will also 
> add the necessary API like mention on the previous review (i.e 
> mode='unmanaged').

I'd prefer wait till QEMU side is refactored to a more maintainable
design (which would include CLI changes). So that libvirt won't have
to maintain both current (which should be deprecated/removed) and
to be CLI interface.

> 
> Thanks
> Anthony
> 
> 
> Anthony Harivel (1):
>   qemu: Add support for RAPL MSRs feature
> 
>  docs/formatdomain.rst  | 15 +++
>  src/conf/domain_conf.c | 18 ++
>  src/conf/domain_conf.h |  2 ++
>  src/conf/schemas/domaincommon.rng  | 10 ++
>  src/qemu/qemu_command.c|  9 +
>  tests/qemuxmlconfdata/kvm-features-off.xml |  1 +
>  .../kvm-features.x86_64-latest.args|  2 +-
>  tests/qemuxmlconfdata/kvm-features.xml |  1 +
>  8 files changed, 57 insertions(+), 1 deletion(-)
> 

Re: [PATCH v2 17/21] qemu: add qemu-rdp helper unit

[Martin Kletzander]

On Thu, Mar 13, 2025 at 12:13:44AM +0400, Marc-André Lureau wrote:

Hi

On Fri, Mar 7, 2025 at 6:40 PM Martin Kletzander  wrote:


On Tue, Feb 18, 2025 at 02:16:22PM +0400, marcandre.lur...@redhat.com wrote:
>From: Marc-André Lureau 
>
>Helpers to start the qemu-rdp server and set it up.
>
>Signed-off-by: Marc-André Lureau 
>---
> po/POTFILES|   1 +
> src/qemu/meson.build   |   1 +
> src/qemu/qemu_domain.c |   1 +
> src/qemu/qemu_domain.h |   2 +
> src/qemu/qemu_rdp.c| 405 +
> src/qemu/qemu_rdp.h|  71 
> 6 files changed, 481 insertions(+)
> create mode 100644 src/qemu/qemu_rdp.c
> create mode 100644 src/qemu/qemu_rdp.h
>
>diff --git a/src/qemu/qemu_rdp.c b/src/qemu/qemu_rdp.c
>new file mode 100644
>index 00..e881b74ee3
>--- /dev/null
>+++ b/src/qemu/qemu_rdp.c
>@@ -0,0 +1,405 @@
>+/*
>+ * qemu_rdp.c: QEMU Rdp support
>+ *
>+ * 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 
>+
>+#include "qemu_dbus.h"
>+#include "qemu_extdevice.h"
>+#include "qemu_security.h"
>+#include "qemu_rdp.h"
>+#include "virenum.h"
>+#include "virerror.h"
>+#include "virjson.h"
>+#include "virlog.h"
>+#include "virpidfile.h"
>+#include "virutil.h"
>+#include "virgdbus.h"
>+
>+#define VIR_FROM_THIS VIR_FROM_NONE
>+
>+VIR_LOG_INIT("qemu.rdp");
>+
>+VIR_ENUM_IMPL(qemuRdpFeature,
>+  QEMU_RDP_FEATURE_LAST,
>+  "",
>+  "dbus-address",
>+  "remotefx"
>+);
>+
>+#define ORG_QEMUDISPLAY_RDP "org.QemuDisplay.RDP"
>+#define ORG_QEMUDISPLAY_RDP_PATH "/org/qemu_display/rdp"
>+#define ORG_QEMUDISPLAY_RDP_IFACE "org.QemuDisplay.RDP"
>+
>+
>+void
>+qemuRdpFree(qemuRdp *rdp)
>+{
>+if (!rdp)
>+return;
>+
>+virBitmapFree(rdp->features);
>+g_free(rdp);
>+}
>+
>+
>+void
>+qemuRdpSetFeature(qemuRdp *rdp,
>+  qemuRdpFeature feature)
>+{
>+ignore_value(virBitmapSetBit(rdp->features, feature));
>+}
>+
>+
>+bool
>+qemuRdpHasFeature(const qemuRdp *rdp,
>+  qemuRdpFeature feature)
>+{
>+return virBitmapIsBitSet(rdp->features, feature);
>+}
>+
>+
>+qemuRdp *
>+qemuRdpNew(void)
>+{
>+g_autoptr(qemuRdp) rdp = g_new0(qemuRdp, 1);
>+
>+rdp->features = virBitmapNew(QEMU_RDP_FEATURE_LAST);
>+rdp->pid = -1;
>+
>+return g_steal_pointer(&rdp);
>+}
>+
>+
>+qemuRdp *
>+qemuRdpNewForHelper(const char *helper)
>+{
>+g_autoptr(qemuRdp) rdp = NULL;
>+g_autoptr(virCommand) cmd = NULL;
>+g_autofree char *output = NULL;
>+g_autoptr(virJSONValue) doc = NULL;
>+virJSONValue *featuresJSON;
>+g_autofree char *helperPath = NULL;
>+size_t i, nfeatures;
>+
>+helperPath = virFindFileInPath(helper);
>+if (!helperPath) {
>+virReportSystemError(errno,
>+ _("'%1$s' is not a suitable qemu-rdp helper 
name"),
>+ helper);
>+return NULL;
>+}
>+
>+rdp = qemuRdpNew();
>+cmd = virCommandNewArgList(helperPath, "--print-capabilities", NULL);
>+virCommandSetOutputBuffer(cmd, &output);
>+if (virCommandRun(cmd, NULL) < 0)
>+return NULL;
>+
>+if (!(doc = virJSONValueFromString(output)) ||
>+!(featuresJSON = virJSONValueObjectGetArray(doc, "features"))) {
>+virReportError(VIR_ERR_INTERNAL_ERROR,
>+   _("unable to parse json capabilities '%1$s'"),

It feels to me like this needs a "from" or "for" in order to
disambiguate the meaning of the string, just:

s/capabilities/capabilities for/
s/capabilities/capabilities from/

would be sufficient.


The same error already exists in qemu_vhost_user.c and qemu_slirp.c.
Should I touch that too?



Oh, I haven't noticed that.  I'll leave that up to you, I'd prefer it
with the preposition.


signature.asc
Description: PGP signature

Re: [PATCH v2 20/21] qemu: add RDP support

[Martin Kletzander]

On Thu, Mar 13, 2025 at 12:18:04AM +0400, Marc-André Lureau wrote:

Hi

On Fri, Mar 7, 2025 at 7:03 PM Martin Kletzander  wrote:


On Tue, Feb 18, 2025 at 02:16:25PM +0400, marcandre.lur...@redhat.com wrote:
>From: Marc-André Lureau 
>
>Wire the external server RDP support with QEMU.
>
>Check the configuration, allocate a port, start the process
>and set the credentials.
>
>Signed-off-by: Marc-André Lureau 
>---
> docs/formatdomain.rst |  25 --
> src/conf/domain_conf.h|   1 +
> src/qemu/qemu_extdevice.c |  46 +--
> src/qemu/qemu_hotplug.c   |  49 ++-
> src/qemu/qemu_hotplug.h   |   1 +
> src/qemu/qemu_process.c   | 167 ++
> 6 files changed, 257 insertions(+), 32 deletions(-)
>
>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>index 28ca321c5c..5dae70cf7f 100644
>--- a/src/qemu/qemu_hotplug.c
>+++ b/src/qemu/qemu_hotplug.c
>@@ -4423,6 +4423,7 @@ int
> qemuDomainChangeGraphicsPasswords(virDomainObj *vm,
>   int type,
>   virDomainGraphicsAuthDef *auth,
>+  const char *defaultUsername,
>   const char *defaultPasswd,
>   int asyncJob)
> {
>@@ -4432,13 +4433,19 @@ qemuDomainChangeGraphicsPasswords(virDomainObj *vm,
> g_autofree char *validTo = NULL;
> const char *connected = NULL;
> const char *password;
>+const char *username;
> int ret = -1;
>
> if (!auth->passwd && !defaultPasswd)
> return 0;
>
>+username = auth->username ? auth->username : defaultUsername;
> password = auth->passwd ? auth->passwd : defaultPasswd;
>
>+if (type == VIR_DOMAIN_GRAPHICS_TYPE_RDP) {
>+return qemuRdpSetCredentials(vm, username, password, "");
>+}
>+

It's not immediately visible that defaultPasswd must not be NULL if the
graphics type is RDP.  The patch is semantically fine, but I worry about
the future, would you mind adding a simple check here so that we do not
end up calling g_variant_new() with NULL?


assert(password != NULL) ?



We prefer setting an VIR_ERR_INTERNAL_ERROR instead of aborting the
whole daemon.



>diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>index 30d6560d52..53572f7315 100644
>--- a/src/qemu/qemu_process.c
>+++ b/src/qemu/qemu_process.c
>@@ -5988,6 +6077,42 @@ qemuProcessPrepareHostNetwork(virDomainObj *vm)
> return 0;
> }
>
>+#include "qemu_rdp.h"
>+

Any reason why you cannot put the include at the top with the other ones?


oops, that's a left over, thanks!



>+static int
>+qemuPrepareGraphicsRdp(virQEMUDriver *driver,
>+   virDomainGraphicsDef *gfx)
>+{
>+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>+g_autoptr(qemuRdp) rdp = NULL;

No need for autoptr variable here, there is no place where this function
might fail with this variable set to non-NULL;


yeah, I suppose the code evolved. It doesn't hurt though. ok



It doesn't, true.



>+
>+if (!(rdp = qemuRdpNewForHelper(cfg->qemuRdpName)))
>+return -1;
>+
>+QEMU_DOMAIN_GRAPHICS_PRIVATE(gfx)->rdp = g_steal_pointer(&rdp);
>+
>+return 0;
>+}




signature.asc
Description: PGP signature

Re: [PATCH v2 00/21] Add qemu RDP server support

[Marc-André Lureau]

Hi

On Tue, Mar 11, 2025 at 5:28 PM Martin Kletzander  wrote:
> So I tried testing it and I found a couple of issues.  Those might be on
> my side, maybe caused by misunderstanding of it all.  First of all
>  fails with these patches for me even without

How does it fail? (it works on my end)

> .  That might be the source of all other issues,
> but since I was not sure whether to use p2p='yes' or
> address='unix:path=...' I tried both.  When address is specified qemu
> tries to connect to it, so that's not the right approach.  When I
> specify p2p='yes' then the VM starts.  If I add the RDP graphics I get a
> "confusing" error message:
>
> error: unsupported configuration: qemu-rdp support requires a D-Bus bus
> graphics device.
>
> which should probably say "qemu-rdp support requires a non-p2p dbus
> graphics device" instead since it is looking for a p2p='no' dbus
> graphics.

Not sure what is the best wording, but I understand it is confusing.
It requires a "D-Bus bus" to work. (we could make it work with p2p
too, eventually, but that might make multi-process handling more
complicated in the future - say if vhost-user-gpu exports the dbus
display interface etc)

>
> If I delay the checking of qemu-rdp running I find out that it fails
> right away.  It cannot connect to the unix socket, so I tried just
> running qemu VM with dbus graphics and qemu-rdp manually, but that
> failed on the missing certificate files.  That could be checked before
> blindly passing it down to the helper, too (even though I was running it
> manually this time it reminded me of that possibility).

Are you saying that libvirt should check the presence of certificates
before running qemu-rdp and provide an early error?
qemu rdp log is explicit (Error: reading server cert `...`). However,
I admit libvirt error report isn't great, as it fails with "The name
org.QemuDisplay.RDP was not provided by any .service files". I will
look into improving that.


> Ideally some handshake could confirm it is actually running.  If not a
> handshake, then maybe waiting whether it listens on where it is supposed
> to or some other indication of it being started while checking that the
> PID exists, so that it is not a dumb delay.
>
> Even running it manually I cannot verify it works, but that's probably a
> client-side issue?
>
> Server output when trying rdesktop:
> Waiting for org.qemu...
> Starting RDP server, args: ServerArgs { bind_address: 0.0.0.0:3389, cert: 
> None, key: None, remotefx: Disable }
> Cert: "/home/nert/.config/qemu-rdp/server-cert.pem", Key: 
> "/home/nert/.config/qemu-rdp/server-key.pem"
> 2025-03-11T13:21:00.350891Z ERROR ironrdp_server::server: Connection error 
> error=[no credentials while doing credssp] general error
> 2025-03-11T13:21:00.352401Z ERROR ironrdp_server::server: Connection error 
> error=accept_begin failed
> Caused by:
>  [failed to negotiate security protocol] general error
>
> sdl-freerdp client output (no error on the server side reported):
> [14:24:35:046] [449399:0006db7b] [ERROR][com.freerdp.core.transport] - 
> [transport_default_write]: BIO_should_retry returned a system error 32: 
> Broken pipe
>
> rdesktop client output:
> Failed to initialize NLA, do you have correct Kerberos TGT initialized ?
> Core(error): rcp_recv(), connection closed by peer
>

If you run it manually, make sure you set the credentials before
connecting the client, ex:
busctl --user call org.QemuDisplay.RDP /org/qemu_display/rdp
org.QemuDisplay.RDP SetCredentials sss user pass ''

> ===
>
> Anyway, I'm clearly doing something wrong, but there are some issues in
> the code as well and from the greater picture it should not be this
> complicated to test it out, should it?  I can test out other patches
> from you if you want, and I am open to hearing tips on what I'm doing
> wrong.


Thanks a lot for testing and the suggestions!

Re: [PATCH v2 03/22] hypervisor: expand available shutdown actions

[Peter Krempa]

On Wed, Mar 12, 2025 at 17:17:43 +, Daniel P. Berrangé wrote:
> The auto shutdown code can currently only perform managed save,
> which may fail in some cases, for example when PCI devices are
> assigned. On failure, shutdown inhibitors remain in place which
> may be undesirable.
> 
> This expands the logic to try a sequence of operations
> 
>  * Managed save
>  * Graceful shutdown
>  * Forced poweroff
> 
> Each of these operations can be enabled or disabled, but they
> are always applied in this order.
> 
> With the shutdown option, a configurable time is allowed for
> shutdown to complete, defaulting to 30 seconds, before moving
> onto the forced poweroff phase.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/hypervisor/domain_driver.c | 130 -
>  src/hypervisor/domain_driver.h |   7 ++
>  src/qemu/qemu_driver.c |   3 +
>  3 files changed, 121 insertions(+), 19 deletions(-)

[...]

> 
> @@ -732,31 +748,107 @@ 
> virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> 
> VIR_CONNECT_LIST_DOMAINS_ACTIVE)) < 0)
>  goto cleanup;
>  
> -flags = g_new0(unsigned int, numDomains);
> +VIR_DEBUG("Auto shutdown with %d running domains", numDomains);
> +if (cfg->trySave) {
> +g_autofree unsigned int *flags = g_new0(unsigned int, numDomains);
> +for (i = 0; i < numDomains; i++) {
> +int state;
> +/*
> + * Pause all VMs to make them stop dirtying pages,
> + * so save is quicker. We remember if any VMs were
> + * paused so we can restore that on resume.
> + */
> +flags[i] = VIR_DOMAIN_SAVE_RUNNING;
> +if (virDomainGetState(domains[i], &state, NULL, 0) == 0) {
> +if (state == VIR_DOMAIN_PAUSED)
> +flags[i] = VIR_DOMAIN_SAVE_PAUSED;
> +}
> +if (flags[i] & VIR_DOMAIN_SAVE_RUNNING)
> +virDomainSuspend(domains[i]);
> +}
> +
> +for (i = 0; i < numDomains; i++) {
> +if (virDomainManagedSave(domains[i], flags[i]) < 0) {
> +VIR_WARN("Unable to perform managed save of '%s': %s",
> + virDomainGetName(domains[i]),

virDomainGetName() calls virResetLastError()

> + virGetLastErrorMessage());

IIRC argument evaluation order is not defined this might have random
results.

> +if (flags[i] & VIR_DOMAIN_SAVE_RUNNING)
> +virDomainResume(domains[i]);
> +continue;
> +}
> +virObjectUnref(domains[i]);
> +domains[i] = NULL;
> +}
> +}
> +
> +if (cfg->tryShutdown) {
> +GTimer *timer = NULL;
> +for (i = 0; i < numDomains; i++) {
> +if (domains[i] == NULL)
> +continue;
> +if (virDomainShutdown(domains[i]) < 0) {
> +VIR_WARN("Unable to request graceful shutdown of '%s': %s",
> + virDomainGetName(domains[i]),
> + virGetLastErrorMessage());

Same as above

> +break;
> +}
> +}
> +
> +timer = g_timer_new();
> +while (1) {
> +bool anyRunning = false;
> +for (i = 0; i < numDomains; i++) {
> +if (!domains[i])
> +continue;
>  
> -/* First we pause all VMs to make them stop dirtying
> -   pages, etc. We remember if any VMs were paused so
> -   we can restore that on resume. */
> -for (i = 0; i < numDomains; i++) {
> -flags[i] = VIR_DOMAIN_SAVE_RUNNING;
> -if (virDomainGetState(domains[i], &state, NULL, 0) == 0) {
> -if (state == VIR_DOMAIN_PAUSED)
> -flags[i] = VIR_DOMAIN_SAVE_PAUSED;
> +if (virDomainIsActive(domains[i]) == 1) {
> +anyRunning = true;
> +} else {
> +virObjectUnref(domains[i]);
> +domains[i] = NULL;
> +}
> +}
> +
> +if (!anyRunning)
> +break;
> +if (g_timer_elapsed(timer, NULL) > cfg->waitShutdownSecs)
> +break;
> +g_usleep(1000*500);
>  }
> -virDomainSuspend(domains[i]);
> +g_timer_destroy(timer);
>  }
>  
> -/* Then we save the VMs to disk */
> -for (i = 0; i < numDomains; i++)
> -if (virDomainManagedSave(domains[i], flags[i]) < 0)
> -VIR_WARN("Unable to perform managed save of '%s': %s",
> - virDomainGetName(domains[i]),
> - virGetLastErrorMessage());

and here too

> +if (cfg->poweroff) {
> +for (i = 0; i < numDomains; i++) {
> +if (domains[i] == NULL)
> +continue;
> +/*
> + * NB might fail if we gave up o

Re: [PATCH v2 00/21] Add qemu RDP server support

[Marc-André Lureau]

Hi

On Thu, Mar 13, 2025 at 1:07 PM Martin Kletzander  wrote:
>
> On Thu, Mar 13, 2025 at 11:59:41AM +0400, Marc-André Lureau wrote:
> >Hi
> >
> >On Tue, Mar 11, 2025 at 5:28 PM Martin Kletzander  
> >wrote:
> >> So I tried testing it and I found a couple of issues.  Those might be on
> >> my side, maybe caused by misunderstanding of it all.  First of all
> >>  fails with these patches for me even without
> >
> >How does it fail? (it works on my end)
> >
>
> $ virsh dumpxml fedora39 --xpath //graphics
> 
> 
>
>
> 
>
> $ virsh version
> Compiled against library: libvirt 11.1.0
> Using library: libvirt 11.1.0
> Using API: QEMU 11.1.0
> Running hypervisor: QEMU 9.2.2
>
> $ virsh start fedora39
> Domain 'fedora39' started
>
> $ virsh destroy fedora39
> Domain 'fedora39' destroyed
>
> $ echo 'Start daemon with patches'
> Start daemon with patches
> $ virsh version
> Compiled against library: libvirt 11.1.0
> Using library: libvirt 11.1.0
> Using API: QEMU 11.1.0
> Running hypervisor: QEMU 9.2.2
>
> $ # Same version, but this time with the patch series applied
> $ virsh dumpxml fedora39 --xpath //graphics
> 
> 
>
>
> 
>
> $ virsh start fedora39
> error: Failed to start domain 'fedora39'
> error: operation failed: Failed to connect to dbus-daemon: The connection is 
> closed

Hmm, I don't get this issue. libvirt fails to connect to the just
launched bus (or got closed). This is weird, because we checked
earlier the socket exists.

Could you check dbus log? (for some reason it's not in cfg.logDir, but
in cfg.dbusStateDir - I wonder if we can change that now)

thanks

Re: [PATCH v2 04/22] hypervisor: custom shutdown actions for transient vs persistent VMs

[Peter Krempa]

On Wed, Mar 12, 2025 at 17:17:44 +, Daniel P. Berrangé wrote:
> It may be desirable to treat transient VMs differently from persistent
> VMs. For example, while performing managed save on persistent VMs makes
> sense, the same not usually true of transient VMs, since by their
> nature they will have no config to restore from.
> 
> This also lets us fix a long standing problem with incorrectly
> attempting to perform managed save on transient VMs.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/hypervisor/domain_driver.c | 62 +++---
>  src/hypervisor/domain_driver.h | 18 --
>  src/libvirt_private.syms   |  2 ++
>  src/qemu/qemu_driver.c |  6 ++--
>  4 files changed, 77 insertions(+), 11 deletions(-)
> 
> diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
> index e625726c07..c510e1d2ae 100644
> --- a/src/hypervisor/domain_driver.c
> +++ b/src/hypervisor/domain_driver.c
> @@ -35,6 +35,13 @@
>  
>  VIR_LOG_INIT("hypervisor.domain_driver");
>  
> +VIR_ENUM_IMPL(virDomainDriverAutoShutdownScope,
> +  VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_LAST,
> +  "none",
> +  "persistent",
> +  "transient",
> +  "all");
> +
>  char *
>  virDomainDriverGenerateRootHash(const char *drivername,
>  const char *root)
> @@ -721,9 +728,13 @@ 
> virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
>  int numDomains = 0;
>  size_t i;
>  virDomainPtr *domains = NULL;
> +g_autofree bool *transient = NULL;
>  
> -VIR_DEBUG("Run autoshutdown uri=%s trySave=%d tryShutdown=%d poweroff=%d 
> waitShutdownSecs=%u",
> -  cfg->uri, cfg->trySave, cfg->tryShutdown, cfg->poweroff,
> +VIR_DEBUG("Run autoshutdown uri=%s trySave=%s tryShutdown=%s poweroff=%s 
> waitShutdownSecs=%u",
> +  cfg->uri,
> +  virDomainDriverAutoShutdownScopeTypeToString(cfg->trySave),
> +  virDomainDriverAutoShutdownScopeTypeToString(cfg->tryShutdown),
> +  virDomainDriverAutoShutdownScopeTypeToString(cfg->poweroff),
>cfg->waitShutdownSecs);
>  
>  /*
> @@ -740,6 +751,21 @@ 
> virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
>  if (cfg->waitShutdownSecs <= 0)
>  cfg->waitShutdownSecs = 30;
>  
> +/* Short-circuit if all actions are disabled */
> +if (cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE &&
> +cfg->tryShutdown == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE &&
> +cfg->poweroff == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE)
> +return;
> +
> +if (cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL ||
> +cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_TRANSIENT) {
> +/* virDomainManagedSave will return an error. We'll let
> + * the code run, since it'll just fall through to the next
> + * actions, but give a clear warning upfront */
> +VIR_WARN("Managed save not supported for transient VMs");
> +return;

This looks like better suited to the config parser. Reporting a
programming error as warning seems wrong.

This code should IMO at ignore invalid setting for managed-save config
rather than not do anything at all.

> +}
> +
>  if (!(conn = virConnectOpen(cfg->uri)))
>  goto cleanup;

Reviewed-by: Peter Krempa 

Re: [PATCH v2 04/22] hypervisor: custom shutdown actions for transient vs persistent VMs

[Daniel P . Berrangé]

On Thu, Mar 13, 2025 at 12:18:28PM +0100, Peter Krempa wrote:
> On Wed, Mar 12, 2025 at 17:17:44 +, Daniel P. Berrangé wrote:
> > It may be desirable to treat transient VMs differently from persistent
> > VMs. For example, while performing managed save on persistent VMs makes
> > sense, the same not usually true of transient VMs, since by their
> > nature they will have no config to restore from.
> > 
> > This also lets us fix a long standing problem with incorrectly
> > attempting to perform managed save on transient VMs.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/hypervisor/domain_driver.c | 62 +++---
> >  src/hypervisor/domain_driver.h | 18 --
> >  src/libvirt_private.syms   |  2 ++
> >  src/qemu/qemu_driver.c |  6 ++--
> >  4 files changed, 77 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
> > index e625726c07..c510e1d2ae 100644
> > --- a/src/hypervisor/domain_driver.c
> > +++ b/src/hypervisor/domain_driver.c
> > @@ -35,6 +35,13 @@
> >  
> >  VIR_LOG_INIT("hypervisor.domain_driver");
> >  
> > +VIR_ENUM_IMPL(virDomainDriverAutoShutdownScope,
> > +  VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_LAST,
> > +  "none",
> > +  "persistent",
> > +  "transient",
> > +  "all");
> > +
> >  char *
> >  virDomainDriverGenerateRootHash(const char *drivername,
> >  const char *root)
> > @@ -721,9 +728,13 @@ 
> > virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> >  int numDomains = 0;
> >  size_t i;
> >  virDomainPtr *domains = NULL;
> > +g_autofree bool *transient = NULL;
> >  
> > -VIR_DEBUG("Run autoshutdown uri=%s trySave=%d tryShutdown=%d 
> > poweroff=%d waitShutdownSecs=%u",
> > -  cfg->uri, cfg->trySave, cfg->tryShutdown, cfg->poweroff,
> > +VIR_DEBUG("Run autoshutdown uri=%s trySave=%s tryShutdown=%s 
> > poweroff=%s waitShutdownSecs=%u",
> > +  cfg->uri,
> > +  virDomainDriverAutoShutdownScopeTypeToString(cfg->trySave),
> > +  
> > virDomainDriverAutoShutdownScopeTypeToString(cfg->tryShutdown),
> > +  virDomainDriverAutoShutdownScopeTypeToString(cfg->poweroff),
> >cfg->waitShutdownSecs);
> >  
> >  /*
> > @@ -740,6 +751,21 @@ 
> > virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> >  if (cfg->waitShutdownSecs <= 0)
> >  cfg->waitShutdownSecs = 30;
> >  
> > +/* Short-circuit if all actions are disabled */
> > +if (cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE &&
> > +cfg->tryShutdown == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE &&
> > +cfg->poweroff == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE)
> > +return;
> > +
> > +if (cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL ||
> > +cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_TRANSIENT) {
> > +/* virDomainManagedSave will return an error. We'll let
> > + * the code run, since it'll just fall through to the next
> > + * actions, but give a clear warning upfront */
> > +VIR_WARN("Managed save not supported for transient VMs");
> > +return;
> 
> This looks like better suited to the config parser. Reporting a
> programming error as warning seems wrong.
> 
> This code should IMO at ignore invalid setting for managed-save config
> rather than not do anything at all.

The qemu_conf.c file will validate that trySave != all|transient
too, and raise a fatal error there. So this branch should not
be triggered here - its more of a safety net in case we forget
the same checks when extending it to other drivers.

You are right though, we could just force trySave == persistent
in this case, so at least everything else works.

> 
> > +}
> > +
> >  if (!(conn = virConnectOpen(cfg->uri)))
> >  goto cleanup;
> 
> Reviewed-by: Peter Krempa 
> 

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|