From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44732) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZRe3s-0002rf-Du for qemu-devel@nongnu.org; Tue, 18 Aug 2015 06:24:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZRe3p-0001w8-3a for qemu-devel@nongnu.org; Tue, 18 Aug 2015 06:24:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37846) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZRe3o-0001vg-RL for qemu-devel@nongnu.org; Tue, 18 Aug 2015 06:24:05 -0400 Date: Tue, 18 Aug 2015 11:23:58 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20150818102357.GA2314@work-vm> References: <1434450415-11339-1-git-send-email-dgilbert@redhat.com> <1434450415-11339-16-git-send-email-dgilbert@redhat.com> <87wpy4l596.fsf@neno.neno> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87wpy4l596.fsf@neno.neno> Subject: Re: [Qemu-devel] [PATCH v7 15/42] Return path: Source handling of return path 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" > > > > Open a return path, and handle messages that are received upon it. > > > > Signed-off-by: Dr. David Alan Gilbert > > +/* > > + * Handles messages sent on the return path towards the source VM > > + * > > + */ > > +static void *source_return_path_thread(void *opaque) > > +{ > > + MigrationState *ms = opaque; > > + QEMUFile *rp = ms->rp_state.file; > > + uint16_t expected_len, header_len, header_type; > > + const int max_len = 512; > > + uint8_t buf[max_len]; > > + uint32_t tmp32; > > + int res; > > + > > + trace_source_return_path_thread_entry(); > > + while (rp && !qemu_file_get_error(rp) && > > What can make rp == NULL? > THinking about that, could you mean *rp here? I've reworked this; it was meant to catch the case of the rp being closed early, but was racy. I've now got: while (!ms->rp_state.error && !qemu_file_get_error(rp) && migration_already_active(ms)) { and now the rp qemu_file gets closed at the end of the thread, anyone else that wants the rp_thread to exit sets the error flag. > > + migration_already_active(ms)) { > > + trace_source_return_path_thread_loop_top(); > > + header_type = qemu_get_be16(rp); > > + header_len = qemu_get_be16(rp); > > + > > + switch (header_type) { > > + case MIG_RP_MSG_SHUT: > > + case MIG_RP_MSG_PONG: > > + expected_len = 4; > > + break; > > + > > + default: > > + error_report("RP: Received invalid message 0x%04x length 0x%04x", > > + header_type, header_len); > > + source_return_path_bad(ms); > > + goto out; > > + } > > > > + if (header_len > expected_len) { > > + error_report("RP: Received message 0x%04x with" > > + "incorrect length %d expecting %d", > > + header_type, header_len, > > + expected_len); > > I know this is a big request, but getting an array with messages length > and message names to be able to print nice error messages looks ilke good? Done; (same way as for the commands on the forward path). > > + source_return_path_bad(ms); > > + goto out; > > + } > > + > > + /* We know we've got a valid header by this point */ > > + res = qemu_get_buffer(rp, buf, header_len); > > + if (res != header_len) { > > + trace_source_return_path_thread_failed_read_cmd_data(); > > + source_return_path_bad(ms); > > + goto out; > > + } > > + > > + /* OK, we have the message and the data */ > > + switch (header_type) { > > + case MIG_RP_MSG_SHUT: > > + tmp32 = be32_to_cpup((uint32_t *)buf); > > make local variable and call it sibling_error or whatever you like? Done. > > + trace_source_return_path_thread_shut(tmp32); > > + if (tmp32) { > > + error_report("RP: Sibling indicated error %d", tmp32); > > + source_return_path_bad(ms); > > + } > > + /* > > + * We'll let the main thread deal with closing the RP > > + * we could do a shutdown(2) on it, but we're the only user > > + * anyway, so there's nothing gained. > > + */ > > + goto out; > > + > > + case MIG_RP_MSG_PONG: > > + tmp32 = be32_to_cpup((uint32_t *)buf); > > unused? > Althought I guess it is used somewhere to make sure that the value is > the same that whatever we did the ping. credentials? > > I can't see with this and previous patch what value is sent here. I'm not using the ping/pong messages for anything active, they exist primarily as a debug and tracing aid. > > + trace_source_return_path_thread_pong(tmp32); > > + break; > > + > > + default: > > + break; > > + } > > + } > > + if (rp && qemu_file_get_error(rp)) { > > + trace_source_return_path_thread_bad_end(); > > + source_return_path_bad(ms); > > + } > > + > > + trace_source_return_path_thread_end(); > > +out: > > + return NULL; > > +} > > + > > +__attribute__ (( unused )) /* Until later in patch series */ > > unused_by_know attribute required O:-) Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK