[libvirt] [PATCH v10 06/10] backup: Implement virsh support for backup

[Eric Blake]

Introduce a few more new virsh commands for performing backup jobs.

At this time, I did not opt for a convenience command
'backup-begin-as' that cobbles together appropriate XML from the
user's command line arguments, but that may be a viable future
extension. Similarly, since backup is a potentially long-running
operation, it might be nice to add some sugar that automatically
handles waiting for the job to end, rather than making the user have
to poll or figure out virsh event to do the same.  Eventually, we
will also need a way to create a checkpoint atomically with an
external snapshot.

Signed-off-by: Eric Blake 
---
 tools/virsh-domain.c | 245 +++
 tools/virsh.pod  |  49 +
 2 files changed, 294 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index ed9dd269f7..d75c2e954a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -14042,6 +14042,233 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd)
 return ret;
 }

+
+/*
+ * "backup-begin" command
+ */
+static const vshCmdInfo info_backup_begin[] = {
+{.name = "help",
+ .data = N_("Start a disk backup of a live domain")
+},
+{.name = "desc",
+ .data = N_("Use XML to start a full or incremental disk backup of a live "
+"domain, optionally creating a checkpoint")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_backup_begin[] = {
+VIRSH_COMMON_OPT_DOMAIN_FULL(0),
+{.name = "xmlfile",
+ .type = VSH_OT_STRING,
+ .help = N_("domain backup XML"),
+},
+{.name = "checkpointxml",
+ .type = VSH_OT_STRING,
+ .help = N_("domain checkpoint XML"),
+},
+{.name = "no-metadata",
+ .type = VSH_OT_BOOL,
+ .help = N_("create checkpoint but don't track metadata"),
+},
+{.name = "quiesce",
+ .type = VSH_OT_BOOL,
+ .help = N_("quiesce guest's file systems"),
+},
+/* TODO: --wait/--verbose/--timeout flags for push model backups? */
+{.name = NULL}
+};
+
+static bool
+cmdBackupBegin(vshControl *ctl,
+   const vshCmd *cmd)
+{
+virDomainPtr dom = NULL;
+bool ret = false;
+const char *backup_from = NULL;
+char *backup_buffer = NULL;
+const char *check_from = NULL;
+char *check_buffer = NULL;
+unsigned int flags = 0;
+int id;
+
+if (vshCommandOptBool(cmd, "no-metadata"))
+flags |= VIR_DOMAIN_BACKUP_BEGIN_NO_METADATA;
+if (vshCommandOptBool(cmd, "quiesce"))
+flags |= VIR_DOMAIN_BACKUP_BEGIN_QUIESCE;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+goto cleanup;
+
+if (vshCommandOptStringReq(ctl, cmd, "xmlfile", _from) < 0)
+goto cleanup;
+if (!backup_from) {
+backup_buffer = vshStrdup(ctl, "");
+} else {
+if (virFileReadAll(backup_from, VSH_MAX_XML_FILE, _buffer) < 0) 
{
+vshSaveLibvirtError();
+goto cleanup;
+}
+}
+
+if (vshCommandOptStringReq(ctl, cmd, "checkpointxml", _from) < 0)
+goto cleanup;
+if (check_from) {
+if (virFileReadAll(check_from, VSH_MAX_XML_FILE, _buffer) < 0) {
+vshSaveLibvirtError();
+goto cleanup;
+}
+}
+
+id = virDomainBackupBegin(dom, backup_buffer, check_buffer, flags);
+
+if (id < 0)
+goto cleanup;
+
+vshPrint(ctl, _("Backup id %d started\n"), id);
+if (backup_from)
+vshPrintExtra(ctl, _("backup used description from '%s'\n"),
+  backup_from);
+if (check_buffer)
+vshPrintExtra(ctl, _("checkpoint created from '%s'\n"), check_from);
+
+ret = true;
+
+ cleanup:
+VIR_FREE(backup_buffer);
+VIR_FREE(check_buffer);
+virshDomainFree(dom);
+
+return ret;
+}
+
+/* TODO: backup-begin-as? */
+
+/*
+ * "backup-dumpxml" command
+ */
+static const vshCmdInfo info_backup_dumpxml[] = {
+{.name = "help",
+ .data = N_("Dump XML for an ongoing domain block backup job")
+},
+{.name = "desc",
+ .data = N_("Backup Dump XML")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_backup_dumpxml[] = {
+VIRSH_COMMON_OPT_DOMAIN_FULL(0),
+{.name = "id",
+ .type = VSH_OT_INT,
+ .help = N_("backup job id"),
+ /* TODO: Add API for listing active jobs, then adding a completer? */
+},
+/* TODO - worth adding this flag?
+{.name = "checkpoint",
+ .type = VSH_OT_BOOL,
+ .help = N_("if the backup created a checkpoint, also dump that XML")
+},
+*/
+{.name = NULL}
+};
+
+static bool
+cmdBackupDumpXML(vshControl *ctl,
+ const vshCmd *cmd)
+{
+virDomainPtr dom = NULL;
+bool ret = false;
+char *xml = NULL;
+unsigned int flags = 0;
+int id = 0;
+
+if (vshCommandOptBool(cmd, "security-info"))
+flags |= VIR_DOMAIN_XML_SECURE;
+
+if (vshCommandOptInt(ctl, cmd, "id", ) < 0)
+return false;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+ 

[libvirt] [PATCH v10 10/10] backup: Implement qemu incremental pull backup

[Eric Blake]

Complete wiring up incremental backup, by adding in support for
creating a checkpoint at the same time as a backup (make the
transaction have a few more steps) as well as exposing the dirty
bitmap for a prior backup over NBD (requires creating a temporary
bitmap, merging all appropriate bitmaps in, then exposing that
bitmap over NBD).

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_driver.c | 193 +++--
 1 file changed, 165 insertions(+), 28 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d6f7a49ab4..9dd0bdb999 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17054,6 +17054,24 @@ qemuDomainCheckpointPrepare(virQEMUDriverPtr driver, 
virCapsPtr caps,
 if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP)
 continue;

+/* We want to name temporary bitmap after disk name during
+ * incremental backup, which is not possible if that is a
+ * persistent bitmap name. We can also make life easier by
+ * enforcing bitmap names match checkpoint name, although this
+ * is not technically necessary. */
+if (STREQ(disk->name, disk->bitmap)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("checkpoint for disk %s must have distinct bitmap 
name"),
+   disk->name);
+goto cleanup;
+}
+if (STRNEQ(disk->bitmap, def->parent.name)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("disk %s bitmap should match checkpoint name %s"),
+   disk->name, def->parent.name);
+goto cleanup;
+}
+
 if (vm->def->disks[i]->src->format != VIR_STORAGE_FILE_QCOW2) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("checkpoint for disk %s unsupported "
@@ -17594,19 +17612,44 @@ qemuDomainCheckpointDelete(virDomainCheckpointPtr 
checkpoint,

 static int
 qemuDomainBackupPrepare(virQEMUDriverPtr driver, virDomainObjPtr vm,
-virDomainBackupDefPtr def)
+virDomainBackupDefPtr def,
+virDomainMomentObjPtr chk)
 {
 int ret = -1;
 size_t i;
+virDomainCheckpointDefPtr chkdef;

+chkdef = chk ? virDomainCheckpointObjGetDef(chk) : NULL;
+if (chk && def->ndisks != chkdef->ndisks) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("inconsistency between backup and checkpoint disks"));
+goto cleanup;
+}
 if (qemuBlockNodeNamesDetect(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
 goto cleanup;
 for (i = 0; i < def->ndisks; i++) {
 virDomainBackupDiskDef *disk = >disks[i];
 virStorageSourcePtr src = vm->def->disks[disk->idx]->src;

-if (!disk->store)
+/* For now, insist that atomic checkpoint affect same disks as
+ * those being backed up. */
+if (!disk->store) {
+if (chk &&
+chkdef->disks[i].type != VIR_DOMAIN_CHECKPOINT_TYPE_NONE) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("disk %s requested checkpoint without 
backup"),
+   disk->name);
+goto cleanup;
+}
 continue;
+}
+if (chk &&
+chkdef->disks[i].type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("disk %s requested backup without checkpoint"),
+   disk->name);
+goto cleanup;
+}
 if (virAsprintf(>store->nodeformat, "tmp-%s", disk->name) < 0)
 goto cleanup;
 if (!disk->store->format)
@@ -17640,7 +17683,7 @@ qemuDomainBackupDiskCleanup(virQEMUDriverPtr driver, 
virDomainObjPtr vm,
  * shortly be calling nbd-server-stop. */
 }
 if (incremental && disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_BITMAP &&
-qemuMonitorDeleteBitmap(priv->mon, node, disk->store->nodeformat) < 0) 
{
+qemuMonitorDeleteBitmap(priv->mon, node, disk->name) < 0) {
 VIR_WARN("Unable to remove temp bitmap for disk %s after backup",
  disk->name);
 ret = -1;
@@ -17678,28 +17721,22 @@ qemuDomainBackupBegin(virDomainPtr domain, const char 
*diskXml,
 bool job_started = false;
 bool nbd_running = false;
 bool push;
+const char *mode;
 size_t i;
 struct timeval tv;
 char *suffix = NULL;
 virCommandPtr cmd = NULL;
 const char *qemuImgPath;
+virDomainMomentObjPtr chk = NULL;
+virDomainMomentObjPtr other = NULL;
+virDomainMomentObjPtr parent = NULL;
+virDomainMomentObjPtr current;
+virJSONValuePtr arr = NULL;
+VIR_AUTOUNREF(virDomainCheckpointDefPtr) chkdef = NULL;

 virCheckFlags(VIR_DOMAIN_BACKUP_BEGIN_NO_METADATA, -1);
 /* TODO: 

[libvirt] [PATCH v10 03/10] backup: Introduce virDomainBackup APIs

[Eric Blake]

Introduce a few new public APIs related to incremental backups.  This
builds on the previous notion of a checkpoint (without an existing
checkpoint, the new API is a full backup, differing from
virDomainBlockCopy in the point of time chosen and in operation on
multiple disks at once); and also allows creation of a new checkpoint
at the same time as starting the backup (after all, an incremental
backup is only useful if it covers the state since the previous
backup).

A backup job also affects filtering a listing of domains, as well as
adding event reporting for signaling when a push model backup
completes (where the hypervisor creates the backup); note that the
pull model does not have an event (starting the backup lets a third
party access the data, and only the third party knows when it is
finished).

Since multiple backup jobs can be run in parallel in the future (well,
qemu doesn't support it yet, but we don't want to preclude the idea),
virDomainBackupBegin() returns a positive job id, and the id is also
visible in the backup XML. But until a future libvirt release adds a
bunch of APIs related to parallel job management where job ids will
actually matter, the documentation is also clear that job id 0 means
the 'currently running backup job' (provided one exists), for use in
virDomainBackupGetXMLDesc() and virDomainBackupEnd().

The full list of new APIs:
virDomainBackupBegin;
virDomainBackupEnd;
virDomainBackupGetXMLDesc;

Signed-off-by: Eric Blake 
Reviewed-by: Daniel P. Berrangé 
---
 include/libvirt/libvirt-domain.h |  41 +-
 src/driver-hypervisor.h  |  14 ++
 src/qemu/qemu_blockjob.h |   1 +
 examples/c/misc/event-test.c |   3 +
 src/conf/domain_conf.c   |   2 +-
 src/libvirt-domain-checkpoint.c  |   7 +-
 src/libvirt-domain.c | 219 +++
 src/libvirt_public.syms  |   7 +
 src/qemu/qemu_blockjob.c |   3 +
 src/qemu/qemu_domain.c   |   2 +
 src/qemu/qemu_driver.c   |   1 +
 tools/virsh-domain.c |   8 +-
 12 files changed, 301 insertions(+), 7 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index f160ee88b5..8aae7889f7 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -3,7 +3,7 @@
  * Summary: APIs for management of domains
  * Description: Provides APIs for the management of domains
  *
- * Copyright (C) 2006-2015 Red Hat, Inc.
+ * Copyright (C) 2006-2019 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -2446,6 +2446,9 @@ typedef enum {
  * exists as long as sync is active */
 VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4,

+/* Backup (virDomainBackupBegin), job exists until virDomainBackupEnd */
+VIR_DOMAIN_BLOCK_JOB_TYPE_BACKUP = 5,
+
 # ifdef VIR_ENUM_SENTINELS
 VIR_DOMAIN_BLOCK_JOB_TYPE_LAST
 # endif
@@ -3267,6 +3270,7 @@ typedef enum {
 VIR_DOMAIN_JOB_OPERATION_SNAPSHOT = 6,
 VIR_DOMAIN_JOB_OPERATION_SNAPSHOT_REVERT = 7,
 VIR_DOMAIN_JOB_OPERATION_DUMP = 8,
+VIR_DOMAIN_JOB_OPERATION_BACKUP = 9,

 # ifdef VIR_ENUM_SENTINELS
 VIR_DOMAIN_JOB_OPERATION_LAST
@@ -3282,6 +3286,14 @@ typedef enum {
  */
 # define VIR_DOMAIN_JOB_OPERATION"operation"

+/**
+ * VIR_DOMAIN_JOB_ID:
+ *
+ * virDomainGetJobStats field: the id of the job (so far, only for jobs
+ * started by virDomainBackupBegin()), as VIR_TYPED_PARAM_INT.
+ */
+# define VIR_DOMAIN_JOB_ID   "id"
+
 /**
  * VIR_DOMAIN_JOB_TIME_ELAPSED:
  *
@@ -4106,7 +4118,8 @@ typedef void 
(*virConnectDomainEventMigrationIterationCallback)(virConnectPtr co
  * @nparams: size of the params array
  * @opaque: application specific data
  *
- * This callback occurs when a job (such as migration) running on the domain
+ * This callback occurs when a job (such as migration or push-model
+ * virDomainBackupBegin()) running on the domain
  * is completed. The params array will contain statistics of the just completed
  * job as virDomainGetJobStats would return. The callback must not free @params
  * (the array will be freed once the callback finishes).
@@ -4902,4 +4915,28 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain,
int *nparams,
unsigned int flags);

+typedef enum {
+VIR_DOMAIN_BACKUP_BEGIN_NO_METADATA = (1 << 0), /* Make checkpoint without
+   remembering it */
+VIR_DOMAIN_BACKUP_BEGIN_QUIESCE = (1 << 1), /* use guest agent to
+   quiesce all mounted
+   file systems within
+   the domain */
+} virDomainBackupBeginFlags;
+
+/* Begin an incremental backup job, possibly 

[libvirt] [PATCH v10 07/10] backup: qemu: Implement framework for backup job APIs

[Eric Blake]

A later patch will be added to actually kick off the right QMP
commands, but at least this framework allows validation of backup XML,
including the fact that a backup job can survive a libvirtd
restart. Atomically creating a checkpoint alongside the backup is also
left for a later patch.

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_domain.h |   4 +
 src/qemu/qemu_domain.c |  26 +-
 src/qemu/qemu_driver.c | 185 +
 3 files changed, 212 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 37a00323a7..87a4d9a10d 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -392,6 +392,10 @@ struct _qemuDomainObjPrivate {

 /* running block jobs */
 virHashTablePtr blockjobs;
+
+/* Any currently running backup job.
+ * FIXME: allow jobs in parallel. For now, at most one job, always id 1. */
+virDomainBackupDefPtr backup;
 };

 #define QEMU_DOMAIN_PRIVATE(vm) \
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index fb22595129..333a3df247 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -58,6 +58,7 @@
 #include "locking/domain_lock.h"
 #include "virdomainsnapshotobjlist.h"
 #include "virdomaincheckpointobjlist.h"
+#include "backup_conf.h"

 #ifdef MAJOR_IN_MKDEV
 # include 
@@ -2448,6 +2449,7 @@ qemuDomainObjPrivateXMLFormatBlockjobs(virBufferPtr buf,
 bool bj = qemuDomainHasBlockjob(vm, false);
 struct qemuDomainPrivateBlockJobFormatData iterdata = { 
priv->driver->xmlopt,
  };
+int ret = -1;

 virBufferAsprintf(, " active='%s'",
   
virTristateBoolTypeToString(virTristateBoolFromBool(bj)));
@@ -2460,7 +2462,17 @@ qemuDomainObjPrivateXMLFormatBlockjobs(virBufferPtr buf,
) < 0)
 return -1;

-return virXMLFormatElement(buf, "blockjobs", , );
+if (virXMLFormatElement(buf, "blockjobs", , ) < 0)
+goto cleanup;
+
+/* TODO: merge other blockjobs and backups into uniform space? */
+if (priv->backup && virDomainBackupDefFormat(buf, priv->backup, true) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+virBufferFreeAndReset();
+return ret;
 }


@@ -3032,11 +3044,13 @@ 
qemuDomainObjPrivateXMLParseBlockjobData(virDomainObjPtr vm,


 static int
-qemuDomainObjPrivateXMLParseBlockjobs(virDomainObjPtr vm,
+qemuDomainObjPrivateXMLParseBlockjobs(virQEMUDriverPtr driver,
+  virDomainObjPtr vm,
   qemuDomainObjPrivatePtr priv,
   xmlXPathContextPtr ctxt)
 {
 VIR_AUTOFREE(xmlNodePtr *) nodes = NULL;
+xmlNodePtr node;
 ssize_t nnodes = 0;
 VIR_AUTOFREE(char *) active = NULL;
 int tmp;
@@ -3057,6 +3071,12 @@ qemuDomainObjPrivateXMLParseBlockjobs(virDomainObjPtr vm,
 }
 }

+if ((node = virXPathNode("./domainbackup", ctxt)) &&
+!(priv->backup = virDomainBackupDefParseNode(ctxt->doc, node,
+ driver->xmlopt,
+ 
VIR_DOMAIN_BACKUP_PARSE_INTERNAL)))
+return -1;
+
 return 0;
 }

@@ -3414,7 +3434,7 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,

 qemuDomainObjPrivateXMLParsePR(ctxt, >prDaemonRunning);

-if (qemuDomainObjPrivateXMLParseBlockjobs(vm, priv, ctxt) < 0)
+if (qemuDomainObjPrivateXMLParseBlockjobs(driver, vm, priv, ctxt) < 0)
 goto error;

 qemuDomainStorageIdReset(priv);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 86792bc8e1..ba8190e8c4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -105,6 +105,7 @@
 #include "virdomainsnapshotobjlist.h"
 #include "virenum.h"
 #include "virdomaincheckpointobjlist.h"
+#include "backup_conf.h"

 #define VIR_FROM_THIS VIR_FROM_QEMU

@@ -17591,6 +17592,187 @@ qemuDomainCheckpointDelete(virDomainCheckpointPtr 
checkpoint,
 return ret;
 }

+static int qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
+ const char *checkpointXml, unsigned int flags)
+{
+virQEMUDriverPtr driver = domain->conn->privateData;
+virDomainObjPtr vm = NULL;
+virDomainBackupDefPtr def = NULL;
+virQEMUDriverConfigPtr cfg = NULL;
+virCapsPtr caps = NULL;
+qemuDomainObjPrivatePtr priv;
+int ret = -1;
+struct timeval tv;
+char *suffix = NULL;
+
+virCheckFlags(VIR_DOMAIN_BACKUP_BEGIN_NO_METADATA, -1);
+/* TODO: VIR_DOMAIN_BACKUP_BEGIN_QUIESCE */
+
+// FIXME: Support non-null checkpointXML for incremental - what
+// code can be shared with CheckpointCreateXML, then add to transaction
+// to create new checkpoint at same time as starting blockdev-backup
+if (checkpointXml) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _("cannot create 

[libvirt] [PATCH v10 08/10] backup: Wire up qemu full pull backup commands over QMP

[Eric Blake]

Time to actually issue the QMP transactions that start and
stop backup commands (for now, just pull mode, not push).
Starting a job has to kick off several pre-req steps, then
a transaction, and additionally spawn an NBD server for pull
mode; ending a job as well as failing partway through
beginning a job has to unwind the earlier steps.  Implementing
push mode, as well as incremental pull and checkpoint creation,
is deferred to later patches.

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_domain.c   |  17 +-
 src/qemu/qemu_driver.c   | 310 ++-
 src/qemu/qemu_monitor_json.c |   4 +
 src/qemu/qemu_process.c  |   8 +
 4 files changed, 325 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 333a3df247..92f55b507a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3071,11 +3071,18 @@ qemuDomainObjPrivateXMLParseBlockjobs(virQEMUDriverPtr 
driver,
 }
 }

-if ((node = virXPathNode("./domainbackup", ctxt)) &&
-!(priv->backup = virDomainBackupDefParseNode(ctxt->doc, node,
- driver->xmlopt,
- 
VIR_DOMAIN_BACKUP_PARSE_INTERNAL)))
-return -1;
+if ((node = virXPathNode("./domainbackup", ctxt))) {
+if (!(priv->backup = virDomainBackupDefParseNode(ctxt->doc, node,
+ driver->xmlopt,
+ 
VIR_DOMAIN_BACKUP_PARSE_INTERNAL)))
+return -1;
+/* The backup job is only stored in XML if backupBegin
+ * succeeded at exporting the disk, so no need to store disk
+ * state when we can just force-reset it to a known-good
+ * value. */
+for (i = 0; i < priv->backup->ndisks; i++)
+priv->backup->disks[i].state = VIR_DOMAIN_BACKUP_DISK_STATE_EXPORT;
+}

 return 0;
 }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ba8190e8c4..26171c9b9f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17592,8 +17592,80 @@ qemuDomainCheckpointDelete(virDomainCheckpointPtr 
checkpoint,
 return ret;
 }

-static int qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
- const char *checkpointXml, unsigned int flags)
+static int
+qemuDomainBackupPrepare(virQEMUDriverPtr driver, virDomainObjPtr vm,
+virDomainBackupDefPtr def)
+{
+int ret = -1;
+size_t i;
+
+if (qemuBlockNodeNamesDetect(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
+goto cleanup;
+for (i = 0; i < def->ndisks; i++) {
+virDomainBackupDiskDef *disk = >disks[i];
+virStorageSourcePtr src = vm->def->disks[disk->idx]->src;
+
+if (!disk->store)
+continue;
+if (virAsprintf(>store->nodeformat, "tmp-%s", disk->name) < 0)
+goto cleanup;
+if (!disk->store->format)
+disk->store->format = VIR_STORAGE_FILE_QCOW2;
+if (def->incremental) {
+if (src->format != VIR_STORAGE_FILE_QCOW2) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("incremental backup of %s requires qcow2"),
+   disk->name);
+goto cleanup;
+}
+}
+}
+ret = 0;
+ cleanup:
+return ret;
+}
+
+/* Called while monitor lock is held. Best-effort cleanup. */
+static int
+qemuDomainBackupDiskCleanup(virQEMUDriverPtr driver, virDomainObjPtr vm,
+virDomainBackupDiskDef *disk, bool incremental)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+const char *node = vm->def->disks[disk->idx]->src->nodeformat;
+int ret = 0;
+
+if (!disk->store)
+return 0;
+if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_EXPORT) {
+/* No real need to use nbd-server-remove, since we will
+ * shortly be calling nbd-server-stop. */
+}
+if (incremental && disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_BITMAP &&
+qemuMonitorDeleteBitmap(priv->mon, node, disk->store->nodeformat) < 0) 
{
+VIR_WARN("Unable to remove temp bitmap for disk %s after backup",
+ disk->name);
+ret = -1;
+}
+if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_READY &&
+qemuMonitorBlockdevDel(priv->mon, disk->store->nodeformat) < 0) {
+VIR_WARN("Unable to remove temp disk %s after backup",
+ disk->name);
+ret = -1;
+}
+if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_LABEL)
+qemuDomainStorageSourceAccessRevoke(driver, vm, disk->store);
+if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_CREATED &&
+disk->store->detected && unlink(disk->store->path) < 0) {
+VIR_WARN("Unable to unlink temp disk %s after backup",
+ disk->store->path);
+

[libvirt] [PATCH v10 05/10] backup: Parse and output backup XML

[Eric Blake]

Accept XML describing a generic block job, and output it again as
needed. This may still need a few tweaks to match the documented XML
and RNG schema.

Signed-off-by: Eric Blake 
---
 src/conf/backup_conf.h   |  94 +++
 src/conf/virconftypes.h  |   3 +
 src/conf/Makefile.inc.am |   2 +
 src/conf/backup_conf.c   | 546 +++
 src/libvirt_private.syms |   8 +-
 5 files changed, 652 insertions(+), 1 deletion(-)
 create mode 100644 src/conf/backup_conf.h
 create mode 100644 src/conf/backup_conf.c

diff --git a/src/conf/backup_conf.h b/src/conf/backup_conf.h
new file mode 100644
index 00..1714315a1f
--- /dev/null
+++ b/src/conf/backup_conf.h
@@ -0,0 +1,94 @@
+/*
+ * backup_conf.h: domain backup XML processing
+ * (based on domain_conf.h)
+ *
+ * Copyright (C) 2006-2019 Red Hat, Inc.
+ * Copyright (C) 2006-2008 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#pragma once
+
+#include "internal.h"
+#include "domain_conf.h"
+#include "moment_conf.h"
+#include "virenum.h"
+
+/* Items related to incremental backup state */
+
+typedef enum {
+VIR_DOMAIN_BACKUP_TYPE_DEFAULT = 0,
+VIR_DOMAIN_BACKUP_TYPE_PUSH,
+VIR_DOMAIN_BACKUP_TYPE_PULL,
+
+VIR_DOMAIN_BACKUP_TYPE_LAST
+} virDomainBackupType;
+
+typedef enum {
+VIR_DOMAIN_BACKUP_DISK_STATE_DEFAULT = 0, /* Initial */
+VIR_DOMAIN_BACKUP_DISK_STATE_CREATED, /* File created */
+VIR_DOMAIN_BACKUP_DISK_STATE_LABEL, /* Security labels applied */
+VIR_DOMAIN_BACKUP_DISK_STATE_READY, /* Handed to guest */
+VIR_DOMAIN_BACKUP_DISK_STATE_BITMAP, /* Associated temp bitmap created */
+VIR_DOMAIN_BACKUP_DISK_STATE_EXPORT, /* NBD export created */
+VIR_DOMAIN_BACKUP_DISK_STATE_COMPLETE, /* Push job finished */
+} virDomainBackupDiskState;
+
+/* Stores disk-backup information */
+typedef struct _virDomainBackupDiskDef virDomainBackupDiskDef;
+typedef virDomainBackupDiskDef *virDomainBackupDiskDefPtr;
+struct _virDomainBackupDiskDef {
+char *name; /* name matching the dom->disks that matches name */
+
+/* details of target for push-mode, or of the scratch file for pull-mode */
+virStorageSourcePtr store;
+int state;  /* virDomainBackupDiskState, not stored in XML */
+};
+
+/* Stores the complete backup metadata */
+typedef struct _virDomainBackupDef virDomainBackupDef;
+typedef virDomainBackupDef *virDomainBackupDefPtr;
+struct _virDomainBackupDef {
+/* Public XML.  */
+int type; /* virDomainBackupType */
+int id;
+char *incremental;
+virStorageNetHostDefPtr server; /* only when type == PULL */
+
+size_t ndisks; /* should not exceed dom->ndisks */
+virDomainBackupDiskDef *disks;
+};
+
+VIR_ENUM_DECL(virDomainBackup);
+
+typedef enum {
+VIR_DOMAIN_BACKUP_PARSE_INTERNAL = 1 << 0,
+} virDomainBackupParseFlags;
+
+virDomainBackupDefPtr virDomainBackupDefParseString(const char *xmlStr,
+virDomainXMLOptionPtr 
xmlopt,
+unsigned int flags);
+virDomainBackupDefPtr virDomainBackupDefParseNode(xmlDocPtr xml,
+  xmlNodePtr root,
+  virDomainXMLOptionPtr xmlopt,
+  unsigned int flags);
+void virDomainBackupDefFree(virDomainBackupDefPtr def);
+int virDomainBackupDefFormat(virBufferPtr buf,
+ virDomainBackupDefPtr def,
+ bool internal);
+int virDomainBackupAlignDisks(virDomainBackupDefPtr backup,
+  virDomainDefPtr dom, const char *suffix);
diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
index a15cfb5f9e..11d7c3e62a 100644
--- a/src/conf/virconftypes.h
+++ b/src/conf/virconftypes.h
@@ -93,6 +93,9 @@ typedef virDomainABIStability *virDomainABIStabilityPtr;
 typedef struct _virDomainActualNetDef virDomainActualNetDef;
 typedef virDomainActualNetDef *virDomainActualNetDefPtr;

+typedef struct _virDomainBackupDef virDomainBackupDef;
+typedef virDomainBackupDef *virDomainBackupDefPtr;
+
 typedef struct _virDomainBIOSDef virDomainBIOSDef;
 typedef virDomainBIOSDef *virDomainBIOSDefPtr;

diff --git a/src/conf/Makefile.inc.am b/src/conf/Makefile.inc.am

[libvirt] [PATCH v10 00/10] incremental backup

[Eric Blake]

This is not the final version of incremental backup - before we can
accept this series, it needs a lot of polish to pick up cleanups made
possible by Peter's blockdev work, and we need to settle on our Job
API addition (so that the return value of BackupBegin and the
parameter to BackupGetXMLDesc and BackupEnd use the same
representation, whether that be UUID or something else).  I also know
that Peter left quite a few comments against v9, many of which I have
not actually attempted to address yet.  But it rebases things on top
of the checkpoint work that landed in the 5.6 release, and is still
able to perform the pull-mode incremental backups that I demonstrated
at KVM Forum 2018.

There's still probably crashes in portions of the code that are not
exercised by my demo, and I know that we really want to use qemu's
blockdev image creation instead of calling out to qemu-img create for
preparing the qcow2 scratch image in pull mode.  The main point of
this posting is to allow further testing before the actual feature
lands in an upstream libvirt release.

I've pushed a tag backup-v10 to both my libvirt.git and
libvirt-python.git repos to match:
https://repo.or.cz/libvirt/ericb.git/shortlog/refs/tags/backup-v10
https://repo.or.cz/libvirt-python/ericb.git/shortlog/refs/tags/backup-v10

001/10:[0033] [FC] 'backup: qemu: Implement VIR_DOMAIN_CHECKPOINT_XML_SIZE flag'
002/10:[0002] [FC] 'backup: Document new XML for backups'
003/10:[0010] [FC] 'backup: Introduce virDomainBackup APIs'
004/10:[0031] [FC] 'backup: Implement backup APIs for remote driver'
005/10:[] [--] 'backup: Parse and output backup XML'
006/10:[] [--] 'backup: Implement virsh support for backup'
007/10:[0025] [FC] 'backup: qemu: Implement framework for backup job APIs'
008/10:[0002] [FC] 'backup: Wire up qemu full pull backup commands over QMP'
009/10:[] [--] 'backup: qemu: Wire up qemu full push backup commands over 
QMP'
010/10:[0017] [FC] 'backup: Implement qemu incremental pull backup'

Eric Blake (10):
  backup: qemu: Implement VIR_DOMAIN_CHECKPOINT_XML_SIZE flag
  backup: Document new XML for backups
  backup: Introduce virDomainBackup APIs
  backup: Implement backup APIs for remote driver
  backup: Parse and output backup XML
  backup: Implement virsh support for backup
  backup: qemu: Implement framework for backup job APIs
  backup: Wire up qemu full pull backup commands over QMP
  backup: qemu: Wire up qemu full push backup commands over QMP
  backup: Implement qemu incremental pull backup

 include/libvirt/libvirt-domain.h |  41 +-
 src/conf/backup_conf.h   |  94 +++
 src/conf/virconftypes.h  |   3 +
 src/driver-hypervisor.h  |  14 +
 src/qemu/qemu_blockjob.h |   1 +
 src/qemu/qemu_domain.h   |   4 +
 src/qemu/qemu_monitor.h  |   4 +
 src/qemu/qemu_monitor_json.h |   4 +
 docs/docs.html.in|   3 +-
 docs/format.html.in  |   1 +
 docs/formatbackup.html.in| 184 +
 docs/formatcheckpoint.html.in|  12 +-
 docs/index.html.in   |   3 +-
 docs/schemas/domainbackup.rng| 219 ++
 examples/c/misc/event-test.c |   3 +
 libvirt.spec.in  |   1 +
 mingw-libvirt.spec.in|   2 +
 src/conf/Makefile.inc.am |   2 +
 src/conf/backup_conf.c   | 546 +++
 src/conf/domain_conf.c   |   2 +-
 src/libvirt-domain-checkpoint.c  |   7 +-
 src/libvirt-domain.c | 219 ++
 src/libvirt_private.syms |   8 +-
 src/libvirt_public.syms  |   7 +
 src/qemu/qemu_blockjob.c |   3 +
 src/qemu/qemu_domain.c   |  35 +-
 src/qemu/qemu_driver.c   | 684 ++-
 src/qemu/qemu_monitor.c  |  11 +
 src/qemu/qemu_monitor_json.c |  84 +++
 src/qemu/qemu_process.c  |   8 +
 src/remote/remote_driver.c   |   3 +
 src/remote/remote_protocol.x |  54 +-
 src/remote_protocol-structs  |  28 +
 tests/Makefile.am|   2 +
 tests/domainbackupxml2xmlin/backup-pull.xml  |   9 +
 tests/domainbackupxml2xmlin/backup-push.xml  |   9 +
 tests/domainbackupxml2xmlin/empty.xml|   1 +
 tests/domainbackupxml2xmlout/backup-pull.xml |   9 +
 tests/domainbackupxml2xmlout/backup-push.xml |   9 +
 tests/domainbackupxml2xmlout/empty.xml   |   7 +
 tests/virschematest.c|   2 +
 tools/virsh-domain.c | 253 ++-
 tools/virsh.pod  |  49 ++
 43 files changed, 2623 insertions(+), 21 deletions(-)
 create mode 100644 

[libvirt] [PATCH v10 04/10] backup: Implement backup APIs for remote driver

[Eric Blake]

This one is fairly straightforward - the generator already does what
we need.

Signed-off-by: Eric Blake 
Reviewed-by: Daniel P. Berrangé 
---
 src/remote/remote_driver.c   |  3 ++
 src/remote/remote_protocol.x | 54 +++-
 src/remote_protocol-structs  | 28 +++
 3 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index daac506672..eb2abada86 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8733,6 +8733,9 @@ static virHypervisorDriver hypervisor_driver = {
 .domainCheckpointLookupByName = remoteDomainCheckpointLookupByName, /* 
5.6.0 */
 .domainCheckpointGetParent = remoteDomainCheckpointGetParent, /* 5.6.0 */
 .domainCheckpointDelete = remoteDomainCheckpointDelete, /* 5.6.0 */
+.domainBackupBegin = remoteDomainBackupBegin, /* 5.7.0 */
+.domainBackupGetXMLDesc = remoteDomainBackupGetXMLDesc, /* 5.7.0 */
+.domainBackupEnd = remoteDomainBackupEnd, /* 5.7.0 */
 };

 static virNetworkDriver network_driver = {
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 118369e2b3..2fe8b3f902 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -3723,6 +3723,37 @@ struct remote_domain_checkpoint_delete_args {
 unsigned int flags;
 };

+struct remote_domain_backup_begin_args {
+remote_nonnull_domain dom;
+remote_string disk_xml;
+remote_string checkpoint_xml;
+unsigned int flags;
+};
+
+struct remote_domain_backup_begin_ret {
+int id;
+};
+
+struct remote_domain_backup_get_xml_desc_args {
+remote_nonnull_domain dom;
+int id;
+unsigned int flags;
+};
+
+struct remote_domain_backup_get_xml_desc_ret {
+remote_nonnull_string xml;
+};
+
+struct remote_domain_backup_end_args {
+remote_nonnull_domain dom;
+int id;
+unsigned int flags;
+};
+
+struct remote_domain_backup_end_ret {
+int retcode;
+};
+
 /*- Protocol. -*/

 /* Define the program number, protocol version and procedure numbers here. */
@@ -6584,5 +6615,26 @@ enum remote_procedure {
  * @generate: both
  * @acl: domain:checkpoint
  */
-REMOTE_PROC_DOMAIN_CHECKPOINT_DELETE = 417
+REMOTE_PROC_DOMAIN_CHECKPOINT_DELETE = 417,
+
+/**
+ * @generate: both
+ * @acl: domain:checkpoint
+ * @acl: domain:block_write
+ * @acl: domain:fs_freeze:VIR_DOMAIN_BACKUP_BEGIN_QUIESCE
+ */
+REMOTE_PROC_DOMAIN_BACKUP_BEGIN = 418,
+
+/**
+ * @generate: both
+ * @priority: high
+ * @acl: domain:read
+ */
+REMOTE_PROC_DOMAIN_BACKUP_GET_XML_DESC = 419,
+
+/**
+ * @generate: both
+ * @acl: domain:checkpoint
+ */
+REMOTE_PROC_DOMAIN_BACKUP_END = 420
 };
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index a42b4a9671..2975ee1227 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -3105,6 +3105,31 @@ struct remote_domain_checkpoint_delete_args {
 remote_nonnull_domain_checkpoint checkpoint;
 u_int  flags;
 };
+struct remote_domain_backup_begin_args {
+remote_nonnull_domain  dom;
+remote_string  disk_xml;
+remote_string  checkpoint_xml;
+u_int  flags;
+};
+struct remote_domain_backup_begin_ret {
+intid;
+};
+struct remote_domain_backup_get_xml_desc_args {
+remote_nonnull_domain  dom;
+intid;
+u_int  flags;
+};
+struct remote_domain_backup_get_xml_desc_ret {
+remote_nonnull_string  xml;
+};
+struct remote_domain_backup_end_args {
+remote_nonnull_domain  dom;
+intid;
+u_int  flags;
+};
+struct remote_domain_backup_end_ret {
+intretcode;
+};
 enum remote_procedure {
 REMOTE_PROC_CONNECT_OPEN = 1,
 REMOTE_PROC_CONNECT_CLOSE = 2,
@@ -3523,4 +3548,7 @@ enum remote_procedure {
 REMOTE_PROC_DOMAIN_CHECKPOINT_LOOKUP_BY_NAME = 415,
 REMOTE_PROC_DOMAIN_CHECKPOINT_GET_PARENT = 416,
 REMOTE_PROC_DOMAIN_CHECKPOINT_DELETE = 417,
+REMOTE_PROC_DOMAIN_BACKUP_BEGIN = 418,
+REMOTE_PROC_DOMAIN_BACKUP_GET_XML_DESC = 419,
+REMOTE_PROC_DOMAIN_BACKUP_END = 420,
 };
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v10 09/10] backup: qemu: Wire up qemu full push backup commands over QMP

[Eric Blake]

Update the code to support push backups; for now, the destination file
still has to be local, although the XML could be extended into
supporting remote destinations (where we will have to use the full
power of blockdev-add). This also touches up the event handling to
inform the user when the job is complete. (However, there are probably
bugs lurking in the event code; pull mode is more tested than push
mode at the time I write this).

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_driver.c | 81 +-
 1 file changed, 56 insertions(+), 25 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 26171c9b9f..d6f7a49ab4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17628,14 +17628,13 @@ qemuDomainBackupPrepare(virQEMUDriverPtr driver, 
virDomainObjPtr vm,
 /* Called while monitor lock is held. Best-effort cleanup. */
 static int
 qemuDomainBackupDiskCleanup(virQEMUDriverPtr driver, virDomainObjPtr vm,
-virDomainBackupDiskDef *disk, bool incremental)
+virDomainBackupDiskDef *disk, bool push,
+bool incremental, bool completed)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 const char *node = vm->def->disks[disk->idx]->src->nodeformat;
 int ret = 0;

-if (!disk->store)
-return 0;
 if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_EXPORT) {
 /* No real need to use nbd-server-remove, since we will
  * shortly be calling nbd-server-stop. */
@@ -17648,16 +17647,17 @@ qemuDomainBackupDiskCleanup(virQEMUDriverPtr driver, 
virDomainObjPtr vm,
 }
 if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_READY &&
 qemuMonitorBlockdevDel(priv->mon, disk->store->nodeformat) < 0) {
-VIR_WARN("Unable to remove temp disk %s after backup",
- disk->name);
+VIR_WARN("Unable to remove %s disk %s after backup",
+ push ? "target" : "scratch", disk->name);
 ret = -1;
 }
 if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_LABEL)
 qemuDomainStorageSourceAccessRevoke(driver, vm, disk->store);
-if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_CREATED &&
+if ((!push || !completed) &&
+disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_CREATED &&
 disk->store->detected && unlink(disk->store->path) < 0) {
-VIR_WARN("Unable to unlink temp disk %s after backup",
- disk->store->path);
+VIR_WARN("Unable to unlink %s disk %s after backup",
+ push ? "failed target" : "scratch", disk->store->path);
 ret = -1;
 }
 return ret;
@@ -17677,6 +17677,7 @@ qemuDomainBackupBegin(virDomainPtr domain, const char 
*diskXml,
 virJSONValuePtr json = NULL;
 bool job_started = false;
 bool nbd_running = false;
+bool push;
 size_t i;
 struct timeval tv;
 char *suffix = NULL;
@@ -17725,7 +17726,8 @@ qemuDomainBackupBegin(virDomainPtr domain, const char 
*diskXml,
 if (!(def = virDomainBackupDefParseString(diskXml, driver->xmlopt, 0)))
 goto cleanup;

-if (def->type == VIR_DOMAIN_BACKUP_TYPE_PULL) {
+push = def->type == VIR_DOMAIN_BACKUP_TYPE_PUSH;
+if (!push) {
 if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_BITMAP)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("qemu binary lacks pull-mode backup support"));
@@ -17760,10 +17762,6 @@ qemuDomainBackupBegin(virDomainPtr domain, const char 
*diskXml,
_("unexpected transport in "));
 goto cleanup;
 }
-} else {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-   _("push mode backups not supported yet"));
-goto cleanup;
 }
 if (def->incremental) {
 if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BITMAP_MERGE)) {
@@ -17844,6 +17842,7 @@ qemuDomainBackupBegin(virDomainPtr domain, const char 
*diskXml,
 cmd = NULL;
 }

+/* FIXME: allow non-local files for push destinations */
 if (virJSONValueObjectCreate(,
  "s:driver", "file",
  "s:filename", disk->store->path,
@@ -17884,7 +17883,7 @@ qemuDomainBackupBegin(virDomainPtr domain, const char 
*diskXml,
   "blockdev-backup",
   "s:device", src->nodeformat,
   "s:target", disk->store->nodeformat,
-  "s:sync", "none",
+  "s:sync", push ? "full" : "none",
   "s:job-id", disk->name,
   NULL) < 0)
 goto endmon;
@@ -17897,7 +17896,7 @@ qemuDomainBackupBegin(virDomainPtr domain, const char 
*diskXml,
- 

[libvirt] [PATCH v10 02/10] backup: Document new XML for backups

[Eric Blake]

Prepare for new backup APIs by describing the XML that will represent
a backup.  The XML resembles snapshots and checkpoints in being able
to select actions for a set of disks, but has other differences.  It
can support both push model (the hypervisor does the backup directly
into the destination file) and pull model (the hypervisor exposes an
access port for a third party to grab what is necessary).  Add
testsuite coverage for some minimal uses of the XML.

The  element within  tries to model the same
elements as a  under , but sharing the RNG grammar
proved to be hairy. That is in part because while  use
 to describe a host resource in use by the guest, a backup job
is using a host resource that is not visible to the guest: a push
backup action is instead describing a  (which ultimately could
be a remote network resource, but for simplicity the RNG just
validates a local file for now), and a pull backup action is instead
describing a temporary local file  (which probably should not
be a remote resource).  A future refactoring may thus introduce some
way to parameterize RNG to accept ... so that
the name of the subelement can be  for domain, or  or
 as needed for backups. Future patches may improve this area
of code.

Signed-off-by: Eric Blake 
---
 docs/docs.html.in|   3 +-
 docs/format.html.in  |   1 +
 docs/formatbackup.html.in| 184 
 docs/formatcheckpoint.html.in|  12 +-
 docs/index.html.in   |   3 +-
 docs/schemas/domainbackup.rng| 219 +++
 libvirt.spec.in  |   1 +
 mingw-libvirt.spec.in|   2 +
 tests/Makefile.am|   2 +
 tests/domainbackupxml2xmlin/backup-pull.xml  |   9 +
 tests/domainbackupxml2xmlin/backup-push.xml  |   9 +
 tests/domainbackupxml2xmlin/empty.xml|   1 +
 tests/domainbackupxml2xmlout/backup-pull.xml |   9 +
 tests/domainbackupxml2xmlout/backup-push.xml |   9 +
 tests/domainbackupxml2xmlout/empty.xml   |   7 +
 tests/virschematest.c|   2 +
 16 files changed, 465 insertions(+), 8 deletions(-)
 create mode 100644 docs/formatbackup.html.in
 create mode 100644 docs/schemas/domainbackup.rng
 create mode 100644 tests/domainbackupxml2xmlin/backup-pull.xml
 create mode 100644 tests/domainbackupxml2xmlin/backup-push.xml
 create mode 100644 tests/domainbackupxml2xmlin/empty.xml
 create mode 100644 tests/domainbackupxml2xmlout/backup-pull.xml
 create mode 100644 tests/domainbackupxml2xmlout/backup-push.xml
 create mode 100644 tests/domainbackupxml2xmlout/empty.xml

diff --git a/docs/docs.html.in b/docs/docs.html.in
index ba9514279a..b7f3cb4a0d 100644
--- a/docs/docs.html.in
+++ b/docs/docs.html.in
@@ -82,7 +82,8 @@
   node devices,
   secrets,
   snapshots,
-  checkpoints
+  checkpoints,
+  backup jobs

 URI format
 The URI formats used for connecting to libvirt
diff --git a/docs/format.html.in b/docs/format.html.in
index 3be2237663..d013528fe0 100644
--- a/docs/format.html.in
+++ b/docs/format.html.in
@@ -27,6 +27,7 @@
   Secrets
   Snapshots
   Checkpoints
+  Backup jobs
 

 Command line validation
diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in
new file mode 100644
index 00..4f76013d84
--- /dev/null
+++ b/docs/formatbackup.html.in
@@ -0,0 +1,184 @@
+
+
+http://www.w3.org/1999/xhtml;>
+  
+Backup XML format
+
+
+
+Backup XML
+
+
+  Creating a backup, whether full or incremental, is done
+  via virDomainBackupBegin(), which takes an XML
+  description of the actions to perform, as well as an optional
+  second XML document describing a
+  checkpoint to create at the same point in time. See
+  also a comparison between
+  the various state capture APIs.
+
+
+  There are two general modes for backups: a push mode (where the
+  hypervisor writes out the data to the destination file, which
+  may be local or remote), and a pull mode (where the hypervisor
+  creates an NBD server that a third-party client can then read as
+  needed, and which requires the use of temporary storage,
+  typically local, until the backup is complete).
+
+
+  The instructions for beginning a backup job are provided as
+  attributes and elements of the
+  top-level domainbackup element. This element
+  includes an optional attribute mode which can be
+  either "push" or "pull" (default
+  push). virDomainBackupGetXMLDesc() can be used to
+  see the actual values selected for elements omitted during
+  creation (for example, learning which port the NBD server is
+  using in the pull model or what file names libvirt generated
+  when none were supplied). The following child elements are
+  supported:
+
+
+ 

[libvirt] [PATCH v10 01/10] backup: qemu: Implement VIR_DOMAIN_CHECKPOINT_XML_SIZE flag

[Eric Blake]

Once a checkpoint has been created, it is desirable to estimate the
size of the disk delta that is represented between the checkpoint and
the current operation. To do this, we have to scrape information out
of QMP query-block on a request from the user.

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_monitor.h  |  4 ++
 src/qemu/qemu_monitor_json.h |  4 ++
 src/qemu/qemu_driver.c   | 38 -
 src/qemu/qemu_monitor.c  | 11 +
 src/qemu/qemu_monitor_json.c | 80 
 5 files changed, 136 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 88c9702530..9d321bdeda 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -24,6 +24,7 @@
 #include "internal.h"

 #include "domain_conf.h"
+#include "checkpoint_conf.h"
 #include "virbitmap.h"
 #include "virhash.h"
 #include "virjson.h"
@@ -678,6 +679,9 @@ int qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon,
 int qemuMonitorBlockStatsUpdateCapacityBlockdev(qemuMonitorPtr mon,
 virHashTablePtr stats)
 ATTRIBUTE_NONNULL(2);
+int qemuMonitorUpdateCheckpointSize(qemuMonitorPtr mon,
+virDomainCheckpointDefPtr chk)
+ATTRIBUTE_NONNULL(2);

 int qemuMonitorBlockResize(qemuMonitorPtr mon,
const char *device,
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 61e64e831b..3815af6d2b 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -97,6 +97,10 @@ int qemuMonitorJSONBlockResize(qemuMonitorPtr mon,
const char *nodename,
unsigned long long size);

+int qemuMonitorJSONUpdateCheckpointSize(qemuMonitorPtr mon,
+virDomainCheckpointDefPtr chk,
+virDomainObjPtr vm);
+
 int qemuMonitorJSONSetPassword(qemuMonitorPtr mon,
const char *protocol,
const char *password,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 11f97dbc65..ff444ceecd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17420,11 +17420,14 @@ qemuDomainCheckpointGetXMLDesc(virDomainCheckpointPtr 
checkpoint,
 virDomainObjPtr vm = NULL;
 char *xml = NULL;
 virDomainMomentObjPtr chk = NULL;
+qemuDomainObjPrivatePtr priv;
+int rc;
 virDomainCheckpointDefPtr chkdef;
 unsigned int format_flags;

 virCheckFlags(VIR_DOMAIN_CHECKPOINT_XML_SECURE |
-  VIR_DOMAIN_CHECKPOINT_XML_NO_DOMAIN, NULL);
+  VIR_DOMAIN_CHECKPOINT_XML_NO_DOMAIN |
+  VIR_DOMAIN_CHECKPOINT_XML_SIZE, NULL);

 if (!(vm = qemuDomObjFromCheckpoint(checkpoint)))
 return NULL;
@@ -17436,10 +17439,43 @@ qemuDomainCheckpointGetXMLDesc(virDomainCheckpointPtr 
checkpoint,
 goto cleanup;
 chkdef = virDomainCheckpointObjGetDef(chk);

+if (flags & VIR_DOMAIN_CHECKPOINT_XML_SIZE) {
+/* TODO: for non-current checkpoint, this requires a QMP sequence per
+   disk, since the stat of one bitmap in isolation is too low,
+   and merely adding bitmap sizes may be too high:
+ block-dirty-bitmap-create tmp
+ for each bitmap from checkpoint to current:
+   add bitmap to src_list
+ block-dirty-bitmap-merge dst=tmp src_list
+ query-block and read tmp size
+ block-dirty-bitmap-remove tmp
+   So for now, go with simpler query-blocks only for current.
+*/
+if (virDomainCheckpointGetCurrent(vm->checkpoints) != chk) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("cannot compute size for non-current checkpoint 
'%s'"),
+   checkpoint->name);
+goto cleanup;
+}
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+goto cleanup;
+
+if (virDomainObjCheckActive(vm) < 0)
+goto endjob;
+
+priv = vm->privateData;
+qemuDomainObjEnterMonitor(driver, vm);
+rc = qemuMonitorUpdateCheckpointSize(priv->mon, chkdef);
+if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
+goto endjob;
+}
+
 format_flags = virDomainCheckpointFormatConvertXMLFlags(flags);
 xml = virDomainCheckpointDefFormat(chkdef, driver->caps, driver->xmlopt,
format_flags);

+ endjob:
 if (flags & VIR_DOMAIN_CHECKPOINT_XML_SIZE)
 qemuDomainObjEndJob(driver, vm);

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index a880da3ab6..912ff677f2 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2321,6 +2321,17 @@ 
qemuMonitorBlockStatsUpdateCapacityBlockdev(qemuMonitorPtr mon,
 return 

[libvirt] [PATCH v2 8/9] qemu: Implement virDomainGetGuestInfo()

[Jonathon Jongsma]

Iimplements the new guest information API by querying requested
information via the guest agent.

Signed-off-by: Jonathon Jongsma 
---
 src/qemu/qemu_driver.c | 110 +
 1 file changed, 110 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 11f97dbc65..bfbb38a2c8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -23204,6 +23204,115 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain,
 return ret;
 }
 
+static unsigned int supportedGuestInfoTypes =
+VIR_DOMAIN_GUEST_INFO_USERS |
+VIR_DOMAIN_GUEST_INFO_OS |
+VIR_DOMAIN_GUEST_INFO_TIMEZONE |
+VIR_DOMAIN_GUEST_INFO_HOSTNAME |
+VIR_DOMAIN_GUEST_INFO_FILESYSTEM;
+
+static void
+qemuDomainGetGuestInfoCheckSupport(unsigned int *types)
+{
+if (*types == 0)
+*types = supportedGuestInfoTypes;
+
+*types = *types & supportedGuestInfoTypes;
+}
+
+static int
+qemuDomainGetGuestInfo(virDomainPtr dom,
+   unsigned int types,
+   virTypedParameterPtr *params,
+   int *nparams,
+   unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm = NULL;
+qemuAgentPtr agent;
+int ret = -1;
+int rv = -1;
+int maxparams = 0;
+char *hostname = NULL;
+virDomainDefPtr def = NULL;
+virCapsPtr caps = NULL;
+unsigned int supportedTypes = types;
+
+virCheckFlags(0, ret);
+qemuDomainGetGuestInfoCheckSupport();
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+goto cleanup;
+
+if (virDomainGetGuestInfoEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
+goto cleanup;
+
+if (!qemuDomainAgentAvailable(vm, true))
+goto endjob;
+
+agent = qemuDomainObjEnterAgent(vm);
+
+/* Although the libvirt qemu driver supports all of these guest info types,
+ * some guest agents might be too old to support these commands. If these
+ * info categories were explicitly requested (i.e. 'types' is non-zero),
+ * abort and report an error on any failures, otherwise continue and return
+ * as much info as is supported by the guest agent. */
+if (supportedTypes & VIR_DOMAIN_GUEST_INFO_USERS) {
+if (qemuAgentGetUsers(agent, params, nparams, ) < 0 &&
+types != 0)
+goto exitagent;
+}
+if (supportedTypes & VIR_DOMAIN_GUEST_INFO_OS) {
+if (qemuAgentGetOSInfo(agent, params, nparams, ) < 0
+&& types != 0)
+goto exitagent;
+}
+if (supportedTypes & VIR_DOMAIN_GUEST_INFO_TIMEZONE) {
+if (qemuAgentGetTimezone(agent, params, nparams, ) < 0
+&& types != 0)
+goto exitagent;
+}
+if (supportedTypes & VIR_DOMAIN_GUEST_INFO_HOSTNAME) {
+if (qemuAgentGetHostname(agent, ) < 0) {
+if (types != 0)
+goto exitagent;
+} else {
+if (virTypedParamsAddString(params, nparams, , 
"hostname",
+hostname) < 0)
+goto exitagent;
+}
+}
+if (supportedTypes & VIR_DOMAIN_GUEST_INFO_FILESYSTEM) {
+if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
+goto exitagent;
+
+if (!(def = virDomainDefCopy(vm->def, caps, driver->xmlopt, NULL, 
false)))
+goto exitagent;
+
+if (qemuAgentGetFSInfoParams(agent, params, nparams, , def) 
< 0 &&
+types != 0)
+goto exitagent;
+}
+
+rv = 0;
+
+ exitagent:
+qemuDomainObjExitAgent(vm, agent);
+
+ endjob:
+qemuDomainObjEndAgentJob(vm);
+
+ cleanup:
+virDomainObjEndAPI();
+virDomainDefFree(def);
+virObjectUnref(caps);
+VIR_FREE(hostname);
+return rv;
+}
+
 static virHypervisorDriver qemuHypervisorDriver = {
 .name = QEMU_DRIVER_NAME,
 .connectURIProbe = qemuConnectURIProbe,
@@ -23439,6 +23548,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
 .domainCheckpointLookupByName = qemuDomainCheckpointLookupByName, /* 5.6.0 
*/
 .domainCheckpointGetParent = qemuDomainCheckpointGetParent, /* 5.6.0 */
 .domainCheckpointDelete = qemuDomainCheckpointDelete, /* 5.6.0 */
+.domainGetGuestInfo = qemuDomainGetGuestInfo, /* 5.6.0 */
 };
 
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 4/9] qemu: add helper function for querying OS info

[Jonathon Jongsma]

This function queries the guest operating system information and adds
the returned information to an array of typed parameters with field
names intended to be returned in virDomainGetGuestInfo().

Signed-off-by: Jonathon Jongsma 
---
 src/qemu/qemu_agent.c |  53 ++
 src/qemu/qemu_agent.h |   1 +
 tests/qemuagenttest.c | 122 ++
 3 files changed, 176 insertions(+)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 963a4b9359..cd4105bfb8 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -2331,3 +2331,56 @@ qemuAgentGetUsers(qemuAgentPtr mon,
 virJSONValueFree(reply);
 return ret;
 }
+
+int
+qemuAgentGetOSInfo(qemuAgentPtr mon,
+   virTypedParameterPtr *params,
+   int *nparams,
+   int *maxparams)
+{
+int ret = -1;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+virJSONValuePtr data = NULL;
+const char *result;
+
+if (!(cmd = qemuAgentMakeCommand("guest-get-osinfo", NULL)))
+return -1;
+
+if (qemuAgentCommand(mon, cmd, , true,
+ VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+goto cleanup;
+
+if (!(data = virJSONValueObjectGetObject(reply, "return"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("guest-get-osinfo reply was missing return data"));
+goto cleanup;
+}
+
+# define OSINFO_ADD_PARAM(agent_string_, param_string_) \
+do { \
+if ((result = virJSONValueObjectGetString(data, agent_string_))) { \
+if (virTypedParamsAddString(params, nparams, maxparams, \
+param_string_, result) < 0) { \
+goto cleanup; \
+} \
+} \
+} while (0)
+OSINFO_ADD_PARAM("id", "os.id");
+OSINFO_ADD_PARAM("name", "os.name");
+OSINFO_ADD_PARAM("pretty-name", "os.pretty-name");
+OSINFO_ADD_PARAM("version", "os.version");
+OSINFO_ADD_PARAM("version-id", "os.version-id");
+OSINFO_ADD_PARAM("machine", "os.machine");
+OSINFO_ADD_PARAM("variant", "os.variant");
+OSINFO_ADD_PARAM("variant-id", "os.variant-id");
+OSINFO_ADD_PARAM("kernel-release", "os.kernel-release");
+OSINFO_ADD_PARAM("kernel-version", "os.kernel-version");
+
+ret = 0;
+
+ cleanup:
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 05621b521a..ee019455e5 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -122,3 +122,4 @@ int qemuAgentSetUserPassword(qemuAgentPtr mon,
  bool crypted);
 
 int qemuAgentGetUsers(qemuAgentPtr mon, virTypedParameterPtr *params, int 
*nparams, int *maxparams);
+int qemuAgentGetOSInfo(qemuAgentPtr mon, virTypedParameterPtr *params, int 
*nparams, int *maxparams);
diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
index f2936a59f0..9467c51d54 100644
--- a/tests/qemuagenttest.c
+++ b/tests/qemuagenttest.c
@@ -1068,6 +1068,127 @@ testQemuAgentUsers(const void *data)
 return ret;
 }
 
+static const char testQemuAgentOSInfoResponse[] =
+"{\"return\": "
+"   {\"name\":\"CentOS Linux\", "
+"   \"kernel-release\":\"3.10.0-862.14.4.el7.x86_64\", "
+"   \"version\":\"7 (Core)\", "
+"   \"pretty-name\":\"CentOS Linux 7 (Core)\", "
+"   \"version-id\":\"7\", "
+"   \"kernel-version\":\"#1 SMP Wed Sep 26 15:12:11 UTC 2018\", "
+"   \"machine\":\"x86_64\", "
+"   \"id\":\"centos\"} "
+"}";
+
+static const char testQemuAgentOSInfoResponse2[] =
+"{\"return\": "
+"   {\"name\":\"Microsoft Windows\", "
+"   \"kernel-release\":\"7601\", "
+"   \"version\":\"Microsoft Windows 77\", "
+"   \"variant\":\"client\", "
+"   \"pretty-name\":\"Windows 7 Professional\", "
+"   \"version-id\":\"\", "
+"   \"variant-id\":\"client\", "
+"   \"kernel-version\":\"6.1\", "
+"   \"machine\":\"x86_64\", "
+"   \"id\":\"mswindows\"} "
+"}";
+
+static int
+testQemuAgentOSInfo(const void *data)
+{
+virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
+qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt);
+virTypedParameterPtr params = NULL;
+int nparams = 0;
+int maxparams = 0;
+int ret = -1;
+
+if (!test)
+return -1;
+
+if (qemuMonitorTestAddAgentSyncResponse(test) < 0)
+goto cleanup;
+
+if (qemuMonitorTestAddItem(test, "guest-get-osinfo",
+   testQemuAgentOSInfoResponse) < 0)
+goto cleanup;
+
+/* get osinfo */
+if (qemuAgentGetOSInfo(qemuMonitorTestGetAgent(test),
+   , , ) < 0)
+goto cleanup;
+
+if (nparams != 8) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "Expected 8 params, got %d", nparams);
+goto cleanup;
+}
+# define VALIDATE_PARAM(param_name_, expected_) \
+  

[libvirt] [PATCH v2 7/9] qemu: add helper for getting full FSInfo

[Jonathon Jongsma]

This function adds the complete filesystem information returned by the
qemu agent to an array of typed parameters with field names intended to
to be returned by virDomainGetGuestInfo()
---
 src/qemu/qemu_agent.c |  89 ++
 src/qemu/qemu_agent.h |   5 +
 tests/qemuagenttest.c | 210 +++---
 3 files changed, 291 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index d5519cb243..c101805b23 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1953,6 +1953,95 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr 
**info,
 return ret;
 }
 
+int
+qemuAgentGetFSInfoParams(qemuAgentPtr mon,
+ virTypedParameterPtr *params,
+ int *nparams, int *maxparams,
+ virDomainDefPtr vmdef)
+{
+int ret = -1;
+qemuAgentFSInfoPtr *fsinfo = NULL;
+size_t i, j;
+int nfs;
+
+nfs = qemuAgentGetFSInfoInternal(mon, , vmdef);
+
+if (virTypedParamsAddUInt(params, nparams, maxparams,
+  "fs.count", nfs) < 0)
+goto cleanup;
+
+for (i = 0; i < nfs; i++) {
+char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "fs.%zu.name", i);
+if (virTypedParamsAddString(params, nparams, maxparams,
+param_name, fsinfo[i]->name) < 0)
+goto cleanup;
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "fs.%zu.mountpoint", i);
+if (virTypedParamsAddString(params, nparams, maxparams,
+param_name, fsinfo[i]->mountpoint) < 0)
+goto cleanup;
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "fs.%zu.fstype", i);
+if (virTypedParamsAddString(params, nparams, maxparams,
+param_name, fsinfo[i]->fstype) < 0)
+goto cleanup;
+
+/* disk usage values are not returned by older guest agents, so
+ * only add the params if the value is set */
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "fs.%zu.total-bytes", i);
+if (fsinfo[i]->total_bytes != -1 &&
+virTypedParamsAddULLong(params, nparams, maxparams,
+param_name, fsinfo[i]->total_bytes) < 0)
+goto cleanup;
+
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "fs.%zu.used-bytes", i);
+if (fsinfo[i]->used_bytes != -1 &&
+virTypedParamsAddULLong(params, nparams, maxparams,
+param_name, fsinfo[i]->used_bytes) < 0)
+goto cleanup;
+
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "fs.%zu.disk.count", i);
+if (virTypedParamsAddUInt(params, nparams, maxparams,
+  param_name, fsinfo[i]->ndisks) < 0)
+goto cleanup;
+for (j = 0; j < fsinfo[i]->ndisks; j++) {
+qemuAgentDiskInfoPtr d = fsinfo[i]->disks[j];
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "fs.%zu.disk.%zu.alias", i, j);
+if (d->alias &&
+virTypedParamsAddString(params, nparams, maxparams,
+param_name, d->alias) < 0)
+goto cleanup;
+
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "fs.%zu.disk.%zu.serial", i, j);
+if (d->serial &&
+virTypedParamsAddString(params, nparams, maxparams,
+param_name, d->serial) < 0)
+goto cleanup;
+
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "fs.%zu.disk.%zu.device", i, j);
+if (d->devnode &&
+virTypedParamsAddString(params, nparams, maxparams,
+param_name, d->devnode) < 0)
+goto cleanup;
+}
+}
+ret = nfs;
+
+ cleanup:
+for (i = 0; i < nfs; i++)
+qemuAgentFSInfoFree(fsinfo[i]);
+VIR_FREE(fsinfo);
+
+return ret;
+}
 
 static int
 qemuAgentGetFSInfoInternal(qemuAgentPtr mon, qemuAgentFSInfoPtr **info,
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 69b0176855..f6d74a2603 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -74,6 +74,11 @@ int qemuAgentFSThaw(qemuAgentPtr mon);
 int qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
virDomainDefPtr vmdef);
 
+int qemuAgentGetFSInfoParams(qemuAgentPtr mon,
+ virTypedParameterPtr *params,
+ int *nparams, int *maxparams,
+ virDomainDefPtr vmdef);
+
 int qemuAgentSuspend(qemuAgentPtr mon,
  

[libvirt] [PATCH v2 5/9] qemu: add helper for querying timezone info

[Jonathon Jongsma]

This function queries timezone information within the guest and adds
the information to an array of typed parameters with field names
intended to be returned to virDomainGetGuestInfo()

Signed-off-by: Jonathon Jongsma 
---
 src/qemu/qemu_agent.c | 46 ++
 src/qemu/qemu_agent.h |  1 +
 tests/qemuagenttest.c | 76 +++
 3 files changed, 123 insertions(+)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index cd4105bfb8..d7c00250ea 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -2384,3 +2384,49 @@ qemuAgentGetOSInfo(qemuAgentPtr mon,
 virJSONValueFree(reply);
 return ret;
 }
+
+int
+qemuAgentGetTimezone(qemuAgentPtr mon,
+ virTypedParameterPtr *params,
+ int *nparams,
+ int *maxparams)
+{
+int ret = -1;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+virJSONValuePtr data = NULL;
+const char *name;
+int offset;
+
+if (!(cmd = qemuAgentMakeCommand("guest-get-timezone", NULL)))
+return -1;
+
+if (qemuAgentCommand(mon, cmd, , true,
+ VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+goto cleanup;
+
+if (!(data = virJSONValueObjectGetObject(reply, "return"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("guest-get-timezone reply was missing return data"));
+goto cleanup;
+}
+
+if ((name = virJSONValueObjectGetString(data, "zone")) == NULL)
+goto cleanup;
+if (virTypedParamsAddString(params, nparams, maxparams,
+"timezone.name", name) < 0)
+goto cleanup;
+
+if ((virJSONValueObjectGetNumberInt(data, "offset", )) < 0)
+goto cleanup;
+if (virTypedParamsAddInt(params, nparams, maxparams,
+ "timezone.offset", offset) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index ee019455e5..69b0176855 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -123,3 +123,4 @@ int qemuAgentSetUserPassword(qemuAgentPtr mon,
 
 int qemuAgentGetUsers(qemuAgentPtr mon, virTypedParameterPtr *params, int 
*nparams, int *maxparams);
 int qemuAgentGetOSInfo(qemuAgentPtr mon, virTypedParameterPtr *params, int 
*nparams, int *maxparams);
+int qemuAgentGetTimezone(qemuAgentPtr mon, virTypedParameterPtr *params, int 
*nparams, int *maxparams);
diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
index 9467c51d54..aa1e993649 100644
--- a/tests/qemuagenttest.c
+++ b/tests/qemuagenttest.c
@@ -1189,7 +1189,82 @@ testQemuAgentOSInfo(const void *data)
 return ret;
 }
 
+static const char testQemuAgentTimezoneResponse1[] =
+"{\"return\":{\"zone\":\"IST\",\"offset\":19800}}";
+static const char testQemuAgentTimezoneResponse2[] =
+"{\"return\":{\"zone\":\"CEST\",\"offset\":7200}}";
+static const char testQemuAgentTimezoneResponse3[] =
+"{\"return\":{\"zone\":\"NDT\",\"offset\":-9000}}";
+static const char testQemuAgentTimezoneResponse4[] =
+"{\"return\":{\"zone\":\"PDT\",\"offset\":-25200}}";
 
+static int
+testQemuAgentTimezone(const void *data)
+{
+virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
+qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt);
+int ret = -1;
+
+if (!test)
+return -1;
+
+#define VALIDATE_TIMEZONE(response_, expected_name_, expected_offset_) \
+do { \
+virTypedParameterPtr params_ = NULL; \
+int nparams_ = 0; \
+int maxparams_ = 0; \
+const char *name_ = NULL; \
+int offset_; \
+if (qemuMonitorTestAddAgentSyncResponse(test) < 0) \
+goto cleanup; \
+if (qemuMonitorTestAddItem(test, "guest-get-timezone", \
+   response_) < 0) \
+goto cleanup; \
+if (qemuAgentGetTimezone(qemuMonitorTestGetAgent(test), \
+ _, _, _) < 0) \
+goto cleanup; \
+if (nparams_ != 2) { \
+virReportError(VIR_ERR_INTERNAL_ERROR, \
+   "Expected 2 params, got %d", nparams_); \
+goto cleanup; \
+} \
+if (virTypedParamsGetString(params_, nparams_, \
+"timezone.name", _) < 0) { \
+virReportError(VIR_ERR_INTERNAL_ERROR, "missing param '%s'", \
+   "tiemzone.name"); \
+goto cleanup; \
+} \
+if (STRNEQ(name_, expected_name_)) { \
+virReportError(VIR_ERR_INTERNAL_ERROR, \
+   "Expected name '%s', got '%s'", expected_name_, 
name_); \
+goto cleanup; \
+} \
+if (virTypedParamsGetInt(params_, nparams_, \
+ "timezone.offset", _) < 0) { \
+

[libvirt] [PATCH v2 3/9] qemu: add helper for getting guest users

[Jonathon Jongsma]

This function fetches the list of logged-in users from the qemu agent
and adds them to a list of typed parameters so that they can be used
internally in libvirt.

Also add some basic tests for the function.

Signed-off-by: Jonathon Jongsma 
---
 src/qemu/qemu_agent.c |  91 +++
 src/qemu/qemu_agent.h |   2 +
 tests/qemuagenttest.c | 168 ++
 3 files changed, 261 insertions(+)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 361db299a5..963a4b9359 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -2240,3 +2240,94 @@ qemuAgentSetUserPassword(qemuAgentPtr mon,
 VIR_FREE(password64);
 return ret;
 }
+
+int
+qemuAgentGetUsers(qemuAgentPtr mon,
+  virTypedParameterPtr *params,
+  int *nparams,
+  int *maxparams)
+{
+int ret = -1;
+size_t i;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+virJSONValuePtr data = NULL;
+size_t ndata;
+const char *strvalue;
+
+if (!(cmd = qemuAgentMakeCommand("guest-get-users", NULL)))
+return -1;
+
+if (qemuAgentCommand(mon, cmd, , true,
+ VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+goto cleanup;
+
+if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("guest-get-users reply was missing return data"));
+goto cleanup;
+}
+
+if (!virJSONValueIsArray(data)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Malformed guest-get-users data array"));
+goto cleanup;
+}
+
+ndata = virJSONValueArraySize(data);
+
+if (virTypedParamsAddUInt(params, nparams, maxparams,
+  "user.count", ndata) < 0)
+goto cleanup;
+
+for (i = 0; i < ndata; i++) {
+char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
+virJSONValuePtr entry = virJSONValueArrayGet(data, i);
+
+if (!entry) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("array element missing in guest-get-users return "
+ "value"));
+goto cleanup;
+}
+
+if (!(strvalue = virJSONValueObjectGetString(entry, "user"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("'user' missing in reply of guest-get-users"));
+goto cleanup;
+}
+
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "user.%zu.name", i);
+if (virTypedParamsAddString(params, nparams, maxparams,
+param_name, strvalue) < 0)
+goto cleanup;
+
+/* 'domain' is only present for windows guests */
+if ((strvalue = virJSONValueObjectGetString(entry, "domain"))) {
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "user.%zu.domain", i);
+if (virTypedParamsAddString(params, nparams, maxparams,
+param_name, strvalue) < 0)
+goto cleanup;
+}
+
+double logintime;
+if (virJSONValueObjectGetNumberDouble(entry, "login-time", ) 
< 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("'login-time' missing in reply of 
guest-get-users"));
+goto cleanup;
+}
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "user.%zu.login-time", i);
+if (virTypedParamsAddULLong(params, nparams, maxparams,
+param_name, logintime * 1000) < 0)
+goto cleanup;
+}
+
+ret = ndata;
+
+ cleanup:
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 6ae9fe54da..05621b521a 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -120,3 +120,5 @@ int qemuAgentSetUserPassword(qemuAgentPtr mon,
  const char *user,
  const char *password,
  bool crypted);
+
+int qemuAgentGetUsers(qemuAgentPtr mon, virTypedParameterPtr *params, int 
*nparams, int *maxparams);
diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
index 2f79986207..f2936a59f0 100644
--- a/tests/qemuagenttest.c
+++ b/tests/qemuagenttest.c
@@ -902,6 +902,173 @@ testQemuAgentGetInterfaces(const void *data)
 return ret;
 }
 
+static const char testQemuAgentUsersResponse[] =
+"{\"return\": "
+"   ["
+"   {\"user\": \"test\","
+"\"login-time\": 1561739203.584038"
+"   },"
+"   {\"user\": \"test2\","
+"\"login-time\": 1561739229.190697"
+"   }"
+"   ]"
+"}";
+
+static const char testQemuAgentUsersResponse2[] =
+"{\"return\": "
+"   ["
+"   {\"user\": \"test\","
+

[libvirt] [PATCH v2 6/9] qemu: add support for new fields in FSInfo

[Jonathon Jongsma]

Since version 3.0, qemu has returned disk usage statistics in
guest-get-fsinfo. And since 3.1, it has returned information about the
disk serial number and device node of disks that are targeted by the
filesystem.

Unfortunately, the public API virDomainGetFSInfo() returns the
filesystem info using a virDomainFSInfo struct, and due to API/ABI
guaranteeds it cannot be extended. So this new information cannot
easily be added to the public API. However, it is possible to add this
new filesystem information to a new virDomainGetGuestInfo() API which
will be based on typed parameters and is thus more extensible.

In order to support these two use cases, I added an internal struct
which the agent code uses to return all of the new data fields. This
internal struct can be converted to the public struct at a cost of some
extra memory allocation.

In a following commit, this additional information will be used within
virDomainGetGuestInfo().

Signed-off-by: Jonathon Jongsma 
---
 src/qemu/qemu_agent.c | 203 +-
 1 file changed, 182 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index d7c00250ea..d5519cb243 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1827,19 +1827,144 @@ qemuAgentSetTime(qemuAgentPtr mon,
 return ret;
 }
 
+typedef struct _qemuAgentDiskInfo qemuAgentDiskInfo;
+typedef qemuAgentDiskInfo *qemuAgentDiskInfoPtr;
+struct _qemuAgentDiskInfo {
+char *alias;
+char *serial;
+char *devnode;
+};
+
+typedef struct _qemuAgentFSInfo qemuAgentFSInfo;
+typedef qemuAgentFSInfo *qemuAgentFSInfoPtr;
+struct _qemuAgentFSInfo {
+char *mountpoint; /* path to mount point */
+char *name;   /* device name in the guest (e.g. "sda1") */
+char *fstype; /* filesystem type */
+long long total_bytes;
+long long used_bytes;
+size_t ndisks;
+qemuAgentDiskInfoPtr *disks;
+};
+
+static void
+qemuAgentDiskInfoFree(qemuAgentDiskInfoPtr info)
+{
+if (!info)
+return;
+
+VIR_FREE(info->serial);
+VIR_FREE(info->alias);
+VIR_FREE(info->devnode);
+VIR_FREE(info);
+}
+
+static void
+qemuAgentFSInfoFree(qemuAgentFSInfoPtr info)
+{
+size_t i;
+
+if (!info)
+return;
+
+VIR_FREE(info->mountpoint);
+VIR_FREE(info->name);
+VIR_FREE(info->fstype);
+
+for (i = 0; i < info->ndisks; i++)
+qemuAgentDiskInfoFree(info->disks[i]);
+VIR_FREE(info->disks);
+
+VIR_FREE(info);
+}
+
+static virDomainFSInfoPtr
+qemuAgentFSInfoToPublic(qemuAgentFSInfoPtr agent)
+{
+virDomainFSInfoPtr ret = NULL;
+size_t n;
+
+if (VIR_ALLOC(ret) < 0)
+return NULL;
 
+if (VIR_STRDUP(ret->name, agent->name) < 0)
+goto error;
+if (VIR_STRDUP(ret->mountpoint, agent->mountpoint) < 0)
+goto error;
+if (VIR_STRDUP(ret->fstype, agent->fstype) < 0)
+goto error;
+
+ret->ndevAlias = agent->ndisks;
+
+if (ret->ndevAlias == 0)
+return ret;
+
+if (VIR_ALLOC_N(ret->devAlias, ret->ndevAlias) < 0)
+goto error;
+
+for (n = 0; n < ret->ndevAlias; n++) {
+if (VIR_STRDUP(ret->devAlias[n], agent->disks[n]->alias) < 0)
+goto error;
+}
+
+return ret;
+
+ error:
+virDomainFSInfoFree(ret);
+return NULL;
+}
+
+static int
+qemuAgentGetFSInfoInternal(qemuAgentPtr mon, qemuAgentFSInfoPtr **info,
+   virDomainDefPtr vmdef);
 int
 qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
virDomainDefPtr vmdef)
+{
+int ret = -1;
+qemuAgentFSInfoPtr *agentinfo = NULL;
+virDomainFSInfoPtr *info_ret = NULL;
+size_t i;
+int nfs;
+
+nfs = qemuAgentGetFSInfoInternal(mon, , vmdef);
+if (nfs < 0)
+return ret;
+if (VIR_ALLOC_N(info_ret, nfs) < 0)
+goto cleanup;
+
+for (i = 0; i < nfs; i++) {
+if (!(info_ret[i] = qemuAgentFSInfoToPublic(agentinfo[i])))
+goto cleanup;
+}
+
+*info = info_ret;
+info_ret = NULL;
+ret = nfs;
+
+ cleanup:
+for (i = 0; i < nfs; i++) {
+qemuAgentFSInfoFree(agentinfo[i]);
+/* if there was an error, free any memory we've allocated for the
+ * return value */
+if (info_ret)
+virDomainFSInfoFree(info_ret[i]);
+}
+return ret;
+}
+
+
+static int
+qemuAgentGetFSInfoInternal(qemuAgentPtr mon, qemuAgentFSInfoPtr **info,
+   virDomainDefPtr vmdef)
 {
 size_t i, j, k;
 int ret = -1;
-size_t ndata = 0, ndisk;
-char **alias;
+size_t ndata = 0;
 virJSONValuePtr cmd;
 virJSONValuePtr reply = NULL;
 virJSONValuePtr data;
-virDomainFSInfoPtr *info_ret = NULL;
+qemuAgentFSInfoPtr *info_ret = NULL;
 virPCIDeviceAddress pci_address;
 const char *result = NULL;
 
@@ -1915,6 +2040,33 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr 
**info,
 if (VIR_STRDUP(info_ret[i]->fstype, result) < 

[libvirt] [PATCH v2 9/9] virsh: add 'guestinfo' command

[Jonathon Jongsma]

The 'guestinfo' command uses the new virDomainGetGuestInfo() API to
query information about the specified domain and print it out for the
user. The output is modeled roughly on the 'domstats' command.

Signed-off-by: Jonathon Jongsma 
---
 tools/virsh-domain.c | 85 
 1 file changed, 85 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index ccda71d7e0..977783951d 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -14038,6 +14038,85 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd)
 return ret;
 }
 
+/*
+ * "guestinfo" command
+ */
+static const vshCmdInfo info_guestinfo[] = {
+{.name = "help",
+ .data = N_("query information about the guest (via agent)")
+},
+{.name = "desc",
+ .data = N_("Use the guest agent to query various information from guest's 
"
+"point of view")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_guestinfo[] = {
+VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE),
+{.name = "user",
+ .type = VSH_OT_BOOL,
+ .help = N_("report active users"),
+},
+{.name = "os",
+ .type = VSH_OT_BOOL,
+ .help = N_("report operating system information"),
+},
+{.name = "timezone",
+ .type = VSH_OT_BOOL,
+ .help = N_("report timezone information"),
+},
+{.name = "hostname",
+ .type = VSH_OT_BOOL,
+ .help = N_("report hostname"),
+},
+{.name = "filesystem",
+ .type = VSH_OT_BOOL,
+ .help = N_("report filesystem information"),
+},
+{.name = NULL}
+};
+
+static bool
+cmdGuestInfo(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom;
+bool ret = false;
+virTypedParameterPtr params = NULL;
+int nparams = 0;
+size_t i;
+unsigned int types = 0;
+
+if (vshCommandOptBool(cmd, "user"))
+types |= VIR_DOMAIN_GUEST_INFO_USERS;
+if (vshCommandOptBool(cmd, "os"))
+types |= VIR_DOMAIN_GUEST_INFO_OS;
+if (vshCommandOptBool(cmd, "timezone"))
+types |= VIR_DOMAIN_GUEST_INFO_TIMEZONE;
+if (vshCommandOptBool(cmd, "hostname"))
+types |= VIR_DOMAIN_GUEST_INFO_HOSTNAME;
+if (vshCommandOptBool(cmd, "filesystem"))
+types |= VIR_DOMAIN_GUEST_INFO_FILESYSTEM;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+return false;
+
+if (virDomainGetGuestInfo(dom, types, , , 0) < 0)
+goto cleanup;
+
+for (i = 0; i < nparams; i++) {
+char *str = vshGetTypedParamValue(ctl, [i]);
+vshPrint(ctl, "%-20s: %s\n", params[i].field, str);
+VIR_FREE(str);
+}
+
+ret = true;
+
+ cleanup:
+virshDomainFree(dom);
+return ret;
+}
+
 const vshCmdDef domManagementCmds[] = {
 {.name = "attach-device",
  .handler = cmdAttachDevice,
@@ -14653,5 +14732,11 @@ const vshCmdDef domManagementCmds[] = {
  .info = info_domblkthreshold,
  .flags = 0
 },
+{.name = "guestinfo",
+ .handler = cmdGuestInfo,
+ .opts = opts_guestinfo,
+ .info = info_guestinfo,
+ .flags = 0
+},
 {.name = NULL}
 };
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 2/9] remote: implement virDomainGetGuestInfo

[Jonathon Jongsma]

Add daemon and client code to serialize/deserialize
virDomainGetGuestInfo().

Signed-off-by: Jonathon Jongsma 
---
 src/remote/remote_daemon_dispatch.c | 41 ++
 src/remote/remote_driver.c  | 53 +
 src/remote/remote_protocol.x| 21 +++-
 src/remote_protocol-structs | 12 +++
 4 files changed, 126 insertions(+), 1 deletion(-)

diff --git a/src/remote/remote_daemon_dispatch.c 
b/src/remote/remote_daemon_dispatch.c
index 1bd281dd6d..665d938a99 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -7650,3 +7650,44 @@ remoteSerializeDomainDiskErrors(virDomainDiskErrorPtr 
errors,
 }
 return -1;
 }
+
+static int
+remoteDispatchDomainGetGuestInfo(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
+ remote_domain_get_guest_info_args *args,
+ remote_domain_get_guest_info_ret *ret)
+{
+int rv = -1;
+struct daemonClientPrivate *priv = 
virNetServerClientGetPrivateData(client);
+virDomainPtr dom = NULL;
+virTypedParameterPtr params = NULL;
+int nparams = 0;
+
+if (!priv->conn) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
+goto cleanup;
+}
+
+if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
+goto cleanup;
+
+if (virDomainGetGuestInfo(dom, args->types, , , 
args->flags) < 0)
+goto cleanup;
+
+if (virTypedParamsSerialize(params, nparams,
+(virTypedParameterRemotePtr *) 
>params.params_val,
+>params.params_len,
+VIR_TYPED_PARAM_STRING_OKAY) < 0)
+goto cleanup;
+
+rv = 0;
+
+ cleanup:
+if (rv < 0)
+virNetMessageSaveError(rerr);
+virObjectUnref(dom);
+
+return rv;
+}
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index daac506672..5ba144648a 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8306,6 +8306,58 @@ remoteNetworkPortGetParameters(virNetworkPortPtr port,
 return rv;
 }
 
+static int
+remoteDomainGetGuestInfo(virDomainPtr dom,
+ unsigned int types,
+ virTypedParameterPtr *params,
+ int *nparams,
+ unsigned int flags)
+{
+int rv = -1;
+struct private_data *priv = dom->conn->privateData;
+remote_domain_get_guest_info_args args;
+remote_domain_get_guest_info_ret ret;
+
+remoteDriverLock(priv);
+
+make_nonnull_domain(, dom);
+
+args.types = types;
+args.flags = flags;
+
+memset(, 0, sizeof(ret));
+
+if (call(dom->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_GUEST_INFO,
+ (xdrproc_t)xdr_remote_domain_get_guest_info_args, (char *),
+ (xdrproc_t)xdr_remote_domain_get_guest_info_ret, (char *)) == 
-1)
+goto done;
+
+if (ret.params.params_len > REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Too many params in guestinfo: %d for limit %d"),
+   ret.params.params_len, 
REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX);
+goto cleanup;
+}
+
+if (params) {
+if (virTypedParamsDeserialize((virTypedParameterRemotePtr) 
ret.params.params_val,
+  ret.params.params_len,
+  REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX,
+  params,
+  nparams) < 0)
+goto cleanup;
+}
+
+rv = 0;
+
+ cleanup:
+xdr_free((xdrproc_t)xdr_remote_domain_get_guest_info_ret,
+ (char *) );
+
+ done:
+remoteDriverUnlock(priv);
+return rv;
+}
 
 /* get_nonnull_domain and get_nonnull_network turn an on-wire
  * (name, uuid) pair into virDomainPtr or virNetworkPtr object.
@@ -8733,6 +8785,7 @@ static virHypervisorDriver hypervisor_driver = {
 .domainCheckpointLookupByName = remoteDomainCheckpointLookupByName, /* 
5.6.0 */
 .domainCheckpointGetParent = remoteDomainCheckpointGetParent, /* 5.6.0 */
 .domainCheckpointDelete = remoteDomainCheckpointDelete, /* 5.6.0 */
+.domainGetGuestInfo = remoteDomainGetGuestInfo, /* 5.6.0 */
 };
 
 static virNetworkDriver network_driver = {
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 118369e2b3..75c2bc69ff 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -269,6 +269,9 @@ const REMOTE_NODE_SEV_INFO_MAX = 64;
 /* Upper limit on number of launch security information entries */
 const REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX = 64;
 
+/* Upper limit on number of 

[libvirt] [PATCH v2 0/9] Add virDomainGetGuestInfo()

[Jonathon Jongsma]

This series adds several bits of guest information provided by a new API
function virDomainGetGuestInfo(). There is an implementation for qemu using the
guest agent. In particular, it adds information about logged-in users, guest
OS, timezone, hostname, and filesystem info.

This is the second version of this patch series. Various things were improved,
but the biggest difference is that this version includes filesystem
information. Filesystem information is already accessible via the public API
virDomainGetFSInfo(), but recent versions of the qemu guest agent added
additional information, and the existing public API is not able to be extended
without breaking API since it returns a C struct. This new API uses typed
parameters so that it can be extended as necessary when new fields are added.

The overall API follows the bulk stats API quite closely, and tries to return
data in similar ways (for example, the "users.N.*" field name scheme was
inspired by various stats fields).

It should be noted that like version 1 of this patch series, the API still only
operates on a single domain, not a list of domains. I'm willing to consider
changing the API to operate on a list of domains if that is the concensus, but
I lean toward keeping it simpler.

Here's an example of the output of the virsh command added in this patch
operating on a fedora 30 guest:
virsh # guestinfo fedora30
user.count  : 1
user.0.name : test
user.0.login-time   : 1566422940895
os.id   : fedora
os.name : Fedora
os.pretty-name  : Fedora 30 (Workstation Edition)
os.version  : 30 (Workstation Edition)
os.version-id   : 30
os.machine  : x86_64
os.variant  : Workstation Edition
os.variant-id   : workstation
os.kernel-release   : 5.2.7-200.fc30.x86_64
os.kernel-version   : #1 SMP Thu Aug 8 05:35:29 UTC 2019
timezone.name   : CDT
timezone.offset : -18000
hostname: myhostname
fs.count: 3
fs.0.name   : dm-0
fs.0.mountpoint : /
fs.0.fstype : ext4
fs.0.total-bytes: 25928306688
fs.0.used-bytes : 10762133504
fs.0.disk.count : 1
fs.0.disk.0.alias   : vda
fs.0.disk.0.serial  : 947684ABZ8639Q
fs.0.disk.0.device  : /dev/vda2
fs.1.name   : vda1
fs.1.mountpoint : /boot
fs.1.fstype : ext4
fs.1.total-bytes: 952840192
fs.1.used-bytes : 229019648
fs.1.disk.count : 1
fs.1.disk.0.alias   : vda
fs.1.disk.0.serial  : 947684ABZ8639Q
fs.1.disk.0.device  : /dev/vda1
fs.2.name   : md127
fs.2.mountpoint : /run/media/test/971b1edc-da61-4015-b465-560a9b1b6e9b
fs.2.fstype : ext4
fs.2.total-bytes: 1950134272
fs.2.used-bytes : 6291456
fs.2.disk.count : 2
fs.2.disk.0.alias   : vdb
fs.2.disk.0.device  : /dev/vdb1
fs.2.disk.1.alias   : vdc
fs.2.disk.1.device  : /dev/vdc1

In contrast, here is the output of the virsh command operating on a fedora24
guest:
virsh # guestinfo  fedora24
error: Reconnected to the hypervisor
fs.count: 2
fs.0.name   : dm-0
fs.0.mountpoint : /
fs.0.fstype : ext4
fs.0.disk.count : 1
fs.0.disk.0.alias   : vda
fs.1.name   : vda1
fs.1.mountpoint : /boot
fs.1.fstype : ext4
fs.1.disk.count : 1
fs.1.disk.0.alias   : vda

However, if you specifically request an info category that is not supported by
the guest agent:
virsh # guestinfo --user fedora24
error: internal error: unable to execute QEMU agent command 'guest-get-users': 
The command guest-get-users has not been found


Jonathon Jongsma (9):
  lib: add virDomainGetGuestInfo()
  remote: implement virDomainGetGuestInfo
  qemu: add helper for getting guest users
  qemu: add helper function for querying OS info
  qemu: add helper for querying timezone info
  qemu: add support for new fields in FSInfo
  qemu: add helper for getting full FSInfo
  qemu: Implement virDomainGetGuestInfo()
  virsh: add 'guestinfo' command

 include/libvirt/libvirt-domain.h|  14 +
 src/driver-hypervisor.h |   8 +
 src/libvirt-domain.c| 117 ++
 src/libvirt_public.syms |   1 +
 src/qemu/qemu_agent.c   | 482 ++-
 src/qemu/qemu_agent.h   |   9 +
 src/qemu/qemu_driver.c  | 110 ++
 src/remote/remote_daemon_dispatch.c |  41 ++
 src/remote/remote_driver.c  |  53 +++
 src/remote/remote_protocol.x|  21 +-
 src/remote_protocol-structs |  12 +
 tests/qemuagenttest.c   | 576 +++-
 tools/virsh-domain.c|  85 
 13 files changed, 1494 insertions(+), 35 deletions(-)

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 1/9] lib: add virDomainGetGuestInfo()

[Jonathon Jongsma]

This API is intended to aggregate several guest agent information
queries and is ispired by stats API virDomainListGetStats(). It is
anticipated that this information will be provided by a guest agent
running within the domain.

Signed-off-by: Jonathon Jongsma 
---
 include/libvirt/libvirt-domain.h |  14 
 src/driver-hypervisor.h  |   8 +++
 src/libvirt-domain.c | 117 +++
 src/libvirt_public.syms  |   1 +
 4 files changed, 140 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index f160ee88b5..22277b0a84 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -4902,4 +4902,18 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain,
int *nparams,
unsigned int flags);
 
+typedef enum {
+VIR_DOMAIN_GUEST_INFO_USERS = (1 << 0), /* return active users */
+VIR_DOMAIN_GUEST_INFO_OS = (1 << 1), /* return OS information */
+VIR_DOMAIN_GUEST_INFO_TIMEZONE = (1 << 2), /* return timezone information 
*/
+VIR_DOMAIN_GUEST_INFO_HOSTNAME = (1 << 3), /* return hostname information 
*/
+VIR_DOMAIN_GUEST_INFO_FILESYSTEM = (1 << 4), /* return filesystem 
information */
+} virDomainGuestInfoTypes;
+
+int virDomainGetGuestInfo(virDomainPtr domain,
+  unsigned int types,
+  virTypedParameterPtr *params,
+  int *nparams,
+  unsigned int flags);
+
 #endif /* LIBVIRT_DOMAIN_H */
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index c1632ae4c6..58eb731e85 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -1359,6 +1359,13 @@ typedef int
 (*virDrvDomainCheckpointDelete)(virDomainCheckpointPtr checkpoint,
 unsigned int flags);
 
+typedef int
+(*virDrvDomainGetGuestInfo)(virDomainPtr domain,
+unsigned int types,
+virTypedParameterPtr *params,
+int *nparams,
+unsigned int flags);
+
 typedef struct _virHypervisorDriver virHypervisorDriver;
 typedef virHypervisorDriver *virHypervisorDriverPtr;
 
@@ -1617,4 +1624,5 @@ struct _virHypervisorDriver {
 virDrvDomainCheckpointLookupByName domainCheckpointLookupByName;
 virDrvDomainCheckpointGetParent domainCheckpointGetParent;
 virDrvDomainCheckpointDelete domainCheckpointDelete;
+virDrvDomainGetGuestInfo domainGetGuestInfo;
 };
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 2fe9bb8e91..ad68db7549 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -12212,6 +12212,123 @@ virDomainSetVcpu(virDomainPtr domain,
 return -1;
 }
 
+/**
+ * virDomainGetGuestInfo:
+ * @domain: pointer to domain object
+ * @types: types of information to return, binary-OR of virDomainGuestInfoTypes
+ * @params: location to store the guest info parameters
+ * @nparams: number of items in @params
+ * @flags: currently unused, set to 0
+ *
+ * Queries the guest agent for the various information about the guest system.
+ * The reported data depends on the guest agent implementation. the information
+ * is returned as an array of typed parameters containing the individual
+ * parameters. The parameter name for each information field consists of a
+ * dot-separated strign containing the name of the requested group followed by
+ * a group-specific description of the statistic value.
+ *
+ * The information groups are enabled using the @types parameter which is a
+ * binary-OR of enum virDomainGuestInfoTypes. The following groups are 
available
+ * (although not necessarily implemented for each hypervisor):
+ *
+ * VIR_DOMAIN_GUEST_INFO_USERS:
+ *  returns information about users that are currently logged in within the
+ *  guest domain. The typed parameter keys are in this format:
+ *
+ *  "user.count" - the number of active users on this domain as an
+ * unsigned int
+ *  "user..name - username of the user as a string
+ *  "user..domain - domain of the user as a string (may only be
+ *   present on certain guest types)
+ *  "user..login-time - the login time of a user in milliseconds
+ *   since the epoch as unsigned long long
+ *
+ * VIR_DOMAIN_GUEST_INFO_OS:
+ *  Return information about the operating system running within the guest. The
+ *  typed parameter keys are in this format:
+ *
+ *  "os.id" - a string identifying the operating system
+ *  "os.name" - the name of the operating system, suitable for presentation
+ *  to a user, as a string
+ *  "os.pretty-name" - a pretty name for the operating system, suitable for
+ * presentation to a user, as a string
+ *  "os.version" - the version of the operating system suitable for
+ *

Re: [libvirt] QEMU bitmap backup usability FAQ

[John Snow]

On 8/21/19 10:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> [CC Nikolay]
> 
> 21.08.2019 1:25, John Snow wrote:
>> Hi, downstream here at Red Hat I've been fielding some questions about
>> the usability and feature readiness of Bitmaps (and related features) in
>> QEMU.
>>
>> Here are some questions I answered internally that I am copying to the
>> list for two reasons:
>>
>> (1) To make sure my answers are actually correct, and
>> (2) To share this pseudo-reference with the community at large.
>>
>> This is long, and mostly for reference. There's a summary at the bottom
>> with some todo items and observations about the usability of the feature
>> as it exists in QEMU.
>>
>> Before too long, I intend to send a more summarized "roadmap" mail which
>> details all of the current and remaining work to be done in and around
>> the bitmaps feature in QEMU.
>>
>>
>> Questions:
>>
>>> "What format(s) is/are required for this functionality?"
>>
>>  From the QEMU API, any format can be used to create and author
>> incremental backups. The only known format limitations are:
>>
>> 1. Persistent bitmaps cannot be created on any format except qcow2,
>> although there are hooks to add support to other formats at a later date
>> if desired.
>>
>> DANGER CAVEAT #1: Adding bitmaps to QEMU by default creates transient
>> bitmaps instead of persistent ones.
>>
>> Possible TODO: Allow users to 'upgrade' transient bitmaps to persistent
>> ones in case they made a mistake.
> 
> I doubt, as in my opinion real users of Qemu are not people but libvirt, which
> should never make such mistake.
> 

Right, that's largely been the consensus here; but there is some concern
that libvirt might not be the only user of QEMU, so I felt it was worth
documenting some of the crucial moments where it was "easy" to get it wrong.

I think like it or not, the API that QEMU presents has to be considered
on its own without libvirt because it's not a given that libvirt will
forever and always be the only user of QEMU.

I do think that any problems of this kind that can be solved in libvirt
are not immediate, crucial action items. libvirt WILL be the major user
of these features.

However, try as we might, releasing a set of primitive operations that
offer 998 ways to corrupt your data and 2 ways to manage it correctly
are going to provoke some questions from people who are trying to work
with that API, including from libvirt developers.

It might be the conclusion that it's libvirt's job to safeguard the user
from themselves, but we at least need to present consistent and clear
information about the way we expect/anticipate people to use the APIs,
because people DO keep asking me about several of these issues and the
usability problems they perceive with the QEMU API.

So this thread was largely in attempt to explore what some "solutions"
to perceived problems look like, mostly to come to the conclusion that
the actual "must-haves" list in QEMU is not very long compared to the
"nice-to-haves?" list.

>>
>>
>> 2. When using push backups (blockdev-backup, drive-backup), you may use
>> any format as a target format. >
>> DANGER CAVEAT #2: without backing file and/or filesystem-less sparse
>> support, these images will be unusable.
> 
> You mean incremental backups of course, as the whole document is about 
> bitmaps.
> 

Ah, yes, incremental push backups. Full backups are of course not a
problem. :)

>>
>> EXAMPLE: Backing up to a raw file loses allocation information, so we
>> can no longer distinguish between zeroes and unallocated regions. The
>> cluster size is also lost. This file will not be usable without
>> additional metadata recorded elsewhere.*
>>
>> (* This is complicated, but it is in theory possible to do a push backup
>> to e.g. an NBD target with custom server code that saves allocation
>> information to a metadata file, which would allow you to reconstruct
>> backups. For instance, recording in a .json file which extents were
>> written out would allow you to -- with a custom binary -- write this
>> information on top of a base file to reconstruct a backup.)
>>
>>
>> 3. Any format can be used for either shared storage or live storage
>> migrations. There are TWO distinct mechanisms for migrating bitmaps:
>>
>> A) The bitmap is flushed to storage and re-opened on the destination.
>> This is only supported for qcow2 and shared-storage migrations.
> 
> cons: flushing/reopening is done during migration downtime, so if you have
> a lot of bitmap data (for example, 64k granulared bitmap for 16tb disk is
> ~30MB, and there may be several bitmaps) downtime will become long.
> 

Worth documenting the drawback, yes.

>>
>> B) The bitmap is live-migrated to the destination. This is supported for
>> any format and can be used for either shared storage or live storage
>> migrations.
>>
>> DANGER CAVEAT #3: The second bitmap migration technique there is an
>> optional migration capability that must be enabled explicitly.
>> Otherwise, some 

Re: [libvirt] Fwd: libvirtd failing on MacOS in setgroups

[Roman Bolshakov]

On Wed, Aug 21, 2019 at 05:55:51PM +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 21, 2019 at 12:47:03PM -0400, Marcus Furlong wrote:
> > On Wed, 21 Aug 2019 at 08:23, Daniel P. Berrangé  
> > wrote:
> > >
> > > On Tue, Aug 20, 2019 at 11:11:07AM -0400, Marcus Furlong wrote:
> > > > Resend to libvir-list in case that is more appropriate:
> > > >
> > > >
> > > > Hi,
> > > >
> > > > I get the following error when running libvirtd on MacOS as root:
> > > >
> > > > 2019-07-11 00:12:33.673+: 123145573953536: error :
> > > > qemuProcessQMPLaunch:8501 : internal error: Failed to start QEMU
> > > > binary /usr/local/bin/qemu-system-x86_64 for probing: libvirt:  error
> > > > : cannot set supplemental groups: Invalid argument
> > >
> > > Are you able to run 'strace' (or whatever MacOS eqiuv is) to see
> > > the values passed to setgroups when it fails ?
> > 
> > I ran `dtruss -f -l -s /usr/local/sbin/libvirtd` but the setgroups
> > calls seem to be missing.
> > 
> > Looking at other sources, it seems like some have special treatment of
> > setgroups on MacOS, e.g. samba:
> > 
> >
> > https://github.com/samba-team/samba/blob/v4-11-stable/source3/smbd/sec_ctx.c#L261-#L305
> > 
> > Perhaps this is needed for libvirt?
> 
> The capping of ngroups to NGROUPS_MAX looks like a possibe reason.
> 
> Adding this debug might show us if we're exceeding it:
> 
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 89d2cf011f..effc02b898 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1043,6 +1043,7 @@ virSetUIDGID(uid_t uid, gid_t gid, gid_t *groups 
> ATTRIBUTE_UNUSED,
>  }
>  
>  # if HAVE_SETGROUPS
> +VIR_DEBUG("setgroups %d max %d", ngroups, NGROUPS_MAX);
>  if (gid != (gid_t)-1 && setgroups(ngroups, groups) < 0) {
>  virReportSystemError(errno, "%s",
>   _("cannot set supplemental groups"));
> 
> 

Yes, there's an overflow:
2019-08-21 18:25:37.943+: 123145413914624: debug : virSetUIDGID:1046 : 
setgroups 23 max 16

Related samba ticket (it also has references to the python and dovecot
issues):
https://bugzilla.samba.org/show_bug.cgi?id=8773

Thanks,
Roman

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3] tools: console: Relax stream EOF handling

[Daniel Henrique Barboza]

On 8/21/19 10:33 AM, Roman Bolshakov wrote:

Regular VM shutdown triggers the error for existing session of virsh
console and it returns with non-zero exit code:
   error: internal error: console stream EOF

The message and status code are misleading because there's no real
error. virStreamRecv returns 0 correctly when EOF is reached.

Existing implementations of esx, fd, and remote streams behave the same
for virStreamFinish and virStreamAbort: they close the stream. So, we
can continue to use virStreamAbort to handle EOF and errors from
virStreamRecv but additonally we can report error if virStreamAbort
fails.

Fixes: 29f2b5248c6 ("tools: console: pass stream/fd errors to user")
Signed-off-by: Roman Bolshakov 
---


Reviewed-by: Daniel Henrique Barboza 
Tested-by: Daniel Henrique Barboza 



  tools/virsh-console.c | 8 +++-
  1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index e16f841e57..a235a9a283 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -106,7 +106,9 @@ virConsoleShutdown(virConsolePtr con)
  
  if (con->st) {

  virStreamEventRemoveCallback(con->st);
-virStreamAbort(con->st);
+if (virStreamAbort(con->st) < 0)
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("cannot terminate console stream"));
  virStreamFree(con->st);
  con->st = NULL;
  }
@@ -172,10 +174,6 @@ virConsoleEventOnStream(virStreamPtr st,
  if (got == -2)
  goto cleanup; /* blocking */
  if (got <= 0) {
-if (got == 0)
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("console stream EOF"));
-
  virConsoleShutdown(con);
  goto cleanup;
  }


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/11] Fix 10 tests on macOS

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 06:18:11PM +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 21, 2019 at 07:13:12PM +0300, Roman Bolshakov wrote:
> > Hi!
> > 
> > This patch series attempts to reduce the number of failing tests on macOS.
> > 
> > The fixes involve some funk with macOS dynamic and static linkers, dyld and
> > ld64, respectively.
> > 
> > As result, instead of 15 failing tests we get only 5.
> > The tests have been fixed:
> >   qemublocktest
> >   qemumonitorjsontest
> >   viriscsitest
> >   virmacmaptest
> >   virnetserverclienttest
> >   vircryptotest
> >   qemufirmwaretest
> >   domaincapstest
> >   commandtest
> >   sockettest
> > 
> > The tests are still failing:
> >   qemumemlocktest
> >   storagepoolxml2argvtest
> >   qemuxml2xmltest
> >   qemusecuritytest
> >   qemuxml2argvtest
> > 
> > qemucapsprobe doesn't yet works but I started working on the fix.
> > 
> > The failing tests depend on virpcimock that is guarded by ifdefs so no
> > functions are injected and the mock is no-op on macOS. How can we fix
> > the tests that rely on the mock? Should we select only specific tests to
> > run on macOS or we should make virpci mock cross-platform?  Skipping
> > them entirely is not an option IMO as I think qemu driver can be used on
> > macOS with qemu/hvf/haxm domains and the tests are helpful for the
> > domains.
> 
> Realistically the PCI code will only ever execute on Linux, since
> it uses VFIO linux kernel features, so if we don't test PCI bits
> on macOS that's fine.
> 
> That said though, I'd be fine if you wanted to make the virpcimock
> cross-platform too.
> 
> I'd pick whichever strategy results in the nicest code to maintain

