[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 15/42] Return path: Source handling of return
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH v7 15/42] Return path: Source handling of return path |
Date: |
Tue, 18 Aug 2015 11:23:58 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
* Juan Quintela (address@hidden) wrote:
> "Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> >
> > Open a return path, and handle messages that are received upon it.
> >
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > +/*
> > + * 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 / address@hidden / Manchester, UK
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v7 15/42] Return path: Source handling of return path,
Dr. David Alan Gilbert <=