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

From: Wei Liu <wei.liu2@citrix.com>
To: Wen Congyang <wency@cn.fujitsu.com>
Cc: Lars Kurth <lars.kurth@citrix.com>,
	Changlong Xie <xiecl.fnst@cn.fujitsu.com>,
	Wei Liu <wei.liu2@citrix.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: Thu, 25 Feb 2016 15:54:42 +0000	[thread overview]
Message-ID: <20160225155442.GF4235@citrix.com> (raw)
In-Reply-To: <1456109555-28299-13-git-send-email-wency@cn.fujitsu.com>

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 {}.

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

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