Re: [Qemu-devel] [PATCH v4 02/11] block: add error parameter to del_existing_snapshots()

[Luiz Capitulino]

On Tue, 09 Apr 2013 15:27:32 +0200
Markus Armbruster <address@hidden> wrote:

> Pavel Hrdina <address@hidden> writes:
> 
> > Signed-off-by: Pavel Hrdina <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > ---
> >  savevm.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/savevm.c b/savevm.c
> > index 77c5291..dc1f4a4 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -2224,7 +2224,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, 
> > QEMUSnapshotInfo *sn_info,
> >  /*
> >   * Deletes snapshots of a given name in all opened images.
> >   */
> > -static int del_existing_snapshots(Monitor *mon, const char *name)
> > +static int del_existing_snapshots(const char *name, Error **errp)
> >  {
> >      BlockDriverState *bs;
> >      QEMUSnapshotInfo sn1, *snapshot = &sn1;
> > @@ -2237,9 +2237,8 @@ static int del_existing_snapshots(Monitor *mon, const 
> > char *name)
> >          {
> >              ret = bdrv_snapshot_delete(bs, name);
> >              if (ret < 0) {
> > -                monitor_printf(mon,
> > -                               "Error while deleting snapshot on '%s'\n",
> > -                               bdrv_get_device_name(bs));
> > +                error_setg(errp, "error while deleting snapshot on '%s'",
> > +                           bdrv_get_device_name(bs));
> >                  return -1;
> >              }
> >          }
> 
> Any particular reason for de-capitalizing "Error"?
> 
> > @@ -2259,6 +2258,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> >      qemu_timeval tv;
> >      struct tm tm;
> >      const char *name = qdict_get_try_str(qdict, "name");
> > +    Error *local_err = NULL;
> >  
> >      /* Verify if there is a device that doesn't support snapshots and is 
> > writable */
> >      bs = NULL;
> > @@ -2307,7 +2307,9 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> >      }
> >  
> >      /* Delete old snapshots of the same name */
> > -    if (name && del_existing_snapshots(mon, name) < 0) {
> > +    if (name && del_existing_snapshots(name, &local_err) < 0) {
> > +        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
> > +        error_free(local_err);
> >          goto the_end;
> >      }
> 
> Could use hmp_handle_error(), except it's static in hmp.c.  On the other
> hand, even hmp.c doesn't always use it...  Luiz, what do you think?

I've added hmp_handle_error() to avoid copy & paste in hmp.c, but I don't
think it's a good function because it doesn't actually handle the error
and it does two unrelated things (print the error and free it).

So, I think it's better to restrict it to hmp.c.