Re: [libvirt] [PATCH 2/3] Qemu: add CMT support

[Ren, Qiaowei]

On Jul 7, 2015 15:51, Ren, Qiaowei wrote:
> 
> 
> On Jul 6, 2015 14:49, Prerna wrote:
> 
>> On Sun, Jul 5, 2015 at 5:13 PM, Qiaowei Ren >  > wrote:
>> 
>> 
>>  One RFC in
>>  https://www.redhat.com/archives/libvir-list/2015-June/msg01509.html
>> 
>>  CMT (Cache Monitoring Technology) can be used to measure the
>>  usage of cache by VM running on the host. This patch will
>>  extend the bulk stats API (virDomainListGetStats) to add this
>>  field. Applications based on libvirt can use this API to achieve
>>  cache usage of VM. Because CMT implementation in Linux kernel
>>  is based on perf mechanism, this patch will enable perf event
>>  for CMT when VM is created and disable it when VM is destroyed.
>> 
>> 
>> 
>> 
>> Hi Ren,
>> 
>> One query wrt this implementation. I see you make a perf ioctl to
>> gather CMT stats each time the stats API is invoked.
>> 
>> If the CMT stats are exposed by a hardware counter, then this implies
>> logging on a per-cpu (or per-socket ???) basis.
>> 
>> This also implies that the value read will vary as the CPU (or socket)
>> on which it is being called changes.
>> 
>> 
>> Now, with this background, if we need real-world stats on a VM, we need
>> this perf ioctl executed on all CPUs/ sockets on which the VM ran.
>> Also, once done, we will need to aggregate results from each of these
>> sources.
>> 
>> 
>> In this implementation, I am missing this -- there seems no control
>> over which physical CPU the libvirt worker thread will run and collect
>> the perf data from. Data collected from this implementation might not
>> accurately model the system state.
>> 
>> I _think_ libvirt currently has no way of directing a worker thread to
>> collect stats from a given CPU -- if we do, I would be happy to learn
>> about it :)
>> 
> 
> Prerna, thanks for your reply. I checked the CMT implementation in
> kernel, and noticed that the series implement new ->count() of pmu
> driver which can aggregate the results from each cpu if perf type is
> PERF_TYPE_INTEL_CQM . The following is the link for the patch:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?i
> d=bfe1fc d2688f557a6b6a88f59ea7619228728bd7
> 
> So I guess that this patch just need to set right perf type and "cpu=-1". Do 
> you
> think this is ok?
> 

Hi Prerna,

Do you have more comments on this patch series? I would be glad to update my 
implementation. ^-^

Qiaowei


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

Re: [libvirt] [libvirt-glib PATCHv5 7/7] gobject: Add wrapper for virNetworkGetDHCPLeases

[Christophe Fergeau]

On Wed, Jul 08, 2015 at 07:00:36PM +0100, Zeeshan Ali (Khattak) wrote:
> On Wed, Jul 8, 2015 at 3:42 PM, Christophe Fergeau  
> wrote:
> > but you haven't brought
> > anything forward to support that "ugly hack" statement in this specific
> > case,
> 
> #ifdef based solution is going to be ugly, surely you know that. I
> never made any claims about the magnitude of ugliness, I always want
> to avoid ugly hacks whenever possible.

If it just requires 2 or 3 #ifdef, adding them and forgetting they
existed would have been faster than this thread ;)

> 
> > nor any hard data regarding which distros could be impacted by a
> > req bump. I'll stop this discussion until you bring some concrete
> > datapoints to the table.
> 
> Fair enough! The main point of this discussion was not to convince you
> but rather to get a third opinion.

Honestly, this is a very weird attitude, rather than trying to
come with hard facts, you prefer having some kind of poll and make an
arbitary uninformed decision.

Christophe



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

Re: [libvirt] [PATCH v4 2/5] qemu: Enable migration events on QMP monitor

[Peter Krempa]

On Wed, Jul 08, 2015 at 18:39:38 +0200, Jiri Denemark wrote:
> Even if QEMU supports migration events it doesn't send them by default.
> We have to enable them by calling migrate-set-capabilities. Let's enable
> migration events everytime we can and clear QEMU_CAPS_MIGRATION_EVENT in
> case migrate-set-capabilities does not support events.
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> Version 4:
> - new patch
> 
>  src/qemu/qemu_monitor.c |  2 +-
>  src/qemu/qemu_monitor.h |  1 +
>  src/qemu/qemu_process.c | 46 ++
>  3 files changed, 36 insertions(+), 13 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index fb325b6..54695c2 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -163,7 +163,7 @@ VIR_ENUM_IMPL(qemuMonitorMigrationStatus,
>  
>  VIR_ENUM_IMPL(qemuMonitorMigrationCaps,
>QEMU_MONITOR_MIGRATION_CAPS_LAST,
> -  "xbzrle", "auto-converge", "rdma-pin-all")
> +  "xbzrle", "auto-converge", "rdma-pin-all", "events")
>  
>  VIR_ENUM_IMPL(qemuMonitorVMStatus,
>QEMU_MONITOR_VM_STATUS_LAST,
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 8555f7b..ab7d5a7 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -512,6 +512,7 @@ typedef enum {
>  QEMU_MONITOR_MIGRATION_CAPS_XBZRLE,
>  QEMU_MONITOR_MIGRATION_CAPS_AUTO_CONVERGE,
>  QEMU_MONITOR_MIGRATION_CAPS_RDMA_PIN_ALL,
> +QEMU_MONITOR_MIGRATION_CAPS_EVENTS,
>  
>  QEMU_MONITOR_MIGRATION_CAPS_LAST
>  } qemuMonitorMigrationCaps;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index ba84182..04e7e93 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1546,7 +1546,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, 
> virDomainObjPtr vm, int asyncJob,
> vm->def) < 0) {
>  VIR_ERROR(_("Failed to set security context for monitor for %s"),
>vm->def->name);
> -goto error;
> +return -1;
>  }
>  
>  /* Hold an extra reference because we can't allow 'vm' to be
> @@ -1578,26 +1578,48 @@ qemuConnectMonitor(virQEMUDriverPtr driver, 
> virDomainObjPtr vm, int asyncJob,
>  if (virSecurityManagerClearSocketLabel(driver->securityManager, vm->def) 
> < 0) {
>  VIR_ERROR(_("Failed to clear security context for monitor for %s"),
>vm->def->name);
> -goto error;
> +return -1;
>  }
>  
>  if (priv->mon == NULL) {
>  VIR_INFO("Failed to connect monitor for %s", vm->def->name);
> -goto error;
> +return -1;
>  }
>  
>  
>  if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> -goto error;
> -ret = qemuMonitorSetCapabilities(priv->mon);
> -if (ret == 0 &&
> -virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON))
> -ret = virQEMUCapsProbeQMP(priv->qemuCaps, priv->mon);
> +return -1;
> +
> +if (qemuMonitorSetCapabilities(priv->mon) < 0)
> +goto cleanup;
> +
> +if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON) &&
> +virQEMUCapsProbeQMP(priv->qemuCaps, priv->mon) < 0)
> +goto cleanup;
> +
> +if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
> +int rv;
> +
> +rv = qemuMonitorGetMigrationCapability(
> +priv->mon, QEMU_MONITOR_MIGRATION_CAPS_EVENTS);

Is it actually worth querying for the capability? Wouldn't it be
sufficient to just set it and if it fails, clear the capability bit
right away?

> +if (rv < 0) {
> +goto cleanup;
> +} else if (rv == 0) {
> +VIR_DEBUG("Cannot enable migration events; clearing capability");
> +virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
> +} else if (qemuMonitorSetMigrationCapability(
> +priv->mon,
> +QEMU_MONITOR_MIGRATION_CAPS_EVENTS,
> +true) < 0) {

Hmm, this formatting looks rather ugly to me. Do we still need to do
this silly 80 column limit in the age of wide angle displays?

> +goto cleanup;
> +}
> +}
> +
> +ret = 0;
> +
> + cleanup:
>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
> -return -1;
> -
> - error:
> -
> +ret = -1;
>  return ret;

Peter


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

[libvirt] [PATCH v5] qemu: Enable migration events on QMP monitor

[Jiri Denemark]

Even if QEMU supports migration events it doesn't send them by default.
We have to enable them by calling migrate-set-capabilities. Let's enable
migration events everytime we can and clear QEMU_CAPS_MIGRATION_EVENT in
case migrate-set-capabilities does not support events.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 5:
- simplified

Version 4:
- new patch

 src/qemu/qemu_monitor.c |  2 +-
 src/qemu/qemu_monitor.h |  1 +
 src/qemu/qemu_process.c | 36 
 3 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index fb325b6..54695c2 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -163,7 +163,7 @@ VIR_ENUM_IMPL(qemuMonitorMigrationStatus,
 
 VIR_ENUM_IMPL(qemuMonitorMigrationCaps,
   QEMU_MONITOR_MIGRATION_CAPS_LAST,
-  "xbzrle", "auto-converge", "rdma-pin-all")
+  "xbzrle", "auto-converge", "rdma-pin-all", "events")
 
 VIR_ENUM_IMPL(qemuMonitorVMStatus,
   QEMU_MONITOR_VM_STATUS_LAST,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 8555f7b..ab7d5a7 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -512,6 +512,7 @@ typedef enum {
 QEMU_MONITOR_MIGRATION_CAPS_XBZRLE,
 QEMU_MONITOR_MIGRATION_CAPS_AUTO_CONVERGE,
 QEMU_MONITOR_MIGRATION_CAPS_RDMA_PIN_ALL,
+QEMU_MONITOR_MIGRATION_CAPS_EVENTS,
 
 QEMU_MONITOR_MIGRATION_CAPS_LAST
 } qemuMonitorMigrationCaps;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ba84182..1d223d3 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1546,7 +1546,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, 
virDomainObjPtr vm, int asyncJob,
vm->def) < 0) {
 VIR_ERROR(_("Failed to set security context for monitor for %s"),
   vm->def->name);
-goto error;
+return -1;
 }
 
 /* Hold an extra reference because we can't allow 'vm' to be
@@ -1578,26 +1578,38 @@ qemuConnectMonitor(virQEMUDriverPtr driver, 
virDomainObjPtr vm, int asyncJob,
 if (virSecurityManagerClearSocketLabel(driver->securityManager, vm->def) < 
0) {
 VIR_ERROR(_("Failed to clear security context for monitor for %s"),
   vm->def->name);
-goto error;
+return -1;
 }
 
 if (priv->mon == NULL) {
 VIR_INFO("Failed to connect monitor for %s", vm->def->name);
-goto error;
+return -1;
 }
 
 
 if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
-goto error;
-ret = qemuMonitorSetCapabilities(priv->mon);
-if (ret == 0 &&
-virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON))
-ret = virQEMUCapsProbeQMP(priv->qemuCaps, priv->mon);
+return -1;
+
+if (qemuMonitorSetCapabilities(priv->mon) < 0)
+goto cleanup;
+
+if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON) &&
+virQEMUCapsProbeQMP(priv->qemuCaps, priv->mon) < 0)
+goto cleanup;
+
+if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT) &&
+qemuMonitorSetMigrationCapability(priv->mon,
+  QEMU_MONITOR_MIGRATION_CAPS_EVENTS,
+  true) < 0) {
+VIR_DEBUG("Cannot enable migration events; clearing capability");
+virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
+}
+
+ret = 0;
+
+ cleanup:
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
-return -1;
-
- error:
-
+ret = -1;
 return ret;
 }
 
-- 
2.4.5

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

Re: [libvirt] [PATCH v3 4/4] qemu: Fix integer/boolean logic in qemuSetUnprivSGIO

[Peter Krempa]

On Wed, Jul 08, 2015 at 15:30:28 -0400, John Ferlan wrote:
> Setting of 'val' is a boolean expression, so handle it that way and
> adjust the check/return logic to be clearer
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_conf.c | 10 +++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index ddaf7f8..2ab5494 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1433,7 +1433,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
>  virDomainHostdevDefPtr hostdev = NULL;
>  char *sysfs_path = NULL;
>  const char *path = NULL;
> -int val = -1;
> +bool val;
>  int ret = -1;
>  
>  /* "sgio" is only valid for block disk; cdrom
> @@ -1475,8 +1475,12 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
>   * whitelist is enabled.  But if requesting unfiltered access, always 
> call
>   * virSetDeviceUnprivSGIO, to report an error for unsupported 
> unpriv_sgio.
>   */
> -if ((virFileExists(sysfs_path) || val == 1) &&
> -virSetDeviceUnprivSGIO(path, NULL, val) < 0)
> +if (!val || !virFileExists(sysfs_path)) {

With this control flow the @val variable can be entirely avoided since
it's just set once and read once.

> +ret = 0;
> +goto cleanup;
> +}
> +
> +if (virSetDeviceUnprivSGIO(path, NULL, 1) < 0)
>  goto cleanup;
>  
>  ret = 0;

ACK regardles if you choose to optimize @val out.

Peter


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

Re: [libvirt] [PATCH v3 3/4] qemu: Refactor qemuSetUnprivSGIO return values

[Peter Krempa]

On Wed, Jul 08, 2015 at 15:30:27 -0400, John Ferlan wrote:
> Set to ret = -1 and prove otherwise, like usual
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_conf.c | 12 +---
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 

ACK


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

Re: [libvirt] [PATCH v3 2/4] qemu: Inline qemuGetHostdevPath

[Peter Krempa]

On Wed, Jul 08, 2015 at 15:30:26 -0400, John Ferlan wrote:
> Since a future patch will need the device path generated when adding a
> shared host device, remove the qemuAddSharedHostdev and inline the two
> calls into qemuAddSharedHostdev and qemuRemoveSharedHostdev
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_conf.c | 39 ---
>  1 file changed, 16 insertions(+), 23 deletions(-)

ACK


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

[libvirt] [PATCH] cpu: Add support for MPX and AVX512 Intel features

[Jiri Denemark]

Corresponding QEMU commits:
MPX 79e9ebebbf2a00c46fcedb6dc7dd5e12bbd30216
AVX512  9aecd6f8aef653cea58932f06a2740299dbe5fd3

Signed-off-by: Jiri Denemark 
---
 src/cpu/cpu_map.xml | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml
index b9e95cf..b924bd3 100644
--- a/src/cpu/cpu_map.xml
+++ b/src/cpu/cpu_map.xml
@@ -317,6 +317,12 @@
 
   
 
+
+  
+
+ 
+  
+
 
   
 
@@ -326,6 +332,15 @@
 
   
 
+ 
+  
+
+ 
+  
+
+ 
+  
+
 
 
 
-- 
2.4.5

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

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

[Peter Krempa]

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

Yay \o/, finally this is taken care of.

ACK

Peter


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

[libvirt] [libvirt-glib 0/4] gconfig: Leak fixes and small cleanup

[Christophe Fergeau]

Hey,

This patch series makes tests/test-gconfig valgrind-clean, and refactors two
setters in GVirConfigDomainVideo to make them use the helpers provided by
GVirConfigObject.

Christophe

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

Re: [libvirt] [PATCH v5] qemu: Enable migration events on QMP monitor

[Peter Krempa]

On Thu, Jul 09, 2015 at 09:11:16 +0200, Jiri Denemark wrote:
> Even if QEMU supports migration events it doesn't send them by default.
> We have to enable them by calling migrate-set-capabilities. Let's enable
> migration events everytime we can and clear QEMU_CAPS_MIGRATION_EVENT in
> case migrate-set-capabilities does not support events.
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> Version 5:
> - simplified
> 
> Version 4:
> - new patch
> 
>  src/qemu/qemu_monitor.c |  2 +-
>  src/qemu/qemu_monitor.h |  1 +
>  src/qemu/qemu_process.c | 36 
>  3 files changed, 26 insertions(+), 13 deletions(-)
> 

