[libvirt] [PATCH v3 0/4] virshBlockJobWait improvements
v2 discussion at: http://www.redhat.com/archives/libvir-list/2016-January/msg01063.html As requested I've split this into separate commits. All of the comment nitpicks have been applied. With regards to using "break" or "goto cleanup" to exit the loop, I decided to use "goto" on all the error cases and "break" everywhere else. I had noticed another bug where the SIGINT signal action wasn't reset if the virTimeMillisNow() call failed; that's fixed in patch 3 in this series. Michael Chapman (4): virsh: avoid unnecessary progress updates virsh: be consistent with style of loop exit virsh: ensure SIGINT action is reset on all errors virsh: improve waiting for block job readiness tools/virsh-domain.c | 65 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 28 deletions(-) -- 2.4.3
There is no need to call virshPrintJobProgress() unless the block job's cur or end cursors have changed since the last iteration. Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- tools/virsh-domain.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c2146d2..83de02a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1837,7 +1837,7 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data) unsigned int abort_flags = 0; int ret = -1; - virDomainBlockJobInfo info; + virDomainBlockJobInfo info, last; int result; if (!data) @@ -1860,6 +1860,8 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data) return -1; } + last.cur = last.end = 0; + while (true) { pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask); result = virDomainGetBlockJobInfo(data->dom, data->dev, &info, 0); @@ -1891,9 +1893,10 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data) goto cleanup; } - if (data->verbose) + if (data->verbose && (info.cur != last.cur || info.end != last.end)) virshPrintJobProgress(data->job_name, info.end - info.cur, info.end); + last = info; if (data->timeout && virTimeMillisNow(&curr) < 0) { vshSaveLibvirtError(); -- 2.4.3
On Wed, Jan 27, 2016 at 13:24:51 +1100, Michael Chapman wrote:
There is no need to call virshPrintJobProgress() unless the block job's cur or end cursors have changed since the last iteration.
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- tools/virsh-domain.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
ACK
When waiting for a block job, the various statuses (COMPLETED, READY, CANCELED, etc.) should all be treated consistently by having the loop be exited with "break". Use "goto cleanup" for the error cases only, when no block job status is available. Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- tools/virsh-domain.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 83de02a..cdeccac 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1876,21 +1876,23 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data) * the waiting loop */ if ((data->cb_id >= 0 || data->cb_id2 >= 0) && data->status != -1) { ret = data->status; - goto cleanup; + break; } /* 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) + if (result == 0) { + ret = VIR_DOMAIN_BLOCK_JOB_COMPLETED; break; + } /* for two-phase jobs we will try to wait in the synchronized phase * for event arrival 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; + break; } if (data->verbose && (info.cur != last.cur || info.end != last.end)) @@ -1911,21 +1913,19 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data) } ret = VIR_DOMAIN_BLOCK_JOB_CANCELED; - goto cleanup; + break; } 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)) virshPrintJobProgress(data->job_name, 0, 1); + cleanup: sigaction(SIGINT, &old_sig_action, NULL); return ret; } -- 2.4.3
On Wed, Jan 27, 2016 at 13:24:52 +1100, Michael Chapman wrote:
When waiting for a block job, the various statuses (COMPLETED, READY, CANCELED, etc.) should all be treated consistently by having the loop be exited with "break". Use "goto cleanup" for the error cases only, when no block job status is available.
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- tools/virsh-domain.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
ACK
If virTimeMillisNow() fails, the SIGINT action must be reset back to its previous state. Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cdeccac..750b273 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1857,7 +1857,7 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data) if (data->timeout && virTimeMillisNow(&start) < 0) { vshSaveLibvirtError(); - return -1; + goto cleanup; } last.cur = last.end = 0; -- 2.4.3
On Wed, Jan 27, 2016 at 13:24:53 +1100, Michael Chapman wrote:
If virTimeMillisNow() fails, the SIGINT action must be reset back to its previous state.
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
After a block job hits 100%, we only need to apply a timeout waiting for a block job event if exactly one of the BLOCK_JOB or BLOCK_JOB_2 callbacks were able to be registered. If neither callback could be registered, there's clearly no need for a timeout. If both callbacks were registered, then we're guaranteed to eventually get one of the events. The path being used by virsh must be exactly the source path or target device in the domain's disk definition, and these are the respective strings sent back in these two events. Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- 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 750b273..bf65a60 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1810,14 +1810,17 @@ virshBlockJobWaitFree(virshBlockJobWaitDataPtr data) * virshBlockJobWait: * @data: private data initialized by virshBlockJobWaitInit * - * Waits for the block job to complete. This function prefers to get an event - * from libvirt but still has fallback means if the device name can't be matched + * Waits for the block job to complete. This function prefers to wait for a + * matching VIR_DOMAIN_EVENT_ID_BLOCK_JOB or VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2 + * event from libvirt; however, it has a fallback mode should either of these + * events not be available. * - * 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. + * This function returns values from the virConnectDomainEventBlockJobStatus + * enum or -1 in case of an internal error. + * + * If the fallback mode is activated the returned event is + * VIR_DOMAIN_BLOCK_JOB_COMPLETED if the block job vanishes or + * VIR_DOMAIN_BLOCK_JOB_READY if the block job reaches 100%. */ static int virshBlockJobWait(virshBlockJobWaitDataPtr data) @@ -1872,27 +1875,30 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data) goto cleanup; } - /* if we've got an event for the device we are waiting for we can end - * the waiting loop */ + /* If either callback could be registered and we've got an event, we can + * can end the waiting loop */ if ((data->cb_id >= 0 || data->cb_id2 >= 0) && data->status != -1) { ret = data->status; break; } - /* 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) { - ret = VIR_DOMAIN_BLOCK_JOB_COMPLETED; - break; - } + /* Fallback behaviour is only needed if one or both callbacks could not + * be registered */ + if (data->cb_id < 0 || data->cb_id2 < 0) { + /* If the block job vanishes, synthesize a COMPLETED event */ + if (result == 0) { + ret = VIR_DOMAIN_BLOCK_JOB_COMPLETED; + break; + } - /* for two-phase jobs we will try to wait in the synchronized phase - * for event arrival 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; - break; + /* If the block job hits 100%, wait a little while for a possible + * event from libvirt unless both callbacks could not be registered + * in order to synthesize our own READY event */ + if (info.end == info.cur && + ((data->cb_id < 0 && data->cb_id2 < 0) || --retries == 0)) { + ret = VIR_DOMAIN_BLOCK_JOB_READY; + break; + } } if (data->verbose && (info.cur != last.cur || info.end != last.end)) -- 2.4.3
On Wed, Jan 27, 2016 at 13:24:54 +1100, Michael Chapman wrote:
After a block job hits 100%, we only need to apply a timeout waiting for a block job event if exactly one of the BLOCK_JOB or BLOCK_JOB_2 callbacks were able to be registered.
If neither callback could be registered, there's clearly no need for a timeout.
If both callbacks were registered, then we're guaranteed to eventually get one of the events. The path being used by virsh must be exactly the source path or target device in the domain's disk definition, and these are the respective strings sent back in these two events.
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- tools/virsh-domain.c | 50 ++++++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 22 deletions(-)
ACK, I had to test this one a bit to get the desired reproducibility. I've also re-verified the claim that block job name will be matched only to the extent to what libvirt returns in the event, thus this change should be okay. I'll push the series in a while. Thanks for keeping through review. Peter
participants (2)
-
Michael Chapman -
Peter Krempa