[Qemu-devel] [PATCH] hmp: delvm: use hmp_handle_error

[Qemu-devel] [PATCH] hmp: delvm: use hmp_handle_error

[Cole Robinson]

This gives us the consistent 'Error:' prefix added in 66363e9a43f,
which helps users like libvirt who still need to scrape hmp error
messages to detect failure.

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 hmp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hmp.c b/hmp.c
index 8eec768088..74a4bfc1f9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1481,10 +1481,11 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
     const char *name = qdict_get_str(qdict, "name");
 
     if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
-        error_reportf_err(err,
-                          "Error while deleting snapshot on device '%s': ",
-                          bdrv_get_device_name(bs));
+        error_prepend(&err,
+                      "Error while deleting snapshot on device '%s': ",
+                      bdrv_get_device_name(bs));
     }
+    hmp_handle_error(mon, &err);
 }
 
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
-- 
2.21.0

Re: [Qemu-devel] [PATCH for-4.0?] hmp: delvm: use hmp_handle_error

[Eric Blake]

On 4/10/19 1:03 PM, Cole Robinson wrote:
> This gives us the consistent 'Error:' prefix added in 66363e9a43f,
> which helps users like libvirt who still need to scrape hmp error
> messages to detect failure.
> 
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
>  hmp.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Not enough to drive -rc4 on its own, but worth adding to our wishlist of
potential easy patches if we do have a release blocker surface.

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/hmp.c b/hmp.c
> index 8eec768088..74a4bfc1f9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1481,10 +1481,11 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
>      const char *name = qdict_get_str(qdict, "name");
>  
>      if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
> -        error_reportf_err(err,
> -                          "Error while deleting snapshot on device '%s': ",
> -                          bdrv_get_device_name(bs));
> +        error_prepend(&err,
> +                      "Error while deleting snapshot on device '%s': ",

Do we want to reword the message (maybe s/Error while //) to avoid the
word "Error" twice in the same line?

> +                      bdrv_get_device_name(bs));
>      }
> +    hmp_handle_error(mon, &err);
>  }
>  
>  void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-4.0?] hmp: delvm: use hmp_handle_error

[Eric Blake]

On 4/10/19 1:03 PM, Cole Robinson wrote:
> This gives us the consistent 'Error:' prefix added in 66363e9a43f,
> which helps users like libvirt who still need to scrape hmp error
> messages to detect failure.
> 
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
>  hmp.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Not enough to drive -rc4 on its own, but worth adding to our wishlist of
potential easy patches if we do have a release blocker surface.

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/hmp.c b/hmp.c
> index 8eec768088..74a4bfc1f9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1481,10 +1481,11 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
>      const char *name = qdict_get_str(qdict, "name");
>  
>      if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
> -        error_reportf_err(err,
> -                          "Error while deleting snapshot on device '%s': ",
> -                          bdrv_get_device_name(bs));
> +        error_prepend(&err,
> +                      "Error while deleting snapshot on device '%s': ",

Do we want to reword the message (maybe s/Error while //) to avoid the
word "Error" twice in the same line?

> +                      bdrv_get_device_name(bs));
>      }
> +    hmp_handle_error(mon, &err);
>  }
>  
>  void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-4.0?] hmp: delvm: use hmp_handle_error

[Cole Robinson]

On 4/10/19 2:27 PM, Eric Blake wrote:
> On 4/10/19 1:03 PM, Cole Robinson wrote:
>> This gives us the consistent 'Error:' prefix added in 66363e9a43f,
>> which helps users like libvirt who still need to scrape hmp error
>> messages to detect failure.
>>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>> ---
>>  hmp.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Not enough to drive -rc4 on its own, but worth adding to our wishlist of
> potential easy patches if we do have a release blocker surface.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>>
>> diff --git a/hmp.c b/hmp.c
>> index 8eec768088..74a4bfc1f9 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1481,10 +1481,11 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
>>      const char *name = qdict_get_str(qdict, "name");
>>  
>>      if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
>> -        error_reportf_err(err,
>> -                          "Error while deleting snapshot on device '%s': ",
>> -                          bdrv_get_device_name(bs));
>> +        error_prepend(&err,
>> +                      "Error while deleting snapshot on device '%s': ",
> 
> Do we want to reword the message (maybe s/Error while //) to avoid the
> word "Error" twice in the same line?
> 

I'm fine with that, and I checked it won't affect libvirt's error
scraping in this area

Thanks,
Cole

Re: [Qemu-devel] [PATCH for-4.0?] hmp: delvm: use hmp_handle_error

[Cole Robinson]

On 4/10/19 2:27 PM, Eric Blake wrote:
> On 4/10/19 1:03 PM, Cole Robinson wrote:
>> This gives us the consistent 'Error:' prefix added in 66363e9a43f,
>> which helps users like libvirt who still need to scrape hmp error
>> messages to detect failure.
>>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>> ---
>>  hmp.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Not enough to drive -rc4 on its own, but worth adding to our wishlist of
> potential easy patches if we do have a release blocker surface.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>>
>> diff --git a/hmp.c b/hmp.c
>> index 8eec768088..74a4bfc1f9 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1481,10 +1481,11 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
>>      const char *name = qdict_get_str(qdict, "name");
>>  
>>      if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
>> -        error_reportf_err(err,
>> -                          "Error while deleting snapshot on device '%s': ",
>> -                          bdrv_get_device_name(bs));
>> +        error_prepend(&err,
>> +                      "Error while deleting snapshot on device '%s': ",
> 
> Do we want to reword the message (maybe s/Error while //) to avoid the
> word "Error" twice in the same line?
> 

I'm fine with that, and I checked it won't affect libvirt's error
scraping in this area

Thanks,
Cole

Re: [Qemu-devel] [PATCH for-4.0?] hmp: delvm: use hmp_handle_error

[Kevin Wolf]

Am 10.04.2019 um 20:27 hat Eric Blake geschrieben:
> On 4/10/19 1:03 PM, Cole Robinson wrote:
> > This gives us the consistent 'Error:' prefix added in 66363e9a43f,
> > which helps users like libvirt who still need to scrape hmp error
> > messages to detect failure.
> > 
> > Signed-off-by: Cole Robinson <crobinso@redhat.com>
> > ---
> >  hmp.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Not enough to drive -rc4 on its own, but worth adding to our wishlist of
> potential easy patches if we do have a release blocker surface.

As we are going to have an -rc4, I had a look at this.

Commit 66363e9a43f was in February and explicitly says "Note: Some
places don't use hmp_handle_error". So this doesn't seem to be a
regression and even if it's fixed, it's likely not the last place that
doesn't use the "Error:" prefix. This would suggest that this isn't for
-rc4.

Am I misunderstanding the situation?

Kevin

> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> > 
> > diff --git a/hmp.c b/hmp.c
> > index 8eec768088..74a4bfc1f9 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1481,10 +1481,11 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
> >      const char *name = qdict_get_str(qdict, "name");
> >  
> >      if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
> > -        error_reportf_err(err,
> > -                          "Error while deleting snapshot on device '%s': ",
> > -                          bdrv_get_device_name(bs));
> > +        error_prepend(&err,
> > +                      "Error while deleting snapshot on device '%s': ",
> 
> Do we want to reword the message (maybe s/Error while //) to avoid the
> word "Error" twice in the same line?
> 
> > +                      bdrv_get_device_name(bs));
> >      }
> > +    hmp_handle_error(mon, &err);
> >  }
> >  
> >  void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 

Re: [Qemu-devel] [PATCH for-4.0?] hmp: delvm: use hmp_handle_error

[Kevin Wolf]

Am 10.04.2019 um 20:27 hat Eric Blake geschrieben:
> On 4/10/19 1:03 PM, Cole Robinson wrote:
> > This gives us the consistent 'Error:' prefix added in 66363e9a43f,
> > which helps users like libvirt who still need to scrape hmp error
> > messages to detect failure.
> > 
> > Signed-off-by: Cole Robinson <crobinso@redhat.com>
> > ---
> >  hmp.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Not enough to drive -rc4 on its own, but worth adding to our wishlist of
> potential easy patches if we do have a release blocker surface.

As we are going to have an -rc4, I had a look at this.

Commit 66363e9a43f was in February and explicitly says "Note: Some
places don't use hmp_handle_error". So this doesn't seem to be a
regression and even if it's fixed, it's likely not the last place that
doesn't use the "Error:" prefix. This would suggest that this isn't for
-rc4.

Am I misunderstanding the situation?

Kevin

> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> > 
> > diff --git a/hmp.c b/hmp.c
> > index 8eec768088..74a4bfc1f9 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1481,10 +1481,11 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
> >      const char *name = qdict_get_str(qdict, "name");
> >  
> >      if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
> > -        error_reportf_err(err,
> > -                          "Error while deleting snapshot on device '%s': ",
> > -                          bdrv_get_device_name(bs));
> > +        error_prepend(&err,
> > +                      "Error while deleting snapshot on device '%s': ",
> 
> Do we want to reword the message (maybe s/Error while //) to avoid the
> word "Error" twice in the same line?
> 
> > +                      bdrv_get_device_name(bs));
> >      }
> > +    hmp_handle_error(mon, &err);
> >  }
> >  
> >  void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 

Re: [Qemu-devel] [PATCH for-4.0?] hmp: delvm: use hmp_handle_error

[Eric Blake]

On 4/12/19 7:21 AM, Kevin Wolf wrote:
> Am 10.04.2019 um 20:27 hat Eric Blake geschrieben:
>> On 4/10/19 1:03 PM, Cole Robinson wrote:
>>> This gives us the consistent 'Error:' prefix added in 66363e9a43f,
>>> which helps users like libvirt who still need to scrape hmp error
>>> messages to detect failure.
>>>
>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>> ---
>>>  hmp.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> Not enough to drive -rc4 on its own, but worth adding to our wishlist of
>> potential easy patches if we do have a release blocker surface.
> 
> As we are going to have an -rc4, I had a look at this.
> 
> Commit 66363e9a43f was in February and explicitly says "Note: Some
> places don't use hmp_handle_error". So this doesn't seem to be a
> regression and even if it's fixed, it's likely not the last place that
> doesn't use the "Error:" prefix. This would suggest that this isn't for
> -rc4.
> 
> Am I misunderstanding the situation?

No, I think your read is accurate, and delaying this to 4.1 is okay.


>>> +        error_prepend(&err,
>>> +                      "Error while deleting snapshot on device '%s': ",
>>
>> Do we want to reword the message (maybe s/Error while //) to avoid the
>> word "Error" twice in the same line?

especially since we still would want this resolved via a v2, rather than
taking this patch as-is.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-4.0?] hmp: delvm: use hmp_handle_error

[Eric Blake]

On 4/12/19 7:21 AM, Kevin Wolf wrote:
> Am 10.04.2019 um 20:27 hat Eric Blake geschrieben:
>> On 4/10/19 1:03 PM, Cole Robinson wrote:
>>> This gives us the consistent 'Error:' prefix added in 66363e9a43f,
>>> which helps users like libvirt who still need to scrape hmp error
>>> messages to detect failure.
>>>
>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>> ---
>>>  hmp.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> Not enough to drive -rc4 on its own, but worth adding to our wishlist of
>> potential easy patches if we do have a release blocker surface.
> 
> As we are going to have an -rc4, I had a look at this.
> 
> Commit 66363e9a43f was in February and explicitly says "Note: Some
> places don't use hmp_handle_error". So this doesn't seem to be a
> regression and even if it's fixed, it's likely not the last place that
> doesn't use the "Error:" prefix. This would suggest that this isn't for
> -rc4.
> 
> Am I misunderstanding the situation?

No, I think your read is accurate, and delaying this to 4.1 is okay.


>>> +        error_prepend(&err,
>>> +                      "Error while deleting snapshot on device '%s': ",
>>
>> Do we want to reword the message (maybe s/Error while //) to avoid the
>> word "Error" twice in the same line?

especially since we still would want this resolved via a v2, rather than
taking this patch as-is.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-4.0?] hmp: delvm: use hmp_handle_error

[Cole Robinson]

On 4/12/19 10:55 AM, Eric Blake wrote:
> On 4/12/19 7:21 AM, Kevin Wolf wrote:
>> Am 10.04.2019 um 20:27 hat Eric Blake geschrieben:
>>> On 4/10/19 1:03 PM, Cole Robinson wrote:
>>>> This gives us the consistent 'Error:' prefix added in 66363e9a43f,
>>>> which helps users like libvirt who still need to scrape hmp error
>>>> messages to detect failure.
>>>>
>>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>>> ---
>>>>  hmp.c | 7 ++++---
>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> Not enough to drive -rc4 on its own, but worth adding to our wishlist of
>>> potential easy patches if we do have a release blocker surface.
>>
>> As we are going to have an -rc4, I had a look at this.
>>
>> Commit 66363e9a43f was in February and explicitly says "Note: Some
>> places don't use hmp_handle_error". So this doesn't seem to be a
>> regression and even if it's fixed, it's likely not the last place that
>> doesn't use the "Error:" prefix. This would suggest that this isn't for
>> -rc4.
>>
>> Am I misunderstanding the situation?
> 
> No, I think your read is accurate, and delaying this to 4.1 is okay.
>

Yup, this isn't really fixing any specific thing in libvirt, just a bit
of future proofing

> 
>>>> +        error_prepend(&err,
>>>> +                      "Error while deleting snapshot on device '%s': ",
>>>
>>> Do we want to reword the message (maybe s/Error while //) to avoid the
>>> word "Error" twice in the same line?
> 
> especially since we still would want this resolved via a v2, rather than
> taking this patch as-is.
> 
> 

I'll send a v2 after 4.0 is out

Thanks,
Cole

Re: [Qemu-devel] [PATCH for-4.0?] hmp: delvm: use hmp_handle_error

[Cole Robinson]

On 4/12/19 10:55 AM, Eric Blake wrote:
> On 4/12/19 7:21 AM, Kevin Wolf wrote:
>> Am 10.04.2019 um 20:27 hat Eric Blake geschrieben:
>>> On 4/10/19 1:03 PM, Cole Robinson wrote:
>>>> This gives us the consistent 'Error:' prefix added in 66363e9a43f,
>>>> which helps users like libvirt who still need to scrape hmp error
>>>> messages to detect failure.
>>>>
>>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>>> ---
>>>>  hmp.c | 7 ++++---
>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> Not enough to drive -rc4 on its own, but worth adding to our wishlist of
>>> potential easy patches if we do have a release blocker surface.
>>
>> As we are going to have an -rc4, I had a look at this.
>>
>> Commit 66363e9a43f was in February and explicitly says "Note: Some
>> places don't use hmp_handle_error". So this doesn't seem to be a
>> regression and even if it's fixed, it's likely not the last place that
>> doesn't use the "Error:" prefix. This would suggest that this isn't for
>> -rc4.
>>
>> Am I misunderstanding the situation?
> 
> No, I think your read is accurate, and delaying this to 4.1 is okay.
>

Yup, this isn't really fixing any specific thing in libvirt, just a bit
of future proofing

> 
>>>> +        error_prepend(&err,
>>>> +                      "Error while deleting snapshot on device '%s': ",
>>>
>>> Do we want to reword the message (maybe s/Error while //) to avoid the
>>> word "Error" twice in the same line?
> 
> especially since we still would want this resolved via a v2, rather than
> taking this patch as-is.
> 
> 

I'll send a v2 after 4.0 is out

Thanks,
Cole

Re: [Qemu-devel] [PATCH] hmp: delvm: use hmp_handle_error

[Markus Armbruster]

Cole Robinson <crobinso@redhat.com> writes:

> This gives us the consistent 'Error:' prefix added in 66363e9a43f,
> which helps users like libvirt who still need to scrape hmp error
> messages to detect failure.
>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
>  hmp.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 8eec768088..74a4bfc1f9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1481,10 +1481,11 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
>      const char *name = qdict_get_str(qdict, "name");
>  
>      if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
> -        error_reportf_err(err,
> -                          "Error while deleting snapshot on device '%s': ",
> -                          bdrv_get_device_name(bs));
> +        error_prepend(&err,
> +                      "Error while deleting snapshot on device '%s': ",
> +                      bdrv_get_device_name(bs));
>      }
> +    hmp_handle_error(mon, &err);
>  }
>  
>  void hmp_info_snapshots(Monitor *mon, const QDict *qdict)

No objection to this patch, just apropos hmp_handle_error().

HMP command handlers look like this:

    void hmp_FOO(Monitor *mon, const QDict *qdict)

They can report errors however they like.  The monitor core has no
notion of HMP command failure.

Commonly, hmp_FOO() wraps around some qmp_FOO(), or some helper(s) it
shares with qmp_FOO().  These will return errors through an Error **
argument.  The sane way for hmp_FOO() to report them is with
hmp_handle_error().

In other words, we get an hmp_handle_error() on most[*] failure paths.

Why not move it into the monitor core?

    bool hmp_FOO(Monitor *mon, const QDict *qdict, Error **errp)

While at it, ditch the @mon parameter, because it's always cur_mon
anyway:

    bool hmp_FOO(const QDict *qdict, Error **errp)


[*] Common exceptions are failures in code that add convenience over
QMP.  These need not produce an Error object.  Instead, they may report
with error_report(), or even monitor_printf().  The latter would be in
bad taste.