[PATCH] libxl_qmp: Handle unexpected end-of-socket

[PATCH] libxl_qmp: Handle unexpected end-of-socket

[Anthony PERARD]

When read() return 0, the current code just tries again. But this leads to an
infinite loop if QEMU died too soon.

Also, retry select if a signal was caught.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

---
 tools/libxl/libxl_qmp.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 1777e44..d3b1d53 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -385,18 +385,22 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
         FD_ZERO(&rfds);
         FD_SET(qmp->qmp_fd, &rfds);
 
+do_select_again:
         ret = select(qmp->qmp_fd + 1, &rfds, NULL, NULL, &timeout);
         if (ret == 0) {
             LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR, "timeout");
             return -1;
         } else if (ret < 0) {
+            if (errno == EINTR)
+                goto do_select_again;
             LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR, "Select error");
             return -1;
         }
 
         rd = read(qmp->qmp_fd, qmp->buffer, QMP_RECEIVE_BUFFER_SIZE);
         if (rd == 0) {
-            continue;
+            LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR, "Unexpected end of socket");
+            return -1;
         } else if (rd < 0) {
             LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR, "Socket read error");
             return rd;
-- 
tg: (6674c38..) fix/qmp-end-of-socket (depends on: master)

Re: [PATCH] libxl_qmp: Handle unexpected end-of-socket

[Ian Jackson]

Anthony PERARD writes ("[Xen-devel] [PATCH] libxl_qmp: Handle unexpected end-of-socket"):
> When read() return 0, the current code just tries again. But this leads to an
> infinite loop if QEMU died too soon.

Right.

> Also, retry select if a signal was caught.

Why add another goto ?  I think these goto-based loops are a bad idea,
really.

> +            if (errno == EINTR)
> +                goto do_select_again;

I think this could be "continue".  Do you agree.

Thanks,
Ian.

Re: [PATCH] libxl_qmp: Handle unexpected end-of-socket

[Anthony PERARD]

On Mon, Feb 20, 2012 at 17:09, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Anthony PERARD writes ("[Xen-devel] [PATCH] libxl_qmp: Handle unexpected end-of-socket"):
>> When read() return 0, the current code just tries again. But this leads to an
>> infinite loop if QEMU died too soon.
>
> Right.
>
>> Also, retry select if a signal was caught.
>
> Why add another goto ?  I think these goto-based loops are a bad idea,
> really.
>
>> +            if (errno == EINTR)
>> +                goto do_select_again;
>
> I think this could be "continue".  Do you agree.

Yes, I agree. I'll change that for a while and resend it.

Thanks,

-- 
Anthony PERARD

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

Re: [PATCH] libxl_qmp: Handle unexpected end-of-socket

[Ian Campbell]

On Mon, 2012-02-20 at 17:09 +0000, Ian Jackson wrote:
> Anthony PERARD writes ("[Xen-devel] [PATCH] libxl_qmp: Handle unexpected end-of-socket"):
> > When read() return 0, the current code just tries again. But this leads to an
> > infinite loop if QEMU died too soon.
> 
> Right.
> 
> > Also, retry select if a signal was caught.
> 
> Why add another goto ?  I think these goto-based loops are a bad idea,
> really.
> 
> > +            if (errno == EINTR)
> > +                goto do_select_again;
> 
> I think this could be "continue".  Do you agree.

Also select(2) says:
        On error, -1 is returned, and errno is set appropriately; the
               sets and timeout become undefined, so do not  rely  on  their  contents
               after an error.

So by my reading you need to reinitialise the sets anyway.

Ian.