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

2015-09-17 Thread Peter Krempa 
On Wed, Sep 16, 2015 at 17:14:28 -0400, David Mansfield wrote:
> 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);

This cannot be just commented out since the code above is allowing only
inactive pools to be deleted and jumps to the cleanup label otherwise.
With this approach the lock would stay locked and the test driver would
deadlock the next time you are going to reference the same pool.

The right fix here is to clear the 'privpool' pointer after we know that
the pool was removed.

As a note, line comments are not allowed in libvirt.

Should I post the correct patch in your name or do you want to submit it
yourself?

Peter


signature.asc
Description: Digital 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-17 Thread Cedric Bosdonnat 
On Wed, 2015-09-16 at 19:29 +, Serge Hallyn wrote:
> 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.

I can possibly help the coding... though I'm not too versed in the
low-level things (yet), don't count on me as one of the main hackers ;)

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

It could be OK to start with the github issue tracker and we'll see if a
mailing list is really needed. I'm using SF.net for other projects and I
feel it's always a pain to use.

--
Cedric

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

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

2015-09-17 Thread Ruifeng Bian 
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().

Add virDomainDeviceAddressTypeIsValid method in domain_conf to check disk
address type. Address type should match with disk driver, report error
messages if any mismatch.

Signed-off-by: Ruifeng Bian 
---
 src/conf/domain_conf.c   | 32 
 src/conf/domain_conf.h   |  1 +
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_command.c  |  7 +++
 src/qemu/qemu_driver.c   |  2 ++
 src/qemu/qemu_hotplug.c  | 22 ++
 6 files changed, 65 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6df1618..f86760b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3200,6 +3200,38 @@ int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr 
info,
 return 0;
 }
 
+int virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk)
+{
+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)
+return 1;
+break;
+case VIR_DOMAIN_DISK_BUS_USB:
+if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
+return 1;
+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)
+return 1;
+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 */
+return 1;
+}
+}
+return 0;
+}
+
 virDomainDeviceInfoPtr
 virDomainDeviceGetInfo(virDomainDeviceDefPtr device)
 {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f043554..337fe51 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2543,6 +2543,7 @@ virDomainDeviceDefPtr 
virDomainDeviceDefCopy(virDomainDeviceDefPtr src,
  virDomainXMLOptionPtr xmlopt);
 int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
   int type);
+int virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk);
 virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr device);
 int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
 virDomainDeviceInfoPtr src);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5b3da83..6cd5b9e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -229,6 +229,7 @@ virDomainDefPostParse;
 virDomainDefSetMemoryInitial;
 virDomainDeleteConfig;
 virDomainDeviceAddressIsValid;
+virDomainDeviceAddressTypeIsValid;
 virDomainDeviceAddressTypeToString;
 virDomainDeviceDefCheckUnsupportedMemoryDevice;
 virDomainDeviceDefCopy;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 25f57f2..56ba08d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3606,6 +3606,13 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk)
 goto error;
 }
 }
+
+if (virDomainDeviceAddressTypeIsValid(disk)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("disk cannot have an address of type '%s'"),
+   virDomainDeviceAddressTypeToString(disk->info.type));
+goto error;
+}
 return 0;
  error:
 return -1;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fcf86b6..26c1502 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8208,6 +8208,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 e84a78d..6156243 100644
---

[libvirt] [PATCH 1/3] conf: Add new VIR_DOMAIN_VIRT_NONE enum

2015-09-17 Thread Shivangi Dhir 
Introduce VIR_DOMAIN_VIRT_NONE to give domaintype the default value of zero.
This is specially helpful in constructing better error messages
when we don't want to look up the default emulator by virtType.

