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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > 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 <dgilbert@redhat.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  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.

Pretty close, no '.] before the [] - we end up with (at the end of the series):

static struct mig_cmd_args {
    ssize_t     len; /* -1 = variable */
    const char *name;
} mig_cmd_args[] = {
    [MIG_CMD_INVALID]          = { .len = -1, .name = "INVALID" },
    [MIG_CMD_OPEN_RETURN_PATH] = { .len =  0, .name = "OPEN_RETURN_PATH" },
    [MIG_CMD_PING]             = { .len = sizeof(uint32_t), .name = "PING" },
    [MIG_CMD_POSTCOPY_ADVISE]  = { .len = 16, .name = "POSTCOPY_ADVISE" },
    [MIG_CMD_POSTCOPY_LISTEN]  = { .len =  0, .name = "POSTCOPY_LISTEN" },
    [MIG_CMD_POSTCOPY_RUN]     = { .len =  0, .name = "POSTCOPY_RUN" },
    [MIG_CMD_POSTCOPY_RAM_DISCARD] =
                                 { .len = -1, .name = "POSTCOPY_RAM_DISCARD" },
    [MIG_CMD_MAX]              = { .len = -1, .name = "MAX" },
};

> >  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 */

Yes, done.

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

Yep, done.

Dave

> 
> 
> >      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) ""
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK