Re: [Xen-devel] [PATCH v2 2/5] libxl: react correctly to POLLHUP

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-07-27 at 18:01 +0100, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH v2 2/5] libxl: react correctly to POLLHUP"):
> > 2. Provide a new callback function pointer for POLLHUP:
> >     struct libxl__datacopier_state {
> >         ...
> >         libxl__datacopier_callback *callback;
> >         libxl__datacopier_callback *callback_pollhup;
> >         ...
> >     }
> 
> I have done this.  The result (untested, but compiles) is below.  This
> should not be applied until someone has been able to test it on NetBSD.
> 
> From: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Subject: [PATCH v3] libxl: react correctly to bootloader pty master POLLHUP
> 
> Receive POLLHUP on the bootloader master pty is not an error.
> Hopefully it means that the bootloader has exited and therefore the
> pty slave side has no process group any more.  (At least NetBSD
> indicates POLLHUP on the master in this case.)
> 
> So send the bootloader SIGKILL; if it has already exited then this has
> no effect (except that on some versions of NetBSD it erroneously
> returns ESRCH and we print a harmless warning) and we will then
> collect the bootloader's exit status and be satisfied.
> 
> In order to implement this we need to provide a way for users of
> datacopier to handle POLLHUP rather than treating it as fatal.
> 
> We rename bootloader_abort to bootloader_stop since it now no longer
> only applies to error situations.
> 
> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> 
> -
> Changes in v3:
>  * datacopier provides new interface for handling POLLHUP
>  * Do not ignore errors on the xenconsole pty
>  * Rename bootloader_abort.
> 
> ---
>  tools/libxl/libxl_aoutils.c    |   23 +++++++++++++++++++++++
>  tools/libxl/libxl_bootloader.c |   34 ++++++++++++++++++++++++----------
>  tools/libxl/libxl_internal.h   |    5 ++++-
>  3 files changed, 51 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 99972a2..4bd5484 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -97,11 +97,31 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, 
> libxl__datacopier_state *dc,
>      LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
>  }
>  
> +static int datacopier_pollhup_handled(libxl__egc *egc,
> +                                      libxl__datacopier_state *dc,
> +                                      short revents, int onwrite)
> +{
> +    STATE_AO_GC(dc->ao);
> +
> +    if (dc->callback_pollhup && (revents & POLLHUP)) {
> +        LOG(DEBUG, "received POLLHUP on %s during copy of %s",
> +            onwrite ? dc->writewhat : dc->readwhat,
> +            dc->copywhat);
> +        libxl__datacopier_kill(dc);
> +        dc->callback(egc, dc, onwrite, -1);

Shouldn't this be dc->callback_poolhup?

> @@ -508,7 +508,10 @@ static void bootloader_gotptys(libxl__egc *egc, 
> libxl__openpty_state *op)
>      bl->keystrokes.maxsz = BOOTLOADER_BUF_OUT;
>      bl->keystrokes.copywhat =
>          GCSPRINTF("bootloader input for domain %"PRIu32, bl->domid);
> -    bl->keystrokes.callback = bootloader_keystrokes_copyfail;
> +    bl->keystrokes.callback =         bootloader_keystrokes_copyfail;
> +    bl->keystrokes.callback_pollhup = bootloader_keystrokes_copyfail;
> +        /* pollhup gets called with errnoval==-1 which is not otherwise
> +         * possible since errnos are nonnegative, so it's unambiguous */

So the split into two callbacks is just for convenience of some future
user, rather than something needed in this patch? Or have I missed
somewhere which sets callback but not callback_pollhup?

Ian.


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