Re: [Qemu-devel] [PATCH v7 13/42] Return path: Control commands

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> Add two src->dest commands:
>    * OPEN_RETURN_PATH - To request that the destination open the return path
>    * PING - Request an acknowledge from the destination
>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> Reviewed-by: David Gibson <address@hidden>
> ---
>  include/migration/migration.h |  2 ++
>  include/sysemu/sysemu.h       |  6 ++++-
>  migration/savevm.c            | 60 
> +++++++++++++++++++++++++++++++++++++++++++
>  trace-events                  |  2 ++
>  4 files changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 8adaa45..65fe5db 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -47,6 +47,8 @@ typedef QLIST_HEAD(, LoadStateEntry) LoadStateEntry_Head;
>  struct MigrationIncomingState {
>      QEMUFile *file;
>  
> +    QEMUFile *return_path;
> +
>      /* See savevm.c */
>      LoadStateEntry_Head loadvm_handlers;
>  };
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 5869607..d8875ca 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -84,7 +84,9 @@ void qemu_announce_self(void);
>  
>  /* Subcommands for QEMU_VM_COMMAND */
>  enum qemu_vm_cmd {
> -    MIG_CMD_INVALID = 0,   /* Must be 0 */
> +    MIG_CMD_INVALID = 0,       /* Must be 0 */
> +    MIG_CMD_OPEN_RETURN_PATH,  /* Tell the dest to open the Return path */
> +    MIG_CMD_PING,              /* Request a PONG on the RP */

Add    MIG_CMD_MAX
    

struct cmd_args {
       int len;
       char *name;
} cmd_args[] ={
.[MIG_CMD_INVALID] = {
   .len = 0,
   .name = "CMD_INVALID"},
.[MIG_CMD_OPEN_RETURN_PATH] = {
   .len = 0,
   .name = "CMD_OPEN_RETURN_PATH"},
.....
}
}

I have done the initialization by hand, not sure if syntax is ok.


>  static int loadvm_process_command(QEMUFile *f)
>  {
> +    MigrationIncomingState *mis = migration_incoming_get_current();
>      uint16_t cmd;
>      uint16_t len;
> +    uint32_t tmp32;
>  
>      cmd = qemu_get_be16(f);
>      len = qemu_get_be16(f);
>  
>      trace_loadvm_process_command(cmd, len);

/* yes, this should go in previous patch */

if (cmd > MIG_CMD_MAX ) {
            error_report("VM_COMMAND 0x%x unknown (len 0x%x)", cmd, len);
     .....
}

if (loadvm_process_command_simple_lencheck(cmd_args[cmd].name, len,
cmd_args[cmd].len) {
                   return -1;
}

switch (cmd) {
case MIG_CMD_OPEN_RETURN_PATH:
        if (mis->return_path) {
            error_report("CMD_OPEN_RETURN_PATH called when RP already open");
            /* Not really a problem, so don't give up */
            return 0;
        }
        mis->return_path = qemu_file_get_return_path(f);
        if (!mis->return_path) {
            error_report("CMD_OPEN_RETURN_PATH failed");
            return -1;
        }
        break;

.....
}

You get the idea.  I normally even put the code from the command in a
function pointer, but I know that other people don't like it.
This way you factor the common code at the beginning of the function.

What do you think?


>      switch (cmd) {
> +    case MIG_CMD_OPEN_RETURN_PATH:
> +        if (loadvm_process_command_simple_lencheck("CMD_OPEN_RETURN_PATH",
> +                                                   len, 0)) {
> +            return -1;
> +        }
> +        if (mis->return_path) {
> +            error_report("CMD_OPEN_RETURN_PATH called when RP already open");
> +            /* Not really a problem, so don't give up */
> +            return 0;
> +        }
> +        mis->return_path = qemu_file_get_return_path(f);
> +        if (!mis->return_path) {
> +            error_report("CMD_OPEN_RETURN_PATH failed");
> +            return -1;
> +        }
> +        break;
> +
> +    case MIG_CMD_PING:
> +        if (loadvm_process_command_simple_lencheck("CMD_PING", len,
> +                                                   sizeof(tmp32))) {
> +            return -1;
> +        }
> +        tmp32 = qemu_get_be32(f);
> +        trace_loadvm_process_command_ping(tmp32);
> +        if (!mis->return_path) {
> +            error_report("CMD_PING (0x%x) received with no return path",
> +                         tmp32);
> +            return -1;
> +        }
> +        /* migrate_send_rp_pong(mis, tmp32); TODO: gets added later */
> +        break;
>  
>      default:
>          error_report("VM_COMMAND 0x%x unknown (len 0x%x)", cmd, len);
> diff --git a/trace-events b/trace-events
> index 73a65c3..5967fdf 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1184,8 +1184,10 @@ qemu_loadvm_state_section(unsigned int section_type) 
> "%d"
>  qemu_loadvm_state_section_partend(uint32_t section_id) "%u"
>  qemu_loadvm_state_section_startfull(uint32_t section_id, const char *idstr, 
> uint32_t instance_id, uint32_t version_id) "%u(%s) %u %u"
>  loadvm_process_command(uint16_t com, uint16_t len) "com=0x%x len=%d"
> +loadvm_process_command_ping(uint32_t val) "%x"
>  savevm_section_start(const char *id, unsigned int section_id) "%s, 
> section_id %u"
>  savevm_section_end(const char *id, unsigned int section_id, int ret) "%s, 
> section_id %u -> %d"
> +savevm_send_ping(uint32_t val) "%x"
>  savevm_state_begin(void) ""
>  savevm_state_header(void) ""
>  savevm_state_iterate(void) ""