Re: [libvirt] [PATCH LIBVIRT v1 1/2] libxl: Correct value for xendConfigVersion to xen{Parse, Format}ConfigCommon

2015-12-03 Thread Jim Fehlig 
On 11/26/2015 09:59 AM, Ian Campbell wrote:
> libxlConnectDomainXMLFromNative calls both xenParseXM and xenParseXL
> with cfg->verInfo->xen_version_major, however AFAICT they both (either
> inherently, or through there use of xenParseConfigCommon expect a
> value from xenConfigVersionEnum (which does not correspond to
> xen_version_major).

I recall being a little apprehensive about using xen_version_major when writing
the code, and apparently that was justified. But now that we are revisiting
this, I wonder if we care about these old xend config versions at all. Is anyone
even running Xen 3.0.x, or 3.1.x, particularly with a newish libvirt? IMO we
could drop all of this xend config nonsense and go with the code associated with
the last xend config version, i.e. XEND_CONFIG_VERSION_3_1_0.

And while mentioning dropping support for these old xend config versions, do I
dare mention dropping the xend driver altogether? :-) It will certainly need to
be done some day. Question is whether that's a bit premature now.

Regards,
Jim

>
> The actual xend backend passes priv->xendConfigVersion, which seems to
> have been gotten from xend and I assume conforms to that enum, via the
> node/xend_config_format value in the xend sxp.
>
> Add a new value to that enum, XEND_CONFIG_VERSION_4_0_0, on the basis
> that the xl syntax was originally based on (and intended to be
> compatible with) xm circa that point in time and that I will shortly
> want to add handling of a cfg file difference which occured in xend in
> 4.0.0 (maxvcpus instead of vpu_avail bitmask).
>
> Pass this from every caller of xen{Parse,Format}X? in the libxl
> driver.
>
> Update xlconfigtest to pass this new value, and regenerate the output
> files with that (all of the changes are expected AFAICT).
>
> The enum and the parameters are already slightly misnamed now (since
> they kind-of apply to xl too), but I didn't go through and rename
> them. In the future we may want to add new values to the enum to
> reflect evolution of the xl cfg syntax (xend was essentially static
> from 4.0 until it was removed), at that point renaming should probably
> be considered.
>
> Note also that xend's xend_config_format remained at 4 until it's
> removal in Xen 4.5, so there will be no actual change in behaviour for
> xm/xend here.
>
> Signed-off-by: Ian Campbell 
> ---
>  src/libxl/libxl_driver.c|  8 
>  src/xenconfig/xen_sxpr.h|  1 +
>  tests/xlconfigdata/test-fullvirt-direct-kernel-boot.cfg |  3 ++-
>  tests/xlconfigdata/test-fullvirt-direct-kernel-boot.xml |  2 +-
>  tests/xlconfigdata/test-fullvirt-multiusb.cfg   |  3 ++-
>  tests/xlconfigdata/test-fullvirt-multiusb.xml   |  2 +-
>  tests/xlconfigdata/test-new-disk.cfg|  3 ++-
>  tests/xlconfigdata/test-new-disk.xml|  2 +-
>  tests/xlconfigdata/test-spice-features.cfg  |  3 ++-
>  tests/xlconfigdata/test-spice-features.xml  |  2 +-
>  tests/xlconfigdata/test-spice.cfg   |  3 ++-
>  tests/xlconfigdata/test-spice.xml   |  2 +-
>  tests/xlconfigtest.c| 10 +-
>  13 files changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 35d7fae..892ae44 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -2587,14 +2587,14 @@ libxlConnectDomainXMLFromNative(virConnectPtr conn,
>  goto cleanup;
>  if (!(def = xenParseXL(conf,
> cfg->caps,
> -   cfg->verInfo->xen_version_major)))
> +   XEND_CONFIG_VERSION_4_0_0)))
>  goto cleanup;
>  } else if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XM)) {
>  if (!(conf = virConfReadMem(nativeConfig, strlen(nativeConfig), 0)))
>  goto cleanup;
>  
>  if (!(def = xenParseXM(conf,
> -   cfg->verInfo->xen_version_major,
> +   XEND_CONFIG_VERSION_4_0_0,
> cfg->caps)))
>  goto cleanup;
>  } else if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_SEXPR)) {
> @@ -2647,10 +2647,10 @@ libxlConnectDomainXMLToNative(virConnectPtr conn, 
> const char * nativeFormat,
>  goto cleanup;
>  
>  if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XL)) {
> -if (!(conf = xenFormatXL(def, conn, 
> cfg->verInfo->xen_version_major)))
> +if (!(conf = xenFormatXL(def, conn, XEND_CONFIG_VERSION_4_0_0)))
>  goto cleanup;
>  } else if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XM)) {
> -if (!(conf = xenFormatXM(conn, def, 
> cfg->verInfo->xen_version_major)))
> +if (!(conf = xenFormatXM(conn, def, XEND_CONFIG_VERSION_4_0_0)))
>  goto cleanup;
>  } else {
>  
> diff --git

Re: [libvirt] libvirt to report cpuWait of a guest.

2015-12-03 Thread Daniel P. Berrange 
On Thu, Dec 03, 2015 at 11:24:08AM +0200, Avi Weit wrote:
> Hi,
> 
> We are working on fixing CPU utilization calculation of guests (VMs) in 
> Ceilometer metering service of OpenStack.
> 
> Per that fix, we retrieve, at the host level, se.statistics.wait_sum (or 
> se.wait_sum in old versions) Linux kernel schedstat counter of the 
> KVM/QEMU threads ? that represent the VM. That counter denotes the time in 
> milliseconds the thread 'wants' to run (but not actually running because 
> of a contention).
> 
> However, as Daniel Berrange mentioned in the fix review, it would be 
> better to retrieve that information from libvirt instead and he asked me 
> to raise this in this mailing list.
> 
> I could not notice that libvirt exposes such a counter.
> 
> It would be very good if virDomainInfo (
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainInfo) can be 
> added with cpuWait in addition to cpuTime it currently has so that cpuWait 
> will denote the total *time* the VM *wants* to run.
> 
> Adding cpuWait to any other location of domain is fine as well.

We can't modify the virDomainInfo struct becase that would be an ABI
incompatible change. Fortunately we have a better API that is extensible
that we can use - virDomainListGetStats() with the flag
VIR_DOMAIN_STATS_VCPU currently returns per-VCPU information - the
state (running or not) and the CPU time:

# virsh domstats --vcpu demo
Domain: 'demo'
  vcpu.current=3
  vcpu.maximum=3
  vcpu.0.state=1
  vcpu.0.time=10713000
  vcpu.1.state=1
  vcpu.1.time=0
  vcpu.2.state=1
  vcpu.2.time=0


We could easily extend this to add vcpu.NNN.wait=X data of the kind
you request

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] bridge: check for invalid MAC in networkGetDHCPLeases

2015-12-03 Thread Pavel Hrdina 
On Thu, Dec 03, 2015 at 11:03:49AM +0100, Ján Tomko wrote:
> Instead of comparing garbage strings against real MAC addresses,
> introduce an error mesage for unparsable ones:
> 
> $ virsh net-dhcp-leases default  --mac t12
> error: Failed to get leases info for default
> error: invalid MAC address: t12
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1261432
> ---
>  src/network/bridge_driver.c | 7 +++
>  1 file changed, 7 insertions(+)
> 

ACK and safe for release.

Pavel

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

[libvirt] [PATCH] bridge: check for invalid MAC in networkGetDHCPLeases

2015-12-03 Thread Ján Tomko 
Instead of comparing garbage strings against real MAC addresses,
introduce an error mesage for unparsable ones:

$ virsh net-dhcp-leases default  --mac t12
error: Failed to get leases info for default
error: invalid MAC address: t12

https://bugzilla.redhat.com/show_bug.cgi?id=1261432
---
 src/network/bridge_driver.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index a0f6494..362e294 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -3648,9 +3648,16 @@ networkGetDHCPLeases(virNetworkPtr network,
 virNetworkDHCPLeasePtr lease = NULL;
 virNetworkDHCPLeasePtr *leases_ret = NULL;
 virNetworkObjPtr obj;
+virMacAddr mac_addr;
 
 virCheckFlags(0, -1);
 
+/* only to check if the MAC is valid */
+if (mac && virMacAddrParse(mac, _addr) < 0) {
+virReportError(VIR_ERR_INVALID_MAC, "%s", mac);
+return -1;
+}
+
 if (!(obj = networkObjFromNetwork(network)))
 return -1;
 
-- 
2.4.6

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

[libvirt] blockcopy qemu fail and libvirt

2015-12-03 Thread Vasiliy Tolstov 
Hi, i'm use qemu 2.4.0 and libvirt 1.2.16 and try blockcopy to migrate vm disk

virsh -c qemu+ssh://root@xxx/system blockcopy domain sda /dev/nbd2
--wait --pivot

libvirt says:
Successfully pivoted
2015-12-01 14:37:18.188+: 18288: info : libvirt version: 1.2.16
2015-12-01 14:37:18.188+: 18288: warning :
virEventPollUpdateTimeout:268 : Ignoring invalid update timer -1

but virsh command returns zero exit code

qemu blockcopy failed and target device contains zeroes.

i see some qemu git commits about block/migration and block that can
fix this isse, does qemu-devs confirm that this bug is fixed?

Also libvirt devs - do you know about this libvirt issue (when virsh
doing blockcopy and qemu process does not success complete blockcopy)
?

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru

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

Re: [libvirt] [PATCH] libxl: add support for mounts in HVM guests

2015-12-03 Thread Michal Privoznik 
On 02.12.2015 09:15, Cedric Bosdonnat wrote:
> On Tue, 2015-12-01 at 21:59 -0700, Jim Fehlig wrote:
>> On 12/01/2015 08:04 AM, Cédric Bosdonnat wrote:
>>> Xen HVM guests are running a QEMU, libxl allows passing QEMU parameters
>>> in the d_config.b_info.extra list. Take advantage of that to implement
>>> support for 9pfs mounts. For this to work, xen's qemu needs to be built
>>> with virtfs enabled, or an upstream qemu needs to be used.
>>
>> What if the qemu doesn't have virtfs enabled? Does it fail to start? Is a
>> reasonable error returned to the user or available in
>> /var/log/libvirt/libxl/libxl-driver.log or /var/log/xen/qemu-dm-.log?
>> The reason for a qemu start failure can usually be found in
>> /var/log/xen/qemu-dm-.log. If a reasonable error is reported there, I
>> think we are fine.
> 
> The domain fails to start in that case, but the error isn't
> understandable (even a developer like me couldn't figure out what it
> was).
> 
> I'll see if I can add a test for that to fail earlier with a meaningful
> error message.

Please do.

> 
>>> ---
>>>  Note that two functions have been picked from the qemu driver's code.
>>>
>>>  src/libxl/libxl_conf.c | 174 
>>> +
>>>  1 file changed, 174 insertions(+)
>>>
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index 4eed5ca..4437e8f 100644
>>> --- a/src/libxl/libxl_conf.c
>>> +++ b/src/libxl/libxl_conf.c
>>> @@ -55,6 +55,7 @@ VIR_LOG_INIT("libxl.libxl_conf");
>>>  /* see xen-unstable.hg/xen/include/asm-x86/cpufeature.h */
>>>  #define LIBXL_X86_FEATURE_PAE_MASK 0x40
>>>  
>>> +#define LIBXL_FSDEV_HOST_PREFIX "fsdev-"
>>>  
>>>  struct guest_arch {
>>>  virArch arch;
>>> @@ -84,6 +85,15 @@ static int libxlConfigOnceInit(void)
>>>  
>>>  VIR_ONCE_GLOBAL_INIT(libxlConfig)
>>>  
>>> +VIR_ENUM_DECL(libxlDomainFSDriver)
>>> +VIR_ENUM_IMPL(libxlDomainFSDriver, VIR_DOMAIN_FS_DRIVER_TYPE_LAST,
>>> +  "local",
>>> +  "local",
>>> +  "handle",
>>> +  NULL,
>>> +  NULL,
>>> +  NULL);
>>> +
>>>  static void
>>>  libxlDriverConfigDispose(void *obj)
>>>  {
>>> @@ -1854,6 +1864,167 @@ libxlMakeCapabilities(libxl_ctx *ctx)
>>>  return NULL;
>>>  }
>>>  
>>> +static char *
>>> +libxlBuildQemuFSStr(virDomainFSDefPtr fs)
>>> +{
>>> +virBuffer opt = VIR_BUFFER_INITIALIZER;
>>> +const char *driver = libxlDomainFSDriverTypeToString(fs->fsdriver);
>>> +const char *wrpolicy = virDomainFSWrpolicyTypeToString(fs->wrpolicy);
>>> +
>>> +if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +   _("only supports mount filesystem type"));
>>> +goto error;
>>> +}
>>> +
>>> +if (!driver) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +   _("Filesystem driver type not supported"));
>>> +goto error;
>>> +}
>>> +virBufferAdd(, driver, -1);
>>> +
>>> +if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PATH ||
>>> +fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) {
>>> +if (fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_MAPPED) {
>>> +virBufferAddLit(, ",security_model=mapped");
>>> +} else if (fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH) 
>>> {
>>> +virBufferAddLit(, ",security_model=passthrough");
>>> +} else if (fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_SQUASH) {
>>> +virBufferAddLit(, ",security_model=none");
>>> +}
>>> +} else {
>>> +/* For other fs drivers, default(passthru) should always
>>> + * be supported */
>>> +if (fs->accessmode != VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +   _("only supports passthrough accessmode"));
>>> +goto error;
>>> +}
>>> +}
>>> +
>>> +if (fs->wrpolicy)
>>> +virBufferAsprintf(, ",writeout=%s", wrpolicy);
>>> +
>>> +virBufferAsprintf(, ",id=%s%s", LIBXL_FSDEV_HOST_PREFIX, 
>>> fs->info.alias);
>>> +virBufferAsprintf(, ",path=%s", fs->src);
>>> +
>>> +if (fs->readonly)
>>> +virBufferAddLit(, ",readonly");
>>> +
>>> +if (virBufferCheckError() < 0)
>>> +goto error;
>>> +
>>> +return virBufferContentAndReset();
>>> +
>>> + error:
>>> +virBufferFreeAndReset();
>>> +return NULL;
>>> +}
>>> +
>>> +
>>> +static char *
>>> +libxlBuildQemuFSDevStr(virDomainFSDefPtr fs)
>>> +{
>>> +virBuffer opt = VIR_BUFFER_INITIALIZER;
>>> +
>>> +if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +   _("can only passthrough directories"));
>>> +goto error;
>>> +}
>>> +
>>> +if (fs->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)
>>> +virBufferAddLit(, "virtio-9p-ccw");
>>> +else

[libvirt] libvirt to report cpuWait of a guest.

2015-12-03 Thread Avi Weit 
Hi,

We are working on fixing CPU utilization calculation of guests (VMs) in 
Ceilometer metering service of OpenStack.

Per that fix, we retrieve, at the host level, se.statistics.wait_sum (or 
se.wait_sum in old versions) Linux kernel schedstat counter of the 
KVM/QEMU threads ? that represent the VM. That counter denotes the time in 
milliseconds the thread 'wants' to run (but not actually running because 
of a contention).

However, as Daniel Berrange mentioned in the fix review, it would be 
better to retrieve that information from libvirt instead and he asked me 
to raise this in this mailing list.

I could not notice that libvirt exposes such a counter.

It would be very good if virDomainInfo (
https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainInfo) can be 
added with cpuWait in addition to cpuTime it currently has so that cpuWait 
will denote the total *time* the VM *wants* to run.

Adding cpuWait to any other location of domain is fine as well.

Thanks,
Avi Weit

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

[libvirt] [PATCH v7 13/13] qemu: qemuDiskPathToAlias quorum support

2015-12-03 Thread Matthias Gatto 
By adding quorum support to qemuDiskPathToAlias, we're adding support to
qemuDomainGetBlkioParameters, which was returning an error when the domain
was active.

Signed-off-by: Matthias Gatto 
---
 src/qemu/qemu_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 44ce90f..69dfed6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16126,7 +16126,7 @@ qemuDiskPathToAlias(virDomainObjPtr vm, const char 
*path, int *idxret)
 if (idxret)
 *idxret = idx;
 
-if (virDomainDiskGetSource(disk)) {
+if (virDomainDiskGetSource(disk) || virStorageSourceIsRAID(disk->src)) {
 if (virAsprintf(, "drive-%s", disk->info.alias) < 0)
 return NULL;
 }
-- 
2.6.2

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

[libvirt] [PATCH v7 02/13] virstoragefile: Always use virStorageSourceGetBackingStore to get backing store

2015-12-03 Thread Matthias Gatto 
Uniformize backing store usage by calling virStorageSourceGetBackingStore
instead of setting backing store manually.

Signed-off-by: Matthias Gatto 
Signed-off-by: John Ferlan 
---
 src/conf/domain_conf.c|  7 ---
 src/conf/storage_conf.c   |  6 +++---
 src/qemu/qemu_cgroup.c|  4 ++--
 src/qemu/qemu_domain.c|  2 +-
 src/qemu/qemu_driver.c| 18 +
 src/qemu/qemu_monitor_json.c  |  4 +++-
 src/security/security_dac.c   |  2 +-
 src/security/security_selinux.c   |  4 ++--
 src/security/virt-aa-helper.c |  2 +-
 src/storage/storage_backend.c | 22 
 src/storage/storage_backend_fs.c  | 38 ---
 src/storage/storage_backend_gluster.c |  8 +---
 src/storage/storage_backend_logical.c | 12 +++
 src/util/virstoragefile.c | 20 +-
 tests/virstoragetest.c| 14 ++---
 15 files changed, 93 insertions(+), 70 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e6102a0..5b413b5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18625,7 +18625,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf,
 /* We currently don't output seclabels for backing chain element */
 if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 ||
 virDomainDiskBackingStoreFormat(buf,
-backingStore->backingStore,
+
virStorageSourceGetBackingStore(backingStore, 0),
 backingStore->backingStoreRaw,
 idx + 1) < 0)
 return -1;
@@ -18746,7 +18746,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
 /* Don't format backingStore to inactive XMLs until the code for
  * persistent storage of backing chains is ready. */
 if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) &&
-virDomainDiskBackingStoreFormat(buf, def->src->backingStore,
+virDomainDiskBackingStoreFormat(buf,
+
virStorageSourceGetBackingStore(def->src, 0),
 def->src->backingStoreRaw, 1) < 0)
 return -1;
 
@@ -22714,7 +22715,7 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
 }
 }
 
-for (tmp = disk->src; tmp; tmp = tmp->backingStore) {
+for (tmp = disk->src; tmp; tmp = virStorageSourceGetBackingStore(tmp, 0)) {
 int actualType = virStorageSourceGetActualType(tmp);
 /* execute the callback only for local storage */
 if (actualType != VIR_STORAGE_TYPE_NETWORK &&
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 9b8abea..d048e39 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1330,7 +1330,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
 if (virStorageSize(unit, capacity, >target.capacity) < 0)
 goto error;
 } else if (!(flags & VIR_VOL_XML_PARSE_NO_CAPACITY) &&
-   !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) && 
ret->target.backingStore)) {
+   !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) && 
virStorageSourceGetBackingStore(>target, 0))) {
 virReportError(VIR_ERR_XML_ERROR, "%s", _("missing capacity element"));
 goto error;
 }
@@ -1644,9 +1644,9 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool,
  >target, "target") < 0)
 goto cleanup;
 
-if (def->target.backingStore &&
+if (virStorageSourceGetBackingStore(>target, 0) &&
 virStorageVolTargetDefFormat(options, ,
- def->target.backingStore,
+ 
virStorageSourceGetBackingStore(>target, 0),
  "backingStore") < 0)
 goto cleanup;
 
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index b52ce3a..6405944 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -121,7 +121,7 @@ qemuSetupDiskCgroup(virDomainObjPtr vm,
 virStorageSourcePtr next;
 bool forceReadonly = false;
 
-for (next = disk->src; next; next = next->backingStore) {
+for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 
0)) {
 if (qemuSetImageCgroupInternal(vm, next, false, forceReadonly) < 0)
 return -1;
 
@@ -139,7 +139,7 @@ qemuTeardownDiskCgroup(virDomainObjPtr vm,
 {
 virStorageSourcePtr next;
 
-for (next = disk->src; next; next = next->backingStore) {
+for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 
0)) {
 if (qemuSetImageCgroup(vm, next, true) < 0)
 return -1;
 }
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ed21245..28bbe3f 100644
---

[libvirt] [PATCH v7 04/13] virstoragefile: Always use virStorageSourceSetBackingStore to set backing store

2015-12-03 Thread Matthias Gatto 
Replace the parts of the code where a backing store is set manually
with virStorageSourceSetBackingStore

Signed-off-by: Matthias Gatto 
Signed-off-by: John Ferlan 
---
 src/conf/domain_conf.c|  3 ++-
 src/conf/storage_conf.c   | 17 ++---
 src/qemu/qemu_driver.c| 17 +++--
 src/storage/storage_backend_fs.c  |  7 +--
 src/storage/storage_backend_gluster.c |  5 +++--
 src/storage/storage_backend_logical.c |  5 +++--
 src/storage/storage_driver.c  |  3 ++-
 src/util/virstoragefile.c |  8 +---
 tests/virstoragetest.c|  4 ++--
 9 files changed, 43 insertions(+), 26 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5b413b5..d146811 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6359,7 +6359,8 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
 virDomainDiskBackingStoreParse(ctxt, backingStore) < 0)
 goto cleanup;
 
-src->backingStore = backingStore;
+if (virStorageSourceSetBackingStore(src, backingStore, 0) < 0)
+goto cleanup;
 ret = 0;
 
  cleanup:
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index d048e39..b9db5eb 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1259,6 +1259,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
 char *capacity = NULL;
 char *unit = NULL;
 char *backingStore = NULL;
+virStorageSourcePtr backingStorePtr;
 xmlNodePtr node;
 xmlNodePtr *nodes = NULL;
 size_t i;
@@ -1295,20 +1296,22 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
 }
 
 if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) {
-if (VIR_ALLOC(ret->target.backingStore) < 0)
+if (VIR_ALLOC(backingStorePtr) < 0)
 goto error;
 
-ret->target.backingStore->path = backingStore;
+if (virStorageSourceSetBackingStore(>target, backingStorePtr, 0) 
< 0)
+goto error;
+backingStorePtr->path = backingStore;
 backingStore = NULL;
 
 if (options->formatFromString) {
 char *format = 
virXPathString("string(./backingStore/format/@type)", ctxt);
 if (format == NULL)
-ret->target.backingStore->format = options->defaultFormat;
+backingStorePtr->format = options->defaultFormat;
 else
-ret->target.backingStore->format = 
(options->formatFromString)(format);
+backingStorePtr->format = (options->formatFromString)(format);
 
-if (ret->target.backingStore->format < 0) {
+if (backingStorePtr->format < 0) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown volume format type %s"), format);
 VIR_FREE(format);
@@ -1317,9 +1320,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
 VIR_FREE(format);
 }
 
-if (VIR_ALLOC(ret->target.backingStore->perms) < 0)
+if (VIR_ALLOC(backingStorePtr->perms) < 0)
 goto error;
-if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms,
+if (virStorageDefParsePerms(ctxt, backingStorePtr->perms,
 "./backingStore/permissions") < 0)
 goto error;
 }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8ba7566..edfd8e6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14261,13 +14261,18 @@ 
qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
 /* Update vm in place to match changes.  */
 need_unlink = false;
 
-newDiskSrc->backingStore = disk->src;
-disk->src = newDiskSrc;
+if (virStorageSourceSetBackingStore(newDiskSrc, disk->src, 0) < 0) {
+ret = -1;
+goto cleanup;
+}
 newDiskSrc = NULL;
 
 if (persistDisk) {
-persistDiskSrc->backingStore = persistDisk->src;
-persistDisk->src = persistDiskSrc;
+if (virStorageSourceSetBackingStore(persistDiskSrc,
+persistDisk->src, 0) < 0) {
+ret = -1;
+goto cleanup;
+}
 persistDiskSrc = NULL;
 }
 
@@ -14310,13 +14315,13 @@ 
qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver,
 /* Update vm in place to match changes. */
 tmp = disk->src;
 disk->src = virStorageSourceGetBackingStore(tmp, 0);
-tmp->backingStore = NULL;
+ignore_value(virStorageSourceSetBackingStore(tmp, NULL, 0));
 virStorageSourceFree(tmp);
 
 if (persistDisk) {
 tmp = persistDisk->src;
 persistDisk->src = virStorageSourceGetBackingStore(tmp, 0);
-tmp->backingStore = NULL;
+ignore_value(virStorageSourceSetBackingStore(tmp, NULL, 0));
 virStorageSourceFree(tmp);
 }
 }
diff --git

[libvirt] [PATCH v7 11/13] qemu: Forbid blocks operations with quorum

2015-12-03 Thread Matthias Gatto 
For now we block all blocks operations with RAID disks.

Quorum doesn't support BlockRebase neither, but
qemuDomainBlockRebase call qemuDomainBlockPullCommon or
qemuDomainBlockCopyCommon which are alerady blocked.

Signed-off-by: Matthias Gatto 
---
 src/qemu/qemu_driver.c | 28 
 1 file changed, 28 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4a56ebd..d0f7866 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14682,6 +14682,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
 if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
 goto cleanup;
 
+if (virDomainDefHasRAID(vm->def)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Snapshot does not support domain with RAID(like 
quorum) yet"));
+goto cleanup;
+}
+
 if (qemuProcessAutoDestroyActive(driver, vm)) {
 virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("domain is marked for auto destroy"));
@@ -16300,6 +16306,13 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver,
 goto endjob;
 disk = vm->def->disks[idx];
 
+if (virStorageSourceIsRAID(disk->src)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("block pull is not yet supported with disk of format 
'%s'"),
+   virStorageFileFormatTypeToString(disk->src->format));
+goto endjob;
+}
+
 if (qemuDomainDiskBlockJobIsActive(disk))
 goto endjob;
 
@@ -16411,6 +16424,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
 goto endjob;
 disk = vm->def->disks[idx];
 
+if (virStorageSourceIsRAID(disk->src)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("block job is not yes supported with disk of format 
'%s'"),
+   virStorageFileFormatTypeToString(disk->src->format));
+goto endjob;
+}
+
 if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE &&
 disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) {
 virReportError(VIR_ERR_OPERATION_INVALID,
@@ -16702,6 +16722,14 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
_("block copy is not supported with this QEMU binary"));
 goto endjob;
 }
+
+if (virStorageSourceIsRAID(disk->src)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("block copy is not yet supported with disk of format 
'%s'"),
+   virStorageFileFormatTypeToString(disk->src->format));
+goto endjob;
+}
+
 if (vm->persistent) {
 /* XXX if qemu ever lets us start a new domain with mirroring
  * already active, we can relax this; but for now, the risk of
-- 
2.6.2

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

[libvirt] [PATCH v7 05/13] virstoragefile: change backingStore to backingStores.

2015-12-03 Thread Matthias Gatto 
The backingStore field was a virStorageSourcePtr.
Because a quorum can contain more that one backingStore at the same level,
it's now an array of 'virStorageSourcePtr'.

This patch rename  src->backingStore to src->backingStores,
Made the necessary changes to virStorageSourceSetBackingStore
and virStorageSourceGetBackingStore.
virStorageSourceSetBackingStore can now expand the size of src->backingStores.

Signed-off-by: Matthias Gatto 
---
 src/storage/storage_backend.c|  2 +-
 src/storage/storage_backend_fs.c |  2 +-
 src/util/virstoragefile.c| 66 +++-
 src/util/virstoragefile.h|  3 +-
 4 files changed, 56 insertions(+), 17 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 08ed1dd..d71bb1a 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -498,7 +498,7 @@ virStorageBackendCreateRaw(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 goto cleanup;
 }
 
-if (vol->target.backingStore) {
+if (vol->target.backingStores) {
 virReportError(VIR_ERR_NO_SUPPORT, "%s",
_("backing storage not supported for raw volumes"));
 goto cleanup;
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index b216e91..68419e3 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -1100,7 +1100,7 @@ static int createFileDir(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 return -1;
 }
 
-if (vol->target.backingStore) {
+if (vol->target.backingStores) {
 virReportError(VIR_ERR_NO_SUPPORT, "%s",
_("backing storage not supported for directories 
volumes"));
 return -1;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 1299f98..8c05786 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1809,22 +1809,50 @@ virStorageSourcePoolDefCopy(const 
virStorageSourcePoolDef *src)
 }
 
 
+/**
+ * virStorageSourceGetBackingStore:
+ * @src: virStorageSourcePtr containing the backing stores
+ * @pos: position of the backing store to get
+ *
+ * return the backingStore at the position of @pos
+ */
 virStorageSourcePtr
-virStorageSourceGetBackingStore(const virStorageSource *src,
-size_t pos ATTRIBUTE_UNUSED)
+virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos)
 {
-if (!src)
+if (!src || !src->backingStores || pos >= src->nBackingStores)
 return NULL;
-return src->backingStore;
+return src->backingStores[pos];
 }
 
 
+/**
+ * virStorageSourceSetBackingStore:
+ * @src: virStorageSourcePtr to hold @backingStore
+ * @backingStore: backingStore to store
+ * @pos: position of the backing store to store
+ *
+ * Set @backingStore at @pos in src->backingStores.
+ * If src->backingStores don't have the space to contain
+ * @backingStore, we expand src->backingStores.
+ * If src->backingStores[pos] is alerady set, free it.
+ */
 int
 virStorageSourceSetBackingStore(virStorageSourcePtr src,
 virStorageSourcePtr backingStore,
-size_t pos ATTRIBUTE_UNUSED)
+size_t pos)
 {
-src->backingStore = backingStore;
+if (!src)
+return -1;
+
+if (pos >= src->nBackingStores) {
+int nbr = pos - src->nBackingStores + 1;
+if (VIR_EXPAND_N(src->backingStores, src->nBackingStores, nbr) < 0)
+return -1;
+}
+
+if (src->backingStores[pos])
+virStorageSourceFree(src->backingStores[pos]);
+src->backingStores[pos] = backingStore;
 return 0;
 }
 