BTW, I've acked all patches, but will give a little more time
in case anyone else has comments, before pushing. 


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/11] Fix 10 tests on macOS

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 07:13:12PM +0300, Roman Bolshakov wrote:
> Hi!
> 
> This patch series attempts to reduce the number of failing tests on macOS.
> 
> The fixes involve some funk with macOS dynamic and static linkers, dyld and
> ld64, respectively.
> 
> As result, instead of 15 failing tests we get only 5.
> The tests have been fixed:
>   qemublocktest
>   qemumonitorjsontest
>   viriscsitest
>   virmacmaptest
>   virnetserverclienttest
>   vircryptotest
>   qemufirmwaretest
>   domaincapstest
>   commandtest
>   sockettest
> 
> The tests are still failing:
>   qemumemlocktest
>   storagepoolxml2argvtest
>   qemuxml2xmltest
>   qemusecuritytest
>   qemuxml2argvtest
> 
> qemucapsprobe doesn't yet works but I started working on the fix.
> 
> The failing tests depend on virpcimock that is guarded by ifdefs so no
> functions are injected and the mock is no-op on macOS. How can we fix
> the tests that rely on the mock? Should we select only specific tests to
> run on macOS or we should make virpci mock cross-platform?  Skipping
> them entirely is not an option IMO as I think qemu driver can be used on
> macOS with qemu/hvf/haxm domains and the tests are helpful for the
> domains.

