[libvirt] [PATCH 00/13] Improve virsh block job handling
This series refactors the block job command code so that it is handled by separate functions rather than one mega-method. Additionally this series then fixes the routine for waiting for a block job. As it's perhaps obvious from the patches the API design for the block job APIs is rather unfortunate for users. This series contains a few patches that already were submitted and partially reviewed previously. Peter Krempa (13): virsh: blockjob: Extract block job info code into a separate function virsh: cmdBlockJob: Switch to declarative flag interlocking virsh: blockjob: Split out vshBlockJobSetSpeed from blockJobImpl virsh: block job: separate abort from blockJobImpl virsh: Split out block pull implementation from blockJobImpl virsh: Kill blockJobImpl by moving the final impl into cmdBlockCommit virsh: Refactor argument checking in cmdBlockCommit virsh: Refactor argument handling in cmdBlockCopy virsh: Refactor argument handling in cmdBlockPull qemu: Update state of block job to READY only if it actually is ready virsh: Refactor block job waiting in cmdBlockPull virsh: Refactor block job waiting in cmdBlockCommit virsh: Refactor block job waiting in cmdBlockCopy src/qemu/qemu_driver.c | 10 +- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 7 + tools/virsh-domain.c | 962 ++++++++++++++++++++++++------------------- 4 files changed, 551 insertions(+), 429 deletions(-) -- 2.4.5
cmdBlockJob will be converted to a hub that will call into the individual executor functions. --- tools/virsh-domain.c | 91 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 55 insertions(+), 36 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f7edeeb..7a18204 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2451,47 +2451,19 @@ vshDomainBlockJobToString(int type) return str ? _(str) : _("Unknown job"); } + static bool -cmdBlockJob(vshControl *ctl, const vshCmd *cmd) +vshBlockJobInfo(vshControl *ctl, + virDomainPtr dom, + const char *path, + bool raw, + bool bytes) { virDomainBlockJobInfo info; + unsigned long long speed; + unsigned int flags = 0; bool ret = false; int rc = -1; - bool raw = vshCommandOptBool(cmd, "raw"); - bool bytes = vshCommandOptBool(cmd, "bytes"); - bool abortMode = (vshCommandOptBool(cmd, "abort") || - vshCommandOptBool(cmd, "async") || - vshCommandOptBool(cmd, "pivot")); - bool infoMode = vshCommandOptBool(cmd, "info") || raw; - bool bandwidth = vshCommandOptBool(cmd, "bandwidth"); - virDomainPtr dom = NULL; - const char *path; - unsigned int flags = 0; - unsigned long long speed; - - if (abortMode + infoMode + bandwidth > 1) { - vshError(ctl, "%s", - _("conflict between abort, info, and bandwidth modes")); - return false; - } - /* XXX also support --bytes with bandwidth mode */ - if (bytes && (abortMode || bandwidth)) { - vshError(ctl, "%s", _("--bytes requires info mode")); - return false; - } - - if (abortMode) - return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_ABORT, NULL); - if (bandwidth) - return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_SPEED, NULL); - - /* Everything below here is for --info mode */ - if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; - - /* XXX Allow path to be optional to list info on all devices at once */ - if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) - goto cleanup; /* If bytes were requested, or if raw mode is not forcing a MiB/s * query and cache can't prove failure, then query bytes/sec. */ @@ -2556,7 +2528,54 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) } vshPrint(ctl, "\n"); } + ret = true; + + cleanup: + return ret; +} + + +static bool +cmdBlockJob(vshControl *ctl, const vshCmd *cmd) +{ + bool ret = false; + bool raw = vshCommandOptBool(cmd, "raw"); + bool bytes = vshCommandOptBool(cmd, "bytes"); + bool abortMode = (vshCommandOptBool(cmd, "abort") || + vshCommandOptBool(cmd, "async") || + vshCommandOptBool(cmd, "pivot")); + bool infoMode = vshCommandOptBool(cmd, "info") || raw; + bool bandwidth = vshCommandOptBool(cmd, "bandwidth"); + virDomainPtr dom = NULL; + const char *path; + + if (abortMode + infoMode + bandwidth > 1) { + vshError(ctl, "%s", + _("conflict between abort, info, and bandwidth modes")); + return false; + } + /* XXX also support --bytes with bandwidth mode */ + if (bytes && (abortMode || bandwidth)) { + vshError(ctl, "%s", _("--bytes requires info mode")); + return false; + } + + if (abortMode) + return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_ABORT, NULL); + if (bandwidth) + return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_SPEED, NULL); + + /* Everything below here is for --info mode */ + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + goto cleanup; + + /* XXX Allow path to be optional to list info on all devices at once */ + if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) + goto cleanup; + + ret = vshBlockJobInfo(ctl, dom, path, raw, bytes); + cleanup: if (dom) virDomainFree(dom); -- 2.4.5
On 07/15/2015 10:33 AM, Peter Krempa wrote:
cmdBlockJob will be converted to a hub that will call into the individual executor functions. --- tools/virsh-domain.c | 91 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 55 insertions(+), 36 deletions(-)
ACK. It will be nice to finally get rid of this mis-directed multiplexing. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use the VSH_EXCLUSIVE_OPTIONS_VAR to interlock incompatible options --- tools/virsh-domain.c | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 7a18204..24f53ea 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2542,26 +2542,31 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) bool ret = false; bool raw = vshCommandOptBool(cmd, "raw"); bool bytes = vshCommandOptBool(cmd, "bytes"); - bool abortMode = (vshCommandOptBool(cmd, "abort") || - vshCommandOptBool(cmd, "async") || - vshCommandOptBool(cmd, "pivot")); - bool infoMode = vshCommandOptBool(cmd, "info") || raw; + bool abort = vshCommandOptBool(cmd, "abort"); + bool pivot = vshCommandOptBool(cmd, "pivot"); + bool async = vshCommandOptBool(cmd, "async"); + bool info = vshCommandOptBool(cmd, "info"); bool bandwidth = vshCommandOptBool(cmd, "bandwidth"); virDomainPtr dom = NULL; const char *path; - if (abortMode + infoMode + bandwidth > 1) { - vshError(ctl, "%s", - _("conflict between abort, info, and bandwidth modes")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_VAR(raw, abort); + VSH_EXCLUSIVE_OPTIONS_VAR(raw, pivot); + VSH_EXCLUSIVE_OPTIONS_VAR(raw, async); + VSH_EXCLUSIVE_OPTIONS_VAR(raw, bandwidth); + + VSH_EXCLUSIVE_OPTIONS_VAR(info, abort); + VSH_EXCLUSIVE_OPTIONS_VAR(info, pivot); + VSH_EXCLUSIVE_OPTIONS_VAR(info, async); + VSH_EXCLUSIVE_OPTIONS_VAR(info, bandwidth); + + VSH_EXCLUSIVE_OPTIONS_VAR(bytes, abort); + VSH_EXCLUSIVE_OPTIONS_VAR(bytes, pivot); + VSH_EXCLUSIVE_OPTIONS_VAR(bytes, async); /* XXX also support --bytes with bandwidth mode */ - if (bytes && (abortMode || bandwidth)) { - vshError(ctl, "%s", _("--bytes requires info mode")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_VAR(bytes, bandwidth); - if (abortMode) + if (abort || pivot || async) return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_ABORT, NULL); if (bandwidth) return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_SPEED, NULL); -- 2.4.5
On 07/15/2015 10:33 AM, Peter Krempa wrote:
Use the VSH_EXCLUSIVE_OPTIONS_VAR to interlock incompatible options --- tools/virsh-domain.c | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 7a18204..24f53ea 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2542,26 +2542,31 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) bool ret = false; bool raw = vshCommandOptBool(cmd, "raw"); bool bytes = vshCommandOptBool(cmd, "bytes"); - bool abortMode = (vshCommandOptBool(cmd, "abort") || - vshCommandOptBool(cmd, "async") || - vshCommandOptBool(cmd, "pivot")); - bool infoMode = vshCommandOptBool(cmd, "info") || raw; + bool abort = vshCommandOptBool(cmd, "abort");
Will that get you in trouble with older compilers that complain about local variables shadowing global function names? Other than that, ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Jul 20, 2015 at 15:23:15 -0600, Eric Blake wrote:
On 07/15/2015 10:33 AM, Peter Krempa wrote:
Use the VSH_EXCLUSIVE_OPTIONS_VAR to interlock incompatible options --- tools/virsh-domain.c | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 7a18204..24f53ea 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2542,26 +2542,31 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) bool ret = false; bool raw = vshCommandOptBool(cmd, "raw"); bool bytes = vshCommandOptBool(cmd, "bytes"); - bool abortMode = (vshCommandOptBool(cmd, "abort") || - vshCommandOptBool(cmd, "async") || - vshCommandOptBool(cmd, "pivot")); - bool infoMode = vshCommandOptBool(cmd, "info") || raw; + bool abort = vshCommandOptBool(cmd, "abort");
Will that get you in trouble with older compilers that complain about local variables shadowing global function names?
It probably will. We should seriously stop supporting that, but for now I'll leave it as abortMode and tweak the VSH_EXCLUSIVE_OPTIONS_VAR to VSH_EXCLUSIVE_OPTIONS in cases where 'abort' would be mentioned. Peter
--- tools/virsh-domain.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 24f53ea..e3f7220 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1666,7 +1666,6 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) typedef enum { VSH_CMD_BLOCK_JOB_ABORT, - VSH_CMD_BLOCK_JOB_SPEED, VSH_CMD_BLOCK_JOB_PULL, VSH_CMD_BLOCK_JOB_COMMIT, } vshCmdBlockJobMode; @@ -1701,10 +1700,6 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (virDomainBlockJobAbort(dom, path, flags) < 0) goto cleanup; break; - case VSH_CMD_BLOCK_JOB_SPEED: - if (virDomainBlockJobSetSpeed(dom, path, bandwidth, 0) < 0) - goto cleanup; - break; case VSH_CMD_BLOCK_JOB_PULL: if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0) goto cleanup; @@ -2537,6 +2532,26 @@ vshBlockJobInfo(vshControl *ctl, static bool +vshBlockJobSetSpeed(vshControl *ctl, + const vshCmd *cmd, + virDomainPtr dom, + const char *path) +{ + unsigned long bandwidth; + + if (vshCommandOptULWrap(ctl, cmd, "bandwidth", &bandwidth) < 0) { + vshError(ctl, "%s", _("bandwidth must be a number")); + return false; + } + + if (virDomainBlockJobSetSpeed(dom, path, bandwidth, 0) < 0) + return false; + + return true; +} + + +static bool cmdBlockJob(vshControl *ctl, const vshCmd *cmd) { bool ret = false; @@ -2568,10 +2583,7 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) if (abort || pivot || async) return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_ABORT, NULL); - if (bandwidth) - return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_SPEED, NULL); - /* Everything below here is for --info mode */ if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; @@ -2579,7 +2591,10 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) goto cleanup; - ret = vshBlockJobInfo(ctl, dom, path, raw, bytes); + if (bandwidth) + ret = vshBlockJobSetSpeed(ctl, cmd, dom, path); + else + ret = vshBlockJobInfo(ctl, dom, path, raw, bytes); cleanup: if (dom) -- 2.4.5
On 07/15/2015 12:33 PM, Peter Krempa wrote:
--- tools/virsh-domain.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 24f53ea..e3f7220 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1666,7 +1666,6 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
typedef enum { VSH_CMD_BLOCK_JOB_ABORT, - VSH_CMD_BLOCK_JOB_SPEED, VSH_CMD_BLOCK_JOB_PULL, VSH_CMD_BLOCK_JOB_COMMIT, } vshCmdBlockJobMode; @@ -1701,10 +1700,6 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (virDomainBlockJobAbort(dom, path, flags) < 0) goto cleanup; break; - case VSH_CMD_BLOCK_JOB_SPEED: - if (virDomainBlockJobSetSpeed(dom, path, bandwidth, 0) < 0) - goto cleanup; - break; case VSH_CMD_BLOCK_JOB_PULL: if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0) goto cleanup; @@ -2537,6 +2532,26 @@ vshBlockJobInfo(vshControl *ctl,
static bool +vshBlockJobSetSpeed(vshControl *ctl, + const vshCmd *cmd, + virDomainPtr dom, + const char *path) +{ + unsigned long bandwidth; + + if (vshCommandOptULWrap(ctl, cmd, "bandwidth", &bandwidth) < 0) { + vshError(ctl, "%s", _("bandwidth must be a number"));
vshCommandOptULInternal already emits an error message when ret < 0: "Numeric value '%s' for <%s> option is malformed or out of range" This is repeated in patch 5/13 and 6/13 Did you intend to have two messages? John
+ return false; + } + + if (virDomainBlockJobSetSpeed(dom, path, bandwidth, 0) < 0) + return false; + + return true; +} + + +static bool cmdBlockJob(vshControl *ctl, const vshCmd *cmd) { bool ret = false; @@ -2568,10 +2583,7 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
if (abort || pivot || async) return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_ABORT, NULL); - if (bandwidth) - return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_SPEED, NULL);
- /* Everything below here is for --info mode */ if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup;
@@ -2579,7 +2591,10 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) goto cleanup;
- ret = vshBlockJobInfo(ctl, dom, path, raw, bytes); + if (bandwidth) + ret = vshBlockJobSetSpeed(ctl, cmd, dom, path); + else + ret = vshBlockJobInfo(ctl, dom, path, raw, bytes);
cleanup: if (dom)
On Mon, Jul 20, 2015 at 16:47:38 -0400, John Ferlan wrote:
On 07/15/2015 12:33 PM, Peter Krempa wrote:
--- tools/virsh-domain.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 24f53ea..e3f7220 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1666,7 +1666,6 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
typedef enum { VSH_CMD_BLOCK_JOB_ABORT, - VSH_CMD_BLOCK_JOB_SPEED, VSH_CMD_BLOCK_JOB_PULL, VSH_CMD_BLOCK_JOB_COMMIT, } vshCmdBlockJobMode; @@ -1701,10 +1700,6 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (virDomainBlockJobAbort(dom, path, flags) < 0) goto cleanup; break; - case VSH_CMD_BLOCK_JOB_SPEED: - if (virDomainBlockJobSetSpeed(dom, path, bandwidth, 0) < 0) - goto cleanup; - break; case VSH_CMD_BLOCK_JOB_PULL: if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0) goto cleanup; @@ -2537,6 +2532,26 @@ vshBlockJobInfo(vshControl *ctl,
static bool +vshBlockJobSetSpeed(vshControl *ctl, + const vshCmd *cmd, + virDomainPtr dom, + const char *path) +{ + unsigned long bandwidth; + + if (vshCommandOptULWrap(ctl, cmd, "bandwidth", &bandwidth) < 0) { + vshError(ctl, "%s", _("bandwidth must be a number"));
vshCommandOptULInternal already emits an error message when ret < 0:
"Numeric value '%s' for <%s> option is malformed or out of range"
This is repeated in patch 5/13 and 6/13
Did you intend to have two messages?
Actually no I didn't. This patch is pretty old and pre-dates the refactor where vshCommandOptULWrap started reporting it's own error. I've just added the new argument in conflict resolution, but didn't bother checking why it was added. Thanks for pointing out. Peter
--- tools/virsh-domain.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e3f7220..98b6870 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1665,7 +1665,6 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) } typedef enum { - VSH_CMD_BLOCK_JOB_ABORT, VSH_CMD_BLOCK_JOB_PULL, VSH_CMD_BLOCK_JOB_COMMIT, } vshCmdBlockJobMode; @@ -1692,14 +1691,6 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, goto cleanup; switch (mode) { - case VSH_CMD_BLOCK_JOB_ABORT: - if (vshCommandOptBool(cmd, "async")) - flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; - if (vshCommandOptBool(cmd, "pivot")) - flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT; - if (virDomainBlockJobAbort(dom, path, flags) < 0) - goto cleanup; - break; case VSH_CMD_BLOCK_JOB_PULL: if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0) goto cleanup; @@ -2552,6 +2543,26 @@ vshBlockJobSetSpeed(vshControl *ctl, static bool +vshBlockJobAbort(virDomainPtr dom, + const char *path, + bool pivot, + bool async) +{ + unsigned int flags = 0; + + if (async) + flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; + if (pivot) + flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT; + + if (virDomainBlockJobAbort(dom, path, flags) < 0) + return false; + + return true; +} + + +static bool cmdBlockJob(vshControl *ctl, const vshCmd *cmd) { bool ret = false; @@ -2581,9 +2592,6 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) /* XXX also support --bytes with bandwidth mode */ VSH_EXCLUSIVE_OPTIONS_VAR(bytes, bandwidth); - if (abort || pivot || async) - return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_ABORT, NULL); - if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; @@ -2593,6 +2601,8 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) if (bandwidth) ret = vshBlockJobSetSpeed(ctl, cmd, dom, path); + else if (abort || pivot || async) + ret = vshBlockJobAbort(dom, path, pivot, async); else ret = vshBlockJobInfo(ctl, dom, path, raw, bytes); -- 2.4.5
--- tools/virsh-domain.c | 50 ++++++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 98b6870..4f58caa 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1665,7 +1665,6 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) } typedef enum { - VSH_CMD_BLOCK_JOB_PULL, VSH_CMD_BLOCK_JOB_COMMIT, } vshCmdBlockJobMode; @@ -1691,21 +1690,6 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, goto cleanup; switch (mode) { - case VSH_CMD_BLOCK_JOB_PULL: - if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0) - goto cleanup; - if (vshCommandOptBool(cmd, "keep-relative")) - flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE; - - if (base || flags) { - if (virDomainBlockRebase(dom, path, base, bandwidth, flags) < 0) - goto cleanup; - } else { - if (virDomainBlockPull(dom, path, bandwidth, 0) < 0) - goto cleanup; - } - - break; case VSH_CMD_BLOCK_JOB_COMMIT: if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0 || vshCommandOptStringReq(ctl, cmd, "top", &top) < 0) @@ -2681,16 +2665,31 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) struct timeval start; struct timeval curr; const char *path = NULL; + const char *base = NULL; + unsigned long bandwidth = 0; bool quit = false; int abort_flags = 0; int status = -1; int cb_id = -1; + unsigned int flags = 0; + + if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0) + return false; + + if (vshCommandOptULWrap(ctl, cmd, "bandwidth", &bandwidth) < 0) { + vshError(ctl, "%s", _("bandwidth must be a number")); + return false; + } + + if (vshCommandOptBool(cmd, "keep-relative")) + flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE; if (blocking) { if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) return false; - if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) - return false; if (vshCommandOptBool(cmd, "async")) abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; @@ -2710,6 +2709,9 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) return false; } + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + virConnectDomainEventGenericCallback cb = VIR_DOMAIN_EVENT_CALLBACK(vshBlockJobStatusHandler); @@ -2721,8 +2723,13 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) NULL)) < 0) vshResetLibvirtError(); - if (!blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_PULL, &dom)) - goto cleanup; + if (base || flags) { + if (virDomainBlockRebase(dom, path, base, bandwidth, flags) < 0) + goto cleanup; + } else { + if (virDomainBlockPull(dom, path, bandwidth, 0) < 0) + goto cleanup; + } if (!blocking) { vshPrint(ctl, "%s", _("Block Pull started")); @@ -2780,8 +2787,7 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - if (dom) - virDomainFree(dom); + virDomainFree(dom); if (blocking) sigaction(SIGINT, &old_sig_action, NULL); if (cb_id >= 0) -- 2.4.5
Final cleanup to get rid of the hub function. --- tools/virsh-domain.c | 94 +++++++++++++++++++--------------------------------- 1 file changed, 35 insertions(+), 59 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4f58caa..8b354bb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1664,60 +1664,6 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) goto cleanup; } -typedef enum { - VSH_CMD_BLOCK_JOB_COMMIT, -} vshCmdBlockJobMode; - -static bool -blockJobImpl(vshControl *ctl, const vshCmd *cmd, - vshCmdBlockJobMode mode, virDomainPtr *pdom) -{ - virDomainPtr dom = NULL; - const char *path; - unsigned long bandwidth = 0; - bool ret = false; - const char *base = NULL; - const char *top = NULL; - unsigned int flags = 0; - - if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; - - if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) - goto cleanup; - - if (vshCommandOptULWrap(ctl, cmd, "bandwidth", &bandwidth) < 0) - goto cleanup; - - switch (mode) { - case VSH_CMD_BLOCK_JOB_COMMIT: - if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0 || - vshCommandOptStringReq(ctl, cmd, "top", &top) < 0) - goto cleanup; - if (vshCommandOptBool(cmd, "shallow")) - flags |= VIR_DOMAIN_BLOCK_COMMIT_SHALLOW; - if (vshCommandOptBool(cmd, "delete")) - flags |= VIR_DOMAIN_BLOCK_COMMIT_DELETE; - if (vshCommandOptBool(cmd, "active") || - vshCommandOptBool(cmd, "pivot") || - vshCommandOptBool(cmd, "keep-overlay")) - flags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; - if (vshCommandOptBool(cmd, "keep-relative")) - flags |= VIR_DOMAIN_BLOCK_COMMIT_RELATIVE; - if (virDomainBlockCommit(dom, path, base, top, bandwidth, flags) < 0) - goto cleanup; - break; - } - - ret = true; - - cleanup: - if (pdom && ret) - *pdom = dom; - else if (dom) - virDomainFree(dom); - return ret; -} static void vshPrintJobProgress(const char *label, unsigned long long remaining, @@ -1864,10 +1810,40 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) struct timeval start; struct timeval curr; const char *path = NULL; + const char *base = NULL; + const char *top = NULL; bool quit = false; int abort_flags = 0; int status = -1; int cb_id = -1; + unsigned int flags = 0; + unsigned long bandwidth = 0; + + if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "top", &top) < 0) + return false; + + if (vshCommandOptULWrap(ctl, cmd, "bandwidth", &bandwidth) < 0) { + vshError(ctl, "%s", _("bandwidth must be a number")); + return false; + } + + if (vshCommandOptBool(cmd, "shallow")) + flags |= VIR_DOMAIN_BLOCK_COMMIT_SHALLOW; + + if (vshCommandOptBool(cmd, "delete")) + flags |= VIR_DOMAIN_BLOCK_COMMIT_DELETE; + + if (active) + flags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; + + if (vshCommandOptBool(cmd, "keep-relative")) + flags |= VIR_DOMAIN_BLOCK_COMMIT_RELATIVE; blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish; if (blocking) { @@ -1877,8 +1853,6 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) } if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) return false; - if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) - return false; if (vshCommandOptBool(cmd, "async")) abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; @@ -1897,6 +1871,9 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) return false; } + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + virConnectDomainEventGenericCallback cb = VIR_DOMAIN_EVENT_CALLBACK(vshBlockJobStatusHandler); @@ -1908,7 +1885,7 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) NULL)) < 0) vshResetLibvirtError(); - if (!blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_COMMIT, &dom)) + if (virDomainBlockCommit(dom, path, base, top, bandwidth, flags) < 0) goto cleanup; if (!blocking) { @@ -1990,8 +1967,7 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - if (dom) - virDomainFree(dom); + virDomainFree(dom); if (blocking) sigaction(SIGINT, &old_sig_action, NULL); if (cb_id >= 0) -- 2.4.5
Use the VSH_EXCLUSIVE_OPTIONS to exclude combinations of --pivot and --keep-overlay and refactor the enforcing of the --wait option and other flags that imply --wait. --- tools/virsh-domain.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 8b354bb..43eff4f 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1802,7 +1802,8 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) bool pivot = vshCommandOptBool(cmd, "pivot"); bool finish = vshCommandOptBool(cmd, "keep-overlay"); bool active = vshCommandOptBool(cmd, "active") || pivot || finish; - bool blocking = vshCommandOptBool(cmd, "wait"); + bool blocking = vshCommandOptBool(cmd, "wait") || pivot || finish; + bool async = vshCommandOptBool(cmd, "async"); int timeout = 0; struct sigaction sig_action; struct sigaction old_sig_action; @@ -1819,6 +1820,8 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0; unsigned long bandwidth = 0; + VSH_EXCLUSIVE_OPTIONS("pivot", "keep-overlay"); + if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) return false; @@ -1845,17 +1848,30 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "keep-relative")) flags |= VIR_DOMAIN_BLOCK_COMMIT_RELATIVE; - blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish; - if (blocking) { - if (pivot && finish) { - vshError(ctl, "%s", _("cannot mix --pivot and --keep-overlay")); + if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) + return false; + + if (timeout) + blocking = true; + + if (!blocking) { + if (verbose) { + vshError(ctl, "%s", _("--verbose requires at least one of --timeout, " + "--wait, --pivot, or --keep-overlay")); return false; } - if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) + + if (async) { + vshError(ctl, "%s", _("--async requires at least one of --timeout, " + "--wait, --pivot, or --keep-overlay")); return false; - if (vshCommandOptBool(cmd, "async")) - abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; + } + } + if (async) + abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; + + if (blocking) { sigemptyset(&sigmask); sigaddset(&sigmask, SIGINT); @@ -1866,9 +1882,6 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) sigaction(SIGINT, &sig_action, &old_sig_action); GETTIMEOFDAY(&start); - } else if (verbose || vshCommandOptBool(cmd, "async")) { - vshError(ctl, "%s", _("missing --wait option")); - return false; } if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) -- 2.4.5
Put all argument parsing together and refactor the argument checking code. --- tools/virsh-domain.c | 77 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 44 insertions(+), 33 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 43eff4f..8716a09 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2090,11 +2090,12 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) unsigned long long buf_size = 0; unsigned int flags = 0; bool ret = false; - bool blocking = vshCommandOptBool(cmd, "wait"); bool verbose = vshCommandOptBool(cmd, "verbose"); bool pivot = vshCommandOptBool(cmd, "pivot"); bool finish = vshCommandOptBool(cmd, "finish"); bool blockdev = vshCommandOptBool(cmd, "blockdev"); + bool blocking = vshCommandOptBool(cmd, "wait") || finish || pivot; + bool async = vshCommandOptBool(cmd, "async"); int timeout = 0; struct sigaction sig_action; struct sigaction old_sig_action; @@ -2119,22 +2120,56 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) return false; if (vshCommandOptString(ctl, cmd, "format", &format) < 0) return false; + /* XXX: Parse bandwidth as scaled input, rather than forcing + * MiB/s, and either reject negative input or treat it as 0 rather + * than trying to guess which value will work well across both + * APIs with their different sizes and scales. */ + if (vshCommandOptULWrap(ctl, cmd, "bandwidth", &bandwidth) < 0) + return false; + if (vshCommandOptUInt(ctl, cmd, "granularity", &granularity) < 0) + return false; + if (vshCommandOptULongLong(ctl, cmd, "buf-size", &buf_size) < 0) + return false; + /* Exploit that some VIR_DOMAIN_BLOCK_REBASE_* and + * VIR_DOMAIN_BLOCK_COPY_* flags have the same values. */ + if (vshCommandOptBool(cmd, "shallow")) + flags |= VIR_DOMAIN_BLOCK_REBASE_SHALLOW; + if (vshCommandOptBool(cmd, "reuse-external")) + flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT; + if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) + return false; + + if (timeout) + blocking = true; + + if (async) + abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; VSH_EXCLUSIVE_OPTIONS_VAR(dest, xml); VSH_EXCLUSIVE_OPTIONS_VAR(format, xml); VSH_EXCLUSIVE_OPTIONS_VAR(blockdev, xml); + VSH_EXCLUSIVE_OPTIONS_VAR(pivot, finish); - blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish; - if (blocking) { - if (pivot && finish) { - vshError(ctl, "%s", _("cannot mix --pivot and --finish")); + if (!dest && !xml) { + vshError(ctl, "%s", _("need either --dest or --xml")); + return false; + } + + if (!blocking) { + if (verbose) { + vshError(ctl, "%s", _("--verbose requires at least one of --timeout, " + "--wait, --pivot, or --finish")); return false; } - if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) + + if (async) { + vshError(ctl, "%s", _("--async requires at least one of --timeout, " + "--wait, --pivot, or --finish")); return false; - if (vshCommandOptBool(cmd, "async")) - abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; + } + } + if (blocking) { sigemptyset(&sigmask); sigaddset(&sigmask, SIGINT); @@ -2145,9 +2180,6 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) sigaction(SIGINT, &sig_action, &old_sig_action); GETTIMEOFDAY(&start); - } else if (verbose || vshCommandOptBool(cmd, "async")) { - vshError(ctl, "%s", _("missing --wait option")); - return false; } virConnectDomainEventGenericCallback cb = @@ -2164,34 +2196,13 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; - /* XXX: Parse bandwidth as scaled input, rather than forcing - * MiB/s, and either reject negative input or treat it as 0 rather - * than trying to guess which value will work well across both - * APIs with their different sizes and scales. */ - if (vshCommandOptULWrap(ctl, cmd, "bandwidth", &bandwidth) < 0) - goto cleanup; - if (vshCommandOptUInt(ctl, cmd, "granularity", &granularity) < 0) - goto cleanup; - if (vshCommandOptULongLong(ctl, cmd, "buf-size", &buf_size) < 0) - goto cleanup; - if (xml) { if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlstr) < 0) { vshReportError(ctl); goto cleanup; } - } else if (!dest) { - vshError(ctl, "%s", _("need either --dest or --xml")); - goto cleanup; } - /* Exploit that some VIR_DOMAIN_BLOCK_REBASE_* and - * VIR_DOMAIN_BLOCK_COPY_* flags have the same values. */ - if (vshCommandOptBool(cmd, "shallow")) - flags |= VIR_DOMAIN_BLOCK_REBASE_SHALLOW; - if (vshCommandOptBool(cmd, "reuse-external")) - flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT; - if (granularity || buf_size || (format && STRNEQ(format, "raw")) || xml) { /* New API */ if (bandwidth || granularity || buf_size) { @@ -2244,7 +2255,7 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) } else { /* Old API */ flags |= VIR_DOMAIN_BLOCK_REBASE_COPY; - if (vshCommandOptBool(cmd, "blockdev")) + if (blockdev) flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV; if (STREQ_NULLABLE(format, "raw")) flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW; -- 2.4.5
Put all argument parsing together and refactor the argument checking code. --- tools/virsh-domain.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 8716a09..ed7782e 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2658,6 +2658,7 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) bool ret = false; bool blocking = vshCommandOptBool(cmd, "wait"); bool verbose = vshCommandOptBool(cmd, "verbose"); + bool async = vshCommandOptBool(cmd, "async"); int timeout = 0; struct sigaction sig_action; struct sigaction old_sig_action; @@ -2673,6 +2674,9 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) int cb_id = -1; unsigned int flags = 0; + VSH_REQUIRE_OPTION("verbose", "wait"); + VSH_REQUIRE_OPTION("async", "wait"); + if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) return false; @@ -2684,15 +2688,16 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) return false; } + if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) + return false; + if (vshCommandOptBool(cmd, "keep-relative")) flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE; - if (blocking) { - if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) - return false; - if (vshCommandOptBool(cmd, "async")) - abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; + if (async) + abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; + if (blocking) { sigemptyset(&sigmask); sigaddset(&sigmask, SIGINT); @@ -2703,10 +2708,6 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) sigaction(SIGINT, &sig_action, &old_sig_action); GETTIMEOFDAY(&start); - } else if (verbose || vshCommandOptBool(cmd, "timeout") || - vshCommandOptBool(cmd, "async")) { - vshError(ctl, "%s", _("missing --wait option")); - return false; } if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) -- 2.4.5
Few parts of the code looked at the current progress of and assumed that a two phase blockjob is in the _READY state as soon as the progress reached 100% (info.cur == info.end). In current versions of qemu this assumption is invalid and qemu exposes a new flag 'ready' in the query-block-jobs output that is set to true if the job is actually finished. This patch adds internal data handling for reading the 'ready' flag and acting appropriately as long as the flag is present. While this still doesn't fix the virsh client problem with two phase block jobs and the --pivot option, it at least improves the error message: $ virsh blockcommit --wait --verbose vm vda --base vda[1] --active --pivot Block commit: [100 %]error: failed to pivot job for disk vda error: internal error: unable to execute QEMU command 'block-job-complete': The active block job for device 'drive-virtio-disk0' cannot be completed to $ virsh blockcommit --wait --verbose VM vda --base vda[1] --active --pivot Block commit: [100 %]error: failed to pivot job for disk vda error: block copy still active: disk 'vda' not ready for pivot yet --- src/qemu/qemu_driver.c | 10 ++++++++-- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 7 +++++++ 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f352a88..5fdeb27 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16088,8 +16088,12 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, goto cleanup; if (rc < 0) goto cleanup; - if (rc == 1 && info.cur == info.end && - info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) + if (rc == 1 && + (info.ready == 1 || + (info.ready == -1 && + info.end == info.cur && + (info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY || + info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT)))) disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; } @@ -16487,6 +16491,7 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, * hold the vm lock, so modifying the in-memory representation is * safe, even if we are a query rather than a modify job. */ if (ret == 1 && disk->mirror && + rawInfo.ready != 0 && info->cur == info->end && !disk->mirrorState) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -17117,6 +17122,7 @@ qemuDomainBlockCommit(virDomainPtr dom, * thing if the user specified a relative name). Be prepared for * a ready event to occur while locks are dropped. */ if (mirror) { + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; disk->mirror = mirror; disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 1e7b6bb..85ea822 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -797,6 +797,7 @@ struct _qemuMonitorBlockJobInfo { unsigned long long bandwidth; /* in bytes/s */ virDomainBlockJobCursor cur; virDomainBlockJobCursor end; + int ready; /* -1 if unknown, 0 if not ready, 1 if ready */ }; virHashTablePtr qemuMonitorGetAllBlockJobInfo(qemuMonitorPtr mon); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index ff568d9..5c2f50f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4027,6 +4027,7 @@ qemuMonitorJSONParseBlockJobInfo(virHashTablePtr blockJobs, qemuMonitorBlockJobInfoPtr info = NULL; const char *device; const char *type; + bool ready; if (!(device = virJSONValueObjectGetString(entry, "device"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -4042,6 +4043,9 @@ qemuMonitorJSONParseBlockJobInfo(virHashTablePtr blockJobs, return -1; } + /* assume we don't know the state */ + info->ready = -1; + if (!(type = virJSONValueObjectGetString(entry, "type"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("entry was missing 'type'")); @@ -4074,6 +4078,9 @@ qemuMonitorJSONParseBlockJobInfo(virHashTablePtr blockJobs, return -1; } + if (virJSONValueObjectGetBoolean(entry, "ready", &ready) == 0) + info->ready = ready; + return 0; } -- 2.4.5
On 07/15/2015 10:33 AM, Peter Krempa wrote:
Few parts of the code looked at the current progress of and assumed that a two phase blockjob is in the _READY state as soon as the progress reached 100% (info.cur == info.end). In current versions of qemu this assumption is invalid and qemu exposes a new flag 'ready' in the query-block-jobs output that is set to true if the job is actually finished.
This patch adds internal data handling for reading the 'ready' flag and acting appropriately as long as the flag is present.
While this still doesn't fix the virsh client problem with two phase block jobs and the --pivot option, it at least improves the error message:
$ virsh blockcommit --wait --verbose vm vda --base vda[1] --active --pivot Block commit: [100 %]error: failed to pivot job for disk vda error: internal error: unable to execute QEMU command 'block-job-complete': The active block job for device 'drive-virtio-disk0' cannot be completed
to
$ virsh blockcommit --wait --verbose VM vda --base vda[1] --active --pivot Block commit: [100 %]error: failed to pivot job for disk vda error: block copy still active: disk 'vda' not ready for pivot yet
Thanks for working on this. I think part of the problem is that newer qemu races, and can report info.cur == info.end == 0 very early in the process before starting to report non-zero info.end, but info.ready is correctly false in that point. You're right that this doesn't solve virsh racing when it sees info.cur == info.end as a potential job complete condition (where special-casing 0 then runs into problems for no-op jobs where there really is no work to do and the job can end instantly), but using the extra information from qemu does look like an improvement. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Introduce helper function that will provide logic for waiting for block job completion so the 3 open coded places can be unified and improved. This patch introduces the whole logic and uses it to fix cmdBlockJobPull. The vshBlockJobWait funtion provides common logic for block job waiting that should be robust enough to work across all previous versions of libvirt. Since virsh allows to pass user-provided strings as paths of block devices we can't reliably use block job events for detection of block job states so the function contains a great deal of fallback logic. --- tools/virsh-domain.c | 326 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 244 insertions(+), 82 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ed7782e..f1006c2 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -53,6 +53,7 @@ #include "virsh-console.h" #include "virsh-domain-monitor.h" #include "virerror.h" +#include "virtime.h" #include "virtypedparam.h" #include "virxml.h" @@ -1702,17 +1703,237 @@ static void vshCatchInt(int sig ATTRIBUTE_UNUSED, intCaught = 1; } + +typedef struct _vshBlockJobWaitData vshBlockJobWaitData; +typedef vshBlockJobWaitData *vshBlockJobWaitDataPtr; +struct _vshBlockJobWaitData { + vshControl *ctl; + virDomainPtr dom; + const char *dev; + const char *job_name; + + bool verbose; + unsigned int timeout; + bool async_abort; + + int cb_id; + int cb_id2; + int status; +}; + + static void vshBlockJobStatusHandler(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom ATTRIBUTE_UNUSED, - const char *disk ATTRIBUTE_UNUSED, + const char *disk, int type ATTRIBUTE_UNUSED, int status, void *opaque) { - *(int *) opaque = status; + vshBlockJobWaitDataPtr data = opaque; + + if (STREQ_NULLABLE(disk, data->dev)) + data->status = status; +} + + +/** + * vshBlockJobWaitInit: + * @ctl: vsh control structure + * @dom: domain object + * @dev: block device name to wait for + * @job_name: block job name to display in user-facing messages + * @verbose: enable progress reporting + * @timeout: number of milliseconds to wait before aborting the job + * @async_abort: abort the job asynchronously + * + * Prepares virsh for waiting for completion of a block job. This function + * registers event handlers for block job events and prepares the data structures + * for them. A call to vshBlockJobWait then waits for completion of the given + * block job. This function should be tolerant to different versions of daemon + * and the reporting capabilities of those. + * + * Returns the data structure that holds data needed for block job waiting or + * NULL in case of error. + */ +static vshBlockJobWaitDataPtr +vshBlockJobWaitInit(vshControl *ctl, + virDomainPtr dom, + const char *dev, + const char *job_name, + bool verbose, + unsigned int timeout, + bool async_abort) +{ + vshBlockJobWaitDataPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->ctl = ctl; + ret->dom = dom; + ret->dev = dev; + ret->job_name = job_name; + + ret->async_abort = async_abort; + ret->timeout = timeout; + ret->verbose = verbose; + + ret->status = -1; + + virConnectDomainEventGenericCallback cb = + VIR_DOMAIN_EVENT_CALLBACK(vshBlockJobStatusHandler); + + if ((ret->cb_id = virConnectDomainEventRegisterAny(ctl->conn, dom, + VIR_DOMAIN_EVENT_ID_BLOCK_JOB, + cb, ret, NULL)) < 0) + vshResetLibvirtError(); + + if ((ret->cb_id2 = virConnectDomainEventRegisterAny(ctl->conn, dom, + VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2, + cb, ret, NULL)) < 0) + vshResetLibvirtError(); + + return ret; +} + + +static void +vshBlockJobWaitFree(vshBlockJobWaitDataPtr data) +{ + if (!data) + return; + + if (data->cb_id >= 0) + virConnectDomainEventDeregisterAny(data->ctl->conn, data->cb_id); + if (data->cb_id2 >= 0) + virConnectDomainEventDeregisterAny(data->ctl->conn, data->cb_id2); + + VIR_FREE(data); +} + + +/** + * vshBlockJobWait: + * @data: private data initialized by vshBlockJobWaitInit + * + * Waits for the block job to complete. This fucntion prefers to get an event + * from libvirt but still has fallback means if the device name can't be matched + * + * This function returns values from the virConnectDomainEventBlockJobStatus enum + * or -1 in case of a internal error. Fallback states if a block job vanishes + * without triggering the event is VIR_DOMAIN_BLOCK_JOB_COMPLETED. For two phase + * jobs after the retry count for waiting for the event expires is + * VIR_DOMAIN_BLOCK_JOB_READY. + */ +static int +vshBlockJobWait(vshBlockJobWaitDataPtr data) +{ + /* For two phase jobs like active commit or block copy, the marker reaches + * 100% and an event fires. In case where virsh would not be able to match + * the event to the given block job we will wait for the number of retries + * before claiming that we entered synchronised phase */ + unsigned int retries = 5; + + struct sigaction sig_action; + struct sigaction old_sig_action; + sigset_t sigmask, oldsigmask; + + unsigned long long start = 0; + unsigned long long curr = 0; + + unsigned int abort_flags = 0; + int ret = -1; + virDomainBlockJobInfo info; + int result; + + if (!data) + return 0; + + if (data->async_abort) + abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; + + sigemptyset(&sigmask); + sigaddset(&sigmask, SIGINT); + + intCaught = 0; + sig_action.sa_sigaction = vshCatchInt; + sig_action.sa_flags = SA_SIGINFO; + sigemptyset(&sig_action.sa_mask); + sigaction(SIGINT, &sig_action, &old_sig_action); + + if (data->timeout && virTimeMillisNow(&start) < 0) { + vshSaveLibvirtError(); + return -1; + } + + while (true) { + pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask); + result = virDomainGetBlockJobInfo(data->dom, data->dev, &info, 0); + pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); + + if (result < 0) { + vshError(data->ctl, _("failed to query job for disk %s"), data->dev); + goto cleanup; + } + + /* if we've got an event for the device we are waiting for we can end + * the waiting loop */ + if ((data->cb_id >= 0 || data->cb_id2 >= 0) && data->status != -1) { + ret = data->status; + goto cleanup; + } + + /* since virsh can't guarantee that the path provided by the user will + * later be matched in the event we will need to keep the fallback + * approach and claim success if the block job finishes or vanishes. */ + if (result == 0) + break; + + /* for two-phase jobs we will try to wait in the synchronized phase + * for event arival since 100% completion doesn't necessarily mean that + * the block job has finished and can be terminated with success */ + if (info.end == info.cur && --retries == 0) { + ret = VIR_DOMAIN_BLOCK_JOB_READY; + goto cleanup; + } + + if (data->verbose) + vshPrintJobProgress(data->job_name, info.end - info.cur, info.end); + + if (data->timeout && virTimeMillisNow(&curr) < 0) { + vshSaveLibvirtError(); + goto cleanup; + } + + if (intCaught || (data->timeout && (curr - start > data->timeout))) { + if (virDomainBlockJobAbort(data->dom, data->dev, abort_flags) < 0) { + vshError(data->ctl, _("failed to abort job for disk '%s'"), + data->dev); + goto cleanup; + } + + ret = VIR_DOMAIN_BLOCK_JOB_CANCELED; + goto cleanup; + } + + usleep(500 * 1000); + } + + ret = VIR_DOMAIN_BLOCK_JOB_COMPLETED; + + cleanup: + /* print 100% completed */ + if (data->verbose && + (ret == VIR_DOMAIN_BLOCK_JOB_COMPLETED || + ret == VIR_DOMAIN_BLOCK_JOB_READY)) + vshPrintJobProgress(data->job_name, 0, 1); + + sigaction(SIGINT, &old_sig_action, NULL); + return ret; } + /* * "blockcommit" command */ @@ -2660,19 +2881,11 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) bool verbose = vshCommandOptBool(cmd, "verbose"); bool async = vshCommandOptBool(cmd, "async"); int timeout = 0; - struct sigaction sig_action; - struct sigaction old_sig_action; - sigset_t sigmask, oldsigmask; - struct timeval start; - struct timeval curr; const char *path = NULL; const char *base = NULL; unsigned long bandwidth = 0; - bool quit = false; - int abort_flags = 0; - int status = -1; - int cb_id = -1; unsigned int flags = 0; + vshBlockJobWaitDataPtr bjWait = NULL; VSH_REQUIRE_OPTION("verbose", "wait"); VSH_REQUIRE_OPTION("async", "wait"); @@ -2694,35 +2907,13 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "keep-relative")) flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE; - if (async) - abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; - - if (blocking) { - sigemptyset(&sigmask); - sigaddset(&sigmask, SIGINT); - - intCaught = 0; - sig_action.sa_sigaction = vshCatchInt; - sig_action.sa_flags = SA_SIGINFO; - sigemptyset(&sig_action.sa_mask); - sigaction(SIGINT, &sig_action, &old_sig_action); - - GETTIMEOFDAY(&start); - } - if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - virConnectDomainEventGenericCallback cb = - VIR_DOMAIN_EVENT_CALLBACK(vshBlockJobStatusHandler); - - if ((cb_id = virConnectDomainEventRegisterAny(ctl->conn, - dom, - VIR_DOMAIN_EVENT_ID_BLOCK_JOB, - cb, - &status, - NULL)) < 0) - vshResetLibvirtError(); + if (blocking && + !(bjWait = vshBlockJobWaitInit(ctl, dom, path, _("Block Pull"), verbose, + timeout, async))) + goto cleanup; if (base || flags) { if (virDomainBlockRebase(dom, path, base, bandwidth, flags) < 0) @@ -2738,61 +2929,32 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - while (blocking) { - virDomainBlockJobInfo info; - int result; - - pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask); - result = virDomainGetBlockJobInfo(dom, path, &info, 0); - pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); + /* Execution continues here only if --wait or friends were specifed */ + switch (vshBlockJobWait(bjWait)) { + case -1: + goto cleanup; - if (result < 0) { - vshError(ctl, _("failed to query job for disk %s"), path); + case VIR_DOMAIN_BLOCK_JOB_CANCELED: + vshPrint(ctl, "\n%s", _("Pull aborted")); goto cleanup; - } - if (result == 0) break; - if (verbose) - vshPrintJobProgress(_("Block Pull"), info.end - info.cur, info.end); - - GETTIMEOFDAY(&curr); - if (intCaught || (timeout && - (((int)(curr.tv_sec - start.tv_sec) * 1000 + - (int)(curr.tv_usec - start.tv_usec) / 1000) > - timeout))) { - vshDebug(ctl, VSH_ERR_DEBUG, - intCaught ? "interrupted" : "timeout"); - intCaught = 0; - timeout = 0; - status = VIR_DOMAIN_BLOCK_JOB_CANCELED; - if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { - vshError(ctl, _("failed to abort job for disk %s"), path); - goto cleanup; - } - if (abort_flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC) - break; - } else { - usleep(500 * 1000); - } - } - - if (status == VIR_DOMAIN_BLOCK_JOB_CANCELED) - quit = true; + case VIR_DOMAIN_BLOCK_JOB_FAILED: + vshPrint(ctl, "\n%s", _("Pull failed")); + goto cleanup; + break; - if (verbose && !quit) { - /* printf [100 %] */ - vshPrintJobProgress(_("Block Pull"), 0, 1); + case VIR_DOMAIN_BLOCK_JOB_READY: + case VIR_DOMAIN_BLOCK_JOB_COMPLETED: + vshPrint(ctl, "\n%s", _("Pull complete")); + break; } - vshPrint(ctl, "\n%s", quit ? _("Pull aborted") : _("Pull complete")); ret = true; + cleanup: virDomainFree(dom); - if (blocking) - sigaction(SIGINT, &old_sig_action, NULL); - if (cb_id >= 0) - virConnectDomainEventDeregisterAny(ctl->conn, cb_id); + vshBlockJobWaitFree(bjWait); return ret; } -- 2.4.5
On 07/15/2015 12:34 PM, Peter Krempa wrote:
Introduce helper function that will provide logic for waiting for block job completion so the 3 open coded places can be unified and improved.
This patch introduces the whole logic and uses it to fix cmdBlockJobPull. The vshBlockJobWait funtion provides common logic for block job waiting that should be robust enough to work across all previous versions of libvirt. Since virsh allows to pass user-provided strings as paths of block devices we can't reliably use block job events for detection of block job states so the function contains a great deal of fallback logic. --- tools/virsh-domain.c | 326 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 244 insertions(+), 82 deletions(-)
...
/* * "blockcommit" command */ @@ -2660,19 +2881,11 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) bool verbose = vshCommandOptBool(cmd, "verbose"); bool async = vshCommandOptBool(cmd, "async"); int timeout = 0; - struct sigaction sig_action; - struct sigaction old_sig_action; - sigset_t sigmask, oldsigmask; - struct timeval start; - struct timeval curr; const char *path = NULL; const char *base = NULL; unsigned long bandwidth = 0; - bool quit = false; - int abort_flags = 0; - int status = -1; - int cb_id = -1; unsigned int flags = 0; + vshBlockJobWaitDataPtr bjWait = NULL;
VSH_REQUIRE_OPTION("verbose", "wait"); VSH_REQUIRE_OPTION("async", "wait"); @@ -2694,35 +2907,13 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "keep-relative")) flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE;
- if (async) - abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; - - if (blocking) { - sigemptyset(&sigmask); - sigaddset(&sigmask, SIGINT); - - intCaught = 0; - sig_action.sa_sigaction = vshCatchInt; - sig_action.sa_flags = SA_SIGINFO; - sigemptyset(&sig_action.sa_mask); - sigaction(SIGINT, &sig_action, &old_sig_action); - - GETTIMEOFDAY(&start); - } - if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;
- virConnectDomainEventGenericCallback cb = - VIR_DOMAIN_EVENT_CALLBACK(vshBlockJobStatusHandler); - - if ((cb_id = virConnectDomainEventRegisterAny(ctl->conn, - dom, - VIR_DOMAIN_EVENT_ID_BLOCK_JOB, - cb, - &status, - NULL)) < 0) - vshResetLibvirtError(); + if (blocking && + !(bjWait = vshBlockJobWaitInit(ctl, dom, path, _("Block Pull"), verbose, + timeout, async))) + goto cleanup;
if (base || flags) { if (virDomainBlockRebase(dom, path, base, bandwidth, flags) < 0) @@ -2738,61 +2929,32 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
- while (blocking) { - virDomainBlockJobInfo info; - int result; - - pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask); - result = virDomainGetBlockJobInfo(dom, path, &info, 0); - pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); + /* Execution continues here only if --wait or friends were specifed */
specified This is also repeated in 12/13 and 13/13
+ switch (vshBlockJobWait(bjWait)) { + case -1: + goto cleanup;
- if (result < 0) { - vshError(ctl, _("failed to query job for disk %s"), path); + case VIR_DOMAIN_BLOCK_JOB_CANCELED: + vshPrint(ctl, "\n%s", _("Pull aborted")); goto cleanup; - } - if (result == 0) break;
- if (verbose) - vshPrintJobProgress(_("Block Pull"), info.end - info.cur, info.end); - - GETTIMEOFDAY(&curr); - if (intCaught || (timeout && - (((int)(curr.tv_sec - start.tv_sec) * 1000 + - (int)(curr.tv_usec - start.tv_usec) / 1000) > - timeout))) { - vshDebug(ctl, VSH_ERR_DEBUG, - intCaught ? "interrupted" : "timeout"); - intCaught = 0; - timeout = 0; - status = VIR_DOMAIN_BLOCK_JOB_CANCELED; - if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { - vshError(ctl, _("failed to abort job for disk %s"), path); - goto cleanup; - } - if (abort_flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC) - break; - } else { - usleep(500 * 1000); - } - } - - if (status == VIR_DOMAIN_BLOCK_JOB_CANCELED) - quit = true; + case VIR_DOMAIN_BLOCK_JOB_FAILED: + vshPrint(ctl, "\n%s", _("Pull failed")); + goto cleanup; + break;
- if (verbose && !quit) { - /* printf [100 %] */ - vshPrintJobProgress(_("Block Pull"), 0, 1); + case VIR_DOMAIN_BLOCK_JOB_READY: + case VIR_DOMAIN_BLOCK_JOB_COMPLETED: + vshPrint(ctl, "\n%s", _("Pull complete")); + break; } - vshPrint(ctl, "\n%s", quit ? _("Pull aborted") : _("Pull complete"));
ret = true; + cleanup: virDomainFree(dom); - if (blocking) - sigaction(SIGINT, &old_sig_action, NULL); - if (cb_id >= 0) - virConnectDomainEventDeregisterAny(ctl->conn, cb_id); + vshBlockJobWaitFree(bjWait); return ret; }
On 07/15/2015 10:34 AM, Peter Krempa wrote:
Introduce helper function that will provide logic for waiting for block job completion so the 3 open coded places can be unified and improved.
This patch introduces the whole logic and uses it to fix cmdBlockJobPull. The vshBlockJobWait funtion provides common logic for
s/funtion/function/
block job waiting that should be robust enough to work across all previous versions of libvirt. Since virsh allows to pass user-provided
s/allows to pass/allows passing/
strings as paths of block devices we can't reliably use block job events for detection of block job states so the function contains a great deal of fallback logic. --- tools/virsh-domain.c | 326 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 244 insertions(+), 82 deletions(-)
+/** + * vshBlockJobWait: + * @data: private data initialized by vshBlockJobWaitInit + * + * Waits for the block job to complete. This fucntion prefers to get an event
s/fucntion/function/
+ /* for two-phase jobs we will try to wait in the synchronized phase + * for event arival since 100% completion doesn't necessarily mean that
s/arival/arrival/
+ * the block job has finished and can be terminated with success */ + if (info.end == info.cur && --retries == 0) {
As I said in the other patch, if info.end == info.cur and they are non-zero, that SHOULD be equivalent to the job being complete (a blockpull finishing, or a two-phase job transitioning to synchronized); where it gets tricky is if the two are equal but zero (as some jobs can validly have info.end == 0, but most jobs just see info.end as 0 before getting into the main loop of the job). But it looks like you are doing okay here. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Reuse the vshBlockJobWait infrastructure to refactor cmdBlockCommit to use the common code. This additionally fixes a bug when working with new qemus, where when doing an active commit with --pivot the pivoting would fail, since qemu reaches 100% completion but the job doesn't switch to synchronized phase right away. --- tools/virsh-domain.c | 131 +++++++++++++++------------------------------------ 1 file changed, 39 insertions(+), 92 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f1006c2..0891813 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2025,19 +2025,12 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) bool active = vshCommandOptBool(cmd, "active") || pivot || finish; bool blocking = vshCommandOptBool(cmd, "wait") || pivot || finish; bool async = vshCommandOptBool(cmd, "async"); + vshBlockJobWaitDataPtr bjWait = NULL; int timeout = 0; - struct sigaction sig_action; - struct sigaction old_sig_action; - sigset_t sigmask, oldsigmask; - struct timeval start; - struct timeval curr; const char *path = NULL; const char *base = NULL; const char *top = NULL; - bool quit = false; int abort_flags = 0; - int status = -1; - int cb_id = -1; unsigned int flags = 0; unsigned long bandwidth = 0; @@ -2092,120 +2085,74 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) if (async) abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; - if (blocking) { - sigemptyset(&sigmask); - sigaddset(&sigmask, SIGINT); - - intCaught = 0; - sig_action.sa_sigaction = vshCatchInt; - sig_action.sa_flags = SA_SIGINFO; - sigemptyset(&sig_action.sa_mask); - sigaction(SIGINT, &sig_action, &old_sig_action); - - GETTIMEOFDAY(&start); - } - if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - virConnectDomainEventGenericCallback cb = - VIR_DOMAIN_EVENT_CALLBACK(vshBlockJobStatusHandler); - - if ((cb_id = virConnectDomainEventRegisterAny(ctl->conn, - dom, - VIR_DOMAIN_EVENT_ID_BLOCK_JOB, - cb, - &status, - NULL)) < 0) - vshResetLibvirtError(); + if (blocking && + !(bjWait = vshBlockJobWaitInit(ctl, dom, path, _("Block commit"), + verbose, timeout, async))) + goto cleanup; if (virDomainBlockCommit(dom, path, base, top, bandwidth, flags) < 0) goto cleanup; if (!blocking) { - vshPrint(ctl, "%s", active ? - _("Active Block Commit started") : - _("Block Commit started")); + if (active) + vshPrint(ctl, "%s", _("Active Block Commit started")); + else + vshPrint(ctl, "%s", _("Block Commit started")); + ret = true; goto cleanup; } - while (blocking) { - virDomainBlockJobInfo info; - int result; + /* Execution continues here only if --wait or friends were specifed */ + switch (vshBlockJobWait(bjWait)) { + case -1: + goto cleanup; - pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask); - result = virDomainGetBlockJobInfo(dom, path, &info, 0); - pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); + case VIR_DOMAIN_BLOCK_JOB_CANCELED: + vshPrint(ctl, "\n%s", _("Commit aborted")); + goto cleanup; + break; - if (result < 0) { - vshError(ctl, _("failed to query job for disk %s"), path); + case VIR_DOMAIN_BLOCK_JOB_FAILED: + vshPrint(ctl, "\n%s", _("Commit failed")); goto cleanup; - } - if (result == 0) break; - if (verbose) - vshPrintJobProgress(_("Block Commit"), - info.end - info.cur, info.end); - if (active && info.cur == info.end) + case VIR_DOMAIN_BLOCK_JOB_READY: + case VIR_DOMAIN_BLOCK_JOB_COMPLETED: break; + } - GETTIMEOFDAY(&curr); - if (intCaught || (timeout && - (((int)(curr.tv_sec - start.tv_sec) * 1000 + - (int)(curr.tv_usec - start.tv_usec) / 1000) > - timeout))) { - vshDebug(ctl, VSH_ERR_DEBUG, - intCaught ? "interrupted" : "timeout"); - intCaught = 0; - timeout = 0; - status = VIR_DOMAIN_BLOCK_JOB_CANCELED; + if (active) { + if (pivot) { + abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT; if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { - vshError(ctl, _("failed to abort job for disk %s"), path); + vshError(ctl, _("failed to pivot job for disk %s"), path); goto cleanup; } - if (abort_flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC) - break; - } else { - usleep(500 * 1000); - } - } - if (status == VIR_DOMAIN_BLOCK_JOB_CANCELED) - quit = true; + vshPrint(ctl, "\n%s", _("Successfully pivoted")); + } else if (finish) { + if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to finish job for disk %s"), path); + goto cleanup; + } - if (verbose && !quit) { - /* printf [100 %] */ - vshPrintJobProgress(_("Block Commit"), 0, 1); - } - if (!quit && pivot) { - abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT; - if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { - vshError(ctl, _("failed to pivot job for disk %s"), path); - goto cleanup; + vshPrint(ctl, "\n%s", _("Commit complete, overlay image kept")); + } else { + vshPrint(ctl, "\n%s", _("Now in synchronized phase")); } - } else if (finish && !quit && - virDomainBlockJobAbort(dom, path, abort_flags) < 0) { - vshError(ctl, _("failed to finish job for disk %s"), path); - goto cleanup; - } - if (quit) - vshPrint(ctl, "\n%s", _("Commit aborted")); - else if (pivot) - vshPrint(ctl, "\n%s", _("Successfully pivoted")); - else if (!finish && active) - vshPrint(ctl, "\n%s", _("Now in synchronized phase")); - else + } else { vshPrint(ctl, "\n%s", _("Commit complete")); + } ret = true; cleanup: virDomainFree(dom); - if (blocking) - sigaction(SIGINT, &old_sig_action, NULL); - if (cb_id >= 0) - virConnectDomainEventDeregisterAny(ctl->conn, cb_id); + vshBlockJobWaitFree(bjWait); return ret; } -- 2.4.5
Similarly to the refactor of cmdBlockCommit in a previous commit this does the same change for cmdBlockCopy. --- tools/virsh-domain.c | 119 ++++++++++++++------------------------------------- 1 file changed, 31 insertions(+), 88 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0891813..117cc55 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2265,20 +2265,13 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) bool blocking = vshCommandOptBool(cmd, "wait") || finish || pivot; bool async = vshCommandOptBool(cmd, "async"); int timeout = 0; - struct sigaction sig_action; - struct sigaction old_sig_action; - sigset_t sigmask, oldsigmask; - struct timeval start; - struct timeval curr; const char *path = NULL; - bool quit = false; int abort_flags = 0; const char *xml = NULL; char *xmlstr = NULL; virTypedParameterPtr params = NULL; + vshBlockJobWaitDataPtr bjWait = NULL; int nparams = 0; - int status = -1; - int cb_id = -1; if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) return false; @@ -2337,33 +2330,14 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) } } - if (blocking) { - sigemptyset(&sigmask); - sigaddset(&sigmask, SIGINT); - - intCaught = 0; - sig_action.sa_sigaction = vshCatchInt; - sig_action.sa_flags = SA_SIGINFO; - sigemptyset(&sig_action.sa_mask); - sigaction(SIGINT, &sig_action, &old_sig_action); - - GETTIMEOFDAY(&start); - } - - virConnectDomainEventGenericCallback cb = - VIR_DOMAIN_EVENT_CALLBACK(vshBlockJobStatusHandler); - - if ((cb_id = virConnectDomainEventRegisterAny(ctl->conn, - dom, - VIR_DOMAIN_EVENT_ID_BLOCK_JOB, - cb, - &status, - NULL)) < 0) - vshResetLibvirtError(); - if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; + if (blocking && + !(bjWait = vshBlockJobWaitInit(ctl, dom, path, _("Block Copy"), verbose, + timeout, async))) + goto cleanup; + if (xml) { if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlstr) < 0) { vshReportError(ctl); @@ -2438,83 +2412,52 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - while (blocking) { - virDomainBlockJobInfo info; - int result; - - pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask); - result = virDomainGetBlockJobInfo(dom, path, &info, 0); - pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); - - if (result < 0) { - vshError(ctl, _("failed to query job for disk %s"), path); + /* Execution continues here only if --wait or friends were specifed */ + switch (vshBlockJobWait(bjWait)) { + case -1: goto cleanup; - } - if (result == 0) { - vshError(ctl, _("Block Copy unexpectedly failed")); + case VIR_DOMAIN_BLOCK_JOB_CANCELED: + vshPrint(ctl, "\n%s", _("Copy aborted")); goto cleanup; - } + break; - if (verbose) - vshPrintJobProgress(_("Block Copy"), info.end - info.cur, info.end); - if (info.cur == info.end) + case VIR_DOMAIN_BLOCK_JOB_FAILED: + vshPrint(ctl, "\n%s", _("Copy failed")); + goto cleanup; break; - GETTIMEOFDAY(&curr); - if (intCaught || (timeout && - (((int)(curr.tv_sec - start.tv_sec) * 1000 + - (int)(curr.tv_usec - start.tv_usec) / 1000) > - timeout))) { - vshDebug(ctl, VSH_ERR_DEBUG, - intCaught ? "interrupted" : "timeout"); - intCaught = 0; - timeout = 0; - status = VIR_DOMAIN_BLOCK_JOB_CANCELED; - if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { - vshError(ctl, _("failed to abort job for disk %s"), path); - goto cleanup; - } - if (abort_flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC) - break; - } else { - usleep(500 * 1000); - } + case VIR_DOMAIN_BLOCK_JOB_READY: + case VIR_DOMAIN_BLOCK_JOB_COMPLETED: + break; } - if (status == VIR_DOMAIN_BLOCK_JOB_CANCELED) - quit = true; - - if (!quit && pivot) { + if (pivot) { abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT; if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { vshError(ctl, _("failed to pivot job for disk %s"), path); goto cleanup; } - } else if (finish && !quit && - virDomainBlockJobAbort(dom, path, abort_flags) < 0) { - vshError(ctl, _("failed to finish job for disk %s"), path); - goto cleanup; - } - if (quit) - vshPrint(ctl, "\n%s", _("Copy aborted")); - else if (pivot) + vshPrint(ctl, "\n%s", _("Successfully pivoted")); - else if (finish) + } else if (finish) { + if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to finish job for disk %s"), path); + goto cleanup; + } + vshPrint(ctl, "\n%s", _("Successfully copied")); - else + } else { vshPrint(ctl, "\n%s", _("Now in mirroring phase")); + } ret = true; + cleanup: VIR_FREE(xmlstr); virTypedParamsFree(params, nparams); - if (dom) - virDomainFree(dom); - if (blocking) - sigaction(SIGINT, &old_sig_action, NULL); - if (cb_id >= 0) - virConnectDomainEventDeregisterAny(ctl->conn, cb_id); + virDomainFree(dom); + vshBlockJobWaitFree(bjWait); return ret; } -- 2.4.5
On 07/15/2015 12:33 PM, Peter Krempa wrote:
This series refactors the block job command code so that it is handled by separate functions rather than one mega-method. Additionally this series then fixes the routine for waiting for a block job.
As it's perhaps obvious from the patches the API design for the block job APIs is rather unfortunate for users.
This series contains a few patches that already were submitted and partially reviewed previously.
Peter Krempa (13): virsh: blockjob: Extract block job info code into a separate function virsh: cmdBlockJob: Switch to declarative flag interlocking virsh: blockjob: Split out vshBlockJobSetSpeed from blockJobImpl virsh: block job: separate abort from blockJobImpl virsh: Split out block pull implementation from blockJobImpl virsh: Kill blockJobImpl by moving the final impl into cmdBlockCommit virsh: Refactor argument checking in cmdBlockCommit virsh: Refactor argument handling in cmdBlockCopy virsh: Refactor argument handling in cmdBlockPull qemu: Update state of block job to READY only if it actually is ready virsh: Refactor block job waiting in cmdBlockPull virsh: Refactor block job waiting in cmdBlockCommit virsh: Refactor block job waiting in cmdBlockCopy
src/qemu/qemu_driver.c | 10 +- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 7 + tools/virsh-domain.c | 962 ++++++++++++++++++++++++------------------- 4 files changed, 551 insertions(+), 429 deletions(-)
Noted a couple of minor things, but looks good otherwise. ACK series, John
On 07/20/2015 02:48 PM, John Ferlan wrote:
On 07/15/2015 12:33 PM, Peter Krempa wrote:
This series refactors the block job command code so that it is handled by separate functions rather than one mega-method. Additionally this series then fixes the routine for waiting for a block job.
As it's perhaps obvious from the patches the API design for the block job APIs is rather unfortunate for users.
This series contains a few patches that already were submitted and partially reviewed previously.
Noted a couple of minor things, but looks good otherwise.
and I pointed out a few more
ACK series,
Concur. This makes virsh look a lot nicer, and the big benefit is that it tries to address the data race bug where live block-commit --pivot could read status too early and fail (probably because qemu was reporting info.end of 0 before actually getting into the main loop of the job). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Jul 20, 2015 at 15:49:46 -0600, Eric Blake wrote:
On 07/20/2015 02:48 PM, John Ferlan wrote:
On 07/15/2015 12:33 PM, Peter Krempa wrote:
This series refactors the block job command code so that it is handled by separate functions rather than one mega-method. Additionally this series then fixes the routine for waiting for a block job.
As it's perhaps obvious from the patches the API design for the block job APIs is rather unfortunate for users.
This series contains a few patches that already were submitted and partially reviewed previously.
Noted a couple of minor things, but looks good otherwise.
and I pointed out a few more
ACK series,
Concur. This makes virsh look a lot nicer, and the big benefit is that it tries to address the data race bug where live block-commit --pivot could read status too early and fail (probably because qemu was reporting info.end of 0 before actually getting into the main loop of the job).
Qemu was actually reporting 100% completion (non-zero) for a brief moment before the event is delivered. If you try to pivot the job at that point you'll get the error message too. Peter
On Mon, Jul 20, 2015 at 16:48:01 -0400, John Ferlan wrote:
On 07/15/2015 12:33 PM, Peter Krempa wrote:
This series refactors the block job command code so that it is handled by separate functions rather than one mega-method. Additionally this series then fixes the routine for waiting for a block job.
As it's perhaps obvious from the patches the API design for the block job APIs is rather unfortunate for users.
This series contains a few patches that already were submitted and partially reviewed previously.
Peter Krempa (13): virsh: blockjob: Extract block job info code into a separate function virsh: cmdBlockJob: Switch to declarative flag interlocking virsh: blockjob: Split out vshBlockJobSetSpeed from blockJobImpl virsh: block job: separate abort from blockJobImpl virsh: Split out block pull implementation from blockJobImpl virsh: Kill blockJobImpl by moving the final impl into cmdBlockCommit virsh: Refactor argument checking in cmdBlockCommit virsh: Refactor argument handling in cmdBlockCopy virsh: Refactor argument handling in cmdBlockPull qemu: Update state of block job to READY only if it actually is ready virsh: Refactor block job waiting in cmdBlockPull virsh: Refactor block job waiting in cmdBlockCommit virsh: Refactor block job waiting in cmdBlockCopy
src/qemu/qemu_driver.c | 10 +- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 7 + tools/virsh-domain.c | 962 ++++++++++++++++++++++++------------------- 4 files changed, 551 insertions(+), 429 deletions(-)
Noted a couple of minor things, but looks good otherwise.
ACK series,
I've fixed all the issues pointed out by you and Eric and pushed this series. Thanks. Peter
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa