Re: [Xen-devel] [PATCH 2/3] libxl: Cope with pipes which signal POLLHUP|POLLIN on read eof

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Thu, 2015-04-02 at 16:04 +0100, Ian Jackson wrote:
> Some operating systems (including FreeBSD[1]) signal not just POLLIN
> when a reading pipe reaches EOF, but also POLLHUP.  This is
> permitted[2].  But datacopiers mishandle this, because they always
> treat POLLHUP exceptionally (either reporting it via callback_pollhup,
> or treating it as an error).  datacopiers reading from pipes on such
> OSs can fail (perhaps leaving some data unprocessed) rather than
> completing successfully.
> 
> [1] http://www.greenend.org.uk/rjk/tech/poll.html
> [2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/poll.html
> 
> Most callers in libxl do not care about POLLHUP except as an error
> condition.
> 
> So change the datacopier semantics so that unless callback_pollhup is
> specified we treat POLLHUP|POLLIN as if it were POLLIN - and, read all
> available data).

Punctuation is odd towards the end, a stray looking "-" and an unmatched
")". 

> This fixes the problem which 7e9ec50b0535 was aimed at.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Roger Pau Monnà <roger.pau@xxxxxxxxxx>

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

> CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  tools/libxl/libxl_aoutils.c  |    2 +-
>  tools/libxl/libxl_internal.h |    3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 3942634..c3232a6 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -208,7 +208,7 @@ static void datacopier_readable(libxl__egc *egc, 
> libxl__ev_fd *ev,
>      if (datacopier_pollhup_handled(egc, dc, revents, 0))
>          return;
>  
> -    if (revents & ~POLLIN) {
> +    if (revents & ~POLLIN && revents != (POLLIN|POLLHUP)) {

AIUI you are checking "revents != (POLLIN|POLLHUP)" rather than
"(revents&((POLLIN|POLLHUP)) != (POLLIN|POLLHUP)" because e.g. POLLIN|
POLLHUP|POLLOUT is nonsensical and is never expected.

>          LOG(ERROR, "unexpected poll event 0x%x (should be POLLIN)"
>              " on %s during copy of %s", revents, dc->readwhat, dc->copywhat);
>          datacopier_callback(egc, dc, -1, 0);
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 7f81dd3..c6969f0 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2535,7 +2535,8 @@ typedef struct libxl__datacopier_buf 
> libxl__datacopier_buf;
>   *     errnoval!=0 means we had a read error, logged
>   * onwrite==-1 means some other internal failure, errnoval not valid, logged
>   * If we get POLLHUP, we call callback_pollhup(..., onwrite, -1);
> - * or if callback_pollhup==0 this is an internal failure, as above.
> + * or if callback_pollhup==0 this is treated as eof (if POLLIN|POLLHUP
> + * on the reading fd) or an internal failure (otherwise), as above.
>   * In all cases copier is killed before calling this callback */
>  typedef void libxl__datacopier_callback(libxl__egc *egc,
>       libxl__datacopier_state *dc, int onwrite, int errnoval);



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