Realistically the PCI code will only ever execute on Linux, since
it uses VFIO linux kernel features, so if we don't test PCI bits
on macOS that's fine.

That said though, I'd be fine if you wanted to make the virpcimock
cross-platform too.

I'd pick whichever strategy results in the nicest code to maintain


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/11] tests: Make references to global symbols indirect in test drivers

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 07:13:23PM +0300, Roman Bolshakov wrote:
> A library has to be built with -flat_namespace to get all references to
> global symbols indirected. That can also be achieved with two-level
> namespace interposition but we're not using explicit symbol
> interposition since it's more verbose and requires massive changes to
> the mocks.
> 
> This provides a way to interpose a mock for virQEMUCapsProbeHostCPU from
> qemucpumock and fixes domaincapstest on macOS.
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  tests/Makefile.am | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Daniel P. Berrangé 


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 10/11] tests: Avoid gnulib replacements in mocks

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 07:13:22PM +0300, Roman Bolshakov wrote:
> gnulib headers change stat, lstat and open to replacement functions,
> even for function definitions. This effectively disables standard
> library overrides in virfilewrapper and virmockstathelpers since they
> are never reached.
> 
> Rename the functions and provide a declartion that uses correct
> assembler name for the mocks.
> 
> This fixes firmware lookup in domaincapstest on macOS.
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  tests/virfilewrapper.c |  5 +
>  tests/virmockstathelpers.c | 10 ++
>  2 files changed, 15 insertions(+)

