Re: [libvirt] [PATCH] qemu: fix unsuitable error report when get memory stats

2015-06-02 Thread Wang Yufei 
On 2015/6/1 15:36, Peter Krempa wrote:

> On Sat, May 30, 2015 at 09:35:35 +0800, Wang Yufei wrote:
>> On 2015/5/29 19:16, Peter Krempa wrote:
>>
>>> On Fri, May 29, 2015 at 17:17:07 +0800, Wang Yufei wrote:
 From: Zhang Bo 

 when we run the command 'virsh dommemstat xxx',
 althrough memballoon's model is set 'none' in vm's XML,
 it still reports an error in libvirtd.log.
 error : qemuMonitorFindBalloonObjectPath:1042 : internal error: Cannot 
 determine balloon device path
 Apparently, if we don't set memballoon, we don't need to
 set balloon device path.
> 
> So this patch is fixing a logged error message in case the memory
> balloon was not configured.
> 

 Signed-off-by: Wang Yufei 
 Signed-off-by: Zhang Bo 
 ---
  .gnulib | 2 +-
  src/qemu/qemu_monitor.c | 4 +++-
  2 files changed, 4 insertions(+), 2 deletions(-)

 diff --git a/.gnulib b/.gnulib
 index 875ec93..106a386 16
 --- a/.gnulib
 +++ b/.gnulib
 @@ -1 +1 @@
 -Subproject commit 875ec93e1501d2d2a8bab1b64fa66b8ceb51dc67
 +Subproject commit 106a3866d01f9dd57ab4f10dbeb0d5a8db73a9f7
>>>
>>> Regular patches should not change the gnulib commit id. Also it is
>>> probably decreasing version of gnulib. Please make sure to update the
>>> submodule before commiting code.
>>>
>>
>> OK, I'll check it, thanks.
>>
 diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
 index f959b74..c72702d 100644
 --- a/src/qemu/qemu_monitor.c
 +++ b/src/qemu/qemu_monitor.c
 @@ -1711,7 +1711,9 @@ qemuMonitorGetMemoryStats(qemuMonitorPtr mon,
  QEMU_CHECK_MONITOR(mon);
  
  if (mon->json) {
 -ignore_value(qemuMonitorFindBalloonObjectPath(mon, "/"));
 +if (mon->vm->def->memballoon &&
 +mon->vm->def->memballoon->model != 
 VIR_DOMAIN_MEMBALLOON_MODEL_NONE)
 +ignore_value(qemuMonitorFindBalloonObjectPath(mon, "/"));
>>>
>>> The qemu monitor code is not the right place to do this check. The API
>>> that is calling the function should make sure that it makes sense to do
>>> this call.
>>>
>>
>> In my opinion, no matter whether we set up memballoon, we both can call
>> qemuMonitorGetMemoryStats to get mem stats. There're two situation to get mem
>> stats in qemuMonitorGetMemoryStats: one with memballoon, the other without
>> memballoon. Right?
>>
>> If we judge the memballoon in the caller 'qemuDomainMemoryStats',
>>
>> qemuDomainObjEnterMonitor(driver, vm);
>> ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats);
>> if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>
>> what should we do? Supply two different functions to get mem stats?
> 
> The function that is actually requesting the stats from the monitor is
> handling well if the monitor does not exist. In my opinion we should
> make it optional for qemuMonitorFindBalloonObjectPath to report errors.
> 
> Since it's used only in two places that should be an easy enough fix.
> 

If there's no balloon set, we still call qemuMonitorFindBalloonObjectPath to 
find
the path, which is a little strange logic.

> Peter



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

[libvirt] [PATCH] nwfilter: Fix sscanf off-by-one error in virNWFilterSnoopLeaseFileLoad

2015-06-02 Thread Erik Skultety 
We allocate 16 bytes for IPv4 address and 55 bytes for interface
key, therefore we should read up to 15/54 bytes and let the last byte
reserved for terminating null byte in sscanf.

https://bugzilla.redhat.com/show_bug.cgi?id=1226400
---
 src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c 
b/src/nwfilter/nwfilter_dhcpsnoop.c
index 6da8983..f331e22 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -1958,8 +1958,8 @@ virNWFilterSnoopLeaseFileLoad(void)
 break;
 }
 ln++;