ACK,

Peter


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

[libvirt] [libvirt-glib 3/4] test-gconfig: Test video heads/vram setting

[Christophe Fergeau]

---
 tests/test-gconfig.c  | 2 ++
 tests/xml/gconfig-domain-device-video.xml | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/test-gconfig.c b/tests/test-gconfig.c
index de09c24..aca8f02 100644
--- a/tests/test-gconfig.c
+++ b/tests/test-gconfig.c
@@ -494,6 +494,8 @@ static void test_domain_device_video(void)
 video = gvir_config_domain_video_new();
 gvir_config_domain_video_set_model(video,
GVIR_CONFIG_DOMAIN_VIDEO_MODEL_QXL);
+gvir_config_domain_video_set_heads(video, 4);
+gvir_config_domain_video_set_vram(video, 256*1024);
 gvir_config_domain_add_device(domain, GVIR_CONFIG_DOMAIN_DEVICE(video));
 g_object_unref(G_OBJECT(video));
 
diff --git a/tests/xml/gconfig-domain-device-video.xml 
b/tests/xml/gconfig-domain-device-video.xml
index a6155e2..7af6256 100644
--- a/tests/xml/gconfig-domain-device-video.xml
+++ b/tests/xml/gconfig-domain-device-video.xml
@@ -1,7 +1,7 @@
 
   
 
-  
+  
 
   
 
-- 
2.4.3

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

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

[Peter Krempa]

On Wed, Jul 08, 2015 at 19:36:06 +0200, Jiri Denemark wrote:
> For quite a long time we don't need to postpone queueing events until
> the end of the function since we no longer have the big driver lock.
> Let's make the code of qemuMigrationFinish simpler by queuing events at
> the time we generate them.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_migration.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index ae7433e..cc7754c 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c

...

> @@ -5709,16 +5708,17 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  
>  dom = virGetDomain(dconn, vm->def->name, vm->def->uuid);
>  
> -event = virDomainEventLifecycleNewFromObj(vm,
> - VIR_DOMAIN_EVENT_RESUMED,
> - VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
> +qemuDomainEventQueue(driver,
> + virDomainEventLifecycleNewFromObj(vm,
> +VIR_DOMAIN_EVENT_RESUMED,
> +VIR_DOMAIN_EVENT_RESUMED_MIGRATED));

I dislike this formatting.

>  if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
>  virDomainObjSetState(vm, VIR_DOMAIN_PAUSED,
>   VIR_DOMAIN_PAUSED_USER);
> -qemuDomainEventQueue(driver, event);
> -event = virDomainEventLifecycleNewFromObj(vm,
> - VIR_DOMAIN_EVENT_SUSPENDED,
> - 
> VIR_DOMAIN_EVENT_SUSPENDED_PAUSED);
> +qemuDomainEventQueue(driver,
> + virDomainEventLifecycleNewFromObj(vm,
> +VIR_DOMAIN_EVENT_SUSPENDED,
> +VIR_DOMAIN_EVENT_SUSPENDED_PAUSED));
>  }
>  
>  if (virDomainObjIsActive(vm) &&

ACK,

Peter


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

[libvirt] [libvirt-glib 2/4] test-gconfig: Fix various leaks

[Christophe Fergeau]

Running test-gconfig under valgrind reports a few leaks that this commit
fixes.
---
 tests/test-gconfig.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/tests/test-gconfig.c b/tests/test-gconfig.c
index 0eec53e..de09c24 100644
--- a/tests/test-gconfig.c
+++ b/tests/test-gconfig.c
@@ -48,6 +48,7 @@ static void check_xml(GVirConfigDomain *domain, const char 
*reference_file)
 xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(domain));
 g_assert_cmpstr(xml, ==, reference_xml);
 g_free(xml);
+g_free(reference_xml);
 }
 
 
@@ -224,6 +225,8 @@ static void test_domain_os(void)
 gvir_config_domain_set_os(domain, NULL);
 os = gvir_config_domain_get_os(domain);
 g_assert(os == NULL);
+
+g_object_unref(G_OBJECT(domain));
 }
 
 
@@ -294,9 +297,13 @@ static void test_domain_cpu(void)
   NULL);
 topology = 
gvir_config_capabilities_cpu_get_topology(GVIR_CONFIG_CAPABILITIES_CPU(cpu));
 g_assert(topology == NULL);
+g_object_unref(G_OBJECT(cpu));
+
 gvir_config_domain_set_cpu(domain, NULL);
 cpu = gvir_config_domain_get_cpu(domain);
 g_assert(cpu == NULL);
+
+g_object_unref(G_OBJECT(domain));
 }
 
 
@@ -347,6 +354,7 @@ static void test_domain_device_disk(void)
 g_assert(gvir_config_domain_disk_driver_get_copy_on_read(driver));
 g_assert_cmpint(gvir_config_domain_disk_get_target_bus(disk), ==, 
GVIR_CONFIG_DOMAIN_DISK_BUS_IDE);
 g_assert_cmpstr(gvir_config_domain_disk_get_target_dev(disk), ==, "hda");
+g_object_unref(G_OBJECT(driver));
 
 gvir_config_domain_disk_set_driver(disk, NULL);
 driver = gvir_config_domain_disk_get_driver(disk);
@@ -433,6 +441,7 @@ static void test_domain_device_input(void)
  
GVIR_CONFIG_DOMAIN_INPUT_DEVICE_TABLET);
 gvir_config_domain_input_set_bus(input, GVIR_CONFIG_DOMAIN_INPUT_BUS_USB);
 gvir_config_domain_add_device(domain, GVIR_CONFIG_DOMAIN_DEVICE(input));
+g_object_unref(G_OBJECT(input));
 
 check_xml(domain, "gconfig-domain-device-input.xml");
 
-- 
2.4.3

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

[libvirt] [libvirt-glib 4/4] gconfig: Use GVirConfigObject helpers for XML

[Christophe Fergeau]

GVirConfigDomainVideo is using raw libxml calls to set the 'heads' and
'vram' XML attributes rather than the helpers provided by
GVirConfigObject. This commit changes that, making the code a bit
simpler.
---
 libvirt-gconfig/libvirt-gconfig-domain-video.c | 38 +++---
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/libvirt-gconfig/libvirt-gconfig-domain-video.c 
b/libvirt-gconfig/libvirt-gconfig-domain-video.c
index 947d066..78ac54f 100644
--- a/libvirt-gconfig/libvirt-gconfig-domain-video.c
+++ b/libvirt-gconfig/libvirt-gconfig-domain-video.c
@@ -88,33 +88,27 @@ void 
gvir_config_domain_video_set_model(GVirConfigDomainVideo *video,
 void gvir_config_domain_video_set_vram(GVirConfigDomainVideo *video,
guint kbytes)
 {
-xmlNodePtr node;
-char *vram_str;
+GVirConfigObject *node;
 
-node = gvir_config_object_get_xml_node(GVIR_CONFIG_OBJECT(video));
-if (node == NULL)
-return;
-node = gvir_config_xml_get_element(node, "model", NULL);
-if (node == NULL)
-return;
-vram_str = g_strdup_printf("%u", kbytes);
-xmlNewProp(node, (xmlChar*)"vram", (xmlChar*)vram_str);
-g_free(vram_str);
+g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_VIDEO(video));
+node = gvir_config_object_add_child(GVIR_CONFIG_OBJECT(video), "model");
+g_return_if_fail(GVIR_CONFIG_IS_OBJECT(node));
+gvir_config_object_set_attribute_with_type(node, "vram",
+   G_TYPE_UINT, kbytes,
+   NULL);
+g_object_unref(G_OBJECT(node));
 }
 
 void gvir_config_domain_video_set_heads(GVirConfigDomainVideo *video,
 guint head_count)
 {
-xmlNodePtr node;
-char *heads_str;
+GVirConfigObject *node;
 
-node = gvir_config_object_get_xml_node(GVIR_CONFIG_OBJECT(video));
-if (node == NULL)
-return;
-node = gvir_config_xml_get_element(node, "model", NULL);
-if (node == NULL)
-return;
-heads_str = g_strdup_printf("%u", head_count);
-xmlNewProp(node, (xmlChar*)"heads", (xmlChar*)heads_str);
-g_free(heads_str);
+g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_VIDEO(video));
+node = gvir_config_object_add_child(GVIR_CONFIG_OBJECT(video), "model");
+g_return_if_fail(GVIR_CONFIG_IS_OBJECT(node));
+gvir_config_object_set_attribute_with_type(node, "heads",
+   G_TYPE_UINT, head_count,
+   NULL);
+g_object_unref(G_OBJECT(node));
 }
-- 
2.4.3

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

[libvirt] [libvirt-glib 1/4] gconfig: Fix leak in gvir_config_domain_filesys_set_ram_usage

[Christophe Fergeau]

The object returned by gvir_config_object_replace_child() must be
unref'ed when no longer needed.
---
 libvirt-gconfig/libvirt-gconfig-domain-filesys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libvirt-gconfig/libvirt-gconfig-domain-filesys.c 
b/libvirt-gconfig/libvirt-gconfig-domain-filesys.c
index 860480c..97b7bd6 100644
--- a/libvirt-gconfig/libvirt-gconfig-domain-filesys.c
+++ b/libvirt-gconfig/libvirt-gconfig-domain-filesys.c
@@ -201,7 +201,7 @@ void 
gvir_config_domain_filesys_set_ram_usage(GVirConfigDomainFilesys *filesys,
"usage", G_TYPE_UINT64, bytes,
"units", G_TYPE_STRING, "bytes",
NULL);
-
+g_object_unref(G_OBJECT(src));
 }
 
 void gvir_config_domain_filesys_set_target(GVirConfigDomainFilesys *filesys,
-- 
2.4.3

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

Re: [libvirt] [PATCH v3 1/4] qemu: Refactor qemuCheckSharedDisk to create qemuCheckUnprivSGIO

[Peter Krempa]

On Wed, Jul 08, 2015 at 15:30:25 -0400, John Ferlan wrote:
> Split out the current function in order to share the code with hostdev
> in a future patch. Failure to match the expected sgio value against what
> is stored will cause an error which the caller would need to handle since
> only the caller has the disk (or eventually hostdev) specific data in
> order to uniquely identify the disk in an error message.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_conf.c | 90 
> +++-
>  1 file changed, 61 insertions(+), 29 deletions(-)
> 

ACK, but this patch seems kind of useless without the followup patches
that were dropped due to lack of upstream support.

Peter


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

Re: [libvirt] [PATCH] conf: Don't allow duplicated targets regardless of bus

[Ján Tomko]

On Thu, Jun 18, 2015 at 04:12:10PM -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1142631
> 
> Commit id 'e0e290552' added a check to determine if the same bus
> had the same target value.  It seems that's not quite good enough
> as the check should check the target name value regardless of bus type.
> 
> Also added a DO_TEST_DIFFERENT to exhibit the issue
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/domain_conf.c |  3 +-
>  .../qemuxml2argv-disk-same-targets.xml | 35 
> ++
>  tests/qemuxml2argvtest.c   |  3 ++
>  3 files changed, 39 insertions(+), 2 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-same-targets.xml

ACK

Jan


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

[libvirt] [PATCH v4 0/3] Allow PCI virtio on ARM "virt" machine

[Pavel Fedin]

Virt machine in qemu since v2.3.0 has PCI generic host controller, and can use
PCI devices. This provides performance improvement as well as vhost-net with
irqfd support for virtio-net. However libvirt currently does not allow ARM virt
machine to have PCI devices. This patchset adds the necessary support.

Changes since v3:
- Capability is based not on qemu version but on support of "gpex-pcihost"
  device by qemu
- Added a workaround, allowing to pass "make check". The problem is that
  test suite does not build capabilities cache. Unfortunately this means
  that correct unit-test for the new functionality currently cannot be
  written. Test suite framework needs to be improved.
Changes since v2:
Complete rework, use different approach
- Correctly model PCI Express bus on the machine. It is now possible to
  explicitly specify  with attributes. This allows to
  attach not only virtio, but any other PCI device to the model.
- Default is not changed and still mmio, for backwards compatibility with
  existing installations. PCI bus has to be explicitly specified.
- Check for the capability in correct place, in v2 it actually did not work
Changes since v1:
- Added capability based on qemu version number
- Recognize also "virt-" prefix

Pavel Fedin (3):
  Introduce QEMU_CAPS_OBJECT_GPEX
  Add PCI-Express root to ARM virt machine
  Build correct command line for PCI NICs on ARM

 src/qemu/qemu_capabilities.c |  2 ++
 src/qemu/qemu_capabilities.h |  1 +
 src/qemu/qemu_command.c  |  3 ++-
 src/qemu/qemu_domain.c   | 17 +
 4 files changed, 18 insertions(+), 5 deletions(-)

-- 
1.9.5.msysgit.0

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

[libvirt] [PATCH v4 1/3] Introduce QEMU_CAPS_OBJECT_GPEX

[Pavel Fedin]

This capability specifies that qemu can implement generic PCI host
controller. It is often used for virtual environments, including ARM.

Signed-off-by: Pavel Fedin 
---
 src/qemu/qemu_capabilities.c | 2 ++
 src/qemu/qemu_capabilities.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 27686c3..0f37396 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -287,6 +287,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   "aarch64-off",
 
   "vhost-user-multiqueue", /* 190 */
+  "gpex-pcihost",
 );
 
 
@@ -1566,6 +1567,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
 { "ivshmem", QEMU_CAPS_DEVICE_IVSHMEM },
 { "pc-dimm", QEMU_CAPS_DEVICE_PC_DIMM },
 { "pci-serial", QEMU_CAPS_DEVICE_PCI_SERIAL },
+{ "gpex-pcihost", QEMU_CAPS_OBJECT_GPEX},
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 30aa504..711 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -230,6 +230,7 @@ typedef enum {
 QEMU_CAPS_DEVICE_PCI_SERIAL  = 188, /* -device pci-serial */
 QEMU_CAPS_CPU_AARCH64_OFF= 189, /* -cpu ...,aarch64=off */
 QEMU_CAPS_VHOSTUSER_MULTIQUEUE = 190, /* vhost-user with -netdev queues= */
+QEMU_CAPS_OBJECT_GPEX= 191, /* have generic PCI host controller */
 
 QEMU_CAPS_LAST,   /* this must always be the last item */
 } virQEMUCapsFlags;
-- 
1.9.5.msysgit.0

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

[libvirt] [PATCH v4 3/3] Build correct command line for PCI NICs on ARM

[Pavel Fedin]

Legacy -net option works correctly only with embedded device models, which
do not require any bus specification. Therefore, we should use -device for
PCI hardware

