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

[Nikolay Borisov]

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.


> 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 .

> BTW, some <tab> crept into this patch.

Will fix this.

PS. I usually work on the linux kernel so vim used my linuxsty.vim 
settings. However, I eventually instsalled .editorconfig support so I 
guess those are leftovers.
> >       }
> >   }
> >   
> > @@ -1364,15 +1369,16 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> >                                                       bool in_postcopy,
> >                                                       bool inactivate_disks)
> >   {
> > -    g_autoptr(JSONWriter) vmdesc = NULL;
> > +    MigrationState *s = migrate_get_current();
> >       int vmdesc_len;
> >       SaveStateEntry *se;
> >       int ret;
> >   
> > -    vmdesc = json_writer_new(false);
> > -    json_writer_start_object(vmdesc, NULL);
> > -    json_writer_int64(vmdesc, "page_size", qemu_target_page_size());
> > -    json_writer_start_array(vmdesc, "devices");
> > +    if (!s->send_configuration) {
> > +           json_writer_start_object(s->vmdesc, NULL);
> > +    }
> > +    json_writer_int64(s->vmdesc, "page_size", qemu_target_page_size());
> > +    json_writer_start_array(s->vmdesc, "devices");
> >       QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> >   
> >           if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
> > @@ -1385,12 +1391,12 @@ int 
> > qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> >   
> >           trace_savevm_section_start(se->idstr, se->section_id);
> >   
> > -        json_writer_start_object(vmdesc, NULL);
> > -        json_writer_str(vmdesc, "name", se->idstr);
> > -        json_writer_int64(vmdesc, "instance_id", se->instance_id);
> > +        json_writer_start_object(s->vmdesc, NULL);
> > +        json_writer_str(s->vmdesc, "name", se->idstr);
> > +        json_writer_int64(s->vmdesc, "instance_id", se->instance_id);
> >   
> >           save_section_header(f, se, QEMU_VM_SECTION_FULL);
> > -        ret = vmstate_save(f, se, vmdesc);
> > +        ret = vmstate_save(f, se, s->vmdesc);
> >           if (ret) {
> >               qemu_file_set_error(f, ret);
> >               return ret;
> > @@ -1398,7 +1404,7 @@ int 
> > qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> >           trace_savevm_section_end(se->idstr, se->section_id, 0);
> >           save_section_footer(f, se);
> >   
> > -        json_writer_end_object(vmdesc);
> > +        json_writer_end_object(s->vmdesc);
> >       }
> >   
> >       if (inactivate_disks) {
> > @@ -1417,14 +1423,14 @@ int 
> > qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> >           qemu_put_byte(f, QEMU_VM_EOF);
> >       }
> >   
> > -    json_writer_end_array(vmdesc);
> > -    json_writer_end_object(vmdesc);
> > -    vmdesc_len = strlen(json_writer_get(vmdesc));
> > +    json_writer_end_array(s->vmdesc);
> > +    json_writer_end_object(s->vmdesc);
> > +    vmdesc_len = strlen(json_writer_get(s->vmdesc));
> >   
> >       if (should_send_vmdesc()) {
> >           qemu_put_byte(f, QEMU_VM_VMDESCRIPTION);
> >           qemu_put_be32(f, vmdesc_len);
> > -        qemu_put_buffer(f, (uint8_t *)json_writer_get(vmdesc), vmdesc_len);
> > +        qemu_put_buffer(f, (uint8_t *)json_writer_get(s->vmdesc), vmdesc_len);
> >       }
> >   
> >       return 0;
> > --
> > 2.34.1
>
> With regards,
> Daniel