Re: [Qemu-devel] [PATCH 1/5] migration: split hmp_savevm to do_savevm and hmp_savevm wrapper

[Denis V. Lunev]

On 12/24/2015 12:27 AM, Eric Blake wrote:
> On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> > This would be useful in the next step when QMP version of this call will
> > be introduced.
> >
> > Signed-off-by: Denis V. Lunev <address@hidden>
> > Reviewed-by: Juan Quintela <address@hidden>
> > CC: Amit Shah <address@hidden>
> > CC: Markus Armbruster <address@hidden>
> > CC: Eric Blake <address@hidden>
> > ---
> >   migration/savevm.c | 38 +++++++++++++++++++++++---------------
> >   1 file changed, 23 insertions(+), 15 deletions(-)
> >
> > @@ -1915,28 +1915,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
> >       uint64_t vm_state_size;
> >       qemu_timeval tv;
> >       struct tm tm;
> > -    const char *name = qdict_get_try_str(qdict, "name");
> >       Error *local_err = NULL;
> >       AioContext *aio_context;
> >   
> >       if (!bdrv_all_can_snapshot(&bs)) {
> > -        monitor_printf(mon, "Device '%s' is writable but does not "
> > -                       "support snapshots.\n", bdrv_get_device_name(bs));
> > +        error_setg(errp,
> > +                   "Device '%s' is writable but does not support snapshots.",
> No trailing '.' in error_setg() calls.
>
> > +                   bdrv_get_device_name(bs));
> >           return;
> >       }
> >   
> >       /* Delete old snapshots of the same name */
> >       if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
> > -        monitor_printf(mon,
> > -                       "Error while deleting snapshot on device '%s': %s\n",
> > -                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
> > +        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
> > +                   bdrv_get_device_name(bs1), error_get_pretty(local_err));
> Markus' series to add a prefixing notation would be better to use here
> (although I didn't check if he caught this one in that series already):
> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html

this series is not yet merged. I think that we could do this refactoring 
later on.
This thing could be considered independent. Anyway, this series has its 
own value
and it takes a lot of time to push it in. Could we do  error setting 
improvement later on?

Messages are not changed etc. I'll change them as you suggest.


> >   
> > +void hmp_savevm(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *local_err = NULL;
> > +
> > +    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);
> > +
> > +    if (local_err != NULL) {
> I would have just written 'if (local_err) {'; but that's minor style.
from my point of view explicit != NULL exposes that local_err is a
pointer rather than a boolean value.

> Looks like a clean refactoring, other than the nit about the trailing
> '.', so with that fixed:
> Reviewed-by: Eric Blake <address@hidden>