Signed-off-by: Pavel Fedin 
---
 src/qemu/qemu_command.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b7b85ab..9d6be9f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -457,7 +457,8 @@ qemuDomainSupportsNicdev(virDomainDefPtr def,
 /* non-virtio ARM nics require legacy -net nic */
 if (((def->os.arch == VIR_ARCH_ARMV7L) ||
 (def->os.arch == VIR_ARCH_AARCH64)) &&
-net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO)
+net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO &&
+net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
 return false;
 
 return true;
-- 
1.9.5.msysgit.0

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

[libvirt] [PATCH v4 2/3] Add PCI-Express root to ARM virt machine

[Pavel Fedin]

Here we assume that if qemu supports generic PCI host controller,
it is a part of virt machine and can be used for adding PCI devices.

Signed-off-by: Pavel Fedin 
---
 src/qemu/qemu_domain.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8b050a0..c7d14e4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -981,7 +981,7 @@ virDomainXMLNamespace virQEMUDriverDomainXMLNamespace = {
 static int
 qemuDomainDefPostParse(virDomainDefPtr def,
virCapsPtr caps,
-   void *opaque ATTRIBUTE_UNUSED)
+   void *opaque)
 {
 bool addDefaultUSB = true;
 bool addImplicitSATA = false;
@@ -1030,12 +1030,21 @@ qemuDomainDefPostParse(virDomainDefPtr def,
 break;
 
 case VIR_ARCH_ARMV7L:
-   addDefaultUSB = false;
-   addDefaultMemballoon = false;
-   break;
 case VIR_ARCH_AARCH64:
addDefaultUSB = false;
addDefaultMemballoon = false;
+   if (STREQ(def->os.machine, "virt") ||
+   STRPREFIX(def->os.machine, "virt-")) {
+   virQEMUDriverPtr driver = opaque;
+
+   /* This condition is actually a (temporary) hack for test suite 
which
+* does not create capabilities cache */
+   if (driver->qemuCapsCache) {
+   virQEMUCapsPtr qemuCaps =
+   virQEMUCapsCacheLookup(driver->qemuCapsCache, 
def->emulator);
+   addPCIeRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX);
+   }
+   }
break;
 
 case VIR_ARCH_PPC64:
-- 
1.9.5.msysgit.0

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

[libvirt] shall libvirtd validate guest's name ?

[zhang bo]

linux-ZyvZnF:~ # virsh list --all
 IdName   State

 - redhat7;reboot shut off
 - oscar-vm-5 shut off


As shown above, 
1 we use command "virsh define a.xml" to define a guest with a name containing 
';', that's 'redhat7;reboot'
2 then we start the guest: "virsh start redhat7;reboot"
3 shell consider the command as
  a) run "virsh start redhat7", failed
  b) run "reboot", to reboot the host
  And *the host get rebooted*.

shall libvirtd do the guest-name-validation work? Or other suggustions?



-- 
Oscar
oscar.zhan...@huawei.com  

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

[libvirt] [libvirt-glib 3/3] Add ram and vgamem attributes for graphics model.

[T A Mahadevan]

---
 libvirt-gconfig/libvirt-gconfig-domain-video.c | 25 +
 libvirt-gconfig/libvirt-gconfig-domain-video.h |  5 +
 libvirt-gconfig/libvirt-gconfig.sym|  2 ++
 3 files changed, 32 insertions(+)

diff --git a/libvirt-gconfig/libvirt-gconfig-domain-video.c 
b/libvirt-gconfig/libvirt-gconfig-domain-video.c
index 947d066..cc2034d 100644
--- a/libvirt-gconfig/libvirt-gconfig-domain-video.c
+++ b/libvirt-gconfig/libvirt-gconfig-domain-video.c
@@ -102,6 +102,31 @@ void 
gvir_config_domain_video_set_vram(GVirConfigDomainVideo *video,
 g_free(vram_str);
 }
 
+void gvir_config_domain_video_set_ram(GVirConfigDomainVideo *video,
+   guint kbytes)
+{
+GVirConfigObject *node;
+g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_VIDEO(video));
+node = gvir_config_object_add_child(GVIR_CONFIG_OBJECT(video), "model");
+g_return_if_fail(GVIR_CONFIG_OBJECT(node));
+gvir_config_object_set_attribute_with_type(node, "ram", G_TYPE_UINT,
+   kbytes, NULL);
+g_object_unref(G_OBJECT(node));
+}
+
+
+void gvir_config_domain_video_set_vgamem(GVirConfigDomainVideo *video,
+   guint kbytes)
+{
+GVirConfigObject *node;
+g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_VIDEO(video));
+node = gvir_config_object_add_child(GVIR_CONFIG_OBJECT(video), "model");
+g_return_if_fail(GVIR_CONFIG_OBJECT(node));
+gvir_config_object_set_attribute_with_type(node, "vgamem", G_TYPE_UINT,
+   kbytes, NULL);
+g_object_unref(G_OBJECT(node));
+}
+
 void gvir_config_domain_video_set_heads(GVirConfigDomainVideo *video,
 guint head_count)
 {
diff --git a/libvirt-gconfig/libvirt-gconfig-domain-video.h 
b/libvirt-gconfig/libvirt-gconfig-domain-video.h
index f83d5aa..a87ec4f 100644
--- a/libvirt-gconfig/libvirt-gconfig-domain-video.h
+++ b/libvirt-gconfig/libvirt-gconfig-domain-video.h
@@ -74,6 +74,11 @@ void 
gvir_config_domain_video_set_model(GVirConfigDomainVideo *video,
 GVirConfigDomainVideoModel model);
 void gvir_config_domain_video_set_vram(GVirConfigDomainVideo *video,
guint kbytes);
+
+void gvir_config_domain_video_set_ram(GVirConfigDomainVideo *video,
+   guint kbytes);
+void gvir_config_domain_video_set_vgamem(GVirConfigDomainVideo *video,
+   guint kbytes);
 void gvir_config_domain_video_set_heads(GVirConfigDomainVideo *video,
 guint head_count);
 
diff --git a/libvirt-gconfig/libvirt-gconfig.sym 
b/libvirt-gconfig/libvirt-gconfig.sym
index 6267197..89dd589 100644
--- a/libvirt-gconfig/libvirt-gconfig.sym
+++ b/libvirt-gconfig/libvirt-gconfig.sym
@@ -729,6 +729,8 @@ global:
gvir_config_domain_chardev_source_unix_get_type;
gvir_config_domain_chardev_source_unix_new;
gvir_config_domain_chardev_source_unix_new_from_xml;
+   gvir_config_domain_video_set_ram;
+   gvir_config_domain_video_set_vgamem;
 } LIBVIRT_GCONFIG_0.2.1;
 
 #  define new API here using predicted next version number 
-- 
1.9.1

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

[libvirt] [libvirt-glib 2/3] buildsys: Add missing libraries to LDFLAGS

[T A Mahadevan]

Without these changes 'make check' would not
run on Ubuntu because of a link failure as
the tests are directly using glib/libvirt
symbols.
---
 tests/Makefile.am | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 63865e8..c146817 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -12,7 +12,10 @@ AM_CFLAGS = \
 
 LDADD = \
$(top_builddir)/libvirt-gconfig/libvirt-gconfig-1.0.la \
-   $(top_builddir)/libvirt-glib/libvirt-glib-1.0.la
+   $(top_builddir)/libvirt-glib/libvirt-glib-1.0.la \
+   $(LIBVIRT_LIBS) \
+   $(GLIB2_LIBS) \
+   $(GOBJECT2_LIBS)
 
 test_programs = test-gconfig test-events
 
-- 
1.9.1

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

[libvirt] [libvirt-glib 1/3] Add LibvirtGConfigDomainChardevSourceUnix

[T A Mahadevan]

This is needed to be able to add UNIX channels
---
 libvirt-gconfig/Makefile.am|  2 +
 .../libvirt-gconfig-domain-chardev-source-unix.c   | 84 ++
 .../libvirt-gconfig-domain-chardev-source-unix.h   | 68 ++
 libvirt-gconfig/libvirt-gconfig.h  |  1 +
 libvirt-gconfig/libvirt-gconfig.sym|  7 ++
 libvirt-gconfig/tests/test-domain-create.c | 14 
 tests/test-gconfig.c   | 11 +++
 tests/xml/gconfig-domain-device-channel.xml|  3 +
 8 files changed, 190 insertions(+)
 create mode 100644 libvirt-gconfig/libvirt-gconfig-domain-chardev-source-unix.c
 create mode 100644 libvirt-gconfig/libvirt-gconfig-domain-chardev-source-unix.h

diff --git a/libvirt-gconfig/Makefile.am b/libvirt-gconfig/Makefile.am
index a9a6591..77b2032 100644
--- a/libvirt-gconfig/Makefile.am
+++ b/libvirt-gconfig/Makefile.am
@@ -32,6 +32,7 @@ GCONFIG_HEADER_FILES = \
libvirt-gconfig-domain-chardev-source-pty.h \
libvirt-gconfig-domain-chardev-source-spiceport.h \
libvirt-gconfig-domain-chardev-source-spicevmc.h \
+   libvirt-gconfig-domain-chardev-source-unix.h \
libvirt-gconfig-domain-clock.h \
libvirt-gconfig-domain-console.h \
libvirt-gconfig-domain-controller.h \
@@ -122,6 +123,7 @@ GCONFIG_SOURCE_FILES = \
libvirt-gconfig-domain-chardev-source-pty.c \
libvirt-gconfig-domain-chardev-source-spiceport.c \
libvirt-gconfig-domain-chardev-source-spicevmc.c \
+   libvirt-gconfig-domain-chardev-source-unix.c \
libvirt-gconfig-domain-clock.c \
libvirt-gconfig-domain-console.c \
libvirt-gconfig-domain-controller.c \
diff --git a/libvirt-gconfig/libvirt-gconfig-domain-chardev-source-unix.c 
b/libvirt-gconfig/libvirt-gconfig-domain-chardev-source-unix.c
new file mode 100644
index 000..162b788
--- /dev/null
+++ b/libvirt-gconfig/libvirt-gconfig-domain-chardev-source-unix.c
@@ -0,0 +1,84 @@
+/*
+ * libvirt-gconfig-domain-chardev-source-unix.c: libvirt domain chardev unix 
configuration
+ *
+ * Copyright (C) 2012 Red Hat, Inc.
+ * Copyright (C) 2015 T A Mahadevan
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * .
+ *
+ * Author: T A Mahadevan 
+ */
+
+#include 
+
+#include "libvirt-gconfig/libvirt-gconfig.h"
+#include "libvirt-gconfig/libvirt-gconfig-private.h"
+
+#define GVIR_CONFIG_DOMAIN_CHARDEV_SOURCE_UNIX_GET_PRIVATE(obj)
 \
+(G_TYPE_INSTANCE_GET_PRIVATE((obj), 
GVIR_CONFIG_TYPE_DOMAIN_CHARDEV_SOURCE_UNIX, 
GVirConfigDomainChardevSourceUnixPrivate))
+
+struct _GVirConfigDomainChardevSourceUnixPrivate
+{
+gboolean unused;
+};
+
+G_DEFINE_TYPE(GVirConfigDomainChardevSourceUnix, 
gvir_config_domain_chardev_source_unix, GVIR_CONFIG_TYPE_DOMAIN_CHARDEV_SOURCE);
+
+
+static void 
gvir_config_domain_chardev_source_unix_class_init(GVirConfigDomainChardevSourceUnixClass
 *klass)
