date:20150916

[libvirt] [PATCH v2 5/7] qemu: s/virDomainDiskDiffersSourceOnly/qemuDomainDiskChangeSupported/

2015-09-16 Thread Michal Privoznik

I always felt like this function is qemu specific rather than
libvirt-wide. Other drivers may act differently on virDomainDef
change and in fact may require talking to underlying hypervisor
even if something else's than disk->src has changed.  I know that
the function is still incomplete, but lets break that into two
commits that are easier to review. This one is pure code
movement.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c   | 127 ---
 src/conf/domain_conf.h   |   2 -
 src/libvirt_private.syms |   1 -
 src/qemu/qemu_domain.c   | 127 +++
 src/qemu/qemu_domain.h   |   3 ++
 src/qemu/qemu_driver.c   |   2 +-
 6 files changed, 131 insertions(+), 131 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6df1618..a3b3ccb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5838,133 +5838,6 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def,
 }
 
 
-/*
- * Makes sure the @disk differs from @orig_disk only by the source
- * path and nothing else.  Fields that are being checked and the
- * information whether they are nullable (may not be specified) or is
- * taken from the virDomainDiskDefFormat() code.
- */
-bool
-virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk,
-   virDomainDiskDefPtr orig_disk)
-{
-#define CHECK_EQ(field, field_name, nullable)   \
-do {\
-if (nullable && !disk->field)   \
-break;  \
-if (disk->field != orig_disk->field) {  \
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED,   \
-   _("cannot modify field '%s' of the disk"),   \
-   field_name); \
-return false;   \
-}   \
-} while (0)
-
-CHECK_EQ(device, "device", false);
-CHECK_EQ(cachemode, "cache", true);
-CHECK_EQ(error_policy, "error_policy", true);
-CHECK_EQ(rerror_policy, "rerror_policy", true);
-CHECK_EQ(iomode, "io", true);
-CHECK_EQ(ioeventfd, "ioeventfd", true);
-CHECK_EQ(event_idx, "event_idx", true);
-CHECK_EQ(copy_on_read, "copy_on_read", true);
-CHECK_EQ(discard, "discard", true);
-CHECK_EQ(iothread, "iothread", true);
-
-if (disk->geometry.cylinders &&
-disk->geometry.heads &&
-disk->geometry.sectors) {
-CHECK_EQ(geometry.cylinders, "geometry cylinders", false);
-CHECK_EQ(geometry.heads, "geometry heads", false);
-CHECK_EQ(geometry.sectors, "geometry sectors", false);
-CHECK_EQ(geometry.trans, "BIOS-translation-modus", true);
-}
-
-CHECK_EQ(blockio.logical_block_size,
- "blockio logical_block_size", false);
-CHECK_EQ(blockio.physical_block_size,
- "blockio physical_block_size", false);
-
-if (disk->bus == VIR_DOMAIN_DISK_BUS_USB)
-CHECK_EQ(removable, "removable", true);
-
-CHECK_EQ(blkdeviotune.total_bytes_sec,
- "blkdeviotune total_bytes_sec",
- true);
-CHECK_EQ(blkdeviotune.read_bytes_sec,
- "blkdeviotune read_bytes_sec",
- true);
-CHECK_EQ(blkdeviotune.write_bytes_sec,
- "blkdeviotune write_bytes_sec",
- true);
-CHECK_EQ(blkdeviotune.total_iops_sec,
- "blkdeviotune total_iops_sec",
- true);
-CHECK_EQ(blkdeviotune.read_iops_sec,
- "blkdeviotune read_iops_sec",
- true);
-CHECK_EQ(blkdeviotune.write_iops_sec,
- "blkdeviotune write_iops_sec",
- true);
-CHECK_EQ(blkdeviotune.total_bytes_sec_max,
- "blkdeviotune total_bytes_sec_max",
- true);
-CHECK_EQ(blkdeviotune.read_bytes_sec_max,
- "blkdeviotune read_bytes_sec_max",
- true);
-CHECK_EQ(blkdeviotune.write_bytes_sec_max,
- "blkdeviotune write_bytes_sec_max",
- true);
-CHECK_EQ(blkdeviotune.total_iops_sec_max,
- "blkdeviotune total_iops_sec_max",
- true);
-CHECK_EQ(blkdeviotune.read_iops_sec_max,
- "blkdeviotune read_iops_sec_max",
- true);
-CHECK_EQ(blkdeviotune.write_iops_sec_max,
- "blkdeviotune write_iops_sec_max",
- true);
-CHECK_EQ(blkdeviotune.size_iops_sec,
- "blkdeviotune size_iops_sec",
- true);
-
-CHECK_EQ(transient, "transient", true);
-
-if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
-   _("cannot modify field

[libvirt] [PATCH v2 0/7] Allow startupPolicy change on update-device

2015-09-16 Thread Michal Privoznik

So, as Peter pointed out, we may want to updated startupPolicy for
live domains too. That's what patches 2/7-7/7 do.

Michal Privoznik (7):
  qemuDomainUpdateDeviceConfig: Allow startupPolicy update, yet again
  qemu: s/qemuDomainChangeDiskMediaLive/qemuDomainChangeDiskLive/
  qemu_domain: Introduce qemuDomainDiskSourceDiffers
  qemuDomainChangeDiskLive: rework slightly
  qemu: s/virDomainDiskDiffersSourceOnly/qemuDomainDiskChangeSupported/
  qemuDomainDiskChangeSupported: Fill in missing checks
  qemuDomainChangeDiskLive: Allow startupPolicy change

 src/conf/domain_conf.c   | 127 --
 src/conf/domain_conf.h   |   2 -
 src/libvirt_private.syms |   1 -
 src/qemu/qemu_domain.c   | 176 +++
 src/qemu/qemu_domain.h   |   7 ++
 src/qemu/qemu_driver.c   |  80 -
 6 files changed, 232 insertions(+), 161 deletions(-)

-- 
2.4.6

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

[libvirt] [PATCH v2 2/7] qemu: s/qemuDomainChangeDiskMediaLive/qemuDomainChangeDiskLive/

2015-09-16 Thread Michal Privoznik

While we currently only allow changing a media in a disk, this is
going to change in a while, so the function name would be
invalid. Moreover, the old name does not match the pattern laid
out by other update functions.

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

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fadead5..8946b20 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7914,11 +7914,11 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
 }
 
 static int
-qemuDomainChangeDiskMediaLive(virConnectPtr conn,
-  virDomainObjPtr vm,
-  virDomainDeviceDefPtr dev,
-  virQEMUDriverPtr driver,
-  bool force)
+qemuDomainChangeDiskLive(virConnectPtr conn,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev,
+ virQEMUDriverPtr driver,
+ bool force)
 {
 virDomainDiskDefPtr disk = dev->data.disk;
 virDomainDiskDefPtr orig_disk = NULL;
@@ -7983,7 +7983,7 @@ qemuDomainUpdateDeviceLive(virConnectPtr conn,
 switch ((virDomainDeviceType) dev->type) {
 case VIR_DOMAIN_DEVICE_DISK:
 qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk, -1);
-ret = qemuDomainChangeDiskMediaLive(conn, vm, dev, driver, force);
+ret = qemuDomainChangeDiskLive(conn, vm, dev, driver, force);
 break;
 case VIR_DOMAIN_DEVICE_GRAPHICS:
 ret = qemuDomainChangeGraphics(driver, vm, dev->data.graphics);
-- 
2.4.6

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

[libvirt] [PATCH v2 7/7] qemuDomainChangeDiskLive: Allow startupPolicy change

2015-09-16 Thread Michal Privoznik

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c |  1 -
 src/qemu/qemu_driver.c | 29 -
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ed92d8a..fb8ab30 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3157,7 +3157,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
 CHECK_EQ(event_idx, "event_idx", true);
 CHECK_EQ(copy_on_read, "copy_on_read", true);
 CHECK_EQ(snapshot, "snapshot", true);
-CHECK_EQ(startupPolicy, "startupPolicy", true);
 CHECK_EQ(transient, "transient", true);
 CHECK_EQ(info.bootIndex, "boot order", true);
 CHECK_EQ(rawio, "rawio", true);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1a189cc..d74255b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7922,6 +7922,7 @@ qemuDomainChangeDiskLive(virConnectPtr conn,
 {
 virDomainDiskDefPtr disk = dev->data.disk;
 virDomainDiskDefPtr orig_disk = NULL;
+int startupPolicy;
 int ret = -1;
 
 if (virStorageTranslateDiskSourcePool(conn, disk) < 0)
@@ -7939,23 +7940,29 @@ qemuDomainChangeDiskLive(virConnectPtr conn,
 goto cleanup;
 }
 
+startupPolicy = orig_disk->startupPolicy;
+
 switch ((virDomainDiskDevice) disk->device) {
 case VIR_DOMAIN_DISK_DEVICE_CDROM:
 case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
 if (!qemuDomainDiskChangeSupported(disk, orig_disk))
 goto cleanup;
 
-/* Add the new disk src into shared disk hash table */
-if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0)
-goto cleanup;
+orig_disk->startupPolicy = dev->data.disk->startupPolicy;
 
-if (qemuDomainChangeEjectableMedia(driver, conn, vm,
-   orig_disk, disk->src, force) < 0) {
-ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name));
-goto cleanup;
+if (qemuDomainDiskSourceDiffers(conn, disk, orig_disk)) {
+/* Add the new disk src into shared disk hash table */
+if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0)
+goto cleanup;
+
+if (qemuDomainChangeEjectableMedia(driver, conn, vm,
+   orig_disk, dev->data.disk->src, 
force) < 0) {
+ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk, 
vm->def->name));
+goto rollback;
+}
+
+dev->data.disk->src = NULL;
 }
-
-disk->src = NULL;
 break;
 
 case VIR_DOMAIN_DISK_DEVICE_DISK:
@@ -7974,6 +7981,10 @@ qemuDomainChangeDiskLive(virConnectPtr conn,
 ret = 0;
  cleanup:
 return ret;
+
+ rollback:
+orig_disk->startupPolicy = startupPolicy;
+goto cleanup;
 }
 
 static int
-- 
2.4.6

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

[libvirt] [PATCH v2 4/7] qemuDomainChangeDiskLive: rework slightly

2015-09-16 Thread Michal Privoznik

Firstly, our coding guidelines suggest using 'cleanup' label
instead of 'end'. Then, @ret should be set to value representing
success as the last statement before the 'cleanup' label.
And while I am at this function, lets enumerate all the possible
enum items (virDomainDiskDevice) and avoid using 'default' in
switch(). Pooh. Also, nothing bad happens if we look up the disk
to change in the domain upfront. In fact, it's going to be
helpful later when we want to keep some old values for performing
a rollback.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_driver.c | 48 +++-
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8946b20..6a8e863 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7925,48 +7925,54 @@ qemuDomainChangeDiskLive(virConnectPtr conn,
 int ret = -1;
 
 if (virStorageTranslateDiskSourcePool(conn, disk) < 0)
-goto end;
+goto cleanup;
 
 if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
-goto end;
+goto cleanup;
 
-switch (disk->device) {
+if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def,
+   disk->bus, disk->dst))) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("No device with bus '%s' and target '%s'"),
+   virDomainDiskBusTypeToString(disk->bus),
+   disk->dst);
+goto cleanup;
+}
+
+switch ((virDomainDiskDevice) disk->device) {
 case VIR_DOMAIN_DISK_DEVICE_CDROM:
 case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
-if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def,
-   disk->bus, disk->dst))) 
{
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("No device with bus '%s' and target '%s'"),
-   virDomainDiskBusTypeToString(disk->bus),
-   disk->dst);
-goto end;
-}
-
 if (!virDomainDiskDiffersSourceOnly(disk, orig_disk))
