Re: [Xen-devel] [PATCH v3 17/28] tools/libxl: Infrastructure for reading a libxl migration v2 stream

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Mon, 2015-07-13 at 13:01 +0100, Andrew Cooper wrote:

> +void libxl__stream_read_start(libxl__egc *egc,
> +                              libxl__stream_read_state *stream)
> +{
> +    libxl__datacopier_state *dc = &stream->dc;
> +    int rc = 0;
> +
> +    stream->running = true;

Did you drop the assert prior to this on purpose?

> +
> +static void stream_header_done(libxl__egc *egc,
> +                               libxl__datacopier_state *dc,
> +                               int rc, 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);
> +
> +    if (rc || onwrite || errnoval) {
> +        rc = ERROR_FAIL;

Sorry for not noticing this before but if we come here because of rc
then this clobbers the existing value, which I think is undesirable.

So I think the "if (rc) goto err" should be pulled out of this check.
(the one line form is acceptable in this case).

This no doubt applies to most of these functions.

> +static void stream_continue(libxl__egc *egc,
> +                            libxl__stream_read_state *stream)
> +{
> +    STATE_AO_GC(stream->ao);
> +
> +    /*
> +     * Must not mutually recurse with process_record().
> +     *
> +     * For records whose processing function is synchronous
> +     * (e.g. TOOLSTACK), process_record() does not start another async
> +     * operation, and a further operation should be started.
> +     *
> +     * A naive solution, which would function in general, would be for
> +     * process_record() to call stream_continue().

Wouldn't the more obvious naive thing to do be to call
setup_read_record? That's also potentially recursing I think since the
chain of callbacks may also end in a stream_continue.

>   However, this
> +     * would allow the content of the stream to cause mutual
> +     * recursion, and possibly for us to fall off our stack.
> +     *
> +     * Instead, process_record() indicates with its return value
> +     * whether it a further operation needs to start, and the

"whether it"? 

Ian.


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