From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Jackson <Ian.Jackson@eu.citrix.com>
Subject: Re: [PATCH v2 for-4.6 3/3] tools/libxl: Only continue
	stream operations if the stream is still in progress
Date: Tue, 28 Jul 2015 14:41:11 +0100
Message-ID: <21943.34423.586101.713988@mariner.uk.xensource.com>
References: <1438015647-25377-1-git-send-email-andrew.cooper3@citrix.com>
	<1438015647-25377-4-git-send-email-andrew.cooper3@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1438015647-25377-4-git-send-email-andrew.cooper3@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>, Ian Campbell <Ian.Campbell@citrix.com>, Xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

Andrew Cooper writes ("[PATCH v2 for-4.6 3/3] tools/libxl: Only continue stream operations if the stream is still in progress"):
> Part of the callback contract with check_all_finished() is that each
> running parallel task shall call it exactly once.
...
> @@ -738,14 +738,16 @@ void libxl__xc_domain_restore_done(libxl__egc *egc, void *dcs_void,
>          goto err;
>      }
>  
> -    /*
> -     * Libxc has indicated that it is done with the stream.  Resume reading
> -     * libxl records from it.
> -     */
> -    stream_continue(egc, stream);
> -
>   err:
>      check_all_finished(egc, stream, rc);
> +
> +    if (libxl__stream_read_inuse(stream)) {
> +        /*
> +         * Libxc has indicated that it is done with the stream.  Resume re\
ading
> +         * libxl records from it.
> +         */
> +        stream_continue(egc, stream);
> +    }

This doesn't look convincing to me.  (Also, wrap damage in the
comment.)  It may be possible to prove that it's not buggy, but it's
certainly confusing.  In general the the which sets up the next
callback should be at the end of functions, because that way
reentrancy hazards are more easily seen to be avoided.

Why not just do this:

       /*
        * Libxc has indicated that it is done with the stream.  Resume reading
        * libxl records from it.
        */
       stream_continue(egc, stream);
  +    return;

    err:
       check_all_finished(egc, stream, rc);

?

And the same in the next one.

Ian.