-goto end;
+goto cleanup;
 
 /* Add the new disk src into shared disk hash table */
 if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0)
-goto end;
+goto cleanup;
 
 if (qemuDomainChangeEjectableMedia(driver, conn, vm,
-   orig_disk, dev->data.disk->src, 
force) < 0) {
-ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk, 
vm->def->name));
-goto end;
+   orig_disk, disk->src, force) < 0) {
+ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name));
+goto cleanup;
 }
 
-dev->data.disk->src = NULL;
-ret = 0;
+disk->src = NULL;
 break;
 
-default:
+case VIR_DOMAIN_DISK_DEVICE_DISK:
+case VIR_DOMAIN_DISK_DEVICE_LUN:
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk bus '%s' cannot be updated."),
virDomainDiskBusTypeToString(disk->bus));
+goto cleanup;
+break;
+
+case VIR_DOMAIN_DISK_DEVICE_LAST:
+/* nada */
 break;
 }
 
- end:
+ret = 0;
+ cleanup:
 return ret;
 }
 
-- 
2.4.6

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

[libvirt] [PATCH v2 6/7] qemuDomainDiskChangeSupported: Fill in missing checks

2015-09-16 Thread Michal Privoznik

So far this function was not kept in sync with changing
virDomainDiskDef. Fill in all the missing checks and reorganize
their order so it's easier to track which items are not being
checked for.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 45 +++--
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 21f5002..ed92d8a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3057,15 +3057,15 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
 } while (0)
 
 CHECK_EQ(device, "device", false);
-CHECK_EQ(cachemode, "cache", true);
-CHECK_EQ(error_policy, "error_policy", true);
-CHECK_EQ(rerror_policy, "rerror_policy", true);
-CHECK_EQ(iomode, "io", true);
-CHECK_EQ(ioeventfd, "ioeventfd", true);
-CHECK_EQ(event_idx, "event_idx", true);
-CHECK_EQ(copy_on_read, "copy_on_read", true);
-CHECK_EQ(discard, "discard", true);
-CHECK_EQ(iothread, "iothread", true);
+CHECK_EQ(bus, "bus", false);
+if (STRNEQ(disk->dst, orig_disk->dst)) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("cannot modify field '%s' of the disk"),
+   "bus");
+return false;
+}
+CHECK_EQ(tray_status, "tray", true);
+CHECK_EQ(removable, "removable", true);
 
 if (disk->geometry.cylinders &&
 disk->geometry.heads &&
@@ -3081,9 +3081,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
 CHECK_EQ(blockio.physical_block_size,
  "blockio physical_block_size", false);
 
-if (disk->bus == VIR_DOMAIN_DISK_BUS_USB)
-CHECK_EQ(removable, "removable", true);
-
 CHECK_EQ(blkdeviotune.total_bytes_sec,
  "blkdeviotune total_bytes_sec",
  true);
@@ -3124,8 +3121,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
  "blkdeviotune size_iops_sec",
  true);
 
-CHECK_EQ(transient, "transient", true);
-
 if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("cannot modify field '%s' of the disk"),
@@ -3154,7 +3149,29 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
 return false;
 }
 
+CHECK_EQ(cachemode, "cache", true);
+CHECK_EQ(error_policy, "error_policy", true);
+CHECK_EQ(rerror_policy, "rerror_policy", true);
+CHECK_EQ(iomode, "io", true);
+CHECK_EQ(ioeventfd, "ioeventfd", true);
+CHECK_EQ(event_idx, "event_idx", true);
+CHECK_EQ(copy_on_read, "copy_on_read", true);
+CHECK_EQ(snapshot, "snapshot", true);
+CHECK_EQ(startupPolicy, "startupPolicy", true);
+CHECK_EQ(transient, "transient", true);
 CHECK_EQ(info.bootIndex, "boot order", true);
+CHECK_EQ(rawio, "rawio", true);
+CHECK_EQ(sgio, "sgio", true);
+CHECK_EQ(discard, "discard", true);
+CHECK_EQ(iothread, "iothread", true);
+
+if (disk->domain_name &&
+STRNEQ_NULLABLE(disk->domain_name, orig_disk->domain_name)) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("cannot modify field '%s' of the disk"),
+   "backenddomain");
+return false;
+}
 
 #undef CHECK_EQ
 
-- 
2.4.6

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

[libvirt] [PATCH v2 1/7] qemuDomainUpdateDeviceConfig: Allow startupPolicy update, yet again

2015-09-16 Thread Michal Privoznik

https://bugzilla.redhat.com/show_bug.cgi?id=1159219

So, in 11e058ca589808bd I've tried to make UpdateDevice update
startupPolicy too. And it worked well until somebody came around
and pushed d0dc6c036914da which accidentally removed my
contribution. Redo my commit.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fcf86b6..fadead5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8356,6 +8356,7 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps,
  * We allow updating src/type//driverType/cachemode/
  */
 orig->cachemode = disk->cachemode;
+orig->startupPolicy = disk->startupPolicy;
 
 virStorageSourceFree(orig->src);
 orig->src = disk->src;
-- 
2.4.6

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

[libvirt] [PATCH v2 3/7] qemu_domain: Introduce qemuDomainDiskSourceDiffers

2015-09-16 Thread Michal Privoznik

This new private API should return true iff sources of two disks
differs in sense that qemu should be instructed to change the
disk backend. For instance, ejecting a CDROM is such case, or
pointing disk into a different ISO location, and so on.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 33 +
 src/qemu/qemu_domain.h |  4 
 2 files changed, 37 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c8b0ccd..60bd560 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3002,6 +3002,39 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
 
 
 bool
+qemuDomainDiskSourceDiffers(virConnectPtr conn,
+virDomainDiskDefPtr disk,
+virDomainDiskDefPtr origDisk)
+{
+char *diskSrc = NULL, *origDiskSrc = NULL;
+bool diskEmpty, origDiskEmpty;
+bool ret = true;
+
+diskEmpty = virStorageSourceIsEmpty(disk->src);
+origDiskEmpty = virStorageSourceIsEmpty(origDisk->src);
+
+if (diskEmpty && origDiskEmpty)
+return false;
+
+if (diskEmpty ^ origDiskEmpty)
+return true;
+
+if (qemuGetDriveSourceString(disk->src, conn, ) < 0 ||
+qemuGetDriveSourceString(origDisk->src, conn, ) < 0)
+goto cleanup;
+
+/* So far in qemu disk sources are considered different
+ * if either path to disk or its format changes. */
+ret = virDomainDiskGetFormat(disk) != virDomainDiskGetFormat(origDisk) ||
+  STRNEQ_NULLABLE(diskSrc, origDiskSrc);
+ cleanup:
+VIR_FREE(diskSrc);
+VIR_FREE(origDiskSrc);
+return ret;
+}
+
+
+bool
 qemuDomainDiskBlockJobIsActive(virDomainDiskDefPtr disk)
 {
 qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 91eaea1..0b9fd1c 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -411,6 +411,10 @@ int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
  bool force_probe,
  bool report_broken);
 
+bool qemuDomainDiskSourceDiffers(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
+ virDomainDiskDefPtr origDisk);
+
 int qemuDomainStorageFileInit(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
   virStorageSourcePtr src);
-- 
2.4.6

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

Re: [libvirt] VMware: Map vpx:// to dcPath

2015-09-16 Thread Richard W.M. Jones

On Fri, Sep 11, 2015 at 03:55:03PM +0200, Matthias Bolte wrote:
> 2015-09-07 22:04 GMT+02:00 Richard W.M. Jones :
> > On Mon, Sep 07, 2015 at 02:29:22PM +0200, Matthias Bolte wrote:
> >> I think the datacenter path could be exposed
> >> as part of the domain XML as
> >> /path/to/dc similar to
> >> the way  works. But it would be ignored on parsing.
> >>
> >> Would that work for you? If yes, I can propose a patch that does this.
> >
> > Absolutely this would be brilliant.
> 
> Okay, here's patch that does this. It's only tested using the test
> suite, as I don't have an ESX setup at hand at the moment. Do you have
> the possibility to test this properly?

> From 489e2d5dd29dd4b11716897ca52b14fec141 Mon Sep 17 00:00:00 2001
> From: Matthias Bolte 
> Date: Fri, 11 Sep 2015 12:00:47 +0200
> Subject: [PATCH] vmx: Expose datacenter path in domain XML

If you're happy with this patch, I'd like to push it to the libvirt
repo.  I didn't see any later version on the list.  Let me know if
this is the final version.

Also I have opened a BZ for the problem so the fix can be included in
RHEL 7.3:

  https://bugzilla.redhat.com/show_bug.cgi?id=1263574

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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

Re: [libvirt] [PATCH v3] qemu: Fix using guest architecture as lookup key

2015-09-16 Thread Daniel P. Berrange

On Wed, Sep 16, 2015 at 09:13:09AM +0200, Andrea Bolognani wrote:
> When looking for a QEMU binary suitable for running ppc64le guests
> we have to take into account the fact that we use the QEMU target
> as key for the hash, so direct comparison is not good enough.
> 
> Factor out the logic from virQEMUCapsFindBinaryForArch() to a new
> virQEMUCapsFindTarget() function and use that both when looking
> for QEMU binaries available on the system and when looking up
> QEMU capabilities later.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1260753
> ---
>  src/qemu/qemu_capabilities.c | 105 
> +--
>  1 file changed, 71 insertions(+), 34 deletions(-)

ACK


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

[libvirt] [PATCH v3] qemu: Fix using guest architecture as lookup key

2015-09-16 Thread Andrea Bolognani

When looking for a QEMU binary suitable for running ppc64le guests
we have to take into account the fact that we use the QEMU target
as key for the hash, so direct comparison is not good enough.

Factor out the logic from virQEMUCapsFindBinaryForArch() to a new
virQEMUCapsFindTarget() function and use that both when looking
for QEMU binaries available on the system and when looking up
QEMU capabilities later.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1260753
---
 src/qemu/qemu_capabilities.c | 105 +--
 1 file changed, 71 insertions(+), 34 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4ad1bdb..01fbd65 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -386,6 +386,28 @@ static const char *virQEMUCapsArchToString(virArch arch)
 return virArchToString(arch);
 }
 
+/* Given a host and guest architectures, find a suitable QEMU target.
+ *
+ * This is meant to be used as a second attempt if qemu-system-$guestarch
+ * can't be found, eg. on a x86_64 host you want to use qemu-system-i386,
+ * if available, instead of qemu-system-x86_64 to run i686 guests */
+static virArch
+virQEMUCapsFindTarget(virArch hostarch,
+  virArch guestarch)
+{
+/* Both ppc64 and ppc64le guests can use the ppc64 target */
+if (ARCH_IS_PPC64(guestarch))
+guestarch = VIR_ARCH_PPC64;
+
+/* armv7l guests on aarch64 hosts can use the aarch64 target
+ * i686 guests on x86_64 hosts can use the x86_64 target */
+if ((guestarch == VIR_ARCH_ARMV7L && hostarch == VIR_ARCH_AARCH64) ||
+(guestarch == VIR_ARCH_I686 && hostarch == VIR_ARCH_X86_64)) {
+return hostarch;
+}
+
+return guestarch;
+}
 
 static virCommandPtr
 virQEMUCapsProbeCommand(const char *qemu,
@@ -690,51 +712,55 @@ virQEMUCapsProbeCPUModels(virQEMUCapsPtr qemuCaps, uid_t 
runUid, gid_t runGid)
 return ret;
 }
 
-
 static char *
-virQEMUCapsFindBinaryForArch(virArch hostarch,
- virArch guestarch)
+virQEMUCapsFindBinary(const char *format,
+  const char *archstr)
 {
-char *ret;
-const char *archstr;
-char *binary;
-
-if (ARCH_IS_PPC64(guestarch))
-archstr = virQEMUCapsArchToString(VIR_ARCH_PPC64);
-else
-archstr = virQEMUCapsArchToString(guestarch);
+char *ret = NULL;
+char *binary = NULL;
 
-if (virAsprintf(, "qemu-system-%s", archstr) < 0)
-return NULL;
+if (virAsprintf(, format, archstr) < 0)
+goto out;
 
 ret = virFindFileInPath(binary);
 VIR_FREE(binary);
-if (ret && !virFileIsExecutable(ret))
-VIR_FREE(ret);
+if (ret && virFileIsExecutable(ret))
+goto out;
 
-if (guestarch == VIR_ARCH_ARMV7L &&
-!ret &&
-hostarch == VIR_ARCH_AARCH64) {
-ret = virFindFileInPath("qemu-system-aarch64");
-if (ret && !virFileIsExecutable(ret))
-VIR_FREE(ret);
-}
+VIR_FREE(ret);
 
-if (guestarch == VIR_ARCH_I686 &&
-!ret &&
-hostarch == VIR_ARCH_X86_64) {
-ret = virFindFileInPath("qemu-system-x86_64");
-if (ret && !virFileIsExecutable(ret))
-VIR_FREE(ret);
+ out:
+return ret;
+}
+
+static char *
+virQEMUCapsFindBinaryForArch(virArch hostarch,
+ virArch guestarch)
+{
+char *ret = NULL;
+const char *archstr;
+virArch target;
+
+/* First attempt: try the guest architecture as it is */
+archstr = virQEMUCapsArchToString(guestarch);
+if ((ret = virQEMUCapsFindBinary("qemu-system-%s", archstr)) != NULL)
+goto out;
+
+/* Second attempt: try looking up by target instead */
+target = virQEMUCapsFindTarget(hostarch, guestarch);
+if (target != guestarch) {
+archstr = virQEMUCapsArchToString(target);
+if ((ret = virQEMUCapsFindBinary("qemu-system-%s", archstr)) != NULL)
+goto out;
 }
 
-if (guestarch == VIR_ARCH_I686 &&
-!ret) {
-ret = virFindFileInPath("qemu");
-if (ret && !virFileIsExecutable(ret))
-VIR_FREE(ret);
+/* Third attempt, i686 only: try 'qemu' */
+if (guestarch == VIR_ARCH_I686) {
+if ((ret = virQEMUCapsFindBinary("%s", "qemu")) != NULL)
+goto out;
 }
 
+ out:
 return ret;
 }
 
@@ -3789,14 +3815,25 @@ virQEMUCapsCacheLookupByArch(virQEMUCapsCachePtr cache,
  virArch arch)
 {
 virQEMUCapsPtr ret = NULL;
+virArch target;
 struct virQEMUCapsSearchData data = { .arch = arch };
 
 virMutexLock(>lock);
 ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, );
-VIR_DEBUG("Returning caps %p for arch %s", ret, virArchToString(arch));
+if (!ret) {
+/* If the first attempt at finding capabilities has failed, try
+ * again using the QEMU target as lookup key instead */
+target =

Re: [libvirt] [PATCH v2] qemu: Fix using guest architecture as lookup key

2015-09-16 Thread Andrea Bolognani

On Tue, 2015-09-15 at 17:34 +0100, Daniel P. Berrange wrote:
> > +/* Second attempt: use some arch-specific rules */
> > +archstr =
> > virQEMUCapsArchToString(virQEMUCapsFindTarget(hostarch,
> > +   
> >  guestarch));
> 
> Nitpick, we could avoid the second search if virQEMUCapsFindTarget()
> returns a value that == the original guestarch.

> > +if (!ret) {
> > +/* If the first attempt at findind capabilities has
> > failed, try
> > + * again using the QEMU target as lookup key instead */
> > +data.arch = virQEMUCapsFindTarget(virArchFromHost(),
> > data.arch);
> > +ret = virHashSearch(cache->binaries,
> > virQEMUCapsCompareArch, );
> > +}
> 
> Here too, skip the hash search if data.arch == data.arch from before.

Good idea! After all, the definition of insanity is looking up
using the same key twice and expecting a result the second time
around :)

I've just posted a new version[1] that implements your suggestion.

Cheers.


[1] https://www.redhat.com/archives/libvir-list/2015-September/msg00489
.html
-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH v3] qemu: Fix using guest architecture as lookup key

2015-09-16 Thread Andrea Bolognani

On Wed, 2015-09-16 at 09:22 +0100, Daniel P. Berrange wrote:
> On Wed, Sep 16, 2015 at 09:13:09AM +0200, Andrea Bolognani wrote:
> > When looking for a QEMU binary suitable for running ppc64le guests
> > we have to take into account the fact that we use the QEMU target
> > as key for the hash, so direct comparison is not good enough.
> > 
> > Factor out the logic from virQEMUCapsFindBinaryForArch() to a new
> > virQEMUCapsFindTarget() function and use that both when looking
> > for QEMU binaries available on the system and when looking up
> > QEMU capabilities later.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1260753
> > ---
> >  src/qemu/qemu_capabilities.c | 105 +--
> > 
> >  1 file changed, 71 insertions(+), 34 deletions(-)
> 
> ACK

Pushed, thanks :)

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH] qemu: Check for existence of auto-generated socket path before removing

2015-09-16 Thread John Ferlan



On 09/16/2015 09:05 AM, Michal Privoznik wrote:
> On 15.09.2015 23:03, John Ferlan wrote:
>> Commit id 'f1f68ca33' added code to remove the directory paths for
>> auto-generated sockets, but that code could be called before the
>> paths were created resulting in generating error messages from
>> virFileDeleteTree indicating that the file doesn't exist. So just
>> add a check before attemping the directory delete for existence.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_process.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index ce2c70c..b55eb52 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -5269,13 +5269,13 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>>  
>>  ignore_value(virAsprintf(, "%s/domain-%s",
>>   cfg->libDir, vm->def->name));
>> -if (tmppath)
>> +if (tmppath && virFileExists(tmppath))
>>  virFileDeleteTree(tmppath);
>>  VIR_FREE(tmppath);
>>  
>>  ignore_value(virAsprintf(, "%s/domain-%s",
>>   cfg->channelTargetDir, vm->def->name));
>> -if (tmppath)
>> +if (tmppath && virFileExists(tmppath))
>>  virFileDeleteTree(tmppath);
>>  VIR_FREE(tmppath);
>>  
>>
> 
> Okay, so this is needed so that we don't taint logs with useless error
> messages. ACK to the idea. But what what about moving virFileExists into
> virFileDeleteTree? The reason for that would be that I like functions
> which deals with dummy arguments. In this case it would be:
> 
> virFileDeleteTree("/some/nonexistent/path");
> virFileDeleteTree(NULL);
> 
> But if you dislike the idea, ACK to this patch then. There's nothing
> wrong with it.
> 
Correct with your assertion - I saw the messages while debugging
something else (error validating something during qemu_command creation):

2015-09-15 14:18:40.197+: 28838: error : virFileDeleteTree:945 :
Cannot open dir '/var/lib/libvirt/qemu/domain-test': No such file or
directory
2015-09-15 14:18:40.197+: 28838: error : virFileDeleteTree:945 :
Cannot open dir '/var/lib/libvirt/qemu/channel/target/domain-test': No
such file or directory

and that was after the error I was expecting...  Since it was 'new' I
knew it was a recent change...

Anyway, I took the chisel approach rather than the sledgehammer, but I
see your point. Up to this point it seems the function was primarily
called from *test.c modules and those that weren't sure if 'path' could
be NULL prior to call would always check. I'll post a different patch
with the check in virFileDeleteTree() and of course remove the NULL
checks in the *test.c modules.

John

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

[libvirt] [PATCH v2] virfile: Check for existence of dir in virFileDeleteTree

2015-09-16 Thread John Ferlan

Commit id 'f1f68ca33' added code to remove the directory paths for
auto-generated sockets, but that code could be called before the
paths were created resulting in generating error messages from
virFileDeleteTree indicating that the file doesn't exist.

Rather than "enforce" all callers to make the non-NULL and existence
checks, modify the virFileDeleteTree API to silently ignore NULL on
input and non-existent directory trees.

Signed-off-by: John Ferlan 
---