Reviewed-by: Daniel P. Berrangé 


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 09/11] tests: Use flat namespace on macOS

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 07:13:21PM +0300, Roman Bolshakov wrote:
> Test executables and mocks have assumption that any symbol can be
> replaced with LD_PRELOAD. That's not a case for macOS unless flat
> namespace is used, because every external symbol reference records the
> library to be looked up. And the symbols cannot be replaced unless dyld
> interposing is used.
> 
> Setting DYLD_FORCE_FLAT_NAMESPACE changes symbol lookup behaviour to be
> similar to Linux dynamic linker. It's more lightweight solution than
> explicitly decorating all mock symbols as interpositions and building
> libvirt as interposable dynamic library.
> 
> This fixes vircryptotest and allows to proceed other tests that rely on
> mocks a little bit further.
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  tests/testutils.h | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tests/testutils.h b/tests/testutils.h
> index 7660101991..b46bc86d84 100644
> --- a/tests/testutils.h
> +++ b/tests/testutils.h
> @@ -119,9 +119,12 @@ int virTestMain(int argc,
>  
>  #ifdef __APPLE__
>  # define PRELOAD_VAR "DYLD_INSERT_LIBRARIES"
> +# define FORCE_FLAT_NAMESPACE \
> +setenv("DYLD_FORCE_FLAT_NAMESPACE", "1", 1);

Remove the ';'

>  # define MOCK_EXT ".dylib"
>  #else
>  # define PRELOAD_VAR "LD_PRELOAD"
> +# define FORCE_FLAT_NAMESPACE

Make that  'do {} while (0)'

>  # define MOCK_EXT ".so"
>  #endif
>  
> @@ -141,6 +144,7 @@ int virTestMain(int argc,
>  return EXIT_FAILURE; \
>  } \
>  setenv(PRELOAD_VAR, newenv, 1); \
> +FORCE_FLAT_NAMESPACE \

so that here you can have a trailing ';'

>  execv(argv[0], argv); \
>  } \
>  } while (0)

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 08/11] tests: Lookup extended stat/lstat in mocks

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 07:13:20PM +0300, Roman Bolshakov wrote:
> macOS syscall interface (/usr/lib/system/libsystem_kernel.dylib) has
> three kinds of stat but only one of them can be used to fill
> "struct stat": stat$INODE64.
> 
> virmockstathelpers looks up regular stat instead of stat$INODE64.  That
> causes a failure in qemufirmwaretest because "struct stat" is laid out
> differently from the values returned by stat.
> 
> Introduce VIR_MOCK_REAL_INIT_ALIASED that can be used to lookup
> stat$INODE64 and lstat$INODE64 and use it to setup real functions on
> macOS.
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  tests/virmock.h| 10 ++
>  tests/virmockstathelpers.c |  8 
>  2 files changed, 18 insertions(+)

Reviewed-by: Daniel P. Berrangé 


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 07/11] build: Use flat namespace for libvirt on macOS

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 07:13:19PM +0300, Roman Bolshakov wrote:
> >From ld(1):
> 
>   By default all references resolved to a dynamic library record the
>   library to which they were resolved. At runtime, dyld uses that
>   information to directly resolve symbols. The alternative is to use the
>   -flat_namespace option.  With flat namespace, the library is not
>   recorded.  At runtime, dyld will search each dynamic library in load
>   order when resolving symbols. This is slower, but more like how other
>   operating systems resolve symbols.
> 
> That fixes the set of tests that preload a mock library to replace
> library symbols:
>   qemublocktest
>   qemumonitorjsontest
>   viriscsitest
>   virmacmaptest
>   virnetserverclienttest
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  configure.ac| 1 +
>  src/Makefile.am | 9 +++--
>  2 files changed, 8 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 06/11] tests: Drop /private CWD prefix in commandhelper

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 07:13:18PM +0300, Roman Bolshakov wrote:
> /tmp is a symbolic link to /private/tmp on macOS. That causes failures
> in commandtest, because getcwd returns /private/tmp and the expected
> output doesn't match to "CWD: /tmp".
> 
> Rathern than making a copy of commanddata solely for macOS, the /private
> prefix is stripped.
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  tests/commandhelper.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tests/commandhelper.c b/tests/commandhelper.c
> index 1312f3ee52..358c37e014 100644
> --- a/tests/commandhelper.c
> +++ b/tests/commandhelper.c
> @@ -130,7 +130,16 @@ int main(int argc, char **argv) {
>  if (strlen(cwd) > strlen(".../commanddata") &&
>  STREQ(cwd + strlen(cwd) - strlen("/commanddata"), "/commanddata"))
>  strcpy(cwd, ".../commanddata");
> +#ifdef __APPLE__
> +char *noprivateprefix = NULL;
> +if (strstr(cwd, "/private"))

s/strstr/STRPREFIX/

> +noprivateprefix = cwd + strlen("/private");
> +else
> +noprivateprefix = cwd;
> +fprintf(log, "CWD:%s\n", noprivateprefix);
> +#else
>  fprintf(log, "CWD:%s\n", cwd);
> +#endif

Reviewed-by: Daniel P. Berrangé 

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/11] tests: Remove -module flag for mocks

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 07:13:17PM +0300, Roman Bolshakov wrote:
> macOS has two kinds of loadable libraries: MH_BUNDLE, and MH_DYLIB.
> bundle is used for plugins that are loaded with dlopen/dlsym/dlclose.
> And there's no way to preload a bundle into an application. dynamic
> linker (dyld) will reject it when finds it in DYLD_INSERT_LIBRARIES.
> 
> Unfortunately, a bundle is built if -module flag is provided to libtool.
> The flag has been removed to build dylibs with ".dylib" suffix.
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  tests/Makefile.am | 2 +-
>  tests/testutils.h | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 04/11] tests: Add lib- prefix to all mocks

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 07:13:16PM +0300, Roman Bolshakov wrote:
> In preparation libtool "-module" flag removal, add lib prefix to all
> mock shared objects.
> 
> While at it, introduce VIR_TEST_MOCK macros that makes path out of mock
> name to be used with VIR_TEST_PRELOAD or VIR_TEST_MAIN_PRELOAD.  That,
> hopefully, improves readability, reduces line length and allows to
> tailor VIR_TEST_MOCK for specific platform if it has shared library
> suffix different from ".so".
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  tests/Makefile.am  | 194 -
>  tests/bhyveargv2xmltest.c  |   2 +-
>  tests/bhyvexml2argvtest.c  |   2 +-
>  tests/bhyvexml2xmltest.c   |   2 +-
>  tests/domaincapstest.c |   6 +-
>  tests/fchosttest.c |   2 +-
>  tests/libxlxml2domconfigtest.c |   2 +-
>  tests/nsstest.c|   2 +-
>  tests/qemucaps2xmltest.c   |   2 +-
>  tests/qemucapsprobe.c  |   2 +-
>  tests/qemumemlocktest.c|   3 +-
>  tests/qemumonitorjsontest.c|   2 +-
>  tests/qemuxml2argvtest.c   |   8 +-
>  tests/qemuxml2xmltest.c|   6 +-
>  tests/testutils.c  |   4 +-
>  tests/testutils.h  |   4 +
>  tests/vircaps2xmltest.c|   2 +-
>  tests/vircgrouptest.c  |   2 +-
>  tests/vircryptotest.c  |   2 +-
>  tests/virfilecachetest.c   |   2 +-
>  tests/virfiletest.c|   2 +-
>  tests/virfirewalltest.c|   2 +-
>  tests/virhostcputest.c |   2 +-
>  tests/virhostdevtest.c |   2 +-
>  tests/viriscsitest.c   |   3 +-
>  tests/virmacmaptest.c  |   2 +-
>  tests/virnetdaemontest.c   |   2 +-
>  tests/virnetdevbandwidthtest.c |   2 +-
>  tests/virnetdevtest.c  |   2 +-
>  tests/virnetserverclienttest.c |   2 +-
>  tests/virnettlscontexttest.c   |   2 +-
>  tests/virnettlssessiontest.c   |   2 +-
>  tests/virpcitest.c |   2 +-
>  tests/virpolkittest.c  |   2 +-
>  tests/virportallocatortest.c   |   2 +-
>  tests/virsystemdtest.c |   2 +-
>  tests/virusbtest.c |   2 +-
>  37 files changed, 143 insertions(+), 143 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/11] tests: Preload mocks with DYLD_INSERT_LIBRARIES on macOS

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 07:13:15PM +0300, Roman Bolshakov wrote:
> LD_PRELOAD has no effect on macOS. Instead, dyld(1) provides a way for
> symbol hooking via DYLD_INSERT_LIBRARIES. The variable should contain
> colon-separated paths to the dylibs to be inserted.
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  tests/testutils.h | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/11] tests: Avoid IPv4-translated IPv6 address in sockettest

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 07:13:14PM +0300, Roman Bolshakov wrote:
> getnameinfo on macOS formats certain IPv6 addresses as IPv4-translated
> addresses. The following pattern has been observed:
>   :: is formated as ::0.0.255.255
>   ::fffe is formated as ::0.0.255.254
>   :::0 is formated as ::255.255.0.0
>   ::fffe:0 is formated as ::255.254.0.0
>   :::0:0 is formated as :::0.0.0.0
>   ::fffe:0:0 is formated as ::fffe:0:0
>   :::0:0:0 is formated as :::0:0:0
> 
> The getnameinfo behavior causes a failure for:
>   DO_TEST_PARSE_AND_FORMAT("::", AF_UNSPEC, true);
> 
> Use non-ambigious IPv6 for parse/format testing.
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  tests/sockettest.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/11] tests: Don't test octal localhost IP in sockettest on macOS

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 07:13:13PM +0300, Roman Bolshakov wrote:
> getaddrinfo on macOS doesn't interpret octal IPv4 addresses. Only
> inet_aton can be used for that. Therefore, from macOS standpoint
> "0177.0.0.01" is not the same as "127.0.0.1".
> 
> The issue was also discovered by python and dotnet core:
>   https://bugs.python.org/issue27612
>   https://github.com/dotnet/corefx/issues/8362
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  tests/sockettest.c | 4 
>  1 file changed, 4 insertions(+)

Reviewed-by: Daniel P. Berrangé 


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Fwd: libvirtd failing on MacOS in setgroups

[Daniel P . Berrangé]

On Wed, Aug 21, 2019 at 12:47:03PM -0400, Marcus Furlong wrote:
> On Wed, 21 Aug 2019 at 08:23, Daniel P. Berrangé  wrote:
> >
> > On Tue, Aug 20, 2019 at 11:11:07AM -0400, Marcus Furlong wrote:
> > > Resend to libvir-list in case that is more appropriate:
> > >
> > >
> > > Hi,
> > >
> > > I get the following error when running libvirtd on MacOS as root:
> > >
> > > 2019-07-11 00:12:33.673+: 123145573953536: error :
> > > qemuProcessQMPLaunch:8501 : internal error: Failed to start QEMU
> > > binary /usr/local/bin/qemu-system-x86_64 for probing: libvirt:  error
> > > : cannot set supplemental groups: Invalid argument
> >
> > Are you able to run 'strace' (or whatever MacOS eqiuv is) to see
> > the values passed to setgroups when it fails ?
> 
> I ran `dtruss -f -l -s /usr/local/sbin/libvirtd` but the setgroups
> calls seem to be missing.
> 
> Looking at other sources, it seems like some have special treatment of
> setgroups on MacOS, e.g. samba:
> 
>
> https://github.com/samba-team/samba/blob/v4-11-stable/source3/smbd/sec_ctx.c#L261-#L305
> 
> Perhaps this is needed for libvirt?

The capping of ngroups to NGROUPS_MAX looks like a possibe reason.

Adding this debug might show us if we're exceeding it:

diff --git a/src/util/virutil.c b/src/util/virutil.c
index 89d2cf011f..effc02b898 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1043,6 +1043,7 @@ virSetUIDGID(uid_t uid, gid_t gid, gid_t *groups 
ATTRIBUTE_UNUSED,
 }
 
 # if HAVE_SETGROUPS
+VIR_DEBUG("setgroups %d max %d", ngroups, NGROUPS_MAX);
 if (gid != (gid_t)-1 && setgroups(ngroups, groups) < 0) {
 virReportSystemError(errno, "%s",
  _("cannot set supplemental groups"));


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Fwd: libvirtd failing on MacOS in setgroups

[Marcus Furlong]

On Wed, 21 Aug 2019 at 08:23, Daniel P. Berrangé  wrote:
>
> On Tue, Aug 20, 2019 at 11:11:07AM -0400, Marcus Furlong wrote:
> > Resend to libvir-list in case that is more appropriate:
> >
> >
> > Hi,
> >
> > I get the following error when running libvirtd on MacOS as root:
> >
> > 2019-07-11 00:12:33.673+: 123145573953536: error :
> > qemuProcessQMPLaunch:8501 : internal error: Failed to start QEMU
> > binary /usr/local/bin/qemu-system-x86_64 for probing: libvirt:  error
> > : cannot set supplemental groups: Invalid argument
>
> Are you able to run 'strace' (or whatever MacOS eqiuv is) to see
> the values passed to setgroups when it fails ?

I ran `dtruss -f -l -s /usr/local/sbin/libvirtd` but the setgroups
calls seem to be missing.

Looking at other sources, it seems like some have special treatment of
setgroups on MacOS, e.g. samba:

   
https://github.com/samba-team/samba/blob/v4-11-stable/source3/smbd/sec_ctx.c#L261-#L305

Perhaps this is needed for libvirt?

Marcus.
-- 
Marcus Furlong

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/10] ci: Several fixes and improvements

[Ján Tomko]

On Fri, Aug 16, 2019 at 11:49:44AM +0200, Andrea Bolognani wrote:

See the individual commits for details, but the gist of it is that
after this series it's possible for users to hook into the build
process and customize it according to their needs; on top of that,
the whole thing is made more maintainable in the process.

Andrea Bolognani (10):
 ci: Fix /etc/sub{u,g}id parsing
 ci: Drop $(CI_SUBMODULES)
 ci: Move everything to a separate directory
 ci: Create user's home directory in the container
 ci: Move source directory under $(CI_USER_HOME)
 ci: Introduce $(CI_BUILD_SCRIPT)
 ci: Generalize running commands inside the container
 ci: Introduce $(CI_PREPARE_SCRIPT)
 ci: Run $(CI_PREPARE_SCRIPT) as root
 ci: Stop using --workdir

.gitignore |   2 +-
.travis.yml|   8 +--
Makefile.am|   6 +-
Makefile.ci => ci/Makefile | 109 +++--
ci/build.sh|  40 ++
ci/prepare.sh  |  13 +
6 files changed, 118 insertions(+), 60 deletions(-)
rename Makefile.ci => ci/Makefile (79%)
create mode 100644 ci/build.sh
create mode 100644 ci/prepare.sh



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 06/11] tests: Drop /private CWD prefix in commandhelper

[Roman Bolshakov]

/tmp is a symbolic link to /private/tmp on macOS. That causes failures
in commandtest, because getcwd returns /private/tmp and the expected
output doesn't match to "CWD: /tmp".

Rathern than making a copy of commanddata solely for macOS, the /private
prefix is stripped.

Signed-off-by: Roman Bolshakov 
---
 tests/commandhelper.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/tests/commandhelper.c b/tests/commandhelper.c
index 1312f3ee52..358c37e014 100644
--- a/tests/commandhelper.c
+++ b/tests/commandhelper.c
@@ -130,7 +130,16 @@ int main(int argc, char **argv) {
 if (strlen(cwd) > strlen(".../commanddata") &&
 STREQ(cwd + strlen(cwd) - strlen("/commanddata"), "/commanddata"))
 strcpy(cwd, ".../commanddata");
+#ifdef __APPLE__
+char *noprivateprefix = NULL;
+if (strstr(cwd, "/private"))
+noprivateprefix = cwd + strlen("/private");
+else
+noprivateprefix = cwd;
+fprintf(log, "CWD:%s\n", noprivateprefix);
+#else
 fprintf(log, "CWD:%s\n", cwd);
+#endif
 free(cwd);
 
 fprintf(log, "UMASK:%04o\n", umask(0));
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 11/11] tests: Make references to global symbols indirect in test drivers

[Roman Bolshakov]

A library has to be built with -flat_namespace to get all references to
global symbols indirected. That can also be achieved with two-level
namespace interposition but we're not using explicit symbol
interposition since it's more verbose and requires massive changes to
the mocks.

This provides a way to interpose a mock for virQEMUCapsProbeHostCPU from
qemucpumock and fixes domaincapstest on macOS.

Signed-off-by: Roman Bolshakov 
---
 tests/Makefile.am | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 460efb6b7b..f92710db43 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -56,6 +56,9 @@ DRIVERLIB_LDFLAGS = \
-avoid-version \
-rpath /evil/libtool/hack/to/force/shared/lib/creation \
$(MINGW_EXTRA_LDFLAGS)
+if WITH_MACOS
+DRIVERLIB_LDFLAGS += -Wl,-flat_namespace
+endif WITH_MACOS
 
 PROBES_O =
 if WITH_DTRACE_PROBES
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 07/11] build: Use flat namespace for libvirt on macOS

[Roman Bolshakov]

>From ld(1):

  By default all references resolved to a dynamic library record the
  library to which they were resolved. At runtime, dyld uses that
  information to directly resolve symbols. The alternative is to use the
  -flat_namespace option.  With flat namespace, the library is not
  recorded.  At runtime, dyld will search each dynamic library in load
  order when resolving symbols. This is slower, but more like how other
  operating systems resolve symbols.

That fixes the set of tests that preload a mock library to replace
library symbols:
  qemublocktest
  qemumonitorjsontest
  viriscsitest
  virmacmaptest
  virnetserverclienttest

Signed-off-by: Roman Bolshakov 
---
 configure.ac| 1 +
 src/Makefile.am | 9 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index f41c6d5d86..852b464e97 100644
--- a/configure.ac
+++ b/configure.ac
@@ -212,6 +212,7 @@ fi
 
 AM_CONDITIONAL([WITH_LINUX], [test "$with_linux" = "yes"])
 AM_CONDITIONAL([WITH_FREEBSD], [test "$with_freebsd" = "yes"])
+AM_CONDITIONAL([WITH_MACOS], [test "$with_macos" = "yes"])
 
 # We don't support the daemon yet
 if test "$with_win" = "yes" ; then
diff --git a/src/Makefile.am b/src/Makefile.am
index 817a7ecf34..adaf61350a 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -579,8 +579,13 @@ libvirt_la_LDFLAGS = \
-version-info $(LIBVIRT_VERSION_INFO) \
$(LIBVIRT_NODELETE) \
$(NO_UNDEFINED_LDFLAGS) \
-   $(AM_LDFLAGS) \
-   $(NULL)
+   $(AM_LDFLAGS)
+if WITH_MACOS
+# macOS has two-level namespaces by default.
+# Override it to allow symbol replacement with DYLD_INSERT_LIBRARIES
+libvirt_la_LDFLAGS += -Wl,-flat_namespace
+endif WITH_MACOS
+libvirt_la_LDFLAGS += $(NULL)
 libvirt_la_BUILT_LIBADD += ../gnulib/lib/libgnu.la
 libvirt_la_LIBADD += \
$(DRIVER_MODULES_LIBS) \
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 08/11] tests: Lookup extended stat/lstat in mocks

[Roman Bolshakov]

macOS syscall interface (/usr/lib/system/libsystem_kernel.dylib) has
three kinds of stat but only one of them can be used to fill
"struct stat": stat$INODE64.

virmockstathelpers looks up regular stat instead of stat$INODE64.  That
causes a failure in qemufirmwaretest because "struct stat" is laid out
differently from the values returned by stat.

Introduce VIR_MOCK_REAL_INIT_ALIASED that can be used to lookup
stat$INODE64 and lstat$INODE64 and use it to setup real functions on
macOS.

Signed-off-by: Roman Bolshakov 
---
 tests/virmock.h| 10 ++
 tests/virmockstathelpers.c |  8 
 2 files changed, 18 insertions(+)

diff --git a/tests/virmock.h b/tests/virmock.h
index 74bb0d5b15..7520bb5d6e 100644
--- a/tests/virmock.h
+++ b/tests/virmock.h
@@ -288,3 +288,13 @@
 abort(); \
 } \
 } while (0)
+
+#define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \
+do { \
+if (real_##name == NULL && \
+!(real_##name = dlsym(RTLD_NEXT, \
+  alias))) { \
+fprintf(stderr, "Missing symbol '" alias "'\n"); \
+abort(); \
+} \
+} while (0)
diff --git a/tests/virmockstathelpers.c b/tests/virmockstathelpers.c
index 0bbea4b437..1f6f831bed 100644
--- a/tests/virmockstathelpers.c
+++ b/tests/virmockstathelpers.c
@@ -138,7 +138,11 @@ static void virMockStatInit(void)
 debug = getenv("VIR_MOCK_STAT_DEBUG");
 
 #ifdef MOCK_STAT
+#ifdef __APPLE__
+VIR_MOCK_REAL_INIT_ALIASED(stat, "stat$INODE64");
+#else
 VIR_MOCK_REAL_INIT(stat);
+#endif
 fdebug("real stat %p\n", real_stat);
 #endif
 #ifdef MOCK_STAT64
@@ -154,7 +158,11 @@ static void virMockStatInit(void)
 fdebug("real __xstat64 %p\n", real___xstat64);
 #endif
 #ifdef MOCK_LSTAT
+#ifdef __APPLE__
+VIR_MOCK_REAL_INIT_ALIASED(stat, "lstat$INODE64");
+#else
 VIR_MOCK_REAL_INIT(lstat);
+#endif
 fdebug("real lstat %p\n", real_lstat);
 #endif
 #ifdef MOCK_LSTAT64
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 10/11] tests: Avoid gnulib replacements in mocks

[Roman Bolshakov]

