[PATCH] virsh: cmdBlockcopy: Remove 'error:' prefix for an empty line

Peter Krempa posted 1 patch 3 years, 9 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1769e7a60440b2d4663a5c8797becae8700a4ca5.1592396698.git.pkrempa@redhat.com
tools/virsh-domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] virsh: cmdBlockcopy: Remove 'error:' prefix for an empty line
Posted by Peter Krempa 3 years, 9 months ago
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

Re: [PATCH] virsh: cmdBlockcopy: Remove 'error:' prefix for an empty line
Posted by Michal Privoznik 3 years, 9 months ago
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

Re: [PATCH] virsh: cmdBlockcopy: Remove 'error:' prefix for an empty line
Posted by Ján Tomko 3 years, 9 months ago
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
>
Re: [PATCH] virsh: cmdBlockcopy: Remove 'error:' prefix for an empty line
Posted by Peter Krempa 3 years, 9 months ago
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
> > 


Re: [PATCH] virsh: cmdBlockcopy: Remove 'error:' prefix for an empty line
Posted by Ján Tomko 3 years, 9 months ago
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
>> >
>
>