Difference over v1: Add checks in virFileDeleteTree

 src/qemu/qemu_process.c | 6 ++
 src/util/virfile.c  | 8 ++--
 tests/virhostdevtest.c  | 2 +-
 tests/virscsitest.c | 2 +-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ce2c70c..155846e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5269,14 +5269,12 @@ void qemuProcessStop(virQEMUDriverPtr driver,
 
 ignore_value(virAsprintf(, "%s/domain-%s",
  cfg->libDir, vm->def->name));
-if (tmppath)
-virFileDeleteTree(tmppath);
+virFileDeleteTree(tmppath);
 VIR_FREE(tmppath);
 
 ignore_value(virAsprintf(, "%s/domain-%s",
  cfg->channelTargetDir, vm->def->name));
-if (tmppath)
-virFileDeleteTree(tmppath);
+virFileDeleteTree(tmppath);
 VIR_FREE(tmppath);
 
 ignore_value(virDomainChrDefForeach(vm->def,
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 75819d9..fe9f8d4 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -934,13 +934,17 @@ int virFileNBDDeviceAssociate(const char *file,
  */
 int virFileDeleteTree(const char *dir)
 {
-DIR *dh = opendir(dir);
+DIR *dh;
 struct dirent *de;
 char *filepath = NULL;
 int ret = -1;
 int direrr;
 
-if (!dh) {
+/* Silently return 0 if passed NULL or directory doesn't exist */
+if (!dir || !virFileExists(dir))
+return 0;
+
+if (!(dh = opendir(dir))) {
 virReportSystemError(errno, _("Cannot open dir '%s'"),
  dir);
 return -1;
diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c
index 1e93819..065b825 100644
--- a/tests/virhostdevtest.c
+++ b/tests/virhostdevtest.c
@@ -65,7 +65,7 @@ myCleanup(void)
 }
 
 if (mgr) {
-if (mgr->stateDir && !getenv("LIBVIRT_SKIP_CLEANUP"))
+if (!getenv("LIBVIRT_SKIP_CLEANUP"))
 virFileDeleteTree(mgr->stateDir);
 
 virObjectUnref(mgr->activePCIHostdevs);
diff --git a/tests/virscsitest.c b/tests/virscsitest.c
index a86ca28..88286f1 100644
--- a/tests/virscsitest.c
+++ b/tests/virscsitest.c
@@ -241,7 +241,7 @@ mymain(void)
 ret = -1;
 
  cleanup:
-if (tmpdir && getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
+if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
 virFileDeleteTree(tmpdir);
 VIR_FREE(virscsi_prefix);
 return ret;
-- 
2.1.0

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

Re: [libvirt] [PATCH] qemu: Check for existence of auto-generated socket path before removing

2015-09-16 Thread Michal Privoznik

On 15.09.2015 23:03, John Ferlan wrote:
> Commit id 'f1f68ca33' added code to remove the directory paths for
> auto-generated sockets, but that code could be called before the
> paths were created resulting in generating error messages from
> virFileDeleteTree indicating that the file doesn't exist. So just
> add a check before attemping the directory delete for existence.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_process.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index ce2c70c..b55eb52 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5269,13 +5269,13 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>  
>  ignore_value(virAsprintf(, "%s/domain-%s",
>   cfg->libDir, vm->def->name));
> -if (tmppath)
> +if (tmppath && virFileExists(tmppath))
>  virFileDeleteTree(tmppath);
>  VIR_FREE(tmppath);
>  
>  ignore_value(virAsprintf(, "%s/domain-%s",
>   cfg->channelTargetDir, vm->def->name));
> -if (tmppath)
> +if (tmppath && virFileExists(tmppath))
>  virFileDeleteTree(tmppath);
>  VIR_FREE(tmppath);
>  
> 

Okay, so this is needed so that we don't taint logs with useless error
messages. ACK to the idea. But what what about moving virFileExists into
virFileDeleteTree? The reason for that would be that I like functions
which deals with dummy arguments. In this case it would be:

virFileDeleteTree("/some/nonexistent/path");
virFileDeleteTree(NULL);

But if you dislike the idea, ACK to this patch then. There's nothing
wrong with it.

Michal

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

Re: [libvirt] [PATCH v2] virsh: Teach attach-interface to --print-xml

2015-09-16 Thread John Ferlan



On 09/10/2015 04:31 AM, Michal Privoznik wrote:
> We have the same argument to many other commands that produce an
> XML based on what user typed. But unfortunately attach-interface
> was missing it. Maybe nobody had needed it yet. Well, I did
> just now.
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> Diff to v1:
> - Commit message grammar fix
> - Fixed usage of virDomain
> 
>  tools/virsh-domain.c | 28 
>  tools/virsh.pod  |  4 
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index b029b65..ce84930 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -862,6 +862,10 @@ static const vshCmdOptDef opts_attach_interface[] = {
>   .type = VSH_OT_BOOL,
>   .help = N_("affect current domain")
>  },
> +{.name = "print-xml",
> + .type = VSH_OT_BOOL,
> + .help = N_("print XML document rather than attach the interface")
> +},
>  {.name = NULL}
>  };
>  
> @@ -938,13 +942,6 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>  if (live)
>  flags |= VIR_DOMAIN_AFFECT_LIVE;
>  
> -if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> -return false;
> -
> -if (persistent &&
> -virDomainIsActive(dom) == 1)
> -flags |= VIR_DOMAIN_AFFECT_LIVE;
> -
>  if (vshCommandOptStringReq(ctl, cmd, "type", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "source", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "target", ) < 0 ||
> @@ -1051,6 +1048,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>  virBufferAddLit(, "\n");
>  }
>  
> +virBufferAdjustIndent(, -2);
>  virBufferAddLit(, "\n");
>  
>  if (virBufferError()) {
> @@ -1060,6 +1058,19 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>  
>  xml = virBufferContentAndReset();
>  
> +if (vshCommandOptBool(cmd, "print-xml")) {
> +vshPrint(ctl, "%s", xml);
> +functionReturn = true;
> +goto cleanup;
> +}
> +
> +if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> +goto cleanup;
> +
> +if (persistent &&
> +virDomainIsActive(dom) == 1)
> +flags |= VIR_DOMAIN_AFFECT_LIVE;
> +
>  if (flags || current)
>  ret = virDomainAttachDeviceFlags(dom, xml, flags);
>  else
> @@ -1075,7 +1086,8 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>  }
>  
>   cleanup:

Move the VIR_FREE(xml); to here instead of where it is and before to
initialize it to NULL above...

ACK with those adjustments

John
> -virDomainFree(dom);
> +if (dom)
> +virDomainFree(dom);
>  virBufferFreeAndReset();
>  return functionReturn;
>  }
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 83c445d3..0212e7a 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -2507,6 +2507,7 @@ Likewise, I<--shareable> is an alias for I<--mode 
> shareable>.
>  [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]]
>  [I<--target target>] [I<--mac mac>] [I<--script script>] [I<--model model>]
>  [I<--inbound average,peak,burst,floor>] [I<--outbound average,peak,burst>]
> +[I<--print-xml>]
>  
>  Attach a new network interface to the domain.  I can be
>  I to indicate connection via a libvirt virtual network, or
> @@ -2536,6 +2537,9 @@ kilobytes in a single burst at I speed as 
> described in the
>  Network XML documentation at
>  L.
>  
> +If I<--print-xml> is specified, then the XML of the interface that would be
> +attached is printed instead.
> +
>  If I<--live> is specified, affect a running domain.
>  If I<--config> is specified, affect the next startup of a persistent domain.
>  If I<--current> is specified, affect the current domain state.
> 

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

Re: [libvirt] [PATCH] qemu: Check for existence of auto-generated socket path before removing

2015-09-16 Thread Martin Kletzander


On Tue, Sep 15, 2015 at 05:03:45PM -0400, John Ferlan wrote:

Commit id 'f1f68ca33' added code to remove the directory paths for
auto-generated sockets, but that code could be called before the
paths were created resulting in generating error messages from
virFileDeleteTree indicating that the file doesn't exist. So just
add a check before attemping the directory delete for existence.



Oh, I haven't checked that virFileDeleteTree() causes an error.  Since
we don't want "silent" versions functions (even though we have many :|
), ACK to this.  No race should happen, and if it does; no biggie.


Signed-off-by: John Ferlan 
---
src/qemu/qemu_process.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ce2c70c..b55eb52 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5269,13 +5269,13 @@ void qemuProcessStop(virQEMUDriverPtr driver,

ignore_value(virAsprintf(, "%s/domain-%s",
 cfg->libDir, vm->def->name));
-if (tmppath)
+if (tmppath && virFileExists(tmppath))
virFileDeleteTree(tmppath);
VIR_FREE(tmppath);

ignore_value(virAsprintf(, "%s/domain-%s",
 cfg->channelTargetDir, vm->def->name));
-if (tmppath)
+if (tmppath && virFileExists(tmppath))
virFileDeleteTree(tmppath);
VIR_FREE(tmppath);

--
2.1.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 v2] virfile: Check for existence of dir in virFileDeleteTree

2015-09-16 Thread Martin Kletzander


On Wed, Sep 16, 2015 at 10:01:46AM -0400, John Ferlan wrote:

Commit id 'f1f68ca33' added code to remove the directory paths for
auto-generated sockets, but that code could be called before the
paths were created resulting in generating error messages from
virFileDeleteTree indicating that the file doesn't exist.

Rather than "enforce" all callers to make the non-NULL and existence
checks, modify the virFileDeleteTree API to silently ignore NULL on
input and non-existent directory trees.

Signed-off-by: John Ferlan 
---

Difference over v1: Add checks in virFileDeleteTree



I like this version better as well, and even more when you fixed all
the callers.

ACK


src/qemu/qemu_process.c | 6 ++
src/util/virfile.c  | 8 ++--
tests/virhostdevtest.c  | 2 +-
tests/virscsitest.c | 2 +-
4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ce2c70c..155846e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5269,14 +5269,12 @@ void qemuProcessStop(virQEMUDriverPtr driver,

ignore_value(virAsprintf(, "%s/domain-%s",
 cfg->libDir, vm->def->name));
-if (tmppath)
-virFileDeleteTree(tmppath);
+virFileDeleteTree(tmppath);
VIR_FREE(tmppath);

ignore_value(virAsprintf(, "%s/domain-%s",
 cfg->channelTargetDir, vm->def->name));
-if (tmppath)
-virFileDeleteTree(tmppath);
+virFileDeleteTree(tmppath);
VIR_FREE(tmppath);

ignore_value(virDomainChrDefForeach(vm->def,
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 75819d9..fe9f8d4 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -934,13 +934,17 @@ int virFileNBDDeviceAssociate(const char *file,
 */
int virFileDeleteTree(const char *dir)
{
-DIR *dh = opendir(dir);
+DIR *dh;
struct dirent *de;
char *filepath = NULL;
int ret = -1;
int direrr;

-if (!dh) {
+/* Silently return 0 if passed NULL or directory doesn't exist */
+if (!dir || !virFileExists(dir))
+return 0;
+
+if (!(dh = opendir(dir))) {
virReportSystemError(errno, _("Cannot open dir '%s'"),
 dir);
return -1;
diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c
index 1e93819..065b825 100644
--- a/tests/virhostdevtest.c
+++ b/tests/virhostdevtest.c
@@ -65,7 +65,7 @@ myCleanup(void)
}

if (mgr) {
-if (mgr->stateDir && !getenv("LIBVIRT_SKIP_CLEANUP"))
+if (!getenv("LIBVIRT_SKIP_CLEANUP"))
virFileDeleteTree(mgr->stateDir);

virObjectUnref(mgr->activePCIHostdevs);
diff --git a/tests/virscsitest.c b/tests/virscsitest.c
index a86ca28..88286f1 100644
--- a/tests/virscsitest.c
+++ b/tests/virscsitest.c
@@ -241,7 +241,7 @@ mymain(void)
ret = -1;

 cleanup:
-if (tmpdir && getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
+if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
virFileDeleteTree(tmpdir);
VIR_FREE(virscsi_prefix);
return ret;
--
2.1.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 v2] lxc: fuse mount for /proc/cpuinfo

2015-09-16 Thread Serge Hallyn

Quoting Fabio Kung (fabio.k...@gmail.com):
> On Mon, Sep 7, 2015 at 8:55 AM, Serge Hallyn  wrote:
> >
> > Ah, my memory was failing me, so took a bit of searching, but
> >
> > http://fabiokung.com/2014/03/13/memory-inside-linux-containers/
> >
> > I can't find anything called 'libmymem', and in 2014 he said
> >
> > https://github.com/docker/docker/issues/8427#issuecomment-58255159
> >
> > so maybe this never went anywhere.
> 
> Correct, unfortunately.
> 
> 
> > For the same reasons you cited above, and because everyeone is rolling
> > their own at fuse level, I still think that a libresource and patches
> > to proc tools to use them, is the right way to go.  We have no shortage
> > of sample code for the functions doing the actual work, between libvirt,
> > lxc, docker, etc :)
> >
> > Should we just go ahead and start a libresource github project?
> 
> +1, if there's momentum on this I believe I will be able to contribute
> some cycles. Maybe now is the right time?

Might be.  Maybe the thing to do is start a project and mailing list
(any objections to github?  Do we create a new project for this?), and
see if more than 3 people join :)  Announce on containers@ and cgroup@
mailing lists, and start discussing what a reasonable API would look
like.

-serge

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

Re: [libvirt] [PATCH v3] qemu: Validate address type when attaching a disk device.

2015-09-16 Thread Michal Privoznik

On 09.09.2015 13:17, Ruifeng Bian wrote:
> Bug fix for: https://bugzilla.redhat.com/show_bug.cgi?id=1257844
> 
> Attach-device can hotplug a virtio disk device with any address type now,
> it need to validate the address type before the attachment.
> 
> Attaching a disk device with --config option need to check address type.
> Otherwise, the domain cloudn't be started normally. This patch add a basic
> check for address type.
> 
> Detaching a disk device with --config also need to check disk options,
> add qemuCheckDiskConfig() method in qemuDomainDetachDeviceConfig().
> ---
>  src/qemu/qemu_command.c | 44 +++-
>  src/qemu/qemu_driver.c  |  2 ++
>  src/qemu/qemu_hotplug.c | 22 ++
>  3 files changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ec5e3d4..a2c7483 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3579,12 +3579,54 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk)
>  goto error;
>  }
>  }
> +
> +if (disk->info.type) {
> +switch (disk->bus) {
> +case VIR_DOMAIN_DISK_BUS_IDE:
> +case VIR_DOMAIN_DISK_BUS_SCSI:
> +case VIR_DOMAIN_DISK_BUS_SATA:
> +case VIR_DOMAIN_DISK_BUS_FDC:
> +if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("disk cannot have an address of type '%s'"),
> +   
> virDomainDeviceAddressTypeToString(disk->info.type));
> +goto error;
> +}
> +break;
> +case VIR_DOMAIN_DISK_BUS_USB:
> +if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("usb disk cannot have an address of type 
> '%s'"),
> +   
> virDomainDeviceAddressTypeToString(disk->info.type));
> +goto error;
> +}
> +break;
> +case VIR_DOMAIN_DISK_BUS_VIRTIO:
> +if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
> +disk->info.type != 
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 &&
> +disk->info.type != 
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO &&
> +disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("virtio disk cannot have an address of type 
> '%s'"),
> +   
> virDomainDeviceAddressTypeToString(disk->info.type));
> +goto error;
> +}
> +break;
> +case VIR_DOMAIN_DISK_BUS_XEN:
> +case VIR_DOMAIN_DISK_BUS_UML:
> +case VIR_DOMAIN_DISK_BUS_SD:
> +/* no address type currently, return false if address provided */
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("disk cannot have an address of type '%s'"),
> +   
> virDomainDeviceAddressTypeToString(disk->info.type));
> +goto error;
> +}
> +}
>  return 0;
>   error:
>  return -1;
>  }

