From: Ian Campbell <ian.campbell@citrix.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>,
Wei Liu <wei.liu2@citrix.com>,
Yang Hongyang <yanghy@cn.fujitsu.com>,
Ian Jackson <Ian.Jackson@eu.citrix.com>,
Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream
Date: Tue, 16 Jun 2015 15:31:59 +0100 [thread overview]
Message-ID: <1434465119.13744.196.camel@citrix.com> (raw)
In-Reply-To: <1434375880-30914-17-git-send-email-andrew.cooper3@citrix.com>
On Mon, 2015-06-15 at 14:44 +0100, Andrew Cooper wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
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, correct?
Hrm, isn't setting dc->used = 0 on each reset potentially throwing some
stuff away?
> + 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.
> + dc->callback = emulator_body_done;
> +
> + ret = libxl__datacopier_start(dc);
> + if (ret)
> + goto err;
> + return;
> +
> + err:
> + assert(ret);
> + stream_failed(egc, stream, ret);
> +}
> +
> +static void emulator_body_done(libxl__egc *egc,
> + libxl__datacopier_state *dc,
> + int onwrite, int errnoval)
> +{
> + /* Safe to be static, as it is a write-only discard buffer. */
> + static char padding[1U << REC_ALIGN_ORDER];
> +
> + libxl__stream_read_state *stream = CONTAINER_OF(dc, *stream, dc);
> + libxl_sr_rec_hdr *rec_hdr = &stream->rec_hdr;
> + STATE_AO_GC(dc->ao);
> + unsigned int nr_padding_bytes = (1U << REC_ALIGN_ORDER);
> + 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;
> + }
> +
> + /* Undo modifications for splicing the emulator context. */
Hrm, not so much undo as nuke and rebuild. Is that really necessary,
can't you just reset what you need to in the inverse of the other thing?
If there isn't a problem with buffered stuff on callback, then perhaps
it would be clearer to use a separate dc, at least for the qemu side. Or
to _always_ teardown and restart the dc from scratch instead of doing it
partially in some places and fully in others.
> + memset(dc, 0, sizeof(*dc));
> + dc->ao = stream->ao;
> + dc->readfd = stream->fd;
> + dc->writefd = -1;
> +
> + /* Do we need to eat some padding out of the stream? */
Why only now and not for e.g. the xenstore stuff (which doesn't appear
to be explicitly padded).
And given that why not handle this in some central place rather than in
the emulator only place?
Ian.
next prev parent reply other threads:[~2015-06-16 14:31 UTC|newest]
Thread overview: 107+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-15 13:44 [PATCH 00/27] Libxl migration v2 Andrew Cooper
2015-06-15 13:44 ` [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children Andrew Cooper
2015-06-16 13:21 ` Ian Campbell
2015-06-16 13:36 ` Andrew Cooper
2015-06-16 13:47 ` Ian Jackson
2015-06-16 14:05 ` Andrew Cooper
2015-06-16 15:26 ` Ian Campbell
2015-06-16 15:24 ` Ian Campbell
2015-06-16 13:39 ` Ian Jackson
2015-06-15 13:44 ` [PATCH 02/27] tools/libxc: Always compile the compat qemu variables into xc_sr_context Andrew Cooper
2015-06-16 13:22 ` Ian Campbell
2015-06-15 13:44 ` [PATCH 03/27] tools/libxl: Stash all restore parameters in domain_create_state Andrew Cooper
2015-06-16 13:37 ` Ian Campbell
2015-06-16 14:09 ` Andrew Cooper
2015-06-18 2:32 ` Yang Hongyang
2015-06-15 13:44 ` [PATCH 04/27] tools/xl: Mandatory flag indicating the format of the migration stream Andrew Cooper
2015-06-16 13:39 ` Ian Campbell
2015-06-16 14:10 ` Andrew Cooper
2015-06-15 13:44 ` [PATCH 05/27] tools/libxl: Introduce ROUNDUP() Andrew Cooper
2015-06-16 13:39 ` Ian Campbell
2015-06-15 13:44 ` [PATCH 06/27] libxl: cancellation: Preparations for save/restore cancellation Andrew Cooper
2015-06-15 13:44 ` [PATCH 07/27] libxl: cancellation: Handle SIGTERM in save/restore helper Andrew Cooper
2015-06-15 13:44 ` [PATCH 08/27] tools/libxl: Extra APIs for the save helper Andrew Cooper
2015-06-16 13:50 ` Ian Campbell
2015-06-16 15:03 ` Andrew Cooper
2015-06-15 13:44 ` [PATCH 09/27] tools/libxl: Pass restore_fd as a parameter to libxl__xc_domain_restore() Andrew Cooper
2015-06-16 13:53 ` Ian Campbell
2015-06-15 13:44 ` [PATCH 10/27] docs: Libxl migration v2 stream specification Andrew Cooper
2015-06-16 13:58 ` Ian Campbell
2015-07-08 13:49 ` Andrew Cooper
2015-07-08 13:58 ` Ian Campbell
2015-06-15 13:44 ` [PATCH 11/27] tools/python: Libxc migration v2 infrastructure Andrew Cooper
2015-06-16 14:01 ` Ian Campbell
2015-06-15 13:44 ` [PATCH 12/27] tools/python: Libxl " Andrew Cooper
2015-06-15 13:44 ` [PATCH 13/27] tools/python: Verification utility for v2 stream spec compliance Andrew Cooper
2015-06-15 13:44 ` [PATCH 14/27] tools/python: Conversion utility for legacy migration streams Andrew Cooper
2015-06-16 14:01 ` Ian Campbell
2015-06-15 13:44 ` [PATCH 15/27] tools/libxl: Migration v2 stream format Andrew Cooper
2015-06-16 14:04 ` Ian Campbell
2015-06-15 13:44 ` [PATCH 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream Andrew Cooper
2015-06-16 14:31 ` Ian Campbell [this message]
2015-06-16 15:01 ` Andrew Cooper
2015-06-16 15:35 ` Ian Campbell
2015-06-16 15:46 ` Andrew Cooper
2015-06-17 3:09 ` Wen Congyang
2015-06-17 10:15 ` Ian Campbell
2015-06-17 10:49 ` Wen Congyang
2015-06-17 10:55 ` Ian Campbell
2015-06-17 6:03 ` Wen Congyang
2015-06-17 9:47 ` Andrew Cooper
2015-06-17 7:57 ` Wen Congyang
2015-06-17 9:50 ` Andrew Cooper
2015-06-17 10:01 ` Wen Congyang
2015-06-17 10:48 ` Andrew Cooper
2015-06-15 13:44 ` [PATCH 17/27] tools/libxl: Support converting a legacy stream to a " Andrew Cooper
2015-06-16 14:38 ` Ian Campbell
2015-06-16 15:13 ` Andrew Cooper
2015-06-16 15:38 ` Ian Campbell
2015-06-15 13:44 ` [PATCH 18/27] tools/libxl: Convert a legacy stream if needed Andrew Cooper
2015-06-15 13:44 ` [PATCH 19/27] tools/libxc+libxl+xl: Restore v2 streams Andrew Cooper
2015-06-16 14:53 ` Ian Campbell
2015-06-16 15:23 ` Andrew Cooper
2015-06-16 15:39 ` Ian Campbell
2015-06-15 13:44 ` [PATCH 20/27] tools/libxl: Infrastructure for writing a v2 stream Andrew Cooper
2015-06-16 14:57 ` Ian Campbell
2015-06-16 15:28 ` Andrew Cooper
2015-06-17 1:31 ` Yang Hongyang
2015-06-17 9:51 ` Andrew Cooper
2015-06-17 1:39 ` Wen Congyang
2015-06-17 2:24 ` Wen Congyang
2015-06-17 7:38 ` Yang Hongyang
2015-06-17 10:14 ` Wen Congyang
2015-07-10 10:55 ` Ian Campbell
2015-07-10 11:03 ` Andrew Cooper
2015-07-10 11:05 ` Ian Campbell
2015-06-15 13:44 ` [PATCH 21/27] tools/libxc+libxl+xl: Save v2 streams Andrew Cooper
2015-06-16 14:59 ` Ian Campbell
2015-06-15 13:44 ` [PATCH 22/27] docs/libxl: [RFC] Introduce CHECKPOINT_END to support migration v2 remus streams Andrew Cooper
2015-06-16 15:00 ` Ian Campbell
2015-06-16 15:30 ` Andrew Cooper
2015-06-17 3:30 ` Wen Congyang
2015-06-15 13:44 ` [PATCH 23/27] tools/libxl: [RFC] Write checkpoint records into the stream Andrew Cooper
2015-06-16 15:03 ` Ian Campbell
2015-06-16 15:53 ` Andrew Cooper
2015-06-17 7:30 ` Ian Campbell
2015-06-17 9:55 ` Andrew Cooper
2015-06-18 3:13 ` Wen Congyang
2015-06-18 9:44 ` Andrew Cooper
2015-06-15 13:44 ` [PATCH 24/27] tools/libx{c, l}: [RFC] Introduce restore_callbacks.checkpoint() Andrew Cooper
2015-06-16 2:23 ` Yang Hongyang
2015-06-17 8:20 ` Yang Hongyang
2015-06-15 13:44 ` [PATCH 25/27] tools/libxl: [RFC] Handle checkpoint records in a libxl migration v2 stream Andrew Cooper
2015-06-17 7:28 ` Wen Congyang
2015-06-15 13:44 ` [PATCH 26/27] tools/libxc: Drop all XG_LIBXL_HVM_COMPAT code from libxc Andrew Cooper
2015-06-16 15:03 ` Ian Campbell
2015-06-15 13:44 ` [PATCH 27/27] tools/libxl: Drop all knowledge of toolstack callbacks Andrew Cooper
2015-06-16 15:04 ` Ian Campbell
2015-06-16 15:06 ` Andrew Cooper
2015-06-17 10:14 ` Ian Campbell
2015-06-17 10:43 ` Andrew Cooper
2015-06-17 10:53 ` Ian Campbell
2015-06-16 2:21 ` [PATCH 00/27] Libxl migration v2 Yang Hongyang
2015-06-17 1:55 ` Wen Congyang
2015-06-17 9:45 ` Andrew Cooper
2015-07-02 7:33 ` Yang Hongyang
2015-07-02 9:26 ` Andrew Cooper
2015-07-02 9:33 ` Yang Hongyang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1434465119.13744.196.camel@citrix.com \
--to=ian.campbell@citrix.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=ross.lagerwall@citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
--cc=yanghy@cn.fujitsu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.