gnulib headers change stat, lstat and open to replacement functions,
even for function definitions. This effectively disables standard
library overrides in virfilewrapper and virmockstathelpers since they
are never reached.

Rename the functions and provide a declartion that uses correct
assembler name for the mocks.

This fixes firmware lookup in domaincapstest on macOS.

Signed-off-by: Roman Bolshakov 
---
 tests/virfilewrapper.c |  5 +
 tests/virmockstathelpers.c | 10 ++
 2 files changed, 15 insertions(+)

diff --git a/tests/virfilewrapper.c b/tests/virfilewrapper.c
index 067cb30657..20cb2f4464 100644
--- a/tests/virfilewrapper.c
+++ b/tests/virfilewrapper.c
@@ -157,7 +157,12 @@ int access(const char *path, int mode)
 return real_access(newpath ? newpath : path, mode);
 }
 
+#ifdef __APPLE__
+int _open(const char *path, int flags, ...) __asm("_open");
+int _open(const char *path, int flags, ...)
+#else
 int open(const char *path, int flags, ...)
+#endif
 {
 VIR_AUTOFREE(char *) newpath = NULL;
 va_list ap;
diff --git a/tests/virmockstathelpers.c b/tests/virmockstathelpers.c
index 1f6f831bed..4ddb0dfec1 100644
--- a/tests/virmockstathelpers.c
+++ b/tests/virmockstathelpers.c
@@ -192,7 +192,12 @@ static int virMockStatRedirect(const char *path, char 
**newpath);
 #endif
 
 #ifdef MOCK_STAT
+# ifdef __APPLE__
+int _stat(const char *path, struct stat *sb) __asm("_stat$INODE64");
+int _stat(const char *path, struct stat *sb)
+# else
 int stat(const char *path, struct stat *sb)
+# endif
 {
 VIR_AUTOFREE(char *) newpath = NULL;
 
@@ -262,8 +267,13 @@ __xstat64(int ver, const char *path, struct stat64 *sb)
 #endif
 
 #ifdef MOCK_LSTAT
+# ifdef __APPLE__
+int _lstat(const char *path, struct stat *sb) __asm("_lstat$INODE64");
+int _lstat(const char *path, struct stat *sb)
+# else
 int
 lstat(const char *path, struct stat *sb)
+# endif
 {
 VIR_AUTOFREE(char *) newpath = NULL;
 
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 00/11] Fix 10 tests on macOS

[Roman Bolshakov]

Hi!

This patch series attempts to reduce the number of failing tests on macOS.

The fixes involve some funk with macOS dynamic and static linkers, dyld and
ld64, respectively.

As result, instead of 15 failing tests we get only 5.
The tests have been fixed:
  qemublocktest
  qemumonitorjsontest
  viriscsitest
  virmacmaptest
  virnetserverclienttest
  vircryptotest
  qemufirmwaretest
  domaincapstest
  commandtest
  sockettest

The tests are still failing:
  qemumemlocktest
  storagepoolxml2argvtest
  qemuxml2xmltest
  qemusecuritytest
  qemuxml2argvtest

qemucapsprobe doesn't yet works but I started working on the fix.

The failing tests depend on virpcimock that is guarded by ifdefs so no
functions are injected and the mock is no-op on macOS. How can we fix
the tests that rely on the mock? Should we select only specific tests to
run on macOS or we should make virpci mock cross-platform?  Skipping
them entirely is not an option IMO as I think qemu driver can be used on
macOS with qemu/hvf/haxm domains and the tests are helpful for the
domains.

And as soon as we get working tests and qemucapsprobe I'd want to resend hvf
patchset.

Best regards,
Roman

Roman Bolshakov (11):
  tests: Don't test octal localhost IP in sockettest on macOS
  tests: Avoid IPv4-translated IPv6 address in sockettest
  tests: Preload mocks with DYLD_INSERT_LIBRARIES on macOS
  tests: Add lib- prefix to all mocks
  tests: Remove -module flag for mocks
  tests: Drop /private CWD prefix in commandhelper
  build: Use flat namespace for libvirt on macOS
  tests: Lookup extended stat/lstat in mocks
  tests: Use flat namespace on macOS
  tests: Avoid gnulib replacements in mocks
  tests: Make references to global symbols indirect in test drivers

 configure.ac   |   1 +
 src/Makefile.am|   9 +-
 tests/Makefile.am  | 199 +
 tests/bhyveargv2xmltest.c  |   2 +-
 tests/bhyvexml2argvtest.c  |   2 +-
 tests/bhyvexml2xmltest.c   |   2 +-
 tests/commandhelper.c  |   9 ++
 tests/domaincapstest.c |   6 +-
 tests/fchosttest.c |   2 +-
 tests/libxlxml2domconfigtest.c |   2 +-
 tests/nsstest.c|   2 +-
 tests/qemucaps2xmltest.c   |   2 +-
 tests/qemucapsprobe.c  |   2 +-
 tests/qemumemlocktest.c|   3 +-
 tests/qemumonitorjsontest.c|   2 +-
 tests/qemuxml2argvtest.c   |   8 +-
 tests/qemuxml2xmltest.c|   6 +-
 tests/sockettest.c |   6 +-
 tests/testutils.c  |   4 +-
 tests/testutils.h  |  18 ++-
 tests/vircaps2xmltest.c|   2 +-
 tests/vircgrouptest.c  |   2 +-
 tests/vircryptotest.c  |   2 +-
 tests/virfilecachetest.c   |   2 +-
 tests/virfiletest.c|   2 +-
 tests/virfilewrapper.c |   5 +
 tests/virfirewalltest.c|   2 +-
 tests/virhostcputest.c |   2 +-
 tests/virhostdevtest.c |   2 +-
 tests/viriscsitest.c   |   3 +-
 tests/virmacmaptest.c  |   2 +-
 tests/virmock.h|  10 ++
 tests/virmockstathelpers.c |  18 +++
 tests/virnetdaemontest.c   |   2 +-
 tests/virnetdevbandwidthtest.c |   2 +-
 tests/virnetdevtest.c  |   2 +-
 tests/virnetserverclienttest.c |   2 +-
 tests/virnettlscontexttest.c   |   2 +-
 tests/virnettlssessiontest.c   |   2 +-
 tests/virpcitest.c |   2 +-
 tests/virpolkittest.c  |   2 +-
 tests/virportallocatortest.c   |   2 +-
 tests/virsystemdtest.c |   2 +-
 tests/virusbtest.c |   2 +-
 44 files changed, 214 insertions(+), 149 deletions(-)

-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 04/11] tests: Add lib- prefix to all mocks

[Roman Bolshakov]

In preparation libtool "-module" flag removal, add lib prefix to all
mock shared objects.

While at it, introduce VIR_TEST_MOCK macros that makes path out of mock
name to be used with VIR_TEST_PRELOAD or VIR_TEST_MAIN_PRELOAD.  That,
hopefully, improves readability, reduces line length and allows to
tailor VIR_TEST_MOCK for specific platform if it has shared library
suffix different from ".so".

Signed-off-by: Roman Bolshakov 
---
 tests/Makefile.am  | 194 -
 tests/bhyveargv2xmltest.c  |   2 +-
 tests/bhyvexml2argvtest.c  |   2 +-
 tests/bhyvexml2xmltest.c   |   2 +-
 tests/domaincapstest.c |   6 +-
 tests/fchosttest.c |   2 +-
 tests/libxlxml2domconfigtest.c |   2 +-
 tests/nsstest.c|   2 +-
 tests/qemucaps2xmltest.c   |   2 +-
 tests/qemucapsprobe.c  |   2 +-
 tests/qemumemlocktest.c|   3 +-
 tests/qemumonitorjsontest.c|   2 +-
 tests/qemuxml2argvtest.c   |   8 +-
 tests/qemuxml2xmltest.c|   6 +-
 tests/testutils.c  |   4 +-
 tests/testutils.h  |   4 +
 tests/vircaps2xmltest.c|   2 +-
 tests/vircgrouptest.c  |   2 +-
 tests/vircryptotest.c  |   2 +-
 tests/virfilecachetest.c   |   2 +-
 tests/virfiletest.c|   2 +-
 tests/virfirewalltest.c|   2 +-
 tests/virhostcputest.c |   2 +-
 tests/virhostdevtest.c |   2 +-
 tests/viriscsitest.c   |   3 +-
 tests/virmacmaptest.c  |   2 +-
 tests/virnetdaemontest.c   |   2 +-
 tests/virnetdevbandwidthtest.c |   2 +-
 tests/virnetdevtest.c  |   2 +-
 tests/virnetserverclienttest.c |   2 +-
 tests/virnettlscontexttest.c   |   2 +-
 tests/virnettlssessiontest.c   |   2 +-
 tests/virpcitest.c |   2 +-
 tests/virpolkittest.c  |   2 +-
 tests/virportallocatortest.c   |   2 +-
 tests/virsystemdtest.c |   2 +-
 tests/virusbtest.c |   2 +-
 37 files changed, 143 insertions(+), 143 deletions(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 6b5d05bbed..713dc30de3 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -206,16 +206,16 @@ test_programs = virshtest sockettest \
$(NULL)
 
 test_libraries = libshunload.la \
-   virportallocatormock.la \
-   virnetdaemonmock.la \
-   virnetserverclientmock.la \
-   vircgroupmock.la \
-   virpcimock.la \
-   virnetdevmock.la \
-   virrandommock.la \
-   virhostcpumock.la \
-   domaincapsmock.la \
-   virfilecachemock.la \
+   libvirportallocatormock.la \
+   libvirnetdaemonmock.la \
+   libvirnetserverclientmock.la \
+   libvircgroupmock.la \
+   libvirpcimock.la \
+   libvirnetdevmock.la \
+   libvirrandommock.la \
+   libvirhostcpumock.la \
+   libdomaincapsmock.la \
+   libvirfilecachemock.la \
$(NULL)
 
 if WITH_REMOTE
@@ -234,11 +234,11 @@ test_programs += fchosttest
 test_programs += scsihosttest
 test_programs += vircaps2xmltest
 test_programs += virresctrltest
-test_libraries += virusbmock.la \
-   virnetdevbandwidthmock.la \
-   virnumamock.la \
-   virtestmock.la \
-   virfilemock.la \
+test_libraries += libvirusbmock.la \
+   libvirnetdevbandwidthmock.la \
+   libvirnumamock.la \
+   libvirtestmock.la \
+   libvirfilemock.la \
$(NULL)
 endif WITH_LINUX
 
@@ -250,7 +250,7 @@ if WITH_DBUS
 test_programs += virdbustest \
  virsystemdtest \
  $(NULL)
-test_libraries += virdbusmock.la
+test_libraries += libvirdbusmock.la
 if WITH_POLKIT
 test_programs += virpolkittest
 endif WITH_POLKIT
@@ -291,10 +291,10 @@ test_programs += qemuxml2argvtest qemuxml2xmltest \
 test_helpers += qemucapsprobe
 test_libraries += libqemumonitortestutils.la \
libqemutestdriver.la \
-   qemuxml2argvmock.la \
-   qemucaps2xmlmock.la \
-   qemucapsprobemock.la \
-   qemucpumock.la \
+   libqemuxml2argvmock.la \
+   libqemucaps2xmlmock.la \
+   libqemucapsprobemock.la \
+   libqemucpumock.la \
$(NULL)
 endif WITH_QEMU
 
@@ -324,7 +324,7 @@ endif WITH_VMWARE
 
 if WITH_BHYVE
 test_programs += bhyvexml2argvtest bhyvexml2xmltest bhyveargv2xmltest
-test_libraries += bhyvexml2argvmock.la bhyveargv2xmlmock.la
+test_libraries += libbhyvexml2argvmock.la libbhyveargv2xmlmock.la
 endif WITH_BHYVE
 
 if WITH_CIL
@@ -379,7 +379,7 @@ endif WITH_LINUX
 if WITH_NSS
 test_helpers += nsslinktest nssguestlinktest
 test_programs += nsstest nssguesttest
-test_libraries += nssmock.la
+test_libraries += libnssmock.la
 endif WITH_NSS
 
 test_programs += storagevolxml2xmltest
@@ -555,10 +555,10 @@ libqemutestdriver_la_SOURCES =
 libqemutestdriver_la_LDFLAGS = $(DRIVERLIB_LDFLAGS)
 libqemutestdriver_la_LIBADD = $(qemu_LDADDS)
 
-qemucpumock_la_SOURCES = \
+libqemucpumock_la_SOURCES = \

[libvirt] [PATCH 05/11] tests: Remove -module flag for mocks

[Roman Bolshakov]

macOS has two kinds of loadable libraries: MH_BUNDLE, and MH_DYLIB.
bundle is used for plugins that are loaded with dlopen/dlsym/dlclose.
And there's no way to preload a bundle into an application. dynamic
linker (dyld) will reject it when finds it in DYLD_INSERT_LIBRARIES.

Unfortunately, a bundle is built if -module flag is provided to libtool.
The flag has been removed to build dylibs with ".dylib" suffix.

Signed-off-by: Roman Bolshakov 
---
 tests/Makefile.am | 2 +-
 tests/testutils.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 713dc30de3..460efb6b7b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -48,7 +48,7 @@ AM_CFLAGS = \
 AM_LDFLAGS = \
-export-dynamic
 
-MOCKLIBS_LDFLAGS = -module -avoid-version \
+MOCKLIBS_LDFLAGS = -avoid-version \
-rpath /evil/libtool/hack/to/force/shared/lib/creation \
$(MINGW_EXTRA_LDFLAGS)
 
diff --git a/tests/testutils.h b/tests/testutils.h
index e0fa2b2448..7660101991 100644
--- a/tests/testutils.h
+++ b/tests/testutils.h
@@ -119,8 +119,10 @@ int virTestMain(int argc,
 
 #ifdef __APPLE__
 # define PRELOAD_VAR "DYLD_INSERT_LIBRARIES"
+# define MOCK_EXT ".dylib"
 #else
 # define PRELOAD_VAR "LD_PRELOAD"
+# define MOCK_EXT ".so"
 #endif
 
 #define VIR_TEST_PRELOAD(lib) \
@@ -148,8 +150,6 @@ int virTestMain(int argc,
 return virTestMain(argc, argv, func, __VA_ARGS__, NULL); \
 }
 
-#define MOCK_EXT ".so"
-
 #define VIR_TEST_MOCK(mock) (abs_builddir "/.libs/lib" mock "mock" MOCK_EXT)
 
 virCapsPtr virTestGenericCapsInit(void);
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 09/11] tests: Use flat namespace on macOS

[Roman Bolshakov]

Test executables and mocks have assumption that any symbol can be
replaced with LD_PRELOAD. That's not a case for macOS unless flat
namespace is used, because every external symbol reference records the
library to be looked up. And the symbols cannot be replaced unless dyld
interposing is used.

Setting DYLD_FORCE_FLAT_NAMESPACE changes symbol lookup behaviour to be
similar to Linux dynamic linker. It's more lightweight solution than
explicitly decorating all mock symbols as interpositions and building
libvirt as interposable dynamic library.

This fixes vircryptotest and allows to proceed other tests that rely on
mocks a little bit further.

Signed-off-by: Roman Bolshakov 
---
 tests/testutils.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/tests/testutils.h b/tests/testutils.h
index 7660101991..b46bc86d84 100644
--- a/tests/testutils.h
+++ b/tests/testutils.h
@@ -119,9 +119,12 @@ int virTestMain(int argc,
 
 #ifdef __APPLE__
 # define PRELOAD_VAR "DYLD_INSERT_LIBRARIES"
+# define FORCE_FLAT_NAMESPACE \
+setenv("DYLD_FORCE_FLAT_NAMESPACE", "1", 1);
 # define MOCK_EXT ".dylib"
 #else
 # define PRELOAD_VAR "LD_PRELOAD"
+# define FORCE_FLAT_NAMESPACE
 # define MOCK_EXT ".so"
 #endif
 
@@ -141,6 +144,7 @@ int virTestMain(int argc,
 return EXIT_FAILURE; \
 } \
 setenv(PRELOAD_VAR, newenv, 1); \
+FORCE_FLAT_NAMESPACE \
 execv(argv[0], argv); \
 } \
 } while (0)
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 03/11] tests: Preload mocks with DYLD_INSERT_LIBRARIES on macOS

[Roman Bolshakov]

LD_PRELOAD has no effect on macOS. Instead, dyld(1) provides a way for
symbol hooking via DYLD_INSERT_LIBRARIES. The variable should contain
colon-separated paths to the dylibs to be inserted.

Signed-off-by: Roman Bolshakov 
---
 tests/testutils.h | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/testutils.h b/tests/testutils.h
index 8c12fd7c12..07dda6e355 100644
--- a/tests/testutils.h
+++ b/tests/testutils.h
@@ -117,9 +117,15 @@ int virTestMain(int argc,
 return virTestMain(argc, argv, func, NULL); \
 }
 
+#ifdef __APPLE__
+# define PRELOAD_VAR "DYLD_INSERT_LIBRARIES"
+#else
+# define PRELOAD_VAR "LD_PRELOAD"
+#endif
+
 #define VIR_TEST_PRELOAD(lib) \
 do { \
-const char *preload = getenv("LD_PRELOAD"); \
+const char *preload = getenv(PRELOAD_VAR); \
 if (preload == NULL || strstr(preload, lib) == NULL) { \
 char *newenv; \
 if (!virFileIsExecutable(lib)) { \
@@ -132,7 +138,7 @@ int virTestMain(int argc,
 perror("virAsprintf"); \
 return EXIT_FAILURE; \
 } \
-setenv("LD_PRELOAD", newenv, 1); \
+setenv(PRELOAD_VAR, newenv, 1); \
 execv(argv[0], argv); \
 } \
 } while (0)
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 01/11] tests: Don't test octal localhost IP in sockettest on macOS

[Roman Bolshakov]

getaddrinfo on macOS doesn't interpret octal IPv4 addresses. Only
inet_aton can be used for that. Therefore, from macOS standpoint
"0177.0.0.01" is not the same as "127.0.0.1".

The issue was also discovered by python and dotnet core:
  https://bugs.python.org/issue27612
  https://github.com/dotnet/corefx/issues/8362

Signed-off-by: Roman Bolshakov 
---
 tests/sockettest.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/tests/sockettest.c b/tests/sockettest.c
index 859f02e51e..2b21352c13 100644
--- a/tests/sockettest.c
+++ b/tests/sockettest.c
@@ -465,7 +465,11 @@ mymain(void)
 
 DO_TEST_LOCALHOST("127.0.0.1", true);
 DO_TEST_LOCALHOST("2130706433", true);
+
+/* Octal IPv4 doesn't work in getaddrinfo on macOS */
+#ifndef __APPLE__
 DO_TEST_LOCALHOST("0177.0.0.01", true);
+#endif
 DO_TEST_LOCALHOST("::1", true);
 DO_TEST_LOCALHOST("0::1", true);
 DO_TEST_LOCALHOST("0:0:0::1", true);
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 02/11] tests: Avoid IPv4-translated IPv6 address in sockettest

[Roman Bolshakov]

getnameinfo on macOS formats certain IPv6 addresses as IPv4-translated
addresses. The following pattern has been observed:
  :: is formated as ::0.0.255.255
  ::fffe is formated as ::0.0.255.254
  :::0 is formated as ::255.255.0.0
  ::fffe:0 is formated as ::255.254.0.0
  :::0:0 is formated as :::0.0.0.0
  ::fffe:0:0 is formated as ::fffe:0:0
  :::0:0:0 is formated as :::0:0:0

The getnameinfo behavior causes a failure for:
  DO_TEST_PARSE_AND_FORMAT("::", AF_UNSPEC, true);

Use non-ambigious IPv6 for parse/format testing.

Signed-off-by: Roman Bolshakov 
---
 tests/sockettest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/sockettest.c b/tests/sockettest.c
index 2b21352c13..29a565de40 100644
--- a/tests/sockettest.c
+++ b/tests/sockettest.c
@@ -378,7 +378,7 @@ mymain(void)
 DO_TEST_PARSE_AND_FORMAT("::1", AF_INET, false);
 DO_TEST_PARSE_AND_FORMAT("::1", AF_INET6, true);
 DO_TEST_PARSE_AND_FORMAT("::1", AF_UNIX, false);
-DO_TEST_PARSE_AND_FORMAT("::", AF_UNSPEC, true);
+DO_TEST_PARSE_AND_FORMAT("::fffe:0:0", AF_UNSPEC, true);
 
 /* tests that specify a network that should contain the range */
 DO_TEST_RANGE("192.168.122.1", "192.168.122.1", "192.168.122.1", 24, 1, 
true);
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] qemu: add hypervisor feature cpu-pm support for kvm

[Ján Tomko]

On Mon, Aug 19, 2019 at 10:32:25AM +0200, Menno Lageman wrote:

From: Wim ten Have 

QEMU introduced a CPU power management feature with commit 6f131f13e68d
("kvm: support -overcommit cpu-pm=on|off").

This patch series adds support for controlling QEMU's "-overcommit"
command line switch: "-overcommit cpu-pm=[on|off]". By default "cpu-pm" is off.

To turn the CPU power management feature on, the following
XML needs to be added to the guest's domain description:

 
   
 
   
 

Note that the "cpu-pm" element is only available if the CPU
mode is set to "host-passthrough":

 on, off
  5.7.0 (QEMU 2.12.1)



Hmm, seems we did not bother to document hint-dedicated


+
+  cpu-pm
+  Enable guest management of the host CPU's power state.  Note that 
enabling this will increase latency for other processes running on the same host 
CPU.
+  on, off
+  5.7.0 (QEMU 2.12.1)
+
  
  
  pmu
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 08853f9d9e92..51c9e434aa35 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5970,6 +5970,11 @@

  

+
+  
+
+  
+
  

  
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8642927d6bf3..e348aa0bcb5f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -203,6 +203,7 @@ VIR_ENUM_IMPL(virDomainKVM,
  VIR_DOMAIN_KVM_LAST,
  "hidden",
  "hint-dedicated",
+  "cpu-pm",
);

VIR_ENUM_IMPL(virDomainMsrsUnknown,
@@ -20412,6 +20413,7 @@ virDomainDefParseXML(xmlDocPtr xml,
switch ((virDomainKVM) feature) {
case VIR_DOMAIN_KVM_HIDDEN:
case VIR_DOMAIN_KVM_DEDICATED:
+case VIR_DOMAIN_KVM_CPU_PM:
if (!(tmp = virXMLPropString(nodes[i], "state"))) {
virReportError(VIR_ERR_XML_ERROR,
   _("missing 'state' attribute for "
@@ -22625,6 +22627,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr 
src,
switch ((virDomainKVM) i) {
case VIR_DOMAIN_KVM_HIDDEN:
case VIR_DOMAIN_KVM_DEDICATED:
+case VIR_DOMAIN_KVM_CPU_PM:
if (src->kvm_features[i] != dst->kvm_features[i]) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
   _("State of KVM feature '%s' differs: "
@@ -28126,6 +28129,7 @@ virDomainDefFormatFeatures(virBufferPtr buf,
switch ((virDomainKVM) j) {
case VIR_DOMAIN_KVM_HIDDEN:
case VIR_DOMAIN_KVM_DEDICATED:
+case VIR_DOMAIN_KVM_CPU_PM:
if (def->kvm_features[j])
virBufferAsprintf(, "<%s state='%s'/>\n",
  virDomainKVMTypeToString(j),
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f7423b1b6f89..b5a4cee46c1e 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1766,6 +1766,7 @@ typedef enum {
typedef enum {
VIR_DOMAIN_KVM_HIDDEN = 0,
VIR_DOMAIN_KVM_DEDICATED,
+VIR_DOMAIN_KVM_CPU_PM,

VIR_DOMAIN_KVM_LAST
} virDomainKVM;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9db4ac7933e5..67a3a14c9b36 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -446,6 +446,7 @@ virDomainIOThreadIDDel;
virDomainIOThreadIDFind;
virDomainKeyWrapCipherNameTypeFromString;
virDomainKeyWrapCipherNameTypeToString;
+virDomainKVMTypeToString;
virDomainLeaseDefFree;
virDomainLeaseIndex;
virDomainLeaseInsert;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 922456ec679e..163c98bc7d0f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7217,6 +7217,8 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
virBufferAddLit(, ",kvm-hint-dedicated=on");
break;

+/* Not a -cpu option */
+case VIR_DOMAIN_KVM_CPU_PM:
/* coverity[dead_error_begin] */
case VIR_DOMAIN_KVM_LAST:
break;
@@ -7852,8 +7854,20 @@ qemuBuildOvercommitCommandLine(virCommandPtr cmd,
   virQEMUCapsPtr qemuCaps)
{
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OVERCOMMIT)) {
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+char *overcommit;
+
+virBufferAsprintf(, "mem-lock=%s", def->mem.locked ? "on" : "off");
+
+if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON)
+virBufferAsprintf(, ",cpu-pm=%s",
+  def->kvm_features[VIR_DOMAIN_KVM_CPU_PM] == VIR_TRISTATE_SWITCH_ON ? 
"on" : "off");


const char *cpu_pm = NULL;

if (def->kvm_features[VIR_DOMAIN_KVM_CPU_PM] != VIR_TRISTATE_SWITCH_DEFAULT)
   cpu_pm = 

Re: [libvirt] [PATCH 1/3] qemu: introduce qemuBuildOvercommitCommandLine()

[Ján Tomko]

On Mon, Aug 19, 2019 at 10:32:24AM +0200, Menno Lageman wrote:

In preparation for adding support for '-overcommit cpu-pm=[on|off]',
add qemuBuildOvercommitCommandLine() to generate the '-overcommit'
commandline. Move the existing '-overcommit mem-lock=[on|off]'
generation code from qemuBuildMemCommandline() to this function.



I guess we could proclaim that -realtime mlock= also deals with
overcommit and separate it into the same function, especially since
the rest of the MemCommandLine does not depend on it
(and we would touch def->mem.locked in the new Overcommit function
anyway).

Jano


Signed-off-by: Menno Lageman 
---
src/qemu/qemu_command.c | 25 +
1 file changed, 21 insertions(+), 4 deletions(-)



signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 1/9] vbox: Add various vir*Flags API

[Eric Blake]

On 7/10/19 6:50 AM, Eric Blake wrote:
> On 7/10/19 2:02 AM, Peter Krempa wrote:
>> On Tue, Jul 09, 2019 at 12:46:30 -0500, Eric Blake wrote:
>>> Even though we don't accept any flags, it is unfriendly to callers
>>> that use the modern API to have to fall back to the flag-free API.
>>>
>>> Signed-off-by: Eric Blake 
>>> ---

>>> -vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
>>> +vboxDomainSaveFlags(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED,
>>> +const char *dxml, unsigned int flags)
>>>  {
>>>  vboxDriverPtr data = dom->conn->privateData;
>>>  IConsole *console = NULL;
>>> @@ -564,6 +565,9 @@ vboxDomainSave(virDomainPtr dom, const char *path 
>>> ATTRIBUTE_UNUSED)
>>>  nsresult rc;
>>>  int ret = -1;
>>>
>>> +virCheckFlags(0, -1);
>>> +virCheckNonNullArgReturn(dxml, -1);
>>
>> This reports: invalid argument: dxml in vboxDomainSave must not be NULL
>>

Revisiting this thread:

internal.h has:

virCheckNullArgGoto  (complains if argument is not NULL)
virCheckNonNullArgReturn (complains if argument is NULL)
virCheckNonNullArgGoto   (complains if argument is NULL)

but is missing virCheckNullArgReturn, which is the form I really meant
to use here. I have no idea if that missing macro is intentional, but it
would be easy enough to add.

>> I'm not certain that the internal function name makes sense to external
>> users.
> 
> In which case I can hand-roll a more specific error instead of reusing
> the common macro. But I do see pre-existing uses of
> virCheckNonNullArgXXX in src/esx and src/vz prior to this patch, so it's
> not the first time we've used the common macros.

Directly calling virReportInvalidNonNullArg() would be the only way to
hand-roll this properly, but no one outside of internal.h and
src/util/virobject.c uses it.  I'd prefer to sick with
virCheckNullArgReturn().


>>> +static int
>>> +vboxDomainSave(virDomainPtr dom, const char *path)
>>> +{
>>> +return vboxDomainSaveFlags(dom, path, NULL, 0);
>>
>> So, this passes NULL 'dxml' into vboxDomainSaveFlags which explicitly
>> rejects it. What's the point?
>>
>> Ah I see. Was the above supposed to be virCheckNullArgGoto?
> 
> D'oh. Yes, I got it backwards.  The function wants to succeed only if
> the user omitted the optional dxml argument.
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/10] qemu: Split out preparing of single snapshot from qemuDomainSnapshotDiskDataCollect

[Ján Tomko]

On Fri, Aug 16, 2019 at 03:54:39PM +0200, Peter Krempa wrote:

Move the internals into qemuDomainSnapshotDiskDataCollectOne to make it
obvious what's happening after moving more code here.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 104 +++--
1 file changed, 59 insertions(+), 45 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 04/10] qemu: snapshot: Restrict file existance check only for local storage

[Ján Tomko]

s/existance/existence/
in the commit summary

On Fri, Aug 16, 2019 at 03:54:38PM +0200, Peter Krempa wrote:

Soon we'll allow more protocols and storage types with snapshots where
we in some cases can't check whether the storage already exists.
Restrict the sanity checks whether the destination images exist or not
for local storage where it's easy. For any other case we will fail
later.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 44 ++
1 file changed, 23 insertions(+), 21 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/10] qemu: Remove cleanup label in qemuDomainSnapshotPrepareDiskExternal

[Ján Tomko]

On Fri, Aug 16, 2019 at 03:54:37PM +0200, Peter Krempa wrote:

Refactor the code to avoid having a cleanup label. This will simplify
the change necessary when restricting this check in an upcoming patch.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 28 +++-
1 file changed, 15 insertions(+), 13 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/10] qemu: snapshot: Don't modify persistent XML if disk source is different

[Ján Tomko]

On Fri, Aug 16, 2019 at 03:54:35PM +0200, Peter Krempa wrote:

While the VM is running the persistent source of a disk might differ
e.g. as the 'newDef' was redefined. Our snapshot code would blindly
rewrite the source of such disk if it shared the 'target'. Fix this by
checking whether the source is the same in the first place.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/10] qemu: driver: Remove dead code from qemuDomainSnapshotUpdateDiskSources

[Ján Tomko]

On Fri, Aug 16, 2019 at 03:54:36PM +0200, Peter Krempa wrote:

dd->src is always allocated in this function as it contains the new
source for the snapshot which is meant to replace the disk source.

The label handling code executed if that source was not present thus is
dead code. Remove it.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 7 ---
1 file changed, 7 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 5/5] security_util: Remove stale XATTRs

[Jiri Denemark]

On Wed, Aug 14, 2019 at 16:33:23 +0200, Michal Privoznik wrote:
> It may happen that we leave some XATTRs behind. For instance, on
> a sudden power loss, the host just shuts down without calling
> restore on domain paths. This creates a problem, because when the
> host starts up again, the XATTRs are there but they don't reflect
> the true state and this may result in libvirt denying start of a
> domain.
> 
> To solve this, save a unique timestamp among with our XATTRs. The
> timestamp consists of host UUID + boot timestamp.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/security/security_util.c | 202 ++-
>  tests/qemusecuritymock.c |  12 +++
>  2 files changed, 213 insertions(+), 1 deletion(-)
> 
> diff --git a/src/security/security_util.c b/src/security/security_util.c
> index 365b2dd2d6..d063f526be 100644
> --- a/src/security/security_util.c
> +++ b/src/security/security_util.c
> @@ -22,11 +22,16 @@
>  #include "virfile.h"
>  #include "virstring.h"
>  #include "virerror.h"
> +#include "virlog.h"
> +#include "viruuid.h"
> +#include "virhostuptime.h"
>  
>  #include "security_util.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_SECURITY
>  
> +VIR_LOG_INIT("security.security_util");
> +
>  /* There are four namespaces available on Linux (xattr(7)):
>   *
>   *  user - can be modified by anybody,
> @@ -83,6 +88,157 @@ virSecurityGetRefCountAttrName(const char *name 
> ATTRIBUTE_UNUSED)
>  }
>  
>  
> +static char *
> +virSecurityGetTimestampAttrName(const char *name ATTRIBUTE_UNUSED)
> +{
> +char *ret = NULL;
> +#ifdef XATTR_NAMESPACE
> +ignore_value(virAsprintf(, 
> XATTR_NAMESPACE".libvirt.security.timestamp_%s", name));
> +#else
> +errno = ENOSYS;
> +virReportSystemError(errno, "%s",
> + _("Extended attributes are not supported on this 
> system"));
> +#endif
> +return ret;
> +}

Again, put #ifdef outside, please.

> +
> +
> +/* This global timestamp holds combination of host UUID + boot time so that 
> we
> + * can detect stale XATTRs. For instance, on a sudden power loss, XATTRs are
> + * not going to change (nobody will call restoreLabel()) and thus they 
> reflect
> + * state from just before the power loss and if there was a machine running,
> + * then XATTRs there are stale and no one will ever remove them. They don't
> + * reflect the true state (most notably the ref counter).
> + */
> +static char *timestamp;
> +
> +
> +static int
> +virSecurityEnsureTimestamp(void)
> +{
> +unsigned char uuid[VIR_UUID_BUFLEN] = {0};
> +char uuidstr[VIR_UUID_STRING_BUFLEN] = {0};
> +unsigned long long boottime = 0;
> +
> +if (timestamp)
> +return 0;
> +
> +if (virGetHostUUID(uuid) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   "%s", _("cannot get the host uuid"));
> +return -1;
> +}
> +
> +virUUIDFormat(uuid, uuidstr);
> +
> +if (virHostGetBootTime() < 0) {
> +virReportSystemError(errno, "%s",
> + _("Unable to get host boot time"));
> +return -1;
> +}
> +
> +if (virAsprintf(, "%s-%llu", uuidstr, boottime) < 0)
> +return -1;
> +
> +return 0;
> +}
> +
> +
> +/**
> + * virSecurityValidateTimestamp:
> + * @name: security driver name
> + * @path: file name
> + *
> + * Check if remembered label on @path for security driver @name
> + * is valid, i.e. the label has been set since the last boot. If
> + * the label was set in previous runs, all XATTRs related to
> + * @name are removed so that clean slate is restored.
> + *
> + * Returns: 0 if remembered label is valid,
> + *  1 if remembered label was not valid,
> + * -1 otherwise.
> + */
> +static int
> +virSecurityValidateTimestamp(const char *name,
> + const char *path)
> +{
> +VIR_AUTOFREE(char *) timestamp_name = NULL;
> +VIR_AUTOFREE(char *) value = NULL;
> +
> +if (virSecurityEnsureTimestamp() < 0)
> +return -1;
> +
> +if (!(timestamp_name = virSecurityGetTimestampAttrName(name)))
> +return -1;
> +
> +errno = 0;
> +if (virFileGetXAttrQuiet(path, timestamp_name, ) < 0) {
> +if (errno == ENOSYS || errno == ENOTSUP) {
> +/* XATTRs are not supported. */

Redundant comment.

> +return -1;
> +} else if (errno != ENODATA) {
> +virReportSystemError(errno,
> + _("Unable to get XATTR %s on %s"),
> + timestamp_name,
> + path);
> +return -1;
> +}
> +
> +/* Timestamp is missing. We can continue and claim a valid timestamp.
> + * But then we would never remove stale XATTRs. Therefore, claim it
> + * invalid and have the code below remove all XATTRs. This of course
> + * means that we will not restore the original owner, but the plus 
> side
> + * is that we reset 

Re: [libvirt] [PATCH 4/5] util: Introduce virhostuptime

[Pino Toscano]

On Wednesday, 14 August 2019 16:33:22 CEST Michal Privoznik wrote:
> +int
> +virHostGetBootTime(unsigned long long *when ATTRIBUTE_UNUSED)
> +{
> +#if defined(__linux__) || defined(__FreeBSD__)
> +struct utmpx id = {.ut_type = BOOT_TIME};
> +struct utmpx *res = NULL;
> +
> +if (!(res = getutxid())) {
> +int theerrno = errno;
> +endutxent();
> +return -theerrno;
> +}
> +
> +*when = res->ut_tv.tv_sec;
> +endutxent();
> +return 0;
> +#else
> +/* Unfortunately, mingw lacks utmp */
> +errno = ENOSYS;
> +return -errno;
> +#endif

In addition to what Jirka said, IMHO it would be better to do a
configure detection of the non-POSIX function, rather than assuming on
which platforms/OSes it is available.

-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 14/14] util: storagefile: Flag backing store strings with authentication

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:35PM +0200, Peter Krempa wrote:

Using inline authentication for storage volumes will not work properly
as libvirt requires use of the secret driver for the auth data and
thus would not be able to represent the passwords stored in the backing
store string.

Make sure that the backing store parsers return 1 which is a sign for
the caller to not use the file in certain cases.

The test data include iscsi via a json pseudo-protocol string and URIs
with the userinfo part being present.

Signed-off-by: Peter Krempa 
---
src/util/virstoragefile.c | 11 +--
tests/virstoragetest.c| 28 
2 files changed, 37 insertions(+), 2 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 13/14] util: storagefile: Don't traverse storage sources unusable by VM

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:34PM +0200, Peter Krempa wrote:

virStorageFileGetMetadataRecurse would include in the backing chain
files which would not really be usable by libvirt directly e.g. when


include files in the backing chain


such file would be promoted to the top layer by a active block commit as


an active


for example inline authentication data can't be represented in the VM
xml file. The idea is to use secrets for this.

With the changes to the backing store string parsers we can report and
propagate if such a thing is present in the configuration and thus start
skipping those files in the backing chain traversal code. This approach
still allows to report the appropriate backing store string in the
storage driver which doesn't directly use the backing file.

Signed-off-by: Peter Krempa 
---
src/util/virstoragefile.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 12/14] util: storagefile: Clarify docs for '@report_broken' of virStorageFileGetMetadata

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:33PM +0200, Peter Krempa wrote:

virStorageFileGetMetadata does not report error if we can't interrogate
the file somehow. Clarify this in the description of the @report_broken
flag as it implies we should report an error in that case. The problem
is that we don't know whether there's a problem and unforntnately just


unfortunately


offload it to qemu.

Signed-off-by: Peter Krempa 
---
src/util/virstoragefile.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 10/14] tests: virstorage: Allow testing return value of virStorageSourceNewFromBackingAbsolute

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:31PM +0200, Peter Krempa wrote:

Modiy testBackingParse to allow testing other return values of the


Modify


backing store string parser.

Signed-off-by: Peter Krempa 
---
tests/virstoragetest.c | 24 ++--
1 file changed, 18 insertions(+), 6 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/14] util: storagefile: Add handling of unusable storage sources

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:32PM +0200, Peter Krempa wrote:

Introduce new semantics to virStorageSourceNewFromBacking and some
of the helpers used by it which propagate the return value from the
callers.

The new return value introduced by this patch allows to notify the
calller that the parsed virStorageSource correctly describes the source
but contains data such as inline authentication which libvirt does not
want to support directly. This means that such file would e.g. unusable
as a storage source (e.g. when actively commiting the overlay to it) or
would not work with blockdev.

The caller will then be able to decide whether to consider this backing
file as viable or just fall back to qemu dealing with it.

Signed-off-by: Peter Krempa 
---
src/util/virstoragefile.c | 30 ++
1 file changed, 22 insertions(+), 8 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/5] util: Introduce virhostuptime

[Jiri Denemark]

On Wed, Aug 14, 2019 at 16:33:22 +0200, Michal Privoznik wrote:
> This module contains function to get host boot time.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt_private.syms |  4 +++
>  src/util/Makefile.inc.am |  2 ++
>  src/util/virhostuptime.c | 61 
>  src/util/virhostuptime.h | 27 ++
>  4 files changed, 94 insertions(+)
>  create mode 100644 src/util/virhostuptime.c
>  create mode 100644 src/util/virhostuptime.h
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 7a3feb8efa..bed00c3cb8 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2135,6 +2135,10 @@ virHostMemGetStats;
>  virHostMemSetParameters;
>  
>  
> +# util/virhostuptime.h
> +virHostGetBootTime;
> +
> +
>  # util/viridentity.h
>  virIdentityGetAttr;
>  virIdentityGetCurrent;
> diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
> index a47f333a98..46866cf213 100644
> --- a/src/util/Makefile.inc.am
> +++ b/src/util/Makefile.inc.am
> @@ -91,6 +91,8 @@ UTIL_SOURCES = \
>   util/virhostdev.h \
>   util/virhostmem.c \
>   util/virhostmem.h \
> + util/virhostuptime.c \
> + util/virhostuptime.h \
>   util/viridentity.c \
>   util/viridentity.h \
>   util/virinitctl.c \
> diff --git a/src/util/virhostuptime.c b/src/util/virhostuptime.c
> new file mode 100644
> index 00..f48de672e6
> --- /dev/null
> +++ b/src/util/virhostuptime.c
> @@ -0,0 +1,61 @@
> +/*
> + * virhostuptime.c: helper APIs for host uptime
> + *
> + * Copyright (C) 2019 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * .
> + */
> +
> +#include 
> +
> +#if defined(__linux__) || defined(__FreeBSD__)
> +# include 
> +#endif
> +
> +#include "virhostuptime.h"
> +
> +
> +/**
> + * virHostGetBootTime:
> + * @when: UNIX timestamp of boot time
> + *
> + * Get a UNIX timestamp of host boot time and store it at @when.
> + * Note that this function is not reentrant.
> + *
> + * Return: 0 on success,
> + *-1 otherwise.
> + */
> +int
> +virHostGetBootTime(unsigned long long *when ATTRIBUTE_UNUSED)
> +{
> +#if defined(__linux__) || defined(__FreeBSD__)

I believe we usually use a bit different approach:

#if ...

int
virHostGetBootTime(unsigned long long *when)
{
...
}

#else

int
virHostGetBootTime(unsigned long long *when ATTRIBUTE_UNUSED)
{
return ...;
}

#endif

> +struct utmpx id = {.ut_type = BOOT_TIME};
> +struct utmpx *res = NULL;
> +
> +if (!(res = getutxid())) {

getutxid is not thread safe

> +int theerrno = errno;
> +endutxent();
> +return -theerrno;
> +}
> +
> +*when = res->ut_tv.tv_sec;
> +endutxent();
> +return 0;
> +#else
> +/* Unfortunately, mingw lacks utmp */
> +errno = ENOSYS;
> +return -errno;
> +#endif
> +}

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 09/14] util: storagefile: Modify arguments of virStorageSourceNewFromBackingAbsolue

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:30PM +0200, Peter Krempa wrote:

Return the parsed storage source via an pointer in arguments and return
an integer from the function. Describe the semantics with a comment for
the function and adjust callers to the new semantics.

Signed-off-by: Peter Krempa 
---
src/util/virstoragefile.c | 43 ---
src/util/virstoragefile.h |  3 ++-
tests/qemublocktest.c |  3 ++-
tests/virstoragetest.c|  2 +-
4 files changed, 32 insertions(+), 19 deletions(-)



signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 08/14] util: storagefile: Preserve return value in virStorageSourceParseBackingJSONUriStr

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:29PM +0200, Peter Krempa wrote:

virStorageSourceParseBackingURI will report special return values in
some cases. Preserve it in virStorageSourceParseBackingJSONUriStr.

Signed-off-by: Peter Krempa 
---
src/util/virstoragefile.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 07/14] util: storage: Modify return value of virStorageSourceNewFromBacking

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:28PM +0200, Peter Krempa wrote:

Return the storage source definition via a pointer in the arguments and
document the returned values. This will simplify the possibility to
ignore certain backing store types which are not representable by
libvirt.

Signed-off-by: Peter Krempa 
---
src/storage/storage_util.c |  2 +-
src/util/virstoragefile.c  | 59 --
src/util/virstoragefile.h  |  4 ++-
3 files changed, 41 insertions(+), 24 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 6fff013e3a..4e57a0438e 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3679,42 +3679,57 @@ virStorageSourceNewFromBackingAbsolute(const char *path)
}


-virStorageSourcePtr
-virStorageSourceNewFromBacking(virStorageSourcePtr parent)
+/**
+ * virStorageSourceNewFromBacking:
+ * @parent: storage source parent
+ * @backing: returned backing store definition
+ *
+ * Creates a storage source which describes the backing image of @parent and
+ * fills it into @backing depending on the 'backingStoreRaw' property of 
@parent
+ * and other data. Note that for local storage this function interrogates the


Not quite sure what 'interrogates' is supposed to mean in this context.
Perhaps you could use another verb?


+ * actual type of the backing store.
+ *
+ * Returns 0 and fills @backing, or -1 on error (with appropriate error 
reported).
+ */
+int
+virStorageSourceNewFromBacking(virStorageSourcePtr parent,
+   virStorageSourcePtr *backing)
{


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/5] security_util: Document virSecurityMoveRememberedLabel

[Jiri Denemark]

On Wed, Aug 14, 2019 at 16:33:21 +0200, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  src/security/security_util.c | 13 +
>  1 file changed, 13 insertions(+)

Reviewed-by: Jiri Denemark 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/5] security_util: Use more VIR_AUTOFREE()

[Jiri Denemark]

On Wed, Aug 14, 2019 at 16:33:20 +0200, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  src/security/security_util.c | 78 +++-
>  1 file changed, 32 insertions(+), 46 deletions(-)

Reviewed-by: Jiri Denemark 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/5] virUUIDFormat: s/VIR_UUID_RAW_LEN/VIR_UUID_BUFLEN/ in comment

[Jiri Denemark]

On Wed, Aug 14, 2019 at 16:33:19 +0200, Michal Privoznik wrote:
> The function takes raw UUID and formats it into string
> representation. However, the comment mistakenly states that the
> expected size of raw UUID buffer is VIR_UUID_RAW_LEN bytes. We
> don't have such constant since v0.3.2~24. It should have been
> VIR_UUID_BUFLEN.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/viruuid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/util/viruuid.c b/src/util/viruuid.c
> index 0c12ddcc3e..8930a0e199 100644
> --- a/src/util/viruuid.c
> +++ b/src/util/viruuid.c
> @@ -141,7 +141,7 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid)
>  
>  /**
>   * virUUIDFormat:
> - * @uuid: array of VIR_UUID_RAW_LEN bytes to store the raw UUID
> + * @uuid: array of VIR_UUID_BUFLEN bytes to store the raw UUID
>   * @uuidstr: array of VIR_UUID_STRING_BUFLEN bytes to store the
>   * string representation of the UUID in. The resulting string
>   * will be NULL terminated.

Reviewed-by: Jiri Denemark 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 06/14] tests: storage: Refactor cleanup in testBackingParse

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:27PM +0200, Peter Krempa wrote:

Automatically clean the temporary buffer and get rid of the cleanup
label.

Signed-off-by: Peter Krempa 
---
tests/virstoragetest.c | 22 --
1 file changed, 8 insertions(+), 14 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/14] tests: viruri: Add test for password in URI userinfo

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:26PM +0200, Peter Krempa wrote:

While it's a bad idea to use userinfo to pass credentials via an URI add


s/an URI/a URI/


a test that we at least do the correct thing.

Signed-off-by: Peter Krempa 
---
tests/viruritest.c | 1 +
1 file changed, 1 insertion(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 04/14] util: storagefile: Simplify cleanup in virStorageSourceParseBackingJSON

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:25PM +0200, Peter Krempa wrote:

Automatically free the 'root' temporary variable to get rid fo some


s/fo/of/


complexity.

Signed-off-by: Peter Krempa 
---
src/util/virstoragefile.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/14] util: storagefile: Simplify cleanup handling in virStorageSourceParseBackingURI

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:24PM +0200, Peter Krempa wrote:

Automatically clean the 'uri' variable and get rid of the 'cleanup'
label.

Signed-off-by: Peter Krempa 
---
src/util/virstoragefile.c | 31 +--
1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 8af45bfbd2..e93f6285b0 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2664,32 +2664,31 @@ static int
virStorageSourceParseBackingURI(virStorageSourcePtr src,
const char *uristr)
{
-virURIPtr uri = NULL;
+VIR_AUTOPTR(virURI)uri = NULL;


Space between the closing parenthesis and the variable name, please.


const char *path = NULL;
-int ret = -1;
VIR_AUTOSTRINGLIST scheme = NULL;


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/14] util: storagefile: Remove cleanup label from virStorageSourceParseBackingJSONiSCSI

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:23PM +0200, Peter Krempa wrote:

There is no cleanup code.



Do not try and bend the cleanup code.


Signed-off-by: Peter Krempa 
---
src/util/virstoragefile.c | 20 
1 file changed, 8 insertions(+), 12 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/14] util: storage: Simplify cleanup path handling in virStorageSourceParseBackingJSONInternal

[Ján Tomko]

On Fri, Aug 16, 2019 at 12:39:22PM +0200, Peter Krempa wrote:

Automatically free the intermediate JSON data to get rid of the cleanup
section.

Signed-off-by: Peter Krempa 
---
src/util/virstoragefile.c | 20 +++-
1 file changed, 7 insertions(+), 13 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] QEMU bitmap backup usability FAQ

[Vladimir Sementsov-Ogievskiy]

[CC Nikolay]

21.08.2019 1:25, John Snow wrote:
> Hi, downstream here at Red Hat I've been fielding some questions about
> the usability and feature readiness of Bitmaps (and related features) in
> QEMU.
> 
> Here are some questions I answered internally that I am copying to the
> list for two reasons:
> 
> (1) To make sure my answers are actually correct, and
> (2) To share this pseudo-reference with the community at large.
> 
> This is long, and mostly for reference. There's a summary at the bottom
> with some todo items and observations about the usability of the feature
> as it exists in QEMU.
> 
> Before too long, I intend to send a more summarized "roadmap" mail which
> details all of the current and remaining work to be done in and around
> the bitmaps feature in QEMU.
> 
> 
> Questions:
> 
>> "What format(s) is/are required for this functionality?"
> 
>  From the QEMU API, any format can be used to create and author
> incremental backups. The only known format limitations are:
> 
> 1. Persistent bitmaps cannot be created on any format except qcow2,
> although there are hooks to add support to other formats at a later date
> if desired.
> 
> DANGER CAVEAT #1: Adding bitmaps to QEMU by default creates transient
> bitmaps instead of persistent ones.
> 
> Possible TODO: Allow users to 'upgrade' transient bitmaps to persistent
> ones in case they made a mistake.

I doubt, as in my opinion real users of Qemu are not people but libvirt, which
should never make such mistake.

> 
> 
> 2. When using push backups (blockdev-backup, drive-backup), you may use
> any format as a target format. >
> DANGER CAVEAT #2: without backing file and/or filesystem-less sparse
> support, these images will be unusable.

You mean incremental backups of course, as the whole document is about bitmaps.

> 
> EXAMPLE: Backing up to a raw file loses allocation information, so we
> can no longer distinguish between zeroes and unallocated regions. The
> cluster size is also lost. This file will not be usable without
> additional metadata recorded elsewhere.*
> 
> (* This is complicated, but it is in theory possible to do a push backup
> to e.g. an NBD target with custom server code that saves allocation
> information to a metadata file, which would allow you to reconstruct
> backups. For instance, recording in a .json file which extents were
> written out would allow you to -- with a custom binary -- write this
> information on top of a base file to reconstruct a backup.)
> 
> 
> 3. Any format can be used for either shared storage or live storage
> migrations. There are TWO distinct mechanisms for migrating bitmaps:
> 
> A) The bitmap is flushed to storage and re-opened on the destination.
> This is only supported for qcow2 and shared-storage migrations.

cons: flushing/reopening is done during migration downtime, so if you have
a lot of bitmap data (for example, 64k granulared bitmap for 16tb disk is
~30MB, and there may be several bitmaps) downtime will become long.

> 
> B) The bitmap is live-migrated to the destination. This is supported for
> any format and can be used for either shared storage or live storage
> migrations.
> 
> DANGER CAVEAT #3: The second bitmap migration technique there is an
> optional migration capability that must be enabled explicitly.
> Otherwise, some migration combinations may drop bitmaps.

Also, bad thing may happen if we try to migrate persistent bitmap to not qcow2.

Also, with enabled capability, flushing/reopening is automatically disabled.

> 
> Matrix:
> 
>> migrate = migrate_capability or (persistent and shared_storage)
> 
> Enumerated:
> 
> live storage + raw : transient + no-capability: Dropped
> live-storage + raw : transient + bm-capability: Migrated
> live-storage + qcow2 : transient + no-capability: Dropped
> live-storage + qcow2 : transient + bm-capability: Migrated
> live-storage + qcow2 : persistent + no-capability: Dropped (!)
> live-storage + qcow2 : persistent + bm-capability: Migrated
> 
> shared-storage + raw : transient - no-capability: Dropped
> shared-storage + raw : transient + bm-capability: Migrated
> shared-storage + qcow2 : transient + no-capability: Migrated

Dropped you mean

> shared-storage + qcow2 : transient + bm-capability: Migrated
> shared-storage + qcow2 : persistent + no-capability: Migrated
> shared-storage + qcow2 : persistent + bm-capability: Migrated
> 
> Enabling the bitmap migration capability will ALWAYS migrate the bitmap.
> If it's disabled, we will only migrate the bitmaps for shared storage
> migrations where the bitmap is persistent, which is a qcow2-only case.
> 
> There is no warning or error if you attempt to migrate in a manner that
> loses your bitmaps.
> 
> (I might be persuaded to add a case for when you are doing a live
> storage migration of qcow2 with persistent bitmaps, which is somewhat a
> conflicting case: you've asked for the bitmap to be persistent, but it
> seems likely that if this ever happens in practice, it's because you
> have 

[libvirt] [PATCH v3] tools: console: Relax stream EOF handling

[Roman Bolshakov]

Regular VM shutdown triggers the error for existing session of virsh
console and it returns with non-zero exit code:
  error: internal error: console stream EOF

The message and status code are misleading because there's no real
error. virStreamRecv returns 0 correctly when EOF is reached.

Existing implementations of esx, fd, and remote streams behave the same
for virStreamFinish and virStreamAbort: they close the stream. So, we
can continue to use virStreamAbort to handle EOF and errors from
virStreamRecv but additonally we can report error if virStreamAbort
fails.

Fixes: 29f2b5248c6 ("tools: console: pass stream/fd errors to user")
Signed-off-by: Roman Bolshakov 
---
 tools/virsh-console.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index e16f841e57..a235a9a283 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -106,7 +106,9 @@ virConsoleShutdown(virConsolePtr con)
 
 if (con->st) {
 virStreamEventRemoveCallback(con->st);
-virStreamAbort(con->st);
+if (virStreamAbort(con->st) < 0)
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("cannot terminate console stream"));
 virStreamFree(con->st);
 con->st = NULL;
 }
@@ -172,10 +174,6 @@ virConsoleEventOnStream(virStreamPtr st,
 if (got == -2)
 goto cleanup; /* blocking */
 if (got <= 0) {
-if (got == 0)
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("console stream EOF"));
-
 virConsoleShutdown(con);
 goto cleanup;
 }
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virt-aa-helper: Drop unnecessary AppArmor rule