@@ -1843,6 +1871,7 @@ virStorageSourceCopy(const virStorageSource *src,
  bool backingChain)
 {
 virStorageSourcePtr ret = NULL;
+size_t i;
 
 if (VIR_ALLOC(ret) < 0)
 return NULL;
@@ -1855,6 +1884,8 @@ virStorageSourceCopy(const virStorageSource *src,
 ret->physical = src->physical;
 ret->readonly = src->readonly;
 ret->shared = src->shared;
+ret->nBackingStores = src->nBackingStores;
+ret->threshold = src->threshold;
 
 /* storage driver metadata are not copied */
 ret->drv = NULL;
@@ -1903,12 +1934,14 @@ virStorageSourceCopy(const virStorageSource *src,
 !(ret->auth = virStorageAuthDefCopy(src->auth)))
 goto error;
 
-if (backingChain && virStorageSourceGetBackingStore(src, 0)) {
-if (virStorageSourceSetBackingStore(ret,
-
virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0),
- true),
- 0) < 0)
-goto error;
+for (i = 0; i < src->nBackingStores; ++i) {
+if (backingChain && virStorageSourceGetBackingStore(src, i)) {
+if (virStorageSourceSetBackingStore(ret,

[libvirt] [PATCH v7 03/13] virstoragefile: Add virStorageSourceSetBackingStore

2015-12-03 Thread Matthias Gatto 
As explained for virStorageSourceGetBackingStore, quorum allows
multiple backing store, this make the operation to set bs complex
because we have to check if the backingStore is used as an array
or a pointer, and set it differently in both case.

In order to help the manipulation of backing store, I've added a
function virStorageSourceSetBackingStore.

For now virStorageSourceSetBackingStore don't handle the case where
we have more than one backing store in virStorageSource.

Signed-off-by: Matthias Gatto 
Signed-off-by: John Ferlan 
---
 src/libvirt_private.syms  |  1 +
 src/util/virstoragefile.c | 10 ++
 src/util/virstoragefile.h |  4 
 3 files changed, 15 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5354a4a..d3baee8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2200,6 +2200,7 @@ virStorageSourceParseRBDColonString;
 virStorageSourcePoolDefFree;
 virStorageSourcePoolModeTypeFromString;
 virStorageSourcePoolModeTypeToString;
+virStorageSourceSetBackingStore;
 virStorageSourceUpdateBlockPhysicalSize;
 virStorageTypeFromString;
 virStorageTypeToString;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index af17d82..a8a2134 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1819,6 +1819,16 @@ virStorageSourceGetBackingStore(const virStorageSource 
*src,
 }
 
 
+int
+virStorageSourceSetBackingStore(virStorageSourcePtr src,
+virStorageSourcePtr backingStore,
+size_t pos ATTRIBUTE_UNUSED)
+{
+src->backingStore = backingStore;
+return 0;
+}
+
+
 /**
  * virStorageSourcePtr:
  *
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 8cd4854..ce1cb5d 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -289,6 +289,10 @@ struct _virStorageSource {
 #  define DEV_BSIZE 512
 # endif
 
+int virStorageSourceSetBackingStore(virStorageSourcePtr src,
+virStorageSourcePtr backingStore,
+size_t pos);
+
 virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource 
*src,
 size_t pos);
 
-- 
2.6.2

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

[libvirt] [PATCH v7 12/13] qemu: qemuDomainGetBlockInfo quorum support

2015-12-03 Thread Matthias Gatto 
Signed-off-by: Matthias Gatto 
---
 src/qemu/qemu_driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d0f7866..44ce90f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11834,7 +11834,8 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
 goto endjob;
 }
 
-if (virStorageSourceIsEmpty(disk->src)) {
+if (!virStorageSourceIsRAID(disk->src) &&
+virStorageSourceIsEmpty(disk->src)) {
 virReportError(VIR_ERR_INVALID_ARG,
_("disk '%s' does not currently have a source 
assigned"),
path);
-- 
2.6.2

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

[libvirt] [PATCH v7 01/13] virstoragefile: Add virStorageSourceGetBackingStore

2015-12-03 Thread Matthias Gatto 
Create virStorageSourceGetBackingStore function in
preparation for quorum:
Actually, if we want to get a backing store inside a virStorageSource
we have to do it manually(src->backingStore = backingStore).
The problem is that with a quorum, a virStorageSource
can contain multiple backing stores, and src->backingStore can
be treated as a virStorageSourcePtr or a virStorageSourcePtrPtr.

Due to these reason, we need to simplify the manipulation of
virStorageSource, and create a function to get the asked
backingStore in a virStorageSource

For now, this function only return the backingStore field

Signed-off-by: Matthias Gatto 
---
 src/libvirt_private.syms  |  1 +
 src/util/virstoragefile.c | 10 ++
 src/util/virstoragefile.h |  3 +++
 3 files changed, 14 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index dd085c3..5354a4a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2190,6 +2190,7 @@ virStorageSourceClear;
 virStorageSourceCopy;
 virStorageSourceFree;
 virStorageSourceGetActualType;
+virStorageSourceGetBackingStore;
 virStorageSourceGetSecurityLabelDef;
 virStorageSourceInitChainElement;
 virStorageSourceIsEmpty;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 2aa1d90..016beaa 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1809,6 +1809,16 @@ virStorageSourcePoolDefCopy(const 
virStorageSourcePoolDef *src)
 }
 
 
+virStorageSourcePtr
+virStorageSourceGetBackingStore(const virStorageSource *src,
+size_t pos ATTRIBUTE_UNUSED)
+{
+if (!src)
+return NULL;
+return src->backingStore;
+}
+
+
 /**
  * virStorageSourcePtr:
  *
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index b98fe25..8cd4854 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -289,6 +289,9 @@ struct _virStorageSource {
 #  define DEV_BSIZE 512
 # endif
 
+virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource 
*src,
+size_t pos);
+
 int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid);
 int virStorageFileProbeFormatFromBuf(const char *path,
  char *buf,
-- 
2.6.2

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

[libvirt] [PATCH v7 10/13] domain: Add virDomainDefHasRAID

2015-12-03 Thread Matthias Gatto 
This function check if a domain has a RAID as a disk.
This function is useful to block snapshot operations on domain
which contain quorum.

Signed-off-by: Matthias Gatto 
---
 src/conf/domain_conf.c   | 13 +
 src/conf/domain_conf.h   |  1 +
 src/libvirt_private.syms |  1 +
 3 files changed, 15 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5178e57..69d916d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2573,6 +2573,19 @@ virDomainDefNewFull(const char *name,
 }
 
 
+bool
+virDomainDefHasRAID(virDomainDefPtr def)
+{
+size_t i;
+
+for (i = 0; i < def->ndisks; ++i) {
+if (virStorageSourceIsRAID(def->disks[i]->src))
+return true;
+}
+return true;
+}
+
+
 void virDomainObjAssignDef(virDomainObjPtr domain,
virDomainDefPtr def,
bool live,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 90d8e13..d193ab4 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2560,6 +2560,7 @@ void virDomainDefClearPCIAddresses(virDomainDefPtr def);
 void virDomainDefClearCCWAddresses(virDomainDefPtr def);
 void virDomainDefClearDeviceAliases(virDomainDefPtr def);
 void virDomainTPMDefFree(virDomainTPMDefPtr def);
+bool virDomainDefHasRAID(virDomainDefPtr def);
 
 typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 571b6f7..a0c1a88 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -219,6 +219,7 @@ virDomainDefGetMemoryInitial;
 virDomainDefGetSecurityLabelDef;
 virDomainDefHasDeviceAddress;
 virDomainDefHasMemoryHotplug;
+virDomainDefHasRAID;
 virDomainDefMaybeAddController;
 virDomainDefMaybeAddInput;
 virDomainDefNeedsPlacementAdvice;
-- 
2.6.2

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

[libvirt] [PATCH 2/3] virsh: remove custom error for cpulist from cmdIOThreadPin

2015-12-03 Thread Ján Tomko 
Instead of the custom error:
error: iothreadpin: invalid cpulist.

use vshCommandOptStringReq and let it report a more specific error:
error: Failed to get option 'cpulist': Option argument is empty
---
 tools/virsh-domain.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 4d79890..b7e7606 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -7116,10 +7116,8 @@ cmdIOThreadPin(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptUInt(ctl, cmd, "iothread", _id) < 0)
 goto cleanup;
 
-if (vshCommandOptString(ctl, cmd, "cpulist", ) < 0) {
-vshError(ctl, "%s", _("iothreadpin: invalid cpulist."));
+if (vshCommandOptStringReq(ctl, cmd, "cpulist", ) < 0)
 goto cleanup;
-}
 
 if ((maxcpu = virshNodeGetCPUCount(priv->conn)) < 0)
 goto cleanup;
-- 
2.4.6

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

[libvirt] [PATCH 3/3] virsh: rename vshCommandOptString to vshCommandOptStringQuiet

2015-12-03 Thread Ján Tomko 
This function does not set an error. Make it obvious in its name
to discourage its usage without reporting an error in the caller.
---
 tools/virsh-domain.c  | 20 ++--
 tools/virsh-nodedev.c |  4 ++--
 tools/virsh-volume.c  |  2 +-
 tools/vsh.c   | 12 ++--
 tools/vsh.h   |  4 ++--
 5 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index b7e7606..7650535 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1622,7 +1622,7 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
 goto save_error;
 }
 
-rv = vshCommandOptString(ctl, cmd, "device-weights", _weight);
+rv = vshCommandOptStringQuiet(ctl, cmd, "device-weights", _weight);
 if (rv < 0) {
 vshError(ctl, "%s", _("Unable to parse string parameter"));
 goto cleanup;
@@ -1633,7 +1633,7 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
 goto save_error;
 }
 
-rv = vshCommandOptString(ctl, cmd, "device-read-iops-sec", _riops);
+rv = vshCommandOptStringQuiet(ctl, cmd, "device-read-iops-sec", 
_riops);
 if (rv < 0) {
 vshError(ctl, "%s", _("Unable to parse string parameter"));
 goto cleanup;
@@ -1644,7 +1644,7 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
 goto save_error;
 }
 
-rv = vshCommandOptString(ctl, cmd, "device-write-iops-sec", _wiops);
+rv = vshCommandOptStringQuiet(ctl, cmd, "device-write-iops-sec", 
_wiops);
 if (rv < 0) {
 vshError(ctl, "%s", _("Unable to parse string parameter"));
 goto cleanup;
@@ -1655,7 +1655,7 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
 goto save_error;
 }
 
-rv = vshCommandOptString(ctl, cmd, "device-read-bytes-sec", _rbps);
+rv = vshCommandOptStringQuiet(ctl, cmd, "device-read-bytes-sec", 
_rbps);
 if (rv < 0) {
 vshError(ctl, "%s", _("Unable to parse string parameter"));
 goto cleanup;
@@ -1666,7 +1666,7 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
 goto save_error;
 }
 
-rv = vshCommandOptString(ctl, cmd, "device-write-bytes-sec", _wbps);
+rv = vshCommandOptStringQuiet(ctl, cmd, "device-write-bytes-sec", 
_wbps);
 if (rv < 0) {
 vshError(ctl, "%s", _("Unable to parse string parameter"));
 goto cleanup;
@@ -3736,7 +3736,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
 size_t j;
 virshControlPtr priv = ctl->privData;
 
-ignore_value(vshCommandOptString(ctl, cmd, "storage", _string));
+ignore_value(vshCommandOptStringQuiet(ctl, cmd, "storage", _string));
 
 if (!(vol_string || remove_all_storage) && wipe_storage) {
 vshError(ctl,
@@ -4115,7 +4115,7 @@ cmdStartGetFDs(vshControl *ctl,
 *nfdsret = 0;
 *fdsret = NULL;
 
-if (vshCommandOptString(ctl, cmd, "pass-fds", ) <= 0)
+if (vshCommandOptStringQuiet(ctl, cmd, "pass-fds", ) <= 0)
 return 0;
 
 if (!(fdlist = virStringSplit(fdopt, ",", -1))) {
@@ -5310,7 +5310,7 @@ doDump(void *opaque)
 goto out;
 }
 
-if (vshCommandOptString(ctl, cmd, "format", ) > 0) {
+if (vshCommandOptStringQuiet(ctl, cmd, "format", ) > 0) {
 if (STREQ(format, "kdump-zlib")) {
 dumpformat = VIR_DOMAIN_CORE_DUMP_FORMAT_KDUMP_ZLIB;
 } else if (STREQ(format, "kdump-lzo")) {
@@ -8359,7 +8359,7 @@ cmdSendKey(vshControl *ctl, const vshCmd *cmd)
 if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
 return false;
 
-if (vshCommandOptString(ctl, cmd, "codeset", _option) <= 0)
+if (vshCommandOptStringQuiet(ctl, cmd, "codeset", _option) <= 0)
 codeset_option = "linux";
 
 if (vshCommandOptUInt(ctl, cmd, "holdtime", ) < 0)
@@ -8775,7 +8775,7 @@ virshMemtuneGetSize(vshControl *ctl, const vshCmd *cmd,
 const char *str;
 char *end;
 
-ret = vshCommandOptString(ctl, cmd, name, );
+ret = vshCommandOptStringQuiet(ctl, cmd, name, );
 if (ret <= 0)
 return ret;
 if (virStrToLong_ll(str, , 10, value) < 0)
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index cc359e2..bfe507e 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -398,7 +398,7 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 virshNodeDeviceListPtr list = NULL;
 int cap_type = -1;
 
-ignore_value(vshCommandOptString(ctl, cmd, "cap", _str));
+ignore_value(vshCommandOptStringQuiet(ctl, cmd, "cap", _str));
 
 if (cap_str) {
 if (tree) {
@@ -615,7 +615,7 @@ cmdNodeDeviceDetach(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptStringReq(ctl, cmd, "device", ) < 0)
 return false;
 
-ignore_value(vshCommandOptString(ctl, cmd, "driver", ));
+ignore_value(vshCommandOptStringQuiet(ctl, cmd, "driver", ));
 
 if (!(device = virNodeDeviceLookupByName(priv->conn, name))) {
 vshError(ctl, _("Could not find

Re: [libvirt] [PATCH 1/3] conf: Reposition adding SCSI controller for SCSI hostdev hotplug

2015-12-03 Thread Boris Fiuczynski 

On 12/02/2015 06:13 PM, John Ferlan wrote:



On 12/02/2015 11:37 AM, Boris Fiuczynski wrote:

[...]


So, what if qemuDomainFindOrCreateSCSIDiskController checked the "found"
cont in the first loop for "cont->info.alias". If it doesn't exist, then
fall through under the "assumption" that the controller was added by
some other mechanism into the list?  That would require some more
adjustments in qemuDomainAttachControllerDevice to know the controller
was already in the list, but not active in the domain.


I thought about that as well and decided against it because you would
add controllers in the domain data structure and than decide based on
the aliases if the controller needs to be hotplugged afterwards. I
thought that using the alias to detect is the domain is running in such
way would not be proper!
Anyhow what are you going to do if hotplugging the controller fails?
Would this require an additional cleanup method?
qemuDomainAttachControllerDevice is also used when adding a new scsi
controller directly from virsh attach-device (scsi-controller.xml). What
additional special casing would be required there...?



I had just started down that path... not that I like that option, but it
was an idea.  I was able to get it to work in the one case, but other
issues showed up, so I'll just abandon it...


Although we're now in freeze, I will go with these two patches. I think
I would like to encourage you to also pick the third patch fixing the 
SCSI disk hotplug scenario.



it's better to combine them - mostly to make it "clearer" for anyone
venturing down this path in the future to see the relationship(s).

Agreed!



The combined commit message would be (the weird line breaks only occur
here because of my settings on line width/break for sent emails):

conf: Revert some code to resolve issues for hostdev hotplug

This patch reverts parts of commits 0d8b24f6b and 0785966d dealing with
the addition of a controller during virDomainHostdevAssignAddress. This
caused a regression for the hostdev hotplug path which assumes the
qemuDomainFindOrCreateSCSIDiskController will add the new controller
during qemuDomainAttachHostSCSIDevice to both the running domain and
the domain def controller list when the controller doesn't yet exist
(whether due to no SCSI controllers existing or the addition of a new
controller because existing ones are full).

Since commit id 0d8b24f6 will call virDomainHostdevAssignAddress during
virDomainDeviceDefPostParseInternal which is called either during domain
definition post processing (via an iterator during virDomainDefPostParse)
or directly from virDomainDeviceDefParse during hotplug, the change
broke the "side effect" of being able to add both a hostdev and controller
to the running domain.

The regression would only be seen if the running domain didn't have a
SCSI controller alrady defined or if the existing SCSI controller was
"full" of devices and a new controller needed to be created.

This patch will also add some extra comments to the code to avoid a
similar future change.

Reads good.




I'd like to also add a pair of comments to also leave a second trail of
breadcrumbs...

In virDomainHostdevAssignAddress after the initial loop:

/* NB: Do not attempt calling virDomainDefMaybeAddController to
 * automagically add a "new" controller. Doing so will result in
 * qemuDomainFindOrCreateSCSIDiskController "finding" the controller
 * in the domain def list and thus not hotplugging the controller as
 * well as the hostdev in the event that there are either no SCSI
 * controllers defined or there was no space on an existing one.
 */

and in virDomainDefParseXML prior to maybe adding the controller:

/* For a domain definition, we need to check if the controller
 * for this hostdev exists yet and if not add it. This cannot be
 * done during virDomainHostdevAssignAddress (as part of device
 * post processing) because that will result in the failure to
 * load the controller during hostdev hotplug.
 */


Does this all seem reasonable?
Yes, and (again) I would like to ask to also add the third patch as the 
second patch to fix the scsi disk hotplug problem.



--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[libvirt] [PATCH 0/3] virsh: error reporting for empty string parameters

2015-12-03 Thread Ján Tomko 
https://bugzilla.redhat.com/show_bug.cgi?id=1281707

Ján Tomko (3):
  virsh: report errors for empty strings
  virsh: remove custom error for cpulist from cmdIOThreadPin
  virsh: rename vshCommandOptString to vshCommandOptStringQuiet

 tools/virsh-domain-monitor.c |  4 ++--
 tools/virsh-domain.c | 34 --
 tools/virsh-network.c|  4 ++--
 tools/virsh-nodedev.c|  4 ++--
 tools/virsh-volume.c |  2 +-
 tools/vsh.c  | 12 ++--
 tools/vsh.h  |  4 ++--
 7 files changed, 31 insertions(+), 33 deletions(-)

-- 
2.4.6

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

[libvirt] [PATCH 1/3] virsh: report errors for empty strings

2015-12-03 Thread Ján Tomko 
Several callers were using vshCommandOptString without setting an error.
Use vshCommandOptStringReq which sets the error.

https://bugzilla.redhat.com/show_bug.cgi?id=1281707
---
 tools/virsh-domain-monitor.c |  4 ++--
 tools/virsh-domain.c | 10 +-
 tools/virsh-network.c|  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 64ec03d..7dcf833 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -2243,9 +2243,9 @@ cmdDomIfAddr(vshControl *ctl, const vshCmd *cmd)
 if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
 return false;
 
-if (vshCommandOptString(ctl, cmd, "interface", ) < 0)
+if (vshCommandOptStringReq(ctl, cmd, "interface", ) < 0)
 goto cleanup;
-if (vshCommandOptString(ctl, cmd, "source", ) < 0)
+if (vshCommandOptStringReq(ctl, cmd, "source", ) < 0)
 goto cleanup;
 
 if (sourcestr) {
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 413220b..4d79890 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2332,11 +2332,11 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
 
 if (vshCommandOptStringReq(ctl, cmd, "path", ) < 0)
 return false;
-if (vshCommandOptString(ctl, cmd, "dest", ) < 0)
+if (vshCommandOptStringReq(ctl, cmd, "dest", ) < 0)
 return false;
-if (vshCommandOptString(ctl, cmd, "xml", ) < 0)
+if (vshCommandOptStringReq(ctl, cmd, "xml", ) < 0)
 return false;
-if (vshCommandOptString(ctl, cmd, "format", ) < 0)
+if (vshCommandOptStringReq(ctl, cmd, "format", ) < 0)
 return false;
 /* XXX: Parse bandwidth as scaled input, rather than forcing
  * MiB/s, and either reject negative input or treat it as 0 rather
@@ -9241,7 +9241,7 @@ cmdQemuMonitorEvent(vshControl *ctl, const vshCmd *cmd)
 data.count = 0;
 if (vshCommandOptTimeoutToMs(ctl, cmd, ) < 0)
 return false;
-if (vshCommandOptString(ctl, cmd, "event", ) < 0)
+if (vshCommandOptStringReq(ctl, cmd, "event", ) < 0)
 return false;
 
 if (vshCommandOptBool(cmd, "domain"))
@@ -12568,7 +12568,7 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
 return true;
 }
 
-if (vshCommandOptString(ctl, cmd, "event", ) < 0)
+if (vshCommandOptStringReq(ctl, cmd, "event", ) < 0)
 return false;
 if (eventName) {
 for (event = 0; event < VIR_DOMAIN_EVENT_ID_LAST; event++)
diff --git a/tools/virsh-network.c b/tools/virsh-network.c
index a0f7707..bd89c11 100644
--- a/tools/virsh-network.c
+++ b/tools/virsh-network.c
@@ -1262,7 +1262,7 @@ cmdNetworkEvent(vshControl *ctl, const vshCmd *cmd)
 return true;
 }
 
-if (vshCommandOptString(ctl, cmd, "event", ) < 0)
+if (vshCommandOptStringReq(ctl, cmd, "event", ) < 0)
 return false;
 if (!eventName) {
 vshError(ctl, "%s", _("either --list or event type is required"));
@@ -1372,7 +1372,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd)
 unsigned int flags = 0;
 virNetworkPtr network = NULL;
 
-if (vshCommandOptString(ctl, cmd, "mac", ) < 0)
+if (vshCommandOptStringReq(ctl, cmd, "mac", ) < 0)
 return false;
 
 if (!(network = virshCommandOptNetwork(ctl, cmd, )))
-- 
2.4.6

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

[libvirt] [PATCH v7 00/13] qemu: Add quorum support to libvirt

2015-12-03 Thread Matthias Gatto 
The purpose of these patches is to introduce quorum for libvirt
I've try to follow this proposal:
http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html

This feature ask for 5 task:

1) Allow a _virStorageSource to contain more than one backing store.

Because all the actual libvirt code use the backingStore field
as a pointer and we needs want to change that, I've decide to encapsulate
the backingStore field to simplifie the array manipulation.

2) Add the missing field a quorum need in _virStorageSource and
the VIR_STORAGE_TYPE_QUORUM and VIR_STORAGE_FILE_QUORUM in
their respectives enums.

3) Parse and format the xml
Because a quorum allows to have more than one backing store at the same level
we need to change virDomainDiskDefFormat and virDomainDiskDefParseXML
to call virDomainDiskBackingStoreFormat and virDomainDiskBackingStoreParse
in a loop.
virDomainDiskBackingStoreFormat and virDomainDiskBackingStoreParse can
call themself recursively in a loop because a quorum can contain another
quorum

4) Build qemu string
As for the xml, we have to call the function which create quorum recursively.
But this task have the problem explained here:
http://www.redhat.com/archives/libvir-list/2014-October/msg00529.html
The _virStorageSource missing some informations that can be passed to
a child, and therefore this version of quorum is incomplet.

5) Allow to hotplug/change a disk in a quorum
This part is not present in these patches because for this task
we have to use blockdev-add, and currently libvirt use
device_add for hotpluging that doesn't allow to hotplug quorum childs.


V2:
-Rebase on master
-Add Documentation

V3:
-Transforme the backingStore field in virStorageSource into
 an array of pointer instead of a pointer
-Modify virStorageSourceSetBackingStore to allow it to expand
 the backingStore size.

V4:
-Rebase on master

V5:
-Rebase on master
-patch 1-4/9: use patchs from John Ferlan 
-patch 4/9: check return of virStorageSourceSetBackingStore
-patch 5/9: report type of error on virStorageSourceSetBackingStore

v6:
-Rebase on master
-Remove node-name patch
-patch 06/13: Add virStorageSourceIsRAID
-patch 10/13: Add virDomainDefHasRAID
-patch 11-13/13: Block all unsupported operations

v7:
-Rebase on master.
-Parse unconditionally backing store.
-fold qemuBuildRAIDFileSourceStr into qemuBuildRAIDStr.
-use 0/-1 return values when failing instead of bool.
-virStorageSourceSetBackingStore now free backing store when they are
already set.

Matthias Gatto (13):
  virstoragefile: Add virStorageSourceGetBackingStore
  virstoragefile: Always use virStorageSourceGetBackingStore to get
backing store
  virstoragefile: Add virStorageSourceSetBackingStore
  virstoragefile: Always use virStorageSourceSetBackingStore to set
backing store
  virstoragefile: change backingStore to backingStores.
  virstoragefile: Add virStorageSourceIsRAID
  virstoragefile: Add quorum in virstoragefile
  domain_conf: Read and Write quorum config
  qemu: Add quorum support in qemuBuildDriveDevStr
  domain: Add virDomainDefHasRAID
  qemu: Forbid blocks operations with quorum
  qemu: qemuDomainGetBlockInfo quorum support
  qemu: qemuDiskPathToAlias quorum support

 docs/formatdomain.html.in |  23 -
 docs/schemas/domaincommon.rng |  21 +++-
 docs/schemas/storagecommon.rng|   1 +
 docs/schemas/storagevol.rng   |   1 +
 src/conf/domain_conf.c| 174 --
 src/conf/domain_conf.h|   1 +
 src/conf/storage_conf.c   |  23 +++--
 src/libvirt_private.syms  |   4 +
 src/qemu/qemu_cgroup.c|   4 +-
 src/qemu/qemu_command.c   |  78 +++
 src/qemu/qemu_domain.c|   2 +-
 src/qemu/qemu_driver.c|  72 ++
 src/qemu/qemu_migration.c |   1 +
 src/qemu/qemu_monitor_json.c  |   4 +-
 src/security/security_dac.c   |   2 +-
 src/security/security_selinux.c   |   4 +-
 src/security/virt-aa-helper.c |   2 +-
 src/storage/storage_backend.c |  24 +++--
 src/storage/storage_backend_fs.c  |  45 +
 src/storage/storage_backend_gluster.c |  11 ++-
 src/storage/storage_backend_logical.c |  15 ++-
 src/storage/storage_driver.c  |   3 +-
 src/util/virstoragefile.c | 121 ---
 src/util/virstoragefile.h |  15 ++-
 tests/virstoragetest.c|  18 ++--
 25 files changed, 518 insertions(+), 151 deletions(-)

-- 
2.6.2

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

[libvirt] [PATCH v7 08/13] domain_conf: Read and Write quorum config

2015-12-03 Thread Matthias Gatto 
Add the capabiltty to libvirt to parse and format the quorum syntax
as described here:
http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html

We need a different unique index for each backing store
to manipulate individually each child of a quorum.

If a disk have 2 childs A and B, the index will be 1 for A and 2 for B,
but if A have 2 childs C and D, so A will have the index 1, C and D: 2 and 3
and B will be 4.

Here is a representation of our disk tree and they indexs:

 C[2]
/
A[1]
   /\D[3]
HDA
   \
B[4]

Signed-off-by: Matthias Gatto 
---
 src/conf/domain_conf.c | 157 +
 1 file changed, 106 insertions(+), 51 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e2a1870..5178e57 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6306,17 +6306,45 @@ virDomainDiskSourceParse(xmlNodePtr node,
 
 
 static int
+virDomainDiskThresholdParse(virStorageSourcePtr src,
+xmlXPathContextPtr ctxt)
+{
+char *threshold = virXPathString("string(./@threshold)", ctxt);
+int ret;
+
+if (!threshold)
+return 0;
+ret = virStrToLong_ul(threshold, NULL, 10, >threshold);
+if (ret < 0 || src->threshold < 2) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("threshold must be a decimal number greater than 2 "
+ "and less than the number of children"));
+VIR_FREE(threshold);
+return -1;
+}
+VIR_FREE(threshold);
+return 0;
+}
+
+#define VIR_DOMAIN_DISK_BACKING_STORE_PARSE_RECALL 1
+
+static int
 virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
-   virStorageSourcePtr src)
+   virStorageSourcePtr src,
+   size_t pos)
 {
 virStorageSourcePtr backingStore = NULL;
 xmlNodePtr save_ctxt = ctxt->node;
 xmlNodePtr source;
 char *type = NULL;
 char *format = NULL;
+char *path;
 int ret = -1;
 
-if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) {
+if (virAsprintf(, "./backingStore[%lu][*]", pos + 1) < 0)
+return -1;
+
+if (!(ctxt->node = virXPathNode(path, ctxt))) {
 ret = 0;
 goto cleanup;
 }
@@ -6350,29 +6378,35 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
 goto cleanup;
 }
 
-if (!(source = virXPathNode("./source", ctxt))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("missing disk backing store source"));
+source = virXPathNode("./source", ctxt);
+
+if (source && virDomainDiskSourceParse(source, ctxt, backingStore) < 0)
 goto cleanup;
-}
 
-if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 ||
-virDomainDiskBackingStoreParse(ctxt, backingStore) < 0)
+if (virDomainDiskBackingStoreParse(ctxt, backingStore, 0) < 0)
 goto cleanup;
 
-if (virStorageSourceSetBackingStore(src, backingStore, 0) < 0)
+if (virDomainDiskThresholdParse(backingStore, ctxt) < 0)
 goto cleanup;
-ret = 0;
+
+if (virStorageSourceSetBackingStore(src, backingStore, pos) < 0)
+goto cleanup;
+
+ret = VIR_DOMAIN_DISK_BACKING_STORE_PARSE_RECALL;
 
  cleanup:
 if (ret < 0)
 virStorageSourceFree(backingStore);
+VIR_FREE(path);
 VIR_FREE(type);
 VIR_FREE(format);
 ctxt->node = save_ctxt;
-return ret;
+if (ret != VIR_DOMAIN_DISK_BACKING_STORE_PARSE_RECALL)
+return ret;
+return virDomainDiskBackingStoreParse(ctxt, src, pos + 1);
 }
 
+#undef VIR_DOMAIN_DISK_BACKING_STORE_PARSE_RECALL
 
 #define VENDOR_LEN  8
 #define PRODUCT_LEN 16
@@ -6902,12 +6936,16 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 def->device = VIR_DOMAIN_DISK_DEVICE_DISK;
 }
 
+if (virDomainDiskThresholdParse(def->src, ctxt) < 0)
+goto error;
+
 /* Only CDROM and Floppy devices are allowed missing source path
  * to indicate no media present. LUN is for raw access CD-ROMs
  * that are not attached to a physical device presently */
 if (virStorageSourceIsEmpty(def->src) &&
 (def->device == VIR_DOMAIN_DISK_DEVICE_DISK ||
- (flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE))) {
+ (flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) &&
+!(virStorageSourceIsRAID(def->src))) {
 virReportError(VIR_ERR_NO_SOURCE,
target ? "%s" : NULL, target);
 goto error;
@@ -7250,10 +7288,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 }
 }
 
-if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) {
-if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0)
-goto error;
-}
+if (virDomainDiskBackingStoreParse(ctxt, def->src, 0) < 0)
+goto error;
 
  cleanup:
 VIR_FREE(bus);
@@ -18591,12 +18627,14 @@

[libvirt] [libvirt-glib] gobject, gconfig: Drop redundant debug logging

2015-12-03 Thread Zeeshan Ali (Khattak) 
Justification from IRC chat:

 danpb: i'm not sure it's good to have a g_debug() for every
 libvirt-gobject/gconfig object creation/destruction
 danpb: the debug output becomes so big that at least I never
 think about enabling it at runtime and sometimes end up missing
 important debug info
  yeah we could easily kill that
  gobject has systemtap probes builtin these days which let you
 debug object create/finalize problems
 danpb: oh cool
 danpb: i'll write a patch
---
 libvirt-gconfig/libvirt-gconfig-capabilities-cpu-feature.c| 2 --
 libvirt-gconfig/libvirt-gconfig-capabilities-cpu-model.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-capabilities-cpu-topology.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-capabilities-cpu.c| 2 --
 libvirt-gconfig/libvirt-gconfig-capabilities-guest-arch.c | 2 --
 libvirt-gconfig/libvirt-gconfig-capabilities-guest-domain.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-capabilities-guest-feature.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-capabilities-guest.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-capabilities-host-secmodel.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-capabilities-host.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-capabilities.c| 2 --
 libvirt-gconfig/libvirt-gconfig-domain-address-pci.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-address-usb.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-address.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-channel.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-chardev-source-pty.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-chardev-source-spiceport.c | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-chardev-source-spicevmc.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-chardev-source-unix.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-chardev-source.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-chardev.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-clock.c| 2 --
 libvirt-gconfig/libvirt-gconfig-domain-console.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-controller-usb.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-controller.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-cpu-feature.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-cpu-model.c| 2 --
 libvirt-gconfig/libvirt-gconfig-domain-cpu.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-device.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-disk-driver.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-disk.c | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-filesys.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-graphics-desktop.c | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-graphics-rdp.c | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-graphics-sdl.c | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-graphics-spice.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-graphics-vnc.c | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-graphics.c | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-input.c| 2 --
 libvirt-gconfig/libvirt-gconfig-domain-interface-bridge.c | 2 --
 .../libvirt-gconfig-domain-interface-filterref-parameter.c| 2 --
 libvirt-gconfig/libvirt-gconfig-domain-interface-filterref.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-interface-network.c| 2 --
 libvirt-gconfig/libvirt-gconfig-domain-interface-user.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-interface.c| 2 --
 libvirt-gconfig/libvirt-gconfig-domain-memballoon.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-os.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-parallel.c | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-power-management.c | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-redirdev.c | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-seclabel.c | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-serial.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-smartcard-host-certificates.c  | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-smartcard-host.c   | 2 --
 libvirt-gconfig/libvirt-gconfig-domain-smartcard-passthrough.c| 2 --
 libvirt-gconfig/libvirt-gconfig-domain-smartcard.c| 2 --
 libvirt-gconfig/libvirt-gconfig-domain-snapshot-disk.c| 2 --

Re: [libvirt] [PATCH 1/3] conf: Reposition adding SCSI controller for SCSI hostdev hotplug

2015-12-03 Thread Boris Fiuczynski 

On 12/03/2015 02:05 PM, Boris Fiuczynski wrote:

conf: Revert some code to resolve issues for hostdev hotplug

This patch reverts parts of commits 0d8b24f6b and 0785966d dealing with
the addition of a controller during virDomainHostdevAssignAddress. This
caused a regression for the hostdev hotplug path which assumes the
qemuDomainFindOrCreateSCSIDiskController will add the new controller
during qemuDomainAttachHostSCSIDevice to both the running domain and
the domain def controller list when the controller doesn't yet exist
(whether due to no SCSI controllers existing or the addition of a new
controller because existing ones are full).

Since commit id 0d8b24f6 will call virDomainHostdevAssignAddress during
virDomainDeviceDefPostParseInternal which is called either during domain
definition post processing (via an iterator during virDomainDefPostParse)
or directly from virDomainDeviceDefParse during hotplug, the change
broke the "side effect" of being able to add both a hostdev and controller
to the running domain.

The regression would only be seen if the running domain didn't have a
SCSI controller alrady defined or if the existing SCSI controller was

typo  already


"full" of devices and a new controller needed to be created.

This patch will also add some extra comments to the code to avoid a
similar future change.



--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

Re: [libvirt] [PATCH 0/3] virsh: error reporting for empty string parameters

2015-12-03 Thread Erik Skultety 
On 03/12/15 14:05, Ján Tomko wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1281707
> 
> Ján Tomko (3):
>   virsh: report errors for empty strings
>   virsh: remove custom error for cpulist from cmdIOThreadPin
>   virsh: rename vshCommandOptString to vshCommandOptStringQuiet
> 
>  tools/virsh-domain-monitor.c |  4 ++--
>  tools/virsh-domain.c | 34 --
>  tools/virsh-network.c|  4 ++--
>  tools/virsh-nodedev.c|  4 ++--
>  tools/virsh-volume.c |  2 +-
>  tools/vsh.c  | 12 ++--
>  tools/vsh.h  |  4 ++--
>  7 files changed, 31 insertions(+), 33 deletions(-)
> 

ACK series.

Erik

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

[libvirt] [PATCH v7 07/13] virstoragefile: Add quorum in virstoragefile

2015-12-03 Thread Matthias Gatto 
Add VIR_STORAGE_TYPE_QUORUM in virStorageType.
Add VIR_STORAGE_FILE_QUORUM in virStorageFileFormat.

Add threshold value in _virStorageSource

Signed-off-by: Matthias Gatto 
---
 docs/formatdomain.html.in  | 23 ---
 docs/schemas/domaincommon.rng  | 21 -
 docs/schemas/storagecommon.rng |  1 +
 docs/schemas/storagevol.rng|  1 +
 src/conf/domain_conf.c |  2 ++
 src/qemu/qemu_command.c|  1 +
 src/qemu/qemu_driver.c |  4 
 src/qemu/qemu_migration.c  |  1 +
 src/util/virstoragefile.c  |  8 ++--
 src/util/virstoragefile.h  |  3 +++
 10 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index a8bd48e..9bef852 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1958,8 +1958,9 @@
 
 Valid values are "file", "block",
 "dir" (since 0.7.5),
-"network" (since 0.8.7), or
-"volume" (since 1.0.5)
+"network" (since 0.8.7),
+"volume" (since 1.0.5), or
+"quorum" (since 1.3.1)
 and refer to the underlying source for the disk.
 
   device attribute
@@ -2025,6 +2026,14 @@
 snapshot='yes' with a transient disk generally
 does not make sense.
 
+  threshold attribute
+  since 1.3.1
+
+Only use with a quorum disk.
+Indicate the minimum of positive vote a quorum must have to 
validate
+a data to be write. The minimum value is "2". The number of 
backingStores
+contain by the quorum must be superior to the threshold.
+
 
   
   source
@@ -2102,6 +2111,11 @@
 
   
   
+type='quorum'
+since 1.3.1
+  
+  A quorum contain no source element, but a serie of backingStores 
instead.
+  
   
 With "file", "block", and "volume", one or more optional
 sub-elements seclabel, described
@@ -2241,7 +2255,10 @@
 property, but using existing external files for snapshot or
 block copy operations requires the end user to pre-create the
 file correctly). The following attributes and sub-elements are
-supported in backingStore:
+supported in.
+Since 1.3.1. This elements is used to
+describe a quorum child.
+backingStore:
 
   type attribute
   
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 8d12606..ee08542 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1240,9 +1240,15 @@
 
   
 
+  
+
+  
+
+  
+
   
 
-  
+  
   
 
   
@@ -1284,9 +1290,22 @@
   
   
   
+  
 
   
 
+  
+
+  
+quorum
+  
+  
+
+  
+
+  
+
+
   
 
   
diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng
index 7c04462..0ebc2ef 100644
--- a/docs/schemas/storagecommon.rng
+++ b/docs/schemas/storagecommon.rng
@@ -76,6 +76,7 @@
   fat
   vhd
   ploop
+  quorum
   
 
   
diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng
index 7450547..a718576 100644
--- a/docs/schemas/storagevol.rng
+++ b/docs/schemas/storagevol.rng
@@ -20,6 +20,7 @@
 dir
 network
 netdir
+quorum
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d146811..e2a1870 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6281,6 +6281,7 @@ virDomainDiskSourceParse(xmlNodePtr node,
 if (virDomainDiskSourcePoolDefParse(node, >srcpool) < 0)
 goto cleanup;
 break;
+case VIR_STORAGE_TYPE_QUORUM:
 case VIR_STORAGE_TYPE_NONE:
 case VIR_STORAGE_TYPE_LAST:
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -18565,6 +18566,7 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf,
  skipSeclabels);
 break;
 
+case VIR_STORAGE_TYPE_QUORUM:
 case VIR_STORAGE_TYPE_NONE:
 case VIR_STORAGE_TYPE_LAST:
 virReportError(VIR_ERR_INTERNAL_ERROR,
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4ff31dc..7e5a9ab 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3517,6 +3517,7 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
 goto cleanup;
 break;
 
+case VIR_STORAGE_TYPE_QUORUM:
 case VIR_STORAGE_TYPE_VOLUME:
 case VIR_STORAGE_TYPE_NONE:
 case VIR_STORAGE_TYPE_LAST:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index edfd8e6..4a56ebd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13790,6 +13790,7 @@

[libvirt] [PATCH v7 06/13] virstoragefile: Add virStorageSourceIsRAID

2015-12-03 Thread Matthias Gatto 
Add a new function which return true if a virStorageSourcePtr is
a RAID.

For now, quorum is the only RAID we have.

This function is usefull, because, a lot of code access directly
to a virStorageSource internal member (like path) with some functions
like "virDomainDiskGetSource".
This beavious won't work with Quorum, and so we need to add
exeptions for these functions, but I'm not convinced by the idea to add a lot
of "disk->format == QUORUM" in all the code that deserve
exeption for Quorum, so I've add a generic function for this.

Signed-off-by: Matthias Gatto 
---
 src/libvirt_private.syms  |  1 +
 src/util/virstoragefile.c | 27 +++
 src/util/virstoragefile.h |  2 ++
 3 files changed, 30 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d3baee8..571b6f7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2195,6 +2195,7 @@ virStorageSourceGetSecurityLabelDef;
 virStorageSourceInitChainElement;
 virStorageSourceIsEmpty;
 virStorageSourceIsLocalStorage;
+virStorageSourceIsRAID;
 virStorageSourceNewFromBacking;
 virStorageSourceParseRBDColonString;
 virStorageSourcePoolDefFree;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 8c05786..0d27ca6 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1808,6 +1808,33 @@ virStorageSourcePoolDefCopy(const 
virStorageSourcePoolDef *src)
 return NULL;
 }
 
+/**
+ * virStorageSourceIsRAID:
+ * return true if the backingStores field inside @src is use
+ * as a child of a contener
+ */
+bool virStorageSourceIsRAID(virStorageSourcePtr src)
+{
+virStorageType type;
+
+if (!src)
+return false;
+type = virStorageSourceGetActualType(src);
+switch (type) {
+case VIR_STORAGE_TYPE_NONE:
+case VIR_STORAGE_TYPE_FILE:
+case VIR_STORAGE_TYPE_BLOCK:
+case VIR_STORAGE_TYPE_DIR:
+case VIR_STORAGE_TYPE_NETWORK:
+case VIR_STORAGE_TYPE_VOLUME:
+case VIR_STORAGE_TYPE_LAST:
+return false;
+
+case VIR_STORAGE_TYPE_QUORUM:
+return true;
+}
+return false;
+}
 
 /**
  * virStorageSourceGetBackingStore:
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 290c20f..68a21d0 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -290,6 +290,8 @@ struct _virStorageSource {
 #  define DEV_BSIZE 512
 # endif
 
+bool virStorageSourceIsRAID(virStorageSourcePtr src);
+
 int virStorageSourceSetBackingStore(virStorageSourcePtr src,
 virStorageSourcePtr backingStore,
 size_t pos);
-- 
2.6.2

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

[libvirt] [PATCH v7 09/13] qemu: Add quorum support in qemuBuildDriveDevStr

2015-12-03 Thread Matthias Gatto 
Allow libvirt to build the quorum string use by qemu.

Add 2astatic function: qemuBuildRAIDStr

qemuBuildRAIDStr is made because a quorum can have another quorum
as a child, so we may need to call qemuBuildRAIDStr recursively.

Signed-off-by: Matthias Gatto 
---
 src/qemu/qemu_command.c | 77 +
 1 file changed, 77 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7e5a9ab..c7d554b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3596,6 +3596,77 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk)
 }
 
 
+static int
+qemuBuildRAIDStr(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
+ virStorageSourcePtr src,
+ virBuffer *opt,
+ const char *prefix)
+{
+char *tmp = NULL;
+int ret;
+virStorageSourcePtr backingStore;
+size_t i;
+int actualType = virStorageSourceGetActualType(src);
+char *source = NULL;
+
+if (actualType == VIR_STORAGE_TYPE_QUORUM) {
+if (!src->threshold) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("threshold missing in the quorum configuration"));
+return -1;
+}
+if (src->nBackingStores < 2) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("a quorum must have at last 2 children"));
+return -1;
+}
+if (src->threshold > src->nBackingStores) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("threshold must not exceed the number of 
children"));
+return -1;
+}
+virBufferAsprintf(opt, ",%svote-threshold=%lu",
+  prefix, src->threshold);
+} else if (actualType == VIR_STORAGE_TYPE_DIR) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("unsupported disk driver type for '%s'"),
+   virStorageFileFormatTypeToString(src->format));
+return -1;
+}
+
+for (i = 0;  i < src->nBackingStores; ++i) {
+backingStore = virStorageSourceGetBackingStore(src, i);
+ret = virAsprintf(, "%schildren.%lu.", prefix, i);
+if (ret < 0)
+return -1;
+
+virBufferAsprintf(opt, ",%schildren.%lu.driver=%s",
+  prefix, i,
+  
virStorageFileFormatTypeToString(backingStore->format));
+
+if (qemuGetDriveSourceString(backingStore, conn, ) < 0)
+goto error;
+
+if (source) {
+virBufferStrcat(opt, ",", tmp, "file.filename=", NULL);
+virBufferAdd(opt, source, -1);
+}
+
+/* This operation avoid us to made another copy */
+if (virStorageSourceIsRAID(backingStore)) {
+if (qemuBuildRAIDStr(conn, disk, backingStore, opt, tmp) < 0)
+goto error;
+}
+VIR_FREE(tmp);
+}
+return 0;
+ error:
+VIR_FREE(tmp);
+return -1;
+}
+
+
 /* Check whether the device address is using either 'ccw' or default s390
  * address format and whether that's "legal" for the current qemu and/or
  * guest os.machine type. This is the corollary to the code which doesn't
@@ -3764,6 +3835,7 @@ qemuBuildDriveStr(virConnectPtr conn,
 goto error;
 
 if (source &&
+!virStorageSourceIsRAID(disk->src) &&
 !((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
   disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
@@ -4110,6 +4182,11 @@ qemuBuildDriveStr(virConnectPtr conn,
   disk->blkdeviotune.size_iops_sec);
 }
 
+if (virStorageSourceIsRAID(disk->src)) {
+if (qemuBuildRAIDStr(conn, disk, disk->src, , "") < 0)
+goto error;
+}
+
 if (virBufferCheckError() < 0)
 goto error;
 
-- 
2.6.2

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

Re: [libvirt] [PATCH 3/3] Automatic SCSI controller creation in SCSI disk hotplug broken

2015-12-03 Thread John Ferlan 


On 11/30/2015 06:06 AM, Boris Fiuczynski wrote:
> When a SCSI disk is hotplugged to a domain that does not have the required
> scsi controller already defined the following internal error occurs
> 
> error: Failed to attach device from scsi_disk.xml
> error: internal error: Could not find scsi controller with index 0 required 
> for device
> 
> Commit 0260506c added in method qemuBuildDriveDevStr a lookup of the 
> controller
> alias. The internal error occurs because in method qemuDomainAttachSCSIDisk
> the automatic creation of the potentially missing SCSI controller occurs after
> calling qemuBuildDriveDevStr.
> 
> This patch reverses the calling sequence.
> 
> Signed-off-by: Boris Fiuczynski 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Stefan Zimmermann 
> ---
>  src/qemu/qemu_hotplug.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 8804d3d..210d485 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -607,6 +607,12 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
>  goto error;
>  }

Would you say the following is an accurate description to add?

/* Let's make sure our disk has a controller defined and loaded
 * before trying add the disk. The virDomainDiskDefAssignAddress
 * doesn't try to add a controller if one doesn't exist, it just
 * assigns the disk to the calculated spot.
 */

>  
> +for (i = 0; i <= disk->info.addr.drive.controller; i++) {
> +cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i);
> +if (!cont)
> +goto error;
> +}
> +
>  if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
>  if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0)
>  goto error;
> @@ -617,12 +623,6 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
>  if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps)))
>  goto error;
>  
> -for (i = 0; i <= disk->info.addr.drive.controller; i++) {
> -cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i);
> -if (!cont)
> -goto error;
> -}
> -
>  /* Tell clang that "cont" is non-NULL.
> This is because disk->info.addr.driver.controller is unsigned,
> and hence the above loop must iterate at least once.  */
> 

Is there a reason you chose to not grab the clang check too?

John

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

Re: [libvirt] [PATCH 1/3] conf: Reposition adding SCSI controller for SCSI hostdev hotplug

2015-12-03 Thread John Ferlan 


On 12/03/2015 08:20 AM, Boris Fiuczynski wrote:
> On 12/03/2015 02:05 PM, Boris Fiuczynski wrote:
>> conf: Revert some code to resolve issues for hostdev hotplug
>>
>> This patch reverts parts of commits 0d8b24f6b and 0785966d dealing with
>> the addition of a controller during virDomainHostdevAssignAddress. This
>> caused a regression for the hostdev hotplug path which assumes the
>> qemuDomainFindOrCreateSCSIDiskController will add the new controller
>> during qemuDomainAttachHostSCSIDevice to both the running domain and
>> the domain def controller list when the controller doesn't yet exist
>> (whether due to no SCSI controllers existing or the addition of a new
>> controller because existing ones are full).
>>
>> Since commit id 0d8b24f6 will call virDomainHostdevAssignAddress during
>> virDomainDeviceDefPostParseInternal which is called either during domain
>> definition post processing (via an iterator during virDomainDefPostParse)
>> or directly from virDomainDeviceDefParse during hotplug, the change
>> broke the "side effect" of being able to add both a hostdev and
>> controller
>> to the running domain.
>>
>> The regression would only be seen if the running domain didn't have a
>> SCSI controller alrady defined or if the existing SCSI controller was
> typo  already
> 
>> "full" of devices and a new controller needed to be created.
>>
>> This patch will also add some extra comments to the code to avoid a
>> similar future change.
> 
> 

