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

[Denis V. Lunev]

On 01/18/2016 06:47 PM, Markus Armbruster wrote:
> "Denis V. Lunev" <address@hidden> writes:
>
> > This would be useful in the next step when QMP version of this call will
> > be introduced. The patch also moves snapshot name generation to the
> > hmp specific code as QMP version of this code will require the name
> > on the protocol level.
> >
> > Addition of migration_savevm to migration/migration.h is temporary. It
> > will be removed in the next patch with QMP level change.
> >
> > Signed-off-by: Denis V. Lunev <address@hidden>
> > CC: Juan Quintela <address@hidden>
> > CC: Eric Blake <address@hidden>
> > CC: Amit Shah <address@hidden>
> > CC: Markus Armbruster <address@hidden>
> > ---
> >   hmp.c                         | 26 ++++++++++++++++++++++++
> >   include/migration/migration.h |  3 +++
> >   migration/savevm.c            | 46 +++++++++++++++++--------------------------
> >   3 files changed, 47 insertions(+), 28 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index c2b2c16..e7bab75 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -18,6 +18,7 @@
> >   #include "net/eth.h"
> >   #include "sysemu/char.h"
> >   #include "sysemu/block-backend.h"
> > +#include "sysemu/sysemu.h"
> >   #include "qemu/option.h"
> >   #include "qemu/timer.h"
> >   #include "qmp-commands.h"
> > @@ -32,6 +33,7 @@
> >   #include "ui/console.h"
> >   #include "block/qapi.h"
> >   #include "qemu-io.h"
> > +#include "migration/migration.h"
> >   
> >   #ifdef CONFIG_SPICE
> >   #include <spice/enums.h>
> > @@ -2386,3 +2388,27 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict 
> > *qdict)
> >   
> >       qapi_free_RockerOfDpaGroupList(list);
> >   }
> > +
> > +void hmp_savevm(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *local_err = NULL;
> > +    const char *name = qdict_get_try_str(qdict, "name");
> > +    char name_buf[64];
> > +
> > +    if (name == NULL) {
> I'd prefer !name.
>
> > +        qemu_timeval tv;
> > +        struct tm tm;
> > +
> > +        /* cast below needed for OpenBSD where tv_sec is still 'long' */
> > +        localtime_r((const time_t *)&tv.tv_sec, &tm);
> I'm afraid this uses tv.tv_sec uninitialized.  Not sure how this
> survived testing :)
the name is generated :) somehow....
OK, this should be definitely fixed

> Aside: the ugly cast comes from commit d7d9b52.  It works when the
> system conforms to POSIX (tv_sec is time_t), or when tv_sec's type is
> essentially the same as time_t (supposedly the case in old OpenBSD).  As
> far as I can tell, OpenBSD was fixed in 2013.
this is copied from the original code. I do not think
that this should be fixed in this commit. We could
make additional commit removing this ugly cast.
I don't want to have this patch reverted if something
goes wrong during merge.

> > +        strftime(name_buf, sizeof(name_buf), "vm-%Y%m%d%H%M%S", &tm);
> > +
> > +        name = name_buf;
> > +    }
> > +
> > +    migrate_savevm(name, &local_err);
> > +
> > +    if (local_err != NULL) {
> > +        error_report_err(local_err);
> > +    }
> > +}
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index d9494b8..73c8bb1 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -277,6 +277,8 @@ int migrate_compress_threads(void);
> >   int migrate_decompress_threads(void);
> >   bool migrate_use_events(void);
> >   
> > +void migrate_savevm(const char *name, Error **errp);
> > +
> >   /* Sending on the return path - generic and then for each message type */
> >   void migrate_send_rp_message(MigrationIncomingState *mis,
> >                                enum mig_rp_message_type message_type,
> > @@ -321,4 +323,5 @@ int ram_save_queue_pages(MigrationState *ms, const char 
> > *rbname,
> >   PostcopyState postcopy_state_get(void);
> >   /* Set the state and return the old state */
> >   PostcopyState postcopy_state_set(PostcopyState new_state);
> > +
> >   #endif
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 0ad1b93..308302a 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
> >       return ret;
> >   }
> >   
> > -void hmp_savevm(Monitor *mon, const QDict *qdict)
> > +void migrate_savevm(const char *name, Error **errp)
> >   {
> >       BlockDriverState *bs, *bs1;
> >       QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> > @@ -1914,29 +1914,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
> >       int saved_vm_running;
> >       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",
> > +                   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));
> > +    if (bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
> > +        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
> > +                   bdrv_get_device_name(bs1), error_get_pretty(local_err));
> Before the patch, we delete snapshots only when name comes from the
> user, not when we made it up.
>
> After the patch, we delete always.
>
> What happens before the patch when we make up a name, a snapshot with
> this name exists, but we don't delete it?
I think that we will have problems later on in f.e. qcow2_snapshot_create

