From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56512) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5Cmt-0006kg-5f for qemu-devel@nongnu.org; Wed, 17 Jun 2015 08:49:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5Cmo-0002x6-Sn for qemu-devel@nongnu.org; Wed, 17 Jun 2015 08:49:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44934) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5Cmo-0002wQ-Lv for qemu-devel@nongnu.org; Wed, 17 Jun 2015 08:49:46 -0400 From: Juan Quintela In-Reply-To: <1434450415-11339-14-git-send-email-dgilbert@redhat.com> (David Alan Gilbert's message of "Tue, 16 Jun 2015 11:26:26 +0100") References: <1434450415-11339-1-git-send-email-dgilbert@redhat.com> <1434450415-11339-14-git-send-email-dgilbert@redhat.com> Date: Wed, 17 Jun 2015 14:49:42 +0200 Message-ID: <87fv5q4hzt.fsf@neno.neno> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v7 13/42] Return path: Control commands Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert (git)" Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, liang.z.li@intel.com, qemu-devel@nongnu.org, luis@cs.umu.se, amit.shah@redhat.com, pbonzini@redhat.com, david@gibson.dropbear.id.au "Dr. David Alan Gilbert (git)" wrote: > From: "Dr. David Alan Gilbert" > > 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 > Reviewed-by: David Gibson > --- > 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) ""