+{
+g_type_class_add_private(klass, 
sizeof(GVirConfigDomainChardevSourceUnixPrivate));
+}
+
+
+static void 
gvir_config_domain_chardev_source_unix_init(GVirConfigDomainChardevSourceUnix 
*source)
+{
+g_debug("Init GVirConfigDomainChardevSourceUnix=%p", source);
+
+source->priv = GVIR_CONFIG_DOMAIN_CHARDEV_SOURCE_UNIX_GET_PRIVATE(source);
+}
+
+
+GVirConfigDomainChardevSourceUnix 
*gvir_config_domain_chardev_source_unix_new(void)
+{
+GVirConfigObject *object;
+
+/* the name of the root node is just a placeholder, it will be
+ * overwritten when the GVirConfigDomainChardevSourceUnix is attached to a
+ * GVirConfigDomainChardev
+ */
+object = 
gvir_config_object_new(GVIR_CONFIG_TYPE_DOMAIN_CHARDEV_SOURCE_UNIX, "dummy", 
NULL);
+gvir_config_object_set_attribute(object, "type", "unix", NULL);
+return GVIR_CONFIG_DOMAIN_CHARDEV_SOURCE_UNIX(object);
+}
+
+
+GVirConfigDomainChardevSourceUnix 
*gvir_config_domain_chardev_source_unix_new_from_xml(const gchar *xml,
+   
GError **error)
+{
+GVirConfigObject *object;
+
+/* the name of the roo

Re: [libvirt] shall libvirtd validate guest's name ?

[Ján Tomko]

On Thu, Jul 09, 2015 at 05:19:29PM +0800, zhang bo wrote:
> linux-ZyvZnF:~ # virsh list --all
>  IdName   State
> 
>  - redhat7;reboot shut off
>  - oscar-vm-5 shut off
> 
> 
> As shown above, 
> 1 we use command "virsh define a.xml" to define a guest with a name 
> containing ';', that's 'redhat7;reboot'
> 2 then we start the guest: "virsh start redhat7;reboot"
> 3 shell consider the command as
>   a) run "virsh start redhat7", failed
>   b) run "reboot", to reboot the host
>   And *the host get rebooted*.
> 
> shall libvirtd do the guest-name-validation work? Or other suggustions?

Semicolon is a strange but valid character to use in a domain's name.
It's the user's responsibility to escape any strange characters in the
shell prompt.

We don't allow '/' in domain names because we use them in filenames.
Explicitly rejecting it would be nicer than letting the file creation
fail, but rejecting any new characters would unnecessarily break
existing domains.

Jan


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

Re: [libvirt] shall libvirtd validate guest's name ?

[Andrea Bolognani]

On Thu, 2015-07-09 at 17:19 +0800, zhang bo wrote:
> linux-ZyvZnF:~ # virsh list --all
>  IdName   State
> 
>  - redhat7;reboot shut off
>  - oscar-vm-5 shut off
> 
> 
> As shown above, 
> 1 we use command "virsh define a.xml" to define a guest with a name 
> containing ';', that's 'redhat7;reboot'
> 2 then we start the guest: "virsh start redhat7;reboot"
> 3 shell consider the command as
>   a) run "virsh start redhat7", failed
>   b) run "reboot", to reboot the host
>   And *the host get rebooted*.
> 
> shall libvirtd do the guest-name-validation work? Or other 
> suggustions?

Proper usage of string escaping is the user's responsibility.
Unfortunately a lot of shell scripts get this wrong, leading
to more or less catastrophic results.

The same reasoning, by the way, applies to file names, and eg.
the Nautilus file manager happily allows me to use foo;reboot
as name when creating or renaming files...

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [libvirt-glib PATCHv5 7/7] gobject: Add wrapper for virNetworkGetDHCPLeases

[Zeeshan Ali (Khattak)]

On Thu, Jul 9, 2015 at 8:05 AM, Christophe Fergeau  wrote:
> On Wed, Jul 08, 2015 at 07:00:36PM +0100, Zeeshan Ali (Khattak) wrote:
>> On Wed, Jul 8, 2015 at 3:42 PM, Christophe Fergeau  
>> wrote:
>> > but you haven't brought
>> > anything forward to support that "ugly hack" statement in this specific
>> > case,
>>
>> #ifdef based solution is going to be ugly, surely you know that. I
>> never made any claims about the magnitude of ugliness, I always want
>> to avoid ugly hacks whenever possible.
>
> If it just requires 2 or 3 #ifdef, adding them and forgetting they
> existed would have been faster than this thread ;)

I agree but I want to clarify how we intend to proceed about this in
future. I'm fine with 2 or 3 but with this kind of extremely strict
definition of "too new libvirt dep", we'd need to keep on doing this
and before you know it, we'll have lots of these ugly hacks.

>> > nor any hard data regarding which distros could be impacted by a
>> > req bump. I'll stop this discussion until you bring some concrete
>> > datapoints to the table.
>>
>> Fair enough! The main point of this discussion was not to convince you
>> but rather to get a third opinion.
>
> Honestly, this is a very weird attitude, rather than trying to
> come with hard facts, you prefer having some kind of poll and make an
> arbitary uninformed decision.

I regret having to resort to some sort of poll as well but this is not
a purely objective discussion. I did present arguments but you
maintain that I did not. So let's leave at that, shall we?

I'll just proceed as Dan tell me to. It's his project in the end.

-- 
Regards,

Zeeshan Ali (Khattak)

Befriend GNOME: http://www.gnome.org/friends/

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

[libvirt] [PATCH] storage: Revert volume obj list updating after volume creation (4749d82a)

[Erik Skultety]

This patch reverts commit 4749d82a which tried to tweak the logic in
volume creation. We did realloc and update our object list before we executed
volume building within a specific storage backend. If that failed, we
had to update (again) our object list to the original state as it was before the
build and delete the volume from the pool (even though it didn't exist - this
truly depends on the backend).
I misunderstood the base idea to be able to poll the status of the volume
creation using vol-info. After commit 4749d82a this wasn't possible
anymore, although no BZ has been reported yet.

Commit 4749d82a also claimed to fix
https://bugzilla.redhat.com/show_bug.cgi?id=1223177, but commit c8be606b of the
same series as 4749d82ad (which was more of a refactor than a fix)
fixes the same issue so the revert should be pretty straightforward.
Further more, BZ https://bugzilla.redhat.com/show_bug.cgi?id=1241454 can be
fixed with this revert.
---
 src/storage/storage_driver.c | 28 ++--
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index d3cdbc5..b67a5d8 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1803,6 +1803,9 @@ storageVolCreateXML(virStoragePoolPtr obj,
 goto cleanup;
 }
 
+if (VIR_REALLOC_N(pool->volumes.objs,
+  pool->volumes.count+1) < 0)
+goto cleanup;
 
 if (!backend->createVol) {
 virReportError(VIR_ERR_NO_SUPPORT,
@@ -1817,6 +1820,14 @@ storageVolCreateXML(virStoragePoolPtr obj,
 if (backend->createVol(obj->conn, pool, voldef) < 0)
 goto cleanup;
 
+pool->volumes.objs[pool->volumes.count++] = voldef;
+volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name,
+  voldef->key, NULL, NULL);
+if (!volobj) {
+pool->volumes.count--;
+goto cleanup;
+}
+
 if (VIR_ALLOC(buildvoldef) < 0) {
 voldef = NULL;
 goto cleanup;
@@ -1845,18 +1856,15 @@ storageVolCreateXML(virStoragePoolPtr obj,
 voldef->building = false;
 pool->asyncjobs--;
 
-if (buildret < 0)
+if (buildret < 0) {
+VIR_FREE(buildvoldef);
+storageVolDeleteInternal(volobj, backend, pool, voldef,
+ 0, false);
+voldef = NULL;
 goto cleanup;
-}
-
-if (VIR_REALLOC_N(pool->volumes.objs,
-  pool->volumes.count+1) < 0)
-goto cleanup;
+}
 
-pool->volumes.objs[pool->volumes.count++] = voldef;
-if (!(volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name,
-voldef->key, NULL, NULL)))
-goto cleanup;
+}
 
 if (backend->refreshVol &&
 backend->refreshVol(obj->conn, pool, voldef) < 0)
-- 
1.9.3

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

Re: [libvirt] [libvirt-glib PATCHv5 7/7] gobject: Add wrapper for virNetworkGetDHCPLeases

[Christophe Fergeau]

On Thu, Jul 09, 2015 at 12:42:23PM +0100, Zeeshan Ali (Khattak) wrote:
> > Honestly, this is a very weird attitude, rather than trying to
> > come with hard facts, you prefer having some kind of poll and make an
> > arbitary uninformed decision.
> 
> I regret having to resort to some sort of poll as well but this is not
> a purely objective discussion. I did present arguments but you
> maintain that I did not. So let's leave at that, shall we?

Unless I missed important things, your arguments boiled down to "one
year is old enough, we should not care about distros". I've asked what
the impact would be on distros, no answer so far. I've asked what the
impact would be in terms of #ifdef ugliness, no answer so far. Quite
hard to make a decision with so few details ;)

> I'll just proceed as Dan tell me to. It's his project in the end.

danpb is away for a while as I understand it, and my feeling is that we
could resolve this on our own if you were at least trying to give more
details about the various alternatives.

Christophe


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

Re: [libvirt] [PATCH] storage: Revert volume obj list updating after volume creation (4749d82a)

[Ján Tomko]

On Thu, Jul 09, 2015 at 01:48:16PM +0200, Erik Skultety wrote:
> This patch reverts commit 4749d82a which tried to tweak the logic in
> volume creation. We did realloc and update our object list before we executed
> volume building within a specific storage backend. If that failed, we
> had to update (again) our object list to the original state as it was before 
> the
> build and delete the volume from the pool (even though it didn't exist - this
> truly depends on the backend).
> I misunderstood the base idea to be able to poll the status of the volume
> creation using vol-info. After commit 4749d82a this wasn't possible
> anymore, although no BZ has been reported yet.
> 
> Commit 4749d82a also claimed to fix
> https://bugzilla.redhat.com/show_bug.cgi?id=1223177, but commit c8be606b of 
> the
> same series as 4749d82ad (which was more of a refactor than a fix)
> fixes the same issue so the revert should be pretty straightforward.
> Further more, BZ https://bugzilla.redhat.com/show_bug.cgi?id=1241454 can be
> fixed with this revert.
> ---
>  src/storage/storage_driver.c | 28 ++--
>  1 file changed, 18 insertions(+), 10 deletions(-)

ACK

Jan


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

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

[Peter Krempa]

On Wed, Jul 08, 2015 at 19:36:02 +0200, Jiri Denemark wrote:
> When we save status XML at the point during migration where we have
> already started the domain on destination, we can't really go back and
> abort migration. Thus the only thing we can do is to log a warning and
> report success.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_migration.c | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)

ACK,

Peter


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

Re: [libvirt] [PATCH] storage: Revert volume obj list updating after volume creation (4749d82a)

[Erik Skultety]

On 09/07/15 14:13, Ján Tomko wrote:
> On Thu, Jul 09, 2015 at 01:48:16PM +0200, Erik Skultety wrote:
>> This patch reverts commit 4749d82a which tried to tweak the logic in
>> volume creation. We did realloc and update our object list before we executed
>> volume building within a specific storage backend. If that failed, we
>> had to update (again) our object list to the original state as it was before 
>> the
>> build and delete the volume from the pool (even though it didn't exist - this
>> truly depends on the backend).
>> I misunderstood the base idea to be able to poll the status of the volume
>> creation using vol-info. After commit 4749d82a this wasn't possible
>> anymore, although no BZ has been reported yet.
>>
>> Commit 4749d82a also claimed to fix
>> https://bugzilla.redhat.com/show_bug.cgi?id=1223177, but commit c8be606b of 
>> the
>> same series as 4749d82ad (which was more of a refactor than a fix)
>> fixes the same issue so the revert should be pretty straightforward.
>> Further more, BZ https://bugzilla.redhat.com/show_bug.cgi?id=1241454 can be
>> fixed with this revert.
>> ---
>>  src/storage/storage_driver.c | 28 ++--
>>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> ACK
> 
> Jan
> 
Thanks, now pushed.
Erik

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

Re: [libvirt] [PATCH 1/7] qemu: Split qemuMigrationFinish

[Peter Krempa]

On Wed, Jul 08, 2015 at 19:36:00 +0200, Jiri Denemark wrote:
> Separate code which makes incoming domain persistent into
> qemuMigrationPersist.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_migration.c | 75 
> ++-
>  1 file changed, 48 insertions(+), 27 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 97901c6..6b06e79 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5526,6 +5526,51 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr 
> def)
>  }
>  
>  
> +static int
> +qemuMigrationPersist(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + qemuMigrationCookiePtr mig)
> +{
> +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +virCapsPtr caps = NULL;
> +virDomainDefPtr vmdef;
> +virObjectEventPtr event;
> +bool newVM;
> +int ret = -1;
> +
> +if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> +goto cleanup;
> +
> +newVM = !vm->persistent;
> +vm->persistent = 1;
> +
> +if (mig->persistent)
> +vm->newDef = mig->persistent;

This could be done unconditionally since vm->newDef will apperently be
unset.

> +
> +vmdef = virDomainObjGetPersistentDef(caps, driver->xmlopt, vm);
> +if (!vmdef)
> +goto cleanup;
> +
> +if (virDomainSaveConfig(cfg->configDir, vmdef) < 0)
> +goto cleanup;
> +
> +event = virDomainEventLifecycleNewFromObj(vm,
> + VIR_DOMAIN_EVENT_DEFINED,
> + newVM ?
> + VIR_DOMAIN_EVENT_DEFINED_ADDED :
> + VIR_DOMAIN_EVENT_DEFINED_UPDATED);
> +if (event)
> +qemuDomainEventQueue(driver, event);
> +
> +ret = 0;
> +
> + cleanup:
> +virObjectUnref(caps);
> +virObjectUnref(cfg);
> +return ret;
> +}
> +
> +
>  virDomainPtr
>  qemuMigrationFinish(virQEMUDriverPtr driver,
>  virConnectPtr dconn,

ACK as-is.

Peter


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

Re: [libvirt] [PATCH 2/7] qemu: Simplify qemuMigrationFinish

[Peter Krempa]

On Wed, Jul 08, 2015 at 19:36:01 +0200, Jiri Denemark wrote:
> Offline migration migration is quite special because we don't really
> need to do anything but make the domain persistent. Let's do it
> separately from normal migration to avoid cluttering the code with
> !(flags & VIR_MIGRATE_OFFLINE).
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_migration.c | 72 
> +++
>  1 file changed, 36 insertions(+), 36 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 6b06e79..6a3e2e6 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5625,8 +5625,14 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  /* Did the migration go as planned?  If yes, return the domain
>   * object, but if no, clean up the empty qemu process.
>   */
> -if (retcode == 0) {
> -if (!virDomainObjIsActive(vm) && !(flags & VIR_MIGRATE_OFFLINE)) {
> +if (flags & VIR_MIGRATE_OFFLINE) {
> +if (retcode != 0 ||
> +qemuMigrationPersist(driver, vm, mig) < 0)
> +goto endjob;

This isn't entirely equivalend to the previous impl, since if
qemuMigrationPersist fails at a later stage the vm->persistent flag does
get set. The code below cleared it in some cases, but in this case it
would not get cleared.

Either qemuMigrationPersist should get updated so that the flag is set
only when everything went well, or you should clear if afterwards.

Later on in the endjob section there is a check if the object is active
so you'd get a persistent VM object with no definition in some cases.

> +
> +dom = virGetDomain(dconn, vm->def->name, vm->def->uuid);
> +} else if (retcode == 0) {
> +if (!virDomainObjIsActive(vm)) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("guest unexpectedly quit"));
>  qemuMigrationErrorReport(driver, vm->def->name);

ACK if you fix the offline migration error handling.

Peter


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

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

[Peter Krempa]

On Wed, Jul 08, 2015 at 19:36:03 +0200, Jiri Denemark wrote:
> Whenever something fails during incoming migration in Finish phase
> before we started guest CPUs, we need to kill the domain in addition to
> reporting the failure.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_migration.c | 32 
>  1 file changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 9439954..576b32d 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5589,6 +5589,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  unsigned short port;
> +bool keep = false;
>  
>  VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, "
>"cookieout=%p, cookieoutlen=%p, flags=%lx, retcode=%d",
> @@ -5647,15 +5648,9 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  }
>  }
>  
> -if (qemuMigrationVPAssociatePortProfiles(vm->def) < 0) {
> -qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> -VIR_QEMU_PROCESS_STOP_MIGRATED);
> -virDomainAuditStop(vm, "failed");
> -event = virDomainEventLifecycleNewFromObj(vm,
> - VIR_DOMAIN_EVENT_STOPPED,
> - 
> VIR_DOMAIN_EVENT_STOPPED_FAILED);
> +if (qemuMigrationVPAssociatePortProfiles(vm->def) < 0)
>  goto endjob;
> -}
> +
>  if (mig->network && qemuDomainMigrateOPDRelocate(driver, vm, mig) < 
> 0)
>  VIR_WARN("unable to provide network data for relocation");
>  
> @@ -5681,11 +5676,10 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>   * to restart during confirm() step, so we kill it off now.
>   */
>  if (v3proto) {
> -qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> -VIR_QEMU_PROCESS_STOP_MIGRATED);
> -virDomainAuditStop(vm, "failed");
>  if (newVM)
>  vm->persistent = 0;

If you choose to fix qemuMigrationPersist so that it sets the persistent
flag only on succes to fix the previous patch then this will create a
conflict since the above statement shouldn't be necessary.

> +} else {
> +keep = true;
>  }
>  goto endjob;
>  }

ACK,

Peter


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

Re: [libvirt] [PATCH 5/7] qemu: Don't report false errors in migration protocol v2

[Peter Krempa]

On Wed, Jul 08, 2015 at 19:36:04 +0200, Jiri Denemark wrote:
> Finish is the final state in v2 of our migration protocol. If something
> fails, we have no option to abort the migration and resume the original
> domain. Non fatal errors (such as failure to start guest CPUs or make
> the domain persistent) has to be treated as success. Keeping the domain
> running while reporting the failure was just asking for troubles.

s/es/e/

> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_migration.c | 20 +++-
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 

ACK,

Peter


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

Re: [libvirt] [libvirt-glib PATCHv5 7/7] gobject: Add wrapper for virNetworkGetDHCPLeases

[Zeeshan Ali (Khattak)]

On Thu, Jul 9, 2015 at 12:56 PM, Christophe Fergeau  wrote:
> On Thu, Jul 09, 2015 at 12:42:23PM +0100, Zeeshan Ali (Khattak) wrote:
>> > Honestly, this is a very weird attitude, rather than trying to
>> > come with hard facts, you prefer having some kind of poll and make an
>> > arbitary uninformed decision.
>>
>> I regret having to resort to some sort of poll as well but this is not
>> a purely objective discussion. I did present arguments but you
>> maintain that I did not. So let's leave at that, shall we?
>
> Unless I missed important things, your arguments boiled down to "one
> year is old enough, we should not care about distros".

It's very hard to discuss if you over-exagerate my opinions to make
them sound bad. I'm did not say we should not care about distros but
rather that 1 year is plenty of care.

> I've asked what
> the impact would be on distros, no answer so far.

The answer was that 1 year of grace time is enough and we shouldn't
need to be bound by release cycles and packaging of individual
distros. Also I pointed out the fact that it does not (in practical
terms) make any sense to only upgrade to latest libvirt-glib but
wanting to stay with an year old libvirt.

> I've asked what the
> impact would be in terms of #ifdef ugliness, no answer so far.

The answer was that magnitude of ugliness is irrelevant. Surely you
know that this is not anything objective. To me a single #ifdef is
ugly enough but seems it's not for you so how do you expect either of
us to convince each other with arguments? If you really need something
more concrete, I see at least 12 #ifdefs needed. Pretty ugly for my
taste at least.

> Quite
> hard to make a decision with so few details ;)

All needed "details" are avaiable and it's only a matter of taste and
amount of care we want to give to distros. Both are subjective
matters.

>> I'll just proceed as Dan tell me to. It's his project in the end.
>
> danpb is away for a while as I understand it, and my feeling is that we
> could resolve this on our own if you were at least trying to give more
> details about the various alternatives.

I don't know many alternatives. It's either bumping the dep or adding
ugly #ifdef based solution.

-- 
Regards,

Zeeshan Ali (Khattak)

Befriend GNOME: http://www.gnome.org/friends/

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

Re: [libvirt] [PATCH 1/4] Introduce virHashLockable

[Peter Krempa]

On Wed, Jul 08, 2015 at 15:22:49 +0200, Jiri Denemark wrote:
> This is a self-locking wrapper around virHashTable. Only a limited set
> of APIs are implemented now (the ones which are used in the following
> patch) as more can be added on demand.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/libvirt_private.syms |  3 ++
>  src/util/virhash.c   | 81 
> 
>  src/util/virhash.h   | 10 ++
>  3 files changed, 94 insertions(+)

ACK although I dislike the "Lockable" part since you are not "able" to
lock that hash, but that hash is self-locking. But I don't have a better
name suggestion.

Peter


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

Re: [libvirt] [PATCH v2 4/4] qemu_hotplug: try harder to eject media

[John Ferlan]

On 07/03/2015 09:51 AM, Pavel Hrdina wrote:
> On Wed, Jul 01, 2015 at 05:39:49PM -0400, John Ferlan wrote:
>>
>>
>> On 06/29/2015 11:17 AM, Pavel Hrdina wrote:
>>> Some guests lock the tray and QEMU eject command will simply fail to
>>> eject the media.  But the guest OS can handle this attempt to eject the
>>> media and can unlock the tray and open it. In this case, we should try
>>> again to actually eject the media.
>>>
>>> If the first attempt fails to detect a tray_open we will fail with
>>> error, from monitor.  If we receive that event, we know, that the guest
>>> properly reacted to the eject request, unlocked the tray and opened it.
>>> In this case, we need to run the command again to actually eject the
>>> media from the device.  The reason to call it again is, that QEMU don't

s/don't/doesn't

>>> wait for the guest to react and report an error, that the tray is
>>> locked.
>>>
>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1147471
>>>
>>> Signed-off-by: Pavel Hrdina 
>>> ---
>>>  src/qemu/qemu_hotplug.c | 73 
>>> +++--
>>>  src/qemu/qemu_process.c |  2 ++
>>>  2 files changed, 36 insertions(+), 39 deletions(-)
>>>

I suppose if I had gone back to the commit message and reread it in
light of what I was reading in the qemu-devel message linked in the bz
and what I saw in the code, then perhaps it would all make sense. Of
course the text you add below now makes it clear what the order of
processing is and why the while loop is used.

Personally I think this is a case where that commit message could be
placed into the code to make it "clearer" what is being done and why. I
know there are those against that because it's in the commit message or
perhaps some linked bug, but if I'm reading code and wondering what it
does, I'm not necessarily going back to the commit/bug that added the
code (OK, at least not at first).

Whether you add a comment into the code to indicate why this while loop
is important and how the processing works due to the event and locked
media...

ACK series,

John
>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>> index 0628964..17595b7 100644
>>> --- a/src/qemu/qemu_hotplug.c
>>> +++ b/src/qemu/qemu_hotplug.c
>>> @@ -59,7 +59,7 @@
>>>  
>>>  VIR_LOG_INIT("qemu.qemu_hotplug");
>>>  
>>> -#define CHANGE_MEDIA_RETRIES 10
>>> +#define CHANGE_MEDIA_TIMEOUT 5000
>>>  
>>>  /* Wait up to 5 seconds for device removal to finish. */
>>>  unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
>>> @@ -166,12 +166,13 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr 
>>> driver,
>>> virStorageSourcePtr newsrc,
>>> bool force)
>>>  {
>>> -int ret = -1;
>>> +int ret = -1, rc;
>>>  char *driveAlias = NULL;
>>>  qemuDomainObjPrivatePtr priv = vm->privateData;
>>> -int retries = CHANGE_MEDIA_RETRIES;
>>>  const char *format = NULL;
>>>  char *sourcestr = NULL;
>>> +bool ejectRetry = false;
>>> +unsigned long long now;
>>>  
>>>  if (!disk->info.alias) {
>>>  virReportError(VIR_ERR_INTERNAL_ERROR,
>>> @@ -193,36 +194,31 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr 
>>> driver,
>>>  if (!(driveAlias = qemuDeviceDriveHostAlias(disk, priv->qemuCaps)))
>>>  goto error;
>>>  
>>> -qemuDomainObjEnterMonitor(driver, vm);
>>> -ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
>>> -if (qemuDomainObjExitMonitor(driver, vm) < 0) {
>>> -ret = -1;
>>> -goto cleanup;
>>> -}
>>> -
>>> -if (ret < 0)
>>> -goto error;
>>> +do {
>>> +qemuDomainObjEnterMonitor(driver, vm);
>>> +rc = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
>>> +if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>> +goto cleanup;
>>>  
>>> -virObjectRef(vm);
>>> -/* we don't want to report errors from media tray_open polling */
>>> -while (retries) {
>>> -if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)
>>> -break;
>>> +if (rc == -2) {
>>> +/* we've already tried, error out */
>>> +if (ejectRetry)
>>> +goto error;
>>> +VIR_DEBUG("tray is locked, wait for the guest to unlock "
>>> +  "the tray and try to eject it again");
>>> +ejectRetry = true;
>>> +} else if (rc < 0) {
>>> +goto error;
>>> +}
>>>  
>>> -retries--;
>>> -virObjectUnlock(vm);
>>> -VIR_DEBUG("Waiting 500ms for tray to open. Retries left %d", 
>>> retries);
>>> -usleep(500 * 1000); /* sleep 500ms */
>>> -virObjectLock(vm);
>>> -}
>>> -virObjectUnref(vm);
>>> +if (virTimeMillisNow(&now) < 0)
>>> +goto error;
>>>  
>>> -if (retries <= 0) {
>>> -virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>>> -   _("Unable to eject media"));

Re: [libvirt] [libvirt-glib PATCHv5 7/7] gobject: Add wrapper for virNetworkGetDHCPLeases

[Christophe Fergeau]

On Thu, Jul 09, 2015 at 02:41:45PM +0100, Zeeshan Ali (Khattak) wrote:
> The answer was that magnitude of ugliness is irrelevant. Surely you
> know that this is not anything objective. To me a single #ifdef is
> ugly enough but seems it's not for you so how do you expect either of
> us to convince each other with arguments? If you really need something
> more concrete, I see at least 12 #ifdefs needed. Pretty ugly for my
> taste at least.

I'm just trying to have a discussion about something concrete. If just
one #ifdef was needed, I guess you'll bear with me if I try to convince
you to go this way, with 12 #ifdef, you've got a more understandable
standing in opposing this.

> 
> > Quite
> > hard to make a decision with so few details ;)
> 
> All needed "details" are avaiable and it's only a matter of taste and
> amount of care we want to give to distros. Both are subjective
> matters.

Same deal as with the amount of #ifdef needed, if only EL6 has a too old
libvirt, then you are in a much better position to convince me than if
Debian stable and latest Ubuntu (making things up) have a too old
libvirt.
Ie let's know the exact tradeoffs we are discussing here from a
technical point of view, maybe we won't have to agree to disagree and
look for a tie breaker ;)

Christophe


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

[libvirt] [PATCH 1/2] conf: Add getter for network routes

[Martin Kletzander]

Add virNetworkDefGetRouteByIndex() similarly to
virNetworkDefGetIpByIndex(), but for routes.

Signed-off-by: Martin Kletzander 
---
 src/conf/network_conf.c  | 26 ++
 src/conf/network_conf.h  |  3 +++
 src/libvirt_private.syms |  1 +
 3 files changed, 30 insertions(+)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 31d4463a475b..72006e9822d7 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -803,6 +803,32 @@ virNetworkDefGetIpByIndex(const virNetworkDef *def,
 return NULL;
 }

+/* return routes[index], or NULL if there aren't enough routes */
+virNetworkRouteDefPtr
+virNetworkDefGetRouteByIndex(const virNetworkDef *def,
+ int family, size_t n)
+{
+size_t i;
+
+if (!def->routes || n >= def->nroutes)
+return NULL;
+
+if (family == AF_UNSPEC)
+return def->routes[n];
+
+/* find the nth route of type "family" */
+for (i = 0; i < def->nroutes; i++) {
+virSocketAddrPtr addr = virNetworkRouteDefGetAddress(def->routes[i]);
+if (VIR_SOCKET_ADDR_IS_FAMILY(addr, family)
+&& (n-- <= 0)) {
+return def->routes[i];
+}
+}
+
+/* failed to find enough of the right family */
+return NULL;
+}
+
 /* return number of 1 bits in netmask for the network's ipAddress,
  * or -1 on error
  */
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 9411a02e32cb..1cd5100c1278 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -360,6 +360,9 @@ virPortGroupDefPtr virPortGroupFindByName(virNetworkDefPtr 
net,
 virNetworkIpDefPtr
 virNetworkDefGetIpByIndex(const virNetworkDef *def,
   int family, size_t n);
+virNetworkRouteDefPtr
+virNetworkDefGetRouteByIndex(const virNetworkDef *def,
+ int family, size_t n);
 int virNetworkIpDefPrefix(const virNetworkIpDef *def);
 int virNetworkIpDefNetmask(const virNetworkIpDef *def,
virSocketAddrPtr netmask);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1566d11e4156..9d117acbd2bd 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -598,6 +598,7 @@ virNetworkDefFormat;
 virNetworkDefFormatBuf;
 virNetworkDefFree;
 virNetworkDefGetIpByIndex;
+virNetworkDefGetRouteByIndex;
 virNetworkDefParseFile;
 virNetworkDefParseNode;
 virNetworkDefParseString;
-- 
2.4.5

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

[libvirt] [PATCH 2/2] network: Add another collision check into networkCheckRouteCollision

[Martin Kletzander]

The comment above that function says: "This function can be a lot more
exhaustive, ...", so let's be.

Check for collisions between routes in the system and static routes
being added explicitly from the  element of the network XML.

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

Signed-off-by: Martin Kletzander 
---
Laine suggested moving networkCheckRouteCollision() into
networkAddRouteToBridge() and I haven't done that simply because we
can check it where it is now.  It would also mean parsing the file,
which we don't want to parse anyway, multiple times or storing the
results and I don't think it's worth neither the time nor space
complexity.

 src/network/bridge_driver_linux.c | 29 +
 1 file changed, 29 insertions(+)

diff --git a/src/network/bridge_driver_linux.c 
b/src/network/bridge_driver_linux.c
index e394dafb2216..66e5902a7b6f 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -69,6 +69,7 @@ int networkCheckRouteCollision(virNetworkDefPtr def)
 char iface[17], dest[128], mask[128];
 unsigned int addr_val, mask_val;
 virNetworkIpDefPtr ipdef;
+virNetworkRouteDefPtr routedef;
 int num;
 size_t i;

@@ -123,6 +124,34 @@ int networkCheckRouteCollision(virNetworkDefPtr def)
 goto out;
 }
 }
+
+for (i = 0;
+ (routedef = virNetworkDefGetRouteByIndex(def, AF_INET, i));
+ i++) {
+
+virSocketAddr r_mask, r_addr;
+virSocketAddrPtr tmp_addr = virNetworkRouteDefGetAddress(routedef);
+int r_prefix = virNetworkRouteDefGetPrefix(routedef);
+
+if (!tmp_addr ||
+virSocketAddrMaskByPrefix(tmp_addr, r_prefix, &r_addr) < 0 ||
+virSocketAddrPrefixToNetmask(r_prefix, &r_mask, AF_INET) < 0)
+continue;
+
+if ((r_addr.data.inet4.sin_addr.s_addr == addr_val) &&
+(r_mask.data.inet4.sin_addr.s_addr == mask_val)) {
+char *addr_str = virSocketAddrFormat(&r_addr);
+if (!addr_str)
+virResetLastError();
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Route address '%s' collides with one "
+ "that's in the system already"),
+   NULLSTR(addr_str));
+VIR_FREE(addr_str);
+ret = -1;
+goto out;
+}
+}
 }

  out:
--
2.4.5

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

[libvirt] [PATCH 0/2] Check static route collisions

[Martin Kletzander]

Martin Kletzander (2):
  conf: Add getter for network routes
  network: Add another collision check into networkCheckRouteCollision

 src/conf/network_conf.c   | 26 ++
 src/conf/network_conf.h   |  3 +++
 src/libvirt_private.syms  |  1 +
 src/network/bridge_driver_linux.c | 29 +
 4 files changed, 59 insertions(+)

--
2.4.5

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

Re: [libvirt] [PATCH 2/4] qemu: Remember incoming migration errors

[Peter Krempa]

On Wed, Jul 08, 2015 at 15:22:50 +0200, Jiri Denemark wrote:
> If QEMU fails during incoming migration, the domain disappears including
> a possibly useful error message read from QEMU log file. Let's remember
> the error in virQEMUDriver so that Finish can report more than just "no
> such domain".
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_conf.h  |  3 +++
>  src/qemu/qemu_driver.c| 31 +++---
>  src/qemu/qemu_migration.c | 55 
> ++-
>  src/qemu/qemu_migration.h |  7 ++
>  src/qemu/qemu_monitor.c   | 19 
>  src/qemu/qemu_monitor.h   |  2 ++
>  src/qemu/qemu_process.c   |  4 
>  7 files changed, 112 insertions(+), 9 deletions(-)
> 

...

> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index a57a177..82069a1 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c

...

> @@ -6051,3 +6054,53 @@ qemuMigrationJobFinish(virQEMUDriverPtr driver, 
> virDomainObjPtr vm)
>  {
>  qemuDomainObjEndAsyncJob(driver, vm);
>  }
> +
> +
> +static void
> +qemuMigrationErrorFree(void *data,
> +   const void *name ATTRIBUTE_UNUSED)
> +{
> +virErrorPtr err = data;
> +virFreeError(err);
> +}
> +
> +int
> +qemuMigrationErrorInit(virQEMUDriverPtr driver)
> +{
> +driver->migrationErrors = virHashLockableNew(64, qemuMigrationErrorFree);
> +if (driver->migrationErrors)
> +return 0;
> +else
> +return -1;
> +}
> +

This function consumes @err. A comment noting that would be helpful.

> +void
> +qemuMigrationErrorSave(virQEMUDriverPtr driver,
> +   const char *name,
> +   virErrorPtr err)
> +{
> +if (!err)
> +return;
> +
> +VIR_DEBUG("Saving incoming migration error for domain %s: %s",
> +  name, err->message);
> +if (virHashLockableUpdate(driver->migrationErrors, name, err) < 0) {
> +VIR_WARN("Failed to save migration error for domain '%s'", name);
> +virFreeError(err);
> +}
> +}
> +
> +void
> +qemuMigrationErrorReport(virQEMUDriverPtr driver,
> + const char *name)
> +{
> +virErrorPtr err;
> +
> +if (!(err = virHashLockableSteal(driver->migrationErrors, name)))
> +return;
> +
> +VIR_DEBUG("Restoring saved incoming migration error for domain %s: %s",
> +  name, err->message);
> +virSetError(err);
> +virFreeError(err);
> +}

...

> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 896d9fd..9db05c5 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1057,6 +1057,25 @@ qemuMonitorSend(qemuMonitorPtr mon,
>  }
>  

