[libvirt] [PATCH 0/2] BlockPivot issues
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
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
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
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
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
(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
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
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
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
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa