Re: [PATCH v10 12/31] tools/libxl: add back channel support to read stream

From: Wen Congyang <wency@cn.fujitsu.com>
To: Wei Liu <wei.liu2@citrix.com>
Cc: Lars Kurth <lars.kurth@citrix.com>,
	Changlong Xie <xiecl.fnst@cn.fujitsu.com>,
	Ian Campbell <ian.campbell@citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Jiang Yunhong <yunhong.jiang@intel.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	xen devel <xen-devel@lists.xen.org>,
	Dong Eddie <eddie.dong@intel.com>,
	Gui Jianfeng <guijianfeng@cn.fujitsu.com>,
	Shriram Rajagopalan <rshriram@cs.ubc.ca>,
	Yang Hongyang <hongyang.yang@easystack.cn>
Subject: Re: [PATCH v10 12/31] tools/libxl: add back channel support to read stream
Date: Fri, 26 Feb 2016 10:16:43 +0800	[thread overview]
Message-ID: <56CFB58B.10109@cn.fujitsu.com> (raw)
In-Reply-To: <20160225155442.GF4235@citrix.com>

On 02/25/2016 11:54 PM, Wei Liu wrote:
> On Mon, Feb 22, 2016 at 10:52:16AM +0800, Wen Congyang wrote:
>> This is used by primay to read records sent by secondary.
>>
>> Signed-off-by: Yang Hongyang <hongyang.yang@easystack.cn>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>>  tools/libxl/libxl_create.c      |  1 +
>>  tools/libxl/libxl_internal.h    |  1 +
>>  tools/libxl/libxl_stream_read.c | 27 +++++++++++++++++++++++----
>>  3 files changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index 525bf85..fc746fb 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -1030,6 +1030,7 @@ static void domcreate_bootloader_done(libxl__egc *egc,
>>      dcs->srs.dcs = dcs;
>>      dcs->srs.fd = restore_fd;
>>      dcs->srs.legacy = (dcs->restore_params.stream_version == 1);
>> +    dcs->srs.back_channel = false;
>>      dcs->srs.completion_callback = domcreate_stream_done;
>>  
>>      if (restore_fd >= 0) {
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index cdf6751..32957ca 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -3420,6 +3420,7 @@ struct libxl__stream_read_state {
>>      libxl__domain_create_state *dcs;
>>      int fd;
>>      bool legacy;
>> +    bool back_channel;
>>      void (*completion_callback)(libxl__egc *egc,
>>                                  libxl__stream_read_state *srs,
>>                                  int rc);
>> diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
>> index f4781eb..02a2c46 100644
>> --- a/tools/libxl/libxl_stream_read.c
>> +++ b/tools/libxl/libxl_stream_read.c
>> @@ -118,6 +118,15 @@
>>   *    record, and therefore the buffered state is inconsistent. In
>>   *    libxl__xc_domain_restore_done(), we just complete the stream and
>>   *    stream->completion_callback() will be called to resume the guest
>> + *
>> + * For back channel stream:
>> + * - libxl__stream_read_start()
>> + *    - Set up the stream to running state
>> + *
>> + * - libxl__stream_read_continue()
>> + *     - Set up reading the next record from a started stream.
>> + *       Add some codes to process_record() to handle the record.
>> + *       Then call stream->checkpoint_callback() to return.
>>   */
>>  
>>  /* Success/error/cleanup handling. */
>> @@ -221,6 +230,17 @@ void libxl__stream_read_start(libxl__egc *egc,
>>      stream->running = true;
>>      stream->phase   = SRS_PHASE_NORMAL;
>>  
>> +    dc->ao       = stream->ao;
>> +    dc->copywhat = "restore v2 stream";
>> +    dc->writefd  = -1;
>> +
>> +    if (stream->back_channel) {
>> +        assert(!stream->legacy);
>> +
>> +        dc->readfd = stream->fd;
> 
> Why is this needed? dc->readfd is set to stream->fd no matter it is back
> channel or not. This can be moved outside this if {}.

Yes, will fix it in the next version.

> 
>> +        return;
>> +    }
>> +
>>      if (stream->legacy) {
>>          /* Convert the legacy stream. */
>>          libxl__conversion_helper_state *chs = &stream->chs;
>> @@ -243,10 +263,7 @@ void libxl__stream_read_start(libxl__egc *egc,
>>      }
>>      /* stream->fd is now a v2 stream. */
>>  
>> -    dc->ao       = stream->ao;
>> -    dc->copywhat = "restore v2 stream";
>>      dc->readfd   = stream->fd;
>> -    dc->writefd  = -1;
>>  
>>      /* Start reading the stream header. */
>>      rc = setup_read(stream, "stream header",
>> @@ -762,7 +779,9 @@ static void stream_done(libxl__egc *egc,
>>      LIBXL_STAILQ_FOREACH_SAFE(rec, &stream->record_queue, entry, trec)
>>          free_record(rec);
>>  
>> -    check_all_finished(egc, stream, rc);
>> +    if (!stream->back_channel)
>> +        /* back channel stream doesn't have restore helper */
>> +        check_all_finished(egc, stream, rc);
> 
> Even if it doesn't have restore helper, check_all_finished also checks
> if the stream and the conversion helper are till in use.  The
> explanation in the comment doesn't seem to justify this change.

In stream_done(), stream->running is set to false, so libxl__stream_read_in_use()
returns false.

Back channel stream doesn't support legacy stream, so there is no conversion helper.

I will update the comments in the next version.

Thanks
Wen Congyang

> 
> Wei.
> 
>>  }
>>  
>>  void libxl__xc_domain_restore_done(libxl__egc *egc, void *dcs_void,
>> -- 
>> 2.5.0
>>
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> 
> 
> .
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel