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

[Juan Quintela]

"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?


> +        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?

> +            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?

> +            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.


> +            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:-)