| Message ID | f61645d0b3720a039423532d49dd65588d9bccd8.1554919328.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hmp: delvm: use hmp_handle_error | expand |
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) >
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.
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.
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)
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(-)