The test data in vircapstest.c is also modified to reflect this change.
---
 src/conf/capabilities.c |  4 ++--
 src/conf/domain_conf.c  |  3 ++-
 src/conf/domain_conf.h  |  1 +
 tests/vircapstest.c | 32 
 4 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 9c2c6b4..533c925 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -584,7 +584,7 @@ virCapsDomainDataCompare(virCapsGuestPtr guest,
 if ((arch != VIR_ARCH_NONE) && (guest->arch.id != arch))
 return false;
 
-if (domaintype != -1 && (!domain || domain->type != domaintype))
+if (domaintype != VIR_DOMAIN_VIRT_NONE && (!domain || domain->type != 
domaintype))
 return false;
 
 if (emulator) {
@@ -678,7 +678,7 @@ virCapabilitiesDomainDataLookupInternal(virCapsPtr caps,
   virDomainOSTypeToString(ostype));
 if (arch)
 virBufferAsprintf(, "arch=%s ", virArchToString(arch));
-if (domaintype != -1)
+if (domaintype)
 virBufferAsprintf(, "domaintype=%s ",
   virDomainVirtTypeToString(domaintype));
 if (emulator)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6df1618..589546d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -106,6 +106,7 @@ VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST,
   "custom-dtb");
 
 VIR_ENUM_IMPL(virDomainVirt, VIR_DOMAIN_VIRT_LAST,
+  "none",
   "qemu",
   "kqemu",
   "kvm",
@@ -14815,7 +14816,7 @@ virDomainDefParseXML(xmlDocPtr xml,
 virCapsDomainDataPtr capsdata = NULL;
 
 if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type,
-def->os.arch, use_virttype ? def->virtType : -1,
+def->os.arch, use_virttype ? def->virtType : 
VIR_DOMAIN_VIRT_NONE,
 NULL, NULL)))
 goto error;
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f043554..16f449d 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -209,6 +209,7 @@ struct _virDomainDeviceDef {
 /* Different types of hypervisor */
 /* NB: Keep in sync with virDomainVirtTypeToString impl */
 typedef enum {
+VIR_DOMAIN_VIRT_NONE = 0,
 VIR_DOMAIN_VIRT_QEMU,
 VIR_DOMAIN_VIRT_KQEMU,
 VIR_DOMAIN_VIRT_KVM,
diff --git a/tests/vircapstest.c b/tests/vircapstest.c
index 3b41654..acb0c03 100644
--- a/tests/vircapstest.c
+++ b/tests/vircapstest.c
@@ -223,39 +223,39 @@ test_virCapsDomainDataLookupQEMU(const void *data 
ATTRIBUTE_UNUSED)
 }
 
 /* Checking each parameter individually */
-CAPSCOMP(-1, VIR_ARCH_NONE, -1, NULL, NULL,
+CAPSCOMP(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, NULL,
 VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_X86_64,
 VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-x86_64", "pc-0.11");
-CAPSCOMP(VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_NONE, -1, NULL, NULL,
+CAPSCOMP(VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, 
NULL,
 VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_X86_64,
 VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-x86_64", "pc-0.11");
-CAPSCOMP(-1, VIR_ARCH_AARCH64, -1, NULL, NULL,
+CAPSCOMP(-1, VIR_ARCH_AARCH64, VIR_DOMAIN_VIRT_NONE, NULL, NULL,
 VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_AARCH64,
 VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-aarch64", "virt");
 CAPSCOMP(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_KVM, NULL, NULL,
 VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_X86_64,
 VIR_DOMAIN_VIRT_KVM, "/usr/bin/kvm", "pc");
-CAPSCOMP(-1, VIR_ARCH_NONE, -1, "/usr/bin/qemu-system-ppc64", NULL,
+CAPSCOMP(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, 
"/usr/bin/qemu-system-ppc64", NULL,
 VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_PPC64,
 VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-ppc64", "pseries");
-CAPSCOMP(-1, VIR_ARCH_NONE, -1, NULL, "s390-virtio",
+CAPSCOMP(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, "s390-virtio",
 VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_S390X,
 VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-s390x",
 "s390-virtio");
 
-CAPSCOMP(-1, VIR_ARCH_NONE, -1, NULL, "pseries",
+CAPSCOMP(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, "pseries",
 VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_PPC64,
 VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-ppc64", "pseries");
-CAPSCOMP(-1, VIR_ARCH_PPC64LE, -1, NULL, "pseries",
+CAPSCOMP(-1, VIR_ARCH_PPC64LE, VIR_DOMAIN_VIRT_NONE, NULL, "pseries",
 VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_PPC64LE,
 VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-ppc64", "pseries");
 
-CAPS_EXPECT_ERR(VIR_DOMAIN_OSTYPE_LINUX, VIR_ARCH_NONE, -1,

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

2015-09-17 Thread Richard W.M. Jones 
On Thu, Sep 17, 2015 at 10:28:02AM +0200, Matthias Bolte wrote:
> 2015-09-16 10:47 GMT+02:00 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
> 
> Regarding your question about the changes to the VMware driver: The
> only required change is the addition for "ctx.datacenterPath = NULL"
> where virVMXParseConfig is called. The other additions are not really
> necessary. I made them for the sake of completeness and to match the
> usage in the ESX driver.
> 
> Overall I'm happy with this patch. The only concern I have is that the
> domain XML might not be quite the right place to expose this
> information. But there are not many other places to expose this
> without adding new public API. But I assume that the domain XML is the
> most convenient was for libguestfs to get this information, isn't it?

We can get it from anywhere as long as it's in the XML.

Let's wait for a second review, and if that is ACKed then I will
push it.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

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

Re: [libvirt] [PATCH v2 0/12] migration: support all toURI and proto combos

2015-09-17 Thread Daniel P. Berrange 
On Thu, Sep 10, 2015 at 04:20:12PM +0300, Nikolay Shirokovskiy wrote:
> Current implementation of 'toURI' migration interfaces does not support all
> combinations of interface versions and protocol versions. For example 'toURI2'
> with p2p flag will not migrate if driver supports only v3params proto.
> 
> This is not convinient as drivers that starts to support migration have to
> manually support older versions of protocol. I guess this should be done in
> one place, namely here.
> 
> Another issue is that there are a lot of code duplication in implementation of
> toURI interfaces and it is not obvious from code how are they related.
> 
> This implementation uses extensible parameters as intermediate parameters
> representation. This is possible as interfaces are done backward compatible in
> terms of parameters and later versions supports all parameters of former
> versions.
> = Changes from version1
> 
> Patch is splitted into a set. Quite a big one as a result of the following 
> strategy:
> 
> 1. each change in behaviour even subtle one deserves a separate patch. One
>patch changes one aspect in behaviour.
> 
> 2. separate pure refactoring steps into patches too as rather simple refactor
>steps could introduce many line changes. Mark such patches with 'refactor:'
> 
> Now every patch is easy to grasp I think.
> 
> The resulted cumulative patch is slightly different from first in behaviour 
> but
> I'm not going to describe the differece here as original patch was not 
> reviewed
> in details by anyone anyway )
> 
>  src/libvirt-domain.c |  520 
> +-
>  1 files changed, 216 insertions(+), 304 deletions(-)

Just a quick note to say that I haven't forgotten about this patch
series. I'm looking to review it today/tomorrow I hope.

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] VMware: Map vpx:// to dcPath

2015-09-17 Thread Daniel P. Berrange 
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?
> 
> -- 
> Matthias Bolte
> http://photron.blogspot.com

> 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
> 
> Tool such as libguestfs need the datacenter path to get access to disk
> images. The ESX driver knows the correct datacenter path, but this
> information cannot be accessed using libvirt API yet. Also, it cannot
> be deduced from the connection URI in a robust way.
> 
> Expose the datacenter path in the domain XML as 
> node similar to the way the  node works. The new node
> is ignored while parsing the domain XML. In contrast to 
> it is output only.
> ---
>  src/esx/esx_driver.c |  4 ++
>  src/vmware/vmware_conf.c |  3 ++
>  src/vmware/vmware_driver.c   |  9 
>  src/vmx/vmx.c| 68 
> +++-
>  src/vmx/vmx.h| 10 ++--
>  tests/vmx2xmldata/vmx2xml-datacenterpath.vmx |  2 +
>  tests/vmx2xmldata/vmx2xml-datacenterpath.xml | 19 
>  tests/vmx2xmltest.c  |  5 ++
>  tests/xml2vmxdata/xml2vmx-datacenterpath.vmx | 10 
>  tests/xml2vmxdata/xml2vmx-datacenterpath.xml |  9 
>  tests/xml2vmxtest.c  |  3 ++
>  11 files changed, 126 insertions(+), 16 deletions(-)
>  create mode 100644 tests/vmx2xmldata/vmx2xml-datacenterpath.vmx
>  create mode 100644 tests/vmx2xmldata/vmx2xml-datacenterpath.xml
>  create mode 100644 tests/xml2vmxdata/xml2vmx-datacenterpath.vmx
>  create mode 100644 tests/xml2vmxdata/xml2vmx-datacenterpath.xml

ACK, I think this is acceptable given the current requirements
of libguestfs


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 3/3] conf: Change the description of virCapabilitiesDomainDataLookup()

2015-09-17 Thread Shivangi Dhir 
@domaintype: takes domain type to search for (of enum virDomainVirtType)
since we no longer pass out-of-enum values.
---
 src/conf/capabilities.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 533c925..b420f9f 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -723,7 +723,7 @@ virCapabilitiesDomainDataLookupInternal(virCapsPtr caps,
  * @caps: capabilities to query
  * @ostype: guest operating system type, of enum VIR_DOMAIN_OSTYPE
  * @arch: Architecture to search for
- * @domaintype: domain type to search for, of enum VIR_DOMAIN_VIRT
+ * @domaintype: domain type to search for, of enum virDomainVirtType
  * @emulator: Emulator path to search for
  * @machinetype: Machine type to search for
  *
-- 
1.9.1

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

[libvirt] [PATCH 2/3] qemu: Make virtType of type virDomainVirtType

2015-09-17 Thread Shivangi Dhir 
Earlier virtType was of type int. After, introducing the enum 
VIR_DOMAIN_VIRT_NONE,
the type of virtType is modified to virDomainVirtType.
---
 src/conf/domain_conf.h   | 2 +-
 src/qemu/qemu_monitor.c  | 2 +-
 src/qemu/qemu_monitor.h  | 2 +-
 src/qemu/qemu_monitor_json.c | 2 +-
 src/qemu/qemu_monitor_json.h | 2 +-
 src/qemu/qemu_monitor_text.c | 2 +-
 src/qemu/qemu_monitor_text.h | 2 +-
 tests/qemumonitorjsontest.c  | 2 +-
 8 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 16f449d..5576e4d 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2189,7 +2189,7 @@ struct _virDomainKeyWrapDef {
 typedef struct _virDomainDef virDomainDef;
 typedef virDomainDef *virDomainDefPtr;
 struct _virDomainDef {
-int virtType;
+virDomainVirtType virtType;
 int id;
 unsigned char uuid[VIR_UUID_BUFLEN];
 char *name;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 15ba39b..49d4aa2 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1635,7 +1635,7 @@ qemuMonitorSetLink(qemuMonitorPtr mon,
 
 int
 qemuMonitorGetVirtType(qemuMonitorPtr mon,
-   int *virtType)
+   virDomainVirtType *virtType)
 {
 QEMU_CHECK_MONITOR(mon);
 
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 720fb2d..2ce3958 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -360,7 +360,7 @@ int qemuMonitorSystemPowerdown(qemuMonitorPtr mon);
 int qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
   int **pids);
 int qemuMonitorGetVirtType(qemuMonitorPtr mon,
-   int *virtType);
+   virDomainVirtType *virtType);
 int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon,
   unsigned long long *currmem);
 int qemuMonitorGetMemoryStats(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 5c2f50f..df0c82a 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1339,7 +1339,7 @@ int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon,
 
 
 int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon,
-   int *virtType)
+   virDomainVirtType *virtType)
 {
 int ret;
 virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-kvm",
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index b76d85b..120bd93 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -56,7 +56,7 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon);
 int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon,
   int **pids);
 int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon,
-   int *virtType);
+   virDomainVirtType *virtType);
 int qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon,
  virDomainVideoDefPtr video,
  char *path);
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index d5ef089..f44da20 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -564,7 +564,7 @@ int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon,
 
 
 int qemuMonitorTextGetVirtType(qemuMonitorPtr mon,
-   int *virtType)
+   virDomainVirtType *virtType)
 {
 char *reply = NULL;
 
diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h
index 3fa603b..53c503d 100644
--- a/src/qemu/qemu_monitor_text.h
+++ b/src/qemu/qemu_monitor_text.h
@@ -52,7 +52,7 @@ int qemuMonitorTextSystemReset(qemuMonitorPtr mon);
 int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon,
   int **pids);
 int qemuMonitorTextGetVirtType(qemuMonitorPtr mon,
-   int *virtType);
+   virDomainVirtType *virtType);
 int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon,
   unsigned long long *currmem);
 int qemuMonitorTextGetMemoryStats(qemuMonitorPtr mon,
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index ded0423..17ebf64 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1311,7 +1311,7 @@ testQemuMonitorJSONqemuMonitorJSONGetVirtType(const void 
*data)
 virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
 qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt);
 int ret = -1;
-int virtType;
+virDomainVirtType virtType;
 
 if (!test)
 return -1;
-- 
1.9.1

--
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-17 Thread Matthias Bolte 
2015-09-16 10:47 GMT+02:00 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

Regarding your question about the changes to the VMware driver: The
only required change is the addition for "ctx.datacenterPath = NULL"
where virVMXParseConfig is called. The other additions are not really
necessary. I made them for the sake of completeness and to match the
usage in the ESX driver.

Overall I'm happy with this patch. The only concern I have is that the
domain XML might not be quite the right place to expose this
information. But there are not many other places to expose this
without adding new public API. But I assume that the domain XML is the
most convenient was for libguestfs to get this information, isn't it?

Anyway, there is no later version of this patch. I'm okay with you
pushing it to the libvirt repo.

-- 
Matthias Bolte
http://photron.blogspot.com

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

[libvirt] [PATCH 0/3] Introduce new virtType enum item

2015-09-17 Thread Shivangi Dhir 
These patches solve a BiteSizedTask - Introduce new virtType enum item[0]

[0] http://wiki.libvirt.org/page/BiteSizedTasks#Introduce_new_virtType_enum_item

Shivangi Dhir (3):
  conf: Add new VIR_DOMAIN_VIRT_NONE enum
  qemu: Make virtType of type virDomainVirtType
  conf: Change the description of virCapabilitiesDomainDataLookup()

 src/conf/capabilities.c  |  6 +++---
 src/conf/domain_conf.c   |  3 ++-
 src/conf/domain_conf.h   |  3 ++-
 src/qemu/qemu_monitor.c  |  2 +-
 src/qemu/qemu_monitor.h  |  2 +-
 src/qemu/qemu_monitor_json.c |  2 +-
 src/qemu/qemu_monitor_json.h |  2 +-
 src/qemu/qemu_monitor_text.c |  2 +-
 src/qemu/qemu_monitor_text.h |  2 +-
 tests/qemumonitorjsontest.c  |  2 +-
 tests/vircapstest.c  | 32 
 11 files changed, 30 insertions(+), 28 deletions(-)

-- 

1.9.1

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

[libvirt] [PATCH 1/3] client rpc: Report proper error for keepalive disconnections

2015-09-17 Thread Jiri Denemark 
Whenever a connection was closed due to keepalive timeout, we would log
a warning but the interrupted API would return rather useless generic
error:

internal error: received hangup / error event on socket

Let's report a proper keepalive timeout error and make sure it is
propagated to all pending APIs. The error should be better now:

internal error: connection closed due to keepalive timeout

Based on an old patch from Martin Kletzander.

Signed-off-by: Jiri Denemark 
---
 src/rpc/virkeepalive.c | 10 +-
 src/rpc/virnetclient.c | 12 
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/src/rpc/virkeepalive.c b/src/rpc/virkeepalive.c
index c882313..c9faf88 100644
--- a/src/rpc/virkeepalive.c
+++ b/src/rpc/virkeepalive.c
@@ -136,11 +136,11 @@ virKeepAliveTimerInternal(virKeepAlivePtr ka,
   ka, ka->client, ka->countToDeath, timeval);
 
 if (ka->countToDeath == 0) {
-VIR_WARN("No response from client %p after %d keepalive messages in"
- " %d seconds",
- ka->client,
- ka->count,
- timeval);
+VIR_DEBUG("No response from client %p after %d keepalive messages "
+  "in %d seconds",
+  ka->client, ka->count, timeval);
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("connection closed due to keepalive timeout"));
 return true;
 } else {
 ka->countToDeath--;
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index d0b96b6..6e59ea6 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -107,6 +107,7 @@ struct _virNetClient {
 virKeepAlivePtr keepalive;
 bool wantClose;
 int closeReason;
+virErrorPtr error;
 
 virNetClientCloseFunc closeCb;
 void *closeOpaque;
@@ -636,10 +637,14 @@ virNetClientMarkClose(virNetClientPtr client,
   int reason)
 {
 VIR_DEBUG("client=%p, reason=%d", client, reason);
+
 if (client->sock)
 virNetSocketRemoveIOCallback(client->sock);
+
 /* Don't override reason that's already set. */
 if (!client->wantClose) {
+if (!client->error)
+client->error = virSaveLastError();
 client->wantClose = true;
 client->closeReason = reason;
 }
@@ -670,6 +675,9 @@ virNetClientCloseLocked(virNetClientPtr client)
 client->keepalive = NULL;
 client->wantClose = false;
 
+virFreeError(client->error);
+client->error = NULL;
+
 if (ka || client->closeCb) {
 virNetClientCloseFunc closeCb = client->closeCb;
 void *closeOpaque = client->closeOpaque;
@@ -1602,6 +1610,10 @@ static int virNetClientIOEventLoop(virNetClientPtr 
client,
 }
 
  error:
+if (client->error) {
+VIR_DEBUG("error on socket: %s", client->error->message);
+virSetError(client->error);
+}
 virNetClientCallRemove(>waitDispatch, thiscall);
 virNetClientIOEventLoopPassTheBuck(client, thiscall);
 return -1;
-- 
2.5.2

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

[libvirt] [PATCH 0/3] Improve keepalive timeout handling in clients

2015-09-17 Thread Jiri Denemark 
Jiri Denemark (3):
  client rpc: Report proper error for keepalive disconnections
  client rpc: Process pending data on error
  virsh: Notify users about disconnects

 src/rpc/virkeepalive.c | 10 +-
 src/rpc/virnetclient.c | 30 +-
 tools/virsh.c  | 35 ---
 3 files changed, 62 insertions(+), 13 deletions(-)

-- 
2.5.2

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

[libvirt] [PATCH 3/3] virsh: Notify users about disconnects

2015-09-17 Thread Jiri Denemark 
After my "client rpc: Report proper error for keepalive disconnections"
patch, virsh would no long print a warning when it closes a connection
to a daemon after a keepalive timeout. Although the warning

virsh # 2015-09-15 10:59:26.729+: 642080: info :
libvirt version: 1.2.19
2015-09-15 10:59:26.729+: 642080: warning :
virKeepAliveTimerInternal:143 : No response from client
0x7efdc0a46730 after 1 keepalive messages in 2 seconds

was pretty ugly, it was still useful. This patch brings the useful part
back while making it much nicer:

virsh # error: Disconnected from qemu:///system due to keepalive timeout

Signed-off-by: Jiri Denemark 
---
 tools/virsh.c | 35 ---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index bb12dec..23436ea 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -95,12 +95,41 @@ static int disconnected; /* we may have been disconnected */
  * handler, just save the fact it was raised.
  */
 static void
-virshCatchDisconnect(virConnectPtr conn ATTRIBUTE_UNUSED,
+virshCatchDisconnect(virConnectPtr conn,
  int reason,
- void *opaque ATTRIBUTE_UNUSED)
+ void *opaque)
 {
-if (reason != VIR_CONNECT_CLOSE_REASON_CLIENT)
+if (reason != VIR_CONNECT_CLOSE_REASON_CLIENT) {
+vshControl *ctl = opaque;
+const char *str = "unknown reason";
+virErrorPtr error;
+char *uri;
+
+error = virSaveLastError();
+uri = virConnectGetURI(conn);
+
+switch ((virConnectCloseReason) reason) {
+case VIR_CONNECT_CLOSE_REASON_ERROR:
+str = N_("Disconnected from %s due to I/O error");
+break;
+case VIR_CONNECT_CLOSE_REASON_EOF:
+str = N_("Disconnected from %s due to end of file");
+break;
+case VIR_CONNECT_CLOSE_REASON_KEEPALIVE:
+str = N_("Disconnected from %s due to keepalive timeout");
+break;
+case VIR_CONNECT_CLOSE_REASON_CLIENT:
+case VIR_CONNECT_CLOSE_REASON_LAST:
+break;
+}
+vshError(ctl, _(str), NULLSTR(uri));
+
+if (error) {
+virSetError(error);
+virFreeError(error);
+}
 disconnected++;
+}
 }
 
 /* Main Function which should be used for connecting.
-- 
2.5.2

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

[libvirt] [PATCH 2/3] client rpc: Process pending data on error

2015-09-17 Thread Jiri Denemark 
Even though we hit an error in client's IO loop, we still want to
process any pending data. So instead of reporting the error right away,
we can finish the current iteration and report the error once we're done
with it. Note that the error is stored in client->error by
virNetClientMarkClose so we don't need to worry about it being reset or
rewritten by any API we call in the meantime.

Signed-off-by: Jiri Denemark 
---
 src/rpc/virnetclient.c | 18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 6e59ea6..c68da6d 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1460,6 +1460,7 @@ static int virNetClientIOEventLoop(virNetClientPtr client,
virNetClientCallPtr thiscall)
 {
 struct pollfd fds[2];
+bool error = false;
 int ret;
 
 fds[0].fd = virNetSocketGetFD(client->sock);
@@ -1551,10 +1552,11 @@ static int virNetClientIOEventLoop(virNetClientPtr 
client,
 if (virNetSocketHasCachedData(client->sock))
 fds[0].revents |= POLLIN;
 
-/* If wantClose flag is set, pretend there was an error on the socket
+/* If wantClose flag is set, pretend there was an error on the socket,
+ * but still read and process any data we received so far.
  */
 if (client->wantClose)
-fds[0].revents = POLLERR;
+error = true;
 
 if (fds[1].revents) {
 VIR_DEBUG("Woken up from poll by other thread");
@@ -1562,21 +1564,24 @@ static int virNetClientIOEventLoop(virNetClientPtr 
client,
 virReportSystemError(errno, "%s",
  _("read on wakeup fd failed"));
 virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_ERROR);
-goto error;
+error = true;
+/* Fall through to process any pending data. */
 }
 }
 
 if (fds[0].revents & POLLOUT) {
 if (virNetClientIOHandleOutput(client) < 0) {
 virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_ERROR);
-goto error;
+error = true;
+/* Fall through to process any pending data. */
 }
 }
 
 if (fds[0].revents & POLLIN) {
 if (virNetClientIOHandleInput(client) < 0) {
 virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_ERROR);
-goto error;
+error = true;
+/* Fall through to process any pending data. */
 }
 }
 
@@ -1601,6 +1606,9 @@ static int virNetClientIOEventLoop(virNetClientPtr client,
 return 1;
 }
 
+if (error)
+goto error;
+
 if (fds[0].revents & (POLLHUP | POLLERR)) {
 virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_EOF);
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-- 
2.5.2

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

Re: [libvirt] [PATCH v2 06/12] migration: refactor: introduce params version of unmanaged

2015-09-17 Thread John Ferlan 


On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> Let's put main functionality into params version of virDomainMigrateUnmanaged
> as a preparation step for merging it with virDomainMigratePeer2PeerParams.
> virDomainMigrateUnmanaged then does nothing more then just adapting arguments.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   90 -
>  1 files changed, 59 insertions(+), 31 deletions(-)
> 

This patch introduces virDomainMigrateUnmanagedParams and makes
virDomainMigrateUnmanaged a shim to make the call.

Things seem reasonable here w/r/t code motion. Here Coverity is "OK"
with the NULL in dconnuri because it's directly tied to the same "if
(flags & VIR_MIGRATE_PEER2PEER)" check.

John

> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 1631944..79ed9f8 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3339,21 +3339,32 @@ virDomainMigratePeer2PeerParams(virDomainPtr domain,
>  }
>  
>  static int
> -virDomainMigrateUnmanaged(virDomainPtr domain,
> -  const char *xmlin,
> -  unsigned int flags,
> -  const char *dname,
> -  const char *dconnuri,
> -  const char *miguri,
> -  unsigned long long bandwidth)
> +virDomainMigrateUnmanagedParams(virDomainPtr domain,
> +const char *dconnuri,
> +virTypedParameterPtr params,
> +int nparams,
> +unsigned int flags)
>  {
>  const char *uri = NULL;
> +const char *miguri = NULL;
> +const char *dname = NULL;
> +const char *xmlin = NULL;
> +unsigned long long bandwidth = 0;
>  
> -VIR_DOMAIN_DEBUG(domain,
> - "dconnuri=%s, xmlin=%s, dname=%s, miguri=%s, 
> bandwidth=%llu "
> - "flags=%x",
> - dconnuri, NULLSTR(xmlin), NULLSTR(dname), 
> NULLSTR(miguri),
> - bandwidth, flags);
> +if (virTypedParamsGetString(params, nparams,
> +VIR_MIGRATE_PARAM_URI, ) < 0 ||
> +virTypedParamsGetString(params, nparams,
> +VIR_MIGRATE_PARAM_DEST_NAME, ) < 0 ||
> +virTypedParamsGetString(params, nparams,
> +VIR_MIGRATE_PARAM_DEST_XML, ) < 0 ||
> +virTypedParamsGetULLong(params, nparams,
> +VIR_MIGRATE_PARAM_BANDWIDTH, ) < 
> 0) {
> +return -1;
> +}
> +
> +VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparams=%d, flags=%x",
> + dconnuri, params, nparams, flags);
> +VIR_TYPED_PARAMS_DEBUG(params, nparams);
>  
>  if ((flags & VIR_MIGRATE_PEER2PEER) && 
> virDomainMigrateCheckNotLocal(dconnuri) < 0)
>  return -1;
> @@ -3395,6 +3406,38 @@ virDomainMigrateUnmanaged(virDomainPtr domain,
>  }
>  }
>  
> +static int
> +virDomainMigrateUnmanaged(virDomainPtr domain,
> +  const char *xmlin,
> +  unsigned int flags,
> +  const char *dname,
> +  const char *dconnuri,
> +  const char *miguri,
> +  unsigned long long bandwidth)
> +{
> +int ret = -1;
> +virTypedParameterPtr params = NULL;
> +int nparams = 0;
> +int maxparams = 0;
> +
> +if (virTypedParamsAddString(, , ,
> +VIR_MIGRATE_PARAM_URI, miguri) < 0 ||
> +virTypedParamsAddString(, , ,
> +VIR_MIGRATE_PARAM_DEST_NAME, dname) < 0 ||
> +virTypedParamsAddString(, , ,
> +VIR_MIGRATE_PARAM_DEST_XML, xmlin) < 0 ||
> +virTypedParamsAddULLong(, , ,
> +VIR_MIGRATE_PARAM_BANDWIDTH, bandwidth) < 0)
> +goto cleanup;
> +
> +ret = virDomainMigrateUnmanagedParams(domain, dconnuri, params, nparams, 
> flags);
> +
> + cleanup:
> +virTypedParamsFree(params, nparams);
> +
> +return ret;
> +}
> +
>  /**
>   * virDomainMigrate:
>   * @domain: a domain object
> @@ -4361,10 +4404,6 @@ virDomainMigrateToURI3(virDomainPtr domain,
> VIR_MIGRATE_PARAM_DEST_NAME,
> VIR_MIGRATE_PARAM_DEST_XML,
> VIR_MIGRATE_PARAM_BANDWIDTH };
> -const char *uri = NULL;
> -const char *dname = NULL;
> -const char *dxml = NULL;
> -unsigned long long bandwidth = 0;
>  
>  VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparms=%u flags=%x",
>   NULLSTR(dconnuri), params, nparams, flags);
> @@ -4383,17 +4422,6 @@ virDomainMigrateToURI3(virDomainPtr domain,
>  compat = virTypedParamsCheck(params,

Re: [libvirt] [PATCH v2 12/12] migration: refactor: one return in forURI family functions

2015-09-17 Thread John Ferlan 


On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> May be a matter of a taste but this version with one return point in every
> function looks simplier to understand and to exetend too. Anyway after such

s/exetend/extend

> a heavy refactoring a little cleanup will not hurt.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   61 
> +++---
>  1 files changed, 28 insertions(+), 33 deletions(-)
> 

Although I know Daniel has ACK'd already - I figured I'd complete it too...

BTW: I can confirm the following will work in ToURI3:

const char *duri = (flags & VIR_MIGRATE_PEER2PEER) ? dconnuri : NULL;
...

Remove the dconnuri = NULL, replace 'dconnuri' with 'duri' in the
virDomainMigrateUnmanagedParams call and Coverity stops complaining.

Curiously, I see the *ToURI2 function has a similar construct without a
complaint, but it's calling virDomainMigrateUnmanaged and I'll assume
Coverity gets sufficiently boggled with the call path that it doesn't
matter. Your call if you want to change the *ToURI2 function to use a
local static...



Beyond that these changes seem perfectly reasonable to me as well.


John

> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 2d98997..aad2849 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -4216,6 +4216,7 @@ virDomainMigrateToURI(virDomainPtr domain,
>const char *dname,
>unsigned long bandwidth)
>  {
> +int ret = -1;
>  const char *dconnuri = NULL;
>  const char *miguri = NULL;
>  
> @@ -4225,26 +4226,24 @@ virDomainMigrateToURI(virDomainPtr domain,
>  virResetLastError();
>  
>  virCheckDomainReturn(domain, -1);
> -virCheckReadOnlyGoto(domain->conn->flags, error);
> -virCheckNonNullArgGoto(duri, error);
> +virCheckReadOnlyGoto(domain->conn->flags, cleanup);
> +virCheckNonNullArgGoto(duri, cleanup);
>  
>  if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
> -goto error;
> +goto cleanup;
>  
>  if (flags & VIR_MIGRATE_PEER2PEER)
>  dconnuri = duri;
>  else
>  miguri = duri;
>  
> -if (virDomainMigrateUnmanaged(domain, NULL, flags,
> -   dname, dconnuri, miguri, bandwidth) < 0)
> -goto error;
> -
> -return 0;
> +ret = virDomainMigrateUnmanaged(domain, NULL, flags,
> +dname, dconnuri, miguri, bandwidth);
> + cleanup:
> +if (ret < 0)
> +virDispatchError(domain->conn);
>  
> - error:
> -virDispatchError(domain->conn);
> -return -1;
> +return ret;
>  }
>  
>  
> @@ -4343,6 +4342,7 @@ virDomainMigrateToURI2(virDomainPtr domain,
> const char *dname,
> unsigned long bandwidth)
>  {
> +int ret = -1;
>  VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, miguri=%s, dxml=%s, "
>   "flags=%lx, dname=%s, bandwidth=%lu",
>   NULLSTR(dconnuri), NULLSTR(miguri), NULLSTR(dxml),
> @@ -4351,23 +4351,20 @@ virDomainMigrateToURI2(virDomainPtr domain,
>  virResetLastError();
>  
>  virCheckDomainReturn(domain, -1);
> -virCheckReadOnlyGoto(domain->conn->flags, error);
> +virCheckReadOnlyGoto(domain->conn->flags, cleanup);
>  
>  if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
> -goto error;
> +goto cleanup;
>  
>  if (!(flags & VIR_MIGRATE_PEER2PEER))
>  dconnuri = NULL;
>  
> -if (virDomainMigrateUnmanaged(domain, NULL, flags,
> -   dname, dconnuri, miguri, bandwidth) < 0)
> -goto error;
> -
> -return 0;
> -
> - error:
> -virDispatchError(domain->conn);
> -return -1;
> +ret = virDomainMigrateUnmanaged(domain, NULL, flags,
> +dname, dconnuri, miguri, bandwidth);
> + cleanup:
> +if (ret < 0)
> +virDispatchError(domain->conn);
> +return ret;
>  }
>  
>  
> @@ -4415,6 +4412,7 @@ virDomainMigrateToURI3(virDomainPtr domain,
> unsigned int nparams,
> unsigned int flags)
>  {
> +int ret = -1;
>  VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparms=%u flags=%x",
>   NULLSTR(dconnuri), params, nparams, flags);
>  VIR_TYPED_PARAMS_DEBUG(params, nparams);
> @@ -4422,23 +4420,20 @@ virDomainMigrateToURI3(virDomainPtr domain,
>  virResetLastError();
>  
>  virCheckDomainReturn(domain, -1);
> -virCheckReadOnlyGoto(domain->conn->flags, error);
> +virCheckReadOnlyGoto(domain->conn->flags, cleanup);
>  
>  if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
> -goto error;
> +goto cleanup;
>  
>  if (!(flags & VIR_MIGRATE_PEER2PEER))
>  dconnuri = NULL;
>  
> -if (virDomainMigrateUnmanagedParams(domain, dconnuri,
> -

[libvirt] [PATCH] virDomainDiskDef: Turn @device into enum

2015-09-17 Thread Michal Privoznik 
It's used as enum everywhere, so why store its value in an int?

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c   | 9 +++--
 src/conf/domain_conf.h   | 2 +-
 src/qemu/qemu_command.c  | 2 ++
 src/vmx/vmx.c| 6 +++---
 src/vmx/vmx.h| 2 +-
 src/xenconfig/xen_sxpr.c | 4 +++-
 6 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6df1618..8ccce89 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7211,11 +7211,13 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 
 device = virXMLPropString(node, "device");
 if (device) {
-if ((def->device = virDomainDiskDeviceTypeFromString(device)) < 0) {
+int dev = virDomainDiskDeviceTypeFromString(device);
+if (dev < 0) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown disk device '%s'"), device);
 goto error;
 }
+def->device = dev;
 } else {
 def->device = VIR_DOMAIN_DISK_DEVICE_DISK;
 }
@@ -18944,11 +18946,6 @@ virDomainDiskDefFormat(virBufferPtr buf,
_("unexpected disk type %d"), def->src->type);
 return -1;
 }
-if (!device) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("unexpected disk device %d"), def->device);
-return -1;
-}
 if (!bus) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected disk bus %d"), def->bus);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f043554..fe16c78 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -686,7 +686,7 @@ struct _virDomainDiskDef {
 
 virObjectPtr privateData;
 
-int device; /* enum virDomainDiskDevice */
+virDomainDiskDevice device;
 int bus; /* enum virDomainDiskBus */
 char *dst;
 int tray_status; /* enum virDomainDiskTray */
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 25f57f2..781fc4a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10135,6 +10135,8 @@ qemuBuildCommandLine(virConnectPtr conn,
 bootindex = bootDisk;
 bootDisk = 0;
 break;
+case VIR_DOMAIN_DISK_DEVICE_LAST:
+break;
 }
 
 virCommandAddArg(cmd, "-drive");
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 70cda2a..6d519b3 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -1926,8 +1926,8 @@ virVMXParseSCSIController(virConfPtr conf, int 
controller, bool *present,
 
 int
 virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr 
conf,
-int device, int busType, int controllerOrBus, int unit,
-virDomainDiskDefPtr *def, virDomainDefPtr vmdef)
+virDomainDiskDevice device, int busType, int controllerOrBus,
+int unit, virDomainDiskDefPtr *def, virDomainDefPtr vmdef)
 {
 /*
  *  device = {VIR_DOMAIN_DISK_DEVICE_DISK,
@@ -3251,7 +3251,7 @@ virVMXFormatConfig(virVMXContext *ctx, 
virDomainXMLOptionPtr xmlopt, virDomainDe
 
 break;
 
-  default:
+  case VIR_DOMAIN_DISK_DEVICE_LAST:
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported disk device type '%s'"),

virDomainDiskDeviceTypeToString(def->disks[i]->device));
diff --git a/src/vmx/vmx.h b/src/vmx/vmx.h
index e986124..16b616b 100644
--- a/src/vmx/vmx.h
+++ b/src/vmx/vmx.h
@@ -88,7 +88,7 @@ int virVMXParseSCSIController(virConfPtr conf, int 
controller, bool *present,
   int *virtualDev);
 
 int virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
-virConfPtr conf, int device, int busType,
+virConfPtr conf, virDomainDiskDevice device, int busType,
 int controllerOrBus, int unit, virDomainDiskDefPtr *def,
 virDomainDefPtr vmdef);
 
diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c
index 1d43ec1..1b2ddc6 100644
--- a/src/xenconfig/xen_sxpr.c
+++ b/src/xenconfig/xen_sxpr.c
@@ -2357,7 +2357,9 @@ xenFormatSxpr(virConnectPtr conn,
 virBufferEscapeSexpr(, "'%s')", src);
 break;
 
-default:
+case VIR_DOMAIN_DISK_DEVICE_DISK:
+case VIR_DOMAIN_DISK_DEVICE_LUN:
+case VIR_DOMAIN_DISK_DEVICE_LAST:
 break;
 }
 }
-- 
2.4.6

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

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

2015-09-17 Thread Daniel P. Berrange 
On Thu, Sep 10, 2015 at 04:20:14PM +0300, Nikolay Shirokovskiy wrote:
> As promised in previous patch.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   43 +++
>  1 files changed, 23 insertions(+), 20 deletions(-)

ACK with John's suggestion of ATTRIBUTE_NONNULL on the dconnuri
parameter


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] libxl: fix AttachDeviceConfig on hostdev type

2015-09-17 Thread Jim Fehlig 

On 09/16/2015 11:17 PM, Chunyan Liu wrote:

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;


Nice catch, ACK. Thanks for the patch! I've pushed it now.

Regards,
Jim

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

[libvirt] [PATCH 0/3] Rework our enum parsing

2015-09-17 Thread Michal Privoznik 
Only the first two patches are intended for review. The 3rd one is just an
example to show how the code base will shrink in size.

Michal Privoznik (3):
  virutil: Rename virEnum{From,To}String to virType{From,To}String
  virutil: Introduce virEnumFromString
  conf: Example for the new pattern

 src/conf/domain_conf.c   | 34 ++-
 src/libvirt_private.syms |  3 ++-
 src/util/virutil.c   | 53 ++--
 src/util/virutil.h   | 40 +---
 4 files changed, 88 insertions(+), 42 deletions(-)

-- 
2.4.6

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

[libvirt] [PATCH 1/3] virutil: Rename virEnum{From, To}String to virType{From, To}String

2015-09-17 Thread Michal Privoznik 
While the function names are more or less correct,
I will need them in the next commit. So rename these
into virType{From,To}String respectively.

Signed-off-by: Michal Privoznik 
---
 src/libvirt_private.syms |  4 ++--
 src/util/virutil.c   | 22 +++---
 src/util/virutil.h   |  8 
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5b3da83..ee7c229 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2343,8 +2343,6 @@ virUSBDeviceSetUsedBy;
 
 # util/virutil.h
 virDoubleToStr;
-virEnumFromString;
-virEnumToString;
 virFindFCHostCapableVport;
 virFindSCSIHostByPCI;
 virFormatIntDecimal;
@@ -2401,6 +2399,8 @@ virTristateBoolTypeFromString;
 virTristateBoolTypeToString;
 virTristateSwitchTypeFromString;
 virTristateSwitchTypeToString;
+virTypeFromString;
+virTypeToString;
 virUpdateSelfLastChanged;
 virValidateWWN;
 
diff --git a/src/util/virutil.c b/src/util/virutil.c
index cddc78a..3343e0d 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -419,7 +419,7 @@ virParseVersionString(const char *str, unsigned long 
*version,
 return 0;
 }
 
-int virEnumFromString(const char *const*types,
+int virTypeFromString(const char *const*types,
   unsigned int ntypes,
   const char *type)
 {
@@ -434,6 +434,16 @@ int virEnumFromString(const char *const*types,
 return -1;
 }
 
+const char *virTypeToString(const char *const*types,
+unsigned int ntypes,
+int type)
+{
+if (type < 0 || type >= ntypes)
+return NULL;
+
+return types[type];
+}
+
 /* In case thread-safe locales are available */
 #if HAVE_NEWLOCALE
 
@@ -526,16 +536,6 @@ virFormatIntDecimal(char *buf, size_t buflen, int val)
 }
 
 
-const char *virEnumToString(const char *const*types,
-unsigned int ntypes,
-int type)
-{
-if (type < 0 || type >= ntypes)
-return NULL;
-
-return types[type];
-}
-
 /* Translates a device name of the form (regex) /^[fhv]d[a-z]+[0-9]*$/
  * into the corresponding index (e.g. sda => 0, hdz => 25, vdaa => 26)
  * Note that any trailing string of digits is simply ignored.
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 53025f7..648de28 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -70,11 +70,11 @@ char *virFormatIntDecimal(char *buf, size_t buflen, int val)
 int virDiskNameToIndex(const char* str);
 char *virIndexToDiskName(int idx, const char *prefix);
 
-int virEnumFromString(const char *const*types,
+int virTypeFromString(const char *const*types,
   unsigned int ntypes,
   const char *type);
 
-const char *virEnumToString(const char *const*types,
+const char *virTypeToString(const char *const*types,
 unsigned int ntypes,
 int type);
 
@@ -82,12 +82,12 @@ const char *virEnumToString(const char *const*types,
 static const char *const name ## TypeList[] = { __VA_ARGS__ };  \
 verify(ARRAY_CARDINALITY(name ## TypeList) == lastVal); \
 const char *name ## TypeToString(int type) {\
-return virEnumToString(name ## TypeList,\
+return virTypeToString(name ## TypeList,\
ARRAY_CARDINALITY(name ## TypeList), \
type);   \
 }   \
 int name ## TypeFromString(const char *type) {  \
-return virEnumFromString(name ## TypeList,  \
+return virTypeFromString(name ## TypeList,  \
  ARRAY_CARDINALITY(name ## TypeList),   \
  type); \
 }
-- 
2.4.6

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

[libvirt] [PATCH 2/3] virutil: Introduce virEnumFromString

2015-09-17 Thread Michal Privoznik 
So, now we have VIR_ENUM_DECL() and VIR_ENUM_IMPL() macros. They
will define a pair of functions for you to convert from and to
certain enum. Unfortunately, their usage is slightly cumbersome:

  int tmp;
  virMyAwesome ma;

  if ((tmp = virMyAwesomeTypeFromString(str)) < 0) {
  virReportError();
  goto cleanup;
  }

  ma = tmp;

Ideally, we could avoid using the dummy @tmp variable:

  virMyAwesome ma;

  if (virMyAwesomeEnumFromString(str, ,
  "Unable to convert '%s'", str) < 0)
  goto cleanup;

So, the first @str is string to convert from. @ma should point to
the variable, where result is stored. Then, the third argument is
the error message to be printed if conversion fails, followed by
all the necessary arguments (message is in printf format).

Signed-off-by: Michal Privoznik 
---
 src/libvirt_private.syms |  1 +
 src/util/virutil.c   | 31 +++
 src/util/virutil.h   | 32 +---
 3 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ee7c229..730f2f8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2343,6 +2343,7 @@ virUSBDeviceSetUsedBy;
 
 # util/virutil.h
 virDoubleToStr;
+virEnumFromString;
 virFindFCHostCapableVport;
 virFindSCSIHostByPCI;
 virFormatIntDecimal;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 3343e0d..6bb9e35 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -444,6 +444,37 @@ const char *virTypeToString(const char *const*types,
 return types[type];
 }
 
+int
+virEnumFromString(const char *str,
+  int *result,
+  virEnumConvertFunc convertFunc,
+  const char *enumName,
+  const char *fmt,
+  va_list ap)
+{
+int ret = -1;
+int type;
+char *errMsg = NULL;
+
+if ((type = (convertFunc)(str)) < 0) {
+if (fmt) {
+if (virVasprintf(, fmt, ap) >= 0)
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", errMsg);
+} else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Unable to convert '%s' to %s type"),
+   str, enumName);
+}
+goto cleanup;
+}
+
+*result = type;
+ret = 0;
+ cleanup:
+VIR_FREE(errMsg);
+return ret;
+}
+
 /* In case thread-safe locales are available */
 #if HAVE_NEWLOCALE
 
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 648de28..5dd2790 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -27,6 +27,7 @@
 
 # include "internal.h"
 # include 
+# include 
 # include 
 
 # ifndef MIN
@@ -78,6 +79,16 @@ const char *virTypeToString(const char *const*types,
 unsigned int ntypes,
 int type);
 
+typedef int (*virEnumConvertFunc) (const char *str);
+
+int virEnumFromString(const char *str,
+  int *result,
+  virEnumConvertFunc convertFunc,
+  const char *enumName,
+  const char *fmt,
+  va_list ap)
+ATTRIBUTE_FMT_PRINTF(5, 0);
+
 # define VIR_ENUM_IMPL(name, lastVal, ...)   \
 static const char *const name ## TypeList[] = { __VA_ARGS__ };  \
 verify(ARRAY_CARDINALITY(name ## TypeList) == lastVal); \
@@ -90,11 +101,26 @@ const char *virTypeToString(const char *const*types,
 return virTypeFromString(name ## TypeList,  \
  ARRAY_CARDINALITY(name ## TypeList),   \
  type); \
+}   \
+int name ## EnumFromString(const char *str, int *result,\
+   const char *fmt, ...)\
+{   \
+int ret;\
+va_list ap; \
+va_start(ap, fmt);  \
+ret = virEnumFromString(str, result,\
+name ## TypeFromString, \
+#name, fmt, ap);\
+va_end(ap); \
+return ret; \
 }
 
-# define VIR_ENUM_DECL(name) \
-const char *name ## TypeToString(int type); \
-int name ## TypeFromString(const char*type);
+# define VIR_ENUM_DECL(name)\
+const char *name ## TypeToString(int type); \
+int name ##

[libvirt] [PATCH 3/3] conf: Example for the new pattern

2015-09-17 Thread Michal Privoznik 
NOT TO BE MERGED UPSTREAM

This is just an example of usage of new EnumFromString() API.
It's intentionally incomplete.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c | 34 +++---
 1 file changed, 11 insertions(+), 23 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6df1618..31b06b7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4904,11 +4904,10 @@ virDomainDeviceInfoParseXML(xmlNodePtr node,
 type = virXMLPropString(address, "type");
 
 if (type) {
-if ((info->type = virDomainDeviceAddressTypeFromString(type)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown address type '%s'"), type);
+if (virDomainDeviceAddressEnumFromString(type, >type,
+ _("unknown address type 
'%s'"),
+ type) < 0)
 goto cleanup;
-}
 } else {
 virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("No type specified for device address"));
@@ -7430,8 +7429,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 }
 
 if (ioeventfd) {
-int val;
-
 if (def->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("disk ioeventfd mode supported "
@@ -7439,13 +7436,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 goto error;
 }
 
-if ((val = virTristateSwitchTypeFromString(ioeventfd)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown disk ioeventfd mode '%s'"),
-   ioeventfd);
+if (virTristateSwitchEnumFromString(ioeventfd, >ioeventfd,
+_("unknown disk ioeventfd mode 
'%s'"),
+ioeventfd) < 0)
 goto error;
-}
-def->ioeventfd = val;
 }
 
 if (event_idx) {
@@ -7456,14 +7450,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 goto error;
 }
 
-int idx;
-if ((idx = virTristateSwitchTypeFromString(event_idx)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown disk event_idx mode '%s'"),
-   event_idx);
+if (virTristateSwitchEnumFromString(event_idx, >event_idx,
+_("unknown disk event_idx mode 
'%s'"),
+event_idx) < 0)
 goto error;
-}
-def->event_idx = idx;
 }
 
 if (copy_on_read) {
@@ -14754,11 +14744,9 @@ virDomainDefParseXML(xmlDocPtr xml,
 goto error;
 }
 
-if ((def->virtType = virDomainVirtTypeFromString(tmp)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("invalid domain type %s"), tmp);
+if (virDomainVirtEnumFromString(tmp, >virtType,
+_("invalid domain type %s"), tmp) < 0)
 goto error;
-}
 VIR_FREE(tmp);
 
 def->os.bootloader = virXPathString("string(./bootloader)", ctxt);
-- 
2.4.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-17 Thread Joao Martins 


On 09/17/2015 04:09 AM, Jim Fehlig wrote:
> 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.
> 
You're right. And I don't do that in all of the other patches of this series
that use libxl_domain_info. Tracking down these calls only libxlReconnectDomain
and libxlDomainGetInfo are missing the dispose. I will send a patch for these 
two.

>> +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.
> 
This one slipped slipped me, will fix that. I ran make syntax-check again and
there is one two other tiny issue regarding BlockStats macros, so I am including
those in v2.

>> +{
>> +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().
> 
Got it.

> 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

Re: [libvirt] [PATCH v2 0/12] migration: support all toURI and proto combos

2015-09-17 Thread Daniel P. Berrange 
On Thu, Sep 17, 2015 at 01:11:59PM +0100, Daniel P. Berrange wrote:
> On Thu, Sep 10, 2015 at 04:20:12PM +0300, Nikolay Shirokovskiy wrote:
> > Current implementation of 'toURI' migration interfaces does not support all
> > combinations of interface versions and protocol versions. For example 
> > 'toURI2'
> > with p2p flag will not migrate if driver supports only v3params proto.
> > 
> > This is not convinient as drivers that starts to support migration have to
> > manually support older versions of protocol. I guess this should be done in
> > one place, namely here.
> > 
> > Another issue is that there are a lot of code duplication in implementation 
> > of
> > toURI interfaces and it is not obvious from code how are they related.
> > 
> > This implementation uses extensible parameters as intermediate parameters
> > representation. This is possible as interfaces are done backward compatible 
> > in
> > terms of parameters and later versions supports all parameters of former
> > versions.
> > = Changes from version1
> > 
> > Patch is splitted into a set. Quite a big one as a result of the following 
> > strategy:
> > 
> > 1. each change in behaviour even subtle one deserves a separate patch. One
> >patch changes one aspect in behaviour.
> > 
> > 2. separate pure refactoring steps into patches too as rather simple 
> > refactor
> >steps could introduce many line changes. Mark such patches with 
> > 'refactor:'
> > 
> > Now every patch is easy to grasp I think.
> > 
> > The resulted cumulative patch is slightly different from first in behaviour 
> > but
> > I'm not going to describe the differece here as original patch was not 
> > reviewed
> > in details by anyone anyway )
> > 
> >  src/libvirt-domain.c |  520 
> > +-
> >  1 files changed, 216 insertions(+), 304 deletions(-)
> 
> Just a quick note to say that I haven't forgotten about this patch
> series. I'm looking to review it today/tomorrow I hope.

So I've finally run through this. The patches look fine, and I also compared
the final state, with the current code to understand the final logic we now
have. That all still looks good and has the nice new features you mention.
There's the few points John points out in review, but nothing too critical
I've seen so far.

Given we have a history of screwing up migration though, I'd like one of the
other migration experts, such as Jiri, to take a look too before we push it.

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 07/12] migration: refactor: extract parameter adaption functions

2015-09-17 Thread Daniel P. Berrange 
On Thu, Sep 10, 2015 at 04:20:19PM +0300, Nikolay Shirokovskiy wrote:
> Extract parametes adapdation and checking which is protocol dependent into
> designated functions. Leave only branching and common checks in
> virDomainMigrateUnmanagedParams.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   84 
> +-
>  1 files changed, 62 insertions(+), 22 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

Re: [libvirt] [PATCH v2 10/12] migration: refactor: prepare to reuse flag vs feature checks

2015-09-17 Thread Daniel P. Berrange 
On Thu, Sep 10, 2015 at 04:20:22PM +0300, Nikolay Shirokovskiy wrote:
> All toURI functions have same checks for flags and features compatibility for
> direct and p2p case. Let's factor this checks out before we reuse common code
> in toURI functions family. As a side affect we have a more clear code
> representation of uri passing conventions for p2p and direct cases.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |  126 +++--
>  1 files changed, 49 insertions(+), 77 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

Re: [libvirt] [PATCH v2 10/12] migration: refactor: prepare to reuse flag vs feature checks

2015-09-17 Thread John Ferlan 


On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> All toURI functions have same checks for flags and features compatibility for
> direct and p2p case. Let's factor this checks out before we reuse common code

s/this/the

> in toURI functions family. As a side affect we have a more clear code
> representation of uri passing conventions for p2p and direct cases.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |  126 +++--
>  1 files changed, 49 insertions(+), 77 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index f7d0777..483537a 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -4187,6 +4187,9 @@ virDomainMigrateToURI(virDomainPtr domain,
>const char *dname,
>unsigned long bandwidth)
>  {
> +const char *dconnuri = NULL;
> +const char *miguri = NULL;
> +
>  VIR_DOMAIN_DEBUG(domain, "duri=%p, flags=%lx, dname=%s, bandwidth=%lu",
>   NULLSTR(duri), flags, NULLSTR(dname), bandwidth);
>  
> @@ -4211,34 +4214,25 @@ virDomainMigrateToURI(virDomainPtr domain,
>  goto error;
>  }
>  
> -if (flags & VIR_MIGRATE_PEER2PEER) {
> -if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> - VIR_DRV_FEATURE_MIGRATION_P2P)) {
> -VIR_DEBUG("Using peer2peer migration");
> -if (virDomainMigrateUnmanaged(domain, NULL, flags,
> -  dname, duri, NULL, bandwidth) < 0)
> -goto error;
> -} else {
> -/* No peer to peer migration supported */
> -virReportUnsupportedError();
> -goto error;
> -}
> -} else {
> -if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> - VIR_DRV_FEATURE_MIGRATION_DIRECT)) {
> -VIR_DEBUG("Using direct migration");
> -if (virDomainMigrateUnmanaged(domain, NULL, flags,
> -   dname, NULL, duri, bandwidth) < 0)
> -goto error;
> -} else {
> -/* Cannot do a migration with only the perform step */
> -virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> -   _("direct migration is not supported by the"
> - " connection driver"));
> -goto error;
> -}
> +if (((flags & VIR_MIGRATE_PEER2PEER) &&
> +!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> +   VIR_DRV_FEATURE_MIGRATION_P2P)) ||

   

Not well aligned and shall I say a very "difficult" to read construct -
multiple ! and || with a couple of &&'s for good measure!

Can there be a

bool flg_p2p = (flags & VIR_MIGRATE_PEER2PEER);
bool drv_p2p = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver,
 domain->conn,
 VIR_DRV_FEATURE_MIGRATION_P2P);
bool dvr_direct = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver,
   domain->conn,
  VIR_DRV_FEATURE_MIGRATION_DIRECT);
const char *dconnuri = flg_p2p ? duri : NULL;
const char *miguri = !flg_p2p ? duri : NULL;

Then:

if ((flg_p2p && !drv_p2p) || (!flg_p2p && !drv_direct)) {
virReportUnsupportedError()


> +(!(flags & VIR_MIGRATE_PEER2PEER) &&
> +!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> +   VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
> +virReportUnsupportedError();
> +goto error;
>  }
>  
> +if (flags & VIR_MIGRATE_PEER2PEER)
> +dconnuri = duri;
> +else
> +miguri = duri;
> +
> +if (virDomainMigrateUnmanaged(domain, NULL, flags,
> +   dname, dconnuri, miguri, bandwidth) < 0)

  ^^
Need to move the args over...

> +goto error;
> +
>  return 0;
>  
>   error:
> @@ -4357,34 +4351,23 @@ :q

(virDomainPtr domain,
>   VIR_MIGRATE_NON_SHARED_INC,
>   error);
>  
> -if (flags & VIR_MIGRATE_PEER2PEER) {
> -if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> - VIR_DRV_FEATURE_MIGRATION_P2P)) {
> -VIR_DEBUG("Using peer2peer migration");
> -if (virDomainMigrateUnmanaged(domain, dxml, flags,
> -  dname, dconnuri, miguri, 
> bandwidth) < 0)
> -goto error;
> -} else {
> -/* No peer to peer migration supported */
> -virReportUnsupportedError();
> -goto error;
> -}
> -} else {
> -if

Re: [libvirt] [PATCH v2 08/12] migration: refactor: gather parameters compatibility checks

2015-09-17 Thread Daniel P. Berrange 
On Thu, Sep 10, 2015 at 04:20:20PM +0300, Nikolay Shirokovskiy wrote:
> Checks for migration's parameter set and support by protocol are slightly
> scattered in code. Let's put it in one place, namely every protocol function
> should check it's parameter set.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   56 -
>  1 files changed, 23 insertions(+), 33 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

Re: [libvirt] [PATCH v2 08/12] migration: refactor: gather parameters compatibility checks

2015-09-17 Thread John Ferlan 
$subj:  "migration: refactor parameter compatibility checks

On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> Checks for migration's parameter set and support by protocol are slightly

s/migration's parameter set/migration parameter sets
> scattered in code. Let's put it in one place, namely every protocol function
> should check it's parameter set.
> 

Perhaps more simply said - "Use the common virTypedParamsCheck for each
virDomainMigrate* function to check its expected params list prior to
the API making the driver call.  Thi"


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

I think the "one" (perhaps naive) mental note I make is that you add a
virTypedParamsCheck to virDomainMigrateUnmanagedProto3; however, it
seems it's unnecessary for virDomainMigratePeer2PeerParams... Yes, I see
one calls Perform3 and one calls Perform3Params

Still seems sane to me

John
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 55efd49..8b4171e 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3345,30 +3345,31 @@ virDomainMigrateUnmanagedProto2(virDomainPtr domain,
>  int nparams,
>  unsigned int flags)
>  {
> +/* uri parameter is added for direct case */
> +const char *compatParams[] = { VIR_MIGRATE_PARAM_DEST_NAME,
> +VIR_MIGRATE_PARAM_BANDWIDTH,
> +VIR_MIGRATE_PARAM_URI };
>  const char *uri = NULL;
>  const char *miguri = NULL;
>  const char *dname = NULL;
> -const char *xmlin = NULL;
>  unsigned long long bandwidth = 0;
>  
> +if (!virTypedParamsCheck(params, nparams, compatParams,
> + ARRAY_CARDINALITY(compatParams))) {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +   _("Migration does not support some of parameters."));
> +return -1;
> +}
> +
>  if (virTypedParamsGetString(params, nparams,
>  VIR_MIGRATE_PARAM_URI, ) < 0 ||
>  virTypedParamsGetString(params, nparams,
>  VIR_MIGRATE_PARAM_DEST_NAME, ) < 0 ||
> -virTypedParamsGetString(params, nparams,
> -VIR_MIGRATE_PARAM_DEST_XML, ) < 0 ||
>  virTypedParamsGetULLong(params, nparams,
>  VIR_MIGRATE_PARAM_BANDWIDTH, ) < 
> 0) {
>  return -1;
>  }
>  
> -if (xmlin) {
> -virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> -   _("Unable to change target guest XML during "
> - "migration"));
> -return -1;
> -}
> -
>  if (flags & VIR_MIGRATE_PEER2PEER) {
>  if (miguri) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -3391,11 +3392,22 @@ virDomainMigrateUnmanagedProto3(virDomainPtr domain,
>  int nparams,
>  unsigned int flags)
>  {
> +const char *compatParams[] = { VIR_MIGRATE_PARAM_URI,
> +   VIR_MIGRATE_PARAM_DEST_NAME,
> +   VIR_MIGRATE_PARAM_DEST_XML,
> +   VIR_MIGRATE_PARAM_BANDWIDTH };
>  const char *miguri = NULL;
>  const char *dname = NULL;
>  const char *xmlin = NULL;
>  unsigned long long bandwidth = 0;
>  
> +if (!virTypedParamsCheck(params, nparams, compatParams,
> + ARRAY_CARDINALITY(compatParams))) {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +   _("Migration does not support some of parameters."));
> +return -1;
> +}
> +
>  if (virTypedParamsGetString(params, nparams,
>  VIR_MIGRATE_PARAM_URI, ) < 0 ||
>  virTypedParamsGetString(params, nparams,
> @@ -4439,12 +4451,6 @@ virDomainMigrateToURI3(virDomainPtr domain,
> unsigned int nparams,
> unsigned int flags)
>  {
> -bool compat;
> -const char *compatParams[] = { VIR_MIGRATE_PARAM_URI,
> -   VIR_MIGRATE_PARAM_DEST_NAME,
> -   VIR_MIGRATE_PARAM_DEST_XML,
> -   VIR_MIGRATE_PARAM_BANDWIDTH };
> -
>  VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparms=%u flags=%x",
>   NULLSTR(dconnuri), params, nparams, flags);
>  VIR_TYPED_PARAMS_DEBUG(params, nparams);
> @@ -4459,9 +4465,6 @@ virDomainMigrateToURI3(virDomainPtr domain,
>   VIR_MIGRATE_NON_SHARED_INC,
>   error);
>  
> -compat = virTypedParamsCheck(params, nparams, compatParams,
> -

Re: [libvirt] [PATCH v2 09/12] migration: merge all proto branches into single function

2015-09-17 Thread Daniel P. Berrange 
On Thu, Sep 10, 2015 at 04:20:21PM +0300, Nikolay Shirokovskiy wrote:
> Finally on this step we get what we were aimed for - toURI{1, 2} (and
> migration{*} APIs too) now can work thru V3_PARAMS protocol. Execution path
> goes thru unchanged virDomainMigrateUnmanaged adapter function which is called
> by all target places.
> 
> Note that we keep the fact that direct migration never works
> thru V3_PARAMS proto. We can't change this aspect without
> further investigation.

Yeah, that is puzzeling - when reviewing this i wondered why our
current code doesn't support it..

> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   53 ++---
>  1 files changed, 15 insertions(+), 38 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

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

2015-09-17 Thread Daniel P. Berrange 
On Thu, Sep 10, 2015 at 04:20:17PM +0300, 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(-)

Took a while to understand the URI passing, but I think it looks sane, so 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

Re: [libvirt] [PATCH v2 06/12] migration: refactor: introduce params version of unmanaged

2015-09-17 Thread Daniel P. Berrange 
On Thu, Sep 10, 2015 at 04:20:18PM +0300, Nikolay Shirokovskiy wrote:
> Let's put main functionality into params version of virDomainMigrateUnmanaged
> as a preparation step for merging it with virDomainMigratePeer2PeerParams.
> virDomainMigrateUnmanaged then does nothing more then just adapting arguments.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   90 -
>  1 files changed, 59 insertions(+), 31 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

Re: [libvirt] [PATCH v2 09/12] migration: merge all proto branches into single function

2015-09-17 Thread John Ferlan 


On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> Finally on this step we get what we were aimed for - toURI{1, 2} (and
> migration{*} APIs too) now can work thru V3_PARAMS protocol. Execution path
> goes thru unchanged virDomainMigrateUnmanaged adapter function which is called
> by all target places.
> 
> Note that we keep the fact that direct migration never works
> thru V3_PARAMS proto. We can't change this aspect without
> further investigation.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   53 ++---
>  1 files changed, 15 insertions(+), 38 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 8b4171e..f7d0777 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3314,31 +3314,6 @@ virDomainMigrateCheckNotLocal(const char *dconnuri)
>  }
>  
>  static int
> -virDomainMigratePeer2PeerParams(virDomainPtr domain,
> -const char *dconnuri,
> -virTypedParameterPtr params,
> -int nparams,
> -unsigned int flags)
> -{
> -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 (virDomainMigrateCheckNotLocal(dconnuri) < 0)
> -return -1;
> -
> -VIR_DEBUG("Using migration protocol 3 with extensible parameters");
> -return domain->conn->driver->domainMigratePerform3Params
> -(domain, dconnuri, params, nparams,
> - NULL, 0, NULL, NULL, flags);
> -}
> -
> -static int
>  virDomainMigrateUnmanagedProto2(virDomainPtr domain,
>  const char *dconnuri,
>  virTypedParameterPtr params,
> @@ -3438,7 +3413,18 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain,
>  if ((flags & VIR_MIGRATE_PEER2PEER) && 
> virDomainMigrateCheckNotLocal(dconnuri) < 0)
>  return -1;

^^^  Note: Should have noted this in patch 5

The && should be spread across 2 lines since the line is longer than 80
cols...  It's a visual thing... There perhaps are more, this one now
stands out though.

>  
> -if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> +if ((flags & VIR_MIGRATE_PEER2PEER) &&
> +VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> + VIR_DRV_FEATURE_MIGRATION_PARAMS)) {
> +VIR_DEBUG("Using migration protocol 3 with extensible parameters");
> +if (!domain->conn->driver->domainMigratePerform3Params) {
> +virReportUnsupportedError();
> +return -1;
> +}
> +return domain->conn->driver->domainMigratePerform3Params
> +(domain, dconnuri, params, nparams,
> + NULL, 0, NULL, NULL, flags);
> +} else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
>  VIR_DRV_FEATURE_MIGRATION_V3)) {


It almost seems things could be written as:

if (flags & VIR_MIGRATE_PEER2PEER) {
if (virDomainMigrateCheckNotLocal(dconnuri) < 0)


if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
 )
else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver,
  ...
else
something's wrong
} else {
VIR_DEBUG("Using migration protocol 2");
...
}

IOW: I'm trying to isolate the dconnuri a bit more. Up to this point it
seems the assumption calling this routine was that if we support
VIR_DRV_FEATURE_MIGRATION_V3, then this would only be called if
VIR_MIGRATE_PEER2PEER was true, correct?

Not that there's anything wrong with the method you chose, I am trying
to clarify in my mind!

It seems that the code motion went well otherwise.

John


>  VIR_DEBUG("Using migration protocol 3");
>  if (!domain->conn->driver->domainMigratePerform3) {
> @@ -4474,18 +4460,9 @@ virDomainMigrateToURI3(virDomainPtr domain,
>  goto error;
>  }
>  
> -if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> - VIR_DRV_FEATURE_MIGRATION_PARAMS)) {
> -VIR_DEBUG("Using peer2peer migration with extensible 
> parameters");
> -if (virDomainMigratePeer2PeerParams(domain, dconnuri, params,
> -nparams, flags) < 0)
> -goto error;
> -} else {
> -VIR_DEBUG("Using peer2peer migration");
> -if (virDomainMigrateUnmanagedParams(domain, dconnuri, params,
> -

Re: [libvirt] [PATCH v2 12/12] migration: refactor: one return in forURI family functions

2015-09-17 Thread Daniel P. Berrange 
On Thu, Sep 10, 2015 at 04:20:24PM +0300, Nikolay Shirokovskiy wrote:
> May be a matter of a taste but this version with one return point in every
> function looks simplier to understand and to exetend too. Anyway after such
> a heavy refactoring a little cleanup will not hurt.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   61 
> +++---
>  1 files changed, 28 insertions(+), 33 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

Re: [libvirt] [PATCH v2 11/12] migration: reuse flags vs features checks in toURI family

2015-09-17 Thread Daniel P. Berrange 
On Thu, Sep 10, 2015 at 04:20:23PM +0300, Nikolay Shirokovskiy wrote:
> Introduce a new function for the check. virDomainMigrateUnmanagedParams is not
> a good candidate for this functionality as it is used by migrate family
> functions too and its have its own checks that are superset of extracted and 
> we
> don't need to check twice.
> 
> Actually name of the function is slightly misleading as there is also a check
> for consitensy of flags parameter alone. So it could be refactored further and
> reused by all migrate functions bu for now let it be a matter of a different
> patchset.
> 
> It is *not* a pure refactoring patch as it introduces offline check for all
> versions. Looks like it must be done that way and no one will be broken too.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   81 +++--
>  1 files changed, 32 insertions(+), 49 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

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

2015-09-17 Thread Daniel P. Berrange 
On Thu, Sep 10, 2015 at 04:20:16PM +0300, 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.
> ---
>  src/libvirt-domain.c |   13 -
>  1 files changed, 8 insertions(+), 5 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

Re: [libvirt] [PATCH v2 11/12] migration: reuse flags vs features checks in toURI family

2015-09-17 Thread John Ferlan 


On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> Introduce a new function for the check. virDomainMigrateUnmanagedParams is not
> a good candidate for this functionality as it is used by migrate family
> functions too and its have its own checks that are superset of extracted and 
> we
> don't need to check twice.
> 
> Actually name of the function is slightly misleading as there is also a check
> for consitensy of flags parameter alone. So it could be refactored further and

s/consitensy/consistency

> reused by all migrate functions bu for now let it be a matter of a different
> patchset.
> 
> It is *not* a pure refactoring patch as it introduces offline check for all
> versions. Looks like it must be done that way and no one will be broken too.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   81 +++--
>  1 files changed, 32 insertions(+), 49 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 483537a..2d98997 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -4108,6 +4108,35 @@ virDomainMigrate3(virDomainPtr domain,
>  return NULL;
>  }
>  
> +static
> +int virDomainMigrateUnmanagedCheckCompat(virDomainPtr domain, unsigned int 
> flags)

Should be:

static int
virDomainMigrateUnmanagedCheckCompat(virDomainPtr domain,
 unsigned int flags)

The "unsigned int flags" should be a separate line

Be sure to keep 2 blank lines between functions...

Also I now see where you lifted the changes in patch 10 from.  Makes me
wonder if the two should change slightly such that patch 10 becomes
create the new API with the only caller virDomainMigrateToURI.  Then
patch 11 becomes, modify virDomainMigrateToURI2 && URI3 to call it
rather than compressing the checks.

BTW: I still think the local duri in current patch 10 will do enough to
make it so Coverity doesn't whine.


> +{
> +VIR_EXCLUSIVE_FLAGS_RET(VIR_MIGRATE_NON_SHARED_DISK,
> +VIR_MIGRATE_NON_SHARED_INC,
> +-1);
> +
> +if (flags & VIR_MIGRATE_OFFLINE &&
> +!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> +  VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +   _("offline migration is not supported by "
> + "the source host"));
> +return -1;
> +}
> +
> +if (((flags & VIR_MIGRATE_PEER2PEER) &&
> +!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> +  VIR_DRV_FEATURE_MIGRATION_P2P)) ||
> +(!(flags & VIR_MIGRATE_PEER2PEER) &&
> +!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> +  VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
> +virReportUnsupportedError();
> +return -1;
> +}
> +
> +return 0;
> +}
> +
>  
>  /**
>   * virDomainMigrateToURI:
> @@ -4195,34 +4224,12 @@ virDomainMigrateToURI(virDomainPtr domain,
>  
>  virResetLastError();
>  
> -/* First checkout the source */

any particular reason this line gets delete in each of the 3 functions?

John

>  virCheckDomainReturn(domain, -1);
>  virCheckReadOnlyGoto(domain->conn->flags, error);
> -
>  virCheckNonNullArgGoto(duri, error);
>  
> -VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK,
> - VIR_MIGRATE_NON_SHARED_INC,
> - error);
> -
> -if (flags & VIR_MIGRATE_OFFLINE &&
> -!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> -  VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
> -virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> -   _("offline migration is not supported by "
> - "the source host"));
> -goto error;
> -}
> -
> -if (((flags & VIR_MIGRATE_PEER2PEER) &&
> -!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> -   VIR_DRV_FEATURE_MIGRATION_P2P)) ||
> -(!(flags & VIR_MIGRATE_PEER2PEER) &&
> -!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> -   VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
> -virReportUnsupportedError();
> +if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
>  goto error;
> -}
>  
>  if (flags & VIR_MIGRATE_PEER2PEER)
>  dconnuri = duri;
> @@ -4343,23 +4350,11 @@ virDomainMigrateToURI2(virDomainPtr domain,
>  
>  virResetLastError();
>  
> -/* First checkout the source */
>  virCheckDomainReturn(domain, -1);
>  virCheckReadOnlyGoto(domain->conn->flags, error);
>  
> -VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK,
> -

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

2015-09-17 Thread Daniel P. Berrange 
On Thu, Sep 10, 2015 at 04:20:15PM +0300, 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.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   14 --
>  1 files changed, 8 insertions(+), 6 deletions(-)
> 
> 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 "

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

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

2015-09-17 Thread David Mansfield 



On 09/17/2015 01:58 AM, Peter Krempa wrote:

On Wed, Sep 16, 2015 at 17:14:28 -0400, David Mansfield wrote:

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);


This cannot be just commented out since the code above is allowing only
inactive pools to be deleted and jumps to the cleanup label otherwise.
With this approach the lock would stay locked and the test driver would
deadlock the next time you are going to reference the same pool.


You're right of course. Corrected patch is attached, this time directly 
against git master.




The right fix here is to clear the 'privpool' pointer after we know that
the pool was removed.

As a note, line comments are not allowed in libvirt.


Second dumb mistake. I've been in c++ all week and should have know better.

--
Thanks,
David Mansfield


>From 08927d6ff222603e1be2a032c5fed68a5df8c68f Mon Sep 17 00:00:00 2001
From: David Mansfield 
Date: Thu, 17 Sep 2015 08:59:24 -0400
Subject: [PATCH v2] test driver: don't unlock pool after freeing it

---
 src/test/test_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 213a9a1..90ab09f 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4321,6 +4321,7 @@ testStoragePoolUndefine(virStoragePoolPtr pool)
 }
 
 virStoragePoolObjRemove(>pools, privpool);
+privpool = NULL;
 ret = 0;
 
  cleanup:
-- 
2.4.3

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

Re: [libvirt] [PATCH v2 07/12] migration: refactor: extract parameter adaption functions

2015-09-17 Thread John Ferlan 


On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> Extract parametes adapdation and checking which is protocol dependent into

s/parametes adapdation/parameter adaptation

> designated functions. Leave only branching and common checks in
> virDomainMigrateUnmanagedParams.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/libvirt-domain.c |   84 
> +-
>  1 files changed, 62 insertions(+), 22 deletions(-)
> 

More code motion which seems OK to me, although the "Proto" could be
removed from function names for just Unmanaged3 and Unmanaged2, but I
suppose that name will be changing soon too so it's perhaps moot point.

John
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 79ed9f8..55efd49 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3339,7 +3339,7 @@ virDomainMigratePeer2PeerParams(virDomainPtr domain,
>  }
>  
>  static int
> -virDomainMigrateUnmanagedParams(virDomainPtr domain,
> +virDomainMigrateUnmanagedProto2(virDomainPtr domain,
>  const char *dconnuri,
>  virTypedParameterPtr params,
>  int nparams,
> @@ -3362,6 +3362,63 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain,
>  return -1;
>  }
>  
> +if (xmlin) {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +   _("Unable to change target guest XML during "
> + "migration"));
> +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, uri, flags, dname, bandwidth);
> +}
> +
> +static int
> +virDomainMigrateUnmanagedProto3(virDomainPtr domain,
> +const char *dconnuri,
> +virTypedParameterPtr params,
> +int nparams,
> +unsigned int flags)
> +{
> +const char *miguri = NULL;
> +const char *dname = NULL;
> +const char *xmlin = NULL;
> +unsigned long long bandwidth = 0;
> +
> +if (virTypedParamsGetString(params, nparams,
> +VIR_MIGRATE_PARAM_URI, ) < 0 ||
> +virTypedParamsGetString(params, nparams,
> +VIR_MIGRATE_PARAM_DEST_NAME, ) < 0 ||
> +virTypedParamsGetString(params, nparams,
> +VIR_MIGRATE_PARAM_DEST_XML, ) < 0 ||
> +virTypedParamsGetULLong(params, nparams,
> +VIR_MIGRATE_PARAM_BANDWIDTH, ) < 
> 0) {
> +return -1;
> +}
> +
> +return domain->conn->driver->domainMigratePerform3
> +(domain, xmlin, NULL, 0, NULL, NULL, dconnuri,
> + miguri, flags, dname, bandwidth);
> +}
> +
> +static int
> +virDomainMigrateUnmanagedParams(virDomainPtr domain,
> +const char *dconnuri,
> +virTypedParameterPtr params,
> +int nparams,
> +unsigned int flags)
> +{
>  VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparams=%d, flags=%x",
>   dconnuri, params, nparams, flags);
>  VIR_TYPED_PARAMS_DEBUG(params, nparams);
> @@ -3376,33 +3433,16 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain,
>  virReportUnsupportedError();
>  return -1;
>  }
> -return domain->conn->driver->domainMigratePerform3
> -(domain, xmlin, NULL, 0, NULL, NULL, dconnuri,
> - miguri, flags, dname, bandwidth);
> +return virDomainMigrateUnmanagedProto3(domain, dconnuri,
> +   params, nparams, flags);
>  } 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 "
> - "migration"));
> -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;
>

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

2015-09-17 Thread Daniel P. Berrange 
On Wed, Sep 16, 2015 at 05:54:30PM -0400, John Ferlan wrote:
> 
> 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.

I generall agree, but given this is being refactored more in later
patches, it probably isn't worth the pain of trying to rearrange it
again.

> Nothing more beyond that. I'll let others contemplate the Plain name -
> since it's going away eventually shouldn't be a problem. It could have
> been "Static" too...

Yeah, I think its fine given it is changing more in later patches.


Broadly speaking ACK from me, with the commit message changes John
suggests. I like this particular cleanup alot !

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 5/8] qemu: Don't report false errors in migration protocol v2

2015-09-17 Thread John Ferlan 


On 09/11/2015 09:26 AM, Jiri Denemark wrote:
> Finish is the final state in v2 of our migration protocol. If something
> fails, we have no option to abort the migration and resume the original
> domain. Non fatal errors (such as failure to start guest CPUs or make
> the domain persistent) has to be treated as success. Keeping the domain
> running while reporting the failure was just asking for trouble.
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> Version 2:
> - rebased on top of changed patch 1
> 
>  src/qemu/qemu_migration.c | 21 -
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 

Well we didn't 'keep' that long ;-)  (need to look-ahead more often ;-))

Seems reasonable ACK

