hmp.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
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
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
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
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
>
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
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
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.
© 2016 - 2026 Red Hat, Inc.