While this is technically correct I wonder if this should live here or
in the config parsing part. I mean, these constraints are not qemu
specific, are they?

>  
> -
>  /* Check whether the device address is using either 'ccw' or default s390
>   * address format and whether that's "legal" for the current qemu and/or
>   * guest os.machine type. This is the corollary to the code which doesn't
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 91eb661..61424b0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -8205,6 +8205,8 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
>  switch ((virDomainDeviceType) dev->type) {
>  case VIR_DOMAIN_DEVICE_DISK:
>  disk = dev->data.disk;
> +if (qemuCheckDiskConfig(disk) < 0)
> +return -1;
>  if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) {
>  virReportError(VIR_ERR_INVALID_ARG,
> _("no target device %s"), disk->dst);
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 63fafa6..1f46647 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -335,6 +335,28 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>  if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info, 
> priv->qemuCaps,
>  disk->dst))
>  goto cleanup;
> +
> +if (qemuDomainMachineIsS390CCW(vm->def) &&
> +virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
> +if (!virDomainDeviceAddressIsValid(>info,
> +

Re: [libvirt] [PATCH v2] lxc: fuse mount for /proc/cpuinfo

2015-09-16 Thread Daniel P. Berrange

On Wed, Sep 16, 2015 at 03:15:52PM +, Serge Hallyn wrote:
> Quoting Fabio Kung (fabio.k...@gmail.com):
> > On Mon, Sep 7, 2015 at 8:55 AM, Serge Hallyn  
> > wrote:
> > >
> > > Ah, my memory was failing me, so took a bit of searching, but
> > >
> > > http://fabiokung.com/2014/03/13/memory-inside-linux-containers/
> > >
> > > I can't find anything called 'libmymem', and in 2014 he said
> > >
> > > https://github.com/docker/docker/issues/8427#issuecomment-58255159
> > >
> > > so maybe this never went anywhere.
> > 
> > Correct, unfortunately.
> > 
> > 
> > > For the same reasons you cited above, and because everyeone is rolling
> > > their own at fuse level, I still think that a libresource and patches
> > > to proc tools to use them, is the right way to go.  We have no shortage
> > > of sample code for the functions doing the actual work, between libvirt,
> > > lxc, docker, etc :)
> > >
> > > Should we just go ahead and start a libresource github project?
> > 
> > +1, if there's momentum on this I believe I will be able to contribute
> > some cycles. Maybe now is the right time?
> 
> Might be.  Maybe the thing to do is start a project and mailing list
> (any objections to github?  Do we create a new project for this?), and
> see if more than 3 people join :)  Announce on containers@ and cgroup@
> mailing lists, and start discussing what a reasonable API would look
> like.

FWIW, I would support any such effort, but I'm unlikely to have free
resources to do anything more than watch its mailing list.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH v2] lxc: fuse mount for /proc/cpuinfo

2015-09-16 Thread Serge Hallyn

Quoting Daniel P. Berrange (berra...@redhat.com):
> On Wed, Sep 16, 2015 at 03:15:52PM +, Serge Hallyn wrote:
> > Quoting Fabio Kung (fabio.k...@gmail.com):
> > > On Mon, Sep 7, 2015 at 8:55 AM, Serge Hallyn  
> > > wrote:
> > > >
> > > > Ah, my memory was failing me, so took a bit of searching, but
> > > >
> > > > http://fabiokung.com/2014/03/13/memory-inside-linux-containers/
> > > >
> > > > I can't find anything called 'libmymem', and in 2014 he said
> > > >
> > > > https://github.com/docker/docker/issues/8427#issuecomment-58255159
> > > >
> > > > so maybe this never went anywhere.
> > > 
> > > Correct, unfortunately.
> > > 
> > > 
> > > > For the same reasons you cited above, and because everyeone is rolling
> > > > their own at fuse level, I still think that a libresource and patches
> > > > to proc tools to use them, is the right way to go.  We have no shortage
> > > > of sample code for the functions doing the actual work, between libvirt,
> > > > lxc, docker, etc :)
> > > >
> > > > Should we just go ahead and start a libresource github project?
> > > 
> > > +1, if there's momentum on this I believe I will be able to contribute
> > > some cycles. Maybe now is the right time?
> > 
> > Might be.  Maybe the thing to do is start a project and mailing list
> > (any objections to github?  Do we create a new project for this?), and
> > see if more than 3 people join :)  Announce on containers@ and cgroup@
> > mailing lists, and start discussing what a reasonable API would look
> > like.
> 
> FWIW, I would support any such effort, but I'm unlikely to have free
> resources to do anything more than watch its mailing list.

NP - if you can correct our course if we're heading someplace bad for
libvirt that'll be great.  Though I suspect lxc/lxd and libvirt will
mostly agree.

Ok, so I could create a project on github, but that doesn't come with
a m-l.  Last I used it, sf was problematic.  Any other suggestions for
where to host a mailing list?  Might the github issue tracker suffice?
We could (as worked quite well for lxd) have a specs/ directory in a
libresource source tree, and use issues and pull reuqests to guide the
api specifications under that directory.  Just a thought.

-serge

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

[libvirt] [PATCH] test driver: don't unlock pool after freeing it

2015-09-16 Thread David Mansfield

The attached patch (taken from my modified Fedora 22 source rpm, 
1.2.13.1-2.fc22, sorry),  fixes a case where, in the test driver, memory 
is accessed after it's freed.


Patch applies to latest git with:

Hunk #1 succeeded at 4395 (offset -469 lines).

The illegal access was found using valgrind.

The function testStoragePoolUndefine() calls virStoragePoolObjRemove() 
(which seems to free the memory for that pool structure) and then in the 
cleanup stanza calls virStoragePoolObjUnlock() which tampers with the 
freed structure.


--
Thanks,
David Mansfield
From: David Mansfield 
Date: Tue, 15 Sep 2015 10:05:56 -0400
Subject: [PATCH] test driver: don't unlock pool after freeing it 

The test driver was unlocking the pool object after it had been
freed causing memory corruption.

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index a386270..c2256dc 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4864,8 +4864,10 @@ testStoragePoolUndefine(virStoragePoolPtr pool)
 ret = 0;
 
  cleanup:
-if (privpool)
-virStoragePoolObjUnlock(privpool);
+// privpool cannot be unlocked because memory for it has been
+// freed by the virStoragePoolObjRemove call above
+// if (privpool)
+// virStoragePoolObjUnlock(privpool);
 testDriverUnlock(privconn);
 return ret;
 }
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 01/12] migration: refactor: get rid of use_params p2p_full

2015-09-16 Thread John Ferlan


FWIW: I figured I'd at least take a look - it's not my area of expertise
though. I also ran the changes through my Coverity checker. The first
pass found an issue in patch 10, which seems to be a result of some
changes in patch 2 and perhaps patch 3...


On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> 'useParams' parameter usage is an example of contol coupling. Most of the work

s/contol/control

> inside the function is done differently for different value of this flag 
> except

s/value of this flag//
> for the uri check. Lets split this function into 2, one with extensible

s/2/two

> parameters set and one with hardcoded parameter set. Common uri check we 
> factor
> out in different patch for clarity.

Move this last sentence to the commit message of patch 2...

> 
> Aim of this patchset is to unify logic for differet parameters representation
> so finally we merge this split back thru extensible parameters.

This paragraph could state - this patch begins a series of changes to
the Peer2Peer API's to utilize a common API with extensible parameters.
Or it could be stricken or moved to the cover letter...

> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |  129 +
>  1 files changed, 55 insertions(+), 74 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 964a4d7..1a00485 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3292,46 +3292,59 @@ virDomainMigrateVersion3Params(virDomainPtr domain,
>  params, nparams, true, flags);
>  }
>  
> +static int
> +virDomainMigratePeer2PeerParams(virDomainPtr domain,
> +const char *dconnuri,
> +virTypedParameterPtr params,
> +int nparams,
> +unsigned int flags)
> +{
> +virURIPtr tempuri = NULL;
> +
> +VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparams=%d, flags=%x",
> + dconnuri, params, nparams, flags);
> +VIR_TYPED_PARAMS_DEBUG(params, nparams);
> +
> +if (!domain->conn->driver->domainMigratePerform3Params) {
> +virReportUnsupportedError();
> +return -1;
> +}
> +
> +if (!(tempuri = virURIParse(dconnuri)))
> +return -1;
> +if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) {
> +virReportInvalidArg(dconnuri, "%s",
> +_("unable to parse server from dconnuri"));
> +virURIFree(tempuri);
> +return -1;
> +}
> +virURIFree(tempuri);
> +
> +VIR_DEBUG("Using migration protocol 3 with extensible parameters");
> +return domain->conn->driver->domainMigratePerform3Params
> +(domain, dconnuri, params, nparams,
> + NULL, 0, NULL, NULL, flags);
> +}

If this function goes below the "Plain" function, I think the git diff
output will be a lot cleaner...  Right now it's still a bit confusing.
That is Plain first then Params second.

>  
> -/*
> - * In normal migration, the libvirt client co-ordinates communication
> - * between the 2 libvirtd instances on source & dest hosts.
> - *
> - * In this peer-2-peer migration alternative, the libvirt client
> - * only talks to the source libvirtd instance. The source libvirtd
> - * then opens its own connection to the destination and co-ordinates
> - * migration itself.
> - *
> - * If useParams is true, params and nparams contain migration parameters and
> - * we know it's safe to call the API which supports extensible parameters.
> - * Otherwise, we have to use xmlin, dname, uri, and bandwidth and pass them
> - * to the old-style APIs.
> - */

Perhaps a hassle, but for now the comments could have moved with their
respective function... Perhaps the first two paragraphs for the "Plain"
function with the last "Otherwise" sentence changed slightly to indicate
an "old style" static parameter listing.

Then for the "Params" function that last paragraph preceded with a
reference to the "Plain" function...  Unlike the "Plain" function, this
function uses an extensible parameter list...

>  static int
> -virDomainMigratePeer2PeerFull(virDomainPtr domain,
> -  const char *dconnuri,
> -  const char *xmlin,
> -  const char *dname,
> -  const char *uri,
> -  unsigned long long bandwidth,
> -  virTypedParameterPtr params,
> -  int nparams,
> -  bool useParams,
> -  unsigned int flags)
> +virDomainMigratePeer2PeerPlain(virDomainPtr domain,
> +   const char *xmlin,
> +   unsigned int flags,
> +   const char *dname,
> +

Re: [libvirt] [PATCH v2 02/12] migration: refactor: reuse p2p url check

2015-09-16 Thread John Ferlan



On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> As promised in previous patch.

Update commit message to say what's being done from patch 1:

"Common uri check we factor out in different patch for clarity."

Or for me ;-)  "Refactor dconnuri local server URI check to common API"


> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   43 +++
>  1 files changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 1a00485..07e342f 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3293,14 +3293,33 @@ virDomainMigrateVersion3Params(virDomainPtr domain,
>  }
>  

I missed nothing this with the previous change - keep to two blank lines
between functions...

>  static int
> +virDomainMigrateCheckNotLocal(const char *dconnuri)
> +{
> +virURIPtr tempuri = NULL;
> +int ret = -1;
> +
> +if (!(tempuri = virURIParse(dconnuri)))

^^ This will get us into trouble later because 'dconnuri' cannot be NULL
when being passed to virURIParse; however, by patch 10 there's a call to
virDomainMigrateUnmanaged with a NULL dconnuri "if (!(flags &
VIR_MIGRATE_PEER2PEER))"

I haven't yet followed all the future code motion, although you could
add a ATTRIBUTE_NONNULL(1) after the "static int" to be more clear even
though yes, it's perhaps not something that was in the "existing" code.


Other than spacing and missing commit message, this seems fine.

John

> +goto cleanup;
> +if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) {
> +virReportInvalidArg(dconnuri, "%s",
> +_("Attempt to migrate guest to the same host 
> %s"));
> +goto cleanup;
> +}
> +
> +ret = 0;
> + cleanup:
> +
> +virURIFree(tempuri);
> +return ret;
> +}
> +
> +static int
>  virDomainMigratePeer2PeerParams(virDomainPtr domain,
>  const char *dconnuri,
>  virTypedParameterPtr params,
>  int nparams,
>  unsigned int flags)
>  {
> -virURIPtr tempuri = NULL;
> -
>  VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparams=%d, flags=%x",
>   dconnuri, params, nparams, flags);
>  VIR_TYPED_PARAMS_DEBUG(params, nparams);
> @@ -3310,15 +3329,8 @@ virDomainMigratePeer2PeerParams(virDomainPtr domain,
>  return -1;
>  }
>  
> -if (!(tempuri = virURIParse(dconnuri)))
> +if (virDomainMigrateCheckNotLocal(dconnuri) < 0)
>  return -1;
> -if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) {
> -virReportInvalidArg(dconnuri, "%s",
> -_("unable to parse server from dconnuri"));
> -virURIFree(tempuri);
> -return -1;
> -}
> -virURIFree(tempuri);
>  
>  VIR_DEBUG("Using migration protocol 3 with extensible parameters");
>  return domain->conn->driver->domainMigratePerform3Params
> @@ -3335,8 +3347,6 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain,
> const char *uri,
> unsigned long long bandwidth)
>  {
> -virURIPtr tempuri = NULL;
> -
>  VIR_DOMAIN_DEBUG(domain,
>   "dconnuri=%s, xmlin=%s, dname=%s, uri=%s, 
> bandwidth=%llu "
>   "flags=%x",
> @@ -3349,15 +3359,8 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain,
>  return -1;
>  }
>  
> -if (!(tempuri = virURIParse(dconnuri)))
> -return -1;
> -if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) {
> -virReportInvalidArg(dconnuri, "%s",
> -_("unable to parse server from dconnuri"));
> -virURIFree(tempuri);
> +if (virDomainMigrateCheckNotLocal(dconnuri) < 0)
>  return -1;
> -}
> -virURIFree(tempuri);
>  
>  if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
>  VIR_DRV_FEATURE_MIGRATION_V3)) {
> 

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

Re: [libvirt] [PATCH v2 03/12] migration: move implementation check to branches in p2p

2015-09-16 Thread John Ferlan



On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> This is more structured code so it will be easier to add branch for _PARAMS
> protocol here. It is not a pure refactoring strictly speaking as we remove
> scenarios for broken cases when driver defines V3 feature and implements
> perform function. So it is additionally a more solid code.
> 

The commit message describes a bit more than what's going on... This
patch just moves the checks for driver function closer to the respective
calls...

> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   14 --
>  1 files changed, 8 insertions(+), 6 deletions(-)
> 

Seems reasonable - although it becomes interesting to note that the
following patch adds the Perform3 check to the Direct code...

Of course back to my patch 2 comment about dconnuri - here's perhaps an
assumption made at one time that the Perform & Perform3 API's will
always have a non NULL dconnuri (although I didn't see any specific
NONNULL checks).

John

> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 07e342f..6f10c74 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3353,23 +3353,25 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain,
>   dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(uri),
>   bandwidth, flags);
>  
> -if (!domain->conn->driver->domainMigratePerform &&
> -!domain->conn->driver->domainMigratePerform3) {
> -virReportUnsupportedError();
> -return -1;
> -}
> -
>  if (virDomainMigrateCheckNotLocal(dconnuri) < 0)
>  return -1;
>  
>  if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
>  VIR_DRV_FEATURE_MIGRATION_V3)) {
>  VIR_DEBUG("Using migration protocol 3");
> +if (!domain->conn->driver->domainMigratePerform3) {
> +virReportUnsupportedError();
> +return -1;
> +}
>  return domain->conn->driver->domainMigratePerform3
>  (domain, xmlin, NULL, 0, NULL, NULL, dconnuri,
>   uri, flags, dname, bandwidth);
>  } else {
>  VIR_DEBUG("Using migration protocol 2");
> +if (!domain->conn->driver->domainMigratePerform) {
> +virReportUnsupportedError();
> +return -1;
> +}
>  if (xmlin) {
>  virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> _("Unable to change target guest XML during "
> 

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

Re: [libvirt] [PATCH v2 04/12] migration: remove direct migration dependency on version1 of driver

2015-09-16 Thread John Ferlan



On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> From: Michal Privoznik 
> 
> Direct migration should work if *perform3 is present but *perform
> is not. This is situation when driver migration is implemented
> after new version of driver function is introduced. We should not
> be forced to support old version too as its parameter space is
> subspace of newer one.
> 
> This patch has been already sent to mailist and has form
> that finally suggested by Michal Privoznik. I just include
> it in the set as it is not merged yet.

This last paragraph could go below the "---" since you're providing
"extra" information that we don't necessarily want in git history...


> ---
>  src/libvirt-domain.c |   13 -
>  1 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 6f10c74..15de714 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3412,16 +3412,15 @@ virDomainMigrateDirect(virDomainPtr domain,
>   NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(uri),
>   bandwidth);
>  
> -if (!domain->conn->driver->domainMigratePerform) {
> -virReportUnsupportedError();
> -return -1;
> -}
> -
>  /* Perform the migration.  The driver isn't supposed to return
>   * until the migration is complete.
>   */
>  if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
>   VIR_DRV_FEATURE_MIGRATION_V3)) {
> +if (!domain->conn->driver->domainMigratePerform3) {
> +virReportUnsupportedError();
> +return -1;
> +}

Ahhh... now I see where patch 10 is going... Direct has no dconnuri...
Hmmm... and I know why Coverity is complaining...

John


>  VIR_DEBUG("Using migration protocol 3");
>  /* dconn URI not relevant in direct migration, since no
>   * target libvirtd is involved */
> @@ -3437,6 +3436,10 @@ virDomainMigrateDirect(virDomainPtr domain,
> dname,
> bandwidth);
>  } else {
> +if (!domain->conn->driver->domainMigratePerform) {
> +virReportUnsupportedError();
> +return -1;
> +}
>  VIR_DEBUG("Using migration protocol 2");
>  if (xmlin) {
>  virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> 

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

Re: [libvirt] [PATCH v2 05/12] migration: refactor: merge direct and p2p into unmanaged

2015-09-16 Thread John Ferlan



On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> p2p plain and direct function are good candidates for code reuse. Their main
> function is same - to branch among different versions of migration protocol 
> and
> implementation of this function is also same. Also they have other common
> functionality in lesser aspects. So let's merge them.
> 
> But as they have different signatures we have to get to convention on how to
> pass direct migration 'uri' in 'dconnuri' and 'miguri'. Fortunately we alreay
> have such convention in parameters passed to toURI2 function, just let's 
> follow
> it. 'uri' is passed in miguri and dconnuri is ignored.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |  140 
> ++
>  1 files changed, 39 insertions(+), 101 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 15de714..1631944 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3339,21 +3339,23 @@ virDomainMigratePeer2PeerParams(virDomainPtr domain,
>  }
>  
>  static int
> -virDomainMigratePeer2PeerPlain(virDomainPtr domain,
> -   const char *xmlin,
> -   unsigned int flags,
> -   const char *dname,
> -   const char *dconnuri,
> -   const char *uri,
> -   unsigned long long bandwidth)
> +virDomainMigrateUnmanaged(virDomainPtr domain,
> +  const char *xmlin,
> +  unsigned int flags,
> +  const char *dname,
> +  const char *dconnuri,
> +  const char *miguri,
> +  unsigned long long bandwidth)

Perhaps should have just used that "Unmanaged" name all along as this
patch has two things going on - renaming a function and merging another
into it.  Perhaps even with the miguri parameter change too!

I know already present but thumbs down to modules that are felt to be
self commenting - especially the *uri params ;-)

>  {
> +const char *uri = NULL;
> +
>  VIR_DOMAIN_DEBUG(domain,
> - "dconnuri=%s, xmlin=%s, dname=%s, uri=%s, 
> bandwidth=%llu "
> + "dconnuri=%s, xmlin=%s, dname=%s, miguri=%s, 
> bandwidth=%llu "
>   "flags=%x",
> - dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(uri),
> + dconnuri, NULLSTR(xmlin), NULLSTR(dname), 
> NULLSTR(miguri),
>   bandwidth, flags);
>  
> -if (virDomainMigrateCheckNotLocal(dconnuri) < 0)
> +if ((flags & VIR_MIGRATE_PEER2PEER) && 
> virDomainMigrateCheckNotLocal(dconnuri) < 0)

Ahh... now I see... The "flags" check here is for only make this call on
VIR_MIGRATE_PEER2PEER; however, in patch 10, the dconnuri is cleared if
!(flags & VIR_MIGRATE_PEER2PEER) which I'm going to assume confuses
Coverity since flags can be "many" things. I don't have any smart ideas
how to resolve this yet, but at least it's beginning to make sense.

This API assumes only be the Peer2Peer or Direct - Peer2Peer is a
'flags' bit and Direct is a VIR_DRV_FEATURE_MIGRATION_DIRECT driver
feature.  We trust (ahem) our caller to do the right thing and call us
the right way.

>  return -1;
>  
>  if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> @@ -3365,7 +3367,7 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain,
>  }
>  return domain->conn->driver->domainMigratePerform3
>  (domain, xmlin, NULL, 0, NULL, NULL, dconnuri,
> - uri, flags, dname, bandwidth);
> + miguri, flags, dname, bandwidth);
>  } else {
>  VIR_DEBUG("Using migration protocol 2");
>  if (!domain->conn->driver->domainMigratePerform) {
> @@ -3378,85 +3380,21 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain,
>   "migration"));
>  return -1;
>  }
> -if (uri) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("Unable to override peer2peer migration URI"));
> -return -1;
> +if (flags & VIR_MIGRATE_PEER2PEER) {
> +if (miguri) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("Unable to override peer2peer migration 
> URI"));
> +return -1;
> +}
> +uri = dconnuri;
> +} else {
> +uri = miguri;
>  }
>  return domain->conn->driver->domainMigratePerform
> -(domain, NULL, 0, dconnuri, flags, dname, bandwidth);
> +(domain, NULL, 0, uri, flags, dname, bandwidth);
>  }
>  }
>  
> -/*
> - * In normal migration, the libvirt client co-ordinates communication
> - * between the

Re: [libvirt] [PATCH v2 05/12] migration: refactor: merge direct and p2p into unmanaged

2015-09-16 Thread John Ferlan


Of course I sent it too quick - one extra note...

[ ... snip... ]


>  /**
>   * virDomainMigrate:
>   * @domain: a domain object
> @@ -3594,8 +3532,8 @@ virDomainMigrate(virDomainPtr domain,
>  }
>  
>  VIR_DEBUG("Using peer2peer migration");
> -if (virDomainMigratePeer2PeerPlain(domain, NULL, flags, dname,
> -   uri ? uri : dstURI, NULL, 
> bandwidth) < 0) {
> +if (virDomainMigrateUnmanaged(domain, NULL, flags, dname,
> +  uri ? uri : dstURI, NULL, 
> bandwidth) < 0) {
>  VIR_FREE(dstURI);
>  goto error;
>  }
> @@ -3815,8 +3753,8 @@ virDomainMigrate2(virDomainPtr domain,
>  return NULL;
>  
>  VIR_DEBUG("Using peer2peer migration");
> -if (virDomainMigratePeer2PeerPlain(domain, dxml, flags, dname,
> -   dstURI, uri, bandwidth) < 0) {
> +if (virDomainMigrateUnmanaged(domain, dxml, flags, dname,
> +  dstURI, uri, bandwidth) < 0) {
>  VIR_FREE(dstURI);
>  goto error;
>  }
> @@ -4196,8 +4134,8 @@ virDomainMigrateToURI(virDomainPtr domain,
>  if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
>   VIR_DRV_FEATURE_MIGRATION_P2P)) {
>  VIR_DEBUG("Using peer2peer migration");
> -if (virDomainMigratePeer2PeerPlain(domain, NULL, flags,
> -   dname, duri, NULL, bandwidth) 
> < 0)
> +if (virDomainMigrateUnmanaged(domain, NULL, flags,
> +  dname, duri, NULL, bandwidth) < 0)
>  goto error;
>  } else {
>  /* No peer to peer migration supported */
> @@ -4208,8 +4146,8 @@ virDomainMigrateToURI(virDomainPtr domain,
>  if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
>   VIR_DRV_FEATURE_MIGRATION_DIRECT)) {
>  VIR_DEBUG("Using direct migration");
> -if (virDomainMigrateDirect(domain, NULL, flags,
> -   dname, duri, bandwidth) < 0)
> +if (virDomainMigrateUnmanaged(domain, NULL, flags,
> +   dname, NULL, duri, bandwidth) < 0)

Need extra spacing^^^

Same for other Direct to Unmanaged changes...

>  goto error;
>  } else {
>  /* Cannot do a migration with only the perform step */
> @@ -4342,8 +4280,8 @@ virDomainMigrateToURI2(virDomainPtr domain,
>  if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
>   VIR_DRV_FEATURE_MIGRATION_P2P)) {
>  VIR_DEBUG("Using peer2peer migration");
> -if (virDomainMigratePeer2PeerPlain(domain, dxml, flags,
> -   dname, dconnuri, miguri, 
> bandwidth) < 0)
> +if (virDomainMigrateUnmanaged(domain, dxml, flags,
> +  dname, dconnuri, miguri, 
> bandwidth) < 0)
>  goto error;
>  } else {
>  /* No peer to peer migration supported */
> @@ -4354,8 +4292,8 @@ virDomainMigrateToURI2(virDomainPtr domain,
>  if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
>   VIR_DRV_FEATURE_MIGRATION_DIRECT)) {
>  VIR_DEBUG("Using direct migration");
> -if (virDomainMigrateDirect(domain, dxml, flags,
> -   dname, miguri, bandwidth) < 0)
> +if (virDomainMigrateUnmanaged(domain, dxml, flags,
> +   dname, NULL, miguri, bandwidth) < 0)

Here

>  goto error;
>  } else {
>  /* Cannot do a migration with only the perform step */
> @@ -4473,8 +4411,8 @@ virDomainMigrateToURI3(virDomainPtr domain,
>  goto error;
>  } else if (compat) {
>  VIR_DEBUG("Using peer2peer migration");
> -if (virDomainMigratePeer2PeerPlain(domain, dxml, flags, dname,
> -   dconnuri, uri, bandwidth) < 0)
> +if (virDomainMigrateUnmanaged(domain, dxml, flags, dname,
> +  dconnuri, uri, bandwidth) < 0)
>  goto error;
>  } else {
>  virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> @@ -4501,8 +4439,8 @@ virDomainMigrateToURI3(virDomainPtr domain,
>  }
>  
>  VIR_DEBUG("Using direct migration");
> -if (virDomainMigrateDirect(domain, dxml, flags,
> -   dname, uri, bandwidth) < 0)
> +if (virDomainMigrateUnmanaged(domain, dxml,

[libvirt] [PATCH] libxl: fix AttachDeviceConfig on hostdev type

2015-09-16 Thread Chunyan Liu

After attach-device a  with --config, new device doesn't
show up in dumpxml and in guest.

To fix that, set dev->data.hostdev = NULL after work so that the
pointer is not freed, since vmdef has the pointer and still need it.

Signed-off-by: Chunyan Liu 
---
 src/libxl/libxl_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index e2797d5..8087c27 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3312,6 +3312,7 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, 
virDomainDeviceDefPtr dev)
 
 if (virDomainHostdevInsert(vmdef, hostdev) < 0)
 return -1;
+dev->data.hostdev = NULL;
 break;
 
 default:
-- 
1.8.5.6

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

Re: [libvirt] [PATCH RFC 1/7] libxl: implement virDomainGetCPUStats

2015-09-16 Thread Jim Fehlig


On 09/08/2015 02:27 AM, Joao Martins wrote:

Introduce support for domainGetCPUStats API call and consequently
allow us to use `virsh cpu-stats`. The latter returns a more brief
output than the one provided by`virsh vcpuinfo`.

Signed-off-by: Joao Martins 
---
  src/libxl/libxl_driver.c | 111 +++
  1 file changed, 111 insertions(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 5f69b49..101e4c7 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -73,6 +73,8 @@ VIR_LOG_INIT("libxl.libxl_driver");
  #define LIBXL_CONFIG_FORMAT_XM "xen-xm"
  #define LIBXL_CONFIG_FORMAT_SEXPR "xen-sxpr"
  
+#define LIBXL_NB_TOTAL_CPU_STAT_PARAM 1

+
  #define HYPERVISOR_CAPABILITIES "/proc/xen/capabilities"
  #define HYPERVISOR_XENSTORED "/dev/xen/xenstored"
  
@@ -4638,6 +4640,114 @@ libxlDomainIsUpdated(virDomainPtr dom)

  }
  
  static int

+libxlDomainGetTotalCPUStats(libxlDriverPrivatePtr driver,
+virDomainObjPtr vm,
+virTypedParameterPtr params,
+unsigned int nparams)
+{
+libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
+libxl_dominfo d_info;
+int ret = -1;
+
+if (nparams == 0)
+return LIBXL_NB_TOTAL_CPU_STAT_PARAM;
+
+if (libxl_domain_info(cfg->ctx, _info, vm->def->id) != 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("libxl_domain_info failed for domain '%d'"),
+   vm->def->id);
+return ret;
+}
+


I see that xl calls libxl_dominfo_dispose() after successful 
libxl_domain_info(). We might be missing that elsewhere in the libxl driver.



+if (virTypedParameterAssign([0], VIR_DOMAIN_CPU_STATS_CPUTIME,
+VIR_TYPED_PARAM_ULLONG, d_info.cpu_time) < 0)
+return ret;
+
+return nparams;
+}
+
+static int
+libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver,
+virDomainObjPtr vm,
+virTypedParameterPtr params,
+unsigned int nparams,
+int start_cpu,
+unsigned int ncpus)


Indentation of parameters is off.


+{
+libxl_vcpuinfo *vcpuinfo;
+int maxcpu, hostcpus;
+size_t i;
+libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
+int ret = -1;
+
+if (nparams == 0 && ncpus != 0)
+return LIBXL_NB_TOTAL_CPU_STAT_PARAM;
+else if (nparams == 0)
+return vm->def->maxvcpus;
+
+if ((vcpuinfo = libxl_list_vcpu(cfg->ctx, vm->def->id, ,
+)) == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Failed to list vcpus for domain '%d' with 
libxenlight"),
+   vm->def->id);
+goto cleanup;
+}
+
+for (i = start_cpu; i < maxcpu && i < ncpus; ++i) {
+if (virTypedParameterAssign([(i-start_cpu)],
+VIR_DOMAIN_CPU_STATS_CPUTIME,
+VIR_TYPED_PARAM_ULLONG,
+vcpuinfo[i].vcpu_time) < 0)
+goto cleanup;
+
+libxl_vcpuinfo_dispose([i]);


The call to libxl_vcpuinfo_dispose() can be removed


+}
+ret = nparams;
+
+ cleanup:
+VIR_FREE(vcpuinfo);


if the VIR_FREE() is replaced with libxl_vcpuinfo_list_free().

Other than the few nits, looks good.

BTW, thanks for the patches! But be warned: I may make slow progress reviewing 
them.

Regards,
Jim


+return ret;
+}
+
+static int
+libxlDomainGetCPUStats(virDomainPtr dom,
+   virTypedParameterPtr params,
+   unsigned int nparams,
+   int start_cpu,
+   unsigned int ncpus,
+   unsigned int flags)
+{
+libxlDriverPrivatePtr driver = dom->conn->privateData;
+virDomainObjPtr vm = NULL;
+int ret = -1;
+
+virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
+
+if (!(vm = libxlDomObjFromDomain(dom)))
+goto cleanup;
+
+if (virDomainGetCPUStatsEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   "%s", _("domain is not running"));
+goto cleanup;
+}
+
+if (start_cpu == -1)
+ret = libxlDomainGetTotalCPUStats(driver, vm, params, nparams);
+else
+ret = libxlDomainGetPerCPUStats(driver, vm, params, nparams,
+  start_cpu, ncpus);
+
+ cleanup:
+if (vm)
+virObjectUnlock(vm);
+return ret;
+}
+
+static int
  libxlConnectDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int 