> Should deleting old snapshots move to the HMP command as well?

actually I don't want to. In this case we will have to add here the
code to check that there are no snapshots with this name on
all states. This will place us into the ugly loop:
- hmp_info_snapshots reports snapshot names which are available
  on ALL drives only
- thus if the snapshot resides on one image out of four we will not
  be able to detect this in advace

I think that we should remove here and this is correct.

> >           error_free(local_err);
> >           return;
> >       }
> >   
> >       bs = bdrv_all_find_vmstate_bs();
> >       if (bs == NULL) {
> > -        monitor_printf(mon, "No block device can accept snapshots\n");
> > +        error_setg(errp, "No block device can accept snapshots");
> >           return;
> >       }
> >       aio_context = bdrv_get_aio_context(bs);
> > @@ -1945,7 +1943,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
> >   
> >       ret = global_state_store();
> >       if (ret) {
> > -        monitor_printf(mon, "Error saving global state\n");
> > +        error_setg(errp, "Error saving global state");
> >           return;
> >       }
> >       vm_stop(RUN_STATE_SAVE_VM);
> > @@ -1960,39 +1958,31 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
> >       sn->date_nsec = tv.tv_usec * 1000;
> >       sn->vm_clock_nsec = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> >   
> > -    if (name) {
> > -        ret = bdrv_snapshot_find(bs, old_sn, name);
> > -        if (ret >= 0) {
> > -            pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> > -            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
> > -        } else {
> > -            pstrcpy(sn->name, sizeof(sn->name), name);
> > -        }
> > +    ret = bdrv_snapshot_find(bs, old_sn, name);
> > +    if (ret >= 0) {
> > +        pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> > +        pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
> >       } else {
> > -        /* cast below needed for OpenBSD where tv_sec is still 'long' */
> > -        localtime_r((const time_t *)&tv.tv_sec, &tm);
> > -        strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
> > +        pstrcpy(sn->name, sizeof(sn->name), name);
> >       }
> Another place where we now use the code for "name from user" for made-up
> name as well.  Not sure about the impact.
this resides in HMP only. I hope that original functionality is preserved.

> >   
> >       /* save the VM state */
> >       f = qemu_fopen_bdrv(bs, 1);
> >       if (!f) {
> > -        monitor_printf(mon, "Could not open VM state file\n");
> > +        error_setg(errp, "Could not open VM state file");
> >           goto the_end;
> >       }
> > -    ret = qemu_savevm_state(f, &local_err);
> > +    ret = qemu_savevm_state(f, errp);
> >       vm_state_size = qemu_ftell(f);
> >       qemu_fclose(f);
> >       if (ret < 0) {
> > -        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
> > -        error_free(local_err);
> >           goto the_end;
> >       }
> >   
> >       ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
> >       if (ret < 0) {
> > -        monitor_printf(mon, "Error while creating snapshot on '%s'\n",
> > -                       bdrv_get_device_name(bs));
> > +        error_setg(errp, "Error while creating snapshot on '%s'",
> > +                   bdrv_get_device_name(bs));
> >       }
> >   
> >    the_end: