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

["Daniel P. Berrangé" <berrange@redhat.com>]

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

Also, there's two  json_writer_start_object calls, but only
one json_writer_end_object.

BTW, some <tab> crept into this patch.


>      }
>  }
>  
> @@ -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
-- 
|: 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 :|