I have pushed this pair of changes as one patch.

I'm hesitant to push the third patch because I'm not sure what it might
break.  I'll address it in a response to that patch though...

John

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

Re: [libvirt] [libvirt-glib] gobject, gconfig: Drop redundant debug logging

2015-12-03 Thread Daniel P. Berrange 
On Thu, Dec 03, 2015 at 02:43:32PM +, Zeeshan Ali (Khattak) wrote:
> Justification from IRC chat:
> 
>  danpb: i'm not sure it's good to have a g_debug() for every
>  libvirt-gobject/gconfig object creation/destruction
>  danpb: the debug output becomes so big that at least I never
>  think about enabling it at runtime and sometimes end up missing
>  important debug info
>   yeah we could easily kill that
>   gobject has systemtap probes builtin these days which let you
>  debug object create/finalize problems
>  danpb: oh cool
>  danpb: i'll write a patch

The commit message could better say:

  "The g_debug messages in libvirt-gobject creation/destruction
   were to aid troubleshooting of object reference leaks. GObject
   has built-in systemtap probes that allow the same to be achieved
   in a more flexible manner, so the g_debug calls can be removed"

ACK with that change.

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] libxl: add p2p migration

2015-12-03 Thread Joao Martins 


On 12/02/2015 11:27 PM, Jim Fehlig wrote:
> On 11/10/2015 08:32 AM, Joao Martins wrote:
>> Introduce support for VIR_MIGRATE_PEER2PEER in libxl driver
>> for supporting migration in Openstack. Most of the changes
>> occur at the source and no modifications at the receiver.
>>
>> In P2P mode there is only the Perform phase so we must handle
>> the connection with the destination and actually perform the
>> migration. libxlDomainPerformP2P implements the connection to
>> the destination and let libxlDoMigrateP2P implements the actual
>> migration logic with virConnectPtr. In this function we do
>> the migration steps in the destination similar to
>> virDomainMigrateVersion3Full. We appropriately save the last
>> error reported in each of the phases to provide proper
>> reporting. We don't yet support VIR_MIGRATE_TUNNELED and
>> we always use V3 with extensible params, making the
>> implementation simpler.
>>
>> It is worth noting that the receiver didn't have any changes,
>> and because it's still the v3 sequence thus it is possible to
>> migrate from a P2P to non-P2P host.
>>
>> Signed-off-by: Joao Martins 
>> ---
>> Changes since v1:
>>  - Move Begin step to libxlDoMigrateP2P to have all 4 steps
>>  together.
>>  - Remove if before VIR_FREE(dom_xml)
>> ---
>>  src/libxl/libxl_driver.c|  13 ++-
>>  src/libxl/libxl_migration.c | 220 
>> 
>>  src/libxl/libxl_migration.h |  11 +++
>>  3 files changed, 241 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index fcdcbdb..da98265 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -4713,6 +4713,7 @@ libxlConnectSupportsFeature(virConnectPtr conn, int 
>> feature)
>>  switch (feature) {
>>  case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
>>  case VIR_DRV_FEATURE_MIGRATION_PARAMS:
>> +case VIR_DRV_FEATURE_MIGRATION_P2P:
>>  return 1;
>>  default:
>>  return 0;
>> @@ -5039,9 +5040,15 @@ libxlDomainMigratePerform3Params(virDomainPtr dom,
>>  if (virDomainMigratePerform3ParamsEnsureACL(dom->conn, vm->def) < 0)
>>  goto cleanup;
>>  
>> -if (libxlDomainMigrationPerform(driver, vm, dom_xml, dconnuri,
>> -uri, dname, flags) < 0)
>> -goto cleanup;
>> +if (flags & VIR_MIGRATE_PEER2PEER) {
>> +if (libxlDomainMigrationPerformP2P(driver, vm, dom->conn, dom_xml,
>> +   dconnuri, uri, dname, flags) < 0)
>> +goto cleanup;
>> +} else {
>> +if (libxlDomainMigrationPerform(driver, vm, dom_xml, dconnuri,
>> +uri, dname, flags) < 0)
>> +goto cleanup;
>> +}
>>  
>>  ret = 0;
>>  
>> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
>> index 0d23e5f..a1c7b55 100644
>> --- a/src/libxl/libxl_migration.c
>> +++ b/src/libxl/libxl_migration.c
>> @@ -42,6 +42,7 @@
>>  #include "libxl_conf.h"
>>  #include "libxl_migration.h"
>>  #include "locking/domain_lock.h"
>> +#include "virtypedparam.h"
>>  
>>  #define VIR_FROM_THIS VIR_FROM_LIBXL
>>  
>> @@ -456,6 +457,225 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
>>  return ret;
>>  }
>>  
>> +/* This function is a simplification of virDomainMigrateVersion3Full
>> + * excluding tunnel support and restricting it to migration v3
>> + * with params since it was the first to be introduced in libxl.
>> + */
>> +static int
>> +libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
>> +  virDomainObjPtr vm,
>> +  virConnectPtr sconn,
>> +  const char *xmlin,
>> +  virConnectPtr dconn,
>> +  const char *dconnuri ATTRIBUTE_UNUSED,
>> +  const char *dname,
>> +  const char *uri,
>> +  unsigned int flags)
>> +{
>> +virDomainPtr ddomain = NULL;
>> +virTypedParameterPtr params = NULL;
>> +int nparams = 0;
>> +int maxparams = 0;
>> +char *uri_out = NULL;
>> +char *dom_xml = NULL;
>> +unsigned long destflags;
>> +bool cancelled = true;
>> +virErrorPtr orig_err = NULL;
>> +int ret = -1;
>> +
>> +dom_xml = libxlDomainMigrationBegin(sconn, vm, xmlin);
>> +if (!dom_xml)
>> +goto cleanup;
>> +
>> +if (virTypedParamsAddString(, , ,
>> +VIR_MIGRATE_PARAM_DEST_XML, dom_xml) < 0)
>> +goto cleanup;
>> +
>> +if (dname &&
>> +virTypedParamsAddString(, , ,
>> +VIR_MIGRATE_PARAM_DEST_NAME, dname) < 0)
>> +goto cleanup;
>> +
>> +if (uri &&
>> +virTypedParamsAddString(, , ,
>> +VIR_MIGRATE_PARAM_URI, uri) < 0)
>> +goto cleanup;
>> +
>> +/* We don't require the destination to have P2P support
>> + * as it looks to be normal migration from the

Re: [libvirt] [PATCH v3 8/8] libxl: implement virDomainGetJobStats

2015-12-03 Thread Joao Martins 


On 12/03/2015 06:48 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>> Introduces support for domainGetJobStats which has the same
>> info as domainGetJobInfo but in a slightly different format.
>> Another difference is that virDomainGetJobStats can also
>> retrieve info on the most recently completed job. Though so
>> far this is only used in the source node to know if the
>> migration has been completed. But because we don't support
>> completed jobs we will deliver an error.
> 
> This patch, and 7/8, look good - ACK. Nice to see all the
> 
> error : virDomainGetJobInfo:8844 : this function is not supported by the
> connection driver: virDomainGetJobInfo
> 
> log entries disappear when doing save, migrate, etc :-). But I'll wait until 
> the
> freeze is lifted to push them.
Great! And it's also nice to see migration no longer crashing on Openstack,
since the monitoring of it relies on these API calls. Perhaps in the future with
per-domain libxl logs we could provide better support for migration progress.

Regards,
Joao

> 
> Regards,
> Jim
> 
>>
>> Signed-off-by: Joao Martins 
>> ---
>> Changes since v1:
>>  - Fixed indentation on libxlDomainGetJobStats()
>>  - s/estimed/estimated/g
>>  - Bump version to 1.2.22
>> ---
>>  src/libxl/libxl_driver.c | 53 
>> 
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index b0b6ea7..dda14c2 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -5273,6 +5273,58 @@ libxlDomainGetJobInfo(virDomainPtr dom,
>>  return ret;
>>  }
>>  
>> +static int
>> +libxlDomainGetJobStats(virDomainPtr dom,
>> +   int *type,
>> +   virTypedParameterPtr *params,
>> +   int *nparams,
>> +   unsigned int flags)
>> +{
>> +libxlDomainObjPrivatePtr priv;
>> +virDomainObjPtr vm;
>> +virDomainJobInfoPtr jobInfo;
>> +int ret = -1;
>> +int maxparams = 0;
>> +
>> +/* VIR_DOMAIN_JOB_STATS_COMPLETED not supported yet */
>> +virCheckFlags(0, -1);
>> +
>> +if (!(vm = libxlDomObjFromDomain(dom)))
>> +goto cleanup;
>> +
>> +if (virDomainGetJobStatsEnsureACL(dom->conn, vm->def) < 0)
>> +goto cleanup;
>> +
>> +priv = vm->privateData;
>> +jobInfo = priv->job.current;
>> +if (!priv->job.active) {
>> +*type = VIR_DOMAIN_JOB_NONE;
>> +*params = NULL;
>> +*nparams = 0;
>> +ret = 0;
>> +goto cleanup;
>> +}
>> +
>> +/* In libxl we don't have an estimated completion time
>> + * thus we always set to unbounded and update time
>> + * for the active job. */
>> +if (libxlDomainJobUpdateTime(>job) < 0)
>> +goto cleanup;
>> +
>> +if (virTypedParamsAddULLong(params, nparams, ,
>> +VIR_DOMAIN_JOB_TIME_ELAPSED,
>> +jobInfo->timeElapsed) < 0)
>> +goto cleanup;
>> +
>> +*type = jobInfo->type;
>> +ret = 0;
>> +
>> + cleanup:
>> +if (vm)
>> +virObjectUnlock(vm);
>> +return ret;
>> +}
>> +
>>  #undef LIBXL_SET_MEMSTAT
>>  
>>  #define LIBXL_RECORD_UINT(error, key, value, ...) \
>> @@ -6134,6 +6186,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>>  .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>>  .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
>>  .domainGetJobInfo = libxlDomainGetJobInfo, /* 1.2.22 */
>> +.domainGetJobStats = libxlDomainGetJobStats, /* 1.2.22 */
>>  .domainBlockStats = libxlDomainBlockStats, /* 1.2.22 */
>>  .domainBlockStatsFlags = libxlDomainBlockStatsFlags, /* 1.2.22 */
>>  .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */
> 

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

Re: [libvirt] [PATCH 04/10] pci: Introduce virPCIDeviceIOMMUGroupIterate()

2015-12-03 Thread Laine Stump 

On 12/02/2015 12:39 PM, Andrea Bolognani wrote:

This is a straightforward wrapper around
virPCIDeviceAddressIOMMUGroupIterate() that will make some code less
awkward to write later on.
---
  src/libvirt_private.syms |  1 +
  src/util/virpci.c| 26 ++
  src/util/virpci.h|  8 ++--
  3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c1fd9f6..f8aaa4c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1967,6 +1967,7 @@ virPCIDeviceGetStubDriver;
  virPCIDeviceGetUnbindFromStub;
  virPCIDeviceGetUsedBy;
  virPCIDeviceHasPCIExpressLink;
+virPCIDeviceIOMMUGroupIterate;
  virPCIDeviceIsAssignable;
  virPCIDeviceIsPCIExpress;
  virPCIDeviceListAdd;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index e82583a..d3b2c7e 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1786,6 +1786,32 @@ void virPCIDeviceReattachInit(virPCIDevicePtr pci)
  pci->reprobe = true;
  }
  
+/**

+ * virPCIDeviceIOMMUGroupIterate:
+ * @dev: PCI device
+ * @actor: function to be called for all PCI addresses in @dev's IOMMU group
+ * @opaque: data passed as the last parameter to @actor
+ *
+ * Convenience wrapper around virPCIDeviceAddressIOMMUGroupIterate().
+ *
+ * Behaves exactly the same, except it takes a virPCIDevicePtr instead of a
+ * virPCIDeviceAddressPtr as its first argument.
+ *
+ * Returns: 0 on success, <0 on failure.
+ */
+int
+virPCIDeviceIOMMUGroupIterate(virPCIDevicePtr dev,
+  virPCIDeviceAddressActor actor,
+  void *opaque)
+{
+virPCIDeviceAddress devAddr = { dev->domain, dev->bus,
+dev->slot, dev->function };
+
+return virPCIDeviceAddressIOMMUGroupIterate(,
+actor,
+opaque);
+}


Instead of creating this new function, how about if we change 
virPCIDevice to contain a virPCIDeviceAddress rather than individual 
domain, bus, slot, and function?


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

Re: [libvirt] [PATCH v3 8/8] libxl: implement virDomainGetJobStats

2015-12-03 Thread Jim Fehlig 
Joao Martins wrote:
> Introduces support for domainGetJobStats which has the same
> info as domainGetJobInfo but in a slightly different format.
> Another difference is that virDomainGetJobStats can also
> retrieve info on the most recently completed job. Though so
> far this is only used in the source node to know if the
> migration has been completed. But because we don't support
> completed jobs we will deliver an error.

This patch, and 7/8, look good - ACK. Nice to see all the

error : virDomainGetJobInfo:8844 : this function is not supported by the
connection driver: virDomainGetJobInfo

log entries disappear when doing save, migrate, etc :-). But I'll wait until the
freeze is lifted to push them.

Regards,
Jim

> 
> Signed-off-by: Joao Martins 
> ---
> Changes since v1:
>  - Fixed indentation on libxlDomainGetJobStats()
>  - s/estimed/estimated/g
>  - Bump version to 1.2.22
> ---
>  src/libxl/libxl_driver.c | 53 
> 
>  1 file changed, 53 insertions(+)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index b0b6ea7..dda14c2 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -5273,6 +5273,58 @@ libxlDomainGetJobInfo(virDomainPtr dom,
>  return ret;
>  }
>  
> +static int
> +libxlDomainGetJobStats(virDomainPtr dom,
> +   int *type,
> +   virTypedParameterPtr *params,
> +   int *nparams,
> +   unsigned int flags)
> +{
> +libxlDomainObjPrivatePtr priv;
> +virDomainObjPtr vm;
> +virDomainJobInfoPtr jobInfo;
> +int ret = -1;
> +int maxparams = 0;
> +
> +/* VIR_DOMAIN_JOB_STATS_COMPLETED not supported yet */
> +virCheckFlags(0, -1);
> +
> +if (!(vm = libxlDomObjFromDomain(dom)))
> +goto cleanup;
> +
> +if (virDomainGetJobStatsEnsureACL(dom->conn, vm->def) < 0)
> +goto cleanup;
> +
> +priv = vm->privateData;
> +jobInfo = priv->job.current;
> +if (!priv->job.active) {
> +*type = VIR_DOMAIN_JOB_NONE;
> +*params = NULL;
> +*nparams = 0;
> +ret = 0;
> +goto cleanup;
> +}
> +
> +/* In libxl we don't have an estimated completion time
> + * thus we always set to unbounded and update time
> + * for the active job. */
> +if (libxlDomainJobUpdateTime(>job) < 0)
> +goto cleanup;
> +
> +if (virTypedParamsAddULLong(params, nparams, ,
> +VIR_DOMAIN_JOB_TIME_ELAPSED,
> +jobInfo->timeElapsed) < 0)
> +goto cleanup;
> +
> +*type = jobInfo->type;
> +ret = 0;
> +
> + cleanup:
> +if (vm)
> +virObjectUnlock(vm);
> +return ret;
> +}
> +
>  #undef LIBXL_SET_MEMSTAT
>  
>  #define LIBXL_RECORD_UINT(error, key, value, ...) \
> @@ -6134,6 +6186,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>  .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>  .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
>  .domainGetJobInfo = libxlDomainGetJobInfo, /* 1.2.22 */
> +.domainGetJobStats = libxlDomainGetJobStats, /* 1.2.22 */
>  .domainBlockStats = libxlDomainBlockStats, /* 1.2.22 */
>  .domainBlockStatsFlags = libxlDomainBlockStatsFlags, /* 1.2.22 */
>  .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */

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