From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49044) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7TO9-0000L5-DC for qemu-devel@nongnu.org; Tue, 23 Jun 2015 14:57:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z7TO4-0005PS-RQ for qemu-devel@nongnu.org; Tue, 23 Jun 2015 14:57:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60896) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7TO4-0005PH-KL for qemu-devel@nongnu.org; Tue, 23 Jun 2015 14:57:36 -0400 Date: Tue, 23 Jun 2015 19:57:29 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20150623185729.GG2167@work-vm> References: <1434450415-11339-1-git-send-email-dgilbert@redhat.com> <1434450415-11339-14-git-send-email-dgilbert@redhat.com> <87fv5q4hzt.fsf@neno.neno> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87fv5q4hzt.fsf@neno.neno> Subject: Re: [Qemu-devel] [PATCH v7 13/42] Return path: Control commands List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela 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 * Juan Quintela (quintela@redhat.com) wrote: > "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. 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