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

[Dr. David Alan Gilbert]

* Amit Shah (address@hidden) wrote:
> On (Tue) 16 Jun 2015 [11:26:28], Dr. David Alan Gilbert (git) 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>
> 
> 
> > -/* migration thread support */
> > +/*
> > + * Something bad happened to the RP stream, mark an error
> > + * The caller shall print something to indicate why
> > + */
> > +static void source_return_path_bad(MigrationState *s)
> 
> Can you rename this to something like
> 
> mark_source_rp_bad()
> 
> ?
> 
> Intent is clearer that way.

Done.

> Also, the comment says caller will print something, but the
> invocations below are a mix of printfs and traces.  Not saying the
> caller has to print always, but maybe only comment needs update.

Yes, I've changed the comment, and changed one of the traces into
an error_report.

Thanks,

Dave

> 
> > +{
> > +    s->rp_state.error = true;
> > +    migrate_fd_cleanup_src_rp(s);
> > +}
> > +
> > +/*
> > + * 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) &&
> > +        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);
> > +            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;
> > +        }
> 
>               Amit
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK