Re: [libvirt PATCH v4 0/4] Ask qemu about migration blockers

[Eugenio Perez Martin]

On Thu, Jul 21, 2022 at 7:02 AM Laine Stump  wrote:
>
> This all looks good except a couple small nits that I've noted in
> separate messages for the individual patches.
>
> I've already made these minor changes locally and pushed them here:
>
>https://gitlab.com/lainestump/libvirt/-/commits/REVIEW
>
> If it all looks okay to you, I'll push it as soon as I get up in the AM.
> If you don't like the changes I've made to the commit log messages, feel
> free to revise and re-send them :-)
>

All changes look good to me :). Not an english native, and not used to
libvirt codebase so I missed a few of these coding styles.

Thanks!

> This is a pretty good turn around time for your first patch in libvirt.
> Thanks for the contribution!
>
> On 7/20/22 12:05 PM, Eugenio Pérez wrote:
> > There are some hardcoded migration blockers in libvirt, like having a net
> > vhost-vdpa device. While it's true that they cannot be migrated at the 
> > moment,
> > there are plans to be able to migrate them soon
> >
> > They'll put a migration blockers in the cases where you cannot migrate them.
> > Ask qemu about then before rejecting the migration. In case the question is 
> > not
> > possible, assume they're not migratable.
> >
> > v4:
> > * Do not override qemuDomainGetMigrationBlockers error calling again
> >virReportError.
> > * Replace ", " with "; " in blockers separators.
> >
> > v3:
> > * Return ok in qemuMonitorJSONGetMigrationBlockers is there are no
> >blockers.
> > * Fix indentation
> > * Report all blockers instead of only the first.
> > * Squash some patches
> > * Move note to function doc.
> > * s/QEMU_MIGRATION_BLOCKED_REASONS/QEMU_CAPS_MIGRATION_BLOCKED_REASONS/
> >
> > v2:
> > * Ask qemu if it has some pending blockers before try the migration
> >
> > Eugenio Pérez (3):
> >qemu_monitor: add support for get qemu migration blockers
> >qemu_migration: get migration blockers before hardcoded checks
> >qemu_migration: Do not forbid vDPA devices if can query blockers
> >
> > Jonathon Jongsma (1):
> >qemu: introduce capability QEMU_CAPS_MIGRATION_BLOCKED_REASONS
> >
> >   src/qemu/qemu_capabilities.c  |  2 +
> >   src/qemu/qemu_capabilities.h  |  1 +
> >   src/qemu/qemu_migration.c | 34 +-
> >   src/qemu/qemu_monitor.c   | 11 +
> >   src/qemu/qemu_monitor.h   |  4 ++
> >   src/qemu/qemu_monitor_json.c  | 44 +++
> >   src/qemu/qemu_monitor_json.h  |  3 ++
> >   .../caps_6.0.0.aarch64.xml|  1 +
> >   .../qemucapabilitiesdata/caps_6.0.0.s390x.xml |  1 +
> >   .../caps_6.0.0.x86_64.xml |  1 +
> >   .../caps_6.1.0.x86_64.xml |  1 +
> >   .../caps_6.2.0.aarch64.xml|  1 +
> >   .../qemucapabilitiesdata/caps_6.2.0.ppc64.xml |  1 +
> >   .../caps_6.2.0.x86_64.xml |  1 +
> >   .../caps_7.0.0.aarch64.xml|  1 +
> >   .../qemucapabilitiesdata/caps_7.0.0.ppc64.xml |  1 +
> >   .../caps_7.0.0.x86_64.xml |  1 +
> >   .../caps_7.1.0.x86_64.xml |  1 +
> >   18 files changed, 109 insertions(+), 1 deletion(-)
> >
>

Re: [libvirt PATCH v4 2/4] qemu_monitor: add support for get qemu migration blockers

[Laine Stump]

On 7/20/22 12:05 PM, Eugenio Pérez wrote:

since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
will return an array of error strings describing the migration blockers.
This can be used to check whether there are any devices blocking
migration, etc.

Enable qemu monitor to send this query.  This will allow
qemuMigrationSrcIsAllowed to dynamically ask for migration blockers,
reducing duplication.


I reworded this commit log a bit - see the branch I linked in my 
response to the cover letter and let me know if you approve.




Signed-off-by: Eugenio Pérez 
Reviewed-by: Jiri Denemark 
---
v4:
* Separate return type into its own line

v3:
* Squash some patches
* Return ok in qemuMonitorJSONGetMigrationBlockers is there are no
   blockers.
* Move note to function doc.
---
  src/qemu/qemu_monitor.c  | 11 +
  src/qemu/qemu_monitor.h  |  4 
  src/qemu/qemu_monitor_json.c | 44 
  src/qemu/qemu_monitor_json.h |  3 +++
  4 files changed, 62 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 109107eaae..e0939beecd 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4486,3 +4486,14 @@ qemuMonitorMigrateRecover(qemuMonitor *mon,
  
  return qemuMonitorJSONMigrateRecover(mon, uri);

  }
+
+int
+qemuMonitorGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers)


We try to keep 2 blank lines between each function. I added in the extra 
blank lines before and after this function.



+{
+VIR_DEBUG("blockers=%p", blockers);
+
+QEMU_CHECK_MONITOR(mon);
+
+return qemuMonitorJSONGetMigrationBlockers(mon, blockers);
+}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index cc1a0bc8c9..b82f198285 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1543,3 +1543,7 @@ qemuMonitorChangeMemoryRequestedSize(qemuMonitor *mon,
  int
  qemuMonitorMigrateRecover(qemuMonitor *mon,
const char *uri);
+
+int
+qemuMonitorGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 5e4a86e5ad..6b26dfcb54 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3338,6 +3338,50 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
  return 0;
  }
  
+/*

+ * Get the exposed migration blockers.
+ *
+ * This function assume qemu has the capability of request them.
+ *
+ * It returns a NULL terminated array on blockers if there are any, or it set
+ * it to NULL otherwise.
+ */
+int
+qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+virJSONValue *data;
+virJSONValue *jblockers;
+size_t i;
+
+*blockers = NULL;
+if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL)))
+return -1;
+
+if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
+return -1;
+
+if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
+return -1;
+
+data = virJSONValueObjectGetObject(reply, "return");
+
+if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons")))
+return 0;
+
+*blockers = g_new0(char *, virJSONValueArraySize(jblockers) + 1);
+for (i = 0; i < virJSONValueArraySize(jblockers); i++) {
+virJSONValue *jblocker = virJSONValueArrayGet(jblockers, i);
+const char *blocker = virJSONValueGetString(jblocker);
+
+(*blockers)[i] = g_strdup(blocker);
+}
+
+return 0;
+}
+
  int qemuMonitorJSONMigrateCancel(qemuMonitor *mon)
  {
  g_autoptr(virJSONValue) cmd = 
qemuMonitorJSONMakeCommand("migrate_cancel", NULL);
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 2759566892..e4c65e250e 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -184,6 +184,9 @@ qemuMonitorJSONMigrate(qemuMonitor *mon,
 unsigned int flags,
 const char *uri);
  int
+qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers);
+int
  qemuMonitorJSONGetSpiceMigrationStatus(qemuMonitor *mon,
 bool *spice_migrated);
  

Re: [libvirt PATCH v4 4/4] qemu_migration: Do not forbid vDPA devices if can query blockers

[Laine Stump]

On 7/20/22 12:05 PM, Eugenio Pérez wrote:

vDPA devices will be migratable soon. Since they are not migratable
before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
for migration blockers, let it hardcoded in that case.

Otherwise, ask qemu about the explicit blocker.


I reworded the commit log message, but otherwise this is all fine.



Signed-off-by: Eugenio Pérez 
Reviewed-by: Jiri Denemark 
---
v3: Fix indentation
---
  src/qemu/qemu_migration.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 2e3044289a..b554027da2 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
  int pauseReason;
  size_t i;
  int r;
+bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
+
QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
  
  /* Ask qemu if it have a migration blocker */

-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
+if (blockedReasonsCap) {
  g_auto(GStrv) blockers = NULL;
  r = qemuDomainGetMigrationBlockers(driver, vm, );
  if (r != 0)
@@ -1576,7 +1578,7 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
  virDomainNetDef *net = vm->def->nets[i];
  qemuSlirp *slirp;
  
-if (net->type == VIR_DOMAIN_NET_TYPE_VDPA) {

+if (!blockedReasonsCap && net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
 _("vDPA devices cannot be migrated"));
  return false;

Re: [libvirt PATCH v4 3/4] qemu_migration: get migration blockers before hardcoded checks

[Laine Stump]

On 7/20/22 12:05 PM, Eugenio Pérez wrote:

since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
will return an array of error strings describing the migration blockers.
This can be used to check whether there are any devices blocking
migration, etc.

Enable qemuMigrationSrcIsAllowed to query it.


I reworded the commit log message a bit. Rather than repeating it here, 
I'll just point you to the branch I pushed to gitlab (link in my 
response to the cover letter).


Signed-off-by: Eugenio Pérez 
Reviewed-by: Jiri Denemark 
---
v4:
* Do not override qemuDomainGetMigrationBlockers error calling again
   virReportError.
* Replace ", " with "; " in blockers separators.

v3:
* Report message with a colon.
* Report all blockers instead of only the first.
---
  src/qemu/qemu_migration.c | 30 ++
  1 file changed, 30 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index b12cb518ee..2e3044289a 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef *def)
  return true;
  }
  
+static int

+qemuDomainGetMigrationBlockers(virQEMUDriver *driver,
+   virDomainObj *vm,
+   char ***blockers)
+{
+qemuDomainObjPrivate *priv = vm->privateData;
+int rc;
+
+qemuDomainObjEnterMonitor(driver, vm);
+rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers);
+qemuDomainObjExitMonitor(vm);
+
+return rc;
+}
  


Note that we've been trying to keep 2 blank lines between each function, 
but here you've added a new function in between those 2 blank lines, so 
there's only a single blank before and after. I added in the extra blanks.



  /**
   * qemuMigrationSrcIsAllowed:
@@ -1439,6 +1453,22 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
  int nsnapshots;
  int pauseReason;
  size_t i;
+int r;
+
+/* Ask qemu if it have a migration blocker */


   "has a migration blocker"


+if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
+g_auto(GStrv) blockers = NULL;
+r = qemuDomainGetMigrationBlockers(driver, vm, );
+if (r != 0)
+return false;


in libvirt we usually return -1 on failure, and check with "if (r < 0)". 
And since that is the only use of "r", we prefer to not have the extra 
stack variable cluttering things up, so:


 if (qemuDomainGetMigrationBlockers(driver, vm, ) < 0)
 return false;


+
+if (blockers && blockers[0]) {
+g_autofree char *reasons = g_strjoinv("; ", blockers);
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("cannot migrate domain: %s"), reasons);
+return false;
+}
+}
  
  /* perform these checks only when migrating to remote hosts */

  if (remote) {

Re: [libvirt PATCH v4 0/4] Ask qemu about migration blockers

[Laine Stump]

This all looks good except a couple small nits that I've noted in 
separate messages for the individual patches.


I've already made these minor changes locally and pushed them here:

  https://gitlab.com/lainestump/libvirt/-/commits/REVIEW

If it all looks okay to you, I'll push it as soon as I get up in the AM. 
If you don't like the changes I've made to the commit log messages, feel 
free to revise and re-send them :-)


This is a pretty good turn around time for your first patch in libvirt. 
Thanks for the contribution!


On 7/20/22 12:05 PM, Eugenio Pérez wrote:

There are some hardcoded migration blockers in libvirt, like having a net
vhost-vdpa device. While it's true that they cannot be migrated at the moment,
there are plans to be able to migrate them soon

They'll put a migration blockers in the cases where you cannot migrate them.
Ask qemu about then before rejecting the migration. In case the question is not
possible, assume they're not migratable.

v4:
* Do not override qemuDomainGetMigrationBlockers error calling again
   virReportError.
* Replace ", " with "; " in blockers separators.

v3:
* Return ok in qemuMonitorJSONGetMigrationBlockers is there are no
   blockers.
* Fix indentation
* Report all blockers instead of only the first.
* Squash some patches
* Move note to function doc.
* s/QEMU_MIGRATION_BLOCKED_REASONS/QEMU_CAPS_MIGRATION_BLOCKED_REASONS/

v2:
* Ask qemu if it has some pending blockers before try the migration

Eugenio Pérez (3):
   qemu_monitor: add support for get qemu migration blockers
   qemu_migration: get migration blockers before hardcoded checks
   qemu_migration: Do not forbid vDPA devices if can query blockers

Jonathon Jongsma (1):
   qemu: introduce capability QEMU_CAPS_MIGRATION_BLOCKED_REASONS

  src/qemu/qemu_capabilities.c  |  2 +
  src/qemu/qemu_capabilities.h  |  1 +
  src/qemu/qemu_migration.c | 34 +-
  src/qemu/qemu_monitor.c   | 11 +
  src/qemu/qemu_monitor.h   |  4 ++
  src/qemu/qemu_monitor_json.c  | 44 +++
  src/qemu/qemu_monitor_json.h  |  3 ++
  .../caps_6.0.0.aarch64.xml|  1 +
  .../qemucapabilitiesdata/caps_6.0.0.s390x.xml |  1 +
  .../caps_6.0.0.x86_64.xml |  1 +
  .../caps_6.1.0.x86_64.xml |  1 +
  .../caps_6.2.0.aarch64.xml|  1 +
  .../qemucapabilitiesdata/caps_6.2.0.ppc64.xml |  1 +
  .../caps_6.2.0.x86_64.xml |  1 +
  .../caps_7.0.0.aarch64.xml|  1 +
  .../qemucapabilitiesdata/caps_7.0.0.ppc64.xml |  1 +
  .../caps_7.0.0.x86_64.xml |  1 +
  .../caps_7.1.0.x86_64.xml |  1 +
  18 files changed, 109 insertions(+), 1 deletion(-)

RE: [libvirt][PATCH v13 1/6] Define SGX capabilities structs

[Huang, Haibin]

> -Original Message-
> From: Michal Prívozník 
> Sent: Wednesday, July 20, 2022 7:27 PM
> To: Yang, Lin A ; libvir-list@redhat.com; Huang, Haibin
> ; Ding, Jian-feng 
> Subject: Re: [libvirt][PATCH v13 1/6] Define SGX capabilities structs
> 
> On 7/1/22 21:14, Lin Yang wrote:
> > From: Haibin Huang 
> >
> > Signed-off-by: Michal Privoznik 
> > Signed-off-by: Haibin Huang 
> > ---
> >  src/conf/domain_capabilities.c | 10 ++
> > src/conf/domain_capabilities.h | 24 
> >  src/libvirt_private.syms   |  1 +
> >  3 files changed, 35 insertions(+)
> >
> > diff --git a/src/conf/domain_capabilities.c
> > b/src/conf/domain_capabilities.c index 895e8d00e8..27f3fb8d36 100644
> > --- a/src/conf/domain_capabilities.c
> > +++ b/src/conf/domain_capabilities.c
> > @@ -76,6 +76,16 @@ virSEVCapabilitiesFree(virSEVCapability *cap)  }
> >
> >
> > +void
> > +virSGXCapabilitiesFree(virSGXCapability *cap) {
> > +if (!cap)
> > +return;
> > +
> 
> This leaks cap->pSections.
[Haibin] OK
> 
> > +g_free(cap);
> > +}
> > +
> > +
> >  static void
> >  virDomainCapsDispose(void *obj)
> >  {
> > diff --git a/src/conf/domain_capabilities.h
> > b/src/conf/domain_capabilities.h index f2eed80b15..dac1098e98 100644
> > --- a/src/conf/domain_capabilities.h
> > +++ b/src/conf/domain_capabilities.h
> > @@ -192,6 +192,24 @@ struct _virSEVCapability {
> >  unsigned int max_es_guests;
> >  };
> >
> > +typedef struct _virSection virSection; typedef virSection
> > +*virSectionPtr;
> 
> No, if we can help it I'd rather avoid introducing virXXXPtr typedef.
> We've worked hard to get rid of them and I don't quite see a reason to
> reintroduce them.
> 
> > +struct _virSection {
> > +unsigned long long size;
> > +unsigned int node;
> 
> While it's true that QEMU with its current code does not ever report a
> negative number for 'node', at the QAPI level it's declared as signed integer
> and thus we ought to reflect that.
> 
[Haibin] OK
> > +};
> > +
> > +typedef struct _virSGXCapability virSGXCapability; typedef
> > +virSGXCapability *virSGXCapabilityPtr; struct _virSGXCapability {
> > +bool flc;
> > +bool sgx1;
> > +bool sgx2;
> > +unsigned long long section_size;
> > +size_t nSections;
> > +virSectionPtr pSections;
> 
> We tend to use: 'nitems' and 'items', or 'nsections' and 'sections' in cases 
> like
> this.
[Haibin] OK
> 
> > +};
> > +
> 
> Michal

RE: [libvirt][PATCH v13 0/6] Support query and use SGX

[Yang, Lin A]

> This version is a bit better than the previous one. But we're at version
> 13 and I am still unable to even start a guest. Please, make sure that this
> basic functionality works in v14, because this is plain waste of precious
> review bandwidth.
> 
> Anyway, as usual, I've uploaded my suggested fixes here:
> 
> https://gitlab.com/MichalPrivoznik/libvirt/-/commits/sgx/

Sorry to hear it didn't work in your environment. We definitely tested it
several times and it works well for both QEMU 6.2.0 and 7.0.0.

Let me try to reproduce it with the domain xml you shared before.

By my best guess, if you see "qemu-system-x86_64:***: 
invalid object type: memory-backend-epc" error, it means QEMU didn't
get enough permission to launch SGX VM.

Pls add /dev/sgx_vepc, /dev/sgx_enclave and /dev/sgx_provision to the 
"cgroup_device_acl" list in /etc/libvirt/qemu.conf. QEMU requires those access
to assign EPC, but it was denied  by libvirt’s cgroup controllers by default.

cgroup_device_acl = [  
"/dev/null", "/dev/full", "/dev/zero",  
...
"/dev/sgx_vepc", 
"/dev/sgx_enclave”, 
"/dev/sgx_provision” 
] 

Also in /etc/libvirt/qemu.conf, set the runtime user to uid 0,  since QEMU 
needs to
read and write to those sgx devices, like /dev/sgx_vepc. Unfortunately, it is 
owned
by root with file mode 600, so QEMU has to launch as root. 

user = "+0"

It would be really helpful if you can use these steps to see whether it resolve 
the issue. I will add a doc somewhere to include all steps are required for use 
to
use sgx in libvirt.

Thanks,
Lin.

[libvirt PATCH v4 3/4] qemu_migration: get migration blockers before hardcoded checks

[Eugenio Pérez]

since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
will return an array of error strings describing the migration blockers.
This can be used to check whether there are any devices blocking
migration, etc.

Enable qemuMigrationSrcIsAllowed to query it.

Signed-off-by: Eugenio Pérez 
Reviewed-by: Jiri Denemark 
---
v4:
* Do not override qemuDomainGetMigrationBlockers error calling again
  virReportError.
* Replace ", " with "; " in blockers separators.

v3:
* Report message with a colon.
* Report all blockers instead of only the first.
---
 src/qemu/qemu_migration.c | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index b12cb518ee..2e3044289a 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef *def)
 return true;
 }
 
+static int
+qemuDomainGetMigrationBlockers(virQEMUDriver *driver,
+   virDomainObj *vm,
+   char ***blockers)
+{
+qemuDomainObjPrivate *priv = vm->privateData;
+int rc;
+
+qemuDomainObjEnterMonitor(driver, vm);
+rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers);
+qemuDomainObjExitMonitor(vm);
+
+return rc;
+}
 
 /**
  * qemuMigrationSrcIsAllowed:
@@ -1439,6 +1453,22 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
 int nsnapshots;
 int pauseReason;
 size_t i;
+int r;
+
+/* Ask qemu if it have a migration blocker */
+if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
+g_auto(GStrv) blockers = NULL;
+r = qemuDomainGetMigrationBlockers(driver, vm, );
+if (r != 0)
+return false;
+
+if (blockers && blockers[0]) {
+g_autofree char *reasons = g_strjoinv("; ", blockers);
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("cannot migrate domain: %s"), reasons);
+return false;
+}
+}
 
 /* perform these checks only when migrating to remote hosts */
 if (remote) {
-- 
2.31.1

[libvirt PATCH v4 1/4] qemu: introduce capability QEMU_CAPS_MIGRATION_BLOCKED_REASONS

[Eugenio Pérez]

From: Jonathon Jongsma 

since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
will return an array of error strings describing the migration blockers.
This can be used to check whether there are any devices blocking
migration, etc.

Signed-off-by: Jonathon Jongsma 
Signed-off-by: Eugenio Pérez 
Reviewed-by: Jiri Denemark 
---
v3: s/QEMU_MIGRATION_BLOCKED_REASONS/QEMU_CAPS_MIGRATION_BLOCKED_REASONS/
---
 src/qemu/qemu_capabilities.c  | 2 ++
 src/qemu/qemu_capabilities.h  | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml   | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_7.0.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_7.0.0.ppc64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml  | 1 +
 13 files changed, 14 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 30b396d32d..b002fb98ed 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -672,6 +672,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */
   "iothread.thread-pool-max", /* 
QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX */
   "usb-host.guest-resets-all", /* 
QEMU_CAPS_USB_HOST_GUESTS_RESETS_ALL */
+  "migration.blocked-reasons", /* 
QEMU_CAPS_MIGRATION_BLOCKED_REASONS */
 );
 
 
@@ -1622,6 +1623,7 @@ static struct virQEMUCapsStringFlags 
virQEMUCapsQMPSchemaQueries[] = {
 { "chardev-add/arg-type/backend/+qemu-vdagent", 
QEMU_CAPS_CHARDEV_QEMU_VDAGENT },
 { "query-display-options/ret-type/+dbus", QEMU_CAPS_DISPLAY_DBUS },
 { "object-add/arg-type/+iothread/thread-pool-max", 
QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX },
+{ "query-migrate/ret-type/blocked-reasons", 
QEMU_CAPS_MIGRATION_BLOCKED_REASONS },
 };
 
 typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index d979a5ba3b..8f3090e2ce 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -651,6 +651,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_DISPLAY_DBUS, /* -display dbus */
 QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX, /* -object iothread.thread-pool-max */
 QEMU_CAPS_USB_HOST_GUESTS_RESETS_ALL, /* -device usb-host.guest-resets-all 
*/
+QEMU_CAPS_MIGRATION_BLOCKED_REASONS, /* query-migrate returns 
'blocked-reasons */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml
index 01e30f4e02..4afd7b26ce 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml
@@ -187,6 +187,7 @@
   
   
   
+  
   600
   0
   61700242
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
index aa7b5deab5..c9cb85daa0 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
@@ -144,6 +144,7 @@
   
   
   
+  
   600
   0
   39100242
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
index d9e385ab1d..508804521c 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
@@ -229,6 +229,7 @@
   
   
   
+  
   600
   0
   43100242
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index 05f297dfa2..d4a540fafd 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -234,6 +234,7 @@
   
   
   
+  
   6001000
   0
   43100243
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml
index 9cb1a32354..71697fac95 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml
@@ -199,6 +199,7 @@
   
   
   
+  
   6001050
   0
   61700244
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml
index 5df148d787..3f86e03f18 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml
@@ -193,6 +193,7 @@
   
   
   
+  
   6002000
   0
   42900244
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
index 

[libvirt PATCH v4 0/4] Ask qemu about migration blockers

[Eugenio Pérez]

There are some hardcoded migration blockers in libvirt, like having a net
vhost-vdpa device. While it's true that they cannot be migrated at the moment,
there are plans to be able to migrate them soon.

They'll put a migration blockers in the cases where you cannot migrate them.
Ask qemu about then before rejecting the migration. In case the question is not
possible, assume they're not migratable.

v4:
* Do not override qemuDomainGetMigrationBlockers error calling again
  virReportError.
* Replace ", " with "; " in blockers separators.

v3:
* Return ok in qemuMonitorJSONGetMigrationBlockers is there are no
  blockers.
* Fix indentation
* Report all blockers instead of only the first.
* Squash some patches
* Move note to function doc.
* s/QEMU_MIGRATION_BLOCKED_REASONS/QEMU_CAPS_MIGRATION_BLOCKED_REASONS/

v2:
* Ask qemu if it has some pending blockers before try the migration

Eugenio Pérez (3):
  qemu_monitor: add support for get qemu migration blockers
  qemu_migration: get migration blockers before hardcoded checks
  qemu_migration: Do not forbid vDPA devices if can query blockers

Jonathon Jongsma (1):
  qemu: introduce capability QEMU_CAPS_MIGRATION_BLOCKED_REASONS

 src/qemu/qemu_capabilities.c  |  2 +
 src/qemu/qemu_capabilities.h  |  1 +
 src/qemu/qemu_migration.c | 34 +-
 src/qemu/qemu_monitor.c   | 11 +
 src/qemu/qemu_monitor.h   |  4 ++
 src/qemu/qemu_monitor_json.c  | 44 +++
 src/qemu/qemu_monitor_json.h  |  3 ++
 .../caps_6.0.0.aarch64.xml|  1 +
 .../qemucapabilitiesdata/caps_6.0.0.s390x.xml |  1 +
 .../caps_6.0.0.x86_64.xml |  1 +
 .../caps_6.1.0.x86_64.xml |  1 +
 .../caps_6.2.0.aarch64.xml|  1 +
 .../qemucapabilitiesdata/caps_6.2.0.ppc64.xml |  1 +
 .../caps_6.2.0.x86_64.xml |  1 +
 .../caps_7.0.0.aarch64.xml|  1 +
 .../qemucapabilitiesdata/caps_7.0.0.ppc64.xml |  1 +
 .../caps_7.0.0.x86_64.xml |  1 +
 .../caps_7.1.0.x86_64.xml |  1 +
 18 files changed, 109 insertions(+), 1 deletion(-)

-- 
2.31.1

[libvirt PATCH v4 4/4] qemu_migration: Do not forbid vDPA devices if can query blockers

[Eugenio Pérez]

vDPA devices will be migratable soon. Since they are not migratable
before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
for migration blockers, let it hardcoded in that case.

Otherwise, ask qemu about the explicit blocker.

Signed-off-by: Eugenio Pérez 
Reviewed-by: Jiri Denemark 
---
v3: Fix indentation
---
 src/qemu/qemu_migration.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 2e3044289a..b554027da2 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
 int pauseReason;
 size_t i;
 int r;
+bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
+
QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
 
 /* Ask qemu if it have a migration blocker */
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
+if (blockedReasonsCap) {
 g_auto(GStrv) blockers = NULL;
 r = qemuDomainGetMigrationBlockers(driver, vm, );
 if (r != 0)
@@ -1576,7 +1578,7 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
 virDomainNetDef *net = vm->def->nets[i];
 qemuSlirp *slirp;
 
-if (net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
+if (!blockedReasonsCap && net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("vDPA devices cannot be migrated"));
 return false;
-- 
2.31.1

[libvirt PATCH v4 2/4] qemu_monitor: add support for get qemu migration blockers

[Eugenio Pérez]

since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
will return an array of error strings describing the migration blockers.
This can be used to check whether there are any devices blocking
migration, etc.

Enable qemu monitor to send this query.  This will allow
qemuMigrationSrcIsAllowed to dynamically ask for migration blockers,
reducing duplication.

Signed-off-by: Eugenio Pérez 
Reviewed-by: Jiri Denemark 
---
v4:
* Separate return type into its own line

v3:
* Squash some patches
* Return ok in qemuMonitorJSONGetMigrationBlockers is there are no
  blockers.
* Move note to function doc.
---
 src/qemu/qemu_monitor.c  | 11 +
 src/qemu/qemu_monitor.h  |  4 
 src/qemu/qemu_monitor_json.c | 44 
 src/qemu/qemu_monitor_json.h |  3 +++
 4 files changed, 62 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 109107eaae..e0939beecd 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4486,3 +4486,14 @@ qemuMonitorMigrateRecover(qemuMonitor *mon,
 
 return qemuMonitorJSONMigrateRecover(mon, uri);
 }
+
+int
+qemuMonitorGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers)
+{
+VIR_DEBUG("blockers=%p", blockers);
+
+QEMU_CHECK_MONITOR(mon);
+
+return qemuMonitorJSONGetMigrationBlockers(mon, blockers);
+}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index cc1a0bc8c9..b82f198285 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1543,3 +1543,7 @@ qemuMonitorChangeMemoryRequestedSize(qemuMonitor *mon,
 int
 qemuMonitorMigrateRecover(qemuMonitor *mon,
   const char *uri);
+
+int
+qemuMonitorGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 5e4a86e5ad..6b26dfcb54 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3338,6 +3338,50 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
 return 0;
 }
 
+/*
+ * Get the exposed migration blockers.
+ *
+ * This function assume qemu has the capability of request them.
+ *
+ * It returns a NULL terminated array on blockers if there are any, or it set
+ * it to NULL otherwise.
+ */
+int
+qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+virJSONValue *data;
+virJSONValue *jblockers;
+size_t i;
+
+*blockers = NULL;
+if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL)))
+return -1;
+
+if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
+return -1;
+
+if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
+return -1;
+
+data = virJSONValueObjectGetObject(reply, "return");
+
+if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons")))
+return 0;
+
+*blockers = g_new0(char *, virJSONValueArraySize(jblockers) + 1);
+for (i = 0; i < virJSONValueArraySize(jblockers); i++) {
+virJSONValue *jblocker = virJSONValueArrayGet(jblockers, i);
+const char *blocker = virJSONValueGetString(jblocker);
+
+(*blockers)[i] = g_strdup(blocker);
+}
+
+return 0;
+}
+
 int qemuMonitorJSONMigrateCancel(qemuMonitor *mon)
 {
 g_autoptr(virJSONValue) cmd = qemuMonitorJSONMakeCommand("migrate_cancel", 
NULL);
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 2759566892..e4c65e250e 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -184,6 +184,9 @@ qemuMonitorJSONMigrate(qemuMonitor *mon,
unsigned int flags,
const char *uri);
 int
+qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers);
+int
 qemuMonitorJSONGetSpiceMigrationStatus(qemuMonitor *mon,
bool *spice_migrated);
 
-- 
2.31.1

Re: [libvirt PATCH v3 3/4] qemu_migration: get migration blockers before hardcoded checks

[Eugenio Perez Martin]

On Wed, Jul 20, 2022 at 4:48 PM Jiri Denemark  wrote:
>
> On Wed, Jul 20, 2022 at 16:29:45 +0200, Eugenio Perez Martin wrote:
> > On Wed, Jul 20, 2022 at 4:01 PM Jiri Denemark  wrote:
> > >
> > > On Wed, Jul 20, 2022 at 14:15:57 +0200, Eugenio Pérez wrote:
> > > > since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> > > > will return an array of error strings describing the migration blockers.
> > > > This can be used to check whether there are any devices blocking
> > > > migration, etc.
> > > >
> > > > Enable qemuMigrationSrcIsAllowed to query it.
> > > >
> > > > Signed-off-by: Eugenio Pérez 
> > > > ---
> > > > v3:
> > > > * Report message with a colon.
> > > > * Report all blockers instead of only the first.
> > > > ---
> > > >  src/qemu/qemu_migration.c | 34 ++
> > > >  1 file changed, 34 insertions(+)
> > > >
> > > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > > index b12cb518ee..6ac4ef150b 100644
> > > > --- a/src/qemu/qemu_migration.c
> > > > +++ b/src/qemu/qemu_migration.c
> > > > @@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const 
> > > > virDomainDef *def)
> > > >  return true;
> > > >  }
> > > >
> > > > +static int
> > > > +qemuDomainGetMigrationBlockers(virQEMUDriver *driver,
> > > > +   virDomainObj *vm,
> > > > +   char ***blockers)
> > > > +{
> > > > +qemuDomainObjPrivate *priv = vm->privateData;
> > > > +int rc;
> > > > +
> > > > +qemuDomainObjEnterMonitor(driver, vm);
> > > > +rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers);
> > > > +qemuDomainObjExitMonitor(vm);
> > > > +
> > > > +return rc;
> > > > +}
> > > >
> > > >  /**
> > > >   * qemuMigrationSrcIsAllowed:
> > > > @@ -1439,6 +1453,26 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> > > >  int nsnapshots;
> > > >  int pauseReason;
> > > >  size_t i;
> > > > +int r;
> > > > +
> > > > +/* Ask qemu if it have a migration blocker */
> > > > +if (virQEMUCapsGet(priv->qemuCaps, 
> > > > QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
> > > > +g_auto(GStrv) blockers = NULL;
> > > > +r = qemuDomainGetMigrationBlockers(driver, vm, );
> > > > +if (r != 0) {
> > > > +virReportError(VIR_ERR_OPERATION_INVALID,
> > > > +   _("cannot migrate domain: %s"),
> > > > +   _("error getting blockers"));
> > > > +return false;
> > > > +}
> > >
> > > As mentioned in v2 review the virReportError call should be dropped as
> > > it overwrites the error reported by qemuDomainGetMigrationBlockers. That
> > > is, you can just
> > >
> > > if (qemuDomainGetMigrationBlockers(driver, vm, ) < 0)
> > > return false;
> > >
> >
> > But there are a few conditions that don't report an error, like a bad
> > JSON answer. For example, if "blockers" is not an array parsing the
> > response JSON, libvirt would not print any error, isn't it?
>
> Well in qemuDomainGetMigrationBlockers you now have
>
> if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons")))
> return 0;
>
> so you would not get pass r != 0 in such case anyway. If you wanted to
> distinguish missing blocked-reasons (that is no blockers) and
> blocked-reasons which is not an array (invalid response), you would need
> to do something else:
>
> if (!(jblockers = virJSONValueObjectGet(data, "blocked-reasons")))
> return 0;
>
> if (!virJSONValueIsArray(jblockers)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>_("blocker-reasons is not an array"));
> return -1;
> }
>
> All this inside qemuDomainGetMigrationBlockers() to make sure the
> function reports an error when returning -1. The caller would not report
> any error anyway.
>

That's right.

Deleting error report then.

Thanks!

RE: Persistent Failed to attach device following transient Failed to read product/vendor ID

[Pighin, Anthony (Nokia - CA/Ottawa)]

Correct, PCIe link bounce/flap. The attached PCIe device entered a failed state 
where it was repeatedly resetting, and therefore the link itself was going up 
and down.

-Original Message-
From: Michal Prívozník  
Sent: Wednesday, July 20, 2022 11:07 AM
To: Pighin, Anthony (Nokia - CA/Ottawa) ; 
libvir-list@redhat.com
Subject: Re: Persistent Failed to attach device following transient Failed to 
read product/vendor ID

On 7/11/22 20:14, Pighin, Anthony (Nokia - CA/Ottawa) wrote:
> I'm attempting to solve the issue reported here:
> 
> https://gitlab.com/libvirt/libvirt/-/issues/345
> 
> Hoping the originator of virHostdevDeleteMissingPCIDevices() will be able to 
> comment, as I am still trying to reproduce the issue with additional debug in 
> place.
> 
> diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c 
> index c0ce867596..d43354963e 100644
> --- a/src/hypervisor/virhostdev.c
> +++ b/src/hypervisor/virhostdev.c
> @@ -1101,11 +1101,11 @@ virHostdevReAttachPCIDevices(virHostdevManager *mgr,
>  VIR_ERROR(_("Failed to allocate PCI device list: %s"),
>virGetLastErrorMessage());
>  virResetLastError();
> -return;
>  }
> -
> -virHostdevReAttachPCIDevicesImpl(mgr, drv_name, dom_name, pcidevs,
> - hostdevs, nhostdevs);
> +else {
> +virHostdevReAttachPCIDevicesImpl(mgr, drv_name, dom_name, pcidevs,
> +hostdevs, nhostdevs);
> +}
> 
>  /* Handle the case where PCI devices from the host went missing
>   * during the domain lifetime */
> 

Yeah, this looks like a correct fix, but I'm trying to understand the original 
problem more. In the gilab issue you mention 'link bounce' - do you mean PCIe 
link?

Michal

Re: Persistent Failed to attach device following transient Failed to read product/vendor ID

[Michal Prívozník]

On 7/11/22 20:14, Pighin, Anthony (Nokia - CA/Ottawa) wrote:
> I'm attempting to solve the issue reported here:
> 
> https://gitlab.com/libvirt/libvirt/-/issues/345
> 
> Hoping the originator of virHostdevDeleteMissingPCIDevices() will be able to 
> comment, as I am still trying to reproduce the issue with additional debug in 
> place.
> 
> diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
> index c0ce867596..d43354963e 100644
> --- a/src/hypervisor/virhostdev.c
> +++ b/src/hypervisor/virhostdev.c
> @@ -1101,11 +1101,11 @@ virHostdevReAttachPCIDevices(virHostdevManager *mgr,
>  VIR_ERROR(_("Failed to allocate PCI device list: %s"),
>virGetLastErrorMessage());
>  virResetLastError();
> -return;
>  }
> -
> -virHostdevReAttachPCIDevicesImpl(mgr, drv_name, dom_name, pcidevs,
> - hostdevs, nhostdevs);
> +else {
> +virHostdevReAttachPCIDevicesImpl(mgr, drv_name, dom_name, pcidevs,
> +hostdevs, nhostdevs);
> +}
> 
>  /* Handle the case where PCI devices from the host went missing
>   * during the domain lifetime */
> 

Yeah, this looks like a correct fix, but I'm trying to understand the
original problem more. In the gilab issue you mention 'link bounce' - do
you mean PCIe link?

Michal

Re: [PATCH] qemu: support hotplug cdrom with usb/scsi bus

[Michal Prívozník]

On 7/6/22 11:57, minglei.liu wrote:
> Qemu support hotplug cdrom device with usb or scsi bus,
> just unblock these devices in qemuDomainAttachDeviceDiskLiveInternal
> and qemuDomainDetachPrepDisk.
> 
> Fixes: #261

We like the full URL as it's easily clickable when viewing git log.

> 
> Signed-off-by: minglei.liu 
> ---
>  src/qemu/qemu_hotplug.c   | 13 +++-
>  tests/qemuhotplugtest.c   | 18 ++
>  .../qemuhotplug-cdrom-scsi.xml|  6 ++
>  .../qemuhotplug-cdrom-usb.xml |  6 ++
>  .../qemuhotplug-base-live+cdrom-scsi.xml  | 60 +++
>  .../qemuhotplug-base-live+cdrom-usb.xml   | 60 +++
>  6 files changed, 160 insertions(+), 3 deletions(-)
>  create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-cdrom-scsi.xml
>  create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-cdrom-usb.xml
>  create mode 100644 
> tests/qemuhotplugtestdomains/qemuhotplug-base-live+cdrom-scsi.xml
>  create mode 100644 
> tests/qemuhotplugtestdomains/qemuhotplug-base-live+cdrom-usb.xml
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 27e68370cf..d917086023 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -992,10 +992,9 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver 
> *driver,
>  bool releaseSeclabel = false;
>  int ret = -1;
>  
> -if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ||
> -disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> +if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
>  virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -   _("cdrom/floppy device hotplug isn't supported"));
> +   _("floppy device hotplug isn't supported"));
>  return -1;
>  }
>  
> @@ -1025,6 +1024,10 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver 
> *driver,
>  break;
>  
>  case VIR_DOMAIN_DISK_BUS_VIRTIO:
> +if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +   _("cdrom device with virtio bus isn't supported"));

Alignment.

> +}
>  if (qemuDomainEnsureVirtioAddress(, vm, dev) < 0)
>  goto cleanup;
>  break;
> @@ -5414,6 +5417,10 @@ qemuDomainDetachPrepDisk(virDomainObj *vm,
>  
>  case VIR_DOMAIN_DISK_DEVICE_CDROM:
>  case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
> +if ((virDomainDiskBus) disk->bus == VIR_DOMAIN_DISK_BUS_USB ||
> +(virDomainDiskBus) disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {

No need for typecasting here. However, this allows floppy hotunplug
which I believe is not supported on QEMU side.

> +break;
> +}
>  virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> _("disk device type '%s' cannot be detached"),
> virDomainDiskDeviceTypeToString(disk->device));


I'm fixing all these minor issues before pushing.

Reviewed-by: Michal Privoznik 

Congratulations on your first libvirt contribution!

Michal

Re: [libvirt PATCH v3 3/4] qemu_migration: get migration blockers before hardcoded checks

[Jiri Denemark]

On Wed, Jul 20, 2022 at 16:29:45 +0200, Eugenio Perez Martin wrote:
> On Wed, Jul 20, 2022 at 4:01 PM Jiri Denemark  wrote:
> >
> > On Wed, Jul 20, 2022 at 14:15:57 +0200, Eugenio Pérez wrote:
> > > since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> > > will return an array of error strings describing the migration blockers.
> > > This can be used to check whether there are any devices blocking
> > > migration, etc.
> > >
> > > Enable qemuMigrationSrcIsAllowed to query it.
> > >
> > > Signed-off-by: Eugenio Pérez 
> > > ---
> > > v3:
> > > * Report message with a colon.
> > > * Report all blockers instead of only the first.
> > > ---
> > >  src/qemu/qemu_migration.c | 34 ++
> > >  1 file changed, 34 insertions(+)
> > >
> > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > index b12cb518ee..6ac4ef150b 100644
> > > --- a/src/qemu/qemu_migration.c
> > > +++ b/src/qemu/qemu_migration.c
> > > @@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const 
> > > virDomainDef *def)
> > >  return true;
> > >  }
> > >
> > > +static int
> > > +qemuDomainGetMigrationBlockers(virQEMUDriver *driver,
> > > +   virDomainObj *vm,
> > > +   char ***blockers)
> > > +{
> > > +qemuDomainObjPrivate *priv = vm->privateData;
> > > +int rc;
> > > +
> > > +qemuDomainObjEnterMonitor(driver, vm);
> > > +rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers);
> > > +qemuDomainObjExitMonitor(vm);
> > > +
> > > +return rc;
> > > +}
> > >
> > >  /**
> > >   * qemuMigrationSrcIsAllowed:
> > > @@ -1439,6 +1453,26 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> > >  int nsnapshots;
> > >  int pauseReason;
> > >  size_t i;
> > > +int r;
> > > +
> > > +/* Ask qemu if it have a migration blocker */
> > > +if (virQEMUCapsGet(priv->qemuCaps, 
> > > QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
> > > +g_auto(GStrv) blockers = NULL;
> > > +r = qemuDomainGetMigrationBlockers(driver, vm, );
> > > +if (r != 0) {
> > > +virReportError(VIR_ERR_OPERATION_INVALID,
> > > +   _("cannot migrate domain: %s"),
> > > +   _("error getting blockers"));
> > > +return false;
> > > +}
> >
> > As mentioned in v2 review the virReportError call should be dropped as
> > it overwrites the error reported by qemuDomainGetMigrationBlockers. That
> > is, you can just
> >
> > if (qemuDomainGetMigrationBlockers(driver, vm, ) < 0)
> > return false;
> >
> 
> But there are a few conditions that don't report an error, like a bad
> JSON answer. For example, if "blockers" is not an array parsing the
> response JSON, libvirt would not print any error, isn't it?

Well in qemuDomainGetMigrationBlockers you now have

if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons")))
return 0;

so you would not get pass r != 0 in such case anyway. If you wanted to
distinguish missing blocked-reasons (that is no blockers) and
blocked-reasons which is not an array (invalid response), you would need
to do something else:

if (!(jblockers = virJSONValueObjectGet(data, "blocked-reasons")))
return 0;

if (!virJSONValueIsArray(jblockers)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
   _("blocker-reasons is not an array"));
return -1;
}

All this inside qemuDomainGetMigrationBlockers() to make sure the
function reports an error when returning -1. The caller would not report
any error anyway.

Jirka

Re: [PATCH 5/9] domain_conf: replace switch with if in virDomainChrDefFree()

[Michal Prívozník]

On 7/20/22 15:44, Peter Krempa wrote:
> On Wed, Jul 20, 2022 at 15:11:08 +0200, Kristina Hanicova wrote:
>> Switch is used for just one case, so I replaced it with a simple
>> if condition.
>>
>> Signed-off-by: Kristina Hanicova 
>> ---
>>  src/conf/domain_conf.c | 7 +--
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index b903dac1cb..f51476c968 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -2904,8 +2904,7 @@ void virDomainChrDefFree(virDomainChrDef *def)
>>  if (!def)
>>  return;
>>  
>> -switch (def->deviceType) {
> 
> Alternatively a more future proof (but more verbose) approach which we
> are doing in many places is to use the proper type (either by fixing the
> struct to use proper type, or typecasting) in the switch expression and 
> then simply enumerate all values.
> 
> That way any further addition doesn't have to un-do this patch.

When I tried to do that it wasn't met with much appreciation:

https://listman.redhat.com/archives/libvir-list/2022-May/231776.html

Michal

Re: [libvirt PATCH v3 3/4] qemu_migration: get migration blockers before hardcoded checks

[Eugenio Perez Martin]

On Wed, Jul 20, 2022 at 4:01 PM Jiri Denemark  wrote:
>
> On Wed, Jul 20, 2022 at 14:15:57 +0200, Eugenio Pérez wrote:
> > since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> > will return an array of error strings describing the migration blockers.
> > This can be used to check whether there are any devices blocking
> > migration, etc.
> >
> > Enable qemuMigrationSrcIsAllowed to query it.
> >
> > Signed-off-by: Eugenio Pérez 
> > ---
> > v3:
> > * Report message with a colon.
> > * Report all blockers instead of only the first.
> > ---
> >  src/qemu/qemu_migration.c | 34 ++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index b12cb518ee..6ac4ef150b 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef 
> > *def)
> >  return true;
> >  }
> >
> > +static int
> > +qemuDomainGetMigrationBlockers(virQEMUDriver *driver,
> > +   virDomainObj *vm,
> > +   char ***blockers)
> > +{
> > +qemuDomainObjPrivate *priv = vm->privateData;
> > +int rc;
> > +
> > +qemuDomainObjEnterMonitor(driver, vm);
> > +rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers);
> > +qemuDomainObjExitMonitor(vm);
> > +
> > +return rc;
> > +}
> >
> >  /**
> >   * qemuMigrationSrcIsAllowed:
> > @@ -1439,6 +1453,26 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> >  int nsnapshots;
> >  int pauseReason;
> >  size_t i;
> > +int r;
> > +
> > +/* Ask qemu if it have a migration blocker */
> > +if (virQEMUCapsGet(priv->qemuCaps, 
> > QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
> > +g_auto(GStrv) blockers = NULL;
> > +r = qemuDomainGetMigrationBlockers(driver, vm, );
> > +if (r != 0) {
> > +virReportError(VIR_ERR_OPERATION_INVALID,
> > +   _("cannot migrate domain: %s"),
> > +   _("error getting blockers"));
> > +return false;
> > +}
>
> As mentioned in v2 review the virReportError call should be dropped as
> it overwrites the error reported by qemuDomainGetMigrationBlockers. That
> is, you can just
>
> if (qemuDomainGetMigrationBlockers(driver, vm, ) < 0)
> return false;
>

But there are a few conditions that don't report an error, like a bad
JSON answer. For example, if "blockers" is not an array parsing the
response JSON, libvirt would not print any error, isn't it?

> > +
> > +if (blockers && blockers[0]) {
> > +g_autofree char *reasons = g_strjoinv(", ", blockers);
>
> In the following patch you change ", " to "; ". I don't mind that much
> either way, but it should be done in this patch :-)
>
> > +virReportError(VIR_ERR_OPERATION_INVALID,
> > +   _("cannot migrate domain: %s"), reasons);
> > +return false;
> > +}
> > +}
> >
> >  /* perform these checks only when migrating to remote hosts */
> >  if (remote) {
>
> Hmm, easy but not trivial changes so I guess v4 would be better.
>
> Jirka
>

Re: [PATCH 9/9] domain_conf: rewrite variable setting to ternary operator

[Michal Prívozník]

On 7/20/22 15:40, Peter Krempa wrote:
> On Wed, Jul 20, 2022 at 15:11:12 +0200, Kristina Hanicova wrote:
>> Signed-off-by: Kristina Hanicova 
>> ---
>>  src/conf/domain_conf.c | 8 ++--
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index e52f39c809..b600bfec31 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -4206,12 +4206,8 @@ virDomainObjGetOneDefState(virDomainObj *vm,
>>  if (virDomainObjUpdateModificationImpact(vm, ) < 0)
>>  return NULL;
>>  
>> -if (live) {
>> -if (flags & VIR_DOMAIN_AFFECT_LIVE)
>> -*live = true;
>> -else
>> -*live = false;
>> -}
>> +if (live)
>> +*live = (flags & VIR_DOMAIN_AFFECT_LIVE) ? true : false;
>>  
> 
> https://libvirt.org/coding-style.html#conditional-expressions
> 
> We suggest that new code avoids ternary operators.
> 
> I'd prefer if this patch is dropped.
> 

And what about:

if (live)
  *live = !!(flags & VIR_DOMAIN_AFFECT_LIVE);

? I agree that current version of the code is more verbose than it needs
to be. And while ternary operators might look bad, in fact I've
suggested them here:


https://gitlab.com/MichalPrivoznik/libvirt/-/commit/d2894d9f07ff2dde77bea53bb39dc8d0176eac85#f6109f17d3eb7394abb620a27ec56d76dd5220b0_4892_4878

Is there any better way?

Michal

Re: [libvirt PATCH v3 2/4] qemu_monitor: add support for get qemu migration blockers

[Eugenio Perez Martin]

On Wed, Jul 20, 2022 at 3:58 PM Jiri Denemark  wrote:
>
> On Wed, Jul 20, 2022 at 14:15:56 +0200, Eugenio Pérez wrote:
> > since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> > will return an array of error strings describing the migration blockers.
> > This can be used to check whether there are any devices blocking
> > migration, etc.
> >
> > Enable qemu monitor to send this query.  This will allow
> > qemuMigrationSrcIsAllowed to dynamically ask for migration blockers,
> > reducing duplication.
> >
> > Signed-off-by: Eugenio Pérez 
> > ---
> > v3:
> > * Squash some patches
> > * Return ok in qemuMonitorJSONGetMigrationBlockers is there are no
> >   blockers.
> > * Move note to function doc.
> > ---
> >  src/qemu/qemu_monitor.c  | 11 +
> >  src/qemu/qemu_monitor.h  |  4 
> >  src/qemu/qemu_monitor_json.c | 43 
> >  src/qemu/qemu_monitor_json.h |  3 +++
> >  4 files changed, 61 insertions(+)
> >
> > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> > index 109107eaae..e0939beecd 100644
> > --- a/src/qemu/qemu_monitor.c
> > +++ b/src/qemu/qemu_monitor.c
> > @@ -4486,3 +4486,14 @@ qemuMonitorMigrateRecover(qemuMonitor *mon,
> >
> >  return qemuMonitorJSONMigrateRecover(mon, uri);
> >  }
> > +
> > +int
> > +qemuMonitorGetMigrationBlockers(qemuMonitor *mon,
> > +char ***blockers)
> > +{
> > +VIR_DEBUG("blockers=%p", blockers);
> > +
> > +QEMU_CHECK_MONITOR(mon);
> > +
> > +return qemuMonitorJSONGetMigrationBlockers(mon, blockers);
> > +}
> > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> > index cc1a0bc8c9..b82f198285 100644
> > --- a/src/qemu/qemu_monitor.h
> > +++ b/src/qemu/qemu_monitor.h
> > @@ -1543,3 +1543,7 @@ qemuMonitorChangeMemoryRequestedSize(qemuMonitor *mon,
> >  int
> >  qemuMonitorMigrateRecover(qemuMonitor *mon,
> >const char *uri);
> > +
> > +int
> > +qemuMonitorGetMigrationBlockers(qemuMonitor *mon,
> > +char ***blockers);
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index 5e4a86e5ad..6d15a458a3 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -3338,6 +3338,49 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
> >  return 0;
> >  }
> >
> > +/*
> > + * Get the exposed migration blockers.
> > + *
> > + * This function assume qemu has the capability of request them.
> > + *
> > + * It returns a NULL terminated array on blockers if there are any, or it 
> > set
> > + * it to NULL otherwise.
> > + */
> > +int qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
>
> I'm sorry for not noticing this in v2, but the return type should be on
> a separate line (yeah, I know the three functions around the place you
> put yours do not follow this).
>

I'll send a new version with this.

> I can change it before pushing to avoid trivial v4 if you don't mind.
>
> With the change
> Reviewed-by: Jiri Denemark 
>

Re: [libvirt PATCH v3 4/4] qemu_migration: Do not forbid vDPA devices if can query blockers

[Eugenio Perez Martin]

On Wed, Jul 20, 2022 at 4:02 PM Jiri Denemark  wrote:
>
> On Wed, Jul 20, 2022 at 14:15:58 +0200, Eugenio Pérez wrote:
> > vDPA devices will be migratable soon. Since they are not migratable
> > before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> > for migration blockers, let it hardcoded in that case.
> >
> > Otherwise, ask qemu about the explicit blocker.
> >
> > Signed-off-by: Eugenio Pérez 
> > ---
> > v3: Fix indentation
> > ---
> >  src/qemu/qemu_migration.c | 8 +---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 6ac4ef150b..45e16242f0 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> >  int pauseReason;
> >  size_t i;
> >  int r;
> > +bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
> > +
> > QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
> >
> >  /* Ask qemu if it have a migration blocker */
> > -if (virQEMUCapsGet(priv->qemuCaps, 
> > QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
> > +if (blockedReasonsCap) {
> >  g_auto(GStrv) blockers = NULL;
> >  r = qemuDomainGetMigrationBlockers(driver, vm, );
> >  if (r != 0) {
> > @@ -1467,7 +1469,7 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> >  }
> >
> >  if (blockers && blockers[0]) {
> > -g_autofree char *reasons = g_strjoinv(", ", blockers);
> > +g_autofree char *reasons = g_strjoinv("; ", blockers);
> >  virReportError(VIR_ERR_OPERATION_INVALID,
> > _("cannot migrate domain: %s"), reasons);
> >  return false;
>
> This hunk should be squashed into the previous patch.
>

Sorry, I squashed in the wrong patch, I'll send v4 with this.

Thanks!

> > @@ -1580,7 +1582,7 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> >  virDomainNetDef *net = vm->def->nets[i];
> >  qemuSlirp *slirp;
> >
> > -if (net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
> > +if (!blockedReasonsCap && net->type == 
> > VIR_DOMAIN_NET_TYPE_VDPA) {
> >  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > _("vDPA devices cannot be migrated"));
> >  return false;
>
> With that change
>
> Reviewed-by: Jiri Denemark 
>

Re: [PATCH 9/9] domain_conf: rewrite variable setting to ternary operator

[Kristina Hanicova]

On Wed, Jul 20, 2022 at 3:41 PM Peter Krempa  wrote:

> On Wed, Jul 20, 2022 at 15:11:12 +0200, Kristina Hanicova wrote:
> > Signed-off-by: Kristina Hanicova 
> > ---
> >  src/conf/domain_conf.c | 8 ++--
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index e52f39c809..b600bfec31 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -4206,12 +4206,8 @@ virDomainObjGetOneDefState(virDomainObj *vm,
> >  if (virDomainObjUpdateModificationImpact(vm, ) < 0)
> >  return NULL;
> >
> > -if (live) {
> > -if (flags & VIR_DOMAIN_AFFECT_LIVE)
> > -*live = true;
> > -else
> > -*live = false;
> > -}
> > +if (live)
> > +*live = (flags & VIR_DOMAIN_AFFECT_LIVE) ? true : false;
> >
>
> https://libvirt.org/coding-style.html#conditional-expressions
>
> We suggest that new code avoids ternary operators.
>
> I'd prefer if this patch is dropped.
>
>
I think that it is reasonably used in this case and makes the code much
more readable. Also its simple enough, no nesting or spanning more lines


Kristina

Re: [libvirt PATCH v3 4/4] qemu_migration: Do not forbid vDPA devices if can query blockers

[Jiri Denemark]

On Wed, Jul 20, 2022 at 14:15:58 +0200, Eugenio Pérez wrote:
> vDPA devices will be migratable soon. Since they are not migratable
> before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> for migration blockers, let it hardcoded in that case.
> 
> Otherwise, ask qemu about the explicit blocker.
> 
> Signed-off-by: Eugenio Pérez 
> ---
> v3: Fix indentation
> ---
>  src/qemu/qemu_migration.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 6ac4ef150b..45e16242f0 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
>  int pauseReason;
>  size_t i;
>  int r;
> +bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
> +
> QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
>  
>  /* Ask qemu if it have a migration blocker */
> -if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) 
> {
> +if (blockedReasonsCap) {
>  g_auto(GStrv) blockers = NULL;
>  r = qemuDomainGetMigrationBlockers(driver, vm, );
>  if (r != 0) {
> @@ -1467,7 +1469,7 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
>  }
>  
>  if (blockers && blockers[0]) {
> -g_autofree char *reasons = g_strjoinv(", ", blockers);
> +g_autofree char *reasons = g_strjoinv("; ", blockers);
>  virReportError(VIR_ERR_OPERATION_INVALID,
> _("cannot migrate domain: %s"), reasons);
>  return false;

This hunk should be squashed into the previous patch.

> @@ -1580,7 +1582,7 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
>  virDomainNetDef *net = vm->def->nets[i];
>  qemuSlirp *slirp;
>  
> -if (net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
> +if (!blockedReasonsCap && net->type == VIR_DOMAIN_NET_TYPE_VDPA) 
> {
>  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("vDPA devices cannot be migrated"));
>  return false;

With that change

Reviewed-by: Jiri Denemark 

Re: [libvirt PATCH v3 3/4] qemu_migration: get migration blockers before hardcoded checks

[Jiri Denemark]

On Wed, Jul 20, 2022 at 14:15:57 +0200, Eugenio Pérez wrote:
> since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> will return an array of error strings describing the migration blockers.
> This can be used to check whether there are any devices blocking
> migration, etc.
> 
> Enable qemuMigrationSrcIsAllowed to query it.
> 
> Signed-off-by: Eugenio Pérez 
> ---
> v3:
> * Report message with a colon.
> * Report all blockers instead of only the first.
> ---
>  src/qemu/qemu_migration.c | 34 ++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index b12cb518ee..6ac4ef150b 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef 
> *def)
>  return true;
>  }
>  
> +static int
> +qemuDomainGetMigrationBlockers(virQEMUDriver *driver,
> +   virDomainObj *vm,
> +   char ***blockers)
> +{
> +qemuDomainObjPrivate *priv = vm->privateData;
> +int rc;
> +
> +qemuDomainObjEnterMonitor(driver, vm);
> +rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers);
> +qemuDomainObjExitMonitor(vm);
> +
> +return rc;
> +}
>  
>  /**
>   * qemuMigrationSrcIsAllowed:
> @@ -1439,6 +1453,26 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
>  int nsnapshots;
>  int pauseReason;
>  size_t i;
> +int r;
> +
> +/* Ask qemu if it have a migration blocker */
> +if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) 
> {
> +g_auto(GStrv) blockers = NULL;
> +r = qemuDomainGetMigrationBlockers(driver, vm, );
> +if (r != 0) {
> +virReportError(VIR_ERR_OPERATION_INVALID,
> +   _("cannot migrate domain: %s"),
> +   _("error getting blockers"));
> +return false;
> +}

As mentioned in v2 review the virReportError call should be dropped as
it overwrites the error reported by qemuDomainGetMigrationBlockers. That
is, you can just

if (qemuDomainGetMigrationBlockers(driver, vm, ) < 0)
return false;

> +
> +if (blockers && blockers[0]) {
> +g_autofree char *reasons = g_strjoinv(", ", blockers);

In the following patch you change ", " to "; ". I don't mind that much
either way, but it should be done in this patch :-)

> +virReportError(VIR_ERR_OPERATION_INVALID,
> +   _("cannot migrate domain: %s"), reasons);
> +return false;
> +}
> +}
>  
>  /* perform these checks only when migrating to remote hosts */
>  if (remote) {

Hmm, easy but not trivial changes so I guess v4 would be better.

Jirka

Re: [PATCH] domain_conf: remove else after return / goto

[Kristina Hanicova]

On Wed, Jul 20, 2022 at 3:34 PM Daniel P. Berrangé 
wrote:

>
> What's the reasoning for making this change ?
>

I stumbled upon this and decided to rewrite code in src/conf/ that could be
easily improved.


>
> On Wed, Jul 20, 2022 at 02:59:58PM +0200, Kristina Hanicova wrote:
> > Signed-off-by: Kristina Hanicova 
> > ---
> >  src/conf/domain_conf.c | 55 --
> >  1 file changed, 31 insertions(+), 24 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 44a01ab628..bc4b74c1c8 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -4084,8 +4084,8 @@ virDomainObjGetPersistentDef(virDomainXMLOption
> *xmlopt,
> >
> >  if (domain->newDef)
> >  return domain->newDef;
> > -else
> > -return domain->def;
> > +
> > +return domain->def;
> >  }
> > @@ -4223,8 +4223,8 @@ virDomainObjGetOneDefState(virDomainObj *vm,
> >
> >  if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG)
> >  return vm->newDef;
> > -else
> > -return vm->def;
> > +
> > +return vm->def;
> >  }
>
>
> I'm not really convinced these two changes are better.
>

Well, the else after return is redundant because it will never reach the
'else' branch if the condition is true.

I think this looks cleaner and is more readable, because having 'else'
branch indicates to me that no return / break / goto is in the previous
branch and the funcion can reach it.


>
> >
> >
> > @@ -6029,7 +6029,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
> >   VIR_XML_PROP_NONZERO,
> >   >sgio)) < 0) {
> >  return -1;
> > -} else if (rv > 0) {
> > +}
> > +
> > +if (rv > 0) {
> >  if (def->source.subsys.type !=
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
> >  virReportError(VIR_ERR_XML_ERROR, "%s",
> > _("sgio is only supported for scsi host
> device"));
> > @@ -6041,8 +6043,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
> >   VIR_XML_PROP_NONE,
> >   >rawio)) < 0) {
> >  return -1;
> > -} else if (rv > 0 &&
> > -   def->source.subsys.type !=
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
> > +}
> > +
> > +if (rv > 0 && def->source.subsys.type !=
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
> >  virReportError(VIR_ERR_XML_ERROR, "%s",
> > _("rawio is only supported for scsi host
> device"));
> >  return -1;
>
>
> I don't mind eliminating the else, when the first 'if' is just an
> error return/goto case.
>
> > @@ -8056,15 +8059,15 @@ virDomainControllerModelTypeFromString(const
> virDomainControllerDef *def,
> >  {
> >  if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
> >  return virDomainControllerModelSCSITypeFromString(model);
> > -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
> > +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
> >  return virDomainControllerModelUSBTypeFromString(model);
> > -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> > +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> >  return virDomainControllerModelPCITypeFromString(model);
> > -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> > +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> >  return virDomainControllerModelIDETypeFromString(model);
> > -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
> > +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
> >  return
> virDomainControllerModelVirtioSerialTypeFromString(model);
> > -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
> > +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
> >  return virDomainControllerModelISATypeFromString(model);
>
> This giant if/else should be a switch instead
>

Thanks, way better idea.


>
> >
> >  return -1;
> > @@ -8077,15 +8080,15 @@
> virDomainControllerModelTypeToString(virDomainControllerDef *def,
> >  {
> >  if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
> >  return virDomainControllerModelSCSITypeToString(model);
> > -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
> > +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
> >  return virDomainControllerModelUSBTypeToString(model);
> > -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> > +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> >  return virDomainControllerModelPCITypeToString(model);
> > -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> > +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> >  return virDomainControllerModelIDETypeToString(model);
> > -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
> > +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
> > 

Re: [libvirt PATCH v3 2/4] qemu_monitor: add support for get qemu migration blockers

[Jiri Denemark]

On Wed, Jul 20, 2022 at 14:15:56 +0200, Eugenio Pérez wrote:
> since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> will return an array of error strings describing the migration blockers.
> This can be used to check whether there are any devices blocking
> migration, etc.
> 
> Enable qemu monitor to send this query.  This will allow
> qemuMigrationSrcIsAllowed to dynamically ask for migration blockers,
> reducing duplication.
> 
> Signed-off-by: Eugenio Pérez 
> ---
> v3:
> * Squash some patches
> * Return ok in qemuMonitorJSONGetMigrationBlockers is there are no
>   blockers.
> * Move note to function doc.
> ---
>  src/qemu/qemu_monitor.c  | 11 +
>  src/qemu/qemu_monitor.h  |  4 
>  src/qemu/qemu_monitor_json.c | 43 
>  src/qemu/qemu_monitor_json.h |  3 +++
>  4 files changed, 61 insertions(+)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 109107eaae..e0939beecd 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -4486,3 +4486,14 @@ qemuMonitorMigrateRecover(qemuMonitor *mon,
>  
>  return qemuMonitorJSONMigrateRecover(mon, uri);
>  }
> +
> +int
> +qemuMonitorGetMigrationBlockers(qemuMonitor *mon,
> +char ***blockers)
> +{
> +VIR_DEBUG("blockers=%p", blockers);
> +
> +QEMU_CHECK_MONITOR(mon);
> +
> +return qemuMonitorJSONGetMigrationBlockers(mon, blockers);
> +}
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index cc1a0bc8c9..b82f198285 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1543,3 +1543,7 @@ qemuMonitorChangeMemoryRequestedSize(qemuMonitor *mon,
>  int
>  qemuMonitorMigrateRecover(qemuMonitor *mon,
>const char *uri);
> +
> +int
> +qemuMonitorGetMigrationBlockers(qemuMonitor *mon,
> +char ***blockers);
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 5e4a86e5ad..6d15a458a3 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -3338,6 +3338,49 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
>  return 0;
>  }
>  
> +/*
> + * Get the exposed migration blockers.
> + *
> + * This function assume qemu has the capability of request them.
> + *
> + * It returns a NULL terminated array on blockers if there are any, or it set
> + * it to NULL otherwise.
> + */
> +int qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,

I'm sorry for not noticing this in v2, but the return type should be on
a separate line (yeah, I know the three functions around the place you
put yours do not follow this).

I can change it before pushing to avoid trivial v4 if you don't mind.

With the change
Reviewed-by: Jiri Denemark 

Re: [libvirt PATCH v3 1/4] qemu: introduce capability QEMU_CAPS_MIGRATION_BLOCKED_REASONS

[Jiri Denemark]

On Wed, Jul 20, 2022 at 14:15:55 +0200, Eugenio Pérez wrote:
> From: Jonathon Jongsma 
> 
> since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> will return an array of error strings describing the migration blockers.
> This can be used to check whether there are any devices blocking
> migration, etc.
> 
> Signed-off-by: Jonathon Jongsma 
> Signed-off-by: Eugenio Pérez 
> ---
> v3: s/QEMU_MIGRATION_BLOCKED_REASONS/QEMU_CAPS_MIGRATION_BLOCKED_REASONS/
> ---
>  src/qemu/qemu_capabilities.c  | 2 ++
>  src/qemu/qemu_capabilities.h  | 1 +
>  tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml   | 1 +
>  tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml   | 1 +
>  tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_7.0.0.aarch64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_7.0.0.ppc64.xml   | 1 +
>  tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml  | 1 +
>  13 files changed, 14 insertions(+)

Reviewed-by: Jiri Denemark 

Re: [PATCH 5/9] domain_conf: replace switch with if in virDomainChrDefFree()

[Peter Krempa]

On Wed, Jul 20, 2022 at 15:11:08 +0200, Kristina Hanicova wrote:
> Switch is used for just one case, so I replaced it with a simple
> if condition.
> 
> Signed-off-by: Kristina Hanicova 
> ---
>  src/conf/domain_conf.c | 7 +--
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b903dac1cb..f51476c968 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2904,8 +2904,7 @@ void virDomainChrDefFree(virDomainChrDef *def)
>  if (!def)
>  return;
>  
> -switch (def->deviceType) {

Alternatively a more future proof (but more verbose) approach which we
are doing in many places is to use the proper type (either by fixing the
struct to use proper type, or typecasting) in the switch expression and 
then simply enumerate all values.

That way any further addition doesn't have to un-do this patch.

> -case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL:
> +if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL) {
>  switch (def->targetType) {
>  case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD:
>  g_free(def->target.addr);
> @@ -2916,10 +2915,6 @@ void virDomainChrDefFree(virDomainChrDef *def)
>  g_free(def->target.name);
>  break;
>  }
> -break;
> -
> -default:
> -break;
>  }
>  
>  virObjectUnref(def->source);
> -- 
> 2.35.3
> 

Re: [PATCH 9/9] domain_conf: rewrite variable setting to ternary operator

[Peter Krempa]

On Wed, Jul 20, 2022 at 15:11:12 +0200, Kristina Hanicova wrote:
> Signed-off-by: Kristina Hanicova 
> ---
>  src/conf/domain_conf.c | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e52f39c809..b600bfec31 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4206,12 +4206,8 @@ virDomainObjGetOneDefState(virDomainObj *vm,
>  if (virDomainObjUpdateModificationImpact(vm, ) < 0)
>  return NULL;
>  
> -if (live) {
> -if (flags & VIR_DOMAIN_AFFECT_LIVE)
> -*live = true;
> -else
> -*live = false;
> -}
> +if (live)
> +*live = (flags & VIR_DOMAIN_AFFECT_LIVE) ? true : false;
>  

https://libvirt.org/coding-style.html#conditional-expressions

We suggest that new code avoids ternary operators.

I'd prefer if this patch is dropped.

Re: [PATCH 6/9] domain_conf: rewrite virDomainSoundCodecDefFree()

[Daniel P . Berrangé]

On Wed, Jul 20, 2022 at 03:11:09PM +0200, Kristina Hanicova wrote:
> Signed-off-by: Kristina Hanicova 
> ---
>  src/conf/domain_conf.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f51476c968..88d50d27f5 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2955,10 +2955,8 @@ void virDomainSmartcardDefFree(virDomainSmartcardDef 
> *def)
>  
>  void virDomainSoundCodecDefFree(virDomainSoundCodecDef *def)
>  {
> -if (!def)
> -return;
> -
> -g_free(def);
> +if (def)
> +g_free(def);
>  }

This is not desirable - our default pattern for Free() funcs is
to return immediately if NULL. At a later date other statements
may end up being added in the middle of this existing function,
which would involve then reverting this proposed change.

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] domain_conf: remove else after return / goto

[Daniel P . Berrangé]

What's the reasoning for making this change ?

On Wed, Jul 20, 2022 at 02:59:58PM +0200, Kristina Hanicova wrote:
> Signed-off-by: Kristina Hanicova 
> ---
>  src/conf/domain_conf.c | 55 --
>  1 file changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 44a01ab628..bc4b74c1c8 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4084,8 +4084,8 @@ virDomainObjGetPersistentDef(virDomainXMLOption *xmlopt,
>  
>  if (domain->newDef)
>  return domain->newDef;
> -else
> -return domain->def;
> +
> +return domain->def;
>  }
> @@ -4223,8 +4223,8 @@ virDomainObjGetOneDefState(virDomainObj *vm,
>  
>  if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG)
>  return vm->newDef;
> -else
> -return vm->def;
> +
> +return vm->def;
>  }


I'm not really convinced these two changes are better.

>  
>  
> @@ -6029,7 +6029,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>   VIR_XML_PROP_NONZERO,
>   >sgio)) < 0) {
>  return -1;
> -} else if (rv > 0) {
> +}
> +
> +if (rv > 0) {
>  if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
>  virReportError(VIR_ERR_XML_ERROR, "%s",
> _("sgio is only supported for scsi host device"));
> @@ -6041,8 +6043,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>   VIR_XML_PROP_NONE,
>   >rawio)) < 0) {
>  return -1;
> -} else if (rv > 0 &&
> -   def->source.subsys.type != 
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
> +}
> +
> +if (rv > 0 && def->source.subsys.type != 
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
>  virReportError(VIR_ERR_XML_ERROR, "%s",
> _("rawio is only supported for scsi host device"));
>  return -1;


I don't mind eliminating the else, when the first 'if' is just an
error return/goto case.

> @@ -8056,15 +8059,15 @@ virDomainControllerModelTypeFromString(const 
> virDomainControllerDef *def,
>  {
>  if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
>  return virDomainControllerModelSCSITypeFromString(model);
> -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
> +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
>  return virDomainControllerModelUSBTypeFromString(model);
> -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
>  return virDomainControllerModelPCITypeFromString(model);
> -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
>  return virDomainControllerModelIDETypeFromString(model);
> -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
> +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
>  return virDomainControllerModelVirtioSerialTypeFromString(model);
> -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
> +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
>  return virDomainControllerModelISATypeFromString(model);

This giant if/else should be a switch instead

>  
>  return -1;
> @@ -8077,15 +8080,15 @@ 
> virDomainControllerModelTypeToString(virDomainControllerDef *def,
>  {
>  if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
>  return virDomainControllerModelSCSITypeToString(model);
> -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
> +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
>  return virDomainControllerModelUSBTypeToString(model);
> -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
>  return virDomainControllerModelPCITypeToString(model);
> -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
>  return virDomainControllerModelIDETypeToString(model);
> -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
> +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
>  return virDomainControllerModelVirtioSerialTypeToString(model);
> -else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
> +if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
>  return virDomainControllerModelISATypeToString(model);

Likewise a switch.

>  
>  return NULL;
> @@ -9915,9 +9918,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef 
> *def,
>  
>  ctxt->node = cur;
>  
> -if ((nsources = virXPathNodeSet("./source", ctxt, )) < 0) {
> +if ((nsources = virXPathNodeSet("./source", ctxt, )) < 0)
>  goto error;
> -} else if (nsources > 0) {
> +
> +if (nsources > 0) {
>  /* Parse only the first source 

[PATCH 2/9] domain_capabilities: reformat virDomainCapsFeatureSEVFormat()

[Kristina Hanicova]

Signed-off-by: Kristina Hanicova 
---
 src/conf/domain_capabilities.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index bb36a956db..6e8f957657 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -598,14 +598,12 @@ virDomainCapsFeatureSEVFormat(virBuffer *buf,
 virBufferAsprintf(buf, "%d\n", sev->cbitpos);
 virBufferAsprintf(buf, "%d\n",
   sev->reduced_phys_bits);
-virBufferAsprintf(buf, "%d\n",
-  sev->max_guests);
-virBufferAsprintf(buf, "%d\n",
-  sev->max_es_guests);
-if (sev->cpu0_id != NULL) {
-virBufferAsprintf(buf, "%s\n",
-  sev->cpu0_id);
-}
+virBufferAsprintf(buf, "%d\n", sev->max_guests);
+virBufferAsprintf(buf, "%d\n", 
sev->max_es_guests);
+
+if (sev->cpu0_id != NULL)
+virBufferAsprintf(buf, "%s\n", sev->cpu0_id);
+
 virBufferAdjustIndent(buf, -2);
 virBufferAddLit(buf, "\n");
 }
-- 
2.35.3

Re: [PATCH] qemu: domainjob: remove async variable from qemuDomainObjBeginJobInternal()

[Michal Prívozník]

On 7/19/22 14:47, Kristina Hanicova wrote:
> This patch removes variable 'async', which is used only once, and
> replaces it with direct comparison with an enum member.
> 
> Signed-off-by: Kristina Hanicova 
> ---
>  src/qemu/qemu_domainjob.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Michal Privoznik 

Michal

[PATCH 8/9] domain_conf: rewrite conditions in virDomainObjWaitUntil()

[Kristina Hanicova]

This patch rewrites conditions to make the code easier and less
structured.

Signed-off-by: Kristina Hanicova 
---
 src/conf/domain_conf.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1c29a2d929..e52f39c809 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3999,15 +3999,15 @@ int
 virDomainObjWaitUntil(virDomainObj *vm,
   unsigned long long whenms)
 {
-if (virCondWaitUntil(>cond, >parent.lock, whenms) < 0) {
-if (errno != ETIMEDOUT) {
-virReportSystemError(errno, "%s",
- _("failed to wait for domain condition"));
-return -1;
-}
+if (virCondWaitUntil(>cond, >parent.lock, whenms) >= 0)
+return 0;
+
+if (errno == ETIMEDOUT)
 return 1;
-}
-return 0;
+
+virReportSystemError(errno, "%s",
+ _("failed to wait for domain condition"));
+return -1;
 }
 
 
-- 
2.35.3

[PATCH 1/9] domain_capabilities: use early return in virDomainCapsFeatureSEVFormat()

[Kristina Hanicova]

Signed-off-by: Kristina Hanicova 
---
 src/conf/domain_capabilities.c | 33 -
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 33570a51db..bb36a956db 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -590,25 +590,24 @@ virDomainCapsFeatureSEVFormat(virBuffer *buf,
 {
 if (!sev) {
 virBufferAddLit(buf, "\n");
-} else {
-virBufferAddLit(buf, "\n");
-virBufferAdjustIndent(buf, 2);
-virBufferAsprintf(buf, "%d\n", sev->cbitpos);
-virBufferAsprintf(buf, "%d\n",
-  sev->reduced_phys_bits);
-virBufferAsprintf(buf, "%d\n",
-  sev->max_guests);
-virBufferAsprintf(buf, "%d\n",
-  sev->max_es_guests);
-if (sev->cpu0_id != NULL) {
-virBufferAsprintf(buf, "%s\n",
-  sev->cpu0_id);
-}
-virBufferAdjustIndent(buf, -2);
-virBufferAddLit(buf, "\n");
+return;
 }
 
-return;
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
+virBufferAsprintf(buf, "%d\n", sev->cbitpos);
+virBufferAsprintf(buf, "%d\n",
+  sev->reduced_phys_bits);
+virBufferAsprintf(buf, "%d\n",
+  sev->max_guests);
+virBufferAsprintf(buf, "%d\n",
+  sev->max_es_guests);
+if (sev->cpu0_id != NULL) {
+virBufferAsprintf(buf, "%s\n",
+  sev->cpu0_id);
+}
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
 }
 
 
-- 
2.35.3

[PATCH 6/9] domain_conf: rewrite virDomainSoundCodecDefFree()

[Kristina Hanicova]

Signed-off-by: Kristina Hanicova 
---
 src/conf/domain_conf.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f51476c968..88d50d27f5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2955,10 +2955,8 @@ void virDomainSmartcardDefFree(virDomainSmartcardDef 
*def)
 
 void virDomainSoundCodecDefFree(virDomainSoundCodecDef *def)
 {
-if (!def)
-return;
-
-g_free(def);
+if (def)
+g_free(def);
 }
 
 void virDomainSoundDefFree(virDomainSoundDef *def)
-- 
2.35.3

[PATCH 3/9] domain_capabilities: reformat virDomainCapsCPUCustomFormat()

[Kristina Hanicova]

Signed-off-by: Kristina Hanicova 
---
 src/conf/domain_capabilities.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 6e8f957657..653123f293 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -376,12 +376,14 @@ virDomainCapsCPUCustomFormat(virBuffer *buf,
 
 for (i = 0; i < custom->nmodels; i++) {
 virDomainCapsCPUModel *model = custom->models + i;
+
 virBufferAsprintf(buf, "usable));
+
 if (model->deprecated)
 virBufferAddLit(buf, " deprecated='yes'");
-virBufferAsprintf(buf, ">%s\n",
-  model->name);
+
+virBufferAsprintf(buf, ">%s\n", model->name);
 }
 
 virBufferAdjustIndent(buf, -2);
-- 
2.35.3

[PATCH 5/9] domain_conf: replace switch with if in virDomainChrDefFree()

[Kristina Hanicova]

Switch is used for just one case, so I replaced it with a simple
if condition.

Signed-off-by: Kristina Hanicova 
---
 src/conf/domain_conf.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b903dac1cb..f51476c968 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2904,8 +2904,7 @@ void virDomainChrDefFree(virDomainChrDef *def)
 if (!def)
 return;
 
-switch (def->deviceType) {
-case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL:
+if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL) {
 switch (def->targetType) {
 case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD:
 g_free(def->target.addr);
@@ -2916,10 +2915,6 @@ void virDomainChrDefFree(virDomainChrDef *def)
 g_free(def->target.name);
 break;
 }
-break;
-
-default:
-break;
 }
 
 virObjectUnref(def->source);
-- 
2.35.3

[PATCH 4/9] domain_conf: remove breaks after return in virDomainChrSourceDefIsEqual()

[Kristina Hanicova]

Signed-off-by: Kristina Hanicova 
---
 src/conf/domain_conf.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 26a241db38..b903dac1cb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2844,21 +2844,22 @@ virDomainChrSourceDefIsEqual(const 
virDomainChrSourceDef *src,
 case VIR_DOMAIN_CHR_TYPE_FILE:
 return src->data.file.append == tgt->data.file.append &&
 STREQ_NULLABLE(src->data.file.path, tgt->data.file.path);
-break;
+
 case VIR_DOMAIN_CHR_TYPE_PTY:
 case VIR_DOMAIN_CHR_TYPE_DEV:
 case VIR_DOMAIN_CHR_TYPE_PIPE:
 return STREQ_NULLABLE(src->data.file.path, tgt->data.file.path);
+
 case VIR_DOMAIN_CHR_TYPE_NMDM:
 return STREQ_NULLABLE(src->data.nmdm.master, tgt->data.nmdm.master) &&
 STREQ_NULLABLE(src->data.nmdm.slave, tgt->data.nmdm.slave);
-break;
+
 case VIR_DOMAIN_CHR_TYPE_UDP:
 return STREQ_NULLABLE(src->data.udp.bindHost, tgt->data.udp.bindHost) 
&&
 STREQ_NULLABLE(src->data.udp.bindService, 
tgt->data.udp.bindService) &&
 STREQ_NULLABLE(src->data.udp.connectHost, 
tgt->data.udp.connectHost) &&
 STREQ_NULLABLE(src->data.udp.connectService, 
tgt->data.udp.connectService);
-break;
+
 case VIR_DOMAIN_CHR_TYPE_TCP:
 return src->data.tcp.listen == tgt->data.tcp.listen &&
 src->data.tcp.protocol == tgt->data.tcp.protocol &&
@@ -2866,18 +2867,16 @@ virDomainChrSourceDefIsEqual(const 
virDomainChrSourceDef *src,
 STREQ_NULLABLE(src->data.tcp.service, tgt->data.tcp.service) &&
 src->data.tcp.reconnect.enabled == tgt->data.tcp.reconnect.enabled 
&&
 src->data.tcp.reconnect.timeout == tgt->data.tcp.reconnect.timeout;
-break;
+
 case VIR_DOMAIN_CHR_TYPE_UNIX:
 return src->data.nix.listen == tgt->data.nix.listen &&
 STREQ_NULLABLE(src->data.nix.path, tgt->data.nix.path) &&
 src->data.nix.reconnect.enabled == tgt->data.nix.reconnect.enabled 
&&
 src->data.nix.reconnect.timeout == tgt->data.nix.reconnect.timeout;
-break;
 
 case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
 return STREQ_NULLABLE(src->data.spiceport.channel,
   tgt->data.spiceport.channel);
-break;
 
 case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
 return src->data.spicevmc == tgt->data.spicevmc;
@@ -2885,12 +2884,10 @@ virDomainChrSourceDefIsEqual(const 
virDomainChrSourceDef *src,
 case VIR_DOMAIN_CHR_TYPE_QEMU_VDAGENT:
 return src->data.qemuVdagent.clipboard == 
tgt->data.qemuVdagent.clipboard &&
 src->data.qemuVdagent.mouse == tgt->data.qemuVdagent.mouse;
-break;
 
 case VIR_DOMAIN_CHR_TYPE_DBUS:
 return STREQ_NULLABLE(src->data.dbus.channel,
   tgt->data.dbus.channel);
-break;
 
 case VIR_DOMAIN_CHR_TYPE_NULL:
 case VIR_DOMAIN_CHR_TYPE_VC:
-- 
2.35.3

[PATCH 7/9] domain_conf: use early return in virDomainObjAssignDef()

[Kristina Hanicova]

Signed-off-by: Kristina Hanicova 
---
 src/conf/domain_conf.c | 32 +---
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 88d50d27f5..1c29a2d929 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3918,22 +3918,24 @@ void virDomainObjAssignDef(virDomainObj *domain,
 else
 virDomainDefFree(domain->newDef);
 domain->newDef = g_steal_pointer(def);
-} else {
-if (live) {
-/* save current configuration to be restored on domain shutdown */
-if (!domain->newDef)
-domain->newDef = domain->def;
-else
-virDomainDefFree(domain->def);
-domain->def = g_steal_pointer(def);
-} else {
-if (oldDef)
-*oldDef = domain->def;
-else
-virDomainDefFree(domain->def);
-domain->def = g_steal_pointer(def);
-}
+return;
+}
+
+if (live) {
+/* save current configuration to be restored on domain shutdown */
+if (!domain->newDef)
+domain->newDef = domain->def;
+else
+virDomainDefFree(domain->def);
+domain->def = g_steal_pointer(def);
+return;
 }
+
+if (oldDef)
+*oldDef = domain->def;
+else
+virDomainDefFree(domain->def);
+domain->def = g_steal_pointer(def);
 }
 
 
-- 
2.35.3

[PATCH 0/9] domain_conf & domain_capabilities: small refactoring

[Kristina Hanicova]

*** BLURB HERE ***

Kristina Hanicova (9):
  domain_capabilities: use early return in
virDomainCapsFeatureSEVFormat()
  domain_capabilities: reformat virDomainCapsFeatureSEVFormat()
  domain_capabilities: reformat virDomainCapsCPUCustomFormat()
  domain_conf: remove breaks after return in
virDomainChrSourceDefIsEqual()
  domain_conf: replace switch with if in virDomainChrDefFree()
  domain_conf: rewrite virDomainSoundCodecDefFree()
  domain_conf: use early return in virDomainObjAssignDef()
  domain_conf: rewrite conditions in virDomainObjWaitUntil()
  domain_conf: rewrite variable setting to ternary operator

 src/conf/domain_capabilities.c | 37 ---
 src/conf/domain_conf.c | 82 +++---
 2 files changed, 53 insertions(+), 66 deletions(-)

-- 
2.35.3

[PATCH 9/9] domain_conf: rewrite variable setting to ternary operator

[Kristina Hanicova]

Signed-off-by: Kristina Hanicova 
---
 src/conf/domain_conf.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e52f39c809..b600bfec31 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4206,12 +4206,8 @@ virDomainObjGetOneDefState(virDomainObj *vm,
 if (virDomainObjUpdateModificationImpact(vm, ) < 0)
 return NULL;
 
-if (live) {
-if (flags & VIR_DOMAIN_AFFECT_LIVE)
-*live = true;
-else
-*live = false;
-}
+if (live)
+*live = (flags & VIR_DOMAIN_AFFECT_LIVE) ? true : false;
 
 if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG)
 return vm->newDef;
-- 
2.35.3

Re: [PATCH] schemas: Update ref acpi for devices

[Michal Prívozník]

On 7/19/22 09:02, Han Han wrote:
> According to a9fe9569ab, the  is only for PCI
> devices. Remove the ref acpi from devices channel, smartcard, tpm,
> redirdev, panic, hub because none of them has PCI address. And add the
> ref acpi to iommu device.
> 
> Fixes: a9fe9569ab
> 
> Signed-off-by: Han Han 
> ---
>  src/conf/schemas/domaincommon.rng | 21 +++--
>  1 file changed, 3 insertions(+), 18 deletions(-)

Reviewed-by: Michal Privoznik 

Michal

[PATCH] domain_conf: remove else after return / goto

[Kristina Hanicova]

Signed-off-by: Kristina Hanicova 
---
 src/conf/domain_conf.c | 55 --
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 44a01ab628..bc4b74c1c8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4084,8 +4084,8 @@ virDomainObjGetPersistentDef(virDomainXMLOption *xmlopt,
 
 if (domain->newDef)
 return domain->newDef;
-else
-return domain->def;
+
+return domain->def;
 }
 
 
@@ -4223,8 +4223,8 @@ virDomainObjGetOneDefState(virDomainObj *vm,
 
 if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG)
 return vm->newDef;
-else
-return vm->def;
+
+return vm->def;
 }
 
 
@@ -6029,7 +6029,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
  VIR_XML_PROP_NONZERO,
  >sgio)) < 0) {
 return -1;
-} else if (rv > 0) {
+}
+
+if (rv > 0) {
 if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("sgio is only supported for scsi host device"));
@@ -6041,8 +6043,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
  VIR_XML_PROP_NONE,
  >rawio)) < 0) {
 return -1;
-} else if (rv > 0 &&
-   def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) 
{
+}
+
+if (rv > 0 && def->source.subsys.type != 
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("rawio is only supported for scsi host device"));
 return -1;
@@ -8056,15 +8059,15 @@ virDomainControllerModelTypeFromString(const 
virDomainControllerDef *def,
 {
 if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
 return virDomainControllerModelSCSITypeFromString(model);
-else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
+if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
 return virDomainControllerModelUSBTypeFromString(model);
-else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
+if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
 return virDomainControllerModelPCITypeFromString(model);
-else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
+if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
 return virDomainControllerModelIDETypeFromString(model);
-else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
+if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
 return virDomainControllerModelVirtioSerialTypeFromString(model);
-else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
+if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
 return virDomainControllerModelISATypeFromString(model);
 
 return -1;
@@ -8077,15 +8080,15 @@ 
virDomainControllerModelTypeToString(virDomainControllerDef *def,
 {
 if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
 return virDomainControllerModelSCSITypeToString(model);
-else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
+if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
 return virDomainControllerModelUSBTypeToString(model);
-else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
+if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
 return virDomainControllerModelPCITypeToString(model);
-else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
+if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
 return virDomainControllerModelIDETypeToString(model);
-else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
+if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
 return virDomainControllerModelVirtioSerialTypeToString(model);
-else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
+if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
 return virDomainControllerModelISATypeToString(model);
 
 return NULL;
@@ -9915,9 +9918,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def,
 
 ctxt->node = cur;
 
-if ((nsources = virXPathNodeSet("./source", ctxt, )) < 0) {
+if ((nsources = virXPathNodeSet("./source", ctxt, )) < 0)
 goto error;
-} else if (nsources > 0) {
+
+if (nsources > 0) {
 /* Parse only the first source element since only one is used
  * for chardev devices, the only exception is UDP type, where
  * user can specify two source elements. */
@@ -9926,7 +9930,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def,
_("only one source element is allowed for "
  "character device"));
 goto error;
-} else if (nsources > 2) {
+}
+if (nsources > 2) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("only two source 

Re: [PATCH] domain_conf: rewrite if else condition

[Michal Prívozník]

On 7/20/22 14:42, Kristina Hanicova wrote:
> This patch prevents nesting of if conditions and makes the code
> cleaner.
> 
> Signed-off-by: Kristina Hanicova 
> ---
>  src/conf/domain_conf.c | 50 +-
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 44a01ab628..6b81c61056 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5309,18 +5309,18 @@ virDomainDeviceAddressParseXML(xmlNodePtr address,
>  {
>  g_autofree char *type = virXMLPropString(address, "type");
>  
> -if (type) {
> -if ((info->type = virDomainDeviceAddressTypeFromString(type)) <= 0) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("unknown address type '%s'"), type);
> -return -1;
> -}
> -} else {
> +if (!type) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("No type specified for device address"));
>  return -1;
>  }
>  
> +if ((info->type = virDomainDeviceAddressTypeFromString(type)) <= 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("unknown address type '%s'"), type);
> +return -1;
> +}
> +
>  switch ((virDomainDeviceAddressType) info->type) {
>  case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
>  if (virPCIDeviceAddressParseXML(address, >addr.pci) < 0)
> @@ -5996,20 +5996,20 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>   * .  (the functions we're going to call expect address
>   * type to already be known).
>   */
> -if (type) {
> -if ((def->source.subsys.type
> - = virDomainHostdevSubsysTypeFromString(type)) < 0) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("unknown host device source address type '%s'"),
> -   type);
> -return -1;
> -}
> -} else {
> +if (!type) {
>  virReportError(VIR_ERR_XML_ERROR,
> "%s", _("missing source address type"));
>  return -1;
>  }
>  
> +if ((def->source.subsys.type
> + = virDomainHostdevSubsysTypeFromString(type)) < 0) {

Here, and 

> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("unknown host device source address type '%s'"),
> +   type);
> +return -1;
> +}
> +
>  if (!(sourcenode = virXPathNode("./source", ctxt))) {
>  virReportError(VIR_ERR_XML_ERROR, "%s",
> _("Missing  element in hostdev device"));
> @@ -6304,20 +6304,20 @@ virDomainHostdevDefParseXMLCaps(xmlNodePtr node 
> G_GNUC_UNUSED,
>   * .  (the functions we're going to call expect address
>   * type to already be known).
>   */
> -if (type) {
> -if ((def->source.caps.type
> - = virDomainHostdevCapsTypeFromString(type)) < 0) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("unknown host device source address type '%s'"),
> -   type);
> -return -1;
> -}
> -} else {
> +if (!type) {
>  virReportError(VIR_ERR_XML_ERROR,
> "%s", _("missing source address type"));
>  return -1;
>  }
>  
> +if ((def->source.caps.type
> + = virDomainHostdevCapsTypeFromString(type)) < 0) {

.. here I'd rather have a slightly longer line than this. I'll fix that
before pushing.

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH 0/5] hypervisor: move job object into hypervisor

[Michal Prívozník]

On 7/19/22 15:48, Kristina Hanicova wrote:
> This series moves generalized qemu job object into hypervisor/ and
> replaces all other hypervisor-specific job objects.
> 
> Kristina Hanicova (5):
>   qemu & hypervisor: move job object into hypervisor
>   hypervisor: domain_job: rename members in
> virDomainObjPrivateJobCallbacks
>   LXC: use virDomainJobObj
>   libxl: use virDomainJobObj
>   CH: use virDomainJobObj
> 
>  src/ch/ch_domain.c   |  4 +-
>  src/ch/ch_domain.h   |  9 +
>  src/hypervisor/domain_job.h  | 62 ++
>  src/libxl/libxl_domain.c |  6 +--
>  src/libxl/libxl_domain.h | 11 +-
>  src/lxc/lxc_domain.c |  4 +-
>  src/lxc/lxc_domain.h |  9 +
>  src/qemu/qemu_domain.c   | 10 ++---
>  src/qemu/qemu_domain.h   |  2 +-
>  src/qemu/qemu_domainjob.c| 26 ++---
>  src/qemu/qemu_domainjob.h| 66 +++-
>  src/qemu/qemu_migration.c|  2 +-
>  src/qemu/qemu_migration.h|  2 +-
>  src/qemu/qemu_migration_params.c |  2 +-
>  src/qemu/qemu_process.c  | 12 +++---
>  15 files changed, 106 insertions(+), 121 deletions(-)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH] hypervisor: domain_job: add and edit description

[Michal Prívozník]

On 7/19/22 14:52, Kristina Hanicova wrote:
> Signed-off-by: Kristina Hanicova 
> ---
>  src/hypervisor/domain_job.c | 2 ++
>  src/hypervisor/domain_job.h | 5 -
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 

Reviewed-by: Michal Privoznik 

Michal

[PATCH] domain_conf: rewrite if else condition

[Kristina Hanicova]

This patch prevents nesting of if conditions and makes the code
cleaner.

Signed-off-by: Kristina Hanicova 
---
 src/conf/domain_conf.c | 50 +-
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 44a01ab628..6b81c61056 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5309,18 +5309,18 @@ virDomainDeviceAddressParseXML(xmlNodePtr address,
 {
 g_autofree char *type = virXMLPropString(address, "type");
 
-if (type) {
-if ((info->type = virDomainDeviceAddressTypeFromString(type)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown address type '%s'"), type);
-return -1;
-}
-} else {
+if (!type) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("No type specified for device address"));
 return -1;
 }
 
+if ((info->type = virDomainDeviceAddressTypeFromString(type)) <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unknown address type '%s'"), type);
+return -1;
+}
+
 switch ((virDomainDeviceAddressType) info->type) {
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
 if (virPCIDeviceAddressParseXML(address, >addr.pci) < 0)
@@ -5996,20 +5996,20 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
  * .  (the functions we're going to call expect address
  * type to already be known).
  */
-if (type) {
-if ((def->source.subsys.type
- = virDomainHostdevSubsysTypeFromString(type)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown host device source address type '%s'"),
-   type);
-return -1;
-}
-} else {
+if (!type) {
 virReportError(VIR_ERR_XML_ERROR,
"%s", _("missing source address type"));
 return -1;
 }
 
+if ((def->source.subsys.type
+ = virDomainHostdevSubsysTypeFromString(type)) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unknown host device source address type '%s'"),
+   type);
+return -1;
+}
+
 if (!(sourcenode = virXPathNode("./source", ctxt))) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("Missing  element in hostdev device"));
@@ -6304,20 +6304,20 @@ virDomainHostdevDefParseXMLCaps(xmlNodePtr node 
G_GNUC_UNUSED,
  * .  (the functions we're going to call expect address
  * type to already be known).
  */
-if (type) {
-if ((def->source.caps.type
- = virDomainHostdevCapsTypeFromString(type)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown host device source address type '%s'"),
-   type);
-return -1;
-}
-} else {
+if (!type) {
 virReportError(VIR_ERR_XML_ERROR,
"%s", _("missing source address type"));
 return -1;
 }
 
+if ((def->source.caps.type
+ = virDomainHostdevCapsTypeFromString(type)) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unknown host device source address type '%s'"),
+   type);
+return -1;
+}
+
 if (!virXPathNode("./source", ctxt)) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("Missing  element in hostdev device"));
-- 
2.35.3

Re: [libvirt PATCH] nodedev: support 'mtty' device for testing

[Michal Prívozník]

On 7/19/22 18:33, Jonathon Jongsma wrote:
> It would be nice to be able to test the mediated device capabilities
> without having physical hardware which supports it. The 'mtty' kernel
> module presents a virtual parent device which is capable of creating
> 'fake' mediated devices, and as such it would be useful for testing.
> 
> However, the 'mtty' device is not part of an existing device subsystem
> (e.g. PCI, etc), so libvirt ignores it and it does not get added to the
> node device list. And because it does not get added to the node device
> list, it cannot be used to create child mdevs using `virsh
> nodedev-create`.
> 
> There is already a node device type capability
> VIR_NODE_DEV_CAP_MDEV_TYPES that indicates whether a device supports
> creating child mediated devices, but libvirt assumes that this is a
> nested capability (in other words, it assumes that the primary
> capability of a device is something like PCI). If we allow this
> MDEV_TYPES capability to be a primary device capability, then we can
> support virtual devices like 'mtty' as a parent for mediated devices.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=2107031
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  src/conf/node_device_conf.c  | 41 +++-
>  src/conf/node_device_conf.h  | 13 +
>  src/libvirt_private.syms |  2 ++
>  src/node_device/node_device_driver.c |  5 +++-
>  src/node_device/node_device_udev.c   | 25 +
>  src/util/virmdev.c   | 28 +++
>  src/util/virmdev.h   |  4 +++
>  7 files changed, 116 insertions(+), 2 deletions(-)

Reviewed-by: Michal Privoznik 

Michal

[libvirt PATCH v3 3/4] qemu_migration: get migration blockers before hardcoded checks

[Eugenio Pérez]

since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
will return an array of error strings describing the migration blockers.
This can be used to check whether there are any devices blocking
migration, etc.

Enable qemuMigrationSrcIsAllowed to query it.

Signed-off-by: Eugenio Pérez 
---
v3:
* Report message with a colon.
* Report all blockers instead of only the first.
---
 src/qemu/qemu_migration.c | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index b12cb518ee..6ac4ef150b 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef *def)
 return true;
 }
 
+static int
+qemuDomainGetMigrationBlockers(virQEMUDriver *driver,
+   virDomainObj *vm,
+   char ***blockers)
+{
+qemuDomainObjPrivate *priv = vm->privateData;
+int rc;
+
+qemuDomainObjEnterMonitor(driver, vm);
+rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers);
+qemuDomainObjExitMonitor(vm);
+
+return rc;
+}
 
 /**
  * qemuMigrationSrcIsAllowed:
@@ -1439,6 +1453,26 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
 int nsnapshots;
 int pauseReason;
 size_t i;
+int r;
+
+/* Ask qemu if it have a migration blocker */
+if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
+g_auto(GStrv) blockers = NULL;
+r = qemuDomainGetMigrationBlockers(driver, vm, );
+if (r != 0) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("cannot migrate domain: %s"),
+   _("error getting blockers"));
+return false;
+}
+
+if (blockers && blockers[0]) {
+g_autofree char *reasons = g_strjoinv(", ", blockers);
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("cannot migrate domain: %s"), reasons);
+return false;
+}
+}
 
 /* perform these checks only when migrating to remote hosts */
 if (remote) {
-- 
2.31.1

[libvirt PATCH v3 0/4] Ask qemu about migration blockers

[Eugenio Pérez]

There are some hardcoded migration blockers in libvirt, like having a net
vhost-vdpa device. While it's true that they cannot be migrated at the moment,
there are plans to be able to migrate them soon.

They'll put a migration blockers in the cases where you cannot migrate them.
Ask qemu about then before rejecting the migration. In case the question is not
possible, assume they're not migratable.

v3:
* Return ok in qemuMonitorJSONGetMigrationBlockers is there are no
  blockers.
* Fix indentation
* Report all blockers instead of only the first.
* Squash some patches
* Move note to function doc.
* s/QEMU_MIGRATION_BLOCKED_REASONS/QEMU_CAPS_MIGRATION_BLOCKED_REASONS/

v2:
* Ask qemu if it has some pending blockers before try the migration

Eugenio Pérez (3):
  qemu_monitor: add support for get qemu migration blockers
  qemu_migration: get migration blockers before hardcoded checks
  qemu_migration: Do not forbid vDPA devices if can query blockers

Jonathon Jongsma (1):
  qemu: introduce capability QEMU_CAPS_MIGRATION_BLOCKED_REASONS

 src/qemu/qemu_capabilities.c  |  2 +
 src/qemu/qemu_capabilities.h  |  1 +
 src/qemu/qemu_migration.c | 38 +++-
 src/qemu/qemu_monitor.c   | 11 +
 src/qemu/qemu_monitor.h   |  4 ++
 src/qemu/qemu_monitor_json.c  | 43 +++
 src/qemu/qemu_monitor_json.h  |  3 ++
 .../caps_6.0.0.aarch64.xml|  1 +
 .../qemucapabilitiesdata/caps_6.0.0.s390x.xml |  1 +
 .../caps_6.0.0.x86_64.xml |  1 +
 .../caps_6.1.0.x86_64.xml |  1 +
 .../caps_6.2.0.aarch64.xml|  1 +
 .../qemucapabilitiesdata/caps_6.2.0.ppc64.xml |  1 +
 .../caps_6.2.0.x86_64.xml |  1 +
 .../caps_7.0.0.aarch64.xml|  1 +
 .../qemucapabilitiesdata/caps_7.0.0.ppc64.xml |  1 +
 .../caps_7.0.0.x86_64.xml |  1 +
 .../caps_7.1.0.x86_64.xml |  1 +
 18 files changed, 112 insertions(+), 1 deletion(-)

-- 
2.31.1

[libvirt PATCH v3 1/4] qemu: introduce capability QEMU_CAPS_MIGRATION_BLOCKED_REASONS

[Eugenio Pérez]

From: Jonathon Jongsma 

since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
will return an array of error strings describing the migration blockers.
This can be used to check whether there are any devices blocking
migration, etc.

Signed-off-by: Jonathon Jongsma 
Signed-off-by: Eugenio Pérez 
---
v3: s/QEMU_MIGRATION_BLOCKED_REASONS/QEMU_CAPS_MIGRATION_BLOCKED_REASONS/
---
 src/qemu/qemu_capabilities.c  | 2 ++
 src/qemu/qemu_capabilities.h  | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml   | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_7.0.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_7.0.0.ppc64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml  | 1 +
 13 files changed, 14 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 30b396d32d..b002fb98ed 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -672,6 +672,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */
   "iothread.thread-pool-max", /* 
QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX */
   "usb-host.guest-resets-all", /* 
QEMU_CAPS_USB_HOST_GUESTS_RESETS_ALL */
+  "migration.blocked-reasons", /* 
QEMU_CAPS_MIGRATION_BLOCKED_REASONS */
 );
 
 
@@ -1622,6 +1623,7 @@ static struct virQEMUCapsStringFlags 
virQEMUCapsQMPSchemaQueries[] = {
 { "chardev-add/arg-type/backend/+qemu-vdagent", 
QEMU_CAPS_CHARDEV_QEMU_VDAGENT },
 { "query-display-options/ret-type/+dbus", QEMU_CAPS_DISPLAY_DBUS },
 { "object-add/arg-type/+iothread/thread-pool-max", 
QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX },
+{ "query-migrate/ret-type/blocked-reasons", 
QEMU_CAPS_MIGRATION_BLOCKED_REASONS },
 };
 
 typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index d979a5ba3b..8f3090e2ce 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -651,6 +651,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_DISPLAY_DBUS, /* -display dbus */
 QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX, /* -object iothread.thread-pool-max */
 QEMU_CAPS_USB_HOST_GUESTS_RESETS_ALL, /* -device usb-host.guest-resets-all 
*/
+QEMU_CAPS_MIGRATION_BLOCKED_REASONS, /* query-migrate returns 
'blocked-reasons */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml
index 01e30f4e02..4afd7b26ce 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml
@@ -187,6 +187,7 @@
   
   
   
+  
   600
   0
   61700242
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
index aa7b5deab5..c9cb85daa0 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
@@ -144,6 +144,7 @@
   
   
   
+  
   600
   0
   39100242
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
index d9e385ab1d..508804521c 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
@@ -229,6 +229,7 @@
   
   
   
+  
   600
   0
   43100242
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index 05f297dfa2..d4a540fafd 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -234,6 +234,7 @@
   
   
   
+  
   6001000
   0
   43100243
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml
index 9cb1a32354..71697fac95 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml
@@ -199,6 +199,7 @@
   
   
   
+  
   6001050
   0
   61700244
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml
index 5df148d787..3f86e03f18 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml
@@ -193,6 +193,7 @@
   
   
   
+  
   6002000
   0
   42900244
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
index dd011f8408..1a1a9643d4 

[libvirt PATCH v3 2/4] qemu_monitor: add support for get qemu migration blockers

[Eugenio Pérez]

since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
will return an array of error strings describing the migration blockers.
This can be used to check whether there are any devices blocking
migration, etc.

Enable qemu monitor to send this query.  This will allow
qemuMigrationSrcIsAllowed to dynamically ask for migration blockers,
reducing duplication.

Signed-off-by: Eugenio Pérez 
---
v3:
* Squash some patches
* Return ok in qemuMonitorJSONGetMigrationBlockers is there are no
  blockers.
* Move note to function doc.
---
 src/qemu/qemu_monitor.c  | 11 +
 src/qemu/qemu_monitor.h  |  4 
 src/qemu/qemu_monitor_json.c | 43 
 src/qemu/qemu_monitor_json.h |  3 +++
 4 files changed, 61 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 109107eaae..e0939beecd 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4486,3 +4486,14 @@ qemuMonitorMigrateRecover(qemuMonitor *mon,
 
 return qemuMonitorJSONMigrateRecover(mon, uri);
 }
+
+int
+qemuMonitorGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers)
+{
+VIR_DEBUG("blockers=%p", blockers);
+
+QEMU_CHECK_MONITOR(mon);
+
+return qemuMonitorJSONGetMigrationBlockers(mon, blockers);
+}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index cc1a0bc8c9..b82f198285 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1543,3 +1543,7 @@ qemuMonitorChangeMemoryRequestedSize(qemuMonitor *mon,
 int
 qemuMonitorMigrateRecover(qemuMonitor *mon,
   const char *uri);
+
+int
+qemuMonitorGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 5e4a86e5ad..6d15a458a3 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3338,6 +3338,49 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
 return 0;
 }
 
+/*
+ * Get the exposed migration blockers.
+ *
+ * This function assume qemu has the capability of request them.
+ *
+ * It returns a NULL terminated array on blockers if there are any, or it set
+ * it to NULL otherwise.
+ */
+int qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+virJSONValue *data;
+virJSONValue *jblockers;
+size_t i;
+
+*blockers = NULL;
+if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL)))
+return -1;
+
+if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
+return -1;
+
+if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
+return -1;
+
+data = virJSONValueObjectGetObject(reply, "return");
+
+if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons")))
+return 0;
+
+*blockers = g_new0(char *, virJSONValueArraySize(jblockers) + 1);
+for (i = 0; i < virJSONValueArraySize(jblockers); i++) {
+virJSONValue *jblocker = virJSONValueArrayGet(jblockers, i);
+const char *blocker = virJSONValueGetString(jblocker);
+
+(*blockers)[i] = g_strdup(blocker);
+}
+
+return 0;
+}
+
 int qemuMonitorJSONMigrateCancel(qemuMonitor *mon)
 {
 g_autoptr(virJSONValue) cmd = qemuMonitorJSONMakeCommand("migrate_cancel", 
NULL);
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 2759566892..e4c65e250e 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -184,6 +184,9 @@ qemuMonitorJSONMigrate(qemuMonitor *mon,
unsigned int flags,
const char *uri);
 int
+qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers);
+int
 qemuMonitorJSONGetSpiceMigrationStatus(qemuMonitor *mon,
bool *spice_migrated);
 
-- 
2.31.1

[libvirt PATCH v3 4/4] qemu_migration: Do not forbid vDPA devices if can query blockers

[Eugenio Pérez]

vDPA devices will be migratable soon. Since they are not migratable
before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
for migration blockers, let it hardcoded in that case.

Otherwise, ask qemu about the explicit blocker.

Signed-off-by: Eugenio Pérez 
---
v3: Fix indentation
---
 src/qemu/qemu_migration.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 6ac4ef150b..45e16242f0 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
 int pauseReason;
 size_t i;
 int r;
+bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
+
QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
 
 /* Ask qemu if it have a migration blocker */
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
+if (blockedReasonsCap) {
 g_auto(GStrv) blockers = NULL;
 r = qemuDomainGetMigrationBlockers(driver, vm, );
 if (r != 0) {
@@ -1467,7 +1469,7 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
 }
 
 if (blockers && blockers[0]) {
-g_autofree char *reasons = g_strjoinv(", ", blockers);
+g_autofree char *reasons = g_strjoinv("; ", blockers);
 virReportError(VIR_ERR_OPERATION_INVALID,
_("cannot migrate domain: %s"), reasons);
 return false;
@@ -1580,7 +1582,7 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
 virDomainNetDef *net = vm->def->nets[i];
 qemuSlirp *slirp;
 
-if (net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
+if (!blockedReasonsCap && net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("vDPA devices cannot be migrated"));
 return false;
-- 
2.31.1

Re: [libvirt PATCH v2 4/5] qemu_migration: get migration blockers before hardcoded checks

[Jiri Denemark]

On Wed, Jul 20, 2022 at 13:25:45 +0200, Eugenio Perez Martin wrote:
> On Wed, Jul 20, 2022 at 12:31 PM Jiri Denemark  wrote:
> >
> > On Wed, Jul 20, 2022 at 11:11:53 +0200, Eugenio Pérez wrote:
> > > since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> > > will return an array of error strings describing the migration blockers.
> > > This can be used to check whether there are any devices blocking
> > > migration, etc.
> > >
> > > Enable qemuMigrationSrcIsAllowed to query it.
> > >
> > > Signed-off-by: Eugenio Pérez 
> > > ---
> > >  src/qemu/qemu_migration.c | 33 +
> > >  1 file changed, 33 insertions(+)
> > >
> > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > index b12cb518ee..4224339f39 100644
> > > --- a/src/qemu/qemu_migration.c
> > > +++ b/src/qemu/qemu_migration.c
> > > @@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const 
> > > virDomainDef *def)
> > >  return true;
> > >  }
> > >
> > > +static int
> > > +qemuDomainGetMigrationBlockers(virQEMUDriver *driver,
> > > +   virDomainObj *vm,
> > > +   char ***blockers)
> > > +{
> > > +qemuDomainObjPrivate *priv = vm->privateData;
> > > +int rc;
> > > +
> > > +qemuDomainObjEnterMonitor(driver, vm);
> > > +rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers);
> > > +qemuDomainObjExitMonitor(vm);
> > > +
> > > +return rc;
> > > +}
> > >
> > >  /**
> > >   * qemuMigrationSrcIsAllowed:
> > > @@ -1439,6 +1453,25 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> > >  int nsnapshots;
> > >  int pauseReason;
> > >  size_t i;
> > > +int r;
> > > +
> > > +/* Ask qemu if it have a migration blocker */
> > > +if (virQEMUCapsGet(priv->qemuCaps, 
> > > QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
> > > +g_auto(GStrv) blockers = NULL;
> > > +r = qemuDomainGetMigrationBlockers(driver, vm, );
> > > +if (r != 0) {
> > > +virReportError(VIR_ERR_OPERATION_INVALID,
> > > +   _("cannot migrate domain, %s"),
> > > +   _("error getting blockers"));
> >
> > If qemuDomainGetMigrationBlockers returned -1 a better error message
> > should already be set and you would overwrite it here. Also with your
> > current patch you would report this error even in case there was no
> > migration blocker reported by QEMU. But this part would be fixed by
> > letting qemuDomainGetMigrationBlockers return 0 when the blocked-reasons
> > field is missing.
> >
> 
> Got it.
> 
> > > +return false;
> > > +}
> > > +
> > > +if (blockers[0]) {
> >
> > blockers && blockers[0]
> >
> > > +virReportError(VIR_ERR_OPERATION_INVALID,
> > > +   _("cannot migrate domain, %s"), blockers[0]);
> >
> > I would prefer a colon there: "cannot migrate domain: %s". And we got a
> > list of blockers from QEMU, but only use the first one? Please, join
> > them with g_strjoinv().
> >
> 
> So the resulting message should be:
> "cannot migrate domain: , , ..., ", right?

Yeah, something like that... or with "; " as a separator to avoid
confusion in case any migration blocker contains ','.

Jirka

Re: [libvirt][PATCH v13 2/6] Get SGX capabilities form QMP

[Michal Prívozník]

On 7/1/22 21:14, Lin Yang wrote:
> From: Haibin Huang 
> 
> Generate the QMP command for query-sgx-capabilities and the command
> return sgx capabilities from QMP.
> 
> {"execute":"query-sgx-capabilities"}
> 
> the right reply:
>   {"return":
> {
>   "sgx": true,
>   "section-size": 197132288,
>   "flc": true
> }
>   }
> 
> the error reply:
>   {"error":
> {"class": "GenericError", "desc": "SGX is not enabled in KVM"}
>   }
> 
> Signed-off-by: Michal Privoznik 
> Signed-off-by: Haibin Huang 
> ---
>  src/qemu/qemu_monitor.c  |  10 
>  src/qemu/qemu_monitor.h  |   3 +
>  src/qemu/qemu_monitor_json.c | 113 +++
>  src/qemu/qemu_monitor_json.h |   4 ++
>  4 files changed, 130 insertions(+)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index fda5d2f368..a1b2138921 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3653,6 +3653,16 @@ qemuMonitorGetSEVCapabilities(qemuMonitor *mon,
>  }
>  
>  
> +int
> +qemuMonitorGetSGXCapabilities(qemuMonitor *mon,
> +  virSGXCapability **capabilities)
> +{
> +QEMU_CHECK_MONITOR(mon);
> +
> +return qemuMonitorJSONGetSGXCapabilities(mon, capabilities);
> +}
> +
> +
>  int
>  qemuMonitorNBDServerStart(qemuMonitor *mon,
>const virStorageNetHostDef *server,
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 95267ec6c7..451ac8eec9 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -864,6 +864,9 @@ int qemuMonitorGetGICCapabilities(qemuMonitor *mon,
>  int qemuMonitorGetSEVCapabilities(qemuMonitor *mon,
>virSEVCapability **capabilities);
>  
> +int qemuMonitorGetSGXCapabilities(qemuMonitor *mon,
> +  virSGXCapability **capabilities);
> +
>  typedef enum {
>QEMU_MONITOR_MIGRATE_BACKGROUND   = 1 << 0,
>QEMU_MONITOR_MIGRATE_NON_SHARED_DISK  = 1 << 1, /* migration with 
> non-shared storage with full disk copy */
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 3aad2ab212..c900956f82 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -6445,6 +6445,119 @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitor *mon,
>  return 1;
>  }
>  
> +
> +/**
> + * qemuMonitorJSONGetSGXCapabilities:
> + * @mon: qemu monitor object
> + * @capabilities: pointer to pointer to a SGX capability structure to be 
> filled
> + *
> + * This function queries and fills in INTEL's SGX platform-specific data.
> + * Note that from QEMU's POV both -object sgx-epc and query-sgx-capabilities
> + * can be present even if SGX is not available, which basically leaves us 
> with
> + * checking for JSON "GenericError" in order to differentiate between 
> compiled-in
> + * support and actual SGX support on the platform.
> + *
> + * Returns: -1 on error,
> + *   0 if SGX is not supported, and
> + *   1 if SGX is supported on the platform.
> + */
> +int
> +qemuMonitorJSONGetSGXCapabilities(qemuMonitor *mon,
> +  virSGXCapability **capabilities)
> +{
> +g_autoptr(virJSONValue) cmd = NULL;
> +g_autoptr(virJSONValue) reply = NULL;
> +g_autoptr(virSGXCapability) capability = NULL;
> +
> +virJSONValue *sections = NULL;
> +virJSONValue *caps;
> +bool flc = false;
> +bool sgx1 = false;
> +bool sgx2 = false;
> +unsigned long long section_size = 0;
> +unsigned long long size;
> +size_t i;
> +
> +*capabilities = NULL;
> +capability = g_new0(virSGXCapability, 1);
> +
> +if (!(cmd = qemuMonitorJSONMakeCommand("query-sgx-capabilities", NULL)))
> +return -1;
> +
> +if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> +return -1;
> +
> +/* QEMU has only compiled-in support of SGX */
> +if (qemuMonitorJSONHasError(reply, "GenericError"))
> +return 0;
> +
> +if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> +return -1;
> +
> +caps = virJSONValueObjectGetObject(reply, "return");
> +
> +if (virJSONValueObjectGetBoolean(caps, "flc", ) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("query-sgx-capabilities reply was missing 'flc' 
> field"));
> +return -1;
> +}
> +capability->flc = flc;
> +
> +if (virJSONValueObjectGetBoolean(caps, "sgx1", ) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("query-sgx-capabilities reply was missing 'sgx1' 
> field"));
> +return -1;
> +}
> +capability->sgx1 = sgx1;
> +
> +if (virJSONValueObjectGetBoolean(caps, "sgx2", ) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("query-sgx-capabilities reply was missing 'sgx2' 
> field"));
> +return -1;
> +}
> +capability->sgx2 = sgx2;
> +
> +if 

Re: [libvirt][PATCH v13 1/6] Define SGX capabilities structs

[Michal Prívozník]

On 7/1/22 21:14, Lin Yang wrote:
> From: Haibin Huang 
> 
> Signed-off-by: Michal Privoznik 
> Signed-off-by: Haibin Huang 
> ---
>  src/conf/domain_capabilities.c | 10 ++
>  src/conf/domain_capabilities.h | 24 
>  src/libvirt_private.syms   |  1 +
>  3 files changed, 35 insertions(+)
> 
> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
> index 895e8d00e8..27f3fb8d36 100644
> --- a/src/conf/domain_capabilities.c
> +++ b/src/conf/domain_capabilities.c
> @@ -76,6 +76,16 @@ virSEVCapabilitiesFree(virSEVCapability *cap)
>  }
>  
>  
> +void
> +virSGXCapabilitiesFree(virSGXCapability *cap)
> +{
> +if (!cap)
> +return;
> +

This leaks cap->pSections.

> +g_free(cap);
> +}
> +
> +
>  static void
>  virDomainCapsDispose(void *obj)
>  {
> diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
> index f2eed80b15..dac1098e98 100644
> --- a/src/conf/domain_capabilities.h
> +++ b/src/conf/domain_capabilities.h
> @@ -192,6 +192,24 @@ struct _virSEVCapability {
>  unsigned int max_es_guests;
>  };
>  
> +typedef struct _virSection virSection;
> +typedef virSection *virSectionPtr;

No, if we can help it I'd rather avoid introducing virXXXPtr typedef.
We've worked hard to get rid of them and I don't quite see a reason to
reintroduce them.

> +struct _virSection {
> +unsigned long long size;
> +unsigned int node;

While it's true that QEMU with its current code does not ever report a
negative number for 'node', at the QAPI level it's declared as signed
integer and thus we ought to reflect that.

> +};
> +
> +typedef struct _virSGXCapability virSGXCapability;
> +typedef virSGXCapability *virSGXCapabilityPtr;
> +struct _virSGXCapability {
> +bool flc;
> +bool sgx1;
> +bool sgx2;
> +unsigned long long section_size;
> +size_t nSections;
> +virSectionPtr pSections;

We tend to use: 'nitems' and 'items', or 'nsections' and 'sections' in
cases like this.

> +};
> +

Michal

Re: [libvirt][PATCH v13 3/6] Convert QMP capabilities to domain capabilities

[Michal Prívozník]

On 7/1/22 21:14, Lin Yang wrote:
> From: Haibin Huang 
> 
> the QMP capabilities:
>   {"return":
> {
>   "sgx": true,
>   "section-size": 1024,
>   "flc": true
> }
>   }
> 
> the domain capabilities:
>   
> yes
> 1
>   
> 
> Signed-off-by: Michal Privoznik 
> Signed-off-by: Haibin Huang 
> ---
>  src/qemu/qemu_capabilities.c  | 230 ++
>  src/qemu/qemu_capabilities.h  |   4 +
>  .../caps_6.2.0.x86_64.replies |  30 ++-
>  .../caps_6.2.0.x86_64.xml |   7 +
>  .../caps_7.0.0.x86_64.replies |  34 ++-
>  .../caps_7.0.0.x86_64.xml |  11 +
>  .../caps_7.1.0.x86_64.replies |  34 ++-
>  .../caps_7.1.0.x86_64.xml |  11 +
>  8 files changed, 346 insertions(+), 15 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 2c3be3ecec..57b5acb150 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -671,6 +671,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
>"chardev.qemu-vdagent", /* QEMU_CAPS_CHARDEV_QEMU_VDAGENT */
>"display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */
>"iothread.thread-pool-max", /* 
> QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX */
> +  "sgx-epc", /* QEMU_CAPS_SGX_EPC */
>  );
>  
>  
> @@ -752,6 +753,8 @@ struct _virQEMUCaps {
>  
>  virSEVCapability *sevCapabilities;
>  
> +virSGXCapability *sgxCapabilities;
> +
>  /* Capabilities which may differ depending on the accelerator. */
>  virQEMUCapsAccel kvm;
>  virQEMUCapsAccel hvf;
> @@ -1394,6 +1397,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] 
> = {
>  { "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
>  { "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
>  { "virtio-iommu-pci", QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI },
> +{ "sgx-epc", QEMU_CAPS_SGX_EPC },
>  };
>  
>  
> @@ -1972,6 +1976,32 @@ virQEMUCapsSEVInfoCopy(virSEVCapability **dst,
>  }
>  
>  
> +static int
> +virQEMUCapsSGXInfoCopy(virSGXCapability **dst,
> +   virSGXCapability *src)
> +{
> +g_autoptr(virSGXCapability) tmp = NULL;
> +

For a convenience of caller, we can have a src == NULL check here and
return early.

> +tmp = g_new0(virSGXCapability, 1);
> +
> +tmp->flc = src->flc;
> +tmp->sgx1 = src->sgx1;
> +tmp->sgx2 = src->sgx2;
> +tmp->section_size = src->section_size;
> +
> +if (src->nSections == 0) {
> +tmp->nSections = 0;
> +tmp->pSections = NULL;
> +} else {
> +tmp->nSections = src->nSections;
> +tmp->pSections = src->pSections;

Ouch, this will definitely lead to a double free. If I run
qemucapabilitiestest after this patch I can see it clearly. I don't
quite understand why you didn't though. Fortunately, we can use plain
memcpy() since virSection struct does not contain any pointer, just a
pair of integers.

> +}
> +
> +*dst = g_steal_pointer();
> +return 0;
> +}
> +
> +
>  static void
>  virQEMUCapsAccelCopyMachineTypes(virQEMUCapsAccel *dst,
>   virQEMUCapsAccel *src)
> @@ -2053,6 +2083,12 @@ virQEMUCaps *virQEMUCapsNewCopy(virQEMUCaps *qemuCaps)
> qemuCaps->sevCapabilities) < 0)
>  return NULL;
>  
> +
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC) &&
> +virQEMUCapsSGXInfoCopy(>sgxCapabilities,
> +   qemuCaps->sgxCapabilities) < 0)
> +return NULL;
> +
>  return g_steal_pointer();
>  }
>  
> @@ -2091,6 +2127,7 @@ void virQEMUCapsDispose(void *obj)
>  virCPUDataFree(qemuCaps->cpuData);
>  
>  virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
> +virSGXCapabilitiesFree(qemuCaps->sgxCapabilities);
>  
>  virQEMUCapsAccelClear(>kvm);
>  virQEMUCapsAccelClear(>hvf);
> @@ -2616,6 +2653,13 @@ virQEMUCapsGetSEVCapabilities(virQEMUCaps *qemuCaps)
>  }
>  
>  
> +virSGXCapabilityPtr
> +virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps)
> +{
> +return qemuCaps->sgxCapabilities;
> +}
> +
> +
>  static int
>  virQEMUCapsProbeQMPCommands(virQEMUCaps *qemuCaps,
>  qemuMonitor *mon)
> @@ -3442,6 +3486,31 @@ virQEMUCapsProbeQMPSEVCapabilities(virQEMUCaps 
> *qemuCaps,
>  }
>  
>  
> +static int
> +virQEMUCapsProbeQMPSGXCapabilities(virQEMUCaps *qemuCaps,
> +   qemuMonitor *mon)
> +{
> +int rc = -1;
> +virSGXCapability *caps = NULL;
> +
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC))
> +return 0;
> +
> +if ((rc = qemuMonitorGetSGXCapabilities(mon, )) < 0)
> +return -1;
> +
> +/* SGX isn't actually supported */
> +if (rc == 0) {
> +virQEMUCapsClear(qemuCaps, QEMU_CAPS_SGX_EPC);
> +return 0;
> +}
> +
> +virSGXCapabilitiesFree(qemuCaps->sgxCapabilities);
> +qemuCaps->sgxCapabilities = caps;

Re: [libvirt][PATCH v13 4/6] conf: expose SGX feature in domain capabilities

[Michal Prívozník]

On 7/1/22 21:14, Lin Yang wrote:
> From: Haibin Huang 
> 
> Extend hypervisor capabilities to include sgx feature. When available,
> the hypervisor supports launching an VM with SGX on Intel platfrom.
> The SGX feature tag privides additional details like section size and
> sgx1 or sgx2.
> 
> Signed-off-by: Michal Privoznik 
> Signed-off-by: Haibin Huang 
> ---
>  docs/formatdomaincaps.rst | 40 
>  src/conf/domain_capabilities.c| 48 +++
>  src/conf/schemas/domaincaps.rng   | 42 
>  src/qemu/qemu_capabilities.c  | 28 +++
>  tests/domaincapsdata/bhyve_basic.x86_64.xml   |  1 +
>  tests/domaincapsdata/bhyve_fbuf.x86_64.xml|  1 +
>  tests/domaincapsdata/bhyve_uefi.x86_64.xml|  1 +
>  tests/domaincapsdata/empty.xml|  1 +
>  tests/domaincapsdata/libxl-xenfv.xml  |  1 +
>  tests/domaincapsdata/libxl-xenpv.xml  |  1 +
>  .../domaincapsdata/qemu_2.11.0-q35.x86_64.xml |  1 +
>  .../domaincapsdata/qemu_2.11.0-tcg.x86_64.xml |  1 +
>  tests/domaincapsdata/qemu_2.11.0.s390x.xml|  1 +
>  tests/domaincapsdata/qemu_2.11.0.x86_64.xml   |  1 +
>  .../domaincapsdata/qemu_2.12.0-q35.x86_64.xml |  1 +
>  .../domaincapsdata/qemu_2.12.0-tcg.x86_64.xml |  1 +
>  .../qemu_2.12.0-virt.aarch64.xml  |  1 +
>  tests/domaincapsdata/qemu_2.12.0.aarch64.xml  |  1 +
>  tests/domaincapsdata/qemu_2.12.0.ppc64.xml|  1 +
>  tests/domaincapsdata/qemu_2.12.0.s390x.xml|  1 +
>  tests/domaincapsdata/qemu_2.12.0.x86_64.xml   |  1 +
>  .../domaincapsdata/qemu_3.0.0-q35.x86_64.xml  |  1 +
>  .../domaincapsdata/qemu_3.0.0-tcg.x86_64.xml  |  1 +
>  tests/domaincapsdata/qemu_3.0.0.ppc64.xml |  1 +
>  tests/domaincapsdata/qemu_3.0.0.s390x.xml |  1 +
>  tests/domaincapsdata/qemu_3.0.0.x86_64.xml|  1 +
>  .../domaincapsdata/qemu_3.1.0-q35.x86_64.xml  |  1 +
>  .../domaincapsdata/qemu_3.1.0-tcg.x86_64.xml  |  1 +
>  tests/domaincapsdata/qemu_3.1.0.ppc64.xml |  1 +
>  tests/domaincapsdata/qemu_3.1.0.x86_64.xml|  1 +
>  .../domaincapsdata/qemu_4.0.0-q35.x86_64.xml  |  1 +
>  .../domaincapsdata/qemu_4.0.0-tcg.x86_64.xml  |  1 +
>  .../qemu_4.0.0-virt.aarch64.xml   |  1 +
>  tests/domaincapsdata/qemu_4.0.0.aarch64.xml   |  1 +
>  tests/domaincapsdata/qemu_4.0.0.ppc64.xml |  1 +
>  tests/domaincapsdata/qemu_4.0.0.s390x.xml |  1 +
>  tests/domaincapsdata/qemu_4.0.0.x86_64.xml|  1 +
>  .../domaincapsdata/qemu_4.1.0-q35.x86_64.xml  |  1 +
>  .../domaincapsdata/qemu_4.1.0-tcg.x86_64.xml  |  1 +
>  tests/domaincapsdata/qemu_4.1.0.x86_64.xml|  1 +
>  .../domaincapsdata/qemu_4.2.0-q35.x86_64.xml  |  1 +
>  .../domaincapsdata/qemu_4.2.0-tcg.x86_64.xml  |  1 +
>  .../qemu_4.2.0-virt.aarch64.xml   |  1 +
>  tests/domaincapsdata/qemu_4.2.0.aarch64.xml   |  1 +
>  tests/domaincapsdata/qemu_4.2.0.ppc64.xml |  1 +
>  tests/domaincapsdata/qemu_4.2.0.s390x.xml |  1 +
>  tests/domaincapsdata/qemu_4.2.0.x86_64.xml|  1 +
>  .../domaincapsdata/qemu_5.0.0-q35.x86_64.xml  |  1 +
>  .../domaincapsdata/qemu_5.0.0-tcg.x86_64.xml  |  1 +
>  .../qemu_5.0.0-virt.aarch64.xml   |  1 +
>  tests/domaincapsdata/qemu_5.0.0.aarch64.xml   |  1 +
>  tests/domaincapsdata/qemu_5.0.0.ppc64.xml |  1 +
>  tests/domaincapsdata/qemu_5.0.0.x86_64.xml|  1 +
>  .../domaincapsdata/qemu_5.1.0-q35.x86_64.xml  |  1 +
>  .../domaincapsdata/qemu_5.1.0-tcg.x86_64.xml  |  1 +
>  tests/domaincapsdata/qemu_5.1.0.sparc.xml |  1 +
>  tests/domaincapsdata/qemu_5.1.0.x86_64.xml|  1 +
>  .../domaincapsdata/qemu_5.2.0-q35.x86_64.xml  |  1 +
>  .../domaincapsdata/qemu_5.2.0-tcg.x86_64.xml  |  1 +
>  .../qemu_5.2.0-virt.aarch64.xml   |  1 +
>  tests/domaincapsdata/qemu_5.2.0.aarch64.xml   |  1 +
>  tests/domaincapsdata/qemu_5.2.0.ppc64.xml |  1 +
>  tests/domaincapsdata/qemu_5.2.0.s390x.xml |  1 +
>  tests/domaincapsdata/qemu_5.2.0.x86_64.xml|  1 +
>  .../domaincapsdata/qemu_6.0.0-q35.x86_64.xml  |  1 +
>  .../domaincapsdata/qemu_6.0.0-tcg.x86_64.xml  |  1 +
>  .../qemu_6.0.0-virt.aarch64.xml   |  1 +
>  tests/domaincapsdata/qemu_6.0.0.aarch64.xml   |  1 +
>  tests/domaincapsdata/qemu_6.0.0.s390x.xml |  1 +
>  tests/domaincapsdata/qemu_6.0.0.x86_64.xml|  1 +
>  .../domaincapsdata/qemu_6.1.0-q35.x86_64.xml  |  1 +
>  .../domaincapsdata/qemu_6.1.0-tcg.x86_64.xml  |  1 +
>  tests/domaincapsdata/qemu_6.1.0.x86_64.xml|  1 +
>  .../domaincapsdata/qemu_6.2.0-q35.x86_64.xml  |  6 +++
>  .../domaincapsdata/qemu_6.2.0-tcg.x86_64.xml  |  6 +++
>  .../qemu_6.2.0-virt.aarch64.xml   |  1 +
>  tests/domaincapsdata/qemu_6.2.0.aarch64.xml   |  1 +
>  tests/domaincapsdata/qemu_6.2.0.ppc64.xml |  1 +
>  tests/domaincapsdata/qemu_6.2.0.x86_64.xml|  6 +++
>  .../domaincapsdata/qemu_7.0.0-q35.x86_64.xml  | 10 
>  .../domaincapsdata/qemu_7.0.0-tcg.x86_64.xml  | 10 
>  

Re: [libvirt][PATCH v13 5/6] conf: Introduce SGX EPC element into device memory xml

[Michal Prívozník]

On 7/1/22 21:14, Lin Yang wrote:
> With NUMA config:
> 
> 
>   ...
>   
> 
>   0-1
> 
> 
>   512
>   0
> 
>   
>   ...
> 
> 
> Without NUMA config:
> 
> 
>   ...
>   
> 
>   512
> 
>   
>   ...
> 
> 
> Signed-off-by: Lin Yang 
> Signed-off-by: Michal Privoznik 
> ---
>  docs/formatdomain.rst | 27 +++-
>  src/conf/domain_conf.c| 27 
>  src/conf/domain_conf.h|  1 +
>  src/conf/domain_validate.c|  9 +++
>  src/conf/schemas/domaincommon.rng |  1 +
>  src/qemu/qemu_alias.c |  3 +
>  src/qemu/qemu_command.c   |  1 +
>  src/qemu/qemu_domain.c| 48 ++
>  src/qemu/qemu_domain_address.c|  6 ++
>  src/qemu/qemu_driver.c|  1 +
>  src/qemu/qemu_process.c   |  2 +
>  src/qemu/qemu_validate.c  |  8 +++
>  src/security/security_apparmor.c  |  1 +
>  src/security/security_dac.c   |  2 +
>  src/security/security_selinux.c   |  2 +
>  tests/qemuxml2argvdata/sgx-epc-numa.xml   | 50 +++
>  tests/qemuxml2argvdata/sgx-epc.xml| 36 +++
>  .../sgx-epc-numa.x86_64-latest.xml| 64 +++
>  .../sgx-epc.x86_64-6.2.0.xml  | 52 +++
>  tests/qemuxml2xmltest.c   |  3 +
>  20 files changed, 329 insertions(+), 15 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/sgx-epc-numa.xml
>  create mode 100644 tests/qemuxml2argvdata/sgx-epc.xml
>  create mode 100644 tests/qemuxml2xmloutdata/sgx-epc-numa.x86_64-latest.xml
>  create mode 100644 tests/qemuxml2xmloutdata/sgx-epc.x86_64-6.2.0.xml
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 62a94890f0..b95c930d73 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -7910,6 +7910,20 @@ Example: usage of the memory devices
>   524288
> 
>   
> + 
> +   
> + 0-1
> +   
> +   
> + 16384
> + 0
> +   
> + 
> + 
> +   
> + 16384
> +   
> + 
> 
> ...
>  
> @@ -7918,7 +7932,9 @@ Example: usage of the memory devices
> 1.2.14` Provide ``nvdimm`` model that adds a Non-Volatile DIMM module.
> :since:`Since 3.2.0` Provide ``virtio-pmem`` model to add a 
> paravirtualized
> persistent memory device. :since:`Since 7.1.0` Provide ``virtio-mem`` 
> model
> -   to add paravirtualized memory device. :since:`Since 7.9.0`
> +   to add paravirtualized memory device. :since:`Since 7.9.0` Provide
> +   ``sgx-epc`` model to add a SGX enclave page cache (EPC) memory to the 
> guest.
> +   :since:`Since 8.6.0 and QEMU 6.2.0`
>  
>  ``access``
> An optional attribute ``access`` ( :since:`since 3.2.0` ) that provides
> @@ -7978,6 +7994,13 @@ Example: usage of the memory devices
>   Represents a path in the host that backs the virtio memory module in the
>   guest. It is mandatory.
>  
> +   For model ``sgx-epc`` this element is optional. The following optional
> +   elements may be used:
> +
> +   ``nodemask``
> +  This element can be used to override the default set of NUMA nodes 
> where
> +  the memory would be allocated. :since:`Since 8.6.0 and QEMU 7.0.0`
> +
>  ``target``
> The mandatory ``target`` element configures the placement and sizing of 
> the
> added memory from the perspective of the guest.
> @@ -7988,6 +8011,8 @@ Example: usage of the memory devices
>  
> The ``node`` subelement configures the guest NUMA node to attach the 
> memory
> to. The element shall be used only if the guest has NUMA nodes configured.
> +   For model ``sgx-epc`` this element is optional.

This looks redudnand. The sentence right before suggests that the
element is optional.

> It will be set to 0 as
> +   default. :since:`Since 8.6.0 and QEMU 7.0.0`

This is a bit misleading as we don't report this back in the domain XML.
If you really want to document the default then say it's hypervisor
dependant. But I'd just leave these two sentences out.

>  
> The following optional elements may be used:
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 709ca53790..f8b67eb375 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1431,6 +1431,7 @@ VIR_ENUM_IMPL(virDomainMemoryModel,
>"nvdimm",
>"virtio-pmem",
>"virtio-mem",
> +  "sgx-epc",
>  );
>  
>  VIR_ENUM_IMPL(virDomainShmemModel,
> @@ -5680,6 +5681,7 @@ virDomainMemoryDefPostParse(virDomainMemoryDef *mem,
>  
>  case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
>  case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> +case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC:
>  case VIR_DOMAIN_MEMORY_MODEL_NONE:
>  case VIR_DOMAIN_MEMORY_MODEL_LAST:
>  

Re: [libvirt][PATCH v13 6/6] qemu: Add command-line to generate SGX EPC memory backend

[Michal Prívozník]

On 7/1/22 21:14, Lin Yang wrote:
> According to the result parsing from xml, add the argument of
> SGX EPC memory backend into QEMU command line.
> 
> With NUMA config:
> 
> #qemu-system-x86_64 \
> .. \
> -object 
> '{"qom-type":"memory-backend-epc","id":"memepc0","prealloc":true,"size":67108864,"host-nodes":[0,1],"policy":"bind"}'
>  \
> -object 
> '{"qom-type":"memory-backend-epc","id":"memepc1","prealloc":true,"size":16777216,"host-nodes":[2,3],"policy":"bind"}'
>  \
> -machine 
> sgx-epc.0.memdev=memepc0,sgx-epc.0.node=0,sgx-epc.1.memdev=memepc1,sgx-epc.1.node=1
> 
> Without NUMA config:
> 
> #qemu-system-x86_64 \
> .. \
> -object 
> '{"qom-type":"memory-backend-epc","id":"memepc0","prealloc":true,"size":67108864}'
>  \
> -object 
> '{"qom-type":"memory-backend-epc","id":"memepc1","prealloc":true,"size":16777216}'
>  \
> -machine sgx-epc.0.memdev=memepc0,sgx-epc.1.memdev=memepc1

Except, ...

> 
> Signed-off-by: Lin Yang 
> Signed-off-by: Michal Privoznik 
> Signed-off-by: Haibin Huang 
> ---
>  src/qemu/qemu_alias.c |  3 +-
>  src/qemu/qemu_command.c   | 86 ++-
>  src/qemu/qemu_monitor_json.c  | 41 +++--
>  .../sgx-epc-numa.x86_64-latest.args   | 40 +
>  .../sgx-epc.x86_64-6.2.0.args | 37 
>  tests/qemuxml2argvtest.c  |  3 +
>  6 files changed, 198 insertions(+), 12 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/sgx-epc-numa.x86_64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/sgx-epc.x86_64-6.2.0.args
> 


> --- /dev/null
> +++ b/tests/qemuxml2argvdata/sgx-epc-numa.x86_64-latest.args
> @@ -0,0 +1,40 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/tmp/lib/domain--1-QEMUGuest1 \
> +USER=test \
> +LOGNAME=test \
> +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
> +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
> +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
> +/usr/bin/qemu-system-x86_64 \
> +-name guest=QEMUGuest1,debug-threads=on \
> +-S \
> +-object 
> '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}'
>  \
> +-machine 
> q35,usb=off,dump-guest-core=off,sgx-epc.0.memdev=memepc0,sgx-epc.0.node=0,sgx-epc.1.memdev=memepc1,sgx-epc.1.node=1
>  \
> +-accel tcg \
> +-cpu qemu64 \
> +-m 214 \
> +-overcommit mem-lock=off \
> +-smp 2,sockets=2,cores=1,threads=1 \
> +-object 
> '{"qom-type":"memory-backend-ram","id":"ram-node0","size":112197632}' \
> +-numa node,nodeid=0,cpus=0,memdev=ram-node0 \
> +-object 
> '{"qom-type":"memory-backend-ram","id":"ram-node1","size":112197632}' \
> +-numa node,nodeid=1,cpus=1,memdev=ram-node1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-display none \
> +-no-user-config \
> +-nodefaults \
> +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
> +-mon chardev=charmonitor,id=monitor,mode=control \
> +-rtc base=utc \
> +-no-shutdown \
> +-no-acpi \
> +-boot strict=on \
> +-device 
> '{"driver":"pcie-root-port","port":8,"chassis":1,"id":"pci.1","bus":"pcie.0","multifunction":true,"addr":"0x1"}'
>  \
> +-device 
> '{"driver":"pcie-root-port","port":9,"chassis":2,"id":"pci.2","bus":"pcie.0","addr":"0x1.0x1"}'
>  \
> +-object 
> '{"qom-type":"memory-backend-epc","id":"memepc0","prealloc":true,"size":67108864,"host-nodes":[0,1],"policy":"bind"}'
>  \
> +-object 
> '{"qom-type":"memory-backend-epc","id":"memepc1","prealloc":true,"size":16777216,"host-nodes":[2,3],"policy":"bind"}'
>  \

... nor here ...

> +-audiodev '{"id":"audio1","driver":"none"}' \
> +-device 
> '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.1","addr":"0x0"}' \
> +-sandbox 
> on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
> +-msg timestamp=on
> diff --git a/tests/qemuxml2argvdata/sgx-epc.x86_64-6.2.0.args 
> b/tests/qemuxml2argvdata/sgx-epc.x86_64-6.2.0.args
> new file mode 100644
> index 00..56c476b777
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/sgx-epc.x86_64-6.2.0.args
> @@ -0,0 +1,37 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/tmp/lib/domain--1-QEMUGuest1 \
> +USER=test \
> +LOGNAME=test \
> +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
> +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
> +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
> +/usr/bin/qemu-system-x86_64 \
> +-name guest=QEMUGuest1,debug-threads=on \
> +-S \
> +-object 
> '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}'
>  \
> +-machine 
> pc-q35-6.2,usb=off,dump-guest-core=off,memory-backend=pc.ram,sgx-epc.0.memdev=memepc0,sgx-epc.1.memdev=memepc1
>  \
> +-accel tcg \
> +-cpu qemu64 \
> +-m 134 \
> +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":140509184}' \
> +-overcommit mem-lock=off \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid 

Re: [libvirt][PATCH v13 0/6] Support query and use SGX

[Michal Prívozník]

On 7/1/22 21:14, Lin Yang wrote:
> This patch series provides support for enabling Intel's Software Guard
> Extensions (SGX) feature in guest VM.
> 

This version is a bit better than the previous one. But we're at version
13 and I am still unable to even start a guest. Please, make sure that
this basic functionality works in v14, because this is plain waste of
precious review bandwidth.

Anyway, as usual, I've uploaded my suggested fixes here:

https://gitlab.com/MichalPrivoznik/libvirt/-/commits/sgx/

Michal

Re: [libvirt PATCH v2 4/5] qemu_migration: get migration blockers before hardcoded checks

[Eugenio Perez Martin]

On Wed, Jul 20, 2022 at 12:31 PM Jiri Denemark  wrote:
>
> On Wed, Jul 20, 2022 at 11:11:53 +0200, Eugenio Pérez wrote:
> > since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> > will return an array of error strings describing the migration blockers.
> > This can be used to check whether there are any devices blocking
> > migration, etc.
> >
> > Enable qemuMigrationSrcIsAllowed to query it.
> >
> > Signed-off-by: Eugenio Pérez 
> > ---
> >  src/qemu/qemu_migration.c | 33 +
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index b12cb518ee..4224339f39 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef 
> > *def)
> >  return true;
> >  }
> >
> > +static int
> > +qemuDomainGetMigrationBlockers(virQEMUDriver *driver,
> > +   virDomainObj *vm,
> > +   char ***blockers)
> > +{
> > +qemuDomainObjPrivate *priv = vm->privateData;
> > +int rc;
> > +
> > +qemuDomainObjEnterMonitor(driver, vm);
> > +rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers);
> > +qemuDomainObjExitMonitor(vm);
> > +
> > +return rc;
> > +}
> >
> >  /**
> >   * qemuMigrationSrcIsAllowed:
> > @@ -1439,6 +1453,25 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> >  int nsnapshots;
> >  int pauseReason;
> >  size_t i;
> > +int r;
> > +
> > +/* Ask qemu if it have a migration blocker */
> > +if (virQEMUCapsGet(priv->qemuCaps, 
> > QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
> > +g_auto(GStrv) blockers = NULL;
> > +r = qemuDomainGetMigrationBlockers(driver, vm, );
> > +if (r != 0) {
> > +virReportError(VIR_ERR_OPERATION_INVALID,
> > +   _("cannot migrate domain, %s"),
> > +   _("error getting blockers"));
>
> If qemuDomainGetMigrationBlockers returned -1 a better error message
> should already be set and you would overwrite it here. Also with your
> current patch you would report this error even in case there was no
> migration blocker reported by QEMU. But this part would be fixed by
> letting qemuDomainGetMigrationBlockers return 0 when the blocked-reasons
> field is missing.
>

Got it.

> > +return false;
> > +}
> > +
> > +if (blockers[0]) {
>
> blockers && blockers[0]
>
> > +virReportError(VIR_ERR_OPERATION_INVALID,
> > +   _("cannot migrate domain, %s"), blockers[0]);
>
> I would prefer a colon there: "cannot migrate domain: %s". And we got a
> list of blockers from QEMU, but only use the first one? Please, join
> them with g_strjoinv().
>

So the resulting message should be:
"cannot migrate domain: , , ..., ", right?

Thanks!

> > +return false;
> > +}
> > +}
> >
> >  /* perform these checks only when migrating to remote hosts */
> >  if (remote) {
>
> Jirka
>

Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers

[Jiri Denemark]

On Wed, Jul 20, 2022 at 13:11:57 +0200, Eugenio Perez Martin wrote:
> On Wed, Jul 20, 2022 at 12:36 PM Jiri Denemark  wrote:
> >
> > On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> > > vDPA devices will be migratable soon. Since they are not migratable
> > > before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> > > for migration blockers, let it hardcoded in that case.
> > >
> > > Otherwise, ask qemu about the explicit blocker.
> > >
> > > Signed-off-by: Eugenio Pérez 
> > > ---
> > >  src/qemu/qemu_migration.c | 7 +--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > index 4224339f39..2f5c1d8121 100644
> > > --- a/src/qemu/qemu_migration.c
> > > +++ b/src/qemu/qemu_migration.c
> > > @@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> > >  int pauseReason;
> > >  size_t i;
> > >  int r;
> > > +bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
> > > +  
> > > QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
> >
> > Wrong indentation of the second line (QEMU... should be aligned with
> > priv).
> >
> 
> I'm fine with that indentation, but that will be a line with more than 80 
> chars.

No problem, it's not a hard requirement anymore in cases where it would
make the result look worse.

Jirka

Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers

[Eugenio Perez Martin]

On Wed, Jul 20, 2022 at 1:13 PM Peter Krempa  wrote:
>
> On Wed, Jul 20, 2022 at 13:11:57 +0200, Eugenio Perez Martin wrote:
> > On Wed, Jul 20, 2022 at 12:36 PM Jiri Denemark  wrote:
> > >
> > > On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> > > > vDPA devices will be migratable soon. Since they are not migratable
> > > > before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> > > > for migration blockers, let it hardcoded in that case.
> > > >
> > > > Otherwise, ask qemu about the explicit blocker.
> > > >
> > > > Signed-off-by: Eugenio Pérez 
> > > > ---
> > > >  src/qemu/qemu_migration.c | 7 +--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > > index 4224339f39..2f5c1d8121 100644
> > > > --- a/src/qemu/qemu_migration.c
> > > > +++ b/src/qemu/qemu_migration.c
> > > > @@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> > > >  int pauseReason;
> > > >  size_t i;
> > > >  int r;
> > > > +bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
> > > > +  
> > > > QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
> > >
> > > Wrong indentation of the second line (QEMU... should be aligned with
> > > priv).
> > >
> >
> > I'm fine with that indentation, but that will be a line with more than 80 
> > chars.
>
> The max 80 columns limitation is only a soft limit. We prefer way more
> to keep alignment and readability of the code rather than stick to the
> antiquated 80 columns limit.
>

Got it, I'll indent properly for the next version then.

Thanks!

Re: [libvirt PATCH v2 2/5] qemu_monitor_json: Add qemuMonitorJSONGetMigrationBlockers

[Eugenio Perez Martin]

On Wed, Jul 20, 2022 at 1:13 PM Jiri Denemark  wrote:
>
> On Wed, Jul 20, 2022 at 12:43:44 +0200, Eugenio Perez Martin wrote:
> > On Wed, Jul 20, 2022 at 12:14 PM Jiri Denemark  wrote:
> > >
> > > On Wed, Jul 20, 2022 at 11:11:51 +0200, Eugenio Pérez wrote:
> > > > This will allow qemuMigrationSrcIsAllowed to dynamically ask for
> > > > migration blockers, reducing duplication.
> > > >
> > > > Signed-off-by: Eugenio Pérez 
> > > > ---
> > > >  src/qemu/qemu_monitor_json.c | 35 +++
> > > >  src/qemu/qemu_monitor_json.h |  3 +++
> > > >  2 files changed, 38 insertions(+)
> > > >
> > > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > > > index 5e4a86e5ad..a53d721720 100644
> > > > --- a/src/qemu/qemu_monitor_json.c
> > > > +++ b/src/qemu/qemu_monitor_json.c
> > > > @@ -3338,6 +3338,41 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
> > > >  return 0;
> > > >  }
> > > >
> > > > +int qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
> > > > +char ***blockers)
> > > > +{
> > > > +g_autoptr(virJSONValue) cmd = NULL;
> > > > +g_autoptr(virJSONValue) reply = NULL;
> > > > +virJSONValue *data;
> > > > +virJSONValue *jblockers;
> > > > +size_t i;
> > > > +
> > > > +if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL)))
> > > > +return -1;
> > >
> > > We already have a function which calls query-migrate in JSON monitor,
> > > but I actually agree with adding this separate function as it serves a
> > > completely different purpose.
> > >
> >
> > I'm totally ok if anybody prefers to merge both too.
>
> I don't think anyone would prefer that as the existing function is a big
> beast which would get even more complicated if generalized for this use
> case.
>
> > > > +
> > > > +if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> > > > +return -1;
> > > > +
> > > > +if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 
> > > > 0)
> > > > +return -1;
> > > > +
> > > > +data = virJSONValueObjectGetObject(reply, "return");
> > > > +
> > > > +if (!(jblockers = virJSONValueObjectGetArray(data, 
> > > > "blocked-reasons")))
> > > > +return -1;
> > >
> > > This is not an error (not to mention that you would return -1 without a
> > > proper error message) as missing blocked-reasons means there's no
> > > migration blocker and the domain can be migrated (as long as the
> > > QEMU_CAPS_MIGRATION_BLOCKED_REASONS capability is set).
> > >
> >
> > Right, I'll fix it.
> >
> > Regarding the message on failure, a lot of the functions in this file
> > returns -1 without a message. How can I print it properly here or
> > propagate to the caller?
>
> Well, returning -1 is fine if the function called
> (qemuMonitorJSONCommand, qemuMonitorJSONCheckReply) already reports an
> error. But this is not the case of virJSONValueObjectGetArray. Reporting
> an error would be done just by calling virReportError() here instead of
> having it in the caller. Anyway, nothing to worry about here as you
> will be returning 0.
>

Got it.

Thanks!

> Jirka
>

Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers

[Peter Krempa]

On Wed, Jul 20, 2022 at 13:11:57 +0200, Eugenio Perez Martin wrote:
> On Wed, Jul 20, 2022 at 12:36 PM Jiri Denemark  wrote:
> >
> > On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> > > vDPA devices will be migratable soon. Since they are not migratable
> > > before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> > > for migration blockers, let it hardcoded in that case.
> > >
> > > Otherwise, ask qemu about the explicit blocker.
> > >
> > > Signed-off-by: Eugenio Pérez 
> > > ---
> > >  src/qemu/qemu_migration.c | 7 +--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > index 4224339f39..2f5c1d8121 100644
> > > --- a/src/qemu/qemu_migration.c
> > > +++ b/src/qemu/qemu_migration.c
> > > @@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> > >  int pauseReason;
> > >  size_t i;
> > >  int r;
> > > +bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
> > > +  
> > > QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
> >
> > Wrong indentation of the second line (QEMU... should be aligned with
> > priv).
> >
> 
> I'm fine with that indentation, but that will be a line with more than 80 
> chars.

The max 80 columns limitation is only a soft limit. We prefer way more
to keep alignment and readability of the code rather than stick to the
antiquated 80 columns limit. 

Re: [libvirt PATCH v2 2/5] qemu_monitor_json: Add qemuMonitorJSONGetMigrationBlockers

[Jiri Denemark]

On Wed, Jul 20, 2022 at 12:43:44 +0200, Eugenio Perez Martin wrote:
> On Wed, Jul 20, 2022 at 12:14 PM Jiri Denemark  wrote:
> >
> > On Wed, Jul 20, 2022 at 11:11:51 +0200, Eugenio Pérez wrote:
> > > This will allow qemuMigrationSrcIsAllowed to dynamically ask for
> > > migration blockers, reducing duplication.
> > >
> > > Signed-off-by: Eugenio Pérez 
> > > ---
> > >  src/qemu/qemu_monitor_json.c | 35 +++
> > >  src/qemu/qemu_monitor_json.h |  3 +++
> > >  2 files changed, 38 insertions(+)
> > >
> > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > > index 5e4a86e5ad..a53d721720 100644
> > > --- a/src/qemu/qemu_monitor_json.c
> > > +++ b/src/qemu/qemu_monitor_json.c
> > > @@ -3338,6 +3338,41 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
> > >  return 0;
> > >  }
> > >
> > > +int qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
> > > +char ***blockers)
> > > +{
> > > +g_autoptr(virJSONValue) cmd = NULL;
> > > +g_autoptr(virJSONValue) reply = NULL;
> > > +virJSONValue *data;
> > > +virJSONValue *jblockers;
> > > +size_t i;
> > > +
> > > +if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL)))
> > > +return -1;
> >
> > We already have a function which calls query-migrate in JSON monitor,
> > but I actually agree with adding this separate function as it serves a
> > completely different purpose.
> >
> 
> I'm totally ok if anybody prefers to merge both too.

I don't think anyone would prefer that as the existing function is a big
beast which would get even more complicated if generalized for this use
case.

> > > +
> > > +if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> > > +return -1;
> > > +
> > > +if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
> > > +return -1;
> > > +
> > > +data = virJSONValueObjectGetObject(reply, "return");
> > > +
> > > +if (!(jblockers = virJSONValueObjectGetArray(data, 
> > > "blocked-reasons")))
> > > +return -1;
> >
> > This is not an error (not to mention that you would return -1 without a
> > proper error message) as missing blocked-reasons means there's no
> > migration blocker and the domain can be migrated (as long as the
> > QEMU_CAPS_MIGRATION_BLOCKED_REASONS capability is set).
> >
> 
> Right, I'll fix it.
> 
> Regarding the message on failure, a lot of the functions in this file
> returns -1 without a message. How can I print it properly here or
> propagate to the caller?

Well, returning -1 is fine if the function called
(qemuMonitorJSONCommand, qemuMonitorJSONCheckReply) already reports an
error. But this is not the case of virJSONValueObjectGetArray. Reporting
an error would be done just by calling virReportError() here instead of
having it in the caller. Anyway, nothing to worry about here as you
will be returning 0.

Jirka

Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers

[Eugenio Perez Martin]

On Wed, Jul 20, 2022 at 12:36 PM Jiri Denemark  wrote:
>
> On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> > vDPA devices will be migratable soon. Since they are not migratable
> > before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> > for migration blockers, let it hardcoded in that case.
> >
> > Otherwise, ask qemu about the explicit blocker.
> >
> > Signed-off-by: Eugenio Pérez 
> > ---
> >  src/qemu/qemu_migration.c | 7 +--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 4224339f39..2f5c1d8121 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> >  int pauseReason;
> >  size_t i;
> >  int r;
> > +bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
> > +  
> > QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
>
> Wrong indentation of the second line (QEMU... should be aligned with
> priv).
>

I'm fine with that indentation, but that will be a line with more than 80 chars.

> >
> >  /* Ask qemu if it have a migration blocker */
> > -if (virQEMUCapsGet(priv->qemuCaps, 
> > QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
> > +if (blockedReasonsCap) {
> >  g_auto(GStrv) blockers = NULL;
> >  r = qemuDomainGetMigrationBlockers(driver, vm, );
> >  if (r != 0) {
> > @@ -1579,7 +1581,8 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> >  virDomainNetDef *net = vm->def->nets[i];
> >  qemuSlirp *slirp;
> >
> > -if (net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
> > +
>
> Extra empty line.
>

I'll delete it in the next version.

> > +if (!blockedReasonsCap && net->type == 
> > VIR_DOMAIN_NET_TYPE_VDPA) {
>
> So possibly we could skip more checks in the function if migration
> blockers are supported by QEMU, but this is a good conservative
> approach. The other checks (if any) can be taken care of separately.
>

I did a fast search but I didn't see something super obvious to me.

Thanks!

> >  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > _("vDPA devices cannot be migrated"));
> >  return false;
>
> Jirka
>

Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers

[Eugenio Perez Martin]

On Wed, Jul 20, 2022 at 12:37 PM Jiri Denemark  wrote:
>
> On Wed, Jul 20, 2022 at 12:36:16 +0200, Jiri Denemark wrote:
> > On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> > > vDPA devices will be migratable soon. Since they are not migratable
> > > before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> > > for migration blockers, let it hardcoded in that case.
> > >
> > > Otherwise, ask qemu about the explicit blocker.
> > >
> > > Signed-off-by: Eugenio Pérez 
> > > ---
> > >  src/qemu/qemu_migration.c | 7 +--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
>
> Oh and I forgot to mention s/qemu_migrate/qemu_migration/ in the
> subject.
>

I'll fix it in the next version.

Thanks!

> Jirka
>

Re: [libvirt PATCH v2 2/5] qemu_monitor_json: Add qemuMonitorJSONGetMigrationBlockers

[Eugenio Perez Martin]

On Wed, Jul 20, 2022 at 12:14 PM Jiri Denemark  wrote:
>
> On Wed, Jul 20, 2022 at 11:11:51 +0200, Eugenio Pérez wrote:
> > This will allow qemuMigrationSrcIsAllowed to dynamically ask for
> > migration blockers, reducing duplication.
> >
> > Signed-off-by: Eugenio Pérez 
> > ---
> >  src/qemu/qemu_monitor_json.c | 35 +++
> >  src/qemu/qemu_monitor_json.h |  3 +++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index 5e4a86e5ad..a53d721720 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -3338,6 +3338,41 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
> >  return 0;
> >  }
> >
> > +int qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
> > +char ***blockers)
> > +{
> > +g_autoptr(virJSONValue) cmd = NULL;
> > +g_autoptr(virJSONValue) reply = NULL;
> > +virJSONValue *data;
> > +virJSONValue *jblockers;
> > +size_t i;
> > +
> > +if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL)))
> > +return -1;
>
> We already have a function which calls query-migrate in JSON monitor,
> but I actually agree with adding this separate function as it serves a
> completely different purpose.
>

I'm totally ok if anybody prefers to merge both too.

> > +
> > +if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> > +return -1;
> > +
> > +if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
> > +return -1;
> > +
> > +data = virJSONValueObjectGetObject(reply, "return");
> > +
> > +if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons")))
> > +return -1;
>
> This is not an error (not to mention that you would return -1 without a
> proper error message) as missing blocked-reasons means there's no
> migration blocker and the domain can be migrated (as long as the
> QEMU_CAPS_MIGRATION_BLOCKED_REASONS capability is set).
>

Right, I'll fix it.

Regarding the message on failure, a lot of the functions in this file
returns -1 without a message. How can I print it properly here or
propagate to the caller?

> I think we should return 0 and set *blockers = NULL in this case.
>

Sure, I'll change that.

> > +
> > +/* NULL terminated array */
>
> This comment would be better as part of a proper documentation above the
> function.
>

I'll move to the function doc.

Thanks!



> > +*blockers = g_new0(char *, virJSONValueArraySize(jblockers) + 1);
> > +for (i = 0; i < virJSONValueArraySize(jblockers); i++) {
> > +virJSONValue *jblocker = virJSONValueArrayGet(jblockers, i);
> > +const char *blocker = virJSONValueGetString(jblocker);
> > +
> > +(*blockers)[i] = g_strdup(blocker);
> > +}
> > +
> > +return 0;
> > +}
> > +
> >  int qemuMonitorJSONMigrateCancel(qemuMonitor *mon)
> >  {
> >  g_autoptr(virJSONValue) cmd = 
> > qemuMonitorJSONMakeCommand("migrate_cancel", NULL);
>
> Jirka
>

Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers

[Jiri Denemark]

On Wed, Jul 20, 2022 at 12:36:16 +0200, Jiri Denemark wrote:
> On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> > vDPA devices will be migratable soon. Since they are not migratable
> > before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> > for migration blockers, let it hardcoded in that case.
> > 
> > Otherwise, ask qemu about the explicit blocker.
> > 
> > Signed-off-by: Eugenio Pérez 
> > ---
> >  src/qemu/qemu_migration.c | 7 +--
> >  1 file changed, 5 insertions(+), 2 deletions(-)

Oh and I forgot to mention s/qemu_migrate/qemu_migration/ in the
subject.

Jirka

Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers

[Jiri Denemark]

On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> vDPA devices will be migratable soon. Since they are not migratable
> before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> for migration blockers, let it hardcoded in that case.
> 
> Otherwise, ask qemu about the explicit blocker.
> 
> Signed-off-by: Eugenio Pérez 
> ---
>  src/qemu/qemu_migration.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 4224339f39..2f5c1d8121 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
>  int pauseReason;
>  size_t i;
>  int r;
> +bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
> +  
> QEMU_CAPS_MIGRATION_BLOCKED_REASONS);

Wrong indentation of the second line (QEMU... should be aligned with
priv).

>  
>  /* Ask qemu if it have a migration blocker */
> -if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) 
> {
> +if (blockedReasonsCap) {
>  g_auto(GStrv) blockers = NULL;
>  r = qemuDomainGetMigrationBlockers(driver, vm, );
>  if (r != 0) {
> @@ -1579,7 +1581,8 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
>  virDomainNetDef *net = vm->def->nets[i];
>  qemuSlirp *slirp;
>  
> -if (net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
> +

Extra empty line.

> +if (!blockedReasonsCap && net->type == VIR_DOMAIN_NET_TYPE_VDPA) 
> {

So possibly we could skip more checks in the function if migration
blockers are supported by QEMU, but this is a good conservative
approach. The other checks (if any) can be taken care of separately.

>  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("vDPA devices cannot be migrated"));
>  return false;

Jirka

Re: [libvirt PATCH v2 4/5] qemu_migration: get migration blockers before hardcoded checks

[Jiri Denemark]

On Wed, Jul 20, 2022 at 11:11:53 +0200, Eugenio Pérez wrote:
> since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> will return an array of error strings describing the migration blockers.
> This can be used to check whether there are any devices blocking
> migration, etc.
> 
> Enable qemuMigrationSrcIsAllowed to query it.
> 
> Signed-off-by: Eugenio Pérez 
> ---
>  src/qemu/qemu_migration.c | 33 +
>  1 file changed, 33 insertions(+)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index b12cb518ee..4224339f39 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef 
> *def)
>  return true;
>  }
>  
> +static int
> +qemuDomainGetMigrationBlockers(virQEMUDriver *driver,
> +   virDomainObj *vm,
> +   char ***blockers)
> +{
> +qemuDomainObjPrivate *priv = vm->privateData;
> +int rc;
> +
> +qemuDomainObjEnterMonitor(driver, vm);
> +rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers);
> +qemuDomainObjExitMonitor(vm);
> +
> +return rc;
> +}
>  
>  /**
>   * qemuMigrationSrcIsAllowed:
> @@ -1439,6 +1453,25 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
>  int nsnapshots;
>  int pauseReason;
>  size_t i;
> +int r;
> +
> +/* Ask qemu if it have a migration blocker */
> +if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) 
> {
> +g_auto(GStrv) blockers = NULL;
> +r = qemuDomainGetMigrationBlockers(driver, vm, );
> +if (r != 0) {
> +virReportError(VIR_ERR_OPERATION_INVALID,
> +   _("cannot migrate domain, %s"),
> +   _("error getting blockers"));

If qemuDomainGetMigrationBlockers returned -1 a better error message
should already be set and you would overwrite it here. Also with your
current patch you would report this error even in case there was no
migration blocker reported by QEMU. But this part would be fixed by
letting qemuDomainGetMigrationBlockers return 0 when the blocked-reasons
field is missing.

> +return false;
> +}
> +
> +if (blockers[0]) {

blockers && blockers[0]

> +virReportError(VIR_ERR_OPERATION_INVALID,
> +   _("cannot migrate domain, %s"), blockers[0]);

I would prefer a colon there: "cannot migrate domain: %s". And we got a
list of blockers from QEMU, but only use the first one? Please, join
them with g_strjoinv().

> +return false;
> +}
> +}
>  
>  /* perform these checks only when migrating to remote hosts */
>  if (remote) {

Jirka

Re: [libvirt PATCH v2 3/5] qemu_monitor: add support for get qemu migration blockers

[Jiri Denemark]

On Wed, Jul 20, 2022 at 11:11:52 +0200, Eugenio Pérez wrote:
> since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> will return an array of error strings describing the migration blockers.
> This can be used to check whether there are any devices blocking
> migration, etc.
> 
> Enable qemu monitor to send this query.
> 
> Signed-off-by: Eugenio Pérez 
> ---
>  src/qemu/qemu_monitor.c | 11 +++
>  src/qemu/qemu_monitor.h |  4 
>  2 files changed, 15 insertions(+)

You can just squash this into the previous patch.

Jirka

Re: [libvirt PATCH v2 1/5] qemu: introduce capability QEMU_MIGRATION_BLOCKED_REASONS

[Jiri Denemark]

On Wed, Jul 20, 2022 at 11:11:50 +0200, Eugenio Pérez wrote:
> From: Jonathon Jongsma 
> 
> since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> will return an array of error strings describing the migration blockers.
> This can be used to check whether there are any devices blocking
> migration, etc.
> 
> Signed-off-by: Jonathon Jongsma 
> Signed-off-by: Eugenio Pérez 

s/QEMU_MIGRATION_BLOCKED_REASONS/QEMU_CAPS_MIGRATION_BLOCKED_REASONS/ in
the subject.

Jirka

Re: [libvirt PATCH v2 2/5] qemu_monitor_json: Add qemuMonitorJSONGetMigrationBlockers

[Jiri Denemark]

On Wed, Jul 20, 2022 at 11:11:51 +0200, Eugenio Pérez wrote:
> This will allow qemuMigrationSrcIsAllowed to dynamically ask for
> migration blockers, reducing duplication.
> 
> Signed-off-by: Eugenio Pérez 
> ---
>  src/qemu/qemu_monitor_json.c | 35 +++
>  src/qemu/qemu_monitor_json.h |  3 +++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 5e4a86e5ad..a53d721720 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -3338,6 +3338,41 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
>  return 0;
>  }
>  
> +int qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
> +char ***blockers)
> +{
> +g_autoptr(virJSONValue) cmd = NULL;
> +g_autoptr(virJSONValue) reply = NULL;
> +virJSONValue *data;
> +virJSONValue *jblockers;
> +size_t i;
> +
> +if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL)))
> +return -1;

We already have a function which calls query-migrate in JSON monitor,
but I actually agree with adding this separate function as it serves a
completely different purpose.

> +
> +if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> +return -1;
> +
> +if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
> +return -1;
> +
> +data = virJSONValueObjectGetObject(reply, "return");
> +
> +if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons")))
> +return -1;

This is not an error (not to mention that you would return -1 without a
proper error message) as missing blocked-reasons means there's no
migration blocker and the domain can be migrated (as long as the
QEMU_CAPS_MIGRATION_BLOCKED_REASONS capability is set).

I think we should return 0 and set *blockers = NULL in this case.

> +
> +/* NULL terminated array */

This comment would be better as part of a proper documentation above the
function.

> +*blockers = g_new0(char *, virJSONValueArraySize(jblockers) + 1);
> +for (i = 0; i < virJSONValueArraySize(jblockers); i++) {
> +virJSONValue *jblocker = virJSONValueArrayGet(jblockers, i);
> +const char *blocker = virJSONValueGetString(jblocker);
> +
> +(*blockers)[i] = g_strdup(blocker);
> +}
> +
> +return 0;
> +}
> +
>  int qemuMonitorJSONMigrateCancel(qemuMonitor *mon)
>  {
>  g_autoptr(virJSONValue) cmd = 
> qemuMonitorJSONMakeCommand("migrate_cancel", NULL);

Jirka

Re: [PATCH 2/2] schemas: Drop from capabilities schemas

[Michal Prívozník]

On 7/20/22 10:42, Daniel P. Berrangé wrote:
> On Wed, Jul 20, 2022 at 08:36:29AM +0200, Michal Privoznik wrote:
>> Currently, we have two types of output only XMLs: capabilities
>> and domaincapabilities. Neither of these is ever parsed.
>> Moreover, the order of elements inside these two documents is
>> well defined by their respective format functions. Therefore,
>> there's no need to have  anywhere in their
>> corresponding schemas.
> 
> The consumers of libvirt need to parse the XML, and they may wish
> to use the RNG files to validate. If the libvirt XML formatter has
> ever, or will ever, change ordering of elements, our RNG should
> be prepared for that. IOW, we should consider the RNG to be something
> that must validate XML output by all previous versions of libvirt.

Well, that's a very strong requirement that we don't even have for
domain XML. I though that using RNG from corresponding libvirt release
is recommended.

> How confident can we be that we've never changed the order of
> any elements ?  IMHO its best practice to always use 

We're currently only halfway there. Some elements are allowed to
interleave, some aren't. But okay, let's drop my patches and if we ever
decide to change ordering we'll have domaincapstest, well virschematest
shouting at us.

Michal

[libvirt PATCH v2 4/5] qemu_migration: get migration blockers before hardcoded checks

[Eugenio Pérez]

since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
will return an array of error strings describing the migration blockers.
This can be used to check whether there are any devices blocking
migration, etc.

Enable qemuMigrationSrcIsAllowed to query it.

Signed-off-by: Eugenio Pérez 
---
 src/qemu/qemu_migration.c | 33 +
 1 file changed, 33 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index b12cb518ee..4224339f39 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef *def)
 return true;
 }
 
+static int
+qemuDomainGetMigrationBlockers(virQEMUDriver *driver,
+   virDomainObj *vm,
+   char ***blockers)
+{
+qemuDomainObjPrivate *priv = vm->privateData;
+int rc;
+
+qemuDomainObjEnterMonitor(driver, vm);
+rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers);
+qemuDomainObjExitMonitor(vm);
+
+return rc;
+}
 
 /**
  * qemuMigrationSrcIsAllowed:
@@ -1439,6 +1453,25 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
 int nsnapshots;
 int pauseReason;
 size_t i;
+int r;
+
+/* Ask qemu if it have a migration blocker */
+if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
+g_auto(GStrv) blockers = NULL;
+r = qemuDomainGetMigrationBlockers(driver, vm, );
+if (r != 0) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("cannot migrate domain, %s"),
+   _("error getting blockers"));
+return false;
+}
+
+if (blockers[0]) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("cannot migrate domain, %s"), blockers[0]);
+return false;
+}
+}
 
 /* perform these checks only when migrating to remote hosts */
 if (remote) {
-- 
2.31.1

[libvirt PATCH v2 2/5] qemu_monitor_json: Add qemuMonitorJSONGetMigrationBlockers

[Eugenio Pérez]

This will allow qemuMigrationSrcIsAllowed to dynamically ask for
migration blockers, reducing duplication.

Signed-off-by: Eugenio Pérez 
---
 src/qemu/qemu_monitor_json.c | 35 +++
 src/qemu/qemu_monitor_json.h |  3 +++
 2 files changed, 38 insertions(+)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 5e4a86e5ad..a53d721720 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3338,6 +3338,41 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
 return 0;
 }
 
+int qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+virJSONValue *data;
+virJSONValue *jblockers;
+size_t i;
+
+if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL)))
+return -1;
+
+if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
+return -1;
+
+if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
+return -1;
+
+data = virJSONValueObjectGetObject(reply, "return");
+
+if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons")))
+return -1;
+
+/* NULL terminated array */
+*blockers = g_new0(char *, virJSONValueArraySize(jblockers) + 1);
+for (i = 0; i < virJSONValueArraySize(jblockers); i++) {
+virJSONValue *jblocker = virJSONValueArrayGet(jblockers, i);
+const char *blocker = virJSONValueGetString(jblocker);
+
+(*blockers)[i] = g_strdup(blocker);
+}
+
+return 0;
+}
+
 int qemuMonitorJSONMigrateCancel(qemuMonitor *mon)
 {
 g_autoptr(virJSONValue) cmd = qemuMonitorJSONMakeCommand("migrate_cancel", 
NULL);
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 2759566892..e4c65e250e 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -184,6 +184,9 @@ qemuMonitorJSONMigrate(qemuMonitor *mon,
unsigned int flags,
const char *uri);
 int
+qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers);
+int
 qemuMonitorJSONGetSpiceMigrationStatus(qemuMonitor *mon,
bool *spice_migrated);
 
-- 
2.31.1

[libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers

[Eugenio Pérez]

vDPA devices will be migratable soon. Since they are not migratable
before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
for migration blockers, let it hardcoded in that case.

Otherwise, ask qemu about the explicit blocker.

Signed-off-by: Eugenio Pérez 
---
 src/qemu/qemu_migration.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 4224339f39..2f5c1d8121 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
 int pauseReason;
 size_t i;
 int r;
+bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
+  QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
 
 /* Ask qemu if it have a migration blocker */
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
+if (blockedReasonsCap) {
 g_auto(GStrv) blockers = NULL;
 r = qemuDomainGetMigrationBlockers(driver, vm, );
 if (r != 0) {
@@ -1579,7 +1581,8 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
 virDomainNetDef *net = vm->def->nets[i];
 qemuSlirp *slirp;
 
-if (net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
+
+if (!blockedReasonsCap && net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("vDPA devices cannot be migrated"));
 return false;
-- 
2.31.1

[libvirt PATCH v2 3/5] qemu_monitor: add support for get qemu migration blockers

[Eugenio Pérez]

since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
will return an array of error strings describing the migration blockers.
This can be used to check whether there are any devices blocking
migration, etc.

Enable qemu monitor to send this query.

Signed-off-by: Eugenio Pérez 
---
 src/qemu/qemu_monitor.c | 11 +++
 src/qemu/qemu_monitor.h |  4 
 2 files changed, 15 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 109107eaae..e0939beecd 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4486,3 +4486,14 @@ qemuMonitorMigrateRecover(qemuMonitor *mon,
 
 return qemuMonitorJSONMigrateRecover(mon, uri);
 }
+
+int
+qemuMonitorGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers)
+{
+VIR_DEBUG("blockers=%p", blockers);
+
+QEMU_CHECK_MONITOR(mon);
+
+return qemuMonitorJSONGetMigrationBlockers(mon, blockers);
+}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index cc1a0bc8c9..b82f198285 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1543,3 +1543,7 @@ qemuMonitorChangeMemoryRequestedSize(qemuMonitor *mon,
 int
 qemuMonitorMigrateRecover(qemuMonitor *mon,
   const char *uri);
+
+int
+qemuMonitorGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers);
-- 
2.31.1

[libvirt PATCH v2 0/5] Ask qemu about migration blockers

[Eugenio Pérez]

There are some hardcoded migration blockers in libvirt, like having a net
vhost-vdpa device. While it's true that they cannot be migrated at the moment,
there are plans to be able to migrate them soon.

They'll put a migration blockers in the cases where you cannot migrate them.
Ask qemu about then before rejecting the migration. In case the question is not
possible, assume they're not migratable.

Eugenio Pérez (4):
  qemu_monitor_json: Add qemuMonitorJSONGetMigrationBlockers
  qemu_monitor: add support for get qemu migration blockers
  qemu_migration: get migration blockers before hardcoded checks
  qemu_migrate: Do not forbif vDPA devices if can query blockers

Jonathon Jongsma (1):
  qemu: introduce capability QEMU_MIGRATION_BLOCKED_REASONS

 src/qemu/qemu_capabilities.c  |  2 +
 src/qemu/qemu_capabilities.h  |  1 +
 src/qemu/qemu_migration.c | 38 ++-
 src/qemu/qemu_monitor.c   | 11 ++
 src/qemu/qemu_monitor.h   |  4 ++
 src/qemu/qemu_monitor_json.c  | 35 +
 src/qemu/qemu_monitor_json.h  |  3 ++
 .../caps_6.0.0.aarch64.xml|  1 +
 .../qemucapabilitiesdata/caps_6.0.0.s390x.xml |  1 +
 .../caps_6.0.0.x86_64.xml |  1 +
 .../caps_6.1.0.x86_64.xml |  1 +
 .../caps_6.2.0.aarch64.xml|  1 +
 .../qemucapabilitiesdata/caps_6.2.0.ppc64.xml |  1 +
 .../caps_6.2.0.x86_64.xml |  1 +
 .../caps_7.0.0.aarch64.xml|  1 +
 .../qemucapabilitiesdata/caps_7.0.0.ppc64.xml |  1 +
 .../caps_7.0.0.x86_64.xml |  1 +
 .../caps_7.1.0.x86_64.xml |  1 +
 18 files changed, 104 insertions(+), 1 deletion(-)

-- 
2.31.1

[libvirt PATCH v2 1/5] qemu: introduce capability QEMU_MIGRATION_BLOCKED_REASONS

[Eugenio Pérez]

From: Jonathon Jongsma 

since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
will return an array of error strings describing the migration blockers.
This can be used to check whether there are any devices blocking
migration, etc.

Signed-off-by: Jonathon Jongsma 
Signed-off-by: Eugenio Pérez 
---
 src/qemu/qemu_capabilities.c  | 2 ++
 src/qemu/qemu_capabilities.h  | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml   | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_7.0.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_7.0.0.ppc64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml  | 1 +
 13 files changed, 14 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 30b396d32d..b002fb98ed 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -672,6 +672,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */
   "iothread.thread-pool-max", /* 
QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX */
   "usb-host.guest-resets-all", /* 
QEMU_CAPS_USB_HOST_GUESTS_RESETS_ALL */
+  "migration.blocked-reasons", /* 
QEMU_CAPS_MIGRATION_BLOCKED_REASONS */
 );
 
 
@@ -1622,6 +1623,7 @@ static struct virQEMUCapsStringFlags 
virQEMUCapsQMPSchemaQueries[] = {
 { "chardev-add/arg-type/backend/+qemu-vdagent", 
QEMU_CAPS_CHARDEV_QEMU_VDAGENT },
 { "query-display-options/ret-type/+dbus", QEMU_CAPS_DISPLAY_DBUS },
 { "object-add/arg-type/+iothread/thread-pool-max", 
QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX },
+{ "query-migrate/ret-type/blocked-reasons", 
QEMU_CAPS_MIGRATION_BLOCKED_REASONS },
 };
 
 typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index d979a5ba3b..8f3090e2ce 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -651,6 +651,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_DISPLAY_DBUS, /* -display dbus */
 QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX, /* -object iothread.thread-pool-max */
 QEMU_CAPS_USB_HOST_GUESTS_RESETS_ALL, /* -device usb-host.guest-resets-all 
*/
+QEMU_CAPS_MIGRATION_BLOCKED_REASONS, /* query-migrate returns 
'blocked-reasons */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml
index 01e30f4e02..4afd7b26ce 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml
@@ -187,6 +187,7 @@
   
   
   
+  
   600
   0
   61700242
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
index aa7b5deab5..c9cb85daa0 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
@@ -144,6 +144,7 @@
   
   
   
+  
   600
   0
   39100242
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
index d9e385ab1d..508804521c 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
@@ -229,6 +229,7 @@
   
   
   
+  
   600
   0
   43100242
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index 05f297dfa2..d4a540fafd 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -234,6 +234,7 @@
   
   
   
+  
   6001000
   0
   43100243
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml
index 9cb1a32354..71697fac95 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml
@@ -199,6 +199,7 @@
   
   
   
+  
   6001050
   0
   61700244
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml
index 5df148d787..3f86e03f18 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml
@@ -193,6 +193,7 @@
   
   
   
+  
   6002000
   0
   42900244
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
index dd011f8408..1a1a9643d4 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
+++ 

[PATCH] remote: Make remote_daemon depend on qemu_protocol.h

[He Zhe]

We sometimes meet the following parallel compililation issue, since
remote_daemon depends on remote_protocol.h qemu_protocol.h and lxc_protocol.h,
which are usually generated due to remote_driver .

| FAILED: src/virtnetworkd.p/remote_remote_daemon_dispatch.c.o
| x86_64-wrs-linux-gcc ...
| In file included from ../libvirt-8.1.0/src/remote/remote_daemon_dispatch.c:26:
| ../libvirt-8.1.0/src/remote/remote_daemon.h:30:10: fatal error: 
qemu_protocol.h: No such file or directory
|30 | #include "qemu_protocol.h"
|   |  ^
| compilation terminated.

This patch adds the headers as dependencies of remote_daemon to make sure they
are always in place in advance.

Signed-off-by: He Zhe 
---
 src/remote/meson.build | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/src/remote/meson.build b/src/remote/meson.build
index eb4f7a0068..04525fb4a6 100644
--- a/src/remote/meson.build
+++ b/src/remote/meson.build
@@ -5,6 +5,15 @@ remote_driver_sources = [
 
 remote_driver_generated = []
 
+remote_daemon_sources = files(
+  'remote_daemon.c',
+  'remote_daemon_config.c',
+  'remote_daemon_dispatch.c',
+  'remote_daemon_stream.c',
+)
+
+remote_daemon_generated = []
+
 foreach name : [ 'remote', 'qemu', 'lxc' ]
   client_bodies_h = '@0@_client_bodies.h'.format(name)
   protocol_c = '@0@_protocol.c'.format(name)
@@ -21,7 +30,7 @@ foreach name : [ 'remote', 'qemu', 'lxc' ]
 capture: true,
   )
 
-  remote_driver_generated += custom_target(
+  protocol_h_generated = custom_target(
 protocol_h,
 input: protocol_x,
 output: protocol_h,
@@ -30,6 +39,9 @@ foreach name : [ 'remote', 'qemu', 'lxc' ]
 ],
   )
 
+  remote_driver_generated += protocol_h_generated
+  remote_daemon_generated += protocol_h_generated
+
   remote_driver_generated += custom_target(
 protocol_c,
 input: protocol_x,
@@ -42,15 +54,6 @@ foreach name : [ 'remote', 'qemu', 'lxc' ]
   rpc_probe_files += files(protocol_x)
 endforeach
 
-remote_daemon_sources = files(
-  'remote_daemon.c',
-  'remote_daemon_config.c',
-  'remote_daemon_dispatch.c',
-  'remote_daemon_stream.c',
-)
-
-remote_daemon_generated = []
-
 virt_ssh_helper_sources = files(
   'remote_sockets.c',
   'remote_ssh_helper.c',
-- 
2.17.1

Re: [PATCH 2/2] schemas: Drop from capabilities schemas

[Daniel P . Berrangé]

On Wed, Jul 20, 2022 at 08:36:29AM +0200, Michal Privoznik wrote:
> Currently, we have two types of output only XMLs: capabilities
> and domaincapabilities. Neither of these is ever parsed.
> Moreover, the order of elements inside these two documents is
> well defined by their respective format functions. Therefore,
> there's no need to have  anywhere in their
> corresponding schemas.

The consumers of libvirt need to parse the XML, and they may wish
to use the RNG files to validate. If the libvirt XML formatter has
ever, or will ever, change ordering of elements, our RNG should
be prepared for that. IOW, we should consider the RNG to be something
that must validate XML output by all previous versions of libvirt.
How confident can we be that we've never changed the order of
any elements ?  IMHO its best practice to always use 

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/conf/schemas/capability.rng | 160 +++-
>  src/conf/schemas/domaincaps.rng |  80 
>  2 files changed, 115 insertions(+), 125 deletions(-)
> 
> diff --git a/src/conf/schemas/capability.rng b/src/conf/schemas/capability.rng
> index c7e72c6f39..7a24b2a076 100644
> --- a/src/conf/schemas/capability.rng
> +++ b/src/conf/schemas/capability.rng
> @@ -53,44 +53,40 @@
>  
>
>  
> -  
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -  
> -
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
>  
> -  
> -
> -  
> +  
> +  
> +
> +  
>  
>
>  
>
>  
> -  
> -
> -  
> -
> -  
> -
> -
> -  
> -
> -  
> -
> -
> -  
> -
> -  
> -
> -  
> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +
> +  
>  
>
>  
> @@ -442,57 +438,55 @@
>  
>
>  
> -  
> -
> -  
> -
> -  
> -
> -
> -  
> -
> -  
> -
> -
> -  
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -  
> -
> -
> -  
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -  
> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
>  
>
>  
> diff --git a/src/conf/schemas/domaincaps.rng b/src/conf/schemas/domaincaps.rng
> index 9cbc2467ab..331fdbb1e9 100644
> --- a/src/conf/schemas/domaincaps.rng
> +++ b/src/conf/schemas/domaincaps.rng
> @@ -10,43 +10,41 @@
>  
>
>  
> -  
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -  
> -
> -
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
>
>  
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -  
> +  
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
>  
>
>  
> @@ -78,13 +76,11 @@
>  
>
>  
> -  
> -
> -
> -
> -  
> -
> -  
> +  
> +  
> +  
> +
> +  
>  
>
>  
> -- 
> 2.35.1
> 

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-

Re: [PATCH] spec: Restart sockets even when libvirtd is inactive

[Martin Kletzander]

On Tue, Jul 19, 2022 at 05:51:57PM -0600, Jim Fehlig wrote:

By default libvirtd will terminate itself after 120 seconds, so it is
likely that the daemon will not be running at package upgrade. Try
restarting sockets even if the daemon is inactive.

Signed-off-by: Jim Fehlig 
---

Assuming sockets need restarted on package update?



Probably not, but just in case there is a configuration change I think
it is safer to have this here.


libvirt.spec.in | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 9d788b790f..5201a14431 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1365,16 +1365,19 @@ then
# own the sockets again when it comes back up. Thus we must
# do this particular ordering, so that we get libvirtd
# running with socket activation in use
+is_active=no
/bin/systemctl is-active libvirtd.service 1>/dev/null 2>&1
if test $? = 0


I think this is not needed because this whole function is only called
when the daemon was active (see libvirt_daemon_schedule_restart and
libvirt_daemon_needs_restart).

That could make this whole thing even easier.


then
+is_active=yes
/bin/systemctl stop libvirtd.service >/dev/null 2>&1 || :
-
-/bin/systemctl try-restart \
-libvirtd.socket \
-libvirtd-ro.socket \
-libvirtd-admin.socket >/dev/null 2>&1 || :
-
+fi
+/bin/systemctl try-restart \
+libvirtd.socket \
+libvirtd-ro.socket \
+libvirtd-admin.socket >/dev/null 2>&1 || :
+if test "$is_active" = yes
+then
/bin/systemctl start libvirtd.service >/dev/null 2>&1 || :
fi
fi
--
2.36.1



signature.asc
Description: PGP signature

[PATCH 2/2] schemas: Drop from capabilities schemas

[Michal Privoznik]

Currently, we have two types of output only XMLs: capabilities
and domaincapabilities. Neither of these is ever parsed.
Moreover, the order of elements inside these two documents is
well defined by their respective format functions. Therefore,
there's no need to have  anywhere in their
corresponding schemas.

Signed-off-by: Michal Privoznik 
---
 src/conf/schemas/capability.rng | 160 +++-
 src/conf/schemas/domaincaps.rng |  80 
 2 files changed, 115 insertions(+), 125 deletions(-)

diff --git a/src/conf/schemas/capability.rng b/src/conf/schemas/capability.rng
index c7e72c6f39..7a24b2a076 100644
--- a/src/conf/schemas/capability.rng
+++ b/src/conf/schemas/capability.rng
@@ -53,44 +53,40 @@
 
   
 
-  
-
-  
-
-
-  
-
-
-  
-
-  
-
+  
+
+  
+  
+
+  
+  
+
+  
 
-  
-
-  
+  
+  
+
+  
 
   
 
   
 
-  
-
-  
-
-  
-
-
-  
-
-  
-
-
-  
-
-  
-
-  
+  
+
+  
+
+  
+  
+
+  
+
+  
+  
+
+  
+
+  
 
   
 
@@ -442,57 +438,55 @@
 
   
 
-  
-
-  
-
-  
-
-
-  
-
-  
-
-
-  
-
-  
-
-
-  
-
-
-  
-
-
-  
-
-
-  
-
-
-  
-
-  
-
-
-  
-
-  
-
-
-  
-
-
-  
-
-
-  
-
-
-  
-
-  
+  
+
+  
+
+  
+  
+
+  
+
+  
+  
+
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
+
+  
+  
+
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
 
   
 
diff --git a/src/conf/schemas/domaincaps.rng b/src/conf/schemas/domaincaps.rng
index 9cbc2467ab..331fdbb1e9 100644
--- a/src/conf/schemas/domaincaps.rng
+++ b/src/conf/schemas/domaincaps.rng
@@ -10,43 +10,41 @@
 
   
 
-  
-
-  
-
-
-  
-
-
-  
-
-  
-
-
+  
+
+  
+  
+
+  
+  
+
   
 
-
-  
-
-
-  
-
-
-  
-
-
-  
-
-
-  
-
-
-  
-
-
-  
-
-  
+  
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
 
   
 
@@ -78,13 +76,11 @@
 
   
 
-  
-
-
-
-  
-
-  
+  
+  
+  
+
+  
 
   
 
-- 
2.35.1

[PATCH 0/2] schemas: Drop from capabilities schemas

[Michal Privoznik]

*** BLURB HERE ***

Michal Prívozník (2):
  capabilityschemadata: Fix order of  in caps-test2.xml
  schemas: Drop  from capabilities schemas

 src/conf/schemas/capability.rng   | 160 +++---
 src/conf/schemas/domaincaps.rng   |  80 +--
 tests/capabilityschemadata/caps-test2.xml |   8 +-
 3 files changed, 119 insertions(+), 129 deletions(-)

-- 
2.35.1

[PATCH 1/2] capabilityschemadata: Fix order of in caps-test2.xml

[Michal Privoznik]

The guest  is formatted in
virCapabilitiesFormatGuestFeatures() and the order in which
individual child elements are formatted is fixed, because the
function iterates over the virCapsGuestFeatureType enum
(possibly) formatting each member until _LAST is met.

Therefore,  and  can't ever be
formatted first. Move these elements to proper location.

Signed-off-by: Michal Privoznik 
---
 tests/capabilityschemadata/caps-test2.xml | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/capabilityschemadata/caps-test2.xml 
b/tests/capabilityschemadata/caps-test2.xml
index 125a322998..bf18010440 100644
--- a/tests/capabilityschemadata/caps-test2.xml
+++ b/tests/capabilityschemadata/caps-test2.xml
@@ -72,12 +72,12 @@
   
 
 
-  
-  
   
   
   
   
+  
+  
 
   
 
@@ -114,10 +114,10 @@
   
 
 
-  
-  
   
   
+  
+  
 
   
 
-- 
2.35.1