Re: [Qemu-devel] [PATCH v7 15/42] Return path: Source handling of return path

["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>
> >
> > Open a return path, and handle messages that are received upon it.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > +/*
> > + * 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