[Martin Kletzander]

On Wed, Aug 21, 2019 at 09:45:01AM +0200, Andrea Bolognani wrote:

Apparently /proc/self is automatically converted to /proc/@{pid}
before checking rules, which makes spelling it out explicitly
redundant.



Because it is usually a symlink.

Reviewed-by: Martin Kletzander 


Suggested-by: Jamie Strandboge 
Signed-off-by: Andrea Bolognani 
---
src/security/apparmor/usr.lib.libvirt.virt-aa-helper | 1 -
1 file changed, 1 deletion(-)

diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper 
b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
index 64772f0756..11e9c039ca 100644
--- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
+++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
@@ -18,7 +18,6 @@ profile virt-aa-helper 
/usr/{lib,lib64}/libvirt/virt-aa-helper {
  @{PROC}/filesystems r,

  # Used when internally running another command (namely apparmor_parser)
-  @{PROC}/self/fd/ r,
  @{PROC}/@{pid}/fd/ r,

  /etc/libnl-3/classid r,
--
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/12] Drop KVM assignment

[Daniel Henrique Barboza]

On 8/21/19 9:53 AM, Ján Tomko wrote:

On Tue, Aug 20, 2019 at 02:53:32PM -0300, Daniel Henrique Barboza wrote:



On 8/20/19 11:30 AM, Michal Privoznik wrote:

The KVM style of PCI assignment is not used, and it hasn't been for a
while. Any attempt to start a domain with it would result in error as
kernel dropped its support in 4.12.0 (after being deprecated for 1.5
years).


LGTM. Just a comment in patch 01.


After applying the whole series I tried to find the remaining references
of 'pci-assign'. This is what git grep returns:



Yes, the 'kvm-pci-assign' device was present in those QEMU versions so
it was listed in the replies.



$ git grep 'pci-assign'
src/qemu/qemu_capabilities.h:    X_QEMU_CAPS_PCI_CONFIGFD, /* 
pci-assign.configfd */
src/qemu/qemu_capabilities.h:    X_QEMU_CAPS_PCI_BOOTINDEX, /* 
pci-assign.bootindex */
tests/qemucapabilitiesdata/caps_1.5.3.x86_64.replies: "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_1.6.0.x86_64.replies: "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_1.7.0.x86_64.replies: "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.1.1.x86_64.replies: "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.10.0.x86_64.replies: "name": 
"kvm-pci-assign",
tests/qemucapabilitiesdata/caps_2.4.0.x86_64.replies: "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.5.0.x86_64.replies: "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.6.0.x86_64.replies: "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.7.0.x86_64.replies: "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.8.0.x86_64.replies: "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.9.0.x86_64.replies: "name": 
"kvm-pci-assign"



And with 'configfd':

$ git grep 'configfd'
docs/news-2011.html.in:  qemu: simplify PCI configfd handling in 
monitor (Eric Blake),

src/qemu/qemu_capabilities.c:  "pci-configfd",
src/qemu/qemu_capabilities.h:    X_QEMU_CAPS_PCI_CONFIGFD, /* 
pci-assign.configfd */
tests/qemustatusxml2xmldata/migration-in-params-in.xml: name='pci-configfd'/>
tests/qemustatusxml2xmldata/migration-out-params-in.xml: name='pci-configfd'/>




Do we still need X_QEMU_CAPS_PCI_CONFIGFD and X_QEMU_CAPS_PCI_BOOTINDEX
after this series?



We need to be capable of parsing the qemu capability flags produced by
older libvirt even though we aren't taking them into account.

When upgrading libvirt while a domain is running, or migrating to a host
with newer libvirt, rejecting the capability might break existing guests
even though they did not use that particular capability.

So we keep it around with an X- prefix and the corresponding string
representation unchanged and just quietly ignore it.



Got it. Thanks for the explanation!


DHB



Jano


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 1/2] qemu: formatting XML from domain def choosing the root name

[Jiri Denemark]

On Wed, Aug 14, 2019 at 11:47:21 -0300, Maxiwell S. Garcia wrote:
> The function virDomainDefFormatInternal() has the predefined root name
> "domain" to format the XML. But to save both active and inactive domain
> in the snapshot XML, the new root name "inactiveDomain" was created.
> So, this function was modified to be driven by the new flag
> VIR_DOMAIN_DEF_FORMAT_INACTIVE_NODE to choose the correct root name.
> 
> Signed-off-by: Maxiwell S. Garcia 
> ---
>  src/conf/domain_conf.c | 13 ++---
>  src/conf/domain_conf.h |  1 +
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 03312afaaf..7d6393b9ac 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -28230,6 +28230,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>  unsigned char *uuid;
>  char uuidstr[VIR_UUID_STRING_BUFLEN];
>  const char *type = NULL;
> +const char *rootname = NULL;
>  int n;
>  size_t i;
>  char *netprefix = NULL;
> @@ -28238,7 +28239,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>VIR_DOMAIN_DEF_FORMAT_STATUS |
>VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET |
>VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES |
> -  VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST,
> +  VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST |
> +  VIR_DOMAIN_DEF_FORMAT_INACTIVE_NODE,
>-1);
>  
>  if (!(type = virDomainVirtTypeToString(def->virtType))) {
> @@ -28250,7 +28252,12 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>  if (def->id == -1)
>  flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
>  
> -virBufferAsprintf(buf, " +if (flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE_NODE)
> +rootname = "inactiveDomain";
> +else
> +rootname = "domain";

This looks like an ugly hack. I suggest a bit different approach which
involves creating a new function. Just add a new parameter for the root
element name, rename this function, and create a new one with the
original name which would be a tiny wrapper around the renamed function
passing "domain" as the root element name.

This way you can keep existing code untouched and just call the new
function when you need a different root element name.

In other places we sometimes separate formatter for the internals from
the code formatting the container element. But since we need to add
attributes to the container element, such approach would be quite ugly
in this case.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/12] Drop KVM assignment

[Ján Tomko]

On Tue, Aug 20, 2019 at 02:53:32PM -0300, Daniel Henrique Barboza wrote:



On 8/20/19 11:30 AM, Michal Privoznik wrote:

The KVM style of PCI assignment is not used, and it hasn't been for a
while. Any attempt to start a domain with it would result in error as
kernel dropped its support in 4.12.0 (after being deprecated for 1.5
years).


LGTM. Just a comment in patch 01.


After applying the whole series I tried to find the remaining references
of 'pci-assign'. This is what git grep returns:



Yes, the 'kvm-pci-assign' device was present in those QEMU versions so
it was listed in the replies.



$ git grep 'pci-assign'
src/qemu/qemu_capabilities.h:    X_QEMU_CAPS_PCI_CONFIGFD, /* 
pci-assign.configfd */
src/qemu/qemu_capabilities.h:    X_QEMU_CAPS_PCI_BOOTINDEX, /* 
pci-assign.bootindex */
tests/qemucapabilitiesdata/caps_1.5.3.x86_64.replies:  "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_1.6.0.x86_64.replies:  "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_1.7.0.x86_64.replies:  "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.1.1.x86_64.replies:  "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.10.0.x86_64.replies:  "name": 
"kvm-pci-assign",
tests/qemucapabilitiesdata/caps_2.4.0.x86_64.replies:  "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.5.0.x86_64.replies:  "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.6.0.x86_64.replies:  "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.7.0.x86_64.replies:  "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.8.0.x86_64.replies:  "name": 
"kvm-pci-assign"
tests/qemucapabilitiesdata/caps_2.9.0.x86_64.replies:  "name": 
"kvm-pci-assign"



And with 'configfd':

$ git grep 'configfd'
docs/news-2011.html.in:  qemu: simplify PCI configfd handling in 
monitor (Eric Blake),

src/qemu/qemu_capabilities.c:  "pci-configfd",
src/qemu/qemu_capabilities.h:    X_QEMU_CAPS_PCI_CONFIGFD, /* 
pci-assign.configfd */
tests/qemustatusxml2xmldata/migration-in-params-in.xml:    name='pci-configfd'/>
tests/qemustatusxml2xmldata/migration-out-params-in.xml:    name='pci-configfd'/>




Do we still need X_QEMU_CAPS_PCI_CONFIGFD and X_QEMU_CAPS_PCI_BOOTINDEX
after this series?



We need to be capable of parsing the qemu capability flags produced by
older libvirt even though we aren't taking them into account.

When upgrading libvirt while a domain is running, or migrating to a host
with newer libvirt, rejecting the capability might break existing guests
even though they did not use that particular capability.

So we keep it around with an X- prefix and the corresponding string
representation unchanged and just quietly ignore it.

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 12/12] news: Document KVM assignment removal

[Ján Tomko]

On Tue, Aug 20, 2019 at 04:30:32PM +0200, Michal Privoznik wrote:

Signed-off-by: Michal Privoznik 
---
docs/news.xml | 13 +
1 file changed, 13 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index d63fca3b48..6137ebd943 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -51,6 +51,19 @@

  

+
+  
+
+  Remove KVM assignment support
+
+
+  The KVM style of PCI device assignment was removed from
+  kernel in kernel 4.12.0 after being deprecated since 4.2.0.


s/kernel in kernel/the kernel in version/


+  Libvirt defaults to VFIO for a long time. Remove support for
+  KVM device assignment then.


s/then/from libvirt too/

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 10/12] virpcimock: Don't create new_id or remove_id files

[Ján Tomko]

On Tue, Aug 20, 2019 at 04:30:30PM +0200, Michal Privoznik wrote:

Now that PCI attach/detach happens solely via driver_override
these two files are no longer needed.

Signed-off-by: Michal Privoznik 
---
tests/virpcimock.c | 112 -
1 file changed, 112 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/12] virpcimock: Drop @driverActions enum

[Ján Tomko]

On Tue, Aug 20, 2019 at 04:30:31PM +0200, Michal Privoznik wrote:

This enum was introduced to model how RHEL-7 kernel behaves - for
some reason going with the old way (via new_id + bind) fails but
using driver_override succeeds. Well, we don't need to care about
that anymore since we don't create new_id file.

Signed-off-by: Michal Privoznik 
---
tests/virpcimock.c | 24 +++-
1 file changed, 7 insertions(+), 17 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 08/12] virpci: Drop newid style of PCI device detach

[Ján Tomko]

On Tue, Aug 20, 2019 at 04:30:28PM +0200, Michal Privoznik wrote:

As stated in 84f9358b18346 all kernels that we are interested in
have 'drivers_override'. Drop the other, older style of
overriding PCI device driver - newid.

Signed-off-by: Michal Privoznik 
---
src/util/virpci.c | 284 +-
1 file changed, 2 insertions(+), 282 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 09/12] virpcimock: Don't create "pci-stub" driver

[Ján Tomko]

On Tue, Aug 20, 2019 at 04:30:29PM +0200, Michal Privoznik wrote:

Now that nothing supports "pci-stub" driver (aka KVM style of PCI
device assignment) there is no need for virpcimock to create it.

Signed-off-by: Michal Privoznik 
---
tests/virpcimock.c | 1 -
1 file changed, 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 07/12] virpci: Remove unused virPCIDeviceWaitForCleanup

[Ján Tomko]

On Tue, Aug 20, 2019 at 04:30:27PM +0200, Michal Privoznik wrote:

This function is no longer used after previous commit.

Signed-off-by: Michal Privoznik 
---
src/libvirt_private.syms |   1 -
src/util/virpci.c| 108 ---
src/util/virpci.h|   1 -
3 files changed, 110 deletions(-)



:,-)

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 06/12] virpci: Drop 'pci-stub' driver

[Ján Tomko]

On Tue, Aug 20, 2019 at 04:30:26PM +0200, Michal Privoznik wrote:

Now that no one uses KVM style of PCI assignment we can safely
remove 'pci-stub' backend.

Signed-off-by: Michal Privoznik 
---
src/util/virpci.c | 11 ---
src/util/virpci.h |  1 -
2 files changed, 12 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/12] virhostdev: Disable legacy kvm assignment

[Ján Tomko]

On Tue, Aug 20, 2019 at 04:30:25PM +0200, Michal Privoznik wrote:

The KVM assignment is going to be removed shortly. Don't let the
hostdev module configure it.

Signed-off-by: Michal Privoznik 
---
src/util/virhostdev.c | 12 
1 file changed, 8 insertions(+), 4 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Fwd: libvirtd failing on MacOS in setgroups

[Daniel P . Berrangé]

On Tue, Aug 20, 2019 at 11:11:07AM -0400, Marcus Furlong wrote:
> Resend to libvir-list in case that is more appropriate:
> 
> 
> Hi,
> 
> I get the following error when running libvirtd on MacOS as root:
> 
> 2019-07-11 00:12:33.673+: 123145573953536: error :
> qemuProcessQMPLaunch:8501 : internal error: Failed to start QEMU
> binary /usr/local/bin/qemu-system-x86_64 for probing: libvirt:  error
> : cannot set supplemental groups: Invalid argument

Are you able to run 'strace' (or whatever MacOS eqiuv is) to see
the values passed to setgroups when it fails ?


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 04/12] qemu: Drop unused qemuOpenPCIConfig()

[Ján Tomko]

On Tue, Aug 20, 2019 at 04:30:24PM +0200, Michal Privoznik wrote:

After previous commits, the function is not used anymore.
Remove it.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_command.c | 22 --
src/qemu/qemu_command.h |  2 --
2 files changed, 24 deletions(-)



Beautiful diffstat!

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/12] virhostdev: Unify virDomainHostdevDef to virPCIDevice translation

[Ján Tomko]

On Tue, Aug 20, 2019 at 04:30:23PM +0200, Michal Privoznik wrote:

There are two places where we need to create virPCIDevice from
given virDomainHostdevDef. In both places the code is duplicated.
Move them into a single function and call it from those two
places.

Signed-off-by: Michal Privoznik 
---
src/util/virhostdev.c | 93 +--
1 file changed, 54 insertions(+), 39 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 6861b8a84e..e3f48a9a2e 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -212,6 +212,51 @@ virHostdevManagerGetDefault(void)
return virObjectRef(manager);
}

+/**
+ * virHostdevGetPCIHostDevice:
+ * @hostdev: domain hostdev definition
+ * @pci: returned PCI device
+ *
+ * For given @hostdev which represents a PCI device construct its
+ * virPCIDevice representation and returned it in @pci. If


s/returned/return/


+ * @hostdev does not represent a PCI device then @pci is set to
+ * NULL and 0 is returned.
+ *
+ * Returns: 0 on success (@pci might be NULL though),
+ * -1 otherwise (with error reported).
+ */


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/12] tests: Remove 'kvm' PCI backend from domaincapstest

[Ján Tomko]

On Tue, Aug 20, 2019 at 04:30:22PM +0200, Michal Privoznik wrote:

The KVM assignment was removed in qemu driver in previous commit.
Remove it from domaincapstest too which is hard coding it.

Signed-off-by: Michal Privoznik 
---
tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml| 1 -
tests/domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml | 1 -
tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml| 1 -
tests/domaincapsschemadata/qemu_2.12.0.s390x.xml| 1 -
tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml   | 1 -
tests/domaincapsschemadata/qemu_2.6.0-virt.aarch64.xml  | 1 -
tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml   | 1 -
tests/domaincapsschemadata/qemu_2.6.0.ppc64.xml | 1 -
tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml| 1 -
tests/domaincapsschemadata/qemu_2.7.0.s390x.xml | 1 -
tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml| 1 -
tests/domaincapsschemadata/qemu_2.8.0.s390x.xml | 1 -
tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml| 1 -
tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml| 1 -
tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml| 1 -
tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml| 1 -
tests/domaincapsschemadata/qemu_3.0.0.s390x.xml | 1 -
tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml| 1 -
tests/domaincapsschemadata/qemu_4.0.0.s390x.xml | 1 -
tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml| 1 -
tests/domaincapsschemadata/qemu_4.1.0.x86_64.xml| 1 -
tests/domaincapstest.c  | 1 -
22 files changed, 22 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/12] qemu: Drop KVM assignment

[Ján Tomko]

On Tue, Aug 20, 2019 at 04:30:21PM +0200, Michal Privoznik wrote:

KVM style of PCI devices assignment was dropped in kernel in
favor of vfio pci (see kernel commit v4.12-rc1~68^2~65). Since
vfio is around for quite some time now and is far superior
discourage people in using KVM style.

Ideally, I'd make QEMU_CAPS_VFIO_PCI implicitly assumed but turns
out qemu-3.0.0 doesn't support vfio-pci device for RISC-V.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_capabilities.c |  6 -
src/qemu/qemu_command.c  | 26 ++---
src/qemu/qemu_command.h  |  1 -
src/qemu/qemu_driver.c   | 14 
src/qemu/qemu_hostdev.c  | 44 +++-
src/qemu/qemu_hostdev.h  |  1 -
src/qemu/qemu_hotplug.c  | 20 ++--
tests/domaincapstest.c   |  3 +--
8 files changed, 12 insertions(+), 103 deletions(-)

diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 7e2dc5a60a..e3983bedb2 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -148,7 +148,6 @@ char *qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
char *qemuBuildPCIHostdevDevStr(const virDomainDef *def,
virDomainHostdevDefPtr dev,
unsigned int bootIndex,
-const char *configfd,
virQEMUCapsPtr qemuCaps);

char *qemuBuildRNGDevStr(const virDomainDef *def,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 11f97dbc65..eb373c14d7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13517,7 +13517,6 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
int ret = -1;
virNodeDeviceDefPtr def = NULL;
char *xml = NULL;
-bool legacy = qemuHostdevHostSupportsPassthroughLegacy();
bool vfio = qemuHostdevHostSupportsPassthroughVFIO();
virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;

@@ -13544,8 +13543,6 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
if (!driverName) {
if (vfio) {
driverName = "vfio";
-} else if (legacy) {
-driverName = "kvm";
} else {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
   _("neither VFIO nor KVM device assignment is "


This whole condition can be just:
if (!driverName)
 driverName = "vfio";

now that we would not be able to pick a different one.


@@ -13563,13 +13560,10 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
}
virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO);
} else if (STREQ(driverName, "kvm")) {
-if (!legacy) {
-virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
-   _("KVM device assignment is currently not "
- "supported on this system"));
-goto cleanup;
-}
-virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_KVM);
+virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+   _("KVM device assignment is currently not "
+ "supported on this system"));


s/currently not/no longer/
d/on this system/




+goto cleanup;
} else {
virReportError(VIR_ERR_INVALID_ARG,
   _("unknown driver name '%s'"), driverName);


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] Fwd: libvirtd failing on MacOS in setgroups

[Marcus Furlong]

Resend to libvir-list in case that is more appropriate:


Hi,

I get the following error when running libvirtd on MacOS as root:

2019-07-11 00:12:33.673+: 123145573953536: error :
qemuProcessQMPLaunch:8501 : internal error: Failed to start QEMU
binary /usr/local/bin/qemu-system-x86_64 for probing: libvirt:  error
: cannot set supplemental groups: Invalid argument

As a result `virsh capabilities` as root returns nothing:

$ sudo virsh capabilities | grep qemu
  +0:+0

whereas running as a regular user works fine:

$ virsh capabilites | grep qemu
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  

and by extension, running VMs as regular user works fine via
qemu:///session , but qemu:///system does not work.

It seems like setgroups is failing:

https://github.com/libvirt/libvirt/blob/v5.5.0/src/util/virutil.c#L1045-L1051

Is this the expected behaviour?

Full output below:

$ sudo libvirtd
2019-07-11 00:12:33.379+: 123145573953536: info : libvirt version: 5.5.0
2019-07-11 00:12:33.379+: 123145573953536: warning :
virProcessGetStartTime:1070 : Process start time of pid 49746 not
available on this platform
2019-07-11 00:12:33.379+: 123145573953536: error :
virSysinfoReadDMI:1172 : internal error: Failed to find path for
dmidecode binary
2019-07-11 00:12:33.380+: 123145573953536: error :
virFileFindHugeTLBFS:3734 : this function is not supported by the
connection driver: virFileFindHugeTLBFS
2019-07-11 00:12:33.382+: 123145573953536: warning :
virQEMUCapsInit:919 : Failed to get host CPU cache info
2019-07-11 00:12:33.401+: 123145573953536: error :
virHostCPUGetTscInfo:1405 : Probing TSC is not supported on this
platform: Function not implemented
2019-07-11 00:12:33.401+: 123145573953536: error : virExec:521 :
Cannot find 'pm-is-supported' in path: No such file or directory
2019-07-11 00:12:33.401+: 123145573953536: warning :
virQEMUCapsInit:926 : Failed to get host power management capabilities
2019-07-11 00:12:33.401+: 123145573953536: error :
virNumaGetPages:988 : Operation not supported: page info is not
supported on this platform
2019-07-11 00:12:33.401+: 123145573953536: warning :
virQEMUCapsInit:933 : Failed to get pages info
2019-07-11 00:12:33.407+: 123145573953536: error :
qemuProcessQMPLaunch:8501 : internal error: Failed to start QEMU
binary /usr/local/bin/qemu-system-alpha for probing: libvirt:  error :
cannot set supplemental groups: Invalid argument
2019-07-11 00:12:33.407+: 123145573953536: warning :
virQEMUCapsLogProbeFailure:4578 : Failed to probe capabilities for
/usr/local/bin/qemu-system-alpha: internal error: Failed to start QEMU
binary /usr/local/bin/qemu-system-alpha for probing: libvirt:  error :
cannot set supplemental groups: Invalid argument
2019-07-11 00:12:33.413+: 123145573953536: error :
qemuProcessQMPLaunch:8501 : internal error: Failed to start QEMU
binary /usr/local/bin/qemu-system-arm for probing: libvirt:  error :
cannot set supplemental groups: Invalid argument
2019-07-11 00:12:33.413+: 123145573953536: warning :
virQEMUCapsLogProbeFailure:4578 : Failed to probe capabilities for
/usr/local/bin/qemu-system-arm: internal error: Failed to start QEMU
binary /usr/local/bin/qemu-system-arm for probing: libvirt:  error :
cannot set supplemental groups: Invalid argument
2019-07-11 00:12:33.419+: 123145573953536: error :
qemuProcessQMPLaunch:8501 : internal error: Failed to start QEMU
binary /usr/local/bin/qemu-system-arm for probing: libvirt:  error :
cannot set supplemental groups: Invalid argument
2019-07-11 00:12:33.419+: 123145573953536: warning :
virQEMUCapsLogProbeFailure:4578 : Failed to probe capabilities for
/usr/local/bin/qemu-system-arm: internal error: Failed to start QEMU
binary /usr/local/bin/qemu-system-arm for probing: libvirt:  error :
cannot set supplemental groups: Invalid argument
2019-07-11 00:12:33.424+: 123145573953536: error :
qemuProcessQMPLaunch:8501 : internal error: Failed to start QEMU
binary /usr/local/bin/qemu-system-aarch64 for probing: libvirt:  error
: cannot set supplemental groups: Invalid argument
2019-07-11 00:12:33.424+: 123145573953536: warning :
virQEMUCapsLogProbeFailure:4578 : Failed to probe capabilities for
/usr/local/bin/qemu-system-aarch64: internal error: Failed to start
QEMU binary /usr/local/bin/qemu-system-aarch64 for probing: libvirt:
error : cannot set supplemental groups: Invalid argument
2019-07-11 00:12:33.431+: 123145573953536: error :
qemuProcessQMPLaunch:8501 : internal error: Failed to start QEMU
binary /usr/local/bin/qemu-system-cris for probing: libvirt:  error :
cannot set supplemental groups: Invalid argument
2019-07-11 00:12:33.431+: 123145573953536: warning :
virQEMUCapsLogProbeFailure:4578 : Failed to probe capabilities for
/usr/local/bin/qemu-system-cris: internal error: Failed to