|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v10 10/31] tools/libxl: add back channel support to write stream
On 03/02/2016 11:02 PM, Wei Liu wrote:
> On Fri, Feb 26, 2016 at 10:11:27AM +0800, Wen Congyang wrote:
>> On 02/25/2016 11:54 PM, Wei Liu wrote:
>>> On Mon, Feb 22, 2016 at 10:52:14AM +0800, Wen Congyang 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 <hongyang.yang@xxxxxxxxxxxx>
>>>> Signed-off-by: Wen Congyang <wency@xxxxxxxxxxxxxx>
>>>> ---
>>>> tools/libxl/libxl_dom_save.c | 1 +
>>>> tools/libxl/libxl_internal.h | 1 +
>>>> tools/libxl/libxl_stream_write.c | 26 ++++++++++++++++++++------
>>>> 3 files changed, 22 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/tools/libxl/libxl_dom_save.c b/tools/libxl/libxl_dom_save.c
>>>> index 72b61c7..18946c5 100644
>>>> --- a/tools/libxl/libxl_dom_save.c
>>>> +++ b/tools/libxl/libxl_dom_save.c
>>>> @@ -404,6 +404,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 3d3e8e8..e02e554 100644
>>>> --- a/tools/libxl/libxl_internal.h
>>>> +++ b/tools/libxl/libxl_internal.h
>>>> @@ -3044,6 +3044,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 f6ea55d..5379126 100644
>>>> --- a/tools/libxl/libxl_stream_write.c
>>>> +++ b/tools/libxl/libxl_stream_write.c
>>>> @@ -49,6 +49,13 @@
>>>> * - if (hvm)
>>>> * - Emulator context 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.
>>>
>>> What does this mean? Which new API?
>>
>> The next patch introduces this API. The commits is very old.
>>
>> I think I can merge these two patches into one patch.
>>
>
> Please reference the actual function / API.
>
>>>
>>>> */
>>>>
>>>> /* Success/error/cleanup handling. */
>>>> @@ -225,6 +232,15 @@ void libxl__stream_write_start(libxl__egc *egc,
>>>>
>>>> stream->running = true;
>>>>
>>>> + dc->ao = ao;
>>>> + dc->readfd = -1;
>>>> + dc->copywhat = "save v2 stream";
>>>> + dc->writefd = stream->fd;
>>>> + dc->maxsz = -1;
>>>> +
>>>> + if (stream->back_channel)
>>>> + return;
>>>> +
>>>
>>> There seems to be very subtle change of behaviour.
>>>
>>> Before this patch, the dc->* are not set until the emulator check is
>>> done. With this path, it is possible in the normal case some of the
>>> fields are initialised in the error path.
>>>
>>> I think this is OK given the callbacks in the upper layer and in
>>> the writer don't rely on those fields to clean up. Just one thing to
>>> note.
>>>
>>> That said, I suggest you move all initialisation of dc->* in one place.
>>
>> OK, I will do it.
>>
>>>
>>>> if (dss->type == LIBXL_DOMAIN_TYPE_HVM) {
>>>> stream->device_model_version =
>>>> libxl__device_model_version_running(gc, dss->domid);
>>>> @@ -249,12 +265,7 @@ void libxl__stream_write_start(libxl__egc *egc,
>>>> stream->emu_sub_hdr.index = 0;
>>>> }
>>>>
>>>> - dc->ao = ao;
>>>> - dc->readfd = -1;
>>>> dc->writewhat = "stream header";
>>>> - dc->copywhat = "save v2 stream";
>>>> - dc->writefd = stream->fd;
>>>> - dc->maxsz = -1;
>>>> dc->callback = stream_header_done;
>>>>
>>>> rc = libxl__datacopier_start(dc);
>>>> @@ -279,6 +290,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_emulator_xenstore_record(egc, stream);
>>>> @@ -590,7 +602,9 @@ static void stream_done(libxl__egc *egc,
>>>> libxl__carefd_close(stream->emu_carefd);
>>>> free(stream->emu_body);
>>>>
>>>> - check_all_finished(egc, stream, rc);
>>>> + if (!stream->back_channel)
>>>> + /* back channel stream doesn't have save helper */
>>>> + check_all_finished(egc, stream, rc);
>>>
>>> Though it doesn't have helper, do you not need to check if the back
>>> channel stream itself is OK? The comment itself doesn't seem to justify
>>> this change.
>>
>> What do you want to check? assert(!stream->shs) or
>> assert(!libxl__stream_write_inuse(stream))?
>
>
> The stream itself is not in use.
We set stream->running to false in stream_done, so libxl__stream_write_inuse()
will return false. So I think there is no need to check it. I will update
the comments to document it.
>
> Also check_all_finished calls completion_callback. Don't you need that,
> too?
back channel stream doesn't have completion_callback. See the comments in
check_all_finished().
Thanks
Wen Congyang
>
> Wei.
>
>>
>> Thanks
>> Wen Congyang
>>
>>>
>>> Wei.
>>>
>>>> }
>>>>
>>>> static void checkpoint_done(libxl__egc *egc,
>>>> --
>>>> 2.5.0
>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> Xen-devel@xxxxxxxxxxxxx
>>>> http://lists.xen.org/xen-devel
>>>
>>>
>>> .
>>>
>>
>>
>>
>
>
> .
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |