Re: [PATCH v2 03/11] migration: Make migration json writer part of MigrationState struct

[Daniel P . Berrangé]

On Wed, Oct 19, 2022 at 06:43:46PM +0300, Nikolay Borisov wrote:
> 
> 
> On 18.10.22 г. 13:06 ч., Daniel P. Berrangé wrote:
> > On Mon, Oct 10, 2022 at 04:34:00PM +0300, Nikolay Borisov wrote:
> > > This is required so that migration stream configuration is written
> > > to the migration stream. This would allow analyze-migration to
> > > parse enabled capabilities for the migration and adjust its behavior
> > > accordingly. This is in preparation for analyze-migration.py to support
> > > 'fixed-ram' capability format changes.
> > > 
> > > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > > ---
> > >   migration/migration.c |  5 +++++
> > >   migration/migration.h |  3 +++
> > >   migration/savevm.c    | 38 ++++++++++++++++++++++----------------
> > >   3 files changed, 30 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index 140b0f1a54bd..d0779bbaf862 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -1896,6 +1896,8 @@ static void migrate_fd_cleanup(MigrationState *s)
> > >       g_free(s->hostname);
> > >       s->hostname = NULL;
> > > +    json_writer_free(s->vmdesc);
> > > +
> > >       qemu_savevm_state_cleanup();
> > >       if (s->to_dst_file) {
> > > @@ -2154,6 +2156,7 @@ void migrate_init(MigrationState *s)
> > >       error_free(s->error);
> > >       s->error = NULL;
> > >       s->hostname = NULL;
> > > +    s->vmdesc = NULL;
> > >       migrate_set_state(&s->state, MIGRATION_STATUS_NONE, 
> > > MIGRATION_STATUS_SETUP);
> > > @@ -4269,6 +4272,8 @@ void migrate_fd_connect(MigrationState *s, Error 
> > > *error_in)
> > >           return;
> > >       }
> > > +    s->vmdesc = json_writer_new(false);
> > > +
> > >       if (multifd_save_setup(&local_err) != 0) {
> > >           error_report_err(local_err);
> > >           migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> > > diff --git a/migration/migration.h b/migration/migration.h
> > > index cdad8aceaaab..96f27aba2210 100644
> > > --- a/migration/migration.h
> > > +++ b/migration/migration.h
> > > @@ -17,6 +17,7 @@
> > >   #include "exec/cpu-common.h"
> > >   #include "hw/qdev-core.h"
> > >   #include "qapi/qapi-types-migration.h"
> > > +#include "qapi/qmp/json-writer.h"
> > >   #include "qemu/thread.h"
> > >   #include "qemu/coroutine_int.h"
> > >   #include "io/channel.h"
> > > @@ -261,6 +262,8 @@ struct MigrationState {
> > >       int state;
> > > +    JSONWriter *vmdesc;
> > > +
> > >       /* State related to return path */
> > >       struct {
> > >           /* Protected by qemu_file_lock */
> > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > index 48e85c052c2c..174cdbefc29d 100644
> > > --- a/migration/savevm.c
> > > +++ b/migration/savevm.c
> > > @@ -1137,13 +1137,18 @@ void qemu_savevm_non_migratable_list(strList 
> > > **reasons)
> > >   void qemu_savevm_state_header(QEMUFile *f)
> > >   {
> > > +    MigrationState *s = migrate_get_current();
> > >       trace_savevm_state_header();
> > >       qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
> > >       qemu_put_be32(f, QEMU_VM_FILE_VERSION);
> > > -    if (migrate_get_current()->send_configuration) {
> > > +    if (s->send_configuration) {
> > >           qemu_put_byte(f, QEMU_VM_CONFIGURATION);
> > > -        vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
> > > + json_writer_start_object(s->vmdesc, NULL);
> > > + json_writer_start_object(s->vmdesc, "configuration");
> > > +        vmstate_save_state(f, &vmstate_configuration, &savevm_state, 
> > > s->vmdesc);
> > > + json_writer_end_object(s->vmdesc);
> > > +
> > 
> > IIUC, this is changing the info that is written in the VM
> > configuration section, by adding an extra level of nesting
> > to the object.
> > 
> > Isn't this going to cause backwards compatibility problems ?
> > 
> > Nothing in the patch seems to take account of the exctra
> > 'configuiration' object that has been started
> 
> The resulting json looks like:
> 
> {
>     "configuration": {
>         "vmsd_name": "configuration",
>         "version": 1,
>         "fields": [
>             {
>                 "name": "len",
>                 "type": "uint32",
>                 "size": 4
>             },
>             {
>                 "name": "name",
>                 "type": "buffer",
>                 "size": 13
>             }
>         ],
>         "subsections": [
>             {
>                 "vmsd_name": "configuration/capabilities",
>                 "version": 1,
>                 "fields": [
>                     {
>                         "name": "caps_count",
>                         "type": "uint32",
>                         "size": 4
>                     },
>                     {
>                         "name": "capabilities",
>                         "type": "capability",
>                         "size": 10
>                     }
>                 ]
>             }
>         ]
>     },
>     "page_size": 4096,
>     "devices": [
>         {
>             "name": "timer",
>             "instance_id": 0,
> //ommitted
> 
> So the "configuration" object is indeed added, but older versions of qemu
> can ignore it without any problem.

IIUC, after looking further, this JSON is only used by the
analyze-migration.py script ?  If that's right, then we should
have the change to analyze-migration.py that copes with the
"configuration" option in the same patch.

> > Also, there's two  json_writer_start_object calls, but only
> > one json_writer_end_object.
> 
> That's intentional, the first one begins the top-level object and it is
> actually paired with the final call to json_writer_end_object(s->vmdesc); in
> qemu_savevm_state_complete_precopy_non_iterable .

Oh right, can you add a comment to both locations, mentioning
where their respective pair lives.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|