8:09 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=977678
Ján Tomko (2):
qemu: fix return value of qemuDomainBlockPivot on errors
blockjob: make PIVOT and ASYNC flags mutually exclusive
src/qemu/qemu_driver.c | 15 +++++++++++----
tools/virsh-domain.c | 9 ++++++---
2 files changed, 17 insertions(+), 7 deletions(-)
--
1.8.1.5
8:09 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: fix return value of qemuDomainBlockPivot on errors
If qemuMonitorBlockJob returned 0, qemuDomainBlockPivot
might return 0 even if an error occured.
https://bugzilla.redhat.com/show_bug.cgi?id=977678
---
src/qemu/qemu_driver.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f51e766..6a83fda 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13844,7 +13844,7 @@ qemuDomainBlockPivot(virConnectPtr conn,
virQEMUDriverPtr driver, virDomainObjPtr vm,
const char *device, virDomainDiskDefPtr disk)
{
- int ret = -1;
+ int ret = -1, rc;
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainBlockJobInfo info;
const char *format = virStorageFileFormatTypeToString(disk->mirrorFormat);
@@ -13857,17 +13857,17 @@ qemuDomainBlockPivot(virConnectPtr conn,
/* Probe the status, if needed. */
if (!disk->mirroring) {
qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorBlockJob(priv->mon, device, NULL, 0, &info,
+ rc = qemuMonitorBlockJob(priv->mon, device, NULL, 0, &info,
BLOCK_JOB_INFO, true);
qemuDomainObjExitMonitor(driver, vm);
- if (ret < 0)
+ if (rc < 0)
goto cleanup;
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("domain is not running"));
goto cleanup;
}
- if (ret == 1 && info.cur == info.end &&
+ if (rc == 1 && info.cur == info.end &&
info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY)
disk->mirroring = true;
}
--
1.8.1.5
8:24 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: fix return value of qemuDomainBlockPivot on errors
On 07/01/13 15:09, Ján Tomko wrote:
If qemuMonitorBlockJob returned 0, qemuDomainBlockPivot
might return 0 even if an error occured.
https://bugzilla.redhat.com/show_bug.cgi?id=977678
---
src/qemu/qemu_driver.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
ACK
Peter
1 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: fix return value of qemuDomainBlockPivot on errors
On 07/01/2013 03:24 PM, Peter Krempa wrote:
On 07/01/13 15:09, Ján Tomko wrote:
> If qemuMonitorBlockJob returned 0, qemuDomainBlockPivot
> might return 0 even if an error occured.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=977678
> ---
> src/qemu/qemu_driver.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
ACK
Peter
Thanks, now pushed.
Jan
8:09 a.m.
New subject: [libvirt] [PATCH 2/2] blockjob: make PIVOT and ASYNC flags mutually exclusive
https://bugzilla.redhat.com/show_bug.cgi?id=977678
---
src/qemu/qemu_driver.c | 7 +++++++
tools/virsh-domain.c | 9 ++++++---
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6a83fda..aa7affe 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14153,6 +14153,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom, const char *path,
unsigned int flags)
virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC |
VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1);
+ if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC) &&
+ (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("asynchronnous pivot not supported"));
+ return -1;
+ }
+
if (!(vm = qemuDomObjFromDomain(dom)))
return -1;
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 5257416..2653388 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1919,11 +1919,14 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
virDomainBlockJobInfo info;
const char *type;
int ret;
- bool abortMode = (vshCommandOptBool(cmd, "abort") ||
- vshCommandOptBool(cmd, "async") ||
- vshCommandOptBool(cmd, "pivot"));
+ bool abort = vshCommandOptBool(cmd, "abort");
+ bool async = vshCommandOptBool(cmd, "async");
+ bool pivot = vshCommandOptBool(cmd, "pivot");
bool infoMode = vshCommandOptBool(cmd, "info");
bool bandwidth = vshCommandOptBool(cmd, "bandwidth");
+ bool abortMode = abort || async || pivot;
+
+ VSH_EXCLUSIVE_OPTIONS_VAR(async, pivot);
if (abortMode + infoMode + bandwidth > 1) {
vshError(ctl, "%s",
--
1.8.1.5
10:28 a.m.
New subject: [libvirt] [PATCH 2/2] blockjob: make PIVOT and ASYNC flags mutually exclusive
(CC'd Eric)
On 07/01/13 15:09, Ján Tomko wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=977678
---
src/qemu/qemu_driver.c | 7 +++++++
tools/virsh-domain.c | 9 ++++++---
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6a83fda..aa7affe 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14153,6 +14153,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom, const char *path,
unsigned int flags)
virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC |
VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1);
+ if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC) &&
+ (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("asynchronnous pivot not supported"));
+ return -1;
+ }
+
I agree with this hunk.
if (!(vm = qemuDomObjFromDomain(dom)))
return -1;
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 5257416..2653388 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1919,11 +1919,14 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
virDomainBlockJobInfo info;
const char *type;
int ret;
- bool abortMode = (vshCommandOptBool(cmd, "abort") ||
- vshCommandOptBool(cmd, "async") ||
- vshCommandOptBool(cmd, "pivot"));
+ bool abort = vshCommandOptBool(cmd, "abort");
+ bool async = vshCommandOptBool(cmd, "async");
+ bool pivot = vshCommandOptBool(cmd, "pivot");
bool infoMode = vshCommandOptBool(cmd, "info");
bool bandwidth = vshCommandOptBool(cmd, "bandwidth");
+ bool abortMode = abort || async || pivot;
+
+ VSH_EXCLUSIVE_OPTIONS_VAR(async, pivot);
.. but I don't think we should forbid this combination in virsh. I think
could happen that we might need this combination. I think that the
combination of _ABORT and _PIVOT is less usefull.
Eric, what do you think?
(Or if we do forbid it, we need to document it, and ban it at library
level instead of qemu driver)
if (abortMode + infoMode + bandwidth > 1) {
vshError(ctl, "%s",
Peter
1:01 p.m.
New subject: [libvirt] [PATCH 2/2] blockjob: make PIVOT and ASYNC flags mutually exclusive
On 07/01/2013 09:28 AM, Peter Krempa wrote:
(CC'd Eric)
On 07/01/13 15:09, Ján Tomko wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=977678
> ---
> src/qemu/qemu_driver.c | 7 +++++++
> tools/virsh-domain.c | 9 ++++++---
> 2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 6a83fda..aa7affe 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14153,6 +14153,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> const char *path, unsigned int flags)
> virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC |
> VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1);
>
> + if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC) &&
> + (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("asynchronnous pivot not supported"));
> + return -1;
> + }
> +
I agree with this hunk.
I'm still not convinced, but agree that only this hunk is necessary if
we want the patch.
> + VSH_EXCLUSIVE_OPTIONS_VAR(async, pivot);
.. but I don't think we should forbid this combination in virsh. I think
could happen that we might need this combination. I think that the
combination of _ABORT and _PIVOT is less usefull.
Eric, what do you think?
(Or if we do forbid it, we need to document it, and ban it at library
level instead of qemu driver)
I prefer letting virsh pass ALL combinations down to lower level
software, when possible. It lets us test that the lower-level software
is accepting or rejecting combinations, instead of silently
short-circuiting combinations that may later prove useful. I agree with
your desire to NACK this hunk.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:40 a.m.
New subject: [libvirt] [PATCH 2/2] blockjob: make PIVOT and ASYNC flags mutually exclusive
On 07/01/2013 07:09 AM, Ján Tomko wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=977678
---
src/qemu/qemu_driver.c | 7 +++++++
tools/virsh-domain.c | 9 ++++++---
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6a83fda..aa7affe 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14153,6 +14153,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom, const char *path,
unsigned int flags)
virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC |
VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1);
+ if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC) &&
+ (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("asynchronnous pivot not supported"));
s/asynchronnous/asynchronous/
Should this restriction be enforced at the API level in src/libvirt.c
instead? Probably not, because it might be conceivable that some types
of pivot operations might be long-running.
Next, do we need the restriction? If the 'async' flag merely means
return as fast as possible, and pivots (currently) always return
immediately (no long-running operations), then we can declare that use
or absence of the async flag makes no difference, and can silently
ignore it instead of forcefully rejecting it.
I'm inclined to NACK this patch, if we can't come up with a better
explanation of why it is needed.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:10 a.m.
New subject: [libvirt] [PATCH 2/2] blockjob: make PIVOT and ASYNC flags mutually exclusive
On 07/01/2013 06:40 PM, Eric Blake wrote:
On 07/01/2013 07:09 AM, Ján Tomko wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=977678
> ---
> src/qemu/qemu_driver.c | 7 +++++++
> tools/virsh-domain.c | 9 ++++++---
> 2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 6a83fda..aa7affe 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14153,6 +14153,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom, const char *path,
unsigned int flags)
> virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC |
> VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1);
>
> + if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC) &&
> + (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("asynchronnous pivot not supported"));
s/asynchronnous/asynchronous/
Should this restriction be enforced at the API level in src/libvirt.c
instead? Probably not, because it might be conceivable that some types
of pivot operations might be long-running.
Next, do we need the restriction? If the 'async' flag merely means
return as fast as possible, and pivots (currently) always return
immediately (no long-running operations), then we can declare that use
or absence of the async flag makes no difference, and can silently
ignore it instead of forcefully rejecting it.
Well, since the flag actually works, I agree that we shouldn't reject it.
I'm inclined to NACK this patch, if we can't come up with a better
explanation of why it is needed.
I'm dropping the patch then.
Jan
4198
days inactive
4199
days old
8 comments
3 participants
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa