Re: [PATCH v8 --for 4.6 COLO 05/25] tools/libxl: add back channel support to write stream

[Yang Hongyang <yanghy@cn.fujitsu.com>]

On 07/16/2015 01:25 AM, Andrew Cooper wrote:
> On 15/07/15 10:18, Yang Hongyang wrote:
>> Add back channel support to write stream. If the write stream is
>> a back channel stream, this means the write stream is used by
>> Secondary to send some records back.
>>
>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>> ---
>>   tools/libxl/libxl_dom_save.c     |  1 +
>>   tools/libxl/libxl_internal.h     |  1 +
>>   tools/libxl/libxl_stream_write.c | 16 ++++++++++++++++
>>   3 files changed, 18 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_dom_save.c b/tools/libxl/libxl_dom_save.c
>> index 9b7159f..25813ce 100644
>> --- a/tools/libxl/libxl_dom_save.c
>> +++ b/tools/libxl/libxl_dom_save.c
>> @@ -445,6 +445,7 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_save_state *dss)
>>       dss->sws.ao  = dss->ao;
>>       dss->sws.dss = dss;
>>       dss->sws.fd  = dss->fd;
>> +    dss->sws.back_channel = false;
>>       dss->sws.completion_callback = stream_done;
>>
>>       libxl__stream_write_start(egc, &dss->sws);
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 9c81d8d..a83d6a5 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -2989,6 +2989,7 @@ struct libxl__stream_write_state {
>>       libxl__ao *ao;
>>       libxl__domain_save_state *dss;
>>       int fd;
>> +    bool back_channel;
>>       void (*completion_callback)(libxl__egc *egc,
>>                                   libxl__stream_write_state *sws,
>>                                   int rc);
>> diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c
>> index 16f667a..df55277 100644
>> --- a/tools/libxl/libxl_stream_write.c
>> +++ b/tools/libxl/libxl_stream_write.c
>> @@ -47,6 +47,13 @@
>>    *  - Toolstack record
>>    *  - if (hvm), Qemu record
>>    *  - Checkpoint end record
>> + *
>> + * For back channel stream:
>> + * - libxl__stream_write_start()
>> + *    - Set up the stream to running state
>> + *
>> + * - Add a new API to write the record. When the record is written
>> + *   out, call stream->checkpoint_callback() to return.
>>    */
>>
>>   /* Success/error/cleanup handling. */
>> @@ -178,6 +185,9 @@ void libxl__stream_write_start(libxl__egc *egc,
>>
>>       stream->running = true;
>>
>> +    if (stream->back_channel)
>> +        return;
>> +
>
> Some of the setup below should not be skipped.
>
> While it makes the diff bigger, the end result would be more logical as
>
> dc->ao = ao;
> dc->readfd = -1;
> dc->writefd = stream->fd;
> dc->maxsz = -1;
>
> if (!stream->back_channel) {
>      /* Write the stream header. */
>      dc->writewhat = "save/migration stream";
>      dc->callback = stream_header_done;
>
>      rc = ibxl__datacopier_start(dc);
>      ....
> }
>
> To split the object setup from the action of sending the stream header,
> which are currently mixed.
>
> What happened to the backchannel negotiation header?

There's no negotiation header currently...If it is a broken back channel,
COLO will fail.

>
> ~Andrew
>
>>       dc->ao        = ao;
>>       dc->readfd    = -1;
>>       dc->writewhat = "save/migration stream";
>> @@ -207,6 +217,7 @@ void libxl__stream_write_start_checkpoint(libxl__egc *egc,
>>   {
>>       assert(stream->running);
>>       assert(!stream->in_checkpoint);
>> +    assert(!stream->back_channel);
>>       stream->in_checkpoint = true;
>>
>>       write_toolstack_record(egc, stream);
>> @@ -500,6 +511,11 @@ static void stream_done(libxl__egc *egc,
>>       assert(stream->running);
>>       stream->running = false;
>>
>> +    if (stream->back_channel) {
>> +        stream->completion_callback(egc, stream, stream->rc);
>> +        return;
>> +    }
>> +
>>       if (stream->emu_carefd)
>>           libxl__carefd_close(stream->emu_carefd);
>>       free(stream->emu_body);
>
> .
>

-- 
Thanks,
Yang.