-/* key len 55 = "VMUUID"+'-'+"MAC" */
-if (sscanf(line, "%u %55s %16s %16s", &ipl.timeout,
+/* key len 54 = "VMUUID"+'-'+"MAC" */
+if (sscanf(line, "%u %54s %15s %15s", &ipl.timeout,
ifkey, ipstr, srvstr) < 4) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("virNWFilterSnoopLeaseFileLoad lease file "
-- 
1.9.3

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

Re: [libvirt] [PATCH v3 0/9] Selective block device migration implementation

2015-06-02 Thread Kashyap Chamarthy 
On Mon, Jun 01, 2015 at 04:41:34PM -0400, John Ferlan wrote:
> On 06/01/2015 04:01 PM, Kashyap Chamarthy wrote:

[. . .]

> > $ git log --oneline | head -9
> > a98cb8d virsh: selective block device migration
> > f57141e qemu: migration: selective block device migration
> > 2757805 util: add virTypedParamsAddStringList
> > 9b4e1be util: virTypedParams{Filter,PickStrings}
> > 4385cf0 util: multi-value parameters in virTypedParamsAdd*
> > f83965b util: multi-value virTypedParameter
> > 89d6ddf qemuMigrationDriveMirror: Pass disk format to qemu
> > dd81938 qemuMigrationBeginPhase: Fix function header indentation
> > c98a95a virDomainDiskGetSource: Mark passed disk as 'const'
> > 
> > And, `make` seems to fail here:
> > 
> > $ ~/tinker-space/libvirt/./autogen.sh --system
> > $ make -j4
> > [. . .]
> > /home/kashyapc/tinker-space/libvirt/./src/qemu/qemu_migration.c: In 
> > function 'qemuMigrationRun':
> > 
> > /home/kashyapc/tinker-space/libvirt/./src/qemu/qemu_migration.c:1997:17: 
> > error: 'format' may be used uninitialized in this function 
> > [-Werror=maybe-uninitialized]
> >  mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, 
> > nbd_dest,
> >  ^
> > 
> > /home/kashyapc/tinker-space/libvirt/./src/qemu/qemu_migration.c:1971:21: 
> > note: 'format' was declared here
> >  const char *format;
> > 
> > 
> 
> See my response to 3/9.
> 
> If you initialize to NULL you'll be able to compile.

Yep, initializing format to NULL results in a successful `make`
invocation:

$ git diff src/qemu/qemu_migration.c
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 5a8057e..0b83a2c 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1968,7 +1968,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
 for (i = 0; i < vm->def->ndisks; i++) {
 virDomainDiskDefPtr disk = vm->def->disks[i];
 qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
-const char *format;
+const char *format = NULL;
 int mon_ret;
 
 /* check whether disk should be migrated */

Thanks.

-- 
/kashyap

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

[libvirt] [PATCH v4 1/6] tests: Add a bunch of new tests to virsh-optparse

2015-06-02 Thread Andrea Bolognani 
The new tests deal with numeric options of three kinds: regular,
scaled and timeouts. For each, both valid and invalid inputs
are provided, hopefully covering all cases: this should allow us
to avoid regressions when changing the relevant code in virsh.
---
 tests/virsh-optparse | 179 +++
 1 file changed, 179 insertions(+)

diff --git a/tests/virsh-optparse b/tests/virsh-optparse
index 39e6cde..37a8d42 100755
--- a/tests/virsh-optparse
+++ b/tests/virsh-optparse
@@ -137,4 +137,183 @@ virsh -q -c $test_url qemu-monitor-command test a >out 
2>err && fail=1
 test -s out && fail=1
 compare exp-err err || fail=1
 
+### Test a regular numeric option
+
+# Non-numeric value
+cat <<\EOF > exp-err || framework_failure
+error: Numeric value for  option is malformed or out of range
+EOF
+virsh -q -c $test_url cpu-stats test --start abc >out 2>err && fail=1
+test -s out && fail=1
+compare exp-err err || fail=1
+
+# Numeric value with invalid suffix
+cat <<\EOF > exp-err || framework_failure
+error: Numeric value for  option is malformed or out of range
+EOF
+virsh -q -c $test_url cpu-stats test --start 42WB >out 2>err && fail=1
+test -s out && fail=1
+compare exp-err err || fail=1
+
+# Numeric value with valid suffix. Suffixes are not supported for
+# regular numeric options, so this value is rejected
+cat <<\EOF > exp-err || framework_failure
+error: Numeric value for  option is malformed or out of range
+EOF
+virsh -q -c $test_url cpu-stats test --start 42MB >out 2>err && fail=1
+test -s out && fail=1
+compare exp-err err || fail=1
+
+# Numeric value bigger than INT_MAX
+cat <<\EOF > exp-err || framework_failure
+error: Numeric value for  option is malformed or out of range
+EOF
+virsh -q -c $test_url cpu-stats test --start 2147483648 >out 2>err && fail=1
+test -s out && fail=1
+compare exp-err err || fail=1
+
+# Negative numeric value. The value is not valid for the command
+# we're testing, but it has been parsed correctly
+cat <<\EOF > exp-err || framework_failure
+error: Invalid value for start CPU
+EOF
+virsh -q -c $test_url cpu-stats test --start -1 >out 2>err && fail=1
+test -s out && fail=1
+compare exp-err err || fail=1
+
+# Zero. The test driver doesn't support the operation so the command
+# fails, but the value has been parsed correctly
+cat <<\EOF > exp-err || framework_failure
+error: Failed to retrieve CPU statistics for domain 'test'
+error: this function is not supported by the connection driver: 
virDomainGetCPUStats
+EOF
+virsh -q -c $test_url cpu-stats test --start 0 >out 2>err && fail=1
+test -s out && fail=1
+compare exp-err err || fail=1
+
+# Valid numeric value. The test driver doesn't support the operation
+# so the command fails, but the value has been parsed correctly
+cat <<\EOF > exp-err || framework_failure
+error: Failed to retrieve CPU statistics for domain 'test'
+error: this function is not supported by the connection driver: 
virDomainGetCPUStats
+EOF
+virsh -q -c $test_url cpu-stats test --start 42 >out 2>err && fail=1
+test -s out && fail=1
+compare exp-err err || fail=1
+
+### Test a scaled numeric option
+
+# Non-numeric value
+cat <<\EOF > exp-err || framework_failure
+error: Numeric value for  option is malformed or out of range
+EOF
+virsh -q -c $test_url setmaxmem test abc >out 2>err && fail=1
+test -s out && fail=1
+compare exp-err err || fail=1
+
+# Numeric value with invalid suffix
+cat <<\EOF > exp-err || framework_failure
+error: Numeric value for  option is malformed or out of range
+EOF
+virsh -q -c $test_url setmaxmem test 42WB >out 2>err && fail=1
+test -s out && fail=1
+compare exp-err err || fail=1
+
+# Numeric value with valid suffix
+virsh -q -c $test_url setmaxmem test 42MB >out 2>err || fail=1
+test -s out && fail=1
+test -s err && fail=1
+
+# Numeric value bigger than INT_MAX. No failure here because
+# scaled numeric values are unsigned long long
+virsh -q -c $test_url setmaxmem test 2147483648 >out 2>err || fail=1
+test -s out && fail=1
+test -s err && fail=1
+
+# Negative numeric value
+cat <<\EOF > exp-err || framework_failure
+error: Numeric value for  option is malformed or out of range
+EOF
+virsh -q -c $test_url setmaxmem test -1 >out 2>err && fail=1
+test -s out && fail=1
+compare exp-err err || fail=1
+
+# Zero. The value is not valid for the command we're testing, but
+# it has been parsed correctly
+cat <<\EOF > exp-err || framework_failure
+error: Unable to change MaxMemorySize
+error: memory in virDomainSetMaxMemory must not be zero
+EOF
+virsh -q -c $test_url setmaxmem test 0 >out 2>err && fail=1
+test -s out && fail=1
+compare exp-err err || fail=1
+
+# Numeric value
+virsh -q -c $test_url setmaxmem test 42 >out 2>err || fail=1
+test -s out && fail=1
+test -s err && fail=1
+
+### Test the  option (numeric option converted to ms)
+
+# Non-numeric value
+cat <<\EOF > exp-err || framework_failure
+error: invalid timeout
+EOF
+virsh -q -c $test_url event --all --timeout abc >out 2>err && fail=1
+test

[libvirt] [PATCH v4 2/6] virsh: Use standard error messages in vshCommandOptTimeoutToMs()

2015-06-02 Thread Andrea Bolognani 
I missed this in the first time around, thanks Michal for noticing.
---
 tests/virsh-optparse | 12 ++--
 tools/virsh.c|  8 ++--
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/tests/virsh-optparse b/tests/virsh-optparse
index 37a8d42..952fcc4 100755
--- a/tests/virsh-optparse
+++ b/tests/virsh-optparse
@@ -257,7 +257,7 @@ test -s err && fail=1
 
 # Non-numeric value
 cat <<\EOF > exp-err || framework_failure
-error: invalid timeout
+error: Numeric value for  option is malformed or out of range
 EOF
 virsh -q -c $test_url event --all --timeout abc >out 2>err && fail=1
 test -s out && fail=1
@@ -266,7 +266,7 @@ compare exp-err err || fail=1
 # Numeric value that's too big to be converted to ms and still
 # fit inside an int
 cat <<\EOF > exp-err || framework_failure
-error: timeout is too big
+error: Numeric value for  option is malformed or out of range
 EOF
 virsh -q -c $test_url event --all --timeout 2147484 >out 2>err && fail=1
 test -s out && fail=1
@@ -274,7 +274,7 @@ compare exp-err err || fail=1
 
 # Numeric value with invalid suffix
 cat <<\EOF > exp-err || framework_failure
-error: invalid timeout
+error: Numeric value for  option is malformed or out of range
 EOF
 virsh -q -c $test_url event --all --timeout 42WB >out 2>err && fail=1
 test -s out && fail=1
@@ -283,7 +283,7 @@ compare exp-err err || fail=1
 # Numeric value with valid suffix. Suffixes are not supported for
 # the  option, so this value is rejected
 cat <<\EOF > exp-err || framework_failure
-error: invalid timeout
+error: Numeric value for  option is malformed or out of range
 EOF
 virsh -q -c $test_url event --all --timeout 42MB >out 2>err && fail=1
 test -s out && fail=1
@@ -291,7 +291,7 @@ compare exp-err err || fail=1
 
 # Negative value
 cat <<\EOF > exp-err || framework_failure
-error: invalid timeout
+error: Numeric value for  option is malformed or out of range
 EOF
 virsh -q -c $test_url event --all --timeout -1 >out 2>err && fail=1
 test -s out && fail=1
@@ -300,7 +300,7 @@ compare exp-err err || fail=1
 # Zero. This is not a valid timeout, but the value is parsed
 # correctly
 cat <<\EOF > exp-err || framework_failure
-error: invalid timeout
+error: Numeric value for  option is malformed or out of range
 EOF
 virsh -q -c $test_url event --all --timeout 0 >out 2>err && fail=1
 test -s out && fail=1
diff --git a/tools/virsh.c b/tools/virsh.c
index 1005ba8..d7964a1 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1869,13 +1869,17 @@ vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd 
*cmd, int *timeout)
 int rv = vshCommandOptInt(cmd, "timeout", timeout);
 
 if (rv < 0 || (rv > 0 && *timeout < 1)) {
-vshError(ctl, "%s", _("invalid timeout"));
+vshError(ctl,
+ _("Numeric value for <%s> option is malformed or out of 
range"),
+ "timeout");
 return -1;
 }
 if (rv > 0) {
 /* Ensure that we can multiply by 1000 without overflowing. */
 if (*timeout > INT_MAX / 1000) {
-vshError(ctl, "%s", _("timeout is too big"));
+vshError(ctl,
+ _("Numeric value for <%s> option is malformed or out of 
range"),
+ "timeout");
 return -1;
 }
 *timeout *= 1000;
-- 
2.1.0

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

[libvirt] [PATCH v4 3/6] virsh: Improve vshCommandOptTimeoutToMs()

2015-06-02 Thread Andrea Bolognani 
Use vshCommandOptUInt() instead of parsing the value as a signed
integer and checking whether it's positive afterwards.

Improve comments as well.
---
 tools/virsh.c | 45 +++--
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index d7964a1..55caa87 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1860,33 +1860,42 @@ vshCommandOptArgv(const vshCmd *cmd, const vshCmdOpt 
*opt)
 return NULL;
 }
 
-/* Parse an optional --timeout parameter in seconds, but store the
- * value of the timeout in milliseconds.  Return -1 on error, 0 if
- * no timeout was requested, and 1 if timeout was set.  */
+/*
+ * vshCommandOptTimeoutToMs:
+ * @ctl virsh control structure
+ * @cmd command reference
+ * @timeout result
+ *
+ * Parse an optional --timeout parameter in seconds, but store the
+ * value of the timeout in milliseconds.
+ * See vshCommandOptInt()
+ */
 int
 vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd *cmd, int *timeout)
 {
-int rv = vshCommandOptInt(cmd, "timeout", timeout);
+int ret;
+unsigned int utimeout;
 
-if (rv < 0 || (rv > 0 && *timeout < 1)) {
+if ((ret = vshCommandOptUInt(cmd, "timeout", &utimeout)) < 0)
 vshError(ctl,
  _("Numeric value for <%s> option is malformed or out of 
range"),
  "timeout");
-return -1;
-}
-if (rv > 0) {
-/* Ensure that we can multiply by 1000 without overflowing. */
-if (*timeout > INT_MAX / 1000) {
-vshError(ctl,
- _("Numeric value for <%s> option is malformed or out of 
range"),
- "timeout");
-return -1;
-}
-*timeout *= 1000;
+if (ret <= 0)
+return ret;
+
+/* Ensure that the timeout is not zero and that we can convert
+ * it from seconds to milliseconds without overflowing. */
+if (utimeout == 0 || utimeout > INT_MAX / 1000) {
+vshError(ctl,
+ _("Numeric value for <%s> option is malformed or out of 
range"),
+ "timeout");
+ret = -1;
+} else {
+*timeout = ((int) utimeout) * 1000;
 }
-return rv;
-}
 
+return ret;
+}
 
 static bool
 vshConnectionUsability(vshControl *ctl, virConnectPtr conn)
-- 
2.1.0

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

[libvirt] [PATCH v4 6/6] virsh: Move error messages inside vshCommandOpt*() functions

2015-06-02 Thread Andrea Bolognani 
---
 tests/vcpupin|   4 +-
 tests/virsh-optparse |  26 -
 tools/virsh-domain-monitor.c |   9 +--
 tools/virsh-domain.c | 134 +++
 tools/virsh-host.c   |  61 +++-
 tools/virsh-interface.c  |   6 +-
 tools/virsh-network.c|   6 +-
 tools/virsh-volume.c |  24 ++--
 tools/virsh.c| 124 +++
 9 files changed, 124 insertions(+), 270 deletions(-)

diff --git a/tests/vcpupin b/tests/vcpupin
index ab0d38f..b6b8b31 100755
--- a/tests/vcpupin
+++ b/tests/vcpupin
@@ -34,7 +34,7 @@ fail=0
 $abs_top_builddir/tools/virsh --connect test:///default vcpupin test a 0,1 > 
out 2>&1
 test $? = 1 || fail=1
 cat <<\EOF > exp || fail=1
-error: Numeric value for  option is malformed or out of range
+error: Numeric value 'a' for  option is malformed or out of range
 
 EOF
 compare exp out || fail=1
@@ -52,7 +52,7 @@ compare exp out || fail=1
 $abs_top_builddir/tools/virsh --connect test:///default vcpupin test -100 0,1 
> out 2>&1
 test $? = 1 || fail=1
 cat <<\EOF > exp || fail=1
-error: Numeric value for  option is malformed or out of range
+error: Numeric value '-100' for  option is malformed or out of range
 
 EOF
 compare exp out || fail=1
diff --git a/tests/virsh-optparse b/tests/virsh-optparse
index 952fcc4..e032094 100755
--- a/tests/virsh-optparse
+++ b/tests/virsh-optparse
@@ -141,7 +141,7 @@ compare exp-err err || fail=1
 
 # Non-numeric value
 cat <<\EOF > exp-err || framework_failure
-error: Numeric value for  option is malformed or out of range
+error: Numeric value 'abc' for  option is malformed or out of range
 EOF
 virsh -q -c $test_url cpu-stats test --start abc >out 2>err && fail=1
 test -s out && fail=1
@@ -149,7 +149,7 @@ compare exp-err err || fail=1
 
 # Numeric value with invalid suffix
 cat <<\EOF > exp-err || framework_failure
-error: Numeric value for  option is malformed or out of range
+error: Numeric value '42WB' for  option is malformed or out of range
 EOF
 virsh -q -c $test_url cpu-stats test --start 42WB >out 2>err && fail=1
 test -s out && fail=1
@@ -158,7 +158,7 @@ compare exp-err err || fail=1
 # Numeric value with valid suffix. Suffixes are not supported for
 # regular numeric options, so this value is rejected
 cat <<\EOF > exp-err || framework_failure
-error: Numeric value for  option is malformed or out of range
+error: Numeric value '42MB' for  option is malformed or out of range
 EOF
 virsh -q -c $test_url cpu-stats test --start 42MB >out 2>err && fail=1
 test -s out && fail=1
@@ -166,7 +166,7 @@ compare exp-err err || fail=1
 
 # Numeric value bigger than INT_MAX
 cat <<\EOF > exp-err || framework_failure
-error: Numeric value for  option is malformed or out of range
+error: Numeric value '2147483648' for  option is malformed or out of 
range
 EOF
 virsh -q -c $test_url cpu-stats test --start 2147483648 >out 2>err && fail=1
 test -s out && fail=1
@@ -205,7 +205,7 @@ compare exp-err err || fail=1
 
 # Non-numeric value
 cat <<\EOF > exp-err || framework_failure
-error: Numeric value for  option is malformed or out of range
+error: Numeric value 'abc' for  option is malformed or out of range
 EOF
 virsh -q -c $test_url setmaxmem test abc >out 2>err && fail=1
 test -s out && fail=1
@@ -213,7 +213,7 @@ compare exp-err err || fail=1
 
 # Numeric value with invalid suffix
 cat <<\EOF > exp-err || framework_failure
-error: Numeric value for  option is malformed or out of range
+error: Numeric value '42WB' for  option is malformed or out of range
 EOF
 virsh -q -c $test_url setmaxmem test 42WB >out 2>err && fail=1
 test -s out && fail=1
@@ -232,7 +232,7 @@ test -s err && fail=1
 
 # Negative numeric value
 cat <<\EOF > exp-err || framework_failure
-error: Numeric value for  option is malformed or out of range
+error: Numeric value '-1' for  option is malformed or out of range
 EOF
 virsh -q -c $test_url setmaxmem test -1 >out 2>err && fail=1
 test -s out && fail=1
@@ -257,7 +257,7 @@ test -s err && fail=1
 
 # Non-numeric value
 cat <<\EOF > exp-err || framework_failure
-error: Numeric value for  option is malformed or out of range
+error: Numeric value 'abc' for  option is malformed or out of range
 EOF
 virsh -q -c $test_url event --all --timeout abc >out 2>err && fail=1
 test -s out && fail=1
@@ -266,7 +266,7 @@ compare exp-err err || fail=1
 # Numeric value that's too big to be converted to ms and still
 # fit inside an int
 cat <<\EOF > exp-err || framework_failure
-error: Numeric value for  option is malformed or out of range
+error: Numeric value '2147484' for  option is malformed or out of 
range
 EOF
 virsh -q -c $test_url event --all --timeout 2147484 >out 2>err && fail=1
 test -s out && fail=1
@@ -274,7 +274,7 @@ compare exp-err err || fail=1
 
 # Numeric value with invalid suffix
 cat <<\EOF > exp-err || framework_failure
-error: Numeric value for  option is malformed or out of range
+error: Numeric value '42WB

[libvirt] [PATCH v4 0/6] virsh: Further improve handling of integer options

2015-06-02 Thread Andrea Bolognani 
As suggested by Michal: now that we have a generic error message for
failures related to the parsing of integer options, it makes sense to
perform the corresponding check in a single spot instead of replicating
it every time vshCommandOpt*() is used.

Andrea Bolognani (6):
  tests: Add a bunch of new tests to virsh-optparse
  virsh: Use standard error messages in vshCommandOptTimeoutToMs()
  virsh: Improve vshCommandOptTimeoutToMs()
  virsh: Make vshCommandOptScaledInt() use vshCommandOpt()
  virsh: Pass vshControl to all vshCommandOpt*() calls
  virsh: Move error messages inside vshCommandOpt*() functions

 tests/vcpupin|   4 +-
 tests/virsh-optparse | 179 ++
 tools/virsh-domain-monitor.c |  17 +--
 tools/virsh-domain.c | 226 --
 tools/virsh-host.c   |  67 +++-
 tools/virsh-interface.c  |   6 +-
 tools/virsh-network.c|  10 +-
 tools/virsh-nodedev.c|   4 +-
 tools/virsh-snapshot.c   |   2 +-
 tools/virsh-volume.c |  26 +
 tools/virsh.c| 252 ++-
 tools/virsh.h|  66 ++--
 12 files changed, 461 insertions(+), 398 deletions(-)

-- 
2.1.0

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

[libvirt] [PATCH v4 4/6] virsh: Make vshCommandOptScaledInt() use vshCommandOpt()

2015-06-02 Thread Andrea Bolognani 
This aligns it to the other vshCommandOpt*() functions.
---
 tools/virsh.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index 55caa87..cd2bfef 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1804,16 +1804,16 @@ vshCommandOptScaledInt(const vshCmd *cmd, const char 
*name,
unsigned long long *value, int scale,
unsigned long long max)
 {
-const char *str;
-int ret;
+vshCmdOpt *arg;
 char *end;
+int ret;
 
-ret = vshCommandOptString(cmd, name, &str);
-if (ret <= 0)
+if ((ret = vshCommandOpt(cmd, name, &arg, true)) <= 0)
 return ret;
-if (virStrToLong_ullp(str, &end, 10, value) < 0 ||
+if (virStrToLong_ullp(arg->data, &end, 10, value) < 0 ||
 virScaleInteger(value, end, scale, max) < 0)
 return -1;
+
 return 1;
 }
 
-- 
2.1.0

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

[libvirt] [PATCH v4 5/6] virsh: Pass vshControl to all vshCommandOpt*() calls

2015-06-02 Thread Andrea Bolognani 
This will allow us to use vshError() to report errors from inside
vshCommandOpt*(), instead of replicating the same logic and error
messages all over the place.

We also have more context inside the vshCommandOpt*() functions,
for example the actual value used on the command line, which means
we can produce more detailed error messages.

vshCommandOptBool() is the exception here, because it's explicitly
designed not to report any error.
---
 tools/virsh-domain-monitor.c |  10 ++--
 tools/virsh-domain.c | 128 +--
 tools/virsh-host.c   |  24 
 tools/virsh-interface.c  |   2 +-
 tools/virsh-network.c|   6 +-
 tools/virsh-nodedev.c|   4 +-
 tools/virsh-snapshot.c   |   2 +-
 tools/virsh-volume.c |  10 ++--
 tools/virsh.c|  97 ++--
 tools/virsh.h|  66 +++---
 10 files changed, 183 insertions(+), 166 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index a42c150..d8b217b 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -340,7 +340,7 @@ cmdDomMemStat(vshControl *ctl, const vshCmd *cmd)
 /* Providing a period will adjust the balloon driver collection period.
  * This is not really an unsigned long, but it
  */
-if ((rv = vshCommandOptInt(cmd, "period", &period)) < 0) {
+if ((rv = vshCommandOptInt(ctl, cmd, "period", &period)) < 0) {
 vshError(ctl,
  _("Numeric value for <%s> option is malformed or out of 
range"),
  "period");
@@ -1436,7 +1436,7 @@ cmdDomTime(vshControl *ctl, const vshCmd *cmd)
 if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
 return false;
 
-rv = vshCommandOptLongLong(cmd, "time", &seconds);
+rv = vshCommandOptLongLong(ctl, cmd, "time", &seconds);
 
 if (rv < 0) {
 /* invalid integer format */
@@ -2165,7 +2165,7 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 ndoms = 1;
 
-while ((opt = vshCommandOptArgv(cmd, opt))) {
+while ((opt = vshCommandOptArgv(ctl, cmd, opt))) {
 if (!(dom = vshLookupDomainBy(ctl, opt->data,
   VSH_BYID | VSH_BYUUID | VSH_BYNAME)))
 goto cleanup;
@@ -2244,9 +2244,9 @@ cmdDomIfAddr(vshControl *ctl, const vshCmd *cmd)
 if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
 return false;
 
-if (vshCommandOptString(cmd, "interface", &ifacestr) < 0)
+if (vshCommandOptString(ctl, cmd, "interface", &ifacestr) < 0)
 goto cleanup;
-if (vshCommandOptString(cmd, "source", &sourcestr) < 0)
+if (vshCommandOptString(ctl, cmd, "source", &sourcestr) < 0)
 goto cleanup;
 
 if (sourcestr) {
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index fc4d1fc..79a9a9b 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1292,7 +1292,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptStringReq(ctl, cmd, "device", &disk) < 0)
 goto cleanup;
 
-if ((rv = vshCommandOptULongLong(cmd, "total-bytes-sec", &value)) < 0) {
+if ((rv = vshCommandOptULongLong(ctl, cmd, "total-bytes-sec", &value)) < 
0) {
 goto interror;
 } else if (rv > 0) {
 if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1301,7 +1301,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
 goto save_error;
 }
 
-if ((rv = vshCommandOptULongLong(cmd, "read-bytes-sec", &value)) < 0) {
+if ((rv = vshCommandOptULongLong(ctl, cmd, "read-bytes-sec", &value)) < 0) 
{
 goto interror;
 } else if (rv > 0) {
 if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1310,7 +1310,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
 goto save_error;
 }
 
-if ((rv = vshCommandOptULongLong(cmd, "write-bytes-sec", &value)) < 0) {
+if ((rv = vshCommandOptULongLong(ctl, cmd, "write-bytes-sec", &value)) < 
0) {
 goto interror;
 } else if (rv > 0) {
 if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1319,7 +1319,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
 goto save_error;
 }
 
-if ((rv = vshCommandOptULongLong(cmd, "total-bytes-sec-max", &value)) < 0) 
{
+if ((rv = vshCommandOptULongLong(ctl, cmd, "total-bytes-sec-max", &value)) 
< 0) {
 goto interror;
 } else if (rv > 0) {
 if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1328,7 +1328,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
 goto save_error;
 }
 
-if ((rv = vshCommandOptULongLong(cmd, "read-bytes-sec-max", &value)) < 0) {
+if ((rv = vshCommandOptULongLong(ctl, cmd, "read-bytes-sec-max", &value)) 
< 0) {
 goto interror;
 } else if (rv > 0) {
 if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,

Re: [libvirt] [PATCH v3 2/5] virsh: Improve vshCommandOptTimeoutToMs().

2015-06-02 Thread Andrea Bolognani 
On Sun, 2015-05-31 at 15:09 -0400, John Ferlan wrote:

> >  int
> >  vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd *cmd, int *timeout)
> >  {
> > -int rv = vshCommandOptInt(cmd, "timeout", timeout);
> > +int ret;
> > +unsigned int utimeout;
> >  
> > -if (rv < 0 || (rv > 0 && *timeout < 1)) {
> > +if ((ret = vshCommandOptUInt(cmd, "timeout", &utimeout)) < 0)
> 
> This changes the logic such that utimeout == 0 doesn't get messaged like
> it would have previously if *timeout was == 0 (or < 1).

My bad. I've added a bunch of test cases to v4 so that something like
this is unlikely to slip through the cracks again.

> >  vshError(ctl,
> >   _("Numeric value for <%s> option is malformed or out of 
> > range"),
> >   "timeout");
> > -return -1;
> > -}
> > -if (rv > 0) {
> > -/* Ensure that we can multiply by 1000 without overflowing. */
> > -if (*timeout > INT_MAX / 1000) {
> > -vshError(ctl, "%s", _("timeout is too big"));
> > -return -1;
> > -}
> > -*timeout *= 1000;
> > +if (ret <= 0)
> 
> s/<=/==
> 
> It cannot be < due to previous check. So no option, returns 0

This is actually correct: if the value returned by vshCommandOptUInt()
is negative, an error is reported; an early return is then performed if
the value was <= 0, which means either an error, or no value provided
for an non-mandatory option.

> > +return ret;
> > +
> > +/* Ensure that we can multiply by 1000 without overflowing. */
> > +if (utimeout > INT_MAX / 1000) {
> > +vshError(ctl, "%s", _("timeout is too big"));
> 
> s/big/long
> 
> (ironically ;-))

I've changed it to report the common error message instead, because this
is really an out-of-range condition. Same for zero. If you feel like a
more specific error message is warranted here we can definitely do that,
I don't feel strongly either way :)

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team
$ python -c "print('a'.join(['', 'bologn', '@redh', 't.com']))"

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

[libvirt] [PATCH] qemu: monitor: Add memory balloon support for virtio-ccw

2015-06-02 Thread Boris Fiuczynski 
The search for the memory ballon driver object is extended by a
second known name "virtio-ballon-ccw" in support for virtio-ccw.

Signed-off-by: Boris Fiuczynski 
Reviewed-by: Christian Borntraeger 
---
 src/qemu/qemu_monitor.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index f959b74..1a88329 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1141,9 +1141,9 @@ qemuMonitorFindObjectPath(qemuMonitorPtr mon,
 
 
 /**
- * Search the qom objects for the balloon driver object by it's known name
- * of "virtio-balloon-pci".  The entry for the driver will be found by using
- * function "qemuMonitorFindObjectPath".
+ * Search the qom objects for the balloon driver object by it's known names
+ * of "virtio-balloon-pci" or "virtio-ballon-ccw". The entry for the driver
+ * will be found by using function "qemuMonitorFindObjectPath".
  *
  * Once found, check the entry to ensure it has the correct property listed.
  * If it does not, then obtaining statistics from QEMU will not be possible.
@@ -1183,7 +1183,8 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
 return -1;
 }
 
-if (qemuMonitorFindObjectPath(mon, curpath, "virtio-balloon-pci", &path) < 
0)
+if (qemuMonitorFindObjectPath(mon, curpath, "virtio-balloon-pci", &path) < 
0 &&
+qemuMonitorFindObjectPath(mon, curpath, "virtio-balloon-ccw", &path) < 
0)
 return -1;
 
 nprops = qemuMonitorJSONGetObjectListPaths(mon, path, &bprops);
-- 
2.3.0


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

Re: [libvirt] [PATCH] nwfilter: Fix sscanf off-by-one error in virNWFilterSnoopLeaseFileLoad

2015-06-02 Thread Martin Kletzander 

On Tue, Jun 02, 2015 at 10:18:34AM +0200, Erik Skultety wrote:

We allocate 16 bytes for IPv4 address and 55 bytes for interface
key, therefore we should read up to 15/54 bytes and let the last byte
reserved for terminating null byte in sscanf.

https://bugzilla.redhat.com/show_bug.cgi?id=1226400
---
src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c 
b/src/nwfilter/nwfilter_dhcpsnoop.c
index 6da8983..f331e22 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -1958,8 +1958,8 @@ virNWFilterSnoopLeaseFileLoad(void)
break;
}
ln++;
-/* key len 55 = "VMUUID"+'-'+"MAC" */
-if (sscanf(line, "%u %55s %16s %16s", &ipl.timeout,
+/* key len 54 = "VMUUID"+'-'+"MAC" */
+if (sscanf(line, "%u %54s %15s %15s", &ipl.timeout,
   ifkey, ipstr, srvstr) < 4) {


We initialize ifkey as char ifkey[VIR_IFKEY_LEN], so it might be nicer
to call:

 if (sscanf(line, "%u %*s %*s %*s", &ipl.timeout,
   VIR_IFKEY_LEN - 1, ifkey,
INET_ADDRSTRLEN - 1, ipstr,
INET_ADDRSTRLEN - 1, srvstr) < 4) {
 ...

But what you have is enough, so ACK to that.


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 0/5] virsh: Further improve handling of integer options

2015-06-02 Thread Andrea Bolognani 
On Sun, 2015-05-31 at 15:19 -0400, John Ferlan wrote:

First of all, thanks for the review :)

> Patch 2 had some comments which should be simple to fix

I've commented in detail in your other mail, should have taken care of
them all.

> Patch 4 had a couple of NIT's about going beyond 80 columns on a line,
> but since it's so pervasive in the rest of the module that can be a
> future cleanup ;-)...

Sure, there's always time for cleaning up after the cleanup :)

> I do note that 'vshCommandOpt' now has an
> unrelated difference - I assume it had a ctl argument at one point
> that's since been removed...

I'm not sure what you mean here, it looks fine to me. Care to explain in
more detail?

> Patch 5 still seemed to need some sort of error message in
> vshCommandOptString... Some callers ignore return status, so even adding
> an error may still be "ignored".

This series deals with numeric options only. String options are
something I will probably tackle in the near future :)

> Overall seems OK to me with some minor cleanups.

Looks like this series is growing a new commit for every subsequent
review! I've just posted v4, which should address your comments. Please
take a look at it and let me know if there's more work to do.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team
$ python -c "print('a'.join(['', 'bologn', '@redh', 't.com']))"

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

Re: [libvirt] [PATCH] nwfilter: Fix sscanf off-by-one error in virNWFilterSnoopLeaseFileLoad

2015-06-02 Thread Erik Skultety 


On 06/02/2015 11:35 AM, Martin Kletzander wrote:
> On Tue, Jun 02, 2015 at 10:18:34AM +0200, Erik Skultety wrote:
>> We allocate 16 bytes for IPv4 address and 55 bytes for interface
>> key, therefore we should read up to 15/54 bytes and let the last byte
>> reserved for terminating null byte in sscanf.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1226400
>> ---
>> src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c
>> b/src/nwfilter/nwfilter_dhcpsnoop.c
>> index 6da8983..f331e22 100644
>> --- a/src/nwfilter/nwfilter_dhcpsnoop.c
>> +++ b/src/nwfilter/nwfilter_dhcpsnoop.c
>> @@ -1958,8 +1958,8 @@ virNWFilterSnoopLeaseFileLoad(void)
>> break;
>> }
>> ln++;
>> -/* key len 55 = "VMUUID"+'-'+"MAC" */
>> -if (sscanf(line, "%u %55s %16s %16s", &ipl.timeout,
>> +/* key len 54 = "VMUUID"+'-'+"MAC" */
>> +if (sscanf(line, "%u %54s %15s %15s", &ipl.timeout,
>>ifkey, ipstr, srvstr) < 4) {
> 
> We initialize ifkey as char ifkey[VIR_IFKEY_LEN], so it might be nicer
> to call:
> 
>  if (sscanf(line, "%u %*s %*s %*s", &ipl.timeout,
>VIR_IFKEY_LEN - 1, ifkey,
> INET_ADDRSTRLEN - 1, ipstr,
> INET_ADDRSTRLEN - 1, srvstr) < 4) {
>  ...
> 
I'd like to disagree --> 'error: too many arguments for format', perhaps
you've mistaken this for printf where man says:
"Instead of a decimal digit string one may write "*" or  "*m$" (for some
decimal integer m) to specify that the field width is given in the next
argument, or in the m-th argument, respectively, which must be of type int."

This however is different for scanf, where '*' is defined as
assignment-suppression character

> But what you have is enough, so ACK to that.
Thanks for the ACK though.

Regards,
Erik

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

Re: [libvirt] [PATCH] nwfilter: Fix sscanf off-by-one error in virNWFilterSnoopLeaseFileLoad

2015-06-02 Thread Martin Kletzander 

On Tue, Jun 02, 2015 at 12:21:32PM +0200, Erik Skultety wrote:



On 06/02/2015 11:35 AM, Martin Kletzander wrote:

On Tue, Jun 02, 2015 at 10:18:34AM +0200, Erik Skultety wrote:

We allocate 16 bytes for IPv4 address and 55 bytes for interface
key, therefore we should read up to 15/54 bytes and let the last byte
reserved for terminating null byte in sscanf.

https://bugzilla.redhat.com/show_bug.cgi?id=1226400
---
src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c
b/src/nwfilter/nwfilter_dhcpsnoop.c
index 6da8983..f331e22 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -1958,8 +1958,8 @@ virNWFilterSnoopLeaseFileLoad(void)
break;
}
ln++;
-/* key len 55 = "VMUUID"+'-'+"MAC" */
-if (sscanf(line, "%u %55s %16s %16s", &ipl.timeout,
+/* key len 54 = "VMUUID"+'-'+"MAC" */
+if (sscanf(line, "%u %54s %15s %15s", &ipl.timeout,
   ifkey, ipstr, srvstr) < 4) {


We initialize ifkey as char ifkey[VIR_IFKEY_LEN], so it might be nicer
to call:

 if (sscanf(line, "%u %*s %*s %*s", &ipl.timeout,
   VIR_IFKEY_LEN - 1, ifkey,
INET_ADDRSTRLEN - 1, ipstr,
INET_ADDRSTRLEN - 1, srvstr) < 4) {
 ...


I'd like to disagree --> 'error: too many arguments for format', perhaps
you've mistaken this for printf where man says:
"Instead of a decimal digit string one may write "*" or  "*m$" (for some
decimal integer m) to specify that the field width is given in the next
argument, or in the m-th argument, respectively, which must be of type int."



Yep, for sure that's what I did, sorry ;)


This however is different for scanf, where '*' is defined as
assignment-suppression character


But what you have is enough, so ACK to that.

Thanks for the ACK though.



You're welcome


Regards,
Erik


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

Re: [libvirt] [RFC] libvirt support multi-thread compression for live migration

2015-06-02 Thread Eric Blake 
[correcting list address: libvirt-list, not libver-list]

On 06/02/2015 05:58 AM, Feng, Shaohe wrote:
> Hi folks:
> I'd like to request some comments on enabling multi-thread compression in 
> libvirt.
> 
> Currently, qemu upstream has supported multi-thread compression for live 
> migration.
> $ git log 263170e~1..362ba4e -oneline
> This can preserve bandwidth and speed up the live migration process, so this 
> is an import
> feature we need to enable so for other high level such as openstack will be 
> benefit.
> 
> We plan to add feature of multi-thread compression and actually the patch is 
> working in process.
> 
> Now some things need for comments.
> 
> 1.  Expose new set/get multi-thread compression parameters API for live 
> migration.
> Now qemu only supports zlib compression. Maybe LZ4 and gzip will be 
> supported later.
> but there are 3 parameters needed by qemu currently.
> "compress-level":  compression level, default is 1. For zlib compression, it 
> is from 0-9, 0 means use default level, 9 means high compressed.
> "compress-threads":  compression thread count for multi-thread migration, 
> default is 8 in qemu.
> "decompress-threads": decompression thread count for multi-thread migration, 
> default is 2 in qemu
> 
> So in libvirt we will expose the public symbols as follow, we only 3 
> parameters,
> missing compression method(zlib, LZ4 and gzip) parameters, for qemu do not 
> support it at present.
> index d851225..103b3b9 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -798,6 +798,17 @@ int virDomainMigrateGetMaxSpeed(virDomainPtr domain,
>  unsigned long *bandwidth,
>  unsigned int flags);
> 
> +int virdomainMigrateSetParameters(virDomainPtr domain,
> +  int level,
> +  int threads,
> +  int dthreads,
> +  int flags)
> +int virdomainMigrateGetParameters(virDomainPtr domain,
> +  int *level,
> +  int *threads,
> +  int *dthreads,
> +  int flags)
> +

I'd rather we used virTypedParameters, to make it easier to use the same
API to pass ALL future possible tuning parameters, rather than just
hard-coding to only the parameters of this one feature.

> 
> For virdomainMigrateSetParameters, if specifying a negative value to level, 
> threads, and dthreads parameters, such as -1,
> means do not set the parameters.
> The underlying code will not pass the corresponding parameters to qemu 
> monitor.
> Such as threads and dthreads is -1, then pass the following json streaming to 
> qemu.
> { "execute": "migrate-set-parameters" , "arguments":  { "compress-level": 1 } 
> }
> 
> The virsh will expose the two commands:
> # virsh migrate-setparameters   [--level level] [--threads threads] 
> [--dthreads dthread]
> # virsh migrate-getparameters  
> 
> 2.  How to enable multi-thread compression in application interface?
>There is not a special interface for setting migration capabilities.
> But we can set the capabilities when execute an virsh command as 
> following:
> $ migrate --live] [--offline] [--p2p] [--direct] [--tunnelled] 
> [--persistent] [--undefinesource] [--suspend] [--copy-storage-all] 
> [--copy-storage-inc] [--change-protection] [--unsafe] [--verbose] 
> [--compressed] [--abort-on-error]   [] 
> [] [] [] [--timeout ] [--xml 
> ]
> 
> There is already a "compressed" option,  here "compressed" means "xbzrle" not 
> "multi- compressed".
> can I rename the "compressed" to "xbzrle"?

If we think that it is worth always turning on both compression styles
simultaneously, then reuse the bit.  Otherwise, we need a different bit,
and users can choose which (or both) of the two compression styles as
desired.

> so we add another option for multi-thread compression, which is better option 
> name? "-multi- compressed" ?
> Any way this is confuse to user. We need to 
> distinguish these two.
> 
> 
>So I wonder should we expose new API to enable the migrate multi-thread 
> compress.
> +int virdomainMigrateEnableMultiThreadCompress (virDomainPtr domain,  int 
> flags)
>It will pass the following command to qemu monitor.
>{ "execute": "migrate-set-capabilities" , "arguments": { "capabilities": [ 
> { "capability": " compress", "state": true } ] } }
> 
> NOTE:  Now, multiple thread compression can co-work with xbzrle. When xbzrle 
> is on, multiple thread compression will only work at the first round of RAM 
> data sync.
> Qemu, $ git show 98f1138902195bd9ab8a753d0ee2cf2a0a88b6e8
> if compress and xbzrle are both on, compress only takes effect in the ram 
> bulk stage, after that, it will be disabled and only xbzrle takes effect, 
> this can help to minimize mig

[libvirt] Bug 1227257

2015-06-02 Thread 王松波 
I report a bug [Bug 1227257] . In the environment  libvirt-1.2.16.tar.gz
+ qemu-img version 2.1.2 + ceph version 0.94.1.
libvirt pool will become inactive after one client does vol.delete and the
other does pool.refresh in the same pool simultaneously.

The reason is that rbd_list and  rbd_open are not wrapped in an atomic
operation, but two seperate operations.
For example, two clients are operating in the same pool at the same time.
One client does rbd_list, and got 10 rbd images, meanwhile, the other
client deletes one of the rbd image. In this situation, when the first
client does next operation, such as rbd_open , the command may fail,
because the rbd image has been removed.

I write a testcase in python to reproduce this problem as follow(also have
put it in the attachment):
##
import libvirt
import sys
import time
import sys
#coding:utf-8

QEMU_URL = 'qemu:///system'

VOL_TEMPLATE='''

{vol}
{pool}/{vol}


{cap_size}
{alloc_size}

rbd:{pool}/{vol}


00
0
0


'''

def create_vol(pool_name, vol_name, cap_size, alloc_size):
conn = libvirt.open(QEMU_URL)
if conn == None:
print 'Failed to open connection to the hypervisor'
sys.exit(1)

try:
pool = conn.storagePoolLookupByName(pool_name)
pool.refresh()
template = VOL_TEMPLATE.format(pool=pool_name, vol=vol_name,
cap_size=cap_size, alloc_size=alloc_size)
pool.createXML(template, 0)
except:
print 'Failed to open pool'
sys.exit(1)
finally:
if conn is not None:
conn.close()

def destroy_vol(pool_name, vol_name):
conn = libvirt.open(QEMU_URL)
if conn == None:
sys.exit(1)
pool = conn.storagePoolLookupByName(pool_name)
pool.refresh(0)
vol = pool.storageVolLookupByName(vol_name)
if vol is not None:
vol.delete(0)
if conn is not None:
 conn.close()

if  sys.argv[2] == 'create':
for i in range(1, 20):
volname = 'pool-down-test-%s-%d' % (sys.argv[1], i)
print 'create %s' % volname
create_vol('capacity', volname, '500', '500')
elif sys.argv[2] == 'destroy':
for i in range(1, 20):
volname = 'pool-down-test-%s-%d' % (sys.argv[1], i)
print 'destroy %s' % volname
destroy_vol('capacity', volname)
else:
print 'Usage: python vol-test.py clientId OPER'
print '  '
print 'where  clientId : a num/string used to distinguish different
client'
print '   OPER : create/destroy'




Patch for this is as fllow:

diff --git a/src/storage/storage_backend_rbd.c
b/src/storage/storage_backend_rbd.c
index ae4bcb3..24fbc84 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -266,6 +266,46 @@ static int
virStorageBackendRBDCloseRADOSConn(virStorageBackendRBDStatePtr ptr)
 return ret;
 }

+static int volStorageBackendRBDVolIsExist(char *volname,
virStorageBackendRBDStatePtr ptr)
+{
+int ret = -1;
+char *name, *names = NULL;
+size_t max_size = 1024;
+int len = -1;
+
+while (true) {
+if (VIR_ALLOC_N(names, max_size) < 0)
+goto cleanup;
+
+len = rbd_list(ptr->ioctx, names, &max_size);
+if (len >= 0)
+break;
+if (len != -ERANGE) {
+VIR_WARN("%s", _("A problem occurred while listing RBD
images"));
+goto cleanup;
+}
+VIR_FREE(names);
+}
+
+for (name = names; name < names + max_size;) {
+
+if (STREQ(name, ""))
+break;
+
+name += strlen(name) + 1;
+if (STREQ(volname, name)) {
+VIR_ERROR("RBD images '%s' is exist, but cannot open it",
volname);
+ret = -2;
+break;
+}
+}
+ret = 0;
+
+cleanup:
+VIR_FREE(names);
+return ret;
+}
+
 static int volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
   virStoragePoolObjPtr pool,
   virStorageBackendRBDStatePtr
ptr)
@@ -276,8 +316,15 @@ static int
volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,

 r = rbd_open(ptr->ioctx, vol->name, &image, NULL);
 if (r < 0) {
-virReportSystemError(-r, _("failed to open the RBD image '%s'"),
- vol->name);
+VIR_DEBUG("failed to open RBD image '%s', check if it was still
exist in its pool",\
+  vol->name);
+if (volStorageBackendRBDVolIsExist(vol->name, ptr) == 0) {
+VIR_DEBUG("vol '%s' may be removed by the other rbd client",
vol->name);
+ret = 0;
+} else {
+virReportSystemError(-r, _("failed to open the RBD image
'%s'"),
+vol->name);
+}
 return ret;
 }
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/5] virsh: Further improve handling of integer options

2015-06-02 Thread John Ferlan 


On 06/02/2015 05:35 AM, Andrea Bolognani wrote:
> On Sun, 2015-05-31 at 15:19 -0400, John Ferlan wrote:
> 
> First of all, thanks for the review :)
> 
>> Patch 2 had some comments which should be simple to fix
> 
> I've commented in detail in your other mail, should have taken care of
> them all.
> 
>> Patch 4 had a couple of NIT's about going beyond 80 columns on a line,
>> but since it's so pervasive in the rest of the module that can be a
>> future cleanup ;-)...
> 
> Sure, there's always time for cleaning up after the cleanup :)
> 
>> I do note that 'vshCommandOpt' now has an
>> unrelated difference - I assume it had a ctl argument at one point
>> that's since been removed...
> 
> I'm not sure what you mean here, it looks fine to me. Care to explain in
> more detail?
> 

I use gitk when perusing differences and saw the following which
ironically may have taken care of an 80 col thing, but in reality had no
"real" difference other than putting P2 and P3 on a second line. I thus
assumed you had added the *ctl argument to this function at one time -
perhaps as P1 (I think in fact there may have been a review comment on
it)...

 static int
-vshCommandOpt(const vshCmd *cmd, const char *name, vshCmdOpt **opt,
+vshCommandOpt(const vshCmd *cmd,
+  const char *name, vshCmdOpt **opt,
   bool needData)


>> Patch 5 still seemed to need some sort of error message in
>> vshCommandOptString... Some callers ignore return status, so even adding
>> an error may still be "ignored".
> 
> This series deals with numeric options only. String options are
> something I will probably tackle in the near future :)
> 
>> Overall seems OK to me with some minor cleanups.
> 
> Looks like this series is growing a new commit for every subsequent
> review! I've just posted v4, which should address your comments. Please
> take a look at it and let me know if there's more work to do.
>

I'll look today at the updates...

John

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

[libvirt] [PATCH v2 18/22] qemu: Update migration state according to MIGRATION event

2015-06-02 Thread Jiri Denemark 
We don't need to call query-migrate every 50ms when we get the current
migration state via MIGRATION event.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch

 src/qemu/qemu_migration.c | 14 --
 src/qemu/qemu_process.c   | 31 +++
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index b79bbd1..f27f1ca 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2496,7 +2496,11 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
 qemuDomainObjPrivatePtr priv = vm->privateData;
 qemuDomainJobInfoPtr jobInfo = priv->job.current;
 
-if (qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0)
+bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
+
+if (events)
+qemuMigrationUpdateJobType(jobInfo);
+else if (qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0)
 return -1;
 
 switch (jobInfo->type) {
@@ -2515,9 +2519,15 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
qemuMigrationJobName(vm), _("canceled by client"));
 return -1;
 
+case VIR_DOMAIN_JOB_COMPLETED:
+/* Fetch statistics of a completed migration */
+if (events &&
+qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0)
+return -1;
+break;
+
 case VIR_DOMAIN_JOB_BOUNDED:
 case VIR_DOMAIN_JOB_UNBOUNDED:
-case VIR_DOMAIN_JOB_COMPLETED:
 case VIR_DOMAIN_JOB_LAST:
 break;
 }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 6d1ed28..0427e5d 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1508,6 +1508,36 @@ qemuProcessHandleSpiceMigrated(qemuMonitorPtr mon 
ATTRIBUTE_UNUSED,
 }
 
 
+static int
+qemuProcessHandleMigrationStatus(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm,
+ int status,
+ void *opaque ATTRIBUTE_UNUSED)
+{
+qemuDomainObjPrivatePtr priv;
+
+virObjectLock(vm);
+
+VIR_DEBUG("Migration of domain %p %s changed state to %s",
+  vm, vm->def->name,
+  qemuMonitorMigrationStatusTypeToString(status));
+
+priv = vm->privateData;
+if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT &&
+priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_IN) {
+VIR_DEBUG("got MIGRATION event without a migration job");
+goto cleanup;
+}
+
+priv->job.current->status.status = status;
+virDomainObjSignal(vm);
+
+ cleanup:
+virObjectUnlock(vm);
+return 0;
+}
+
+
 static qemuMonitorCallbacks monitorCallbacks = {
 .eofNotify = qemuProcessHandleMonitorEOF,
 .errorNotify = qemuProcessHandleMonitorError,
@@ -1532,6 +1562,7 @@ static qemuMonitorCallbacks monitorCallbacks = {
 .domainNicRxFilterChanged = qemuProcessHandleNicRxFilterChanged,
 .domainSerialChange = qemuProcessHandleSerialChanged,
 .domainSpiceMigrated = qemuProcessHandleSpiceMigrated,
+.domainMigrationStatus = qemuProcessHandleMigrationStatus,
 };
 
 static int
-- 
2.4.1

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

[libvirt] [PATCH v2 17/22] qemu_monitor: Wire up MIGRATION event

2015-06-02 Thread Jiri Denemark 
Thanks to Juan's work QEMU finally emits an event whenever migration
state changes.

Signed-off-by: Jiri Denemark 
---

Notes:
The MIGRATION event is not supported by QEMU yet, this patch is based
on the current patches available on qemu-devel mailing list. However,
there were no objections to the design of the event, which makes it
unlikely to change. Anyway this will have to wait until the patches
are applied to QEMU.

Version 2:
- new patch

 src/qemu/qemu_capabilities.c |  2 ++
 src/qemu/qemu_capabilities.h |  1 +
 src/qemu/qemu_monitor.c  | 14 ++
 src/qemu/qemu_monitor.h  |  8 
 src/qemu/qemu_monitor_json.c | 23 +++
 5 files changed, 48 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 960afa4..51f8a60 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -284,6 +284,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   "aes-key-wrap",
   "dea-key-wrap",
   "pci-serial",
+  "migration-event",
 );
 
 
@@ -1480,6 +1481,7 @@ struct virQEMUCapsStringFlags virQEMUCapsEvents[] = {
 { "BALLOON_CHANGE", QEMU_CAPS_BALLOON_EVENT },
 { "SPICE_MIGRATE_COMPLETED", QEMU_CAPS_SEAMLESS_MIGRATION },
 { "DEVICE_DELETED", QEMU_CAPS_DEVICE_DEL_EVENT },
+{ "MIGRATION", QEMU_CAPS_MIGRATION_EVENT },
 };
 
 struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 9c956f3..3f4638f 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -228,6 +228,7 @@ typedef enum {
 QEMU_CAPS_AES_KEY_WRAP   = 186, /* -machine aes_key_wrap */
 QEMU_CAPS_DEA_KEY_WRAP   = 187, /* -machine dea_key_wrap */
 QEMU_CAPS_DEVICE_PCI_SERIAL  = 188, /* -device pci-serial */
+QEMU_CAPS_MIGRATION_EVENT   = 189, /* MIGRATION event */
 
 QEMU_CAPS_LAST,   /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index a2a8f0d..00b8380 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1582,6 +1582,20 @@ qemuMonitorEmitSpiceMigrated(qemuMonitorPtr mon)
 
 
 int
+qemuMonitorEmitMigrationStatus(qemuMonitorPtr mon,
+   int status)
+{
+int ret = -1;
+VIR_DEBUG("mon=%p, status=%s",
+  mon, NULLSTR(qemuMonitorMigrationStatusTypeToString(status)));
+
+QEMU_MONITOR_CALLBACK(mon, ret, domainMigrationStatus, mon->vm, status);
+
+return ret;
+}
+
+
+int
 qemuMonitorSetCapabilities(qemuMonitorPtr mon)
 {
 QEMU_CHECK_MONITOR(mon);
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 17b7003..da74492 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -186,6 +186,11 @@ typedef int 
(*qemuMonitorDomainSpiceMigratedCallback)(qemuMonitorPtr mon,
   virDomainObjPtr vm,
   void *opaque);
 
+typedef int (*qemuMonitorDomainMigrationStatusCallback)(qemuMonitorPtr mon,
+virDomainObjPtr vm,
+int status,
+void *opaque);
+
 typedef struct _qemuMonitorCallbacks qemuMonitorCallbacks;
 typedef qemuMonitorCallbacks *qemuMonitorCallbacksPtr;
 struct _qemuMonitorCallbacks {
@@ -214,6 +219,7 @@ struct _qemuMonitorCallbacks {
 qemuMonitorDomainNicRxFilterChangedCallback domainNicRxFilterChanged;
 qemuMonitorDomainSerialChangeCallback domainSerialChange;
 qemuMonitorDomainSpiceMigratedCallback domainSpiceMigrated;
+qemuMonitorDomainMigrationStatusCallback domainMigrationStatus;
 };
 
 char *qemuMonitorEscapeArg(const char *in);
@@ -313,6 +319,8 @@ int qemuMonitorEmitSerialChange(qemuMonitorPtr mon,
 const char *devAlias,
 bool connected);
 int qemuMonitorEmitSpiceMigrated(qemuMonitorPtr mon);
+int qemuMonitorEmitMigrationStatus(qemuMonitorPtr mon,
+   int status);
 
 int qemuMonitorStartCPUs(qemuMonitorPtr mon,
  virConnectPtr conn);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index a138f1f..487039d 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -84,6 +84,7 @@ static void qemuMonitorJSONHandleDeviceDeleted(qemuMonitorPtr 
mon, virJSONValueP
 static void qemuMonitorJSONHandleNicRxFilterChanged(qemuMonitorPtr mon, 
virJSONValuePtr data);
 static void qemuMonitorJSONHandleSerialChange(qemuMonitorPtr mon, 
virJSONValuePtr data);
 static void qemuMonitorJSONHandleSpiceMigrated(qemuMonitorPtr mon, 
virJSONValuePtr data);
+static void qemuMonitorJSONHandleMigrationStatus(qemuMonitorPtr mon, 
virJSONValue

[libvirt] [PATCH v2 14/22] qemu: Refactor qemuMigrationUpdateJobStatus

2015-06-02 Thread Jiri Denemark 
Once we start waiting for migration events instead of polling
query-migrate, priv->job.current will not be regularly updated anymore
because we will get the current status directly from the events. Thus
virDomainGetJob{Info,Stats} will have to query QEMU, but they can't just
blindly update priv->job.current structure. This patch introduces
qemuMigrationFetchJobStatus which just fills in a caller supplied
structure and makes qemuMigrationUpdateJobStatus a tiny wrapper around
it.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch

 src/qemu/qemu_migration.c | 133 ++
 src/qemu/qemu_migration.h |   5 ++
 2 files changed, 93 insertions(+), 45 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 84a66d6..08ea706 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2401,67 +2401,110 @@ qemuMigrationWaitForSpice(virDomainObjPtr vm)
 return 0;
 }
 
-static int
-qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- const char *job,
- qemuDomainAsyncJob asyncJob)
+
+static void
+qemuMigrationUpdateJobType(qemuDomainJobInfoPtr jobInfo)
 {
-qemuDomainObjPrivatePtr priv = vm->privateData;
-qemuMonitorMigrationStatus status;
-qemuDomainJobInfoPtr jobInfo;
-int ret;
-
-memset(&status, 0, sizeof(status));
-
-ret = qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob);
-if (ret < 0) {
-/* Guest already exited or waiting for the job timed out; nothing
- * further to update. */
-return ret;
-}
-ret = qemuMonitorGetMigrationStatus(priv->mon, &status);
-
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
-return -1;
-
-if (ret < 0 ||
-qemuDomainJobInfoUpdateTime(priv->job.current) < 0)
-return -1;
-
-ret = -1;
-jobInfo = priv->job.current;
-switch (status.status) {
+switch (jobInfo->status.status) {
 case QEMU_MONITOR_MIGRATION_STATUS_COMPLETED:
 jobInfo->type = VIR_DOMAIN_JOB_COMPLETED;
-/* fall through */
-case QEMU_MONITOR_MIGRATION_STATUS_SETUP:
-case QEMU_MONITOR_MIGRATION_STATUS_ACTIVE:
-case QEMU_MONITOR_MIGRATION_STATUS_CANCELLING:
-ret = 0;
 break;
 
 case QEMU_MONITOR_MIGRATION_STATUS_INACTIVE:
 jobInfo->type = VIR_DOMAIN_JOB_NONE;
-virReportError(VIR_ERR_OPERATION_FAILED,
-   _("%s: %s"), job, _("is not active"));
 break;
 
 case QEMU_MONITOR_MIGRATION_STATUS_ERROR:
 jobInfo->type = VIR_DOMAIN_JOB_FAILED;
-virReportError(VIR_ERR_OPERATION_FAILED,
-   _("%s: %s"), job, _("unexpectedly failed"));
 break;
 
 case QEMU_MONITOR_MIGRATION_STATUS_CANCELLED:
 jobInfo->type = VIR_DOMAIN_JOB_CANCELLED;
+break;
+
+case QEMU_MONITOR_MIGRATION_STATUS_SETUP:
+case QEMU_MONITOR_MIGRATION_STATUS_ACTIVE:
+case QEMU_MONITOR_MIGRATION_STATUS_CANCELLING:
+break;
+}
+}
+
+
+int
+qemuMigrationFetchJobStatus(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+qemuDomainAsyncJob asyncJob,
+qemuDomainJobInfoPtr jobInfo)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+int rv;
+
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+return -1;
+
+memset(&jobInfo->status, 0, sizeof(jobInfo->status));
+rv = qemuMonitorGetMigrationStatus(priv->mon, &jobInfo->status);
+
+if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0)
+return -1;
+
+qemuMigrationUpdateJobType(jobInfo);
+return qemuDomainJobInfoUpdateTime(jobInfo);
+}
+
+
+static int
+qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuDomainAsyncJob asyncJob)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+qemuDomainJobInfoPtr jobInfo = priv->job.current;
+qemuDomainJobInfo newInfo = *jobInfo;
+
+if (qemuMigrationFetchJobStatus(driver, vm, asyncJob, &newInfo) < 0)
+return -1;
+
+*jobInfo = newInfo;
+return 0;
+}
+
+
+static int
+qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+const char *job,
+qemuDomainAsyncJob asyncJob)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+qemuDomainJobInfoPtr jobInfo = priv->job.current;
+
+if (qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0)
+return -1;
+
+switch (jobInfo->type) {
+case VIR_DOMAIN_JOB_NONE:
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("%s: %s"), job, _("is not active"));
+return -1;
+
+case VIR_DOMAIN_JOB_FAILED:
+virReportError(VIR_ERR_OPERATION_FAILED,
+

[libvirt] [PATCH v2 13/22] qemu: Refactor qemuDomainGetJob{Info, Stats}

2015-06-02 Thread Jiri Denemark 
Move common parts of qemuDomainGetJobInfo and qemuDomainGetJobStats into
a separate API (qemuDomainGetJobStatsInternal).

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch

 src/qemu/qemu_driver.c | 113 ++---
 1 file changed, 61 insertions(+), 52 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0544583..aa40ddd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13320,42 +13320,72 @@ qemuConnectBaselineCPU(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 }
 
 
-static int qemuDomainGetJobInfo(virDomainPtr dom,
-virDomainJobInfoPtr info)
+static int
+qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+  virDomainObjPtr vm,
+  bool completed,
+  qemuDomainJobInfoPtr jobInfo)
 {
+qemuDomainObjPrivatePtr priv = vm->privateData;
+qemuDomainJobInfoPtr info;
+int ret = -1;
+
+if (!completed &&
+!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("domain is not running"));
+goto cleanup;
+}
+
+if (completed)
+info = priv->job.completed;
+else
+info = priv->job.current;
+
+if (!info) {
+jobInfo->type = VIR_DOMAIN_JOB_NONE;
+ret = 0;
+goto cleanup;
+}
+*jobInfo = *info;
+
+if (jobInfo->type == VIR_DOMAIN_JOB_BOUNDED ||
+jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED)
+ret = qemuDomainJobInfoUpdateTime(jobInfo);
+else
+ret = 0;
+
+ cleanup:
+return ret;
+}
+
+
+static int
+qemuDomainGetJobInfo(virDomainPtr dom,
+ virDomainJobInfoPtr info)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+qemuDomainJobInfo jobInfo;
 virDomainObjPtr vm;
 int ret = -1;
-qemuDomainObjPrivatePtr priv;
 
 if (!(vm = qemuDomObjFromDomain(dom)))
 goto cleanup;
 
-priv = vm->privateData;
-
 if (virDomainGetJobInfoEnsureACL(dom->conn, vm->def) < 0)
 goto cleanup;
 
-if (virDomainObjIsActive(vm)) {
-if (priv->job.current) {
-/* Refresh elapsed time again just to ensure it
- * is fully updated. This is primarily for benefit
- * of incoming migration which we don't currently
- * monitor actively in the background thread
- */
-if (qemuDomainJobInfoUpdateTime(priv->job.current) < 0 ||
-qemuDomainJobInfoToInfo(priv->job.current, info) < 0)
-goto cleanup;
-} else {
-memset(info, 0, sizeof(*info));
-info->type = VIR_DOMAIN_JOB_NONE;
-}
-} else {
-virReportError(VIR_ERR_OPERATION_INVALID,
-   "%s", _("domain is not running"));
+if (qemuDomainGetJobStatsInternal(driver, vm, false, &jobInfo) < 0)
+goto cleanup;
+
+if (jobInfo.type == VIR_DOMAIN_JOB_NONE) {
+memset(info, 0, sizeof(*info));
+info->type = VIR_DOMAIN_JOB_NONE;
+ret = 0;
 goto cleanup;
 }
 
-ret = 0;
+ret = qemuDomainJobInfoToInfo(&jobInfo, info);
 
  cleanup:
 virDomainObjEndAPI(&vm);
@@ -13370,9 +13400,11 @@ qemuDomainGetJobStats(virDomainPtr dom,
   int *nparams,
   unsigned int flags)
 {
+virQEMUDriverPtr driver = dom->conn->privateData;
 virDomainObjPtr vm;
 qemuDomainObjPrivatePtr priv;
-qemuDomainJobInfoPtr jobInfo;
+qemuDomainJobInfo jobInfo;
+bool completed = !!(flags & VIR_DOMAIN_JOB_STATS_COMPLETED);
 int ret = -1;
 
 virCheckFlags(VIR_DOMAIN_JOB_STATS_COMPLETED, -1);
@@ -13380,24 +13412,14 @@ qemuDomainGetJobStats(virDomainPtr dom,
 if (!(vm = qemuDomObjFromDomain(dom)))
 goto cleanup;
 
-priv = vm->privateData;
-
 if (virDomainGetJobStatsEnsureACL(dom->conn, vm->def) < 0)
 goto cleanup;
 
-if (!(flags & VIR_DOMAIN_JOB_STATS_COMPLETED) &&
-!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_OPERATION_INVALID,
-   "%s", _("domain is not running"));
+priv = vm->privateData;
+if (qemuDomainGetJobStatsInternal(driver, vm, completed, &jobInfo) < 0)
 goto cleanup;
-}
 
-if (flags & VIR_DOMAIN_JOB_STATS_COMPLETED)
-jobInfo = priv->job.completed;
-else
-jobInfo = priv->job.current;
-
-if (!jobInfo) {
+if (jobInfo.type == VIR_DOMAIN_JOB_NONE) {
 *type = VIR_DOMAIN_JOB_NONE;
 *params = NULL;
 *nparams = 0;
@@ -13405,24 +13427,11 @@ qemuDomainGetJobStats(virDomainPtr dom,
 goto cleanup;
 }
 
-/* Refresh elapsed time again just to ensure it
- * is fully updated. This is primarily for benefit
- * of incoming migration which we don't currently
- * monitor actively in the background thread
- */
-if ((jobInf

[libvirt] [PATCH v2 20/22] qemuDomainGetJobStatsInternal: Support migration events

2015-06-02 Thread Jiri Denemark 
When QEMU supports migration events qemuDomainJobInfo structure is no
longer updated with migration statistics. We have to enter a job and
explicitly ask QEMU every time virDomainGetJob{Info,Stats} is called.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch

 src/qemu/qemu_driver.c | 27 +++
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index aa40ddd..a53840b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13321,15 +13321,27 @@ qemuConnectBaselineCPU(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 
 
 static int
-qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
   bool completed,
   qemuDomainJobInfoPtr jobInfo)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 qemuDomainJobInfoPtr info;
+bool fetch = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
 int ret = -1;
 
+if (completed)
+fetch = false;
+
+/* Do not ask QEMU if migration is not even running yet  */
+if (!priv->job.current || !priv->job.current->status.status)
+fetch = false;
+
+if (fetch &&
+qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+return -1;
+
 if (!completed &&
 !virDomainObjIsActive(vm)) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -13350,12 +13362,19 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
 *jobInfo = *info;
 
 if (jobInfo->type == VIR_DOMAIN_JOB_BOUNDED ||
-jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED)
-ret = qemuDomainJobInfoUpdateTime(jobInfo);
-else
+jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {
+if (fetch)
+ret = qemuMigrationFetchJobStatus(driver, vm, QEMU_ASYNC_JOB_NONE,
+  jobInfo);
+else
+ret = qemuDomainJobInfoUpdateTime(jobInfo);
+} else {
 ret = 0;
+}
 
  cleanup:
+if (fetch)
+qemuDomainObjEndJob(driver, vm);
 return ret;
 }
 
-- 
2.4.1

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

[libvirt] [PATCH v2 15/22] qemu: Don't pass redundant job name around

2015-06-02 Thread Jiri Denemark 
Instead of passing current job name to several functions which already
know what the current job is we can generate the name where we actually
need to use it.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch

 src/qemu/qemu_migration.c | 54 ---
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 08ea706..73121c7 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2453,6 +2453,24 @@ qemuMigrationFetchJobStatus(virQEMUDriverPtr driver,
 }
 
 
+static const char *
+qemuMigrationJobName(virDomainObjPtr vm)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+
+switch (priv->job.asyncJob) {
+case QEMU_ASYNC_JOB_MIGRATION_OUT:
+return _("migration job");
+case QEMU_ASYNC_JOB_SAVE:
+return _("domain save job");
+case QEMU_ASYNC_JOB_DUMP:
+return _("domain core dump job");
+default:
+return _("job");
+}
+}
+
+
 static int
 qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
@@ -2473,7 +2491,6 @@ qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
 static int
 qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
-const char *job,
 qemuDomainAsyncJob asyncJob)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -2484,18 +2501,18 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
 
 switch (jobInfo->type) {
 case VIR_DOMAIN_JOB_NONE:
-virReportError(VIR_ERR_OPERATION_FAILED,
-   _("%s: %s"), job, _("is not active"));
+virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"),
+   qemuMigrationJobName(vm), _("is not active"));
 return -1;
 
 case VIR_DOMAIN_JOB_FAILED:
-virReportError(VIR_ERR_OPERATION_FAILED,
-   _("%s: %s"), job, _("unexpectedly failed"));
+virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"),
+   qemuMigrationJobName(vm), _("unexpectedly failed"));
 return -1;
 
 case VIR_DOMAIN_JOB_CANCELLED:
-virReportError(VIR_ERR_OPERATION_ABORTED,
-   _("%s: %s"), job, _("canceled by client"));
+virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
+   qemuMigrationJobName(vm), _("canceled by client"));
 return -1;
 
 case VIR_DOMAIN_JOB_BOUNDED:
@@ -2521,31 +2538,16 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 qemuDomainJobInfoPtr jobInfo = priv->job.current;
-const char *job;
 int pauseReason;
 int ret = -1;
 
-switch (priv->job.asyncJob) {
-case QEMU_ASYNC_JOB_MIGRATION_OUT:
-job = _("migration job");
-break;
-case QEMU_ASYNC_JOB_SAVE:
-job = _("domain save job");
-break;
-case QEMU_ASYNC_JOB_DUMP:
-job = _("domain core dump job");
-break;
-default:
-job = _("job");
-}
-
 jobInfo->type = VIR_DOMAIN_JOB_UNBOUNDED;
 
 while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {
 /* Poll every 50ms for progress & to allow cancellation */
 struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
 
-if (qemuMigrationCheckJobStatus(driver, vm, job, asyncJob) < 0)
+if (qemuMigrationCheckJobStatus(driver, vm, asyncJob) < 0)
 goto error;
 
 if (storage &&
@@ -2556,8 +2558,8 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 if (abort_on_error &&
 virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED &&
 pauseReason == VIR_DOMAIN_PAUSED_IOERROR) {
-virReportError(VIR_ERR_OPERATION_FAILED,
-   _("%s: %s"), job, _("failed due to I/O error"));
+virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"),
+   qemuMigrationJobName(vm), _("failed due to I/O 
error"));
 goto error;
 }
 
@@ -4151,7 +4153,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
  * rather failed later on.  Check its status before waiting for a
  * connection from qemu which may never be initiated.
  */
-if (qemuMigrationCheckJobStatus(driver, vm, _("migration job"),
+if (qemuMigrationCheckJobStatus(driver, vm,
 QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
 goto cancel;
 
-- 
2.4.1

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

[libvirt] [PATCH v2 16/22] qemu: Refactor qemuMigrationWaitForCompletion

2015-06-02 Thread Jiri Denemark 
Checking status of all part of migration and aborting it when something
failed is a complex thing which makes the waiting loop hard to read.
This patch moves all the checks into a separate function similarly to
what was done for drive mirror loops.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch

 src/qemu/qemu_migration.c | 106 +-
 1 file changed, 66 insertions(+), 40 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 73121c7..b79bbd1 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2525,6 +2525,63 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
 }
 
 
+/**
+ * Returns 1 if migration completed successfully,
+ * 0 if the domain is still being migrated,
+ * -1 migration failed,
+ * -2 something else failed, we need to cancel migration.
+ */
+static int
+qemuMigrationCompleted(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   qemuDomainAsyncJob asyncJob,
+   virConnectPtr dconn,
+   bool abort_on_error,
+   bool storage)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+qemuDomainJobInfoPtr jobInfo = priv->job.current;
+int pauseReason;
+
+if (qemuMigrationCheckJobStatus(driver, vm, asyncJob) < 0)
+goto error;
+
+if (storage && qemuMigrationDriveMirrorReady(driver, vm) < 0)
+goto error;
+
+if (abort_on_error &&
+virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED &&
+pauseReason == VIR_DOMAIN_PAUSED_IOERROR) {
+virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"),
+   qemuMigrationJobName(vm), _("failed due to I/O error"));
+goto error;
+}
+
+if (dconn && virConnectIsAlive(dconn) <= 0) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("Lost connection to destination host"));
+goto error;
+}
+
+if (jobInfo->type == VIR_DOMAIN_JOB_COMPLETED)
+return 1;
+else
+return 0;
+
+ error:
+if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {
+/* The migration was aborted by us rather than QEMU itself. */
+jobInfo->type = VIR_DOMAIN_JOB_FAILED;
+return -2;
+} else if (jobInfo->type == VIR_DOMAIN_JOB_COMPLETED) {
+jobInfo->type = VIR_DOMAIN_JOB_FAILED;
+return -1;
+} else {
+return -1;
+}
+}
+
+
 /* Returns 0 on success, -2 when migration needs to be cancelled, or -1 when
  * QEMU reports failed migration.
  */
@@ -2538,59 +2595,28 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 qemuDomainJobInfoPtr jobInfo = priv->job.current;
-int pauseReason;
-int ret = -1;
+int rv;
 
 jobInfo->type = VIR_DOMAIN_JOB_UNBOUNDED;
-
-while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {
+while ((rv = qemuMigrationCompleted(driver, vm, asyncJob, dconn,
+abort_on_error, storage)) != 1) {
 /* Poll every 50ms for progress & to allow cancellation */
 struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
 
-if (qemuMigrationCheckJobStatus(driver, vm, asyncJob) < 0)
-goto error;
+if (rv < 0)
+return rv;
 
-if (storage &&
-qemuMigrationDriveMirrorReady(driver, vm) < 0)
-break;
-
-/* cancel migration if disk I/O error is emitted while migrating */
-if (abort_on_error &&
-virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED &&
-pauseReason == VIR_DOMAIN_PAUSED_IOERROR) {
-virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"),
-   qemuMigrationJobName(vm), _("failed due to I/O 
error"));
-goto error;
-}
-
-if (dconn && virConnectIsAlive(dconn) <= 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-   _("Lost connection to destination host"));
-goto error;
-}
-
-if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {
-virObjectUnlock(vm);
-nanosleep(&ts, NULL);
-virObjectLock(vm);
-}
+virObjectUnlock(vm);
+nanosleep(&ts, NULL);
+virObjectLock(vm);
 }
 
 qemuDomainJobInfoUpdateDowntime(jobInfo);
 VIR_FREE(priv->job.completed);
 if (VIR_ALLOC(priv->job.completed) == 0)
 *priv->job.completed = *jobInfo;
-return 0;
 
- error:
-/* Check if the migration was aborted by us rather than QEMU itself. */
-if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED ||
-jobInfo->type == VIR_DOMAIN_JOB_COMPLETED) {
-if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED)
-ret = -2;
-jobInfo->type = VIR_DOMAIN_JOB_FAILED;
-}
-return ret;
+re

[libvirt] [PATCH v2 07/22] qemu: Make qemuMigrationCancelDriveMirror usable without async job

2015-06-02 Thread Jiri Denemark 
We don't have an async job when reconnecting to existing domains after
libvirtd restart.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- no changes

 src/qemu/qemu_migration.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 61b3e34..fedadd7 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1837,7 +1837,8 @@ static int
 qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
   virDomainDiskDefPtr disk,
-  bool failNoJob)
+  bool failNoJob,
+  qemuDomainAsyncJob asyncJob)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 char *diskAlias = NULL;
@@ -1865,8 +1866,7 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver,
 QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0)
 return -1;
 
-if (qemuDomainObjEnterMonitorAsync(driver, vm,
-   QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
 goto cleanup;
 
 rv = qemuMonitorBlockJobCancel(priv->mon, diskAlias, true);
@@ -1897,7 +1897,8 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver,
 static int
 qemuMigrationCancelDriveMirror(virQEMUDriverPtr driver,
virDomainObjPtr vm,
-   bool check)
+   bool check,
+   qemuDomainAsyncJob asyncJob)
 {
 virErrorPtr err = NULL;
 int ret = -1;
@@ -1915,7 +1916,8 @@ qemuMigrationCancelDriveMirror(virQEMUDriverPtr driver,
 if (!diskPriv->migrating)
 continue;
 
-rv = qemuMigrationCancelOneDriveMirror(driver, vm, disk, check);
+rv = qemuMigrationCancelOneDriveMirror(driver, vm, disk,
+   check, asyncJob);
 if (rv != 0) {
 if (rv < 0) {
 if (!err)
@@ -3594,7 +3596,8 @@ qemuMigrationConfirmPhase(virQEMUDriverPtr driver,
 virErrorPtr orig_err = virSaveLastError();
 
 /* cancel any outstanding NBD jobs */
-qemuMigrationCancelDriveMirror(driver, vm, false);
+qemuMigrationCancelDriveMirror(driver, vm, false,
+   QEMU_ASYNC_JOB_MIGRATION_OUT);
 
 virSetError(orig_err);
 virFreeError(orig_err);
@@ -4165,7 +4168,8 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 
 /* cancel any outstanding NBD jobs */
 if (mig && mig->nbd) {
-if (qemuMigrationCancelDriveMirror(driver, vm, ret == 0) < 0)
+if (qemuMigrationCancelDriveMirror(driver, vm, ret == 0,
+   QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
 ret = -1;
 }
 
-- 
2.4.1

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

[libvirt] [PATCH v2 21/22] qemu: Wait for migration events on domain condition

2015-06-02 Thread Jiri Denemark 
Since we already support the MIGRATION event, we just need to make sure
the domain condition is signalled whenever a p2p connection drops or the
domain is paused due to IO error and we can avoid waking up every 50 ms
to check whether something happened.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch

 src/qemu/qemu_domain.h|  3 +++
 src/qemu/qemu_migration.c | 45 +++--
 src/qemu/qemu_process.c   |  3 +++
 3 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 55831a5..09bc0e1 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -199,6 +199,9 @@ struct _qemuDomainObjPrivate {
 /* Bitmaps below hold data from the auto NUMA feature */
 virBitmapPtr autoNodeset;
 virBitmapPtr autoCpuset;
+
+bool signalIOError; /* true if the domain condition should be signalled on
+   I/O error */
 };
 
 # define QEMU_DOMAIN_DISK_PRIVATE(disk)\
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index f27f1ca..f4edac8 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2605,20 +2605,28 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 qemuDomainJobInfoPtr jobInfo = priv->job.current;
+bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
 int rv;
 
 jobInfo->type = VIR_DOMAIN_JOB_UNBOUNDED;
 while ((rv = qemuMigrationCompleted(driver, vm, asyncJob, dconn,
 abort_on_error, storage)) != 1) {
-/* Poll every 50ms for progress & to allow cancellation */
-struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
-
 if (rv < 0)
 return rv;
 
-virObjectUnlock(vm);
-nanosleep(&ts, NULL);
-virObjectLock(vm);
+if (events) {
+if (virDomainObjWait(vm) < 0) {
+jobInfo->type = VIR_DOMAIN_JOB_FAILED;
+return -2;
+}
+} else {
+/* Poll every 50ms for progress & to allow cancellation */
+struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull 
};
+
+virObjectUnlock(vm);
+nanosleep(&ts, NULL);
+virObjectLock(vm);
+}
 }
 
 qemuDomainJobInfoUpdateDowntime(jobInfo);
@@ -4035,6 +4043,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 virErrorPtr orig_err = NULL;
 unsigned int cookieFlags = 0;
 bool abort_on_error = !!(flags & VIR_MIGRATE_ABORT_ON_ERROR);
+bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
 int rc;
 
 VIR_DEBUG("driver=%p, vm=%p, cookiein=%s, cookieinlen=%d, "
@@ -4064,6 +4073,9 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 return -1;
 }
 
+if (events)
+priv->signalIOError = abort_on_error;
+
 mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen,
  cookieFlags | QEMU_MIGRATION_COOKIE_GRAPHICS);
 if (!mig)
@@ -4269,6 +4281,9 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 
 qemuMigrationCookieFree(mig);
 
+if (events)
+priv->signalIOError = false;
+
 if (orig_err) {
 virSetError(orig_err);
 virFreeError(orig_err);
@@ -4893,6 +4908,18 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver,
 }
 
 
+static void
+qemuMigrationConnectionClosed(virConnectPtr conn,
+  int reason,
+  void *opaque)
+{
+virDomainObjPtr vm = opaque;
+
+VIR_DEBUG("conn=%p, reason=%d, vm=%s", conn, reason, vm->def->name);
+virDomainObjSignal(vm);
+}
+
+
 static int virConnectCredType[] = {
 VIR_CRED_AUTHNAME,
 VIR_CRED_PASSPHRASE,
@@ -4966,6 +4993,11 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver,
cfg->keepAliveCount) < 0)
 goto cleanup;
 
+if (virConnectRegisterCloseCallback(dconn, qemuMigrationConnectionClosed,
+vm, NULL) < 0) {
+goto cleanup;
+}
+
 qemuDomainObjEnterRemote(vm);
 p2p = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn,
VIR_DRV_FEATURE_MIGRATION_P2P);
@@ -5030,6 +5062,7 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver,
  cleanup:
 orig_err = virSaveLastError();
 qemuDomainObjEnterRemote(vm);
+virConnectUnregisterCloseCallback(dconn, qemuMigrationConnectionClosed);
 virObjectUnref(dconn);
 qemuDomainObjExitRemote(vm);
 if (orig_err) {
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 8575d90..99bc8b7 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -952,6 +952,9 @@ qemuProcessHandleIOError(qemuMonitorPtr mon 
ATTRIBUTE_UNUSED,
 qemuDomainObjPrivatePtr priv = vm->privateData;
 VIR_DEBUG("Transitioned guest

[libvirt] [PATCH v2 19/22] qemu: Work around weired migration status changes

2015-06-02 Thread Jiri Denemark 
When cancelling migration we can see the following conversation on QMP
monitor:

{"execute":"migrate_cancel","id":"libvirt-33"}
{"timestamp": {"seconds": 1432899178, "microseconds": 844907}, "event":
"MIGRATION", "data": {"status": "cancelling"}}
{"return": {}, "id": "libvirt-33"}
{"timestamp": {"seconds": 1432899178, "microseconds": 845625}, "event":
"MIGRATION", "data": {"status": "failed"}}
{"timestamp": {"seconds": 1432899178, "microseconds": 846432}, "event":
"MIGRATION", "data": {"status": "cancelled"}}

That is, migration status first changes to "failed" just to change to
the correct "cancelled" state in a few moments later. However, this is
enough to let libvirt report migration failed for unknown reason instead
of having been cancelled by a user.

This should really be fixed in QEMU but I'm not sure how easy it is.
However, it's pretty easy for us to work around it.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch

 src/qemu/qemu_process.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 0427e5d..8575d90 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1515,6 +1515,7 @@ qemuProcessHandleMigrationStatus(qemuMonitorPtr mon 
ATTRIBUTE_UNUSED,
  void *opaque ATTRIBUTE_UNUSED)
 {
 qemuDomainObjPrivatePtr priv;
+qemuDomainJobInfoPtr jobInfo;
 
 virObjectLock(vm);
 
@@ -1529,7 +1530,15 @@ qemuProcessHandleMigrationStatus(qemuMonitorPtr mon 
ATTRIBUTE_UNUSED,
 goto cleanup;
 }
 
-priv->job.current->status.status = status;
+jobInfo = priv->job.current;
+if (status == QEMU_MONITOR_MIGRATION_STATUS_ERROR &&
+jobInfo->status.status == QEMU_MONITOR_MIGRATION_STATUS_CANCELLING) {
+VIR_DEBUG("State changed from \"cancelling\" to \"failed\"; setting "
+  "current state to \"cancelled\"");
+status = QEMU_MONITOR_MIGRATION_STATUS_CANCELLED;
+}
+
+jobInfo->status.status = status;
 virDomainObjSignal(vm);
 
  cleanup:
-- 
2.4.1

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

[libvirt] [PATCH v2 22/22] qemu: cancel drive mirrors when p2p connection breaks

2015-06-02 Thread Jiri Denemark 
When a connection to the destination host during a p2p migration drops,
we know we will have to cancel the migration; it doesn't make sense to
waste resources by trying to finish the migration. We already do so
after sending "migrate" command to QEMU and we should do it while
waiting for drive mirrors to become ready too.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch

 src/qemu/qemu_migration.c | 30 --
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index f4edac8..fda775d 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1898,7 +1898,8 @@ static int
 qemuMigrationCancelDriveMirror(virQEMUDriverPtr driver,
virDomainObjPtr vm,
bool check,
-   qemuDomainAsyncJob asyncJob)
+   qemuDomainAsyncJob asyncJob,
+   virConnectPtr dconn)
 {
 virErrorPtr err = NULL;
 int ret = -1;
@@ -1931,8 +1932,16 @@ qemuMigrationCancelDriveMirror(virQEMUDriverPtr driver,
 
 while ((rv = qemuMigrationDriveMirrorCancelled(driver, vm,
failedPtr)) != 1) {
+if (check && !failed &&
+dconn && virConnectIsAlive(dconn) <= 0) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("Lost connection to destination host"));
+failed = true;
+}
+
 if (failed && !err)
 err = virSaveLastError();
+
 if (rv < 0 || virDomainObjWait(vm) < 0)
 goto cleanup;
 }
@@ -1974,7 +1983,8 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
  qemuMigrationCookiePtr mig,
  const char *host,
  unsigned long speed,
- unsigned int *migrate_flags)
+ unsigned int *migrate_flags,
+ virConnectPtr dconn)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 int ret = -1;
@@ -2054,6 +2064,12 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
 goto cleanup;
 }
 
+if (dconn && virConnectIsAlive(dconn) <= 0) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("Lost connection to destination host"));
+goto cleanup;
+}
+
 if (virDomainObjWait(vm) < 0)
 goto cleanup;
 }
@@ -3674,7 +3690,7 @@ qemuMigrationConfirmPhase(virQEMUDriverPtr driver,
 
 /* cancel any outstanding NBD jobs */
 qemuMigrationCancelDriveMirror(driver, vm, false,
-   QEMU_ASYNC_JOB_MIGRATION_OUT);
+   QEMU_ASYNC_JOB_MIGRATION_OUT, NULL);
 
 virSetError(orig_err);
 virFreeError(orig_err);
@@ -4091,7 +4107,8 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 if (qemuMigrationDriveMirror(driver, vm, mig,
  spec->dest.host.name,
  migrate_speed,
- &migrate_flags) < 0) {
+ &migrate_flags,
+ dconn) < 0) {
 goto cleanup;
 }
 } else {
@@ -4250,7 +4267,8 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 /* cancel any outstanding NBD jobs */
 if (mig && mig->nbd) {
 if (qemuMigrationCancelDriveMirror(driver, vm, ret == 0,
-   QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
+   QEMU_ASYNC_JOB_MIGRATION_OUT,
+   dconn) < 0)
 ret = -1;
 }
 
@@ -5838,7 +5856,7 @@ qemuMigrationCancel(virQEMUDriverPtr driver,
 }
 
 if (qemuMigrationCancelDriveMirror(driver, vm, false,
-   QEMU_ASYNC_JOB_NONE) < 0)
+   QEMU_ASYNC_JOB_NONE, NULL) < 0)
 goto endsyncjob;
 
 ret = 0;
-- 
2.4.1

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

[libvirt] [PATCH v2 05/22] qemu: Don't mess with disk->mirrorState

2015-06-02 Thread Jiri Denemark 
This patch reverts commit 76c61cdca20c106960af033e5d0f5da70177af0f.

VIR_DOMAIN_DISK_MIRROR_STATE_ABORT says we asked for a block job to be
aborted rather than saying it was aborted. Let's just use
VIR_DOMAIN_DISK_MIRROR_STATE_NONE consistently whenever a block job
finishes since no caller depends on VIR_DOMAIN_DISK_MIRROR_STATE_ABORT
(anymore) to check whether a block job failed or it was cancelled.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- no changes

 src/qemu/qemu_blockjob.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index cefb168..51370bf 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -164,8 +164,7 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
 case VIR_DOMAIN_BLOCK_JOB_CANCELED:
 virStorageSourceFree(disk->mirror);
 disk->mirror = NULL;
-disk->mirrorState = status == VIR_DOMAIN_BLOCK_JOB_FAILED ?
-VIR_DOMAIN_DISK_MIRROR_STATE_ABORT : 
VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
+disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
 disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
 save = true;
 diskPriv->blockjob = false;
-- 
2.4.1

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

[libvirt] [PATCH v2 04/22] qemu: Use domain condition for synchronous block jobs

2015-06-02 Thread Jiri Denemark 
By switching block jobs to use domain conditions, we can drop some
pretty complicated code in NBD storage migration. Moreover, we are
getting ready for migration events (to replace our 50ms polling on
query-migrate). The ultimate goal is to have a single loop waiting
(virDomainObjWait) for any migration related event (changed status of a
migration, disk mirror events, internal abort requests, ...). This patch
makes NBD storage migration ready for this: first we call a QMP command
to start or cancel drive mirror on all disks we are interested in and
then we wait for a single condition which is signaled on any event
related to any of the mirrors.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- slightly modified to use domain conditions

 po/POTFILES.in|   1 -
 src/qemu/qemu_blockjob.c  | 137 ++---
 src/qemu/qemu_blockjob.h  |  12 +-
 src/qemu/qemu_domain.c|  17 +--
 src/qemu/qemu_domain.h|   1 -
 src/qemu/qemu_driver.c|  24 ++--
 src/qemu/qemu_migration.c | 299 ++
 src/qemu/qemu_process.c   |  13 +-
 8 files changed, 197 insertions(+), 307 deletions(-)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index bb0f6e1..dd06ab3 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -112,7 +112,6 @@ src/parallels/parallels_utils.h
 src/parallels/parallels_storage.c
 src/phyp/phyp_driver.c
 src/qemu/qemu_agent.c
-src/qemu/qemu_blockjob.c
 src/qemu/qemu_capabilities.c
 src/qemu/qemu_cgroup.c
 src/qemu/qemu_command.c
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 605c2a5..cefb168 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -204,19 +204,17 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
  *
  * During a synchronous block job, a block job event for @disk
  * will not be processed asynchronously. Instead, it will be
- * processed only when qemuBlockJobSyncWait* or
- * qemuBlockJobSyncEnd is called.
+ * processed only when qemuBlockJobUpdate or qemuBlockJobSyncEnd
+ * is called.
  */
 void
 qemuBlockJobSyncBegin(virDomainDiskDefPtr disk)
 {
 qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
 
-if (diskPriv->blockJobSync)
-VIR_WARN("Disk %s already has synchronous block job",
- disk->dst);
-
+VIR_DEBUG("disk=%s", disk->dst);
 diskPriv->blockJobSync = true;
+diskPriv->blockJobStatus = -1;
 }
 
 
@@ -225,135 +223,16 @@ qemuBlockJobSyncBegin(virDomainDiskDefPtr disk)
  * @driver: qemu driver
  * @vm: domain
  * @disk: domain disk
- * @ret_status: pointer to virConnectDomainEventBlockJobStatus
  *
  * End a synchronous block job for @disk. Any pending block job event
- * for the disk is processed, and its status is recorded in the
- * virConnectDomainEventBlockJobStatus field pointed to by
- * @ret_status.
+ * for the disk is processed.
  */
 void
 qemuBlockJobSyncEnd(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
-virDomainDiskDefPtr disk,
-virConnectDomainEventBlockJobStatus *ret_status)
+virDomainDiskDefPtr disk)
 {
-qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
-
-if (diskPriv->blockJobSync && diskPriv->blockJobStatus != -1) {
-if (ret_status)
-*ret_status = diskPriv->blockJobStatus;
-qemuBlockJobUpdate(driver, vm, disk);
-diskPriv->blockJobStatus = -1;
-}
-diskPriv->blockJobSync = false;
-}
-
-
-/**
- * qemuBlockJobSyncWaitWithTimeout:
- * @driver: qemu driver
- * @vm: domain
- * @disk: domain disk
- * @timeout: timeout in milliseconds
- * @ret_status: pointer to virConnectDomainEventBlockJobStatus
- *
- * Wait up to @timeout milliseconds for a block job event for @disk.
- * If an event is received it is processed, and its status is recorded
- * in the virConnectDomainEventBlockJobStatus field pointed to by
- * @ret_status.
- *
- * If @timeout is not 0, @vm will be unlocked while waiting for the event.
- *
- * Returns 0 if an event was received or the timeout expired,
- *-1 otherwise.
- */
-int
-qemuBlockJobSyncWaitWithTimeout(virQEMUDriverPtr driver,
-virDomainObjPtr vm,
-virDomainDiskDefPtr disk,
-unsigned long long timeout,
-virConnectDomainEventBlockJobStatus 
*ret_status)
-{
-qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
-
-if (!diskPriv->blockJobSync) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("No current synchronous block job"));
-return -1;
-}
-
-while (diskPriv->blockJobSync && diskPriv->blockJobStatus == -1) {
-int r;
-
-if (!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("guest unexpectedly quit"));
-diskPriv->blockJobSync = false;
-

[libvirt] [PATCH v2 00/22] Add support for migration events

2015-06-02 Thread Jiri Denemark 
QEMU will soon (patches are available on qemu-devel) get support for
migration events which will finally allow us to get rid of polling
query-migrate every 50ms. However, we first need to be able to wait for
all events related to migration (migration status changes, block job
events, async abort requests) at once. This series prepares the
infrastructure and uses it to switch all polling loops in migration code
to pthread_cond_wait.

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

Version 2 (see individual patches for details):
- rewritten using per-domain condition variable
- enahnced to fully support the migration events

Jiri Denemark (22):
  conf: Introduce per-domain condition variable
  qemu: Introduce qemuBlockJobUpdate
  qemu: Properly report failed migration
  qemu: Use domain condition for synchronous block jobs
  qemu: Don't mess with disk->mirrorState
  Pass domain object to private data formatter/parser
  qemu: Make qemuMigrationCancelDriveMirror usable without async job
  qemu: Refactor qemuMonitorBlockJobInfo
  qemu: Cancel disk mirrors after libvirtd restart
  qemu: Use domain condition for asyncAbort
  qemu_monitor: Wire up SPICE_MIGRATE_COMPLETED event
  qemu: Do not poll for spice migration status
  qemu: Refactor qemuDomainGetJob{Info,Stats}
  qemu: Refactor qemuMigrationUpdateJobStatus
  qemu: Don't pass redundant job name around
  qemu: Refactor qemuMigrationWaitForCompletion
  qemu_monitor: Wire up MIGRATION event
  qemu: Update migration state according to MIGRATION event
  qemu: Work around weired migration status changes
  qemuDomainGetJobStatsInternal: Support migration events
  qemu: Wait for migration events on domain condition
  qemu: cancel drive mirrors when p2p connection breaks

 po/POTFILES.in   |   1 -
 src/conf/domain_conf.c   |  51 ++-
 src/conf/domain_conf.h   |  12 +-
 src/libvirt_private.syms |   6 +
 src/libxl/libxl_domain.c |  10 +-
 src/lxc/lxc_domain.c |  12 +-
 src/qemu/qemu_blockjob.c | 175 +++---
 src/qemu/qemu_blockjob.h |  15 +-
 src/qemu/qemu_capabilities.c |   2 +
 src/qemu/qemu_capabilities.h |   1 +
 src/qemu/qemu_domain.c   |  78 +++--
 src/qemu/qemu_domain.h   |   7 +-
 src/qemu/qemu_driver.c   | 201 +++-
 src/qemu/qemu_migration.c| 749 ---
 src/qemu/qemu_migration.h|   8 +
 src/qemu/qemu_monitor.c  |  73 -
 src/qemu/qemu_monitor.h  |  33 +-
 src/qemu/qemu_monitor_json.c | 152 -
 src/qemu/qemu_monitor_json.h |   7 +-
 src/qemu/qemu_process.c  |  92 +-
 tests/qemumonitorjsontest.c  |  40 ---
 21 files changed, 1032 insertions(+), 693 deletions(-)

-- 
2.4.1

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

[libvirt] [PATCH v2 09/22] qemu: Cancel disk mirrors after libvirtd restart

2015-06-02 Thread Jiri Denemark 
When libvirtd is restarted during migration, we properly cancel the
ongoing migration (unless it managed to almost finished before the
restart). But if we were also migrating storage using NBD, we would
completely forget about the running disk mirrors.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- make use of qemuMonitorGetAllBlockJobInfo introduced by the previous patch
- undo qemuBlockJobSyncBegin in case of error

 src/qemu/qemu_domain.c| 45 -
 src/qemu/qemu_migration.c | 85 +++
 src/qemu/qemu_migration.h |  3 ++
 src/qemu/qemu_process.c   |  8 +
 4 files changed, 133 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f6c9add..8cb4daa 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -578,7 +578,27 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
   qemuDomainAsyncJobPhaseToString(
 priv->job.asyncJob, priv->job.phase));
 }
-virBufferAddLit(buf, "/>\n");
+if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT) {
+virBufferAddLit(buf, "/>\n");
+} else {
+size_t i;
+virDomainDiskDefPtr disk;
+qemuDomainDiskPrivatePtr diskPriv;
+
+virBufferAddLit(buf, ">\n");
+virBufferAdjustIndent(buf, 2);
+
+for (i = 0; i < vm->def->ndisks; i++) {
+disk = vm->def->disks[i];
+diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+virBufferAsprintf(buf, "\n",
+  disk->dst,
+  diskPriv->migrating ? "yes" : "no");
+}
+
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+}
 }
 priv->job.active = job;
 
@@ -736,6 +756,29 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
 }
 }
 
+if ((n = virXPathNodeSet("./job[1]/disk[@migrating='yes']",
+ ctxt, &nodes)) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("failed to parse list of disks marked for 
migration"));
+goto error;
+}
+if (n > 0) {
+if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT) {
+VIR_WARN("Found disks marked for migration but we were not "
+ "migrating");
+n = 0;
+}
+for (i = 0; i < n; i++) {
+char *dst = virXMLPropString(nodes[i], "dev");
+virDomainDiskDefPtr disk;
+
+if (dst && (disk = virDomainDiskByName(vm->def, dst, false)))
+QEMU_DOMAIN_DISK_PRIVATE(disk)->migrating = true;
+VIR_FREE(dst);
+}
+}
+VIR_FREE(nodes);
+
 priv->fakeReboot = virXPathBoolean("boolean(./fakereboot)", ctxt) == 1;
 
 if ((n = virXPathNodeSet("./devices/device", ctxt, &nodes)) < 0) {
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index fedadd7..f6b9aa0 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1985,6 +1985,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
 char *hoststr = NULL;
 unsigned int mirror_flags = VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT;
 int rv;
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 
 VIR_DEBUG("Starting drive mirrors for domain %s", vm->def->name);
 
@@ -2034,6 +2035,11 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
 goto cleanup;
 }
 diskPriv->migrating = true;
+
+if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) {
+VIR_WARN("Failed to save status on vm %s", vm->def->name);
+goto cleanup;
+}
 }
 
 while ((rv = qemuMigrationDriveMirrorReady(driver, vm)) != 1) {
@@ -2061,6 +2067,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
 ret = 0;
 
  cleanup:
+virObjectUnref(cfg);
 VIR_FREE(diskAlias);
 VIR_FREE(nbd_dest);
 VIR_FREE(hoststr);
@@ -5683,6 +5690,84 @@ qemuMigrationToFile(virQEMUDriverPtr driver, 
virDomainObjPtr vm,
 return ret;
 }
 
+
+int
+qemuMigrationCancel(virQEMUDriverPtr driver,
+virDomainObjPtr vm)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+virHashTablePtr blockJobs = NULL;
+bool storage = false;
+size_t i;
+int ret = -1;
+
+VIR_DEBUG("Canceling unfinished outgoing migration of domain %s",
+  vm->def->name);
+
+for (i = 0; i < vm->def->ndisks; i++) {
+virDomainDiskDefPtr disk = vm->def->disks[i];
+if (QEMU_DOMAIN_DISK_PRIVATE(disk)->migrating) {
+qemuBlockJobSyncBegin(disk);
+storage = true;
+}
+}
+
+qemuDomainObjEnterMonitor(driver, vm);
+
+ignore_value(qemuMonitorMigrateCancel(priv->mon));
+if (storage)
+blockJobs = qemuMonitorGetAllBlockJobInfo(

[libvirt] [PATCH v2 02/22] qemu: Introduce qemuBlockJobUpdate

2015-06-02 Thread Jiri Denemark 
The wrapper is useful for calling qemuBlockJobEventProcess with the
event details stored in disk's privateData, which is the most likely
usage of qemuBlockJobEventProcess.

Signed-off-by: Jiri Denemark 
---

Notes:
Already ACKed in version 1.

Version 2:
- no changes

 src/libvirt_private.syms |  2 ++
 src/qemu/qemu_blockjob.c | 37 +
 src/qemu/qemu_blockjob.h |  3 +++
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9076135..8846dea 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -265,6 +265,8 @@ virDomainDiskInsert;
 virDomainDiskInsertPreAlloced;
 virDomainDiskIoTypeFromString;
 virDomainDiskIoTypeToString;
+virDomainDiskMirrorStateTypeFromString;
+virDomainDiskMirrorStateTypeToString;
 virDomainDiskPathByName;
 virDomainDiskRemove;
 virDomainDiskRemoveByName;
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 098a43a..605c2a5 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -38,6 +38,27 @@
 
 VIR_LOG_INIT("qemu.qemu_blockjob");
 
+
+int
+qemuBlockJobUpdate(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   virDomainDiskDefPtr disk)
+{
+qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+int ret;
+
+if ((ret = diskPriv->blockJobStatus) == -1)
+return -1;
+
+qemuBlockJobEventProcess(driver, vm, disk,
+ diskPriv->blockJobType,
+ diskPriv->blockJobStatus);
+diskPriv->blockJobStatus = -1;
+
+return ret;
+}
+
+
 /**
  * qemuBlockJobEventProcess:
  * @driver: qemu driver
@@ -49,8 +70,6 @@ VIR_LOG_INIT("qemu.qemu_blockjob");
  * Update disk's mirror state in response to a block job event
  * from QEMU. For mirror state's that must survive libvirt
  * restart, also update the domain's status XML.
- *
- * Returns 0 on success, -1 otherwise.
  */
 void
 qemuBlockJobEventProcess(virQEMUDriverPtr driver,
@@ -67,6 +86,12 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
 bool save = false;
 qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
 
+VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, status=%d",
+  disk->dst,
+  NULLSTR(virDomainDiskMirrorStateTypeToString(disk->mirrorState)),
+  type,
+  status);
+
 /* Have to generate two variants of the event for old vs. new
  * client callbacks */
 if (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT &&
@@ -218,9 +243,7 @@ qemuBlockJobSyncEnd(virQEMUDriverPtr driver,
 if (diskPriv->blockJobSync && diskPriv->blockJobStatus != -1) {
 if (ret_status)
 *ret_status = diskPriv->blockJobStatus;
-qemuBlockJobEventProcess(driver, vm, disk,
- diskPriv->blockJobType,
- diskPriv->blockJobStatus);
+qemuBlockJobUpdate(driver, vm, disk);
 diskPriv->blockJobStatus = -1;
 }
 diskPriv->blockJobSync = false;
@@ -300,9 +323,7 @@ qemuBlockJobSyncWaitWithTimeout(virQEMUDriverPtr driver,
 
 if (ret_status)
 *ret_status = diskPriv->blockJobStatus;
-qemuBlockJobEventProcess(driver, vm, disk,
- diskPriv->blockJobType,
- diskPriv->blockJobStatus);
+qemuBlockJobUpdate(driver, vm, disk);
 diskPriv->blockJobStatus = -1;
 
 return 0;
diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
index ba372a2..81e893e 100644
--- a/src/qemu/qemu_blockjob.h
+++ b/src/qemu/qemu_blockjob.h
@@ -25,6 +25,9 @@
 # include "internal.h"
 # include "qemu_conf.h"
 
+int qemuBlockJobUpdate(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   virDomainDiskDefPtr disk);
 void qemuBlockJobEventProcess(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
   virDomainDiskDefPtr disk,
-- 
2.4.1

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

[libvirt] [PATCH v2 11/22] qemu_monitor: Wire up SPICE_MIGRATE_COMPLETED event

2015-06-02 Thread Jiri Denemark 
Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch

 src/qemu/qemu_monitor.c  | 12 
 src/qemu/qemu_monitor.h  |  6 ++
 src/qemu/qemu_monitor_json.c | 10 ++
 3 files changed, 28 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index f612077..15f7852 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1570,6 +1570,18 @@ qemuMonitorEmitSerialChange(qemuMonitorPtr mon,
 
 
 int
+qemuMonitorEmitSpiceMigrated(qemuMonitorPtr mon)
+{
+int ret = -1;
+VIR_DEBUG("mon=%p", mon);
+
+QEMU_MONITOR_CALLBACK(mon, ret, domainSpiceMigrated, mon->vm);
+
+return ret;
+}
+
+
+int
 qemuMonitorSetCapabilities(qemuMonitorPtr mon)
 {
 QEMU_CHECK_MONITOR(mon);
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 0d9e413..86da7ad 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -182,6 +182,10 @@ typedef int 
(*qemuMonitorDomainSerialChangeCallback)(qemuMonitorPtr mon,
  bool connected,
  void *opaque);
 
+typedef int (*qemuMonitorDomainSpiceMigratedCallback)(qemuMonitorPtr mon,
+  virDomainObjPtr vm,
+  void *opaque);
+
 typedef struct _qemuMonitorCallbacks qemuMonitorCallbacks;
 typedef qemuMonitorCallbacks *qemuMonitorCallbacksPtr;
 struct _qemuMonitorCallbacks {
@@ -209,6 +213,7 @@ struct _qemuMonitorCallbacks {
 qemuMonitorDomainDeviceDeletedCallback domainDeviceDeleted;
 qemuMonitorDomainNicRxFilterChangedCallback domainNicRxFilterChanged;
 qemuMonitorDomainSerialChangeCallback domainSerialChange;
+qemuMonitorDomainSpiceMigratedCallback domainSpiceMigrated;
 };
 
 char *qemuMonitorEscapeArg(const char *in);
@@ -307,6 +312,7 @@ int qemuMonitorEmitNicRxFilterChanged(qemuMonitorPtr mon,
 int qemuMonitorEmitSerialChange(qemuMonitorPtr mon,
 const char *devAlias,
 bool connected);
+int qemuMonitorEmitSpiceMigrated(qemuMonitorPtr mon);
 
 int qemuMonitorStartCPUs(qemuMonitorPtr mon,
  virConnectPtr conn);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 1b92610..32e4100 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -83,6 +83,7 @@ static void qemuMonitorJSONHandleGuestPanic(qemuMonitorPtr 
mon, virJSONValuePtr
 static void qemuMonitorJSONHandleDeviceDeleted(qemuMonitorPtr mon, 
virJSONValuePtr data);
 static void qemuMonitorJSONHandleNicRxFilterChanged(qemuMonitorPtr mon, 
virJSONValuePtr data);
 static void qemuMonitorJSONHandleSerialChange(qemuMonitorPtr mon, 
virJSONValuePtr data);
+static void qemuMonitorJSONHandleSpiceMigrated(qemuMonitorPtr mon, 
virJSONValuePtr data);
 
 typedef struct {
 const char *type;
@@ -107,6 +108,7 @@ static qemuEventHandler eventHandlers[] = {
 { "SPICE_CONNECTED", qemuMonitorJSONHandleSPICEConnect, },
 { "SPICE_DISCONNECTED", qemuMonitorJSONHandleSPICEDisconnect, },
 { "SPICE_INITIALIZED", qemuMonitorJSONHandleSPICEInitialize, },
+{ "SPICE_MIGRATE_COMPLETED", qemuMonitorJSONHandleSpiceMigrated, },
 { "STOP", qemuMonitorJSONHandleStop, },
 { "SUSPEND", qemuMonitorJSONHandlePMSuspend, },
 { "SUSPEND_DISK", qemuMonitorJSONHandlePMSuspendDisk, },
@@ -914,6 +916,14 @@ qemuMonitorJSONHandleSerialChange(qemuMonitorPtr mon,
 }
 
 
+static void
+qemuMonitorJSONHandleSpiceMigrated(qemuMonitorPtr mon,
+   virJSONValuePtr data ATTRIBUTE_UNUSED)
+{
+qemuMonitorEmitSpiceMigrated(mon);
+}
+
+
 int
 qemuMonitorJSONHumanCommandWithFd(qemuMonitorPtr mon,
   const char *cmd_str,
-- 
2.4.1

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

[libvirt] [PATCH v2 08/22] qemu: Refactor qemuMonitorBlockJobInfo

2015-06-02 Thread Jiri Denemark 
"query-block-jobs" QMP command returns all running block jobs at once,
while qemuMonitorBlockJobInfo would only report one. This is not very
nice in case we need to check several block jobs. This patch refactors
the monitor code to always parse all block jobs and store them in a
hash.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch

 src/qemu/qemu_driver.c   | 45 +++-
 src/qemu/qemu_monitor.c  | 37 ++-
 src/qemu/qemu_monitor.h  | 17 ---
 src/qemu/qemu_monitor_json.c | 70 ++--
 src/qemu/qemu_monitor_json.h |  7 ++---
 5 files changed, 104 insertions(+), 72 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0a57c20..0544583 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16408,7 +16408,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
 {
 int ret = -1, rc;
 qemuDomainObjPrivatePtr priv = vm->privateData;
-virDomainBlockJobInfo info;
+qemuMonitorBlockJobInfo info;
 virStorageSourcePtr oldsrc = NULL;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 
@@ -16422,7 +16422,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
 /* Probe the status, if needed.  */
 if (!disk->mirrorState) {
 qemuDomainObjEnterMonitor(driver, vm);
-rc = qemuMonitorBlockJobInfo(priv->mon, device, &info, NULL);
+rc = qemuMonitorGetBlockJobInfo(priv->mon, disk->info.alias, &info);
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 goto cleanup;
 if (rc < 0)
@@ -16755,16 +16755,16 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
 
 
 static int
-qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path,
-   virDomainBlockJobInfoPtr info, unsigned int flags)
+qemuDomainGetBlockJobInfo(virDomainPtr dom,
+  const char *path,
+  virDomainBlockJobInfoPtr info,
+  unsigned int flags)
 {
 virQEMUDriverPtr driver = dom->conn->privateData;
 virDomainObjPtr vm;
-char *device = NULL;
-int idx;
 virDomainDiskDefPtr disk;
 int ret = -1;
-unsigned long long bandwidth;
+qemuMonitorBlockJobInfo rawInfo;
 
 virCheckFlags(VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES, -1);
 
@@ -16787,31 +16787,34 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const 
char *path,
 if (qemuDomainSupportsBlockJobs(vm, NULL) < 0)
 goto endjob;
 
-if (!(device = qemuDiskPathToAlias(vm, path, &idx)))
+if (!(disk = virDomainDiskByName(vm->def, path, true)))
 goto endjob;
-disk = vm->def->disks[idx];
 
 qemuDomainObjEnterMonitor(driver, vm);
-ret = qemuMonitorBlockJobInfo(qemuDomainGetMonitor(vm), device, info,
-  &bandwidth);
+ret = qemuMonitorGetBlockJobInfo(qemuDomainGetMonitor(vm),
+ disk->info.alias, &rawInfo);
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 ret = -1;
 if (ret < 0)
 goto endjob;
 
+info->cur = rawInfo.cur;
+info->end = rawInfo.end;
+
+info->type = rawInfo.type;
 if (info->type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT &&
 disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT)
 info->type = disk->mirrorJob;
-if (bandwidth) {
-if (!(flags & VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES))
-bandwidth = VIR_DIV_UP(bandwidth, 1024 * 1024);
-info->bandwidth = bandwidth;
-if (info->bandwidth != bandwidth) {
-virReportError(VIR_ERR_OVERFLOW,
-   _("bandwidth %llu cannot be represented in result"),
-   bandwidth);
-goto endjob;
-}
+
+if (rawInfo.bandwidth &&
+!(flags & VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES))
+rawInfo.bandwidth = VIR_DIV_UP(rawInfo.bandwidth, 1024 * 1024);
+info->bandwidth = rawInfo.bandwidth;
+if (info->bandwidth != rawInfo.bandwidth) {
+virReportError(VIR_ERR_OVERFLOW,
+   _("bandwidth %llu cannot be represented in result"),
+   rawInfo.bandwidth);
+goto endjob;
 }
 
 /* Snoop block copy operations, so future cancel operations can
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index f959b74..f612077 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3291,17 +3291,40 @@ qemuMonitorBlockJobSetSpeed(qemuMonitorPtr mon,
 }
 
 
+virHashTablePtr
+qemuMonitorGetAllBlockJobInfo(qemuMonitorPtr mon)
+{
+QEMU_CHECK_MONITOR_JSON_NULL(mon);
+return qemuMonitorJSONGetAllBlockJobInfo(mon);
+}
+
+
+/**
+ * qemuMonitorGetBlockJobInfo:
+ * Parse Block Job information, and populate info for the named device.
+ * Return 1 if info available, 0 if device has no block job, and -1 on error.
+ */
 int
-qemuMonitorBlockJobInfo(qemuMonitorPtr mon,
-

[libvirt] [PATCH v2 01/22] conf: Introduce per-domain condition variable

2015-06-02 Thread Jiri Denemark 
Complex jobs, such as migration, need to monitor several events at once,
which is impossible when each of the event uses its own condition
variable. This patch adds a single condition variable to each domain
object. This variable can be used instead of the other event specific
conditions.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch which replaces thread queues and conditions (patch 1
  and 2 in version 1)

 src/conf/domain_conf.c   | 47 +++
 src/conf/domain_conf.h   |  6 ++
 src/libvirt_private.syms |  4 
 3 files changed, 57 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6e57425..0850b9b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2508,6 +2508,7 @@ static void virDomainObjDispose(void *obj)
 virDomainObjPtr dom = obj;
 
 VIR_DEBUG("obj=%p", dom);
+virCondDestroy(&dom->cond);
 virDomainDefFree(dom->def);
 virDomainDefFree(dom->newDef);
 
@@ -2528,6 +2529,12 @@ virDomainObjNew(virDomainXMLOptionPtr xmlopt)
 if (!(domain = virObjectLockableNew(virDomainObjClass)))
 return NULL;
 
+if (virCondInit(&domain->cond) < 0) {
+virReportSystemError(errno, "%s",
+ _("failed to initialize domain condition"));
+goto error;
+}
+
 if (xmlopt->privateData.alloc) {
 if (!(domain->privateData = (xmlopt->privateData.alloc)()))
 goto error;
@@ -2650,6 +2657,46 @@ virDomainObjEndAPI(virDomainObjPtr *vm)
 }
 
 
+void
+virDomainObjSignal(virDomainObjPtr vm)
+{
+virCondSignal(&vm->cond);
+}
+
+
+void
+virDomainObjBroadcast(virDomainObjPtr vm)
+{
+virCondBroadcast(&vm->cond);
+}
+
+
+int
+virDomainObjWait(virDomainObjPtr vm)
+{
+if (virCondWait(&vm->cond, &vm->parent.lock) < 0) {
+virReportSystemError(errno, "%s",
+ _("failed to wait for domain condition"));
+return -1;
+}
+return 0;
+}
+
+
+int
+virDomainObjWaitUntil(virDomainObjPtr vm,
+  unsigned long long whenms)
+{
+if (virCondWaitUntil(&vm->cond, &vm->parent.lock, whenms) < 0 &&
+errno != ETIMEDOUT) {
+virReportSystemError(errno, "%s",
+ _("failed to wait for domain condition"));
+return -1;
+}
+return 0;
+}
+
+
 /*
  *
  * If flags & VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE then
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0fcf52e..1039f78 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2305,6 +2305,7 @@ typedef struct _virDomainObj virDomainObj;
 typedef virDomainObj *virDomainObjPtr;
 struct _virDomainObj {
 virObjectLockable parent;
+virCond cond;
 
 pid_t pid;
 virDomainStateReason state;
@@ -2424,6 +2425,11 @@ void virDomainObjEndAPI(virDomainObjPtr *vm);
 bool virDomainObjTaint(virDomainObjPtr obj,
virDomainTaintFlags taint);
 
+void virDomainObjSignal(virDomainObjPtr vm);
+void virDomainObjBroadcast(virDomainObjPtr vm);
+int virDomainObjWait(virDomainObjPtr vm);
+int virDomainObjWaitUntil(virDomainObjPtr vm,
+  unsigned long long whenms);
 
 int virDomainDefCheckUnsupportedMemoryHotplug(virDomainDefPtr def);
 int virDomainDeviceDefCheckUnsupportedMemoryDevice(virDomainDeviceDefPtr dev);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6a95fb9..9076135 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -382,6 +382,7 @@ virDomainNetTypeToString;
 virDomainNostateReasonTypeFromString;
 virDomainNostateReasonTypeToString;
 virDomainObjAssignDef;
+virDomainObjBroadcast;
 virDomainObjCopyPersistentDef;
 virDomainObjEndAPI;
 virDomainObjFormat;
@@ -409,7 +410,10 @@ virDomainObjParseNode;
 virDomainObjSetDefTransient;
 virDomainObjSetMetadata;
 virDomainObjSetState;
+virDomainObjSignal;
 virDomainObjTaint;
+virDomainObjWait;
+virDomainObjWaitUntil;
 virDomainOSTypeFromString;
 virDomainOSTypeToString;
 virDomainParseMemory;
-- 
2.4.1

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

[libvirt] [PATCH v2 10/22] qemu: Use domain condition for asyncAbort

2015-06-02 Thread Jiri Denemark 
To avoid polling for asyncAbort flag changes.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- rewritten using domain condition

 src/qemu/qemu_domain.c|  5 +++--
 src/qemu/qemu_domain.h|  2 +-
 src/qemu/qemu_migration.c | 11 ---
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8cb4daa..a1ce988 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -169,7 +169,7 @@ qemuDomainObjResetAsyncJob(qemuDomainObjPrivatePtr priv)
 job->phase = 0;
 job->mask = QEMU_JOB_DEFAULT_MASK;
 job->dump_memory_only = false;
-job->asyncAbort = false;
+job->abortJob = false;
 VIR_FREE(job->current);
 }
 
@@ -1652,7 +1652,8 @@ qemuDomainObjAbortAsyncJob(virDomainObjPtr obj)
   qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
   obj, obj->def->name);
 
-priv->job.asyncAbort = true;
+priv->job.abortJob = true;
+virDomainObjBroadcast(obj);
 }
 
 /*
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 7f06cb8..7479f70 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -135,7 +135,7 @@ struct qemuDomainJobObj {
 bool dump_memory_only;  /* use dump-guest-memory to do dump */
 qemuDomainJobInfoPtr current;   /* async job progress data */
 qemuDomainJobInfoPtr completed; /* statistics data of a recently 
completed job */
-bool asyncAbort;/* abort of async job requested */
+bool abortJob;  /* abort of the job requested */
 };
 
 typedef void (*qemuDomainCleanupCallback)(virQEMUDriverPtr driver,
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index f6b9aa0..74c2657 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2043,12 +2043,10 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
 }
 
 while ((rv = qemuMigrationDriveMirrorReady(driver, vm)) != 1) {
-unsigned long long now;
-
 if (rv < 0)
 goto cleanup;
 
-if (priv->job.asyncAbort) {
+if (priv->job.abortJob) {
 priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED;
 virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
@@ -2056,8 +2054,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
 goto cleanup;
 }
 
-if (virTimeMillisNow(&now) < 0 ||
-virDomainObjWaitUntil(vm, now + 500) < 0)
+if (virDomainObjWait(vm) < 0)
 goto cleanup;
 }
 
@@ -4054,10 +4051,10 @@ qemuMigrationRun(virQEMUDriverPtr driver,
QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
 goto cleanup;
 
-if (priv->job.asyncAbort) {
+if (priv->job.abortJob) {
 /* explicitly do this *after* we entered the monitor,
  * as this is a critical section so we are guaranteed
- * priv->job.asyncAbort will not change */
+ * priv->job.abortJob will not change */
 ignore_value(qemuDomainObjExitMonitor(driver, vm));
 priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED;
 virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
-- 
2.4.1

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

[libvirt] [PATCH v2 03/22] qemu: Properly report failed migration

2015-06-02 Thread Jiri Denemark 
Because we are polling we may detect some errors after we asked QEMU for
migration status even though they occurred before. If this happens and
QEMU reports migration completed successfully, we would happily report
the migration succeeded even though we should have cancelled it because
of the other error.

In practise it is not a big issue now but it will become a much bigger
issue once the check for storage migration status is moved inside the
loop in qemuMigrationWaitForCompletion.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- really do what commit message describes

Already ACKed in version 1 :-)

 src/qemu/qemu_migration.c | 48 +++
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index f7432e8..93e29e7 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2442,6 +2442,7 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 qemuDomainJobInfoPtr jobInfo = priv->job.current;
 const char *job;
 int pauseReason;
+int ret = -1;
 
 switch (priv->job.asyncJob) {
 case QEMU_ASYNC_JOB_MIGRATION_OUT:
@@ -2459,12 +2460,12 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 
 jobInfo->type = VIR_DOMAIN_JOB_UNBOUNDED;
 
-while (1) {
+while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {
 /* Poll every 50ms for progress & to allow cancellation */
 struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
 
 if (qemuMigrationUpdateJobStatus(driver, vm, job, asyncJob) == -1)
-break;
+goto error;
 
 /* cancel migration if disk I/O error is emitted while migrating */
 if (abort_on_error &&
@@ -2472,40 +2473,37 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 pauseReason == VIR_DOMAIN_PAUSED_IOERROR) {
 virReportError(VIR_ERR_OPERATION_FAILED,
_("%s: %s"), job, _("failed due to I/O error"));
-break;
+goto error;
 }
 
 if (dconn && virConnectIsAlive(dconn) <= 0) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Lost connection to destination host"));
-break;
+goto error;
 }
 
-if (jobInfo->type != VIR_DOMAIN_JOB_UNBOUNDED)
-break;
-
-virObjectUnlock(vm);
-
-nanosleep(&ts, NULL);
-
-virObjectLock(vm);
+if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {
+virObjectUnlock(vm);
+nanosleep(&ts, NULL);
+virObjectLock(vm);
+}
 }
 
-if (jobInfo->type == VIR_DOMAIN_JOB_COMPLETED) {
-qemuDomainJobInfoUpdateDowntime(jobInfo);
-VIR_FREE(priv->job.completed);
-if (VIR_ALLOC(priv->job.completed) == 0)
-*priv->job.completed = *jobInfo;
-return 0;
-} else if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {
-/* The migration was aborted by us rather than QEMU itself so let's
- * update the job type and notify the caller to send migrate_cancel.
- */
+qemuDomainJobInfoUpdateDowntime(jobInfo);
+VIR_FREE(priv->job.completed);
+if (VIR_ALLOC(priv->job.completed) == 0)
+*priv->job.completed = *jobInfo;
+return 0;
+
+ error:
+/* Check if the migration was aborted by us rather than QEMU itself. */
+if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED ||
+jobInfo->type == VIR_DOMAIN_JOB_COMPLETED) {
+if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED)
+ret = -2;
 jobInfo->type = VIR_DOMAIN_JOB_FAILED;
-return -2;
-} else {
-return -1;
 }
+return ret;
 }
 
 
-- 
2.4.1

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

[libvirt] [PATCH v2 06/22] Pass domain object to private data formatter/parser

2015-06-02 Thread Jiri Denemark 
So that they can format private data (e.g., disk private data) stored
elsewhere in the domain object.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- no changes

 src/conf/domain_conf.c   |  4 ++--
 src/conf/domain_conf.h   |  6 --
 src/libxl/libxl_domain.c | 10 ++
 src/lxc/lxc_domain.c | 12 
 src/qemu/qemu_domain.c   | 10 ++
 5 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0850b9b..cf0f54b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15900,7 +15900,7 @@ virDomainObjParseXML(xmlDocPtr xml,
 VIR_FREE(nodes);
 
 if (xmlopt->privateData.parse &&
-((xmlopt->privateData.parse)(ctxt, obj->privateData)) < 0)
+xmlopt->privateData.parse(ctxt, obj) < 0)
 goto error;
 
 return obj;
@@ -21828,7 +21828,7 @@ virDomainObjFormat(virDomainXMLOptionPtr xmlopt,
 }
 
 if (xmlopt->privateData.format &&
-((xmlopt->privateData.format)(&buf, obj->privateData)) < 0)
+xmlopt->privateData.format(&buf, obj) < 0)
 goto error;
 
 if (virDomainDefFormatInternal(obj->def, flags, &buf) < 0)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 1039f78..05a8884 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2344,8 +2344,10 @@ typedef virDomainXMLOption *virDomainXMLOptionPtr;
 typedef void *(*virDomainXMLPrivateDataAllocFunc)(void);
 typedef void (*virDomainXMLPrivateDataFreeFunc)(void *);
 typedef virObjectPtr (*virDomainXMLPrivateDataNewFunc)(void);
-typedef int (*virDomainXMLPrivateDataFormatFunc)(virBufferPtr, void *);
-typedef int (*virDomainXMLPrivateDataParseFunc)(xmlXPathContextPtr, void *);
+typedef int (*virDomainXMLPrivateDataFormatFunc)(virBufferPtr,
+ virDomainObjPtr);
+typedef int (*virDomainXMLPrivateDataParseFunc)(xmlXPathContextPtr,
+virDomainObjPtr);
 
 /* Called once after everything else has been parsed, for adjusting
  * overall domain defaults.  */
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index c7f0ed9..d38ed61 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -223,9 +223,10 @@ libxlDomainObjPrivateFree(void *data)
 }
 
 static int
-libxlDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data)
+libxlDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
+  virDomainObjPtr vm)
 {
-libxlDomainObjPrivatePtr priv = data;
+libxlDomainObjPrivatePtr priv = vm->privateData;
 
 priv->lockState = virXPathString("string(./lockstate)", ctxt);
 
@@ -233,9 +234,10 @@ libxlDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, 
void *data)
 }
 
 static int
-libxlDomainObjPrivateXMLFormat(virBufferPtr buf, void *data)
+libxlDomainObjPrivateXMLFormat(virBufferPtr buf,
+   virDomainObjPtr vm)
 {
-libxlDomainObjPrivatePtr priv = data;
+libxlDomainObjPrivatePtr priv = vm->privateData;
 
 if (priv->lockState)
 virBufferAsprintf(buf, "%s\n", priv->lockState);
diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
index c2180cb..70606f3 100644
--- a/src/lxc/lxc_domain.c
+++ b/src/lxc/lxc_domain.c
@@ -51,9 +51,11 @@ static void virLXCDomainObjPrivateFree(void *data)
 }
 
 
-static int virLXCDomainObjPrivateXMLFormat(virBufferPtr buf, void *data)
+static int
+virLXCDomainObjPrivateXMLFormat(virBufferPtr buf,
+virDomainObjPtr vm)
 {
-virLXCDomainObjPrivatePtr priv = data;
+virLXCDomainObjPrivatePtr priv = vm->privateData;
 
 virBufferAsprintf(buf, "\n",
   (unsigned long long)priv->initpid);
@@ -61,9 +63,11 @@ static int virLXCDomainObjPrivateXMLFormat(virBufferPtr buf, 
void *data)
 return 0;
 }
 
-static int virLXCDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data)
+static int
+virLXCDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
+   virDomainObjPtr vm)
 {
-virLXCDomainObjPrivatePtr priv = data;
+virLXCDomainObjPrivatePtr priv = vm->privateData;
 unsigned long long thepid;
 
 if (virXPathULongLong("string(./init[1]/@pid)", ctxt, &thepid) < 0) {
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 144a968..f6c9add 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -511,9 +511,10 @@ qemuDomainObjPrivateFree(void *data)
 
 
 static int
-qemuDomainObjPrivateXMLFormat(virBufferPtr buf, void *data)
+qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
+  virDomainObjPtr vm)
 {
-qemuDomainObjPrivatePtr priv = data;
+qemuDomainObjPrivatePtr priv = vm->privateData;
 const char *monitorpath;
 qemuDomainJob job;
 
@@ -600,9 +601,10 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, void *data)
 }
 
 static int
-qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data)
+qemuDomainOb

[libvirt] [PATCH v2 12/22] qemu: Do not poll for spice migration status

2015-06-02 Thread Jiri Denemark 
QEMU_CAPS_SEAMLESS_MIGRATION capability says QEMU supports
SPICE_MIGRATE_COMPLETED event. Thus we can just drop all code which
polls query-spice and replace it with waiting for the event.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch

 src/qemu/qemu_domain.c   |  1 +
 src/qemu/qemu_domain.h   |  1 +
 src/qemu/qemu_migration.c| 38 ++
 src/qemu/qemu_monitor.c  | 10 -
 src/qemu/qemu_monitor.h  |  2 --
 src/qemu/qemu_monitor_json.c | 49 
 src/qemu/qemu_process.c  | 28 +
 tests/qemumonitorjsontest.c  | 40 
 8 files changed, 41 insertions(+), 128 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a1ce988..fe92a17 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -170,6 +170,7 @@ qemuDomainObjResetAsyncJob(qemuDomainObjPrivatePtr priv)
 job->mask = QEMU_JOB_DEFAULT_MASK;
 job->dump_memory_only = false;
 job->abortJob = false;
+job->spiceMigrated = false;
 VIR_FREE(job->current);
 }
 
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 7479f70..55831a5 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -136,6 +136,7 @@ struct qemuDomainJobObj {
 qemuDomainJobInfoPtr current;   /* async job progress data */
 qemuDomainJobInfoPtr completed; /* statistics data of a recently 
completed job */
 bool abortJob;  /* abort of the job requested */
+bool spiceMigrated; /* spice migration completed */
 };
 
 typedef void (*qemuDomainCleanupCallback)(virQEMUDriverPtr driver,
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 74c2657..84a66d6 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2375,45 +2375,29 @@ qemuMigrationSetPinAll(virQEMUDriverPtr driver,
 }
 
 static int
-qemuMigrationWaitForSpice(virQEMUDriverPtr driver,
-  virDomainObjPtr vm)
+qemuMigrationWaitForSpice(virDomainObjPtr vm)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 bool wait_for_spice = false;
-bool spice_migrated = false;
 size_t i = 0;
-int rc;
 
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) {
-for (i = 0; i < vm->def->ngraphics; i++) {
-if (vm->def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
-wait_for_spice = true;
-break;
-}
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION))
+return 0;
+
+for (i = 0; i < vm->def->ngraphics; i++) {
+if (vm->def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
+wait_for_spice = true;
+break;
 }
 }
 
 if (!wait_for_spice)
 return 0;
 
-while (!spice_migrated) {
-/* Poll every 50ms for progress & to allow cancellation */
-struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
-
-if (qemuDomainObjEnterMonitorAsync(driver, vm,
-   QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
+while (!priv->job.spiceMigrated && !priv->job.abortJob) {
+if (virDomainObjWait(vm) < 0)
 return -1;
-
-rc = qemuMonitorGetSpiceMigrationStatus(priv->mon, &spice_migrated);
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
-return -1;
-if (rc < 0)
-return -1;
-virObjectUnlock(vm);
-nanosleep(&ts, NULL);
-virObjectLock(vm);
 }
-
 return 0;
 }
 
@@ -3587,7 +3571,7 @@ qemuMigrationConfirmPhase(virQEMUDriverPtr driver,
 if (retcode == 0) {
 /* If guest uses SPICE and supports seamless migration we have to hold
  * up domain shutdown until SPICE server transfers its data */
-qemuMigrationWaitForSpice(driver, vm);
+qemuMigrationWaitForSpice(vm);
 
 qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED,
 VIR_QEMU_PROCESS_STOP_MIGRATED);
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 15f7852..a2a8f0d 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2201,16 +2201,6 @@ qemuMonitorGetMigrationStatus(qemuMonitorPtr mon,
 
 
 int
-qemuMonitorGetSpiceMigrationStatus(qemuMonitorPtr mon,
-   bool *spice_migrated)
-{
-QEMU_CHECK_MONITOR_JSON(mon);
-
-return qemuMonitorJSONGetSpiceMigrationStatus(mon, spice_migrated);
-}
-
-
-int
 qemuMonitorMigrateToFd(qemuMonitorPtr mon,
unsigned int flags,
int fd)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 86da7ad..17b7003 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -498,8 +498,6 @@ struct _qemuMonitorMigrationStatus {
 
 int qemuMonitorGetMigrationStatus(qemuMonitorPtr mon

Re: [libvirt] Bug 1227257

2015-06-02 Thread Cedric Bosdonnat 
王松波你好,

Thanks a lot for reporting a bug and providing a patch for it. However,
you will need to send the patch using git send-email to this mailing
list: it will ease the review and integration in master.

Thanks a lot for your help,

--
Cedric

On Tue, 2015-06-02 at 20:27 +0800, 王松波 wrote:
> I report a bug [Bug 1227257] . In the environment
>  libvirt-1.2.16.tar.gz + qemu-img version 2.1.2 + ceph version 0.94.1.
> libvirt pool will become inactive after one client does vol.delete and
> the other does pool.refresh in the same pool simultaneously.
> 
> 
> The reason is that rbd_list and  rbd_open are not wrapped in an atomic
> operation, but two seperate operations.
> For example, two clients are operating in the same pool at the same
> time. One client does rbd_list, and got 10 rbd images, meanwhile, the
> other client deletes one of the rbd image. In this situation, when the
> first client does next operation, such as rbd_open , the command may
> fail, because the rbd image has been removed.
> 
> 
> I write a testcase in python to reproduce this problem as follow(also
> have put it in the attachment):
> ##
> import libvirt
> import sys
> import time
> import sys
> #coding:utf-8
> 
> 
> QEMU_URL = 'qemu:///system'
> 
> 
> VOL_TEMPLATE='''
> 
> {vol}
> {pool}/{vol}
> 
> 
> {cap_size}
> {alloc_size}
> 
> rbd:{pool}/{vol}
> 
> 
> 00
> 0
> 0
> 
> 
> '''
> 
> 
> def create_vol(pool_name, vol_name, cap_size, alloc_size):
> conn = libvirt.open(QEMU_URL)
> if conn == None:
> print 'Failed to open connection to the hypervisor'
> sys.exit(1)
> 
> 
> try:
> pool = conn.storagePoolLookupByName(pool_name)
> pool.refresh()
> template = VOL_TEMPLATE.format(pool=pool_name, vol=vol_name,
> cap_size=cap_size, alloc_size=alloc_size)
> pool.createXML(template, 0)
> except:
> print 'Failed to open pool'
> sys.exit(1)
> finally:
> if conn is not None:
> conn.close()
> 
> 
> def destroy_vol(pool_name, vol_name):
> conn = libvirt.open(QEMU_URL)
> if conn == None:
> sys.exit(1)
> pool = conn.storagePoolLookupByName(pool_name)
> pool.refresh(0) 
> vol = pool.storageVolLookupByName(vol_name)
> if vol is not None:
> vol.delete(0)
> if conn is not None:
>  conn.close()
> 
> 
> if  sys.argv[2] == 'create':
> for i in range(1, 20):
> volname = 'pool-down-test-%s-%d' % (sys.argv[1], i)
> print 'create %s' % volname
> create_vol('capacity', volname, '500', '500')
> elif sys.argv[2] == 'destroy':
> for i in range(1, 20):
> volname = 'pool-down-test-%s-%d' % (sys.argv[1], i)
> print 'destroy %s' % volname
> destroy_vol('capacity', volname)
> else:
> print 'Usage: python vol-test.py clientId OPER'
> print '  '
> print 'where  clientId : a num/string used to distinguish
> different client'
> print '   OPER : create/destroy'
> 
> 
> 
> 
> 
> 
> 
> 
> Patch for this is as fllow:
> 
> 
> diff --git a/src/storage/storage_backend_rbd.c
> b/src/storage/storage_backend_rbd.c
> index ae4bcb3..24fbc84 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -266,6 +266,46 @@ static int
> virStorageBackendRBDCloseRADOSConn(virStorageBackendRBDStatePtr ptr)
>  return ret;
>  }
> 
> 
> +static int volStorageBackendRBDVolIsExist(char *volname,
> virStorageBackendRBDStatePtr ptr)
> +{
> +int ret = -1;
> +char *name, *names = NULL;
> +size_t max_size = 1024;
> +int len = -1;
> +
> +while (true) {
> +if (VIR_ALLOC_N(names, max_size) < 0)
> +goto cleanup;
> +
> +len = rbd_list(ptr->ioctx, names, &max_size);
> +if (len >= 0)
> +break;
> +if (len != -ERANGE) {
> +VIR_WARN("%s", _("A problem occurred while listing RBD
> images"));
> +goto cleanup;
> +}
> +VIR_FREE(names);
> +}
> +
> +for (name = names; name < names + max_size;) {
> +
> +if (STREQ(name, ""))
> +break;
> +
> +name += strlen(name) + 1;
> +if (STREQ(volname, name)) {
> +VIR_ERROR("RBD images '%s' is exist, but cannot open it",
> volname);
> +ret = -2;
> +break;
> +}
> +}
> +ret = 0;
> +
> +cleanup:
> +VIR_FREE(names);
> +return ret;
> +}
> +
>  static int volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr
> vol,
>virStoragePoolObjPtr
> pool,
> 
>  virStorageBackendRBDStatePtr ptr)
> @@ -276,8 +316,15 @@ static int
> volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
> 
> 
>  r = rbd_open(ptr->ioctx, vol->name, &image, NULL);
>  if (r < 0) {
> -virReportSystemError(-r, _("failed to open the RBD image '%
> s'"),
> -

Re: [libvirt] [PATCH v2 19/22] qemu: Work around weired migration status changes

2015-06-02 Thread Ján Tomko 
s/weired/weird/ in the commit message

Jan

On Tue, Jun 02, 2015 at 02:34:24PM +0200, Jiri Denemark wrote:
> When cancelling migration we can see the following conversation on QMP
> monitor:
> 
> {"execute":"migrate_cancel","id":"libvirt-33"}
> {"timestamp": {"seconds": 1432899178, "microseconds": 844907}, "event":
> "MIGRATION", "data": {"status": "cancelling"}}
> {"return": {}, "id": "libvirt-33"}
> {"timestamp": {"seconds": 1432899178, "microseconds": 845625}, "event":
> "MIGRATION", "data": {"status": "failed"}}
> {"timestamp": {"seconds": 1432899178, "microseconds": 846432}, "event":
> "MIGRATION", "data": {"status": "cancelled"}}
> 
> That is, migration status first changes to "failed" just to change to
> the correct "cancelled" state in a few moments later. However, this is
> enough to let libvirt report migration failed for unknown reason instead
> of having been cancelled by a user.
> 
> This should really be fixed in QEMU but I'm not sure how easy it is.
> However, it's pretty easy for us to work around it.
> 
> Signed-off-by: Jiri Denemark 
> ---


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 2/2] storage: RBD: do not return error when deleting non-existent volume

2015-06-02 Thread Martin Kletzander 

On Thu, May 28, 2015 at 05:29:55PM +0200, Erik Skultety wrote:

RBD API returns negative value of errno, in that case we can silently
ignore if RBD tries to delete a non-existent volume, just like FS
backend does.
---
src/storage/storage_backend_rbd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/storage/storage_backend_rbd.c 
b/src/storage/storage_backend_rbd.c
index ae4bcb3..8e8d7a7 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -1,7 +1,7 @@
/*
 * storage_backend_rbd.c: storage backend for RBD (RADOS Block Device) handling
 *
- * Copyright (C) 2013-2014 Red Hat, Inc.
+ * Copyright (C) 2013-2015 Red Hat, Inc.
 * Copyright (C) 2012 Wido den Hollander


I wonder how come nobody complained in reply to this patch already...


 *
 * This library is free software; you can redistribute it and/or
@@ -426,7 +426,7 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
VIR_DEBUG("Removing RBD image %s/%s", pool->def->source.name, vol->name);

if (flags & VIR_STORAGE_VOL_DELETE_ZEROED)
-VIR_WARN("%s", _("This storage backend does not supported zeroed removal of 
volumes"));
+VIR_WARN("%s", _("This storage backend does not support zeroed removal of 
volumes"));



This could be another trivial patch, but it's *so* trivial, I'm OK
with keeping it here.


if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source) < 0)
goto cleanup;
@@ -435,7 +435,7 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
goto cleanup;

r = rbd_remove(ptr.ioctx, vol->name);
-if (r < 0) {
+if (r < 0 && (-r) != ENOENT) {


This makes sense.  ACK.


virReportSystemError(-r, _("failed to remove volume '%s/%s'"),
 pool->def->source.name, vol->name);
goto cleanup;
--
1.9.3

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


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

[libvirt] [PATCH 2/2] Simplify virNodeCountThreadSiblings

2015-06-02 Thread Ján Tomko 
Use a for cycle instead of while.

Do not opencode c_isxdigit and virHexToBin.
---
 src/nodeinfo.c | 12 +++-
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 9db3233..2fafe2d 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -361,15 +361,9 @@ virNodeCountThreadSiblings(const char *dir, unsigned int 
cpu)
 if (virFileReadAll(path, SYSFS_THREAD_SIBLINGS_LIST_LENGTH_MAX, &str) < 0)
 goto cleanup;
 
-i = 0;
-while (str[i] != '\0') {
-if (c_isdigit(str[i]))
-ret += count_one_bits(str[i] - '0');
-else if (str[i] >= 'A' && str[i] <= 'F')
-ret += count_one_bits(str[i] - 'A' + 10);
-else if (str[i] >= 'a' && str[i] <= 'f')
-ret += count_one_bits(str[i] - 'a' + 10);
-i++;
+for (i = 0; str[i] != '\0'; i++) {
+if (c_isxdigit(str[i]))
+ret += count_one_bits(virHexToBin(str[i]));
 }
 
  cleanup:
-- 
2.3.6

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

[libvirt] [PATCH 0/2] Report errors in virNodeCountThreadSiblings

2015-06-02 Thread Ján Tomko 
https://bugzilla.redhat.com/show_bug.cgi?id=1207849

Ján Tomko (2):
  Report errors in virNodeCountThreadSiblings
  Simplify virNodeCountThreadSiblings

 src/nodeinfo.c | 34 +++---
 1 file changed, 7 insertions(+), 27 deletions(-)

-- 
2.3.6

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

[libvirt] [PATCH 1/2] Report errors in virNodeCountThreadSiblings

2015-06-02 Thread Ján Tomko 
Use virFileReadAll which reports an error when the file is larger
than the specified maximum.

https://bugzilla.redhat.com/show_bug.cgi?id=1207849
---
 src/nodeinfo.c | 22 --
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 29f1aa7..9db3233 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -344,7 +344,6 @@ virNodeCountThreadSiblings(const char *dir, unsigned int 
cpu)
 {
 unsigned long ret = 0;
 char *path;
-FILE *pathfp;
 char *str = NULL;
 size_t i;
 
@@ -352,27 +351,16 @@ virNodeCountThreadSiblings(const char *dir, unsigned int 
cpu)
 dir, cpu) < 0)
 return 0;
 
-pathfp = fopen(path, "r");
-if (pathfp == NULL) {
+if (!virFileExists(path)) {
 /* If file doesn't exist, then pretend our only
  * sibling is ourself */
-if (errno == ENOENT) {
-VIR_FREE(path);
-return 1;
-}
-virReportSystemError(errno, _("cannot open %s"), path);
-VIR_FREE(path);
-return 0;
+ret = 1;
+goto cleanup;
 }
 
-if (VIR_ALLOC_N(str, SYSFS_THREAD_SIBLINGS_LIST_LENGTH_MAX) < 0)
+if (virFileReadAll(path, SYSFS_THREAD_SIBLINGS_LIST_LENGTH_MAX, &str) < 0)
 goto cleanup;
 
-if (fgets(str, SYSFS_THREAD_SIBLINGS_LIST_LENGTH_MAX, pathfp) == NULL) {
-virReportSystemError(errno, _("cannot read from %s"), path);
-goto cleanup;
-}
-
 i = 0;
 while (str[i] != '\0') {
 if (c_isdigit(str[i]))
@@ -386,9 +374,7 @@ virNodeCountThreadSiblings(const char *dir, unsigned int 
cpu)
 
  cleanup:
 VIR_FREE(str);
-VIR_FORCE_FCLOSE(pathfp);
 VIR_FREE(path);
-
 return ret;
 }
 
-- 
2.3.6

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

Re: [libvirt] [PATCH v2 2/2] storage: RBD: do not return error when deleting non-existent volume

2015-06-02 Thread Erik Skultety 


On 06/02/2015 02:47 PM, Martin Kletzander wrote:
> On Thu, May 28, 2015 at 05:29:55PM +0200, Erik Skultety wrote:
>> RBD API returns negative value of errno, in that case we can silently
>> ignore if RBD tries to delete a non-existent volume, just like FS
>> backend does.
>> ---
>> src/storage/storage_backend_rbd.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_rbd.c
>> b/src/storage/storage_backend_rbd.c
>> index ae4bcb3..8e8d7a7 100644
>> --- a/src/storage/storage_backend_rbd.c
>> +++ b/src/storage/storage_backend_rbd.c
>> @@ -1,7 +1,7 @@
>> /*
>>  * storage_backend_rbd.c: storage backend for RBD (RADOS Block Device)
>> handling
>>  *
>> - * Copyright (C) 2013-2014 Red Hat, Inc.
>> + * Copyright (C) 2013-2015 Red Hat, Inc.
>>  * Copyright (C) 2012 Wido den Hollander
> 
> I wonder how come nobody complained in reply to this patch already...
> 
>>  *
>>  * This library is free software; you can redistribute it and/or
>> @@ -426,7 +426,7 @@ static int
>> virStorageBackendRBDDeleteVol(virConnectPtr conn,
>> VIR_DEBUG("Removing RBD image %s/%s", pool->def->source.name,
>> vol->name);
>>
>> if (flags & VIR_STORAGE_VOL_DELETE_ZEROED)
>> -VIR_WARN("%s", _("This storage backend does not supported
>> zeroed removal of volumes"));
>> +VIR_WARN("%s", _("This storage backend does not support
>> zeroed removal of volumes"));
>>
> 
> This could be another trivial patch, but it's *so* trivial, I'm OK
> with keeping it here.
> 
>> if (virStorageBackendRBDOpenRADOSConn(&ptr, conn,
>> &pool->def->source) < 0)
>> goto cleanup;
>> @@ -435,7 +435,7 @@ static int
>> virStorageBackendRBDDeleteVol(virConnectPtr conn,
>> goto cleanup;
>>
>> r = rbd_remove(ptr.ioctx, vol->name);
>> -if (r < 0) {
>> +if (r < 0 && (-r) != ENOENT) {
> 
> This makes sense.  ACK.
> 
>> virReportSystemError(-r, _("failed to remove volume '%s/%s'"),
>>  pool->def->source.name, vol->name);
>> goto cleanup;
>> -- 
>> 1.9.3
>>
>> -- 
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
> 
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

Thank you for review :), both are now pushed.
Erik

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

Re: [libvirt] [PATCH v4 0/6] virsh: Further improve handling of integer options

2015-06-02 Thread John Ferlan 


On 06/02/2015 05:17 AM, Andrea Bolognani wrote:
> As suggested by Michal: now that we have a generic error message for
> failures related to the parsing of integer options, it makes sense to
> perform the corresponding check in a single spot instead of replicating
> it every time vshCommandOpt*() is used.
> 
> Andrea Bolognani (6):
>   tests: Add a bunch of new tests to virsh-optparse
>   virsh: Use standard error messages in vshCommandOptTimeoutToMs()
>   virsh: Improve vshCommandOptTimeoutToMs()
>   virsh: Make vshCommandOptScaledInt() use vshCommandOpt()
>   virsh: Pass vshControl to all vshCommandOpt*() calls
>   virsh: Move error messages inside vshCommandOpt*() functions
> 
>  tests/vcpupin|   4 +-
>  tests/virsh-optparse | 179 ++
>  tools/virsh-domain-monitor.c |  17 +--
>  tools/virsh-domain.c | 226 --
>  tools/virsh-host.c   |  67 +++-
>  tools/virsh-interface.c  |   6 +-
>  tools/virsh-network.c|  10 +-
>  tools/virsh-nodedev.c|   4 +-
>  tools/virsh-snapshot.c   |   2 +-
>  tools/virsh-volume.c |  26 +
>  tools/virsh.c| 252 
> ++-
>  tools/virsh.h|  66 ++--
>  12 files changed, 461 insertions(+), 398 deletions(-)
> 

ACK series... and pushed


John

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

Re: [libvirt] [PATCH 0/2] Report errors in virNodeCountThreadSiblings

2015-06-02 Thread Martin Kletzander 

On Tue, Jun 02, 2015 at 02:53:43PM +0200, Ján Tomko wrote:

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

Ján Tomko (2):
 Report errors in virNodeCountThreadSiblings
 Simplify virNodeCountThreadSiblings

src/nodeinfo.c | 34 +++---
1 file changed, 7 insertions(+), 27 deletions(-)



ACK series


--
2.3.6

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


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

Re: [libvirt] [PATCH v2 1/2] storage: Don't update volume objs list before we successfully create one

2015-06-02 Thread Martin Kletzander 

On Mon, Jun 01, 2015 at 03:14:24PM +0200, Martin Kletzander wrote:

On Thu, May 28, 2015 at 05:29:54PM +0200, Erik Skultety wrote:

We do update pool volume object list before we actually create any
volume. If buildVol fails, we then try to delete the volume in the
storage as well as remove it from our structures. The problem is, that
any backend that supports both buildVol and deleteVol would fail in this
case which is completely unnecessary. This patch causes the update to
take place after we know a volume has been created successfully, thus no
removal in case of a buildVol failure is necessary.

https://bugzilla.redhat.com/show_bug.cgi?id=1223177
---
src/storage/storage_driver.c | 31 +--
1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index ac4a74a..25a9656 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1,7 +1,7 @@
/*
* storage_driver.c: core driver for storage APIs
*
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2015 Red Hat, Inc.
* Copyright (C) 2006-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -1812,9 +1812,6 @@ 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,
@@ -1832,14 +1829,6 @@ 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;
@@ -1868,14 +1857,20 @@ storageVolCreateXML(virStoragePoolPtr obj,
   voldef->building = false;
   pool->asyncjobs--;

-if (buildret < 0) {
-VIR_FREE(buildvoldef);
-storageVolDeleteInternal(volobj, backend, pool, voldef,
- 0, false);
-voldef = NULL;
+if (buildret < 0)
   goto cleanup;
-}
+}
+
+if (VIR_REALLOC_N(pool->volumes.objs,
+  pool->volumes.count+1) < 0)
+goto cleanup;

+pool->volumes.objs[pool->volumes.count++] = voldef;
+volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name,
+  voldef->key, NULL, NULL);


I know it's pre-existing, but if you switch these two lines, you don't
have to do this unnecessary complicated cleanup (i.e. two lines) here.


+if (!volobj) {
+pool->volumes.count--;
+goto cleanup;


Also, if this fails, you need to delete the the volume anyway,
otherwise you have data inconsistency again.  How about doing this:



Actually, you don't.  My head just gave up on my here.  But it would
be nice to keep the definition in the list and not decrement the count
there with 'pool->volumes.count--'.  We would return OOM error, but
apart from that everything would be consistent.

So ACK with that one line removed.


VIR_REALLOC_N(pool->volumes.objs, ...);
virGetStorageVol(...);
buildVol();

This way the last thing that can fail is the one you don't want to
revert, but because it is the last one already, you don't need to
revert it (adding the definition to the list and increasing the count
can't fail).


   }

   if (backend->refreshVol &&
--
1.9.3

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





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


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

Re: [libvirt] [PATCH 2/2] Simplify virNodeCountThreadSiblings

2015-06-02 Thread Erik Skultety 


On 06/02/2015 02:53 PM, Ján Tomko wrote:
> Use a for cycle instead of while.
s/cycle/loop
> 
> Do not opencode c_isxdigit and virHexToBin.
> ---
>  src/nodeinfo.c | 12 +++-
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 9db3233..2fafe2d 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -361,15 +361,9 @@ virNodeCountThreadSiblings(const char *dir, unsigned int 
> cpu)
>  if (virFileReadAll(path, SYSFS_THREAD_SIBLINGS_LIST_LENGTH_MAX, &str) < 
> 0)
>  goto cleanup;
>  
> -i = 0;
> -while (str[i] != '\0') {
> -if (c_isdigit(str[i]))
> -ret += count_one_bits(str[i] - '0');
> -else if (str[i] >= 'A' && str[i] <= 'F')
> -ret += count_one_bits(str[i] - 'A' + 10);
> -else if (str[i] >= 'a' && str[i] <= 'f')
> -ret += count_one_bits(str[i] - 'a' + 10);
> -i++;
> +for (i = 0; str[i] != '\0'; i++) {
> +if (c_isxdigit(str[i]))
> +ret += count_one_bits(virHexToBin(str[i]));
>  }
>  
>   cleanup:
> 
ACK to both with that minor adjustment in commit message.
Erik

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

Re: [libvirt] [RFC] libvirt support multi-thread compression for live migration

2015-06-02 Thread Qiao, Liyong 
Hi Eric
Thanks for replying the mail, replied in lines.

BR, Eli(Li Yong)Qiao

-Original Message-
From: Eric Blake [mailto:ebl...@redhat.com] 
Sent: Tuesday, June 02, 2015 8:21 PM
To: Feng, Shaohe; libvir-list@redhat.com
Cc: Qiao, Liyong; Bhandaru, Malini K; Ding, Jian-feng; Chylinski, Arek; 
Koniszewski, Pawel; Li, Liang Z; berra...@redhat.com; veill...@redhat.com
Subject: Re: [RFC] libvirt support multi-thread compression for live migration

[correcting list address: libvirt-list, not libver-list]

On 06/02/2015 05:58 AM, Feng, Shaohe wrote:
> Hi folks:
> I'd like to request some comments on enabling multi-thread compression in 
> libvirt.
> 
> Currently, qemu upstream has supported multi-thread compression for live 
> migration.
> $ git log 263170e~1..362ba4e -oneline
> This can preserve bandwidth and speed up the live migration process, 
> so this is an import feature we need to enable so for other high level such 
> as openstack will be benefit.
> 
> We plan to add feature of multi-thread compression and actually the patch is 
> working in process.
> 
> Now some things need for comments.
> 
> 1.  Expose new set/get multi-thread compression parameters API for live 
> migration.
> Now qemu only supports zlib compression. Maybe LZ4 and gzip will be 
> supported later.
> but there are 3 parameters needed by qemu currently.
> "compress-level":  compression level, default is 1. For zlib compression, it 
> is from 0-9, 0 means use default level, 9 means high compressed.
> "compress-threads":  compression thread count for multi-thread migration, 
> default is 8 in qemu.
> "decompress-threads": decompression thread count for multi-thread 
> migration, default is 2 in qemu
> 
> So in libvirt we will expose the public symbols as follow, we only 3 
> parameters, missing compression method(zlib, LZ4 and gzip) parameters, for 
> qemu do not support it at present.
> index d851225..103b3b9 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -798,6 +798,17 @@ int virDomainMigrateGetMaxSpeed(virDomainPtr domain,
>  unsigned long *bandwidth,
>  unsigned int flags);
> 
> +int virdomainMigrateSetParameters(virDomainPtr domain,
> +  int level,
> +  int threads,
> +  int dthreads,
> +  int flags) int 
> +virdomainMigrateGetParameters(virDomainPtr domain,
> +  int *level,
> +  int *threads,
> +  int *dthreads,
> +  int flags)
> +

I'd rather we used virTypedParameters, to make it easier to use the same API to 
pass ALL future possible tuning parameters, rather than just hard-coding to 
only the parameters of this one feature.

Okay ,that sound good, but if virTypedParameters can be passed to qemu_driver 
such as qemu_monitor_json.c qemu_monitor_text.c ?

> 
> For virdomainMigrateSetParameters, if specifying a negative value to 
> level, threads, and dthreads parameters, such as -1, means do not set the 
> parameters.
> The underlying code will not pass the corresponding parameters to qemu 
> monitor.
> Such as threads and dthreads is -1, then pass the following json streaming to 
> qemu.
> { "execute": "migrate-set-parameters" , "arguments":  { 
> "compress-level": 1 } }
> 
> The virsh will expose the two commands:
> # virsh migrate-setparameters   [--level level] [--threads 
> threads] [--dthreads dthread] # virsh migrate-getparameters  
> 
> 2.  How to enable multi-thread compression in application interface?
>There is not a special interface for setting migration capabilities.
> But we can set the capabilities when execute an virsh command as 
> following:
> $ migrate --live] [--offline] [--p2p] [--direct] [--tunnelled] 
> [--persistent] [--undefinesource] [--suspend] [--copy-storage-all] 
> [--copy-storage-inc] [--change-protection] [--unsafe] [--verbose] 
> [--compressed] [--abort-on-error]   [] 
> [] [] [] [--timeout ] 
> [--xml ]
> 
> There is already a "compressed" option,  here "compressed" means "xbzrle" not 
> "multi- compressed".
> can I rename the "compressed" to "xbzrle"?

If we think that it is worth always turning on both compression styles 
simultaneously, then reuse the bit.  Otherwise, we need a different bit, and 
users can choose which (or both) of the two compression styles as desired.

+1 for reuse compressed, and we can set compress-level, compress-threads, 
compress-dthreads by virdomainMigrateSetParameters(maybe some new virsh 
command--- migrate-setparameter)
But how can we passing these parameter when we using `virsh migrate `, is there 
any parameter we can use in 'virsh migrate' command ?
Can you point me out ?

> so we add another option for multi-thread compression, which is better option 
> name? "

Re: [libvirt] [PATCH 07/35] qemu: Refactor qemuDomainHelperGetVcpus by reusing virBitmapToDataBuf

2015-06-02 Thread Ján Tomko 
On Fri, May 29, 2015 at 03:33:28PM +0200, Peter Krempa wrote:
> Get rid of the unnecessary allocation and copying of the bitmap and
> clean up some unnecesary temporary variables.
> ---
>  src/qemu/qemu_driver.c | 20 +++-
>  1 file changed, 3 insertions(+), 17 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 09/35] libxl: Unbreak vcpu pinning

2015-06-02 Thread Ján Tomko 
On Fri, May 29, 2015 at 03:33:30PM +0200, Peter Krempa wrote:
> Libxl's vcpu pinning would work only if the vcpu array was ordered and
> was not sparse. Remove the condition and iterate the pinning array
> properly.
> ---
>  src/libxl/libxl_domain.c | 16 +++-
>  1 file changed, 7 insertions(+), 9 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 08/35] libxl: Reuse virBitmapToData in libxlDomainSetVcpuAffinities

2015-06-02 Thread Ján Tomko 
On Fri, May 29, 2015 at 03:33:29PM +0200, Peter Krempa wrote:
> ---
>  src/libxl/libxl_domain.c | 21 -
>  1 file changed, 4 insertions(+), 17 deletions(-)
> 

ACK

> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index c7f0ed9..89782c3 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -797,34 +797,21 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr 
> driver, virDomainObjPtr vm)
>  virDomainDefPtr def = vm->def;
>  libxl_bitmap map;
>  virBitmapPtr cpumask = NULL;
> -uint8_t *cpumap = NULL;
>  virNodeInfo nodeinfo;
> -size_t cpumaplen;
>  int vcpu;
> -size_t i;
>  int ret = -1;
> 
>  if (libxlDriverNodeGetInfo(driver, &nodeinfo) < 0)
>  goto cleanup;

This call can also be removed - nodeinfo is unused after this patch.

> 
> -cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
> -
>  for (vcpu = 0; vcpu < def->cputune.nvcpupin; ++vcpu) {
>  if (vcpu != def->cputune.vcpupin[vcpu]->id)
>  continue;

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 14/35] qemu: process: Refactor setup of memory ballooning

2015-06-02 Thread Ján Tomko 
On Fri, May 29, 2015 at 03:33:35PM +0200, Peter Krempa wrote:
> Since the monitor code now supports ullongs when setting balloon size,
> drop the legacy code with overflow checking.
> 
> Additionally the comment mentioning that the job is treated as a sync
> job does not make sense any more since the monitor is entered
> asynchronously.
> ---
>  src/qemu/qemu_process.c | 38 --
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 

ACK to patches 10-14.

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 v4 0/6] virsh: Further improve handling of integer options

2015-06-02 Thread Andrea Bolognani 
On Tue, 2015-06-02 at 09:23 -0400, John Ferlan wrote:

> ACK series... and pushed

Thanks :)

-- 
Andrea Bolognani
Software Engineer - Virtualization Team
$ python -c "print('a'.join(['', 'bologn', '@redh', 't.com']))"

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

[libvirt] [PATCH] build: silence ar warnings on rawhide

2015-06-02 Thread Eric Blake 
Newer binutils 'ar' has added an option 'D' for deterministic
builds, and at least on rawhide, this option is enabled by default.
But it conflicts with the 'u' optimization where the linker only
modifies libraries based on file timestamps, but can result in
different library ordering based on which files were touched last.
Thus, it results in some noisy compilation, for every CCLD line:

  CCLD libvirt_driver_qemu_impl.la
  ar: `u' modifier ignored since `D' is the default (see `U')

Upstream automake has decided that defaulting ARFLAGS to 'cru' is
no longer beneficial, and that switching the default to 'cr' will
both silence the noise and not penalize modern build systems.

https://lists.gnu.org/archive/html/automake-patches/2015-06/msg0.html

But rather than wait for newer automake to propagate to all systems
that already have newer binutils, we might as well just use the new
default ourselves, even on older platforms.

Signed-off-by: Eric Blake 
---

This is bugging me even more now that Fedora 22 also suffers from
the same issue (we've been ignoring it in rawhide for several months
now).  While it seems trivial, I'd like a review before pushing.

 configure.ac | 4 
 1 file changed, 4 insertions(+)

diff --git a/configure.ac b/configure.ac
index 0da3ae8..abf4436 100644
--- a/configure.ac
+++ b/configure.ac
@@ -25,6 +25,10 @@ AC_CONFIG_MACRO_DIR([m4])
 dnl Make automake keep quiet about wildcards & other GNUmake-isms; also keep
 dnl quiet about the fact that we intentionally cater to automake 1.9
 AM_INIT_AUTOMAKE([-Wno-portability -Wno-obsolete tar-ustar subdir-objects])
+dnl older automake's default of ARFLAGS=cru is noisy on newer binutils;
+dnl we don't really need the 'u' even in older toolchains.  Then there is
+dnl older libtool, which spelled it AR_FLAGS
+m4_divert_text([DEFAULTS], [: "${ARFLAGS=cr} ${AR_FLAGS=cr}"])

 # Maintainer note - comment this line out if you plan to rerun
 # GNULIB_POSIXCHECK testing to see if libvirt should be using more modules.
-- 
2.4.2

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

Re: [libvirt] [PATCH] build: silence ar warnings on rawhide

2015-06-02 Thread Daniel P. Berrange 
On Tue, Jun 02, 2015 at 09:24:26AM -0600, Eric Blake wrote:
> Newer binutils 'ar' has added an option 'D' for deterministic
> builds, and at least on rawhide, this option is enabled by default.
> But it conflicts with the 'u' optimization where the linker only
> modifies libraries based on file timestamps, but can result in
> different library ordering based on which files were touched last.
> Thus, it results in some noisy compilation, for every CCLD line:
> 
>   CCLD libvirt_driver_qemu_impl.la
>   ar: `u' modifier ignored since `D' is the default (see `U')
> 
> Upstream automake has decided that defaulting ARFLAGS to 'cru' is
> no longer beneficial, and that switching the default to 'cr' will
> both silence the noise and not penalize modern build systems.
> 
> https://lists.gnu.org/archive/html/automake-patches/2015-06/msg0.html
> 
> But rather than wait for newer automake to propagate to all systems
> that already have newer binutils, we might as well just use the new
> default ourselves, even on older platforms.
> 
> Signed-off-by: Eric Blake 
> ---
> 
> This is bugging me even more now that Fedora 22 also suffers from
> the same issue (we've been ignoring it in rawhide for several months
> now).  While it seems trivial, I'd like a review before pushing.
> 
>  configure.ac | 4 
>  1 file changed, 4 insertions(+)

ACK


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

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

Re: [libvirt] [PATCH] virsh: Fix Ctrl-C behavior when watching a job

2015-06-02 Thread Ján Tomko 
On Mon, Jun 01, 2015 at 09:05:59PM +0200, Jiri Denemark wrote:
> When watching a job (save, managedsave, dump, migrate) virsh spawns a
> thread to call the appropriate API and waits for the result while
> watching for interruption signals (SIGINT, Ctrl-C on the terminal).
> Whenever such signal is caught, virsh calls virDomainAbortJob, stops
> waiting for the job, and returns the result of virDomainAbortJob.
> 
> This is wrong because the job might have finished in the meantime or it
> might have been cancelled by someone else and virsh would just report
> the failure to abort the job. However, we are not interested in the
> virDomainAbortJob's result at all, we need to keep waiting for the main
> job to finish and report its result instead.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1131755
> 
> Signed-off-by: Jiri Denemark 
> ---
>  tools/virsh-domain.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 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] qemu: monitor: Add memory balloon support for virtio-ccw

2015-06-02 Thread Ján Tomko 
On Tue, Jun 02, 2015 at 11:27:35AM +0200, Boris Fiuczynski wrote:
> The search for the memory ballon driver object is extended by a
> second known name "virtio-ballon-ccw" in support for virtio-ccw.
> 
> Signed-off-by: Boris Fiuczynski 
> Reviewed-by: Christian Borntraeger 
> ---
>  src/qemu/qemu_monitor.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index f959b74..1a88329 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1141,9 +1141,9 @@ qemuMonitorFindObjectPath(qemuMonitorPtr mon,
>  
>  
>  /**
> - * Search the qom objects for the balloon driver object by it's known name
> - * of "virtio-balloon-pci".  The entry for the driver will be found by using
> - * function "qemuMonitorFindObjectPath".
> + * Search the qom objects for the balloon driver object by it's known names

s/it's/its/

> + * of "virtio-balloon-pci" or "virtio-ballon-ccw". The entry for the driver
> + * will be found by using function "qemuMonitorFindObjectPath".
>   *
>   * Once found, check the entry to ensure it has the correct property listed.
>   * If it does not, then obtaining statistics from QEMU will not be possible.
> @@ -1183,7 +1183,8 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
>  return -1;
>  }
>  
> -if (qemuMonitorFindObjectPath(mon, curpath, "virtio-balloon-pci", &path) 
> < 0)
> +if (qemuMonitorFindObjectPath(mon, curpath, "virtio-balloon-pci", &path) 
> < 0 &&
> +qemuMonitorFindObjectPath(mon, curpath, "virtio-balloon-ccw", &path) 
> < 0)
>  return -1;

qemuMonitorFindObjectPath can return:
  0  - Found
 -1  - Error bail out
 -2  - Not found

But it only reports an error when returning -1. There is a
(pre-existing) missing virReportError in that case.

Looking for the ccw balloon only makes sense when the pci one was not
found. A fatal error (-1) when finding the PCI balloon was not found
will very probably be fatal for CCW as well.

Jan


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

Re: [libvirt] [RFC] libvirt support multi-thread compression for live migration

2015-06-02 Thread Eric Blake 
On 06/02/2015 07:56 AM, Qiao, Liyong wrote:
> Hi Eric
> Thanks for replying the mail, replied in lines.
> 

>> +virdomainMigrateGetParameters(virDomainPtr domain,
>> +  int *level,
>> +  int *threads,
>> +  int *dthreads,
>> +  int flags)
>> +
> 
> I'd rather we used virTypedParameters, to make it easier to use the same API 
> to pass ALL future possible tuning parameters, rather than just hard-coding 
> to only the parameters of this one feature.
> 
> Okay ,that sound good, but if virTypedParameters can be passed to qemu_driver 
> such as qemu_monitor_json.c qemu_monitor_text.c ?

[Your quoting style makes it very hard to distinguish original text from
added text.  Please consider changing your outgoing mail process to
ensure that you add another level of quoting to all previous text so
that your added text is the only unquoted text.  Also, configure your
mailer to wrap long lines.]

Use existing API for a guide - for example, virDomainSetBlockIoTune
takes virTypedParamters, as well as defines a specific set of parameters
that it will understand.  The set of parameters can be enlarged without
needing a new API (good for backporting features without requiring a .so
version bump), but for any given libvirt version, the set of features
understood is finite and limited to what you can translate to the QMP
call.  So qemu_driver.c would take the virTypedParameters, reject the
ones it does not understand, and convert the ones it does understand
into the 'int level, int threads, int dthreads' parameters used in
qemu_monitor_json.c where you drive the actual QMP command with fixed
parameters and types.


> If we think that it is worth always turning on both compression styles 
> simultaneously, then reuse the bit.  Otherwise, we need a different bit, and 
> users can choose which (or both) of the two compression styles as desired.
> 
> +1 for reuse compressed, and we can set compress-level, compress-threads, 
> compress-dthreads by virdomainMigrateSetParameters(maybe some new virsh 
> command--- migrate-setparameter)
> But how can we passing these parameter when we using `virsh migrate `, is 
> there any parameter we can use in 'virsh migrate' command ?
> Can you point me out ?

The underlying API already has a form that takes virTypedParameters (see
virDomainMigrate3()), so your first task is to figure out how to extend
the API to expose new typed parameters for your new migration tunings.
Once that is done, then you can worry about how to tweak the 'virsh
migrate' client to pass in those new parameters.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCH] build: silence ar warnings on rawhide

2015-06-02 Thread Eric Blake 
On 06/02/2015 09:27 AM, Daniel P. Berrange wrote:
> On Tue, Jun 02, 2015 at 09:24:26AM -0600, Eric Blake wrote:
>> Newer binutils 'ar' has added an option 'D' for deterministic
>> builds, and at least on rawhide, this option is enabled by default.
>> But it conflicts with the 'u' optimization where the linker only
>> modifies libraries based on file timestamps, but can result in
>> different library ordering based on which files were touched last.

>> But rather than wait for newer automake to propagate to all systems
>> that already have newer binutils, we might as well just use the new
>> default ourselves, even on older platforms.
>>

>>
>>  configure.ac | 4 
>>  1 file changed, 4 insertions(+)
> 
> ACK
> 

Thanks; pushed.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCHv2] Always add 'console' matching the 'serial' device

2015-06-02 Thread John Ferlan 


On 05/28/2015 09:55 AM, Ján Tomko wrote:
> We have been formatting the first serial device also
> as a console device, but only if there were no other consoles.
> 
> If there is a  device present in the XML, but no serial
> , or if there isn't any  at all but the domain
> definition hasn't gone through a parse->format->parse round-trip,
> the  device would not be formatted.
> 
> Change the code to always add the stub device for the first
> serial device.
> 
> Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1089914
> ---
>  src/conf/domain_conf.c | 23 +
>  .../qemuxml2argv-console-compat2.xml   | 35 
>  .../qemuxml2xmlout-console-compat2.xml | 38 
> ++
>  tests/qemuxml2xmltest.c|  1 +
>  4 files changed, 97 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-compat2.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat2.xml
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6e57425..ec8f9fa 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3587,6 +3587,25 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
>  def->consoles[0]->deviceType = 
> VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE;
>  def->consoles[0]->targetType = 
> VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL;
>  }
> +} else if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && def->nserials > 0 &&
> +   def->serials[0]->deviceType == 
> VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
> +   def->serials[0]->targetType == 
> VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA) {
> +/* Create a stub console to match the serial port.
> + * console[0] either does not exist
> + *or has a different type than SERIAL or NONE.
> + */
> +virDomainChrDefPtr chr;
> +if (VIR_ALLOC(chr) < 0)
> +return -1;
> +
> +if (VIR_INSERT_ELEMENT(def->consoles,
> +   0,
> +   def->nconsoles,
> +   chr) < 0)

If this fails we need to VIR_FREE(chr);

Everything else seems reasonable...

ACK

John
> +return -1;
> +
> +def->consoles[0]->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE;
> +def->consoles[0]->targetType = 
> VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL;
>  }
>  
>  if (virDomainDefRejectDuplicateControllers(def) < 0)
> @@ -21588,6 +21607,10 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>  if (virDomainChrDefFormat(buf, &console, flags) < 0)
>  goto error;
>  }
> +/* The back-compat console device stub is added when parsing the domain 
> XML
> + * and handled above, this is for formatting definitions created via 
> other
> + * means.
> + */
>  if (def->os.type == VIR_DOMAIN_OSTYPE_HVM &&
>  def->nconsoles == 0 &&
>  def->nserials > 0) {
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-compat2.xml 
> b/tests/qemuxml2argvdata/qemuxml2argv-console-compat2.xml
> new file mode 100644
> index 000..ded204a
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-console-compat2.xml
> @@ -0,0 +1,35 @@
> +
> +  QEMUGuest1
> +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> +  219100
> +  219100
> +  1
> +  
> +hvm
> +
> +  
> +  
> +  destroy
> +  restart
> +  destroy
> +  
> +/usr/bin/qemu
> +
> +  
> +  
> +  
> +  
> +
> +
> +
> +
> +
> +
> +  
> +
> +
> +  
> +
> +
> +  
> +
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat2.xml 
> b/tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat2.xml
> new file mode 100644
> index 000..636e984
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat2.xml
> @@ -0,0 +1,38 @@
> +
> +  QEMUGuest1
> +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> +  219100
> +  219100
> +  1
> +  
> +hvm
> +
> +  
> +  
> +  destroy
> +  restart
> +  destroy
> +  
> +/usr/bin/qemu
> +
> +  
> +  
> +  
> +  
> +
> +
> +
> +
> +
> +
> +  
> +
> +
> +  
> +
> +
> +  
> +
> +
> +  
> +
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 53bcc9f..d405e71 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -472,6 +472,7 @@ mymain(void)
>  DO_TEST("serial-spiceport-nospice");
>  DO_TEST("parallel-tcp");
>  DO_TEST("console-compat");
> +DO_TEST_DIFFERENT("console-compat2");
>  DO_TEST("console-virtio-many");
>  DO_TEST("channel-guestfwd");
>  DO_TEST("channel-virtio");
> 

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

Re: [libvirt] [PATCH] qemu: fix wrong call addressrelease function when attach RNG device success

2015-06-02 Thread John Ferlan 


On 05/31/2015 07:29 AM, Luyao Huang wrote:
> Add a return value check to avoid the wrong address release.
> 
> Signed-off-by: Luyao Huang 
> ---
>  src/qemu/qemu_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

ACK - although I'll make an adjustment to the commit message before pushing

John

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

Re: [libvirt] [PATCH] qemu: fix forget pass the return value to variable @ret

2015-06-02 Thread John Ferlan 


On 05/31/2015 09:27 AM, Luyao Huang wrote:
> If we do not pass the return value from qemuDomainRemoveRNGDevice()
> function to variable @ret, qemuDomainRemoveDevice() functiuon will
> always fail when recive rng device unplug event from qemu monitor.
> 
> Signed-off-by: Luyao Huang 
> ---
>  src/qemu/qemu_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

ACK -

Will adjust commit message slightly before pushing

John

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

Re: [libvirt] [PATCH] conf:audit: fix no audit log when start a vm with iothread

2015-06-02 Thread John Ferlan 


On 05/31/2015 10:07 AM, Luyao Huang wrote:
> Signed-off-by: Luyao Huang 
> ---
>  src/conf/domain_audit.c | 2 ++
>  1 file changed, 2 insertions(+)
> 

ACK -

Will adjust commit message slightly before pushing...

John

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

[libvirt] Repository with Parallels SDK sources moved

2015-06-02 Thread Sergey Bronnikov 
Hello,

repository with Parallels SDK has been moved from
https://github.com/CloudServer/parallels-sdk
to https://src.openvz.org/projects/OVZ/repos/libprlsdk/browse.
Please update GIT settings.

-- 
OpenVZ survey http://goo.gl/forms/orYmy0SjFB

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

[libvirt] [PATCH] storage: add RBD support to disk source pool translation

2015-06-02 Thread Thibault VINCENT 
Hello,

We needed this to avoid redundancies when defining domains with RBD backends,
because monitor list and authentication secret could not be pulled from the
storage pool.

I didn't find any bit of documentation to update, actually it already implies
that disks with source type 'volume' would work with all kinds of backing
pools. But mpath, sheepdog, and gluster would still require an implementation
if it's possible.

Using RBD volumes this way still has a limitation, we cannot list snapshots in
libvirt pools. Whereas they could be accessed read-only if the domain is using
a network source explicitely with a 'snapshot' attribute.
How could we implement this so that snapshots are seen as pool volumes? There
are real use-cases for this like attaching to a VM a backup from it's own
disks. Also it would work the same way with ZFS snapshots, but AFAIK not for
LVM pools.

Cheers


Thibault VINCENT (1):
  storage: add RBD support to generic disk source pool translation

 src/storage/storage_driver.c | 49 +++-
 1 file changed, 48 insertions(+), 1 deletion(-)

-- 
2.1.4

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

[libvirt] [PATCH] storage: add RBD support to disk source pool translation

2015-06-02 Thread Thibault VINCENT 
Domains can now reference disks of type 'volume' with an underlying RBD pool.
It won't allow mapping snapshots, pools don't list them yet, only COW clones.

 - virStorageTranslateDiskSourcePool: add case to copy RBD attributes
 - virStorageAddRBDPoolSourceHost: new helper to copy monitor hosts
---
 src/storage/storage_driver.c | 49 +++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 394e4d4..8c27f4b 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -3173,6 +3173,39 @@ virStorageAddISCSIPoolSourceHost(virDomainDiskDefPtr def,
 
 
 static int
+virStorageAddRBDPoolSourceHost(virDomainDiskDefPtr def,
+virStoragePoolDefPtr pooldef)
+{
+int ret = -1;
+size_t i;
+
+if (pooldef->source.nhost > 0) {
+def->src->nhosts = pooldef->source.nhost;
+
+if (VIR_ALLOC_N(def->src->hosts, def->src->nhosts) < 0)
+goto cleanup;
+
+for (i = 0; i < def->src->nhosts; i++) {
+if (VIR_STRDUP(def->src->hosts[i].name,
+pooldef->source.hosts[i].name) < 0)
+goto cleanup;
+
+if (pooldef->source.hosts[i].port) {
+if (virAsprintf(&def->src->hosts[i].port, "%d",
+pooldef->source.hosts[i].port) < 0)
+goto cleanup;
+}
+}
+}
+
+ret = 0;
+
+ cleanup:
+return ret;
+}
+
+
+static int
 virStorageTranslateDiskSourcePoolAuth(virDomainDiskDefPtr def,
   virStoragePoolSourcePtr source)
 {
@@ -3324,8 +3357,22 @@ virStorageTranslateDiskSourcePool(virConnectPtr conn,
}
break;
 
-case VIR_STORAGE_POOL_MPATH:
 case VIR_STORAGE_POOL_RBD:
+if (!(def->src->path = virStorageVolGetPath(vol)))
+goto cleanup;
+
+def->src->srcpool->actualtype = VIR_STORAGE_TYPE_NETWORK;
+def->src->protocol = VIR_STORAGE_NET_PROTOCOL_RBD;
+
+if (virStorageTranslateDiskSourcePoolAuth(def, &pooldef->source) < 0)
+goto cleanup;
+
+if (virStorageAddRBDPoolSourceHost(def, pooldef) < 0)
+goto cleanup;
+
+break;
+
+case VIR_STORAGE_POOL_MPATH:
 case VIR_STORAGE_POOL_SHEEPDOG:
 case VIR_STORAGE_POOL_GLUSTER:
 case VIR_STORAGE_POOL_LAST:
-- 
2.1.4

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

Re: [libvirt] [RFC] libvirt support multi-thread compression for live migration

2015-06-02 Thread Qiao,Liyong 



On 2015年06月03日 01:02, Eric Blake wrote:

On 06/02/2015 07:56 AM, Qiao, Liyong wrote:

Hi Eric
Thanks for replying the mail, replied in lines.


+virdomainMigrateGetParameters(virDomainPtr domain,
+  int *level,
+  int *threads,
+  int *dthreads,
+  int flags)
+

I'd rather we used virTypedParameters, to make it easier to use the same API to 
pass ALL future possible tuning parameters, rather than just hard-coding to 
only the parameters of this one feature.

Okay ,that sound good, but if virTypedParameters can be passed to qemu_driver 
such as qemu_monitor_json.c qemu_monitor_text.c ?

[Your quoting style makes it very hard to distinguish original text from
added text.  Please consider changing your outgoing mail process to
ensure that you add another level of quoting to all previous text so
that your added text is the only unquoted text.  Also, configure your
mailer to wrap long lines.]
hi Eric, sorry about the inconvenient mail client, I switch outlook to 
thunder

bird, hopes it will be better.

Use existing API for a guide - for example, virDomainSetBlockIoTune
takes virTypedParamters, as well as defines a specific set of parameters
that it will understand.  The set of parameters can be enlarged without
needing a new API (good for backporting features without requiring a .so
version bump), but for any given libvirt version, the set of features
understood is finite and limited to what you can translate to the QMP
call.  So qemu_driver.c would take the virTypedParameters, reject the
ones it does not understand, and convert the ones it does understand
into the 'int level, int threads, int dthreads' parameters used in
qemu_monitor_json.c where you drive the actual QMP command with fixed
parameters and types.
ah, that helps, thanks for kindly supporting, we will think a bit more 
to how

implement this API (taken virTypedParamters as parameter)




If we think that it is worth always turning on both compression styles 
simultaneously, then reuse the bit.  Otherwise, we need a different bit, and 
users can choose which (or both) of the two compression styles as desired.

+1 for reuse compressed, and we can set compress-level, compress-threads, 
compress-dthreads by virdomainMigrateSetParameters(maybe some new virsh 
command--- migrate-setparameter)
But how can we passing these parameter when we using `virsh migrate `, is there 
any parameter we can use in 'virsh migrate' command ?
Can you point me out ?

The underlying API already has a form that takes virTypedParameters (see
virDomainMigrate3()), so your first task is to figure out how to extend
the API to expose new typed parameters for your new migration tunings.
Once that is done, then you can worry about how to tweak the 'virsh
migrate' client to pass in those new parameters.


--
BR, Eli(Li Yong)Qiao

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

[libvirt] [PATCHv3] qemu: fix unsuitable error report when get memory stats

2015-06-02 Thread Wang Yufei 
From: Zhang Bo 

when we run the command 'virsh dommemstat xxx',
althrough memballoon's model is set 'none' in vm's XML,
it still reports an error in libvirtd.log.
error : qemuMonitorFindBalloonObjectPath:1042 : internal error: Cannot 
determine balloon device path
Apparently, if we don't set memballoon, we don't need to
set balloon device path.

Signed-off-by: Wang Yufei 
Signed-off-by: Zhang Bo 
---
 src/qemu/qemu_monitor.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index f959b74..8c3c6f3 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1169,8 +1169,10 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
 if (mon->balloonpath) {
 return 0;
 } else if (mon->ballooninit) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Cannot determine balloon device path"));
+if (vm->def->memballoon && 
+vm->def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_NONE)
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Cannot determine balloon device path"));
 return -1;
 }
 
-- 
1.7.12.4


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

Re: [libvirt] [PATCH] qemu: fix wrong call addressrelease function when attach RNG device success

2015-06-02 Thread lhuang 


On 06/03/2015 02:04 AM, John Ferlan wrote:


On 05/31/2015 07:29 AM, Luyao Huang wrote:

Add a return value check to avoid the wrong address release.

Signed-off-by: Luyao Huang 
---
  src/qemu/qemu_hotplug.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


ACK - although I'll make an adjustment to the commit message before pushing


Thanks a lot for your help and review



John


Luyao

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

Re: [libvirt] [PATCH] qemu: fix forget pass the return value to variable @ret

2015-06-02 Thread lhuang 


On 06/03/2015 02:05 AM, John Ferlan wrote:


On 05/31/2015 09:27 AM, Luyao Huang wrote:

If we do not pass the return value from qemuDomainRemoveRNGDevice()
function to variable @ret, qemuDomainRemoveDevice() functiuon will
always fail when recive rng device unplug event from qemu monitor.

Signed-off-by: Luyao Huang 
---
  src/qemu/qemu_hotplug.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


ACK -

Will adjust commit message slightly before pushing


Thank your help John



John


Luyao

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

Re: [libvirt] [PATCH] conf:audit: fix no audit log when start a vm with iothread

2015-06-02 Thread lhuang 


On 06/03/2015 02:06 AM, John Ferlan wrote:


On 05/31/2015 10:07 AM, Luyao Huang wrote:

Signed-off-by: Luyao Huang 
---
  src/conf/domain_audit.c | 2 ++
  1 file changed, 2 insertions(+)


ACK -

Will adjust commit message slightly before pushing...


Thanks again for your review and help... ;)



John


Luyao

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