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 >> > > >
© 2016 - 2024 Red Hat, Inc.