Re: [Xen-devel] [PATCH v4 14/32] libxl_qmp: Implement fd callback and read data

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jul 27, 2018 at 03:05:56PM +0100, Anthony PERARD wrote:
> First step into taking care of the input from QEMU's QMP socket. For
> now, we read data and store them in a buffer.
> 
> Parsing of the data will be done in the following patches.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
> 
> Notes:
>     v4:
>         remove use of a linked list of receive buffer, and use realloc 
> instead.
> 
>  tools/libxl/libxl_internal.h |   9 ++++
>  tools/libxl/libxl_qmp.c      | 101 +++++++++++++++++++++++++++++++++++
>  2 files changed, 110 insertions(+)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 90ac48a659..8c3625a243 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -438,6 +438,15 @@ struct libxl__ev_qmp {
>      libxl__carefd *qmp_cfd;
>      libxl__ev_fd qmp_efd;
>      libxl__qmp_state qmp_state;
> +
> +    /* receive buffer, with:
> +     * buf_size: current allocated size,
> +     * buf_used: actual data in the buffer,
> +     * buf_consumed: data already parsed.  */
> +    char *rx_buf;
> +    size_t buf_size;
> +    size_t buf_used;
> +    size_t buf_consumed;
>  };
>  
>  
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 96a347dd3b..b0554df843 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -75,6 +75,12 @@
>  #  define DEBUG_REPORT_RECEIVED(dom, buf, len) ((void)0)
>  #endif
>  
> +#ifdef DEBUG_QMP_CLIENT
> +#  define LOG_QMP(f, ...) LOGD(DEBUG, ev->domid, f, ##__VA_ARGS__)
> +#else
> +#  define LOG_QMP(f, ...)
> +#endif
> +
>  /*
>   * QMP types & constant
>   */
> @@ -1278,9 +1284,99 @@ int libxl__qmp_initializations(libxl__gc *gc, uint32_t 
> domid,
>  
>  /* ------------ Implementation of libxl__ev_qmp ---------------- */
>  
> +/*
> + * QMP FD callbacks
> + */
> +
> +static int qmp_ev_callback_readable(libxl__egc *egc, libxl__ev_qmp *ev, int 
> fd)
> +{
> +    EGC_GC;
> +    ssize_t r;
> +
> +    if (!ev->rx_buf) {
> +        ev->rx_buf = libxl__malloc(NOGC, QMP_RECEIVE_BUFFER_SIZE);
> +        ev->buf_size = QMP_RECEIVE_BUFFER_SIZE;
> +        ev->buf_used = 0;
> +        ev->buf_consumed = 0;
> +    }
> +
> +    /* Check if last buffer still have space, or increase size */
> +    /* The -1 is because there is always space for a NUL character */
> +    if (ev->buf_used == ev->buf_size - 1) {
> +        ev->buf_size += QMP_RECEIVE_BUFFER_SIZE;
> +        ev->rx_buf = libxl__realloc(NOGC, ev->rx_buf, ev->buf_size);
> +    }
> +
> +    for (;;) {
> +        /* The -1 is because there is always space for a NUL character */
> +        r = read(fd, ev->rx_buf + ev->buf_used,
> +                 ev->buf_size - ev->buf_used - 1);
> +        if (r < 0) {
> +            if (errno == EINTR) continue;
> +            assert(errno);
> +            if (errno == EWOULDBLOCK) {
> +                return 0;
> +            }
> +            LOGED(ERROR, ev->domid, "error reading QMP socket");
> +            return ERROR_FAIL;

I think it would be clearer to use:

assert(errno);
switch (errno) {
case EINTR:
    continue;
case EWOULDBLOCK
    return 0;
default:
    LOGED(...)

> +        }
> +        break;
> +    }
> +
> +    if (r == 0) {
> +        LOGD(ERROR, ev->domid, "No data read on QMP socket");
> +        return 0;
> +    }
> +
> +    LOG_QMP("received %ldB: '%.*s'", r, (int)r, ev->rx_buf + ev->buf_used);
> +
> +    ev->buf_used += r;

Hm, don't you need to do this inside of a loop together with the
realloc, in case the data on the fd is bigger than the current space
on the buffer, and you need to keep growing it until you hit
EWOULDBLOCK?

I think with the current approach you can leave data pending in the fd
if the buffer happens to be smaller than the pending data in the fd?

I think the loop should be something like:

for (;;) {
    1. Check if there's space left in the buffer.
    2. Try to read from the fd.
    3. Parse error, exit loop if errno == EWOULDBLOCK.
    4. ev->buf_used += r;
    5. Restart loop.
}

> +    assert(ev->buf_used < ev->buf_size);
> +
> +    return 0;
> +}
> +
> +static void qmp_ev_callback_error(libxl__egc *egc, libxl__ev_qmp *ev)
> +{
> +    EGC_GC;
> +
> +    LOGD(ERROR, ev->domid, "Error happend with the QMP connection to QEMU");
> +
> +    /* On error, deallocate all private ressources */
> +    libxl__ev_qmp_dispose(gc, ev);
> +}
> +
>  static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
>                                 int fd, short events, short revents)
>  {
> +    EGC_GC;
> +    int rc;
> +
> +    libxl__ev_qmp *ev = CONTAINER_OF(ev_fd, *ev, qmp_efd);
> +
> +    if (revents & (POLLHUP)) {
> +        LOGD(DEBUG, ev->domid, "received POLLHUP from QMP socket");
> +        qmp_ev_callback_error(egc, ev);
> +        return;
> +    }
> +    if (revents & ~(POLLIN|POLLOUT)) {
> +        LOGD(ERROR, ev->domid,
> +             "unexpected poll event 0x%x on QMP socket (expected POLLIN "
> +             "and/or POLLOUT)",
> +            revents);
> +        qmp_ev_callback_error(egc, ev);
> +        return;
> +    }
> +
> +    if (revents & POLLIN) {
> +        rc = qmp_ev_callback_readable(egc, ev, fd);
> +        if (rc)
> +            goto out;
> +    }
> +out:

Since you already have an out label that calls _error, why not use it
for the failure paths above by manually setting rc?

Or else get rid of it and just call _error directly.

    if (revents & POLLIN) {
        rc = qmp_ev_callback_readable(egc, ev, fd);
        if (rc) {
            qmp_ev_callback_error(egc, ev);
            return;
        }
    }

And then you can remove the label altogether.

> +    if (rc) {
> +        qmp_ev_callback_error(egc, ev);
> +    }
>  }
>  
>  static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
> @@ -1346,6 +1442,8 @@ void libxl__ev_qmp_init(libxl__ev_qmp *ev)
>      ev->qmp_cfd = NULL;
>      libxl__ev_fd_init(&ev->qmp_efd);
>      ev->qmp_state = qmp_state_disconnected;
> +
> +    ev->rx_buf = NULL;

Aren't you missing a

ev->buf_size = ev->buf_used = ev->buf_consumed = 0;

?

>  }
>  
>  int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
> @@ -1365,6 +1463,9 @@ void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp 
> *ev)
>  {
>      LOGD(DEBUG, ev->domid, " ev %p", ev);
>  
> +    free(ev->rx_buf);
> +    ev->rx_buf = NULL;

I don't think you need to set rx_buf to NULL if you call _init
afterwards.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel