Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-17 Thread Ian Campbell 
On Wed, 2015-06-17 at 18:49 +0800, Wen Congyang wrote:
> On 06/17/2015 06:15 PM, Ian Campbell wrote:
> > On Wed, 2015-06-17 at 11:09 +0800, Wen Congyang wrote:
> >>> +if (hdr->options & RESTORE_OPT_BIG_ENDIAN) {
> >>> +ret = ERROR_FAIL;
> >>> +LOG(ERROR, "Unable to handle big endian streams");
> >>> +goto err;
> >>
> >> I think it is better to check if the host is big endian or not.
> > 
> > It's not, there are no big endian ports of Xen today. I think encoding
> > that here is fine.
> 
> IIRC, arm supports big endian. Do we only use arm+little endian?

Currently, yes. There are plans afoot to support big endian _guests_ but
not big endian hypervisor (and by extension IMHO tools or at least the
migration stream should remain LE). That would be a lot of faff for very
little benefit IMHO.

Ian.


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

Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-17 Thread Andrew Cooper 
On 17/06/15 11:01, Wen Congyang wrote:
> On 06/17/2015 05:50 PM, Andrew Cooper wrote:
>> On 17/06/15 08:57, Wen Congyang wrote:
 +/* Queue up reading the body. */
> +size_t bytes_to_read;
> +
> +switch (rec_hdr->type) {
> +/*
> + * Emulator records want to retain the blob in the pipe, for a 
> further
> + * datacopier call to move elsewhere.  Just read the emulator 
> header.
> + */
>>> In this case, we should not call ROUNDUP().
>>>
> +case REC_TYPE_EMULATOR_CONTEXT:
> +bytes_to_read = sizeof(struct libxl_sr_emulator_hdr);
> +break;
> +
> +default:
> +bytes_to_read = rec_hdr->length;
> +break;
> +}
> +
> +bytes_to_read = ROUNDUP(bytes_to_read, REC_ALIGN_ORDER);
>>> So, I think it is better to move ROUNDUP to default case.
>>>
>>> Thanks
>>> Wen Congyang
>>>
>> sizeof(struct libxl_sr_emulator_hdr) is cunningly of the appropriate
>> order already.
> Yes
>
>> I suppose it is probably better to move the roundup into the default
>> case and assert() appropriate alignment after the switch()
> Do you mean the sub-header must be aligned

The start of any record is required to be aligned.  It is the
responsibility of any record which is not aligned to insert padding
after the content so the following record starts on an 8 byte boundary.

~Andrew

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

Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-17 Thread Wen Congyang 
On 06/17/2015 06:15 PM, Ian Campbell wrote:
> On Wed, 2015-06-17 at 11:09 +0800, Wen Congyang wrote:
>>> +if (hdr->options & RESTORE_OPT_BIG_ENDIAN) {
>>> +ret = ERROR_FAIL;
>>> +LOG(ERROR, "Unable to handle big endian streams");
>>> +goto err;
>>
>> I think it is better to check if the host is big endian or not.
> 
> It's not, there are no big endian ports of Xen today. I think encoding
> that here is fine.

IIRC, arm supports big endian. Do we only use arm+little endian? If so,
it is OK now.

Thanks
Wen Congyang

> 
> 
> .
> 


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

Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-17 Thread Ian Campbell 
On Wed, 2015-06-17 at 11:09 +0800, Wen Congyang wrote:
> > +if (hdr->options & RESTORE_OPT_BIG_ENDIAN) {
> > +ret = ERROR_FAIL;
> > +LOG(ERROR, "Unable to handle big endian streams");
> > +goto err;
> 
> I think it is better to check if the host is big endian or not.

It's not, there are no big endian ports of Xen today. I think encoding
that here is fine.



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

Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-17 Thread Wen Congyang 
On 06/17/2015 05:50 PM, Andrew Cooper wrote:
> On 17/06/15 08:57, Wen Congyang wrote:
>>> +/* Queue up reading the body. */
 +size_t bytes_to_read;
 +
 +switch (rec_hdr->type) {
 +/*
 + * Emulator records want to retain the blob in the pipe, for a 
 further
 + * datacopier call to move elsewhere.  Just read the emulator 
 header.
 + */
>> In this case, we should not call ROUNDUP().
>>
 +case REC_TYPE_EMULATOR_CONTEXT:
 +bytes_to_read = sizeof(struct libxl_sr_emulator_hdr);
 +break;
 +
 +default:
 +bytes_to_read = rec_hdr->length;
 +break;
 +}
 +
 +bytes_to_read = ROUNDUP(bytes_to_read, REC_ALIGN_ORDER);
>> So, I think it is better to move ROUNDUP to default case.
>>
>> Thanks
>> Wen Congyang
>>
> 
> sizeof(struct libxl_sr_emulator_hdr) is cunningly of the appropriate
> order already.

Yes

> 
> I suppose it is probably better to move the roundup into the default
> case and assert() appropriate alignment after the switch()

Do you mean the sub-header must be aligned

Thanks
Wen Congyang

> 
> ~Andrew
> .
> 


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

Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-17 Thread Andrew Cooper 
On 17/06/15 08:57, Wen Congyang wrote:
>> +/* Queue up reading the body. */
>> > +size_t bytes_to_read;
>> > +
>> > +switch (rec_hdr->type) {
>> > +/*
>> > + * Emulator records want to retain the blob in the pipe, for a 
>> > further
>> > + * datacopier call to move elsewhere.  Just read the emulator 
>> > header.
>> > + */
> In this case, we should not call ROUNDUP().
>
>> > +case REC_TYPE_EMULATOR_CONTEXT:
>> > +bytes_to_read = sizeof(struct libxl_sr_emulator_hdr);
>> > +break;
>> > +
>> > +default:
>> > +bytes_to_read = rec_hdr->length;
>> > +break;
>> > +}
>> > +
>> > +bytes_to_read = ROUNDUP(bytes_to_read, REC_ALIGN_ORDER);
> So, I think it is better to move ROUNDUP to default case.
>
> Thanks
> Wen Congyang
>

sizeof(struct libxl_sr_emulator_hdr) is cunningly of the appropriate
order already.

I suppose it is probably better to move the roundup into the default
case and assert() appropriate alignment after the switch()

~Andrew

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

Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-17 Thread Andrew Cooper 
On 17/06/15 07:03, Wen Congyang wrote:
>> +static void stream_failed(libxl__egc *egc,
>> > +  libxl__stream_read_state *stream, int rc)
>> > +{
>> > +assert(rc);
>> > +stream->rc = rc;
> I have a question: rc is always less than 0?
>
> Thanks
> Wen Congyang
>

I believe so.  It should realistically always be an ERROR_$foo

~Andrew

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

Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-17 Thread Wen Congyang 
On 06/15/2015 09:44 PM, Andrew Cooper wrote:
> From: Ross Lagerwall 
> 
> Signed-off-by: Ross Lagerwall 
> Signed-off-by: Andrew Cooper 
> CC: Ian Campbell 
> CC: Ian Jackson 
> CC: Wei Liu 
> ---
>  tools/libxl/Makefile|1 +
>  tools/libxl/libxl_internal.h|   39 
>  tools/libxl/libxl_stream_read.c |  485 
> +++
>  3 files changed, 525 insertions(+)
>  create mode 100644 tools/libxl/libxl_stream_read.c
> 
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index cc9c152..c71c5fe 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -94,6 +94,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
> libxl_pci.o \
>   libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
>   libxl_internal.o libxl_utils.o libxl_uuid.o \
>   libxl_json.o libxl_aoutils.o libxl_numa.o libxl_vnuma.o 
> \
> + libxl_stream_read.o \
>   libxl_save_callout.o _libxl_save_msgs_callout.o \
>   libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
>  LIBXL_OBJS += libxl_genid.o
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 101994f..4f33cb8 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -19,6 +19,8 @@
>  
>  #include "libxl_osdeps.h" /* must come before any other headers */
>  
> +#include "libxl_sr_stream_format.h"
> +
>  #include 
>  #include 
>  #include 
> @@ -3121,6 +3123,42 @@ typedef void libxl__domain_create_cb(libxl__egc *egc,
>   libxl__domain_create_state*,
>   int rc, uint32_t domid);
>  
> +/* State for manipulating a libxl migration v2 stream */
> +typedef struct libxl__stream_read_state libxl__stream_read_state;
> +
> +struct libxl__stream_read_state {
> +/* filled by the user */
> +libxl__ao *ao;
> +int fd;
> +void (*completion_callback)(libxl__egc *egc,
> +libxl__domain_create_state *dcs,
> +int rc);
> +/* Private */
> +int rc;
> +bool running;
> +libxl__datacopier_state dc;
> +size_t expected_len;
> +libxl_sr_hdr hdr;
> +libxl_sr_rec_hdr rec_hdr;
> +void *rec_body;
> +};
> +
> +_hidden void libxl__stream_read_start(libxl__egc *egc,
> +  libxl__stream_read_state *stream);
> +
> +_hidden void libxl__stream_read_continue(libxl__egc *egc,
> + libxl__stream_read_state *stream);
> +
> +_hidden void libxl__stream_read_abort(libxl__egc *egc,
> +  libxl__stream_read_state *stream, int 
> rc);
> +
> +static inline bool libxl__stream_read_inuse(
> +const libxl__stream_read_state *stream)
> +{
> +return stream->running;
> +}
> +
> +
>  struct libxl__domain_create_state {
>  /* filled in by user */
>  libxl__ao *ao;
> @@ -3137,6 +3175,7 @@ struct libxl__domain_create_state {
>  libxl__stub_dm_spawn_state dmss;
>  /* If we're not doing stubdom, we use only dmss.dm,
>   * for the non-stubdom device model. */
> +libxl__stream_read_state srs;
>  libxl__save_helper_state shs;
>  /* necessary if the domain creation failed and we have to destroy it */
>  libxl__domain_destroy_state dds;
> diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
> new file mode 100644
> index 000..9cdaadf
> --- /dev/null
> +++ b/tools/libxl/libxl_stream_read.c
> @@ -0,0 +1,485 @@
> +/*
> + * Copyright (C) 2015  Citrix Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +
> +#include "libxl_internal.h"
> +
> +/*
> + * Infrastructure for reading and acting on the contents of a libxl migration
> + * stream. There are a lot of moving parts here.
> + *
> + * Entry points from outside:
> + *  - libxl__stream_read_start()
> + * - Set up reading a stream from the start.
> + *
> + *  - libxl__stream_read_continue()
> + * - Set up reading the next record from a started stream.
> + *
> + * The principle loop functionality involves reading the stream header, then
> + * reading a record at time and acting upon it.  It follows the callbacks:
> + *
> + *  - stream_header_done()
> + *  - stream_record_header_done()
> + *  -

Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-16 Thread Wen Congyang 
On 06/15/2015 09:44 PM, Andrew Cooper wrote:
> From: Ross Lagerwall 
> 
> Signed-off-by: Ross Lagerwall 
> Signed-off-by: Andrew Cooper 
> CC: Ian Campbell 
> CC: Ian Jackson 
> CC: Wei Liu 
> ---
>  tools/libxl/Makefile|1 +
>  tools/libxl/libxl_internal.h|   39 
>  tools/libxl/libxl_stream_read.c |  485 
> +++
>  3 files changed, 525 insertions(+)
>  create mode 100644 tools/libxl/libxl_stream_read.c
> 
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index cc9c152..c71c5fe 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -94,6 +94,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
> libxl_pci.o \
>   libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
>   libxl_internal.o libxl_utils.o libxl_uuid.o \
>   libxl_json.o libxl_aoutils.o libxl_numa.o libxl_vnuma.o 
> \
> + libxl_stream_read.o \
>   libxl_save_callout.o _libxl_save_msgs_callout.o \
>   libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
>  LIBXL_OBJS += libxl_genid.o
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 101994f..4f33cb8 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -19,6 +19,8 @@
>  
>  #include "libxl_osdeps.h" /* must come before any other headers */
>  
> +#include "libxl_sr_stream_format.h"
> +
>  #include 
>  #include 
>  #include 
> @@ -3121,6 +3123,42 @@ typedef void libxl__domain_create_cb(libxl__egc *egc,
>   libxl__domain_create_state*,
>   int rc, uint32_t domid);
>  
> +/* State for manipulating a libxl migration v2 stream */
> +typedef struct libxl__stream_read_state libxl__stream_read_state;
> +
> +struct libxl__stream_read_state {
> +/* filled by the user */
> +libxl__ao *ao;
> +int fd;
> +void (*completion_callback)(libxl__egc *egc,
> +libxl__domain_create_state *dcs,
> +int rc);
> +/* Private */
> +int rc;
> +bool running;
> +libxl__datacopier_state dc;
> +size_t expected_len;
> +libxl_sr_hdr hdr;
> +libxl_sr_rec_hdr rec_hdr;
> +void *rec_body;
> +};
> +
> +_hidden void libxl__stream_read_start(libxl__egc *egc,
> +  libxl__stream_read_state *stream);
> +
> +_hidden void libxl__stream_read_continue(libxl__egc *egc,
> + libxl__stream_read_state *stream);
> +
> +_hidden void libxl__stream_read_abort(libxl__egc *egc,
> +  libxl__stream_read_state *stream, int 
> rc);
> +
> +static inline bool libxl__stream_read_inuse(
> +const libxl__stream_read_state *stream)
> +{
> +return stream->running;
> +}
> +
> +
>  struct libxl__domain_create_state {
>  /* filled in by user */
>  libxl__ao *ao;
> @@ -3137,6 +3175,7 @@ struct libxl__domain_create_state {
>  libxl__stub_dm_spawn_state dmss;
>  /* If we're not doing stubdom, we use only dmss.dm,
>   * for the non-stubdom device model. */
> +libxl__stream_read_state srs;
>  libxl__save_helper_state shs;
>  /* necessary if the domain creation failed and we have to destroy it */
>  libxl__domain_destroy_state dds;
> diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
> new file mode 100644
> index 000..9cdaadf
> --- /dev/null
> +++ b/tools/libxl/libxl_stream_read.c
> @@ -0,0 +1,485 @@
> +/*
> + * Copyright (C) 2015  Citrix Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +
> +#include "libxl_internal.h"
> +
> +/*
> + * Infrastructure for reading and acting on the contents of a libxl migration
> + * stream. There are a lot of moving parts here.
> + *
> + * Entry points from outside:
> + *  - libxl__stream_read_start()
> + * - Set up reading a stream from the start.
> + *
> + *  - libxl__stream_read_continue()
> + * - Set up reading the next record from a started stream.
> + *
> + * The principle loop functionality involves reading the stream header, then
> + * reading a record at time and acting upon it.  It follows the callbacks:
> + *
> + *  - stream_header_done()
> + *  - stream_record_header_done()
> + *  -

Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-16 Thread Wen Congyang 
On 06/15/2015 09:44 PM, Andrew Cooper wrote:
> From: Ross Lagerwall 
> 
> Signed-off-by: Ross Lagerwall 
> Signed-off-by: Andrew Cooper 
> CC: Ian Campbell 
> CC: Ian Jackson 
> CC: Wei Liu 
> ---
>  tools/libxl/Makefile|1 +
>  tools/libxl/libxl_internal.h|   39 
>  tools/libxl/libxl_stream_read.c |  485 
> +++
>  3 files changed, 525 insertions(+)
>  create mode 100644 tools/libxl/libxl_stream_read.c
> 
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index cc9c152..c71c5fe 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -94,6 +94,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
> libxl_pci.o \
>   libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
>   libxl_internal.o libxl_utils.o libxl_uuid.o \
>   libxl_json.o libxl_aoutils.o libxl_numa.o libxl_vnuma.o 
> \
> + libxl_stream_read.o \
>   libxl_save_callout.o _libxl_save_msgs_callout.o \
>   libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
>  LIBXL_OBJS += libxl_genid.o
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 101994f..4f33cb8 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -19,6 +19,8 @@
>  
>  #include "libxl_osdeps.h" /* must come before any other headers */
>  
> +#include "libxl_sr_stream_format.h"
> +
>  #include 
>  #include 
>  #include 
> @@ -3121,6 +3123,42 @@ typedef void libxl__domain_create_cb(libxl__egc *egc,
>   libxl__domain_create_state*,
>   int rc, uint32_t domid);
>  
> +/* State for manipulating a libxl migration v2 stream */
> +typedef struct libxl__stream_read_state libxl__stream_read_state;
> +
> +struct libxl__stream_read_state {
> +/* filled by the user */
> +libxl__ao *ao;
> +int fd;
> +void (*completion_callback)(libxl__egc *egc,
> +libxl__domain_create_state *dcs,
> +int rc);
> +/* Private */
> +int rc;
> +bool running;
> +libxl__datacopier_state dc;
> +size_t expected_len;
> +libxl_sr_hdr hdr;
> +libxl_sr_rec_hdr rec_hdr;
> +void *rec_body;
> +};
> +
> +_hidden void libxl__stream_read_start(libxl__egc *egc,
> +  libxl__stream_read_state *stream);
> +
> +_hidden void libxl__stream_read_continue(libxl__egc *egc,
> + libxl__stream_read_state *stream);
> +
> +_hidden void libxl__stream_read_abort(libxl__egc *egc,
> +  libxl__stream_read_state *stream, int 
> rc);
> +
> +static inline bool libxl__stream_read_inuse(
> +const libxl__stream_read_state *stream)
> +{
> +return stream->running;
> +}
> +
> +
>  struct libxl__domain_create_state {
>  /* filled in by user */
>  libxl__ao *ao;
> @@ -3137,6 +3175,7 @@ struct libxl__domain_create_state {
>  libxl__stub_dm_spawn_state dmss;
>  /* If we're not doing stubdom, we use only dmss.dm,
>   * for the non-stubdom device model. */
> +libxl__stream_read_state srs;
>  libxl__save_helper_state shs;
>  /* necessary if the domain creation failed and we have to destroy it */
>  libxl__domain_destroy_state dds;
> diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
> new file mode 100644
> index 000..9cdaadf
> --- /dev/null
> +++ b/tools/libxl/libxl_stream_read.c
> @@ -0,0 +1,485 @@
> +/*
> + * Copyright (C) 2015  Citrix Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +
> +#include "libxl_internal.h"
> +
> +/*
> + * Infrastructure for reading and acting on the contents of a libxl migration
> + * stream. There are a lot of moving parts here.
> + *
> + * Entry points from outside:
> + *  - libxl__stream_read_start()
> + * - Set up reading a stream from the start.
> + *
> + *  - libxl__stream_read_continue()
> + * - Set up reading the next record from a started stream.
> + *
> + * The principle loop functionality involves reading the stream header, then
> + * reading a record at time and acting upon it.  It follows the callbacks:
> + *
> + *  - stream_header_done()
> + *  - stream_record_header_done()
> + *  -

Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-16 Thread Andrew Cooper 
On 16/06/15 16:35, Ian Campbell wrote:
>
 +if (dc->writefd == -1) {
 +ret = ERROR_FAIL;
 +LOGE(ERROR, "Unable to open '%s'", path);
 +goto err;
 +}
 +dc->maxsz = dc->bytes_to_read = rec_hdr->length - sizeof(*emu_hdr);
 +stream->expected_len = dc->used = 0;
>>> expecting 0? This differs from the pattern common everywhere else and
>>> I'm not sure why.
>> The datacopier has been overloaded so many times, it is messy to use.
>>
>> In this case, we are splicing from read fd to a write fd, rather than to
>> a local buffer.
>>
>> Therefore, when the IO is complete, we expect 0 bytes in the local
>> buffer, as all data should end up in the fd.
> I think using 2 or more data copiers to cover the different
> configurations might help? You can still reuse one for the normal record
> processing but a separate dedicated one for writing the emu to a file
> might iron out a wrinkle.

I specifically do not want to risk setting two dc's running at the same
time with the same readfd.

As all of this code is reading from a single readfd, I have specifically
avoided having multiple dc structures lying around.

>
>>> And given that why not handle this in some central place rather than in
>>> the emulator only place?
>> Experimentally, some versions of Qemu barf if they have trailing zeros
>> in save file.  I think they expect to find eof() on a qemu record boundary.
> What I was suggesting was to do the padding in the core, where it would
> often be a zero nop, but would save mistakes (or duplication) if some
> other record also needs such handling in the future.

I don't see an easy way of doing that, given the current divergence in
setting the dcs up in the first place.

~Andrew

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

Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-16 Thread Ian Campbell 
On Tue, 2015-06-16 at 16:01 +0100, Andrew Cooper wrote:
> On 16/06/15 15:31, Ian Campbell wrote:
> > On Mon, 2015-06-15 at 14:44 +0100, Andrew Cooper wrote:
> >> From: Ross Lagerwall 
> >>
> >> Signed-off-by: Ross Lagerwall 
> >> Signed-off-by: Andrew Cooper 
> >> CC: Ian Campbell 
> >> CC: Ian Jackson 
> >> CC: Wei Liu 
> > Overall looks good, I've got some comments below and I think it almost
> > certainly wants eyes from Ian who knows more about the dc infra etc.
> >
> >> +void libxl__stream_read_start(libxl__egc *egc,
> >> +  libxl__stream_read_state *stream)
> >> +{
> >> +libxl__datacopier_state *dc = &stream->dc;
> >> +int ret = 0;
> >> +
> >> +/* State initialisation. */
> >> +assert(!stream->running);
> >> +
> >> +memset(dc, 0, sizeof(*dc));
> > libxl__datacopier_init, please
> 
> That call is made by libxl__datacopier_start() each and every time, and
> unlike here, is matched with an equivalent _kill() call.

Hrm, I think I'd best defer to Ian J on what the right way to deal with
a dc being setup and resused in this way is.

> >> +/* Start reading the stream header. */
> >> +dc->readwhat = "stream header";
> >> +dc->readbuf = &stream->hdr;
> >> +stream->expected_len = dc->bytes_to_read = sizeof(stream->hdr);
> >> +dc->used = 0;
> >> +dc->callback = stream_header_done;
> > This pattern of resetting and reinitialising the dc occurs in multiple
> > places, I think a helper would be in order, some sort of
> > stream_next_record_init or something perhaps?
> 
> The only feasible helper would have to take everything as parameters; 
> there is insufficient similarity between all users.
> 
> I dunno whether that would be harder to read...

I was more concerned about ensuring everyone does everything (especially
if a new field gets added), having a function with parameters would
cause a compile failure when the addition was (hopefully) propagated to
that function and added as a parameter.

Lets see what Ian thinks.

> 
> >
> >> +void libxl__stream_read_abort(libxl__egc *egc,
> >> +  libxl__stream_read_state *stream, int rc)
> >> +{
> >> +stream_failed(egc, stream, rc);
> >> +}
> >> +
> >> +static void stream_success(libxl__egc *egc, libxl__stream_read_state 
> >> *stream)
> >> +{
> >> +stream->rc = 0;
> >> +stream->running = false;
> >> +
> >> +stream_done(egc, stream);
> > Push the running = false into stream_done and flip the assert there?
> > Logically the stream is still running until it is done, so having done
> > assert it isn't running seems counter-intuitive.
> 
> This is more for piece of mind.  stream_done() my strictly only ever be
> called once, hence its assert.

assert(stream->running);
stream->running = false

in stream_done() gives the same piece of mind, doesn't it?

> >> +if (onwrite || dc->used != stream->expected_len) {
> >> +ret = ERROR_FAIL;
> >> +LOG(ERROR, "write %d, err %d, expected %zu, got %zu",
> >> +onwrite, errnoval, stream->expected_len, dc->used);
> >> +goto err;
> >> +}
> > I think you need to check errnoval == 0 in the !onwrite case, otherwise
> > you may miss a read error?
> 
> "dc->used != stream->expected_len" covers all possible read errors, in
> the "something went wrong" kind of way.

With the current implementation, perhaps, but the doc doesn't seem to
say you can rely on it (an appropriate reaction might be to change the
doc rather than the code, I don't mind).

> > Sharing the dc between al these differing usages is starting to rankle a
> > little, but I think it is necessary because it may have queued data from
> > a previous read which was larger than the current record, correct?
> >
> > Hrm, isn't setting dc->used = 0 on each reset potentially throwing some
> > stuff away?
> 
> We should never be in a case where we are setting up a new read/write
> from the dc with any previous IO pending.

Essentially because dc uses its idea of bytes remaining to do the read,
the scenario I was imagining only comes about if the dc is reading large
blocks and segmenting them later, which isn't how it is used here.

> >> +if (dc->writefd == -1) {
> >> +ret = ERROR_FAIL;
> >> +LOGE(ERROR, "Unable to open '%s'", path);
> >> +goto err;
> >> +}
> >> +dc->maxsz = dc->bytes_to_read = rec_hdr->length - sizeof(*emu_hdr);
> >> +stream->expected_len = dc->used = 0;
> > expecting 0? This differs from the pattern common everywhere else and
> > I'm not sure why.
> 
> The datacopier has been overloaded so many times, it is messy to use.
> 
> In this case, we are splicing from read fd to a write fd, rather than to
> a local buffer.
> 
> Therefore, when the IO is complete, we expect 0 bytes in the local
> buffer, as all data should end up in the fd.

I think using 2 or more data copiers to cover the different
configurations might help? You can still reuse one for the normal record
processing but a

Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-16 Thread Andrew Cooper 
On 16/06/15 15:31, Ian Campbell wrote:
> On Mon, 2015-06-15 at 14:44 +0100, Andrew Cooper wrote:
>> From: Ross Lagerwall 
>>
>> Signed-off-by: Ross Lagerwall 
>> Signed-off-by: Andrew Cooper 
>> CC: Ian Campbell 
>> CC: Ian Jackson 
>> CC: Wei Liu 
> Overall looks good, I've got some comments below and I think it almost
> certainly wants eyes from Ian who knows more about the dc infra etc.
>
>> +void libxl__stream_read_start(libxl__egc *egc,
>> +  libxl__stream_read_state *stream)
>> +{
>> +libxl__datacopier_state *dc = &stream->dc;
>> +int ret = 0;
>> +
>> +/* State initialisation. */
>> +assert(!stream->running);
>> +
>> +memset(dc, 0, sizeof(*dc));
> libxl__datacopier_init, please

That call is made by libxl__datacopier_start() each and every time, and
unlike here, is matched with an equivalent _kill() call.

>
>> +dc->ao = stream->ao;
>> +dc->readfd = stream->fd;
>> +dc->writefd = -1;
>> +
>> +/* Start reading the stream header. */
>> +dc->readwhat = "stream header";
>> +dc->readbuf = &stream->hdr;
>> +stream->expected_len = dc->bytes_to_read = sizeof(stream->hdr);
>> +dc->used = 0;
>> +dc->callback = stream_header_done;
> This pattern of resetting and reinitialising the dc occurs in multiple
> places, I think a helper would be in order, some sort of
> stream_next_record_init or something perhaps?

The only feasible helper would have to take everything as parameters; 
there is insufficient similarity between all users.

I dunno whether that would be harder to read...

>
>> +void libxl__stream_read_abort(libxl__egc *egc,
>> +  libxl__stream_read_state *stream, int rc)
>> +{
>> +stream_failed(egc, stream, rc);
>> +}
>> +
>> +static void stream_success(libxl__egc *egc, libxl__stream_read_state 
>> *stream)
>> +{
>> +stream->rc = 0;
>> +stream->running = false;
>> +
>> +stream_done(egc, stream);
> Push the running = false into stream_done and flip the assert there?
> Logically the stream is still running until it is done, so having done
> assert it isn't running seems counter-intuitive.

This is more for piece of mind.  stream_done() my strictly only ever be
called once, hence its assert.

>
>> +static void stream_done(libxl__egc *egc,
>> +libxl__stream_read_state *stream)
>> +{
>> +libxl__domain_create_state *dcs = CONTAINER_OF(stream, *dcs, srs);
>> +
>> +assert(!stream->running);
>> +
>> +stream->completion_callback(egc, dcs, stream->rc);
>> +}
>> +
>> +static void stream_header_done(libxl__egc *egc,
>> +   libxl__datacopier_state *dc,
>> +   int onwrite, int errnoval)
>> +{
>> +libxl__stream_read_state *stream = CONTAINER_OF(dc, *stream, dc);
>> +libxl_sr_hdr *hdr = &stream->hdr;
>> +STATE_AO_GC(dc->ao);
>> +int ret = 0;
>> +
>> +if (onwrite || dc->used != stream->expected_len) {
>> +ret = ERROR_FAIL;
>> +LOG(ERROR, "write %d, err %d, expected %zu, got %zu",
>> +onwrite, errnoval, stream->expected_len, dc->used);
>> +goto err;
>> +}
> I think you need to check errnoval == 0 in the !onwrite case, otherwise
> you may miss a read error?

"dc->used != stream->expected_len" covers all possible read errors, in
the "something went wrong" kind of way.

>
> Also it looks like onwrite can be -1, which is a separate error case.
>
>> +
>> +static void record_header_done(libxl__egc *egc,
>> +   libxl__datacopier_state *dc,
>> +   int onwrite, int errnoval)
>> +{
>> +libxl__stream_read_state *stream = CONTAINER_OF(dc, *stream, dc);
>> +libxl_sr_rec_hdr *rec_hdr = &stream->rec_hdr;
>> +STATE_AO_GC(dc->ao);
>> +int ret = 0;
>> +
>> +if (onwrite || dc->used != stream->expected_len) {
>> +ret = ERROR_FAIL;
>> +LOG(ERROR, "write %d, err %d, expected %zu, got %zu",
>> +onwrite, errnoval, stream->expected_len, dc->used);
>> +goto err;
>> +}
> Same comments wrt the arguments as the previous one.
>
> Maybe a common helper to check (and log) the status at the head of each
> callback? So you can effectively do if (!everything_ok(stream, dc) goto
> err?

I will see what I can do.

>
>> +assert(!ret);
>> +if (rec_hdr->length) {
>> +free(stream->rec_body);
>> +stream->rec_body = NULL;
> reset length too?
>
>> +static void read_emulator_body(libxl__egc *egc,
>> +   libxl__stream_read_state *stream)
>> +{
>> +libxl__domain_create_state *dcs = CONTAINER_OF(stream, *dcs, srs);
>> +libxl__datacopier_state *dc = &stream->dc;
>> +libxl_sr_rec_hdr *rec_hdr = &stream->rec_hdr;
>> +libxl_sr_emulator_hdr *emu_hdr = stream->rec_body;
>> +STATE_AO_GC(stream->ao);
>> +char path[256];
>> +int ret = 0;
>> +
>> +sprintf(path, XC_DEVICE_MODEL_RESTORE_FILE".%u", dcs->guest_domi

Re: [Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-16 Thread Ian Campbell 
On Mon, 2015-06-15 at 14:44 +0100, Andrew Cooper wrote:
> From: Ross Lagerwall 
> 
> Signed-off-by: Ross Lagerwall 
> Signed-off-by: Andrew Cooper 
> CC: Ian Campbell 
> CC: Ian Jackson 
> CC: Wei Liu 

Overall looks good, I've got some comments below and I think it almost
certainly wants eyes from Ian who knows more about the dc infra etc.

> +void libxl__stream_read_start(libxl__egc *egc,
> +  libxl__stream_read_state *stream)
> +{
> +libxl__datacopier_state *dc = &stream->dc;
> +int ret = 0;
> +
> +/* State initialisation. */
> +assert(!stream->running);
> +
> +memset(dc, 0, sizeof(*dc));

libxl__datacopier_init, please

> +dc->ao = stream->ao;
> +dc->readfd = stream->fd;
> +dc->writefd = -1;
> +
> +/* Start reading the stream header. */
> +dc->readwhat = "stream header";
> +dc->readbuf = &stream->hdr;
> +stream->expected_len = dc->bytes_to_read = sizeof(stream->hdr);
> +dc->used = 0;
> +dc->callback = stream_header_done;

This pattern of resetting and reinitialising the dc occurs in multiple
places, I think a helper would be in order, some sort of
stream_next_record_init or something perhaps?

> +void libxl__stream_read_abort(libxl__egc *egc,
> +  libxl__stream_read_state *stream, int rc)
> +{
> +stream_failed(egc, stream, rc);
> +}
> +
> +static void stream_success(libxl__egc *egc, libxl__stream_read_state *stream)
> +{
> +stream->rc = 0;
> +stream->running = false;
> +
> +stream_done(egc, stream);

Push the running = false into stream_done and flip the assert there?
Logically the stream is still running until it is done, so having done
assert it isn't running seems counter-intuitive.

> +static void stream_done(libxl__egc *egc,
> +libxl__stream_read_state *stream)
> +{
> +libxl__domain_create_state *dcs = CONTAINER_OF(stream, *dcs, srs);
> +
> +assert(!stream->running);
> +
> +stream->completion_callback(egc, dcs, stream->rc);
> +}
> +
> +static void stream_header_done(libxl__egc *egc,
> +   libxl__datacopier_state *dc,
> +   int onwrite, int errnoval)
> +{
> +libxl__stream_read_state *stream = CONTAINER_OF(dc, *stream, dc);
> +libxl_sr_hdr *hdr = &stream->hdr;
> +STATE_AO_GC(dc->ao);
> +int ret = 0;
> +
> +if (onwrite || dc->used != stream->expected_len) {
> +ret = ERROR_FAIL;
> +LOG(ERROR, "write %d, err %d, expected %zu, got %zu",
> +onwrite, errnoval, stream->expected_len, dc->used);
> +goto err;
> +}

I think you need to check errnoval == 0 in the !onwrite case, otherwise
you may miss a read error?

Also it looks like onwrite can be -1, which is a separate error case.

> +
> +static void record_header_done(libxl__egc *egc,
> +   libxl__datacopier_state *dc,
> +   int onwrite, int errnoval)
> +{
> +libxl__stream_read_state *stream = CONTAINER_OF(dc, *stream, dc);
> +libxl_sr_rec_hdr *rec_hdr = &stream->rec_hdr;
> +STATE_AO_GC(dc->ao);
> +int ret = 0;
> +
> +if (onwrite || dc->used != stream->expected_len) {
> +ret = ERROR_FAIL;
> +LOG(ERROR, "write %d, err %d, expected %zu, got %zu",
> +onwrite, errnoval, stream->expected_len, dc->used);
> +goto err;
> +}

Same comments wrt the arguments as the previous one.

Maybe a common helper to check (and log) the status at the head of each
callback? So you can effectively do if (!everything_ok(stream, dc) goto
err?

> +assert(!ret);
> +if (rec_hdr->length) {
> +free(stream->rec_body);
> +stream->rec_body = NULL;

reset length too?

> +static void read_emulator_body(libxl__egc *egc,
> +   libxl__stream_read_state *stream)
> +{
> +libxl__domain_create_state *dcs = CONTAINER_OF(stream, *dcs, srs);
> +libxl__datacopier_state *dc = &stream->dc;
> +libxl_sr_rec_hdr *rec_hdr = &stream->rec_hdr;
> +libxl_sr_emulator_hdr *emu_hdr = stream->rec_body;
> +STATE_AO_GC(stream->ao);
> +char path[256];
> +int ret = 0;
> +
> +sprintf(path, XC_DEVICE_MODEL_RESTORE_FILE".%u", dcs->guest_domid);
> +
> +dc->readwhat = "save/migration stream";
> +dc->copywhat = "emulator context";
> +dc->writewhat = "qemu save file";
> +dc->readbuf = NULL;
> +dc->writefd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);

Since it this is all done in the same process (or children of it) with
not setuid etc, I think 0600 would be better to avoid accidentally
leaving the save state world readable (just in case it matters).

Also, should consider whether this fd needs to be subject to the carefd
machinery.

Sharing the dc between al these differing usages is starting to rankle a
little, but I think it is necessary because it may have queued data from
a previous read which was larger than the current record,

[Xen-devel] [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

2015-06-15 Thread Andrew Cooper 
From: Ross Lagerwall 

Signed-off-by: Ross Lagerwall 
Signed-off-by: Andrew Cooper 
CC: Ian Campbell 
CC: Ian Jackson 
CC: Wei Liu 
---
 tools/libxl/Makefile|1 +
 tools/libxl/libxl_internal.h|   39 
 tools/libxl/libxl_stream_read.c |  485 +++
 3 files changed, 525 insertions(+)
 create mode 100644 tools/libxl/libxl_stream_read.c

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index cc9c152..c71c5fe 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -94,6 +94,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
libxl_pci.o \
libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
libxl_internal.o libxl_utils.o libxl_uuid.o \
libxl_json.o libxl_aoutils.o libxl_numa.o libxl_vnuma.o 
\
+   libxl_stream_read.o \
libxl_save_callout.o _libxl_save_msgs_callout.o \
libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
 LIBXL_OBJS += libxl_genid.o
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 101994f..4f33cb8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -19,6 +19,8 @@
 
 #include "libxl_osdeps.h" /* must come before any other headers */
 
+#include "libxl_sr_stream_format.h"
+
 #include 
 #include 
 #include 
@@ -3121,6 +3123,42 @@ typedef void libxl__domain_create_cb(libxl__egc *egc,
  libxl__domain_create_state*,
  int rc, uint32_t domid);
 
+/* State for manipulating a libxl migration v2 stream */
+typedef struct libxl__stream_read_state libxl__stream_read_state;
+
+struct libxl__stream_read_state {
+/* filled by the user */
+libxl__ao *ao;
+int fd;
+void (*completion_callback)(libxl__egc *egc,
+libxl__domain_create_state *dcs,
+int rc);
+/* Private */
+int rc;
+bool running;
+libxl__datacopier_state dc;
+size_t expected_len;
+libxl_sr_hdr hdr;
+libxl_sr_rec_hdr rec_hdr;
+void *rec_body;
+};
+
+_hidden void libxl__stream_read_start(libxl__egc *egc,
+  libxl__stream_read_state *stream);
+
+_hidden void libxl__stream_read_continue(libxl__egc *egc,
+ libxl__stream_read_state *stream);
+
+_hidden void libxl__stream_read_abort(libxl__egc *egc,
+  libxl__stream_read_state *stream, int 
rc);
+
+static inline bool libxl__stream_read_inuse(
+const libxl__stream_read_state *stream)
+{
+return stream->running;
+}
+
+
 struct libxl__domain_create_state {
 /* filled in by user */
 libxl__ao *ao;
@@ -3137,6 +3175,7 @@ struct libxl__domain_create_state {
 libxl__stub_dm_spawn_state dmss;
 /* If we're not doing stubdom, we use only dmss.dm,
  * for the non-stubdom device model. */
+libxl__stream_read_state srs;
 libxl__save_helper_state shs;
 /* necessary if the domain creation failed and we have to destroy it */
 libxl__domain_destroy_state dds;
diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
new file mode 100644
index 000..9cdaadf
--- /dev/null
+++ b/tools/libxl/libxl_stream_read.c
@@ -0,0 +1,485 @@
+/*
+ * Copyright (C) 2015  Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+
+#include "libxl_internal.h"
+
+/*
+ * Infrastructure for reading and acting on the contents of a libxl migration
+ * stream. There are a lot of moving parts here.
+ *
+ * Entry points from outside:
+ *  - libxl__stream_read_start()
+ * - Set up reading a stream from the start.
+ *
+ *  - libxl__stream_read_continue()
+ * - Set up reading the next record from a started stream.
+ *
+ * The principle loop functionality involves reading the stream header, then
+ * reading a record at time and acting upon it.  It follows the callbacks:
+ *
+ *  - stream_header_done()
+ *  - stream_record_header_done()
+ *  - stream_record_body_done()
+ *  - process_record()
+ *
+ * process_record() will choose the correct next action based upon the
+ * record.  Upon completion of the action, the next record header will be read
+ * from the stream.
+ */
+
+static void stream_success(libxl__egc *egc,
+