[PATCH] virsh: cmdBlockcopy: Remove 'error:' prefix for an empty line
When a block copy job fails prior to reaching the synchronized phase while we are waiting for the job to finish virsh would print the following: $ virsh blockcopy backup-test vda /tmp/dst.qcow2 --wait --reuse-external --transient-job error: Copy failed The above message looks like we've forgot to print the error message itself as the line ends after 'error:'. Unfortunately with the current API design clients have no way of actually getting the error message as the VIR_DOMAIN_EVENT_ID_BLOCK_JOB(_2) event only reports the status but not an error and the job then vanishes. Fix the expectations by using vshPrintExtra instead of vshError: $ virsh blockcopy backup-test vda /tmp/dst.qcow2 --wait --reuse-external --transient-job Copy failed Note that the newline is required to avoid printing the 'Copy failed' message on the same line when printing the job progress percentage. Inspired by https://bugzilla.redhat.com/show_bug.cgi?id=1847867 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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 5222949566..3597fb6272 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2457,7 +2457,7 @@ cmdBlockcopy(vshControl *ctl, const vshCmd *cmd) break; case VIR_DOMAIN_BLOCK_JOB_FAILED: - vshError(ctl, "\n%s", _("Copy failed")); + vshPrintExtra(ctl, "\n%s", _("Copy failed")); goto cleanup; break; -- 2.26.2
On 6/17/20 2:24 PM, Peter Krempa wrote:
When a block copy job fails prior to reaching the synchronized phase while we are waiting for the job to finish virsh would print the following:
$ virsh blockcopy backup-test vda /tmp/dst.qcow2 --wait --reuse-external --transient-job error: Copy failed
The above message looks like we've forgot to print the error message itself as the line ends after 'error:'. Unfortunately with the current API design clients have no way of actually getting the error message as the VIR_DOMAIN_EVENT_ID_BLOCK_JOB(_2) event only reports the status but not an error and the job then vanishes.
Fix the expectations by using vshPrintExtra instead of vshError:
$ virsh blockcopy backup-test vda /tmp/dst.qcow2 --wait --reuse-external --transient-job
Copy failed
Note that the newline is required to avoid printing the 'Copy failed' message on the same line when printing the job progress percentage.
Inspired by https://bugzilla.redhat.com/show_bug.cgi?id=1847867
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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 5222949566..3597fb6272 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2457,7 +2457,7 @@ cmdBlockcopy(vshControl *ctl, const vshCmd *cmd) break;
case VIR_DOMAIN_BLOCK_JOB_FAILED: - vshError(ctl, "\n%s", _("Copy failed")); + vshPrintExtra(ctl, "\n%s", _("Copy failed")); goto cleanup; break;
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On a Wednesday in 2020, Peter Krempa wrote:
When a block copy job fails prior to reaching the synchronized phase while we are waiting for the job to finish virsh would print the following:
$ virsh blockcopy backup-test vda /tmp/dst.qcow2 --wait --reuse-external --transient-job error: Copy failed
The above message looks like we've forgot to print the error message itself as the line ends after 'error:'. Unfortunately with the current API design clients have no way of actually getting the error message as the VIR_DOMAIN_EVENT_ID_BLOCK_JOB(_2) event only reports the status but not an error and the job then vanishes.
Fix the expectations by using vshPrintExtra instead of vshError:
$ virsh blockcopy backup-test vda /tmp/dst.qcow2 --wait --reuse-external --transient-job
Copy failed
Note that the newline is required to avoid printing the 'Copy failed' message on the same line when printing the job progress percentage.
Inspired by https://bugzilla.redhat.com/show_bug.cgi?id=1847867
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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 5222949566..3597fb6272 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2457,7 +2457,7 @@ cmdBlockcopy(vshControl *ctl, const vshCmd *cmd) break;
case VIR_DOMAIN_BLOCK_JOB_FAILED: - vshError(ctl, "\n%s", _("Copy failed")); + vshPrintExtra(ctl, "\n%s", _("Copy failed"));
If the newline is only needed after the percentage, we should print it based on the verbose parameter, like virshBlockJobWait does with the percentage. Even though we don't have a meaningful error, we should write something generic instead of quietly exiting (although with an error code). Also, the other two functions calling virshBlockJobWait seem to be affected. Jano
goto cleanup; break;
-- 2.26.2
On Wed, Jun 17, 2020 at 14:52:38 +0200, Ján Tomko wrote:
On a Wednesday in 2020, Peter Krempa wrote:
When a block copy job fails prior to reaching the synchronized phase while we are waiting for the job to finish virsh would print the following:
$ virsh blockcopy backup-test vda /tmp/dst.qcow2 --wait --reuse-external --transient-job error: Copy failed
The above message looks like we've forgot to print the error message itself as the line ends after 'error:'. Unfortunately with the current API design clients have no way of actually getting the error message as the VIR_DOMAIN_EVENT_ID_BLOCK_JOB(_2) event only reports the status but not an error and the job then vanishes.
Fix the expectations by using vshPrintExtra instead of vshError:
$ virsh blockcopy backup-test vda /tmp/dst.qcow2 --wait --reuse-external --transient-job
Copy failed
Note that the newline is required to avoid printing the 'Copy failed' message on the same line when printing the job progress percentage.
Inspired by https://bugzilla.redhat.com/show_bug.cgi?id=1847867
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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 5222949566..3597fb6272 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2457,7 +2457,7 @@ cmdBlockcopy(vshControl *ctl, const vshCmd *cmd) break;
case VIR_DOMAIN_BLOCK_JOB_FAILED: - vshError(ctl, "\n%s", _("Copy failed")); + vshPrintExtra(ctl, "\n%s", _("Copy failed"));
If the newline is only needed after the percentage, we should print it based on the verbose parameter, like virshBlockJobWait does with the percentage.
Do we really care that much to add pointless eye-candy code?
Even though we don't have a meaningful error, we should write something generic instead of quietly exiting (although with an error code).
Well we print "Copy failed". I don't really have any better idea what to print at this point when the error is not passed to the client.
Also, the other two functions calling virshBlockJobWait seem to be affected.
Jano
goto cleanup; break;
-- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
On Wed, Jun 17, 2020 at 14:52:38 +0200, Ján Tomko wrote:
On a Wednesday in 2020, Peter Krempa wrote:
When a block copy job fails prior to reaching the synchronized phase while we are waiting for the job to finish virsh would print the following:
$ virsh blockcopy backup-test vda /tmp/dst.qcow2 --wait --reuse-external --transient-job error: Copy failed
The above message looks like we've forgot to print the error message itself as the line ends after 'error:'. Unfortunately with the current API design clients have no way of actually getting the error message as the VIR_DOMAIN_EVENT_ID_BLOCK_JOB(_2) event only reports the status but not an error and the job then vanishes.
Fix the expectations by using vshPrintExtra instead of vshError:
$ virsh blockcopy backup-test vda /tmp/dst.qcow2 --wait --reuse-external --transient-job
Copy failed
Note that the newline is required to avoid printing the 'Copy failed' message on the same line when printing the job progress percentage.
Inspired by https://bugzilla.redhat.com/show_bug.cgi?id=1847867
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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 5222949566..3597fb6272 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2457,7 +2457,7 @@ cmdBlockcopy(vshControl *ctl, const vshCmd *cmd) break;
case VIR_DOMAIN_BLOCK_JOB_FAILED: - vshError(ctl, "\n%s", _("Copy failed")); + vshPrintExtra(ctl, "\n%s", _("Copy failed"));
If the newline is only needed after the percentage, we should print it based on the verbose parameter, like virshBlockJobWait does with the percentage.
Do we really care that much to add pointless eye-candy code?
That's the question you have to ask yourself. I don't really care about anything lately.
Even though we don't have a meaningful error, we should write something generic instead of quietly exiting (although with an error code).
Well we print "Copy failed". I don't really have any better idea what to print at this point when the error is not passed to the client.
Unlike vshError, vshPrintExtra does not print anything in quiet mode. Jano
Also, the other two functions calling virshBlockJobWait seem to be affected.
Jano
goto cleanup; break;
-- 2.26.2
participants (3)
-
Ján Tomko -
Michal Privoznik -
Peter Krempa