John
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 53444f7..35ab521 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5624,7 +5624,6 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  unsigned short port;
> -bool keep = false;
>  
>  VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, "
>"cookieout=%p, cookieoutlen=%p, flags=%lx, retcode=%d",
> @@ -5700,17 +5699,14 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>   * other hand, if we report failure, then the management 
> tools
>   * might try to restart the domain on the source side, even
>   * though the domain is actually running on the destination.
> - * Return a NULL dom pointer, and hope that this is a rare
> - * situation and management tools are smart.
> - */
> -
> -/*
> + * Pretend success and hope that this is a rare situation and
> + * management tools are smart.
> + *

Close your eyes and kiss your domain goodbye...

Too bad there wasn't a way to inhibit the mgmt tools from doing that
start if they weren't smart, but yet still fake the success


>   * However, in v3 protocol, the source VM is still available
>   * to restart during confirm() step, so we kill it off now.
>   */
> -if (!v3proto)
> -keep = true;
> -goto endjob;
> +if (v3proto)
> +goto endjob;
>  }
>  }
>  
> @@ -5738,9 +5734,8 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>   * target in paused state, in case admin can fix
>   * things up
>   */
> -if (!v3proto)
> -keep = true;
> -goto endjob;
> +if (v3proto)
> +goto endjob;
>  }
>  if (priv->job.completed) {
>  qemuDomainJobInfoUpdateTime(priv->job.completed);
> @@ -5781,7 +5776,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  }
>  
>   endjob:
> -if (!dom && !keep &&
> +if (!dom &&
>  !(flags & VIR_MIGRATE_OFFLINE) &&
>  virDomainObjIsActive(vm)) {
>  qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> 

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

Re: [libvirt] [PATCH v2 6/8] qemuDomainEventQueue: Check if event is non-NULL

2015-09-17 Thread John Ferlan 


On 09/11/2015 09:26 AM, Jiri Denemark wrote:
> Every single call to qemuDomainEventQueue() uses the following pattern:
> 
> if (event)
> qemuDomainEventQueue(driver, event);
> 
> Let's move the check for valid event to qemuDomainEventQueue and
> simplify all callers.
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> Version 2:
> - no change
> 
>  src/qemu/qemu_blockjob.c  |  6 ++--
>  src/qemu/qemu_cgroup.c|  3 +-
>  src/qemu/qemu_domain.c|  6 ++--
>  src/qemu/qemu_driver.c| 87 
> ---
>  src/qemu/qemu_hotplug.c   | 26 ++
>  src/qemu/qemu_migration.c | 24 +
>  src/qemu/qemu_process.c   | 72 +--
>  7 files changed, 78 insertions(+), 146 deletions(-)
> 

Yay!  A couple still are behind "if (event) {" in qemu_driver
(qemuDomainCreateXML, qemuDomainRevertToSnapshot) as well as an if
(event_old) and if (event_new) in qemuDomainRename


ACK - I think those can be cleaned up

John

--
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-17 Thread Fabio Kung 
On Wed, Sep 16, 2015 at 12:29 PM, Serge Hallyn  wrote:
>
> 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.

This all sgtm. A mailing list for design discussions + github issue
tracker seems to be working well for many open source projects I've
been tracking lately. Most of them are using Google Groups for their
mailing lists.

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

Re: [libvirt] [PATCH v2 2/8] qemu: Simplify qemuMigrationFinish

2015-09-17 Thread John Ferlan 


On 09/11/2015 09:26 AM, Jiri Denemark wrote:
> Offline migration migration is quite special because we don't really
> need to do anything but make the domain persistent. Let's do it
> separately from normal migration to avoid cluttering the code with
> !(flags & VIR_MIGRATE_OFFLINE).
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> Version 2:
> - rebased on top of changed patch 1 (context conflict)
> 
>  src/qemu/qemu_migration.c | 72 
> +++
>  1 file changed, 36 insertions(+), 36 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index b9e0c22..48a4476 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5660,8 +5660,14 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  /* Did the migration go as planned?  If yes, return the domain
>   * object, but if no, clean up the empty qemu process.
>   */
> -if (retcode == 0) {
> -if (!virDomainObjIsActive(vm) && !(flags & VIR_MIGRATE_OFFLINE)) {
> +if (flags & VIR_MIGRATE_OFFLINE) {
> +if (retcode != 0 ||
> +qemuMigrationPersist(driver, vm, mig) < 0)
> +goto endjob;
> +

Should this path also update mig->jobInfo if it exists?  That would have
been done previously

ACK otherwise

John

> +dom = virGetDomain(dconn, vm->def->name, vm->def->uuid);
> +} else if (retcode == 0) {
> +if (!virDomainObjIsActive(vm)) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("guest unexpectedly quit"));
>  qemuMigrationErrorReport(driver, vm->def->name);
> @@ -5678,20 +5684,17 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  }
>  }
>  
> -if (!(flags & VIR_MIGRATE_OFFLINE)) {
> -if (qemuMigrationVPAssociatePortProfiles(vm->def) < 0) {
> -qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> -VIR_QEMU_PROCESS_STOP_MIGRATED);
> -virDomainAuditStop(vm, "failed");
> -event = virDomainEventLifecycleNewFromObj(vm,
> - VIR_DOMAIN_EVENT_STOPPED,
> - 
> VIR_DOMAIN_EVENT_STOPPED_FAILED);
> -goto endjob;
> -}
> -if (mig->network)
> -if (qemuDomainMigrateOPDRelocate(driver, vm, mig) < 0)
> -VIR_WARN("unable to provide network data for 
> relocation");
> +if (qemuMigrationVPAssociatePortProfiles(vm->def) < 0) {
> +qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> +VIR_QEMU_PROCESS_STOP_MIGRATED);
> +virDomainAuditStop(vm, "failed");
> +event = virDomainEventLifecycleNewFromObj(vm,
> + VIR_DOMAIN_EVENT_STOPPED,
> + 
> VIR_DOMAIN_EVENT_STOPPED_FAILED);
> +goto endjob;
>  }
> +if (mig->network && qemuDomainMigrateOPDRelocate(driver, vm, mig) < 
> 0)
> +VIR_WARN("unable to provide network data for relocation");
>  
>  if (qemuMigrationStopNBDServer(driver, vm, mig) < 0)
>  goto endjob;
> @@ -5713,17 +5716,15 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>   * to restart during confirm() step, so we kill it off now.
>   */
>  if (v3proto) {
> -if (!(flags & VIR_MIGRATE_OFFLINE)) {
> -qemuProcessStop(driver, vm, 
> VIR_DOMAIN_SHUTOFF_FAILED,
> -VIR_QEMU_PROCESS_STOP_MIGRATED);
> -virDomainAuditStop(vm, "failed");
> -}
> +qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> +VIR_QEMU_PROCESS_STOP_MIGRATED);
> +virDomainAuditStop(vm, "failed");
>  }
>  goto endjob;
>  }
>  }
>  
> -if (!(flags & VIR_MIGRATE_PAUSED) && !(flags & VIR_MIGRATE_OFFLINE)) 
> {
> +if (!(flags & VIR_MIGRATE_PAUSED)) {
>  /* run 'cont' on the destination, which allows migration on qemu
>   * >= 0.10.6 to work properly.  This isn't strictly necessary on
>   * older qemu's, but it also doesn't hurt anything there
> @@ -5765,19 +5766,17 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  
>  dom = virGetDomain(dconn, vm->def->name, vm->def->uuid);
>  
> -if (!(flags & VIR_MIGRATE_OFFLINE)) {
> +event = virDomainEventLifecycleNewFromObj(vm,
> + VIR_DOMAIN_EVENT_RESUMED,
> + VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
> +if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
> +

Re: [libvirt] [PATCH v2 1/8] qemu: Split qemuMigrationFinish

2015-09-17 Thread John Ferlan 


On 09/11/2015 09:26 AM, Jiri Denemark wrote:
> Separate code which makes incoming domain persistent into
> qemuMigrationPersist.
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> Version 2:
> - reset vm->persistent if copying persistent def fails
> 
>  src/qemu/qemu_migration.c | 78 
> +--
>  1 file changed, 49 insertions(+), 29 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 1846239..b9e0c22 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5558,6 +5558,54 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr 
> def)
>  }
>  
>  
> +static int
> +qemuMigrationPersist(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + qemuMigrationCookiePtr mig)
> +{

Could have passed cfg - your call though.

> +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +virCapsPtr caps = NULL;
> +virDomainDefPtr vmdef;
> +virObjectEventPtr event;
> +bool newVM;
> +int ret = -1;
> +
> +if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> +goto cleanup;
> +
> +newVM = !vm->persistent;
> +vm->persistent = 1;
> +
> +if (mig->persistent)
> +vm->newDef = mig->persistent;
> +
> +vmdef = virDomainObjGetPersistentDef(caps, driver->xmlopt, vm);
> +if (!vmdef) {
> +if (newVM)
> +vm->persistent = 0;
> +goto cleanup;
> +}
> +
> +if (virDomainSaveConfig(cfg->configDir, vmdef) < 0)
> +goto cleanup;

[1]In the prior code, if virDomainSaveConfig fails, then the : "if
(newVM) vm->persistent = 0" is also set, but that isn't done there.

Whether that's an 'existing' bug wasn't clear...

Seems reasonable otherwise - ACK

John
> +
> +event = virDomainEventLifecycleNewFromObj(vm,
> +  VIR_DOMAIN_EVENT_DEFINED,
> +  newVM ?
> +  VIR_DOMAIN_EVENT_DEFINED_ADDED 
> :
> +  
> VIR_DOMAIN_EVENT_DEFINED_UPDATED);
> +if (event)
> +qemuDomainEventQueue(driver, event);
> +
> +ret = 0;
> +
> + cleanup:
> +virObjectUnref(caps);
> +virObjectUnref(cfg);
> +return ret;
> +}
> +
> +
>  virDomainPtr
>  qemuMigrationFinish(virQEMUDriverPtr driver,
>  virConnectPtr dconn,
> @@ -5572,13 +5620,11 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  {
>  virDomainPtr dom = NULL;
>  virObjectEventPtr event = NULL;
> -bool newVM = true;
>  qemuMigrationCookiePtr mig = NULL;
>  virErrorPtr orig_err = NULL;
>  int cookie_flags = 0;
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> -virCapsPtr caps = NULL;
>  unsigned short port;
>  
>  VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, "
> @@ -5589,9 +5635,6 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  port = priv->migrationPort;
>  priv->migrationPort = 0;
>  
> -if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> -goto cleanup;
> -
>  if (!qemuMigrationJobIsActive(vm, QEMU_ASYNC_JOB_MIGRATION_IN)) {
>  qemuMigrationErrorReport(driver, vm->def->name);
>  goto cleanup;
> @@ -5654,15 +5697,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  goto endjob;
>  
>  if (flags & VIR_MIGRATE_PERSIST_DEST) {
> -virDomainDefPtr vmdef;
> -if (vm->persistent)
> -newVM = false;
> -vm->persistent = 1;
> -if (mig->persistent)
> -vm->newDef = vmdef = mig->persistent;
> -else
> -vmdef = virDomainObjGetPersistentDef(caps, driver->xmlopt, 
> vm);
> -if (!vmdef || virDomainSaveConfig(cfg->configDir, vmdef) < 0) {

[1]   

> +if (qemuMigrationPersist(driver, vm, mig) < 0) {
>  /* Hmpf.  Migration was successful, but making it persistent
>   * was not.  If we report successful, then when this domain
>   * shuts down, management tools are in for a surprise.  On 
> the
> @@ -5683,23 +5718,9 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  VIR_QEMU_PROCESS_STOP_MIGRATED);
>  virDomainAuditStop(vm, "failed");
>  }
> -if (newVM)
> -vm->persistent = 0;

[1]

>  }
> -if (!vmdef)
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("can't get vmdef"));
>  goto endjob;
>  }
> -
> -event = virDomainEventLifecycleNewFromObj(vm,
> -

Re: [libvirt] [PATCH v2 3/8] qemu: Don't fail migration on save status failure

2015-09-17 Thread John Ferlan 


On 09/11/2015 09:26 AM, Jiri Denemark wrote:
> When we save status XML at the point during migration where we have
> already started the domain on destination, we can't really go back and
> abort migration. Thus the only thing we can do is to log a warning and
> report success.
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> Version 2:
> - no change
> 
>  src/qemu/qemu_migration.c | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 48a4476..3ecc1e5 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3816,10 +3816,8 @@ qemuMigrationConfirmPhase(virQEMUDriverPtr driver,
>
> VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
>  }
>  
> -if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) {
> +if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
>  VIR_WARN("Failed to save status on vm %s", vm->def->name);

Should these virResetLastError() too?  Might even be nice to "get" that
last error and (ahem) use it as part of the VIR_WARN output ;-)

Seems reasonable otherwise - ACK

John
> -goto cleanup;
> -}
>  }
>  
>   done:
> @@ -5780,10 +5778,8 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  }
>  
>  if (virDomainObjIsActive(vm) &&
> -virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) {
> +virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
>  VIR_WARN("Failed to save status on vm %s", vm->def->name);
> -goto endjob;
> -}
>  
>  /* Guest is successfully running, so cancel previous auto destroy */
>  qemuProcessAutoDestroyRemove(driver, vm);
> 

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

Re: [libvirt] [PATCH RFC 2/7] libxl: implement virDomainMemorystats

2015-09-17 Thread Jim Fehlig 

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

Introduce support for domainMemoryStats API call, which
consequently enables the use of `virsh dommemstat` command to
query for memory statistics of a domain. We support
the following statistics: balloon info, available and currently
in use. swap-in, swap-out, major-faults, minor-faults require
cooperation of the guest and thus currently not supported.

We build on the data returned from libxl_domain_info and deliver
it in the virDomainMemoryStat format.

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

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 101e4c7..43e9e47 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -4747,6 +4747,73 @@ libxlDomainGetCPUStats(virDomainPtr dom,
  return ret;
  }
  
+#define LIBXL_SET_MEMSTAT(TAG, VAL) \

+if (i < nr_stats) { \
+stats[i].tag = TAG; \
+stats[i].val = VAL; \
+i++; \
+}
+
+static int
+libxlDomainMemoryStats(virDomainPtr dom,
+   virDomainMemoryStatPtr stats,
+   unsigned int nr_stats,
+   unsigned int flags)
+{
+libxlDriverPrivatePtr driver = dom->conn->privateData;
+libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
+virDomainObjPtr vm;
+libxl_dominfo d_info;
+unsigned mem, maxmem;
+size_t i = 0;
+int ret = -1;
+
+virCheckFlags(0, -1);
+
+if (!(vm = libxlDomObjFromDomain(dom)))
+goto cleanup;
+
+if (virDomainMemoryStatsEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0)
+goto cleanup;
+
+if (!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   "%s", _("domain is not running"));
+goto endjob;
+}
+
+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 -1;


goto endjob; ?

Also, as you've already noted, another case of missing libxl_dominfo_dispose(). 
But I'll stop mentioning those now :-).


Regards,
Jim


+}
+mem = d_info.current_memkb;
+maxmem = d_info.max_memkb;
+
+LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, maxmem - mem);
+LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
+LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_RSS, mem);
+
+ret = i;
+
+ endjob:
+if (!libxlDomainObjEndJob(driver, vm)) {
+virObjectUnlock(vm);
+vm = NULL;
+}
+
+ cleanup:
+if (vm)
+virObjectUnlock(vm);
+return ret;
+}
+
+#undef LIBXL_SET_MEMSTAT
+
  static int
  libxlConnectDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int 
eventID,
 virConnectDomainEventGenericCallback 
callback,
@@ -5340,6 +5407,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
  #endif
  .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
  .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
+.domainMemoryStats = libxlDomainMemoryStats, /* 1.2.20 */
  .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.20 */
  .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
  .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 
0.9.0 */


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

Re: [libvirt] [PATCH v2 4/8] qemu: Kill domain when migration finish fails

2015-09-17 Thread John Ferlan 


On 09/11/2015 09:26 AM, Jiri Denemark wrote:
> Whenever something fails during incoming migration in Finish phase
> before we started guest CPUs, we need to kill the domain in addition to
> reporting the failure.
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> Version 2:
> - rebased on top of changed patch 1
> 
>  src/qemu/qemu_migration.c | 34 --
>  1 file changed, 12 insertions(+), 22 deletions(-)
> 

Although 'keep' is a bit vague, the code motion seems reasonable - ACK

John

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

Re: [libvirt] [PATCH v2 7/8] qemu: Queue events in migration Finish phase ASAP

2015-09-17 Thread John Ferlan 


On 09/11/2015 09:26 AM, Jiri Denemark wrote:
> For quite a long time we don't need to postpone queueing events until
> the end of the function since we no longer have the big driver lock.
> Let's make the code of qemuMigrationFinish simpler by queuing events at
> the time we generate them.
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> Version 2:
> - no change
> 
>  src/qemu/qemu_migration.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 

ACK  (although not my favorite coding style)

John

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

Re: [libvirt] [PATCH v2 8/8] qemu: Fix some corner cases in persistent migration

2015-09-17 Thread John Ferlan 


On 09/11/2015 09:26 AM, Jiri Denemark wrote:
> When persistently migrating a domain to a destination host where the
> same domain already exists (i.e., it is persistent and shutdown at the
> destination), we would happily through away the original persistent

s/through/throw

> definition without properly freeing it. And when updating the definition
> fails for some reason we don't properly revert to the original state
> leaving the domain broken.
> 
> In addition to fixing these issues, the patch also makes sure the domain
> definition parsed from a migration cookie is either used or freed.
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> Version 2:
> - new patch
> 
>  src/qemu/qemu_migration.c | 56 
> +++
>  1 file changed, 37 insertions(+), 19 deletions(-)
> 

Ran using my Coverity checker...

One issue - in qemuMigrationPersist can get to 'cleanup:' calling
qemuMigrationCookieGetPersistent when 'mig == NULL' from either the goto
in the "if (!qemuMigrationJobIsActive(vm...)" or "if (!(mig =
qemuMigrationEatCookie(driver, ..." paths


> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index c761657..1d556eb 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -525,6 +525,19 @@ qemuMigrationCookieAddPersistent(qemuMigrationCookiePtr 
> mig,
>  }
>  
>  
> +static virDomainDefPtr
> +qemuMigrationCookieGetPersistent(qemuMigrationCookiePtr mig)
> +{
> +virDomainDefPtr def = mig->persistent;
> +
> +mig->persistent = NULL;
> +mig->flags &= ~QEMU_MIGRATION_COOKIE_PERSISTENT;
> +mig->flagsMandatory &= ~QEMU_MIGRATION_COOKIE_PERSISTENT;
> +
> +return def;
> +}
> +
> +
>  static int
>  qemuMigrationCookieAddNetwork(qemuMigrationCookiePtr mig,
>virQEMUDriverPtr driver,
> @@ -5554,47 +5567,51 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr 
> def)
>  static int
>  qemuMigrationPersist(virQEMUDriverPtr driver,
>   virDomainObjPtr vm,
> - qemuMigrationCookiePtr mig)
> + qemuMigrationCookiePtr mig,
> + bool ignoreSaveError)
>  {
>  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  virCapsPtr caps = NULL;
>  virDomainDefPtr vmdef;
> +virDomainDefPtr oldDef = NULL;
> +unsigned int oldPersist = vm->persistent;
>  virObjectEventPtr event;
> -bool newVM;
>  int ret = -1;
>  
>  if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
>  goto cleanup;
>  
> -newVM = !vm->persistent;
>  vm->persistent = 1;
> +oldDef = vm->newDef;
> +vm->newDef = qemuMigrationCookieGetPersistent(mig);
>  
> -if (mig->persistent)
> -vm->newDef = mig->persistent;
> +if (!(vmdef = virDomainObjGetPersistentDef(caps, driver->xmlopt, vm)))
> +goto error;
>  
> -vmdef = virDomainObjGetPersistentDef(caps, driver->xmlopt, vm);
> -if (!vmdef) {
> -if (newVM)
> -vm->persistent = 0;
> -goto cleanup;
> -}
> -
> -if (virDomainSaveConfig(cfg->configDir, vmdef) < 0)
> -goto cleanup;
> +if (virDomainSaveConfig(cfg->configDir, vmdef) < 0 && !ignoreSaveError)
> +goto error;
>  
>  event = virDomainEventLifecycleNewFromObj(vm,
>VIR_DOMAIN_EVENT_DEFINED,
> -  newVM ?
> -  VIR_DOMAIN_EVENT_DEFINED_ADDED 
> :
> -  
> VIR_DOMAIN_EVENT_DEFINED_UPDATED);
> +  oldPersist ?
> +  
> VIR_DOMAIN_EVENT_DEFINED_UPDATED :
> +  
> VIR_DOMAIN_EVENT_DEFINED_ADDED);
>  qemuDomainEventQueue(driver, event);
>  
>  ret = 0;
>  
>   cleanup:
> +virDomainDefFree(oldDef);
>  virObjectUnref(caps);
>  virObjectUnref(cfg);
>  return ret;
> +
> + error:
> +virDomainDefFree(vm->newDef);
> +vm->persistent = oldPersist;

This set of changes resolves what I pointed out in patch 1 regarding
setting vm->persistent

> +vm->newDef = oldDef;
> +oldDef = NULL;
> +goto cleanup;
>  }
>  
>  
> @@ -5653,7 +5670,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>   */
>  if (flags & VIR_MIGRATE_OFFLINE) {
>  if (retcode != 0 ||
> -qemuMigrationPersist(driver, vm, mig) < 0)
> +qemuMigrationPersist(driver, vm, mig, false) < 0)
>  goto endjob;
>  
>  dom = virGetDomain(dconn, vm->def->name, vm->def->uuid);
> @@ -5685,7 +5702,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  goto endjob;
>  
>  if (flags & VIR_MIGRATE_PERSIST_DEST) {
> -if (qemuMigrationPersist(driver, vm, mig) < 0) {
> +if (qemuMigrationPersist(driver, vm,