eventID,
 virConnectDomainEventGenericCallback 
callback,
 void *opaque,

Re: [libvirt] [PATCH v3] qemu: Validate address type when attaching a disk device.

2015-09-16 Thread Ruifeng Bian


- Original Message -
> From: "Michal Privoznik" 
> To: "Ruifeng Bian" , libvir-list@redhat.com
> Sent: Wednesday, September 16, 2015 11:32:27 PM
> Subject: Re: [libvirt] [PATCH v3] qemu: Validate address type when attaching 
> a disk device.
> 
> On 09.09.2015 13:17, Ruifeng Bian wrote:
> > Bug fix for: https://bugzilla.redhat.com/show_bug.cgi?id=1257844
> > 
> > Attach-device can hotplug a virtio disk device with any address type now,
> > it need to validate the address type before the attachment.
> > 
> > Attaching a disk device with --config option need to check address type.
> > Otherwise, the domain cloudn't be started normally. This patch add a basic
> > check for address type.
> > 
> > Detaching a disk device with --config also need to check disk options,
> > add qemuCheckDiskConfig() method in qemuDomainDetachDeviceConfig().
> > ---
> >  src/qemu/qemu_command.c | 44 +++-
> >  src/qemu/qemu_driver.c  |  2 ++
> >  src/qemu/qemu_hotplug.c | 22 ++
> >  3 files changed, 67 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index ec5e3d4..a2c7483 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -3579,12 +3579,54 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk)
> >  goto error;
> >  }
> >  }
> > +
> > +if (disk->info.type) {
> > +switch (disk->bus) {
> > +case VIR_DOMAIN_DISK_BUS_IDE:
> > +case VIR_DOMAIN_DISK_BUS_SCSI:
> > +case VIR_DOMAIN_DISK_BUS_SATA:
> > +case VIR_DOMAIN_DISK_BUS_FDC:
> > +if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("disk cannot have an address of type
> > '%s'"),
> > +
> > virDomainDeviceAddressTypeToString(disk->info.type));
> > +goto error;
> > +}
> > +break;
> > +case VIR_DOMAIN_DISK_BUS_USB:
> > +if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("usb disk cannot have an address of type
> > '%s'"),
> > +
> > virDomainDeviceAddressTypeToString(disk->info.type));
> > +goto error;
> > +}
> > +break;
> > +case VIR_DOMAIN_DISK_BUS_VIRTIO:
> > +if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
> > +disk->info.type !=
> > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 &&
> > +disk->info.type !=
> > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO &&
> > +disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("virtio disk cannot have an address of
> > type '%s'"),
> > +
> > virDomainDeviceAddressTypeToString(disk->info.type));
> > +goto error;
> > +}
> > +break;
> > +case VIR_DOMAIN_DISK_BUS_XEN:
> > +case VIR_DOMAIN_DISK_BUS_UML:
> > +case VIR_DOMAIN_DISK_BUS_SD:
> > +/* no address type currently, return false if address provided
> > */
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("disk cannot have an address of type '%s'"),
> > +
> > virDomainDeviceAddressTypeToString(disk->info.type));
> > +goto error;
> > +}
> > +}
> >  return 0;
> >   error:
> >  return -1;
> >  }
> 
> While this is technically correct I wonder if this should live here or
> in the config parsing part. I mean, these constraints are not qemu
> specific, are they?

I have thought about it. I'm not sure whether other hypervisor will use it,
so I put it here. Okay, moving this part to domain_conf is better, I will do it.

Thanks,
Ruifeng

> 
> >  
> > -
> >  /* Check whether the device address is using either 'ccw' or default s390
> >   * address format and whether that's "legal" for the current qemu and/or
> >   * guest os.machine type. This is the corollary to the code which doesn't
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 91eb661..61424b0 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -8205,6 +8205,8 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
> >  switch ((virDomainDeviceType) dev->type) {
> >  case VIR_DOMAIN_DEVICE_DISK:
> >  disk = dev->data.disk;
> > +if (qemuCheckDiskConfig(disk) < 0)
> > +return -1;
> >  if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) {
> >  virReportError(VIR_ERR_INVALID_ARG,
> > _("no target device %s"), disk->dst);
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index

[libvirt] [PATCH] libxl: fix AttachDeviceConfig on hostdev type

2015-09-16 Thread Chunyan Liu

After attach-device a  with --config, new device doesn't
show up in dumpxml and in guest.

To fix that, set dev->data.hostdev = NULL after work so that the
pointer is not freed, since vmdef has the pointer and still need it.

Signed-off-by: Chunyan Liu 
---
 src/libxl/libxl_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index e2797d5..8087c27 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3312,6 +3312,7 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, 
virDomainDeviceDefPtr dev)
 
 if (virDomainHostdevInsert(vmdef, hostdev) < 0)
 return -1;
+dev->data.hostdev = NULL;
 break;
 
 default:
-- 
1.8.5.6

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

[libvirt] [PATCH v2 5/7] qemu: s/virDomainDiskDiffersSourceOnly/qemuDomainDiskChangeSupported/

[libvirt] [PATCH v2 0/7] Allow startupPolicy change on update-device

[libvirt] [PATCH v2 2/7] qemu: s/qemuDomainChangeDiskMediaLive/qemuDomainChangeDiskLive/

[libvirt] [PATCH v2 7/7] qemuDomainChangeDiskLive: Allow startupPolicy change

[libvirt] [PATCH v2 4/7] qemuDomainChangeDiskLive: rework slightly

[libvirt] [PATCH v2 6/7] qemuDomainDiskChangeSupported: Fill in missing checks

[libvirt] [PATCH v2 1/7] qemuDomainUpdateDeviceConfig: Allow startupPolicy update, yet again

[libvirt] [PATCH v2 3/7] qemu_domain: Introduce qemuDomainDiskSourceDiffers

Re: [libvirt] VMware: Map vpx:// to dcPath

Re: [libvirt] [PATCH v3] qemu: Fix using guest architecture as lookup key

[libvirt] [PATCH v3] qemu: Fix using guest architecture as lookup key

Re: [libvirt] [PATCH v2] qemu: Fix using guest architecture as lookup key

Re: [libvirt] [PATCH v3] qemu: Fix using guest architecture as lookup key

Re: [libvirt] [PATCH] qemu: Check for existence of auto-generated socket path before removing

[libvirt] [PATCH v2] virfile: Check for existence of dir in virFileDeleteTree

Re: [libvirt] [PATCH] qemu: Check for existence of auto-generated socket path before removing

Re: [libvirt] [PATCH v2] virsh: Teach attach-interface to --print-xml

Re: [libvirt] [PATCH] qemu: Check for existence of auto-generated socket path before removing

Re: [libvirt] [PATCH v2] virfile: Check for existence of dir in virFileDeleteTree

Re: [libvirt] [PATCH v2] lxc: fuse mount for /proc/cpuinfo

Re: [libvirt] [PATCH v3] qemu: Validate address type when attaching a disk device.

Re: [libvirt] [PATCH v2] lxc: fuse mount for /proc/cpuinfo

Re: [libvirt] [PATCH v2] lxc: fuse mount for /proc/cpuinfo

[libvirt] [PATCH] test driver: don't unlock pool after freeing it

Re: [libvirt] [PATCH v2 01/12] migration: refactor: get rid of use_params p2p_full

Re: [libvirt] [PATCH v2 02/12] migration: refactor: reuse p2p url check

Re: [libvirt] [PATCH v2 03/12] migration: move implementation check to branches in p2p

Re: [libvirt] [PATCH v2 04/12] migration: remove direct migration dependency on version1 of driver

Re: [libvirt] [PATCH v2 05/12] migration: refactor: merge direct and p2p into unmanaged

Re: [libvirt] [PATCH v2 05/12] migration: refactor: merge direct and p2p into unmanaged

[libvirt] [PATCH] libxl: fix AttachDeviceConfig on hostdev type

Re: [libvirt] [PATCH RFC 1/7] libxl: implement virDomainGetCPUStats

Re: [libvirt] [PATCH v3] qemu: Validate address type when attaching a disk device.

[libvirt] [PATCH] libxl: fix AttachDeviceConfig on hostdev type

34 matches

Site Navigation

Mail list logo

Footer information