Telling that the user is responsible for freeing the value would be
helpful here.
>  
> +virErrorPtr
> +qemuMonitorLastError(qemuMonitorPtr mon)
> +{
> +virErrorPtr old;
> +virErrorPtr err;
> +
> +if (mon->lastError.code == VIR_ERR_OK)
> +return NULL;
> +
> +old = virSaveLastError();
> +virSetError(&mon->lastError);
> +err = virSaveLastError();
> +virSetError(old);
> +virFreeError(old);

Ummm, how about exporting virCopyError rather than using this rather
opaque and ugly way to copy the error?

> +
> +return err;
> +}
> +
> +
>  virJSONValuePtr
>  qemuMonitorGetOptions(qemuMonitorPtr mon)
>  {

Peter


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

Re: [libvirt] [PATCHv4 2/2] Fix cloning of raw, sparse volumes

[John Ferlan]

On 07/03/2015 09:54 AM, Ján Tomko wrote:
> From: Prerna Saxena 
> 
> When virsh vol-clone is attempted on a raw file where capacity > allocation,
> the resulting cloned volume has a size that matches the virtual-size of
> the parent; in place of matching its actual, disk size.
> This patch fixes the cloned disk to have same _allocated_size_ as
> the parent file from which it was cloned.
> 
> Ref: http://www.redhat.com/archives/libvir-list/2015-May/msg00050.html
> 
> Also fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1130739
> 
> Signed-off-by: Prerna Saxena 
> Signed-off-by: Ján Tomko 
> ---
>  src/storage/storage_backend.c | 23 ++-
>  src/storage/storage_driver.c  |  5 -
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index c71545c..bab81e3 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -342,7 +342,7 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn 
> ATTRIBUTE_UNUSED,
>  goto cleanup;
>  }
>  
> -remain = vol->target.allocation;
> +remain = vol->target.capacity;
>  
>  if (inputvol) {
>  int res = virStorageBackendCopyToFD(vol, inputvol,
> @@ -401,6 +401,12 @@ createRawFile(int fd, virStorageVolDefPtr vol,
>  int ret = 0;
>  unsigned long long pos = 0;
>  
> +/* If the new allocation is lower than the capacity of the orignal file,

s/orignal/original

ACK series


John

> + * the cloned volume will be sparse */
> +if (inputvol &&
> +vol->target.allocation < inputvol->target.capacity)
> +need_alloc = false;
> +
>  /* Seek to the final size, so the capacity is available upfront
>   * for progress reporting */
>  if (ftruncate(fd, vol->target.capacity) < 0) {
> @@ -420,7 +426,7 @@ createRawFile(int fd, virStorageVolDefPtr vol,
>   * to writing zeroes block by block in case fallocate isn't
>   * available, and since we're going to copy data from another
>   * file it doesn't make sense to write the file twice. */
> -if (vol->target.allocation) {
> +if (vol->target.allocation && need_alloc) {
>  if (fallocate(fd, 0, 0, vol->target.allocation) == 0) {
>  need_alloc = false;
>  } else if (errno != ENOSYS && errno != EOPNOTSUPP) {
> @@ -433,21 +439,20 @@ createRawFile(int fd, virStorageVolDefPtr vol,
>  }
>  #endif
>  
> -
>  if (inputvol) {
> -unsigned long long remain = vol->target.allocation;
> +unsigned long long remain = inputvol->target.capacity;
>  /* allow zero blocks to be skipped if we've requested sparse
>   * allocation (allocation < capacity) or we have already
>   * been able to allocate the required space. */
> -bool want_sparse = !need_alloc ||
> -(vol->target.allocation < inputvol->target.capacity);
> -
>  ret = virStorageBackendCopyToFD(vol, inputvol, fd, &remain,
> -want_sparse, reflink_copy);
> +!need_alloc, reflink_copy);
>  if (ret < 0)
>  goto cleanup;
>  
> -pos = vol->target.allocation - remain;
> +/* If the new allocation is greater than the original capacity,
> + * but fallocate failed, fill the rest with zeroes.
> + */
> +pos = inputvol->target.capacity - remain;
>  }
>  
>  if (need_alloc) {
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index e600514..ba27acf 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -1975,11 +1975,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
>  if (newvol->target.capacity < origvol->target.capacity)
>  newvol->target.capacity = origvol->target.capacity;
>  
> -/* Make sure allocation is at least as large as the destination cap,
> - * to make absolutely sure we copy all possible contents */
> -if (newvol->target.allocation < origvol->target.capacity)
> -newvol->target.allocation = origvol->target.capacity;
> -
>  if (!backend->buildVolFrom) {
>  virReportError(VIR_ERR_NO_SUPPORT,
> "%s", _("storage pool does not support"
> 

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

Re: [libvirt] [PATCH v4 0/2] Fix nodeinfo output on PPC64 KVM hosts

[John Ferlan]

On 07/07/2015 03:25 AM, Andrea Bolognani wrote:
> Changes from v3 to v4:
> 
>   * removed a printf() statement;
> 
>   * fixed typo in a commit message.
> 
> Shivaprasad G Bhat (2):
>   Fix nodeinfo output on PPC64 KVM hosts
>   Add testcase for PPC64 kvm host nodeinfo
> 


Never saw the v4 2/2 come through (nor do I see it in the archive);
however, I assume it's the same as the v3 patch:

http://www.redhat.com/archives/libvir-list/2015-July/msg00155.html

Given it is and what I found reviewing the following:

http://www.redhat.com/archives/libvir-list/2015-July/msg00219.html

regarding nodeinfo.c not really using the tests/nodeinfodata local path
instead the running host's sysfs (/sys/devices/system) path.

I found while testing that the proposed patch wouldn't run correctly on
my host because my /sys/devices/system/cpu/present is "0-3" and the
patch would fail on any test with cpu4+ since the tests/nodeinfodata/
present file isn't referenced (if it existed).

I created a series which adjusts the SYSFS_SYSTEM_PATH logic in
nodeinfo.c to allow for a supplied path or uses the default:

http://www.redhat.com/archives/libvir-list/2015-July/msg00278.html

Not looking for a review of the 9 patch sysfs series, but I am curious
to get a perspective on the patch I initially reviewed which modifies
virNodeParseNode to "filter out" or "exclude" cpu's that are offline
because they're defective/empty and perhaps how/if that applies to this
environment as well.

I'm also curious what happens if the 2/2 patch is run on a PPC64 host
with less than 96 cores (from .../cpu/present) since the results seem to
expect the 96 cores to be present.  It would seem the existing code
without the sysfs path redirection would fail, since the caller
linuxNodeInfoCPUPopulate would be using the host's sysfs path rather
than the tests sysfs path.


John

>  src/libvirt_private.syms   |   1 +
>  src/nodeinfo.c | 138 
> +++--
>  src/nodeinfo.h |   1 +
>  tests/Makefile.am  |   6 +
>  tests/nodeinfodata/linux-ppc64-subcores.cpuinfo|  59 +
>  tests/nodeinfodata/linux-ppc64-subcores.expected   |   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu0/online  |   1 +
>  .../linux-subcores/cpu/cpu0/physical_id|   1 +
>  .../linux-subcores/cpu/cpu0/topology/core_id   |   1 +
>  .../linux-subcores/cpu/cpu0/topology/core_siblings |   1 +
>  .../cpu/cpu0/topology/core_siblings_list   |   1 +
>  .../cpu/cpu0/topology/physical_package_id  |   1 +
>  .../cpu/cpu0/topology/thread_siblings  |   1 +
>  .../cpu/cpu0/topology/thread_siblings_list |   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu1/online  |   1 +
>  .../linux-subcores/cpu/cpu1/physical_id|   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu10/online |   1 +
>  .../linux-subcores/cpu/cpu10/physical_id   |   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu11/online |   1 +
>  .../linux-subcores/cpu/cpu11/physical_id   |   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu12/online |   1 +
>  .../linux-subcores/cpu/cpu12/physical_id   |   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu13/online |   1 +
>  .../linux-subcores/cpu/cpu13/physical_id   |   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu14/online |   1 +
>  .../linux-subcores/cpu/cpu14/physical_id   |   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu15/online |   1 +
>  .../linux-subcores/cpu/cpu15/physical_id   |   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu16/online |   1 +
>  .../linux-subcores/cpu/cpu16/physical_id   |   1 +
>  .../linux-subcores/cpu/cpu16/topology/core_id  |   1 +
>  .../cpu/cpu16/topology/core_siblings   |   1 +
>  .../cpu/cpu16/topology/core_siblings_list  |   1 +
>  .../cpu/cpu16/topology/physical_package_id |   1 +
>  .../cpu/cpu16/topology/thread_siblings |   1 +
>  .../cpu/cpu16/topology/thread_siblings_list|   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu17/online |   1 +
>  .../linux-subcores/cpu/cpu17/physical_id   |   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu18/online |   1 +
>  .../linux-subcores/cpu/cpu18/physical_id   |   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu19/online |   1 +
>  .../linux-subcores/cpu/cpu19/physical_id   |   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu2/online  |   1 +
>  .../linux-subcores/cpu/cpu2/physical_id|   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu20/online |   1 +
>  .../linux-subcores/cpu/cpu20/physical_id   |   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu21/online |   1 +
>  .../linux-subcores/cpu/cpu21/physical_id   |   1 +
>  tests/nodeinfodata/linux-subcores/cpu/cpu22/online |   1 +
>  .../linux-subcores/cpu/cpu22/physical_id   |   1 +
>  tests/node

Re: [libvirt] [PATCH 3/4] qemu: Don't report false error from MigrateFinish

[Peter Krempa]

On Wed, Jul 08, 2015 at 15:22:51 +0200, Jiri Denemark wrote:
> virDomainMigrateFinish* APIs were unfortunately designed to return the
> pointer to the domain on destination and NULL on error. This looks OK in
> normal cases but the same API is also called when we know migration
> failed and thus we expect Finish to return NULL even if it actually did
> all it was supposed to do without any error. The call is defined to
> return nonnull domain pointer over RPC, which means returning NULL will
> always result in an error being send. If this was not in fact an error,
> the API itself wouldn't set anything to the thread local virError, which
> makes the RPC layer come up with it's own "Library function returned
> error but did not set virError" error.
> 
> This is quite confusing and also hard to detect by the caller. This
> patch adds a special error code which can be used to check that Finish
> successfully aborted migration.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  include/libvirt/virterror.h | 1 +
>  src/qemu/qemu_migration.c   | 6 ++
>  src/util/virerror.c | 3 +++
>  3 files changed, 10 insertions(+)

I'm kind of not sure whether I like this solution or not. I understand
the reasons for having it but I'm not liking it. ACK but I'd prefer
another opinion on this if possible.

Peter


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

[libvirt] [PATCH] vz: fix cleanup of nets of bridged type

[Dmitry Guryanov]

We create a virtual network of special type, which
has the same name as bridge name to create bridged
network adapter in vz. So when we delete such an
adapter we have to remove corresponding virtual
network.

So let's rename prlsdkDelNet to prlsdkCleanupBridgedNet
and don't check for return value.

Signed-off-by: Dmitry Guryanov 
---
 src/vz/vz_sdk.c | 25 -
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index a312990..d1bc312 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -2986,20 +2986,15 @@ static int prlsdkAddNet(PRL_HANDLE sdkdom,
 return ret;
 }
 
-static int
-prlsdkDelNet(vzConnPtr privconn, virDomainNetDefPtr net)
+static void
+prlsdkCleanupBridgedNet(vzConnPtr privconn, virDomainNetDefPtr net)
 {
-int ret = -1;
 PRL_RESULT pret;
 PRL_HANDLE vnet = PRL_INVALID_HANDLE;
 PRL_HANDLE job = PRL_INVALID_HANDLE;
 
-if (net->type != VIR_DOMAIN_NET_TYPE_BRIDGE) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
-   _("unplugging network device of type %s is not 
supported"),
-   virDomainNetTypeToString(net->type));
-return ret;
-}
+if (net->type != VIR_DOMAIN_NET_TYPE_BRIDGE)
+return;
 
 pret = PrlVirtNet_Create(&vnet);
 prlsdkCheckRetGoto(pret, cleanup);
@@ -3011,11 +3006,8 @@ prlsdkDelNet(vzConnPtr privconn, virDomainNetDefPtr net)
 if (PRL_FAILED(pret = waitJob(job)))
 goto cleanup;
 
-ret = 0;
-
  cleanup:
 PrlHandle_Free(vnet);
-return ret;
 }
 
 int prlsdkAttachNet(virDomainObjPtr dom,
@@ -3107,8 +3099,7 @@ int prlsdkDetachNet(virDomainObjPtr dom,
 if (sdknet == PRL_INVALID_HANDLE)
 goto cleanup;
 
-if (prlsdkDelNet(privconn, net) < 0)
-goto cleanup;
+prlsdkCleanupBridgedNet(privconn, net);
 
 pret = PrlVmDev_Remove(sdknet);
 prlsdkCheckRetGoto(pret, cleanup);
@@ -3530,7 +3521,7 @@ prlsdkDoApplyConfig(virConnectPtr conn,
 
 if (olddef) {
 for (i = 0; i < olddef->nnets; i++)
-prlsdkDelNet(conn->privateData, olddef->nets[i]);
+prlsdkCleanupBridgedNet(conn->privateData, olddef->nets[i]);
 }
 
 for (i = 0; i < def->nnets; i++) {
@@ -3575,7 +3566,7 @@ prlsdkDoApplyConfig(virConnectPtr conn,
 VIR_FREE(mask);
 
 for (i = 0; i < def->nnets; i++)
-prlsdkDelNet(conn->privateData, def->nets[i]);
+prlsdkCleanupBridgedNet(conn->privateData, def->nets[i]);
 
 return -1;
 }
@@ -3722,7 +3713,7 @@ prlsdkUnregisterDomain(vzConnPtr privconn, 
virDomainObjPtr dom)
 size_t i;
 
 for (i = 0; i < dom->def->nnets; i++)
-prlsdkDelNet(privconn, dom->def->nets[i]);
+prlsdkCleanupBridgedNet(privconn, dom->def->nets[i]);
 
 job = PrlVm_Unreg(privdom->sdkdom);
 if (PRL_FAILED(waitJob(job)))
-- 
2.4.3

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

[libvirt] [PATCH] qemu: Reject updating unsupported disk information

[Martin Kletzander]

If one calls update-device with information that is not updatable,
libvirt reports success even though no data were updated.  The example
used in the bug linked below uses updating device with 
which, in my opinion, is a valid thing to request from user's
perspective.  Mainly since we properly error out if user wants to update
such data on a network device for example.

And since there are many things that might happen (update-device on disk
basically knows just how to change removable media), check for what's
changing and moreover, since the function might be usable in other
dirvers (updating only disk path is a valid possibility) let's abstract
it for any two disks.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1007228
---
 src/conf/domain_conf.c   | 111 +++
 src/conf/domain_conf.h   |   2 +
 src/libvirt_private.syms |   1 +
 src/qemu/qemu_driver.c   |   3 ++
 4 files changed, 117 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0219c3c4814d..a6950087d987 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5687,6 +5687,117 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def,
 return NULL;
 }

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

Re: [libvirt] [PATCH 4/4] qemu: Use error from Finish instead of "unexpectedly failed"

[Peter Krempa]

On Wed, Jul 08, 2015 at 15:22:52 +0200, Jiri Denemark wrote:
> When QEMU exits on destination during migration, the source reports
> either success (if the failure happened at the very end) or unhelpful
> "unexpectedly failed" error message. However, the Finish API called on
> the destination may report a real error so let's use it instead of the
> generic one.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/libvirt-domain.c  | 21 +++--
>  src/qemu/qemu_migration.c | 30 --
>  2 files changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 909c264..f18fee2 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3175,8 +3175,25 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
>  (dconn, dname, cookiein, cookieinlen, &cookieout, &cookieoutlen,
>   NULL, uri, destflags, cancelled);
>  }
> -if (cancelled && ddomain)
> -VIR_ERROR(_("finish step ignored that migration was cancelled"));
> +

See below for comments:

> +if (cancelled) {
> +if (ddomain)
> +VIR_ERROR(_("finish step ignored that migration was cancelled"));
> +
> +/* If Finish reported a useful error, use it instead of the original
> + * "migration unexpectedly failed" error.
> + */
> +if (orig_err &&
> +orig_err->domain == VIR_FROM_QEMU &&
> +orig_err->code == VIR_ERR_OPERATION_FAILED) {
> +virErrorPtr err = virGetLastError();
> +if (err->domain == VIR_FROM_QEMU &&
> +err->code != VIR_ERR_MIGRATE_FINISH_OK) {
> +virFreeError(orig_err);
> +orig_err = NULL;
> +}
> +}
> +}
>  
>  /* If ddomain is NULL, then we were unable to start
>   * the guest on the target, and must restart on the
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 3548d73..d02a0c6 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -4957,8 +4957,25 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver,
>   dconnuri, uri, destflags, cancelled);
>  qemuDomainObjExitRemote(vm);
>  }
> -if (cancelled && ddomain)
> -VIR_ERROR(_("finish step ignored that migration was cancelled"));
> +

The control flow below is weird. Here are a few facts from the code
above:

- ddomain is set via the domainMigrateFinish3(Params) and not anywhere
  else
- domainMigrateFinish3 either reports an error or returns ddomain

thus:

> +if (cancelled) {
> +if (ddomain)
> +VIR_ERROR(_("finish step ignored that migration was cancelled"));
> +

Basically all the code below is an else section to the above if
statement due to the previous fact, so ... the below code makes it kind
of opaque.

> +/* If Finish reported a useful error, use it instead of the original
> + * "migration unexpectedly failed" error.
> + */
> +if (orig_err &&
> +orig_err->domain == VIR_FROM_QEMU &&
> +orig_err->code == VIR_ERR_OPERATION_FAILED) {

The code check isn't robust enough IMO. If you want to keep it, the fact
that this happens in a way like this should be noted in a comment for
doNativeMigrate/doTunnelMigrate that set the errors.

> +virErrorPtr err = virGetLastError();

And additionally there is no error reported that this could possibly
overwrite in case where ddomain is not NULL except for the one reported
in virTypedParamsReplaceString but I doubt that will be a better one.
(If that is the intention a comment would really be helpful.

> +if (err->domain == VIR_FROM_QEMU &&
> +err->code != VIR_ERR_MIGRATE_FINISH_OK) {
> +virFreeError(orig_err);
> +orig_err = NULL;
> +}
> +}
> +}
>  
>  /* If ddomain is NULL, then we were unable to start
>   * the guest on the target, and must restart on the

Otherwise makes sense. I'd like to either have an explanation of the
above control flow or a fixed version.

Peter


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

Re: [libvirt] [PATCH] qemu: Reject updating unsupported disk information

[Peter Krempa]

On Thu, Jul 09, 2015 at 18:36:00 +0200, Martin Kletzander wrote:
> If one calls update-device with information that is not updatable,
> libvirt reports success even though no data were updated.  The example
> used in the bug linked below uses updating device with 
> which, in my opinion, is a valid thing to request from user's
> perspective.  Mainly since we properly error out if user wants to update
> such data on a network device for example.
> 
> And since there are many things that might happen (update-device on disk
> basically knows just how to change removable media), check for what's
> changing and moreover, since the function might be usable in other
> dirvers (updating only disk path is a valid possibility) let's abstract
> it for any two disks.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1007228
> ---
>  src/conf/domain_conf.c   | 111 
> +++
>  src/conf/domain_conf.h   |   2 +
>  src/libvirt_private.syms |   1 +
>  src/qemu/qemu_driver.c   |   3 ++
>  4 files changed, 117 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0219c3c4814d..a6950087d987 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5687,6 +5687,117 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def,
>  return NULL;
>  }
> 
> +
> +/*
> + * Makes sure the @disk differs from @orig_disk only by the source
> + * path and nothing else.  Fields that are being checked and the
> + * information whether they are nullable (can be NULL) or is taken

Um, NULL? see below ...

> + * from the virDomainDiskDefFormat() code.
> + */
> +bool
> +virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk,
> +   virDomainDiskDefPtr orig_disk)
> +{
> +#define CHECK_EQ(field, field_name, nullable)   \
> +do {\
> +if (nullable && !disk->field)   \
> +break;  \
> +if (disk->field != orig_disk->field) {  \
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,   \
> +   _("cannot modify field '%s' of the disk"),   \
> +   field_name); \
> +return false;   \
> +}   \
> +} while (0)
> +
> +CHECK_EQ(device, "device", false);
> +CHECK_EQ(cachemode, "cache", true);

Lets take the line below as an example.

> +CHECK_EQ(error_policy, "error_policy", true);

If disk->error_policy == VIR_DOMAIN_DISK_ERROR_POLICY_DEFAULT (which
equals to 0) the check will be skipped and thus the error_policy setting
might be different and we would not report an error.

> +CHECK_EQ(rerror_policy, "rerror_policy", true);
> +CHECK_EQ(iomode, "io", true);
> +CHECK_EQ(ioeventfd, "ioeventfd", true);
> +CHECK_EQ(event_idx, "event_idx", true);
> +CHECK_EQ(copy_on_read, "copy_on_read", true);
> +CHECK_EQ(discard, "discard", true);
> +CHECK_EQ(iothread, "iothread", true);
> +
> +if (disk->geometry.cylinders &&
> +disk->geometry.heads &&
> +disk->geometry.sectors) {
> +CHECK_EQ(geometry.cylinders, "geometry cylinders", false);
> +CHECK_EQ(geometry.heads, "geometry heads", false);
> +CHECK_EQ(geometry.sectors, "geometry sectors", false);
> +CHECK_EQ(geometry.trans, "BIOS-translation-modus", true);
> +}
> +
> +CHECK_EQ(blockio.logical_block_size,
> + "blockio logical_block_size", false);
> +CHECK_EQ(blockio.physical_block_size,
> + "blockio physical_block_size", false);
> +
> +if (disk->bus == VIR_DOMAIN_DISK_BUS_USB)
> +CHECK_EQ(removable, "removable", true);
> +

As an example of the field below. The field is an integer ... 

> +CHECK_EQ(blkdeviotune.total_bytes_sec,
> + "blkdeviotune.total_bytes_sec",
> + true);
> +CHECK_EQ(blkdeviotune.read_bytes_sec,
> + "blkdeviotune.read_bytes_sec",
> + true);
> +CHECK_EQ(blkdeviotune.write_bytes_sec,
> + "blkdeviotune.write_bytes_sec",
> + true);
> +CHECK_EQ(blkdeviotune.total_iops_sec,
> + "blkdeviotune.total_iops_sec",
> + true);
> +CHECK_EQ(blkdeviotune.read_iops_sec,
> + "blkdeviotune.read_iops_sec",
> + true);
> +CHECK_EQ(blkdeviotune.write_iops_sec,
> + "blkdeviotune.write_iops_sec",
> + true);
> +CHECK_EQ(blkdeviotune.total_bytes_sec_max,
> + "blkdeviotune.total_bytes_sec_max",
> + true);
> +CHECK_EQ(blkdeviotune.read_bytes_sec_max,
> + "blkdeviotune.read_bytes_sec_max",

All of the usage of CHECK_EQ use the s

[libvirt] [PATCH] qemu: don't use initialized ret in qemuRemoveSharedDevice

[Guido Günther]

This fixes

  CC qemu/libvirt_driver_qemu_impl_la-qemu_conf.lo
  qemu/qemu_conf.c: In function 'qemuRemoveSharedDevice':
  qemu/qemu_conf.c:1384:9: error: 'ret' may be used uninitialized in this 
function [-Werror=maybe-uninitialized]
---
This is only catched by Debian Wheezy's gcc 4.7.2, Jessies gcc 4.9.2 doesn't
trap on it.

 src/qemu/qemu_conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 2ab5494..38d4a86 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1381,7 +1381,7 @@ qemuRemoveSharedHostdev(virQEMUDriverPtr driver,
 {
 char *dev_path = NULL;
 char *key = NULL;
-int ret;
+int ret = -1;
 
 if (!qemuIsSharedHostdev(hostdev))
 return 0;
-- 
2.1.4

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

Re: [libvirt] [PATCH] vz: fix cleanup of nets of bridged type

[Maxim Nestratov]

09.07.2015 19:20, Dmitry Guryanov пишет:

We create a virtual network of special type, which
has the same name as bridge name to create bridged
network adapter in vz. So when we delete such an
adapter we have to remove corresponding virtual
network.

So let's rename prlsdkDelNet to prlsdkCleanupBridgedNet
and don't check for return value.

Signed-off-by: Dmitry Guryanov 
---
  src/vz/vz_sdk.c | 25 -
  1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index a312990..d1bc312 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -2986,20 +2986,15 @@ static int prlsdkAddNet(PRL_HANDLE sdkdom,
  return ret;
  }
  
-static int

-prlsdkDelNet(vzConnPtr privconn, virDomainNetDefPtr net)
+static void
+prlsdkCleanupBridgedNet(vzConnPtr privconn, virDomainNetDefPtr net)
  {
-int ret = -1;
  PRL_RESULT pret;
  PRL_HANDLE vnet = PRL_INVALID_HANDLE;
  PRL_HANDLE job = PRL_INVALID_HANDLE;
  
-if (net->type != VIR_DOMAIN_NET_TYPE_BRIDGE) {

-virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
-   _("unplugging network device of type %s is not 
supported"),
-   virDomainNetTypeToString(net->type));
-return ret;
-}
+if (net->type != VIR_DOMAIN_NET_TYPE_BRIDGE)
+return;
  
  pret = PrlVirtNet_Create(&vnet);

  prlsdkCheckRetGoto(pret, cleanup);
@@ -3011,11 +3006,8 @@ prlsdkDelNet(vzConnPtr privconn, virDomainNetDefPtr net)
  if (PRL_FAILED(pret = waitJob(job)))
  goto cleanup;
  
-ret = 0;

-
   cleanup:
  PrlHandle_Free(vnet);
-return ret;
  }
  
  int prlsdkAttachNet(virDomainObjPtr dom,

@@ -3107,8 +3099,7 @@ int prlsdkDetachNet(virDomainObjPtr dom,
  if (sdknet == PRL_INVALID_HANDLE)
  goto cleanup;
  
-if (prlsdkDelNet(privconn, net) < 0)

-goto cleanup;
+prlsdkCleanupBridgedNet(privconn, net);
  
  pret = PrlVmDev_Remove(sdknet);

  prlsdkCheckRetGoto(pret, cleanup);
@@ -3530,7 +3521,7 @@ prlsdkDoApplyConfig(virConnectPtr conn,
  
  if (olddef) {

  for (i = 0; i < olddef->nnets; i++)
-prlsdkDelNet(conn->privateData, olddef->nets[i]);
+prlsdkCleanupBridgedNet(conn->privateData, olddef->nets[i]);
  }
  
  for (i = 0; i < def->nnets; i++) {

@@ -3575,7 +3566,7 @@ prlsdkDoApplyConfig(virConnectPtr conn,
  VIR_FREE(mask);
  
  for (i = 0; i < def->nnets; i++)

-prlsdkDelNet(conn->privateData, def->nets[i]);
+prlsdkCleanupBridgedNet(conn->privateData, def->nets[i]);
  
  return -1;

  }
@@ -3722,7 +3713,7 @@ prlsdkUnregisterDomain(vzConnPtr privconn, 
virDomainObjPtr dom)
  size_t i;
  
  for (i = 0; i < dom->def->nnets; i++)

-prlsdkDelNet(privconn, dom->def->nets[i]);
+prlsdkCleanupBridgedNet(privconn, dom->def->nets[i]);
  
  job = PrlVm_Unreg(privdom->sdkdom);

  if (PRL_FAILED(waitJob(job)))

ACK. Looks better

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

Re: [libvirt] [PATCH] qemu: don't use initialized ret in qemuRemoveSharedDevice

[Peter Krempa]

On Thu, Jul 09, 2015 at 19:22:30 +0200, Guido Günther wrote:
> This fixes
> 
>   CC qemu/libvirt_driver_qemu_impl_la-qemu_conf.lo
>   qemu/qemu_conf.c: In function 'qemuRemoveSharedDevice':
>   qemu/qemu_conf.c:1384:9: error: 'ret' may be used uninitialized in this 
> function [-Werror=maybe-uninitialized]
> ---
> This is only catched by Debian Wheezy's gcc 4.7.2, Jessies gcc 4.9.2 doesn't
> trap on it.
> 
>  src/qemu/qemu_conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Oops. I've missed that in my review. ACK if you didn't push this
already.

Peter


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

Re: [libvirt] [PATCH] qemu: don't use initialized ret in qemuRemoveSharedDevice

[Guido Günther]

Hi,
On Thu, Jul 09, 2015 at 07:28:34PM +0200, Peter Krempa wrote:
> On Thu, Jul 09, 2015 at 19:22:30 +0200, Guido Günther wrote:
> > This fixes
> > 
> >   CC qemu/libvirt_driver_qemu_impl_la-qemu_conf.lo
> >   qemu/qemu_conf.c: In function 'qemuRemoveSharedDevice':
> >   qemu/qemu_conf.c:1384:9: error: 'ret' may be used uninitialized in this 
> > function [-Werror=maybe-uninitialized]
> > ---
> > This is only catched by Debian Wheezy's gcc 4.7.2, Jessies gcc 4.9.2 doesn't
> > trap on it.
> > 
> >  src/qemu/qemu_conf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> 
> Oops. I've missed that in my review. ACK if you didn't push this
> already.

Thanks for the quick response! I would have pushed this under the build
breaker rule but since I'm a bit disconnected with the ML at the moment
I thought I'd rather post a patch first. Pushed now.
Thanks!
 -- Guido

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

[libvirt] [PATCH] qemu: Check duplicate WWNs also for hotplugged disks

[Peter Krempa]

In commit 714b38cb232bcbbd7487af4c058fa6d0999b3326 I tried to avoid
having two disks with the same WWN in a VM. I forgot to check the
hotplug paths though which make it possible bypass that check. Reinforce
the fix by checking the wwn when attaching the disk.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1208009
---
 src/conf/domain_conf.c | 37 +
 1 file changed, 37 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1f7862b..5a9a88d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -22135,6 +22135,34 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
 return 0;
 }

+
+/**
+ * virDomainDefGetDiskByWWN:
+ * @def: domain definition
+ * @wwn: wwn of a disk to find
+ *
+ * Returns a disk definition pointer corresponding to the given WWN identifier
+ * or NULL either if @wwn was NULL or if disk with given WWN is not present in
+ * the domain definition.
+ */
+static virDomainDiskDefPtr
+virDomainDefGetDiskByWWN(virDomainDefPtr def,
+ const char *wwn)
+{
+size_t i;
+
+if (!wwn)
+return NULL;
+
+for (i = 0; i < def->ndisks; i++) {
+if (STREQ_NULLABLE(def->disks[i]->wwn, wwn))
+return def->disks[i];
+}
+
+return NULL;
+}
+
+
 int
 virDomainDefCompatibleDevice(virDomainDefPtr def,
  virDomainDeviceDefPtr dev,
@@ -22178,6 +22206,15 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
 }
 }

+if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
+if (!!virDomainDefGetDiskByWWN(def, dev->data.disk->wwn)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Domain already has a disk with wwn '%s'"),
+   dev->data.disk->wwn);
+return -1;
+}
+}
+
 return 0;
 }

-- 
2.4.5

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

Re: [libvirt] [java] [PATCH 0/6] Fix JNA wrapping, fix memory leaks and wrap security model / label function

[Claudio Bley]

At Wed, 08 Jul 2015 14:42:52 +0200,
Michal Privoznik wrote:
> 
> >> Claudio Bley (6):
> >>   JNA: fix wrong return type void vs. int
> >>   JNA: add CString class and fix memory leaks
> >>   JNA: simplify freeing memory for C strings
> >>   Use the CString class for Arrays of CStrings too
> >>   Implement Domain.getSecurityLabel and add SecurityLabel class
> >>   Implement Connect.getSecurityModel and add SecurityModel class
> > 
> > It has been a while since I posted these patches. Because nobody
> > objected until now, I'm just going to push them.
> 
> Yes, that's the same approach I'm using for libvirt-php, where the
> reviewer's capacity consists of, well, just me. I guess it's okay until
> the time those projects drag more attention.

Thanks, I'll do the same then.

Took me a while, but pushed now (after all this time I had some
_difficulties_ remembering the passphrase of my SSH key... ;-))

-- 
Claudio

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

Re: [libvirt] [PATCH v4 0/5] Add support for migration event

[Jiri Denemark]

On Wed, Jul 08, 2015 at 18:39:36 +0200, Jiri Denemark wrote:
> The final version of migration event patches which got accepted upstream
> had to be slightly modified -- the event is only emitted when "events"
> migration capability is turned on (default is off). Thus a new patch had
> to be added to libvirt. I'm resending all migration event patches for
> context even though they haven't changed at all since they were ACKed.
> 
> Jiri Denemark (5):
>   qemu_monitor: Wire up MIGRATION event
>   qemu: Enable migration events on QMP monitor
>   qemuDomainGetJobStatsInternal: Support migration events
>   qemu: Update migration state according to MIGRATION event
>   qemu: Wait for migration events on domain condition

I did s/virDomainObjSignal/virDomainObjBroadcast/ required after Pavel's
series and pushed these patches. Thanks for the reviews.

Jirka

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

Re: [libvirt] [PATCH] qemu: don't use initialized ret in qemuRemoveSharedDevice

[John Ferlan]

On 07/09/2015 01:28 PM, Peter Krempa wrote:
> On Thu, Jul 09, 2015 at 19:22:30 +0200, Guido Günther wrote:
>> This fixes
>>
>>   CC qemu/libvirt_driver_qemu_impl_la-qemu_conf.lo
>>   qemu/qemu_conf.c: In function 'qemuRemoveSharedDevice':
>>   qemu/qemu_conf.c:1384:9: error: 'ret' may be used uninitialized in this 
>> function [-Werror=maybe-uninitialized]
>> ---
>> This is only catched by Debian Wheezy's gcc 4.7.2, Jessies gcc 4.9.2 doesn't
>> trap on it.
>>
>>  src/qemu/qemu_conf.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
> 
> Oops. I've missed that in my review. ACK if you didn't push this
> already.
> 

Well it was there in the patch that got removed 

http://www.redhat.com/archives/libvir-list/2015-July/msg00209.html

Sorry about that

John

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

Re: [libvirt] [PATCH] qemu: Drop LFs at the end of error from QEMU log

[Jiri Denemark]

On Wed, Jul 08, 2015 at 10:22:06 +0200, Peter Krempa wrote:
> On Wed, Jul 08, 2015 at 10:14:16 +0200, Jiri Denemark wrote:
> > Libvirt's error messages do not end with a LF. However, when reading the
> > error from QEMU log, we would read the LF from the log and keep it in
> > the message.
> > 
> > Signed-off-by: Jiri Denemark 
> > ---
> >  src/qemu/qemu_monitor.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> 
> ACK,

Pushed, thanks.

Jirka

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

Re: [libvirt] [PATCH] qemu: Log all arguments of qemuProcessStart

[Jiri Denemark]

On Wed, Jul 08, 2015 at 09:44:09 +0200, Peter Krempa wrote:
> On Wed, Jul 08, 2015 at 09:31:03 +0200, Jiri Denemark wrote:
> > Signed-off-by: Jiri Denemark 
> > ---
> >  src/qemu/qemu_process.c | 7 ---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> 
> ACK,

Pushed, thanks.

Jirka

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

Re: [libvirt] Entering freeze for libvirt-1.2.17

[Peter Kieser]

On 2015-06-30 2:49 AM, Daniel Veillard wrote:

On Tue, Jun 30, 2015 at 11:00:24AM +0200, Guido Günther wrote:

On Tue, Jun 30, 2015 at 03:00:09PM +0800, Daniel Veillard wrote:

On Mon, Jun 29, 2015 at 10:34:55PM +0200, Guido Günther wrote:

On Sun, Jun 28, 2015 at 01:00:01PM +0800, Daniel Veillard wrote:

   Following discussions on Friday, I applied the patches to deactivate
the subset of Admin APIs and revert from 1.3.0 to 1.2.17. I then tagged
in git and pushed signed tarballs and rpms to the usual place:

   ftp://libvirt.org/pub/libvirt/


  I didn't run my usual tests on that one, my infra is in flux,
so even more reasons for people to give it a try :-)

  I'm likely to make a candidate release 2 on Tuesday and if all goes
well we can push 1.2.17 on Thursday,

Building the tarball fails for me with:

make[4]: Entering directory '/tmp/buildd/libvirt-1.2.17~rc1/debian/build/docs'
missing XHTML1 DTD
cat: internals/locking.html.tmp: No such file or directory
Makefile:2385: recipe for target 'internals/locking.html' failed
make[4]: *** [internals/locking.html] Error 1

   The missing XHTML1 DTD just means you can't validate the locking.html.tmp
against a local copy of the DTD for XHTML1, but then it seems that
the locking.html.tmp wasn't generated.
It should be generated via xsltproc, it seems it's missing in
your build environment, make sure you have it.
Make sure you have xmllint, xsltproc and xhtml1-dtds in your build system,

I should have added that I tried this with and without xsltproc +
xmllint. I now also added the DTDs but no change (the build env didn't
change since the last release).

   humpf ... locking.html.in wasn't touched since Feb, that need more attention
and building from the tarballs worked here, strange

Daniel


http://libvirt.org/git/?p=libvirt.git;a=commit;h=1310b1358cdf9c8acba6e0e85feb869241e59faa

I had to revert this commit to get 1.2.17 to build under debian 
packaging chroot.


-Peter



smime.p7s
Description: S/MIME Cryptographic Signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Entering freeze for libvirt-1.2.17

[Peter Kieser]

On 2015-07-09 2:00 PM, Peter Kieser wrote:



On 2015-06-30 2:49 AM, Daniel Veillard wrote:

On Tue, Jun 30, 2015 at 11:00:24AM +0200, Guido Günther wrote:

On Tue, Jun 30, 2015 at 03:00:09PM +0800, Daniel Veillard wrote:

On Mon, Jun 29, 2015 at 10:34:55PM +0200, Guido Günther wrote:

On Sun, Jun 28, 2015 at 01:00:01PM +0800, Daniel Veillard wrote:
   Following discussions on Friday, I applied the patches to 
deactivate
the subset of Admin APIs and revert from 1.3.0 to 1.2.17. I then 
tagged

in git and pushed signed tarballs and rpms to the usual place:

   ftp://libvirt.org/pub/libvirt/


  I didn't run my usual tests on that one, my infra is in flux,
so even more reasons for people to give it a try :-)

  I'm likely to make a candidate release 2 on Tuesday and if all 
goes

well we can push 1.2.17 on Thursday,

Building the tarball fails for me with:

make[4]: Entering directory 
'/tmp/buildd/libvirt-1.2.17~rc1/debian/build/docs'

missing XHTML1 DTD
cat: internals/locking.html.tmp: No such file or directory
Makefile:2385: recipe for target 'internals/locking.html' failed
make[4]: *** [internals/locking.html] Error 1
   The missing XHTML1 DTD just means you can't validate the 
locking.html.tmp

against a local copy of the DTD for XHTML1, but then it seems that
the locking.html.tmp wasn't generated.
It should be generated via xsltproc, it seems it's missing in
your build environment, make sure you have it.
Make sure you have xmllint, xsltproc and xhtml1-dtds in your build 
system,

I should have added that I tried this with and without xsltproc +
xmllint. I now also added the DTDs but no change (the build env didn't
change since the last release).
   humpf ... locking.html.in wasn't touched since Feb, that need more 
attention

and building from the tarballs worked here, strange

Daniel

http://libvirt.org/git/?p=libvirt.git;a=commit;h=1310b1358cdf9c8acba6e0e85feb869241e59faa 



I had to revert this commit to get 1.2.17 to build under debian 
packaging chroot.


-Peter


As well as:

http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=c0b7d3126be18bea0ce5dcead7bab925bc17cfc5

-Peter



smime.p7s
Description: S/MIME Cryptographic Signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] libxl: set dom0 state to running

[Jim Fehlig]

Commit 45697fe5 added dom0 to driver->domains, but missed
setting its state to 'running'

> virsh list
 IdName   State

 0 Domain-0   shut off

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

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index e72b12d..5f69b49 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -549,6 +549,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
 
 def = NULL;
 
+virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED);
 vm->def->vcpus = d_info.vcpu_online;
 vm->def->maxvcpus = d_info.vcpu_max_id + 1;
 vm->def->mem.cur_balloon = d_info.current_memkb;
-- 
2.1.4

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

Re: [libvirt] Entering freeze for libvirt-1.2.17

[Daniel Veillard]

On Thu, Jul 09, 2015 at 02:00:10PM -0700, Peter Kieser wrote:
> 
> 
> On 2015-06-30 2:49 AM, Daniel Veillard wrote:
> >On Tue, Jun 30, 2015 at 11:00:24AM +0200, Guido Günther wrote:
> >>On Tue, Jun 30, 2015 at 03:00:09PM +0800, Daniel Veillard wrote:
> >>>On Mon, Jun 29, 2015 at 10:34:55PM +0200, Guido Günther wrote:
> On Sun, Jun 28, 2015 at 01:00:01PM +0800, Daniel Veillard wrote:
> >   Following discussions on Friday, I applied the patches to deactivate
> >the subset of Admin APIs and revert from 1.3.0 to 1.2.17. I then tagged
> >in git and pushed signed tarballs and rpms to the usual place:
> >
> >   ftp://libvirt.org/pub/libvirt/
> >
> >
> >  I didn't run my usual tests on that one, my infra is in flux,
> >so even more reasons for people to give it a try :-)
> >
> >  I'm likely to make a candidate release 2 on Tuesday and if all goes
> >well we can push 1.2.17 on Thursday,
> Building the tarball fails for me with:
> 
> make[4]: Entering directory 
> '/tmp/buildd/libvirt-1.2.17~rc1/debian/build/docs'
> missing XHTML1 DTD
> cat: internals/locking.html.tmp: No such file or directory
> Makefile:2385: recipe for target 'internals/locking.html' failed
> make[4]: *** [internals/locking.html] Error 1
> >>>   The missing XHTML1 DTD just means you can't validate the 
> >>> locking.html.tmp
> >>>against a local copy of the DTD for XHTML1, but then it seems that
> >>>the locking.html.tmp wasn't generated.
> >>>It should be generated via xsltproc, it seems it's missing in
> >>>your build environment, make sure you have it.
> >>>Make sure you have xmllint, xsltproc and xhtml1-dtds in your build system,
> >>I should have added that I tried this with and without xsltproc +
> >>xmllint. I now also added the DTDs but no change (the build env didn't
> >>change since the last release).
> >   humpf ... locking.html.in wasn't touched since Feb, that need more 
> > attention
> >and building from the tarballs worked here, strange
> >
> >Daniel
> >
> http://libvirt.org/git/?p=libvirt.git;a=commit;h=1310b1358cdf9c8acba6e0e85feb869241e59faa
> 
> I had to revert this commit to get 1.2.17 to build under debian packaging
> chroot.

  Weird, I don't see why the .html.tmp .html rules fails to apply then ...
it seems to work for everything else

Daniel

-- 
Daniel Veillard  | Open Source and Standards, Red Hat
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

[libvirt] [PATCH v2] qemu: Reject updating unsupported disk information

[Martin Kletzander]

If one calls update-device with information that is not updatable,
libvirt reports success even though no data were updated.  The example
used in the bug linked below uses updating device with 
which, in my opinion, is a valid thing to request from user's
perspective.  Mainly since we properly error out if user wants to update
such data on a network device for example.

And since there are many things that might happen (update-device on disk
basically knows just how to change removable media), check for what's
changing and moreover, since the function might be usable in other
drivers (updating only disk path is a valid possibility) let's abstract
it for any two disks.

We can't possibly check for everything since for many fields our code
does not properly differentiate between default and unspecified values.
Even though this could be changed, I don't feel like it's worth the
complexity so it's not the aim of this patch.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1007228
Signed-off-by: Martin Kletzander 
---

Notes:
v2:
 - Don't say 'NULL' when it should be 'unspecified'
 - Don't blindly copy field name into the error message, but use space
   instead of dot.  That way it looks the same as in the XML provided.
 - Check strings properly instead of addresses

 src/conf/domain_conf.c   | 133 +++
 src/conf/domain_conf.h   |   2 +
 src/libvirt_private.syms |   1 +
 src/qemu/qemu_driver.c   |   3 ++
 4 files changed, 139 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1f7862b00463..69e5df27c270 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5690,6 +5690,139 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def,
 return NULL;
 }

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

Re: [libvirt] [PATCH] qemu: Check duplicate WWNs also for hotplugged disks

[Martin Kletzander]

On Thu, Jul 09, 2015 at 07:56:33PM +0200, Peter Krempa wrote:

In commit 714b38cb232bcbbd7487af4c058fa6d0999b3326 I tried to avoid
having two disks with the same WWN in a VM. I forgot to check the
hotplug paths though which make it possible bypass that check. Reinforce
the fix by checking the wwn when attaching the disk.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1208009
---
src/conf/domain_conf.c | 37 +
1 file changed, 37 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1f7862b..5a9a88d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -22135,6 +22135,34 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
return 0;
}

+


We should reach some consensus with spacing between functions because
every second patch is changing it.  Not your fault, though, just a
random rant.

ACK


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

Re: [libvirt] [PATCH v3] qemu: Use heads parameter for QXL driver

[Martin Kletzander]

On Tue, Jul 07, 2015 at 11:44:11AM +0200, Martin Kletzander wrote:

On Mon, Jul 06, 2015 at 09:18:59AM +0100, Frediano Ziglio wrote:

Allows to specify maximum number of head to QXL driver.

The patch to support the "max_outputs" in Qemu is still not merged but
I got agreement on the name of the argument.



This shouldn't be part of the commit message, we can't push this in
until the code is in qemu anyways, so I'll remove it.


Actually can be a compatiblity problem as heads in the XML configuration
was set by default to '1'.

Signed-off-by: Frediano Ziglio 
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c  | 5 +
3 files changed, 8 insertions(+)

Changes from v2:
- removed capability tests (Martin Kletzander)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 27686c3..68060cd 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -287,6 +287,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
 "aarch64-off",

 "vhost-user-multiqueue", /* 190 */
+  "qxl-vga.max_outputs",
   );


@@ -1649,6 +1650,7 @@ static struct virQEMUCapsStringFlags 
virQEMUCapsObjectPropsQxl[] = {

static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxlVga[] = {
   { "vgamem_mb", QEMU_CAPS_QXL_VGA_VGAMEM },
+{ "max_outputs", QEMU_CAPS_QXL_VGA_MAX_OUTPUTS },
};

struct virQEMUCapsObjectTypeProps {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 30aa504..02f9e81 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -230,6 +230,7 @@ typedef enum {
   QEMU_CAPS_DEVICE_PCI_SERIAL  = 188, /* -device pci-serial */
   QEMU_CAPS_CPU_AARCH64_OFF= 189, /* -cpu ...,aarch64=off */
   QEMU_CAPS_VHOSTUSER_MULTIQUEUE = 190, /* vhost-user with -netdev queues= */
+QEMU_CAPS_QXL_VGA_MAX_OUTPUTS = 191, /* qxl-vga.max_outputs */

   QEMU_CAPS_LAST,   /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 25a7bc6..59666e7 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5661,6 +5661,11 @@ qemuBuildDeviceVideoStr(virDomainDefPtr def,
   /* QEMU accepts mebibytes for vgamem_mb. */
   virBufferAsprintf(&buf, ",vgamem_mb=%u", video->vgamem / 1024);
   }
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGA_MAX_OUTPUTS) &&
+video->heads > 0) {
+virBufferAsprintf(&buf, ",max_outputs=%u", video->heads);
+}


Looks good to me.  I would add a comment here why we're not erroring
out, but rahter using the parameter only if supported, maybe also add
some VIR_INFO output, but no need to resend it just for that.

Let's just wait for QEMU to have this in.  If it's in and I missed the
info, feel free to remind me about pushing this patch to libvirt.



Looking back at this patch, I figured there are no tests to make sure
this doesn't break few commits afterwards.  Take a look at the
tests/qemuxml2argvtest and add at least one there, please.  If the
patch looks like it's adding multiple things, feel free to split it
(one for the capability, other for tests and the hunk in
qemu_command.c).

Sorry for not noticing this earlier.


Martin


   } else if (video->vram &&
   ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
 virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) ||
--
2.1.0



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