Re: [Xen-devel] [PATCH v6 05/11] libxl_qmp: Implementation of libxl__ev_qmp_*

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Nov 12, 2018 at 04:49:24PM +0000, Anthony PERARD wrote:
(...)
> +/* QMP FD callbacks */
> +
> +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__json_object *o = NULL;
> +    libxl__ev_qmp *ev = CONTAINER_OF(ev_fd, *ev, qmp_efd);
> +
> +    if (revents & (POLLHUP)) {
> +        LOGD(ERROR, ev->domid, "received POLLHUP from QMP socket");
> +        rc = ERROR_PROTOCOL_ERROR_QMP;
> +        goto out;
> +    }
> +    if (revents & ~(POLLIN|POLLOUT)) {
> +        LOGD(ERROR, ev->domid,
> +             "unexpected poll event 0x%x on QMP socket (expected POLLIN "
> +             "and/or POLLOUT)",
> +            revents);
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    if (revents & POLLOUT) {
> +        rc = qmp_ev_callback_writable(gc, ev, fd);
> +        if (rc)
> +            goto out;
> +    }
> +
> +    if (revents & POLLIN) {
> +        rc = qmp_ev_callback_readable(egc, ev, fd);
> +        if (rc)
> +            goto out;
> +
> +        /* parse input */
> +        while (1) {
> +            /* parse rx buffer to find one json object */
> +            rc = qmp_ev_get_next_msg(egc, ev, &o);
> +            if (rc == ERROR_NOTFOUND) {
> +                rc = 0;
> +                break;
> +            } else if (rc)
> +                goto out;
> +
> +            /* Must be last and return when the user callback is called */
> +            rc = qmp_ev_handle_message(egc, ev, o);
> +            if (rc < 0)
> +                goto out;
> +            if (rc == 1) {
> +                /* user callback has been called */
> +                return;
> +            }
> +        }
> +    }
> +
> +    qmp_ev_ensure_reading_writing(gc, ev);
> +
> +out:
> +    if (rc) {
> +        LOGD(ERROR, ev->domid,
> +             "Error happend with the QMP connection to QEMU");
> +
> +        /* On error, deallocate all private ressources */
> +        libxl__ev_qmp_dispose(gc, ev);
> +
> +        /* And tell libxl__ev_qmp user about the error */
> +        ev->callback(egc, ev, NULL, rc); /* must be last */
> +    }
> +}
> +
(...)
> +static int qmp_ev_callback_readable(libxl__egc *egc,
> +                                    libxl__ev_qmp *ev, int fd)
> +{
> +    EGC_GC;
> +
> +    while (1) {
> +        ssize_t r;
> +
> +        /* Check if the buffer still have space, or increase size */
> +        if (ev->rx_buf_size - ev->rx_buf_used < QMP_RECEIVE_BUFFER_SIZE) {
> +            ev->rx_buf_size = max(ev->rx_buf_size * 2,
> +                               (size_t)QMP_RECEIVE_BUFFER_SIZE * 2);
> +            assert(ev->rx_buf_size <= QMP_MAX_SIZE_RX_BUF);
> +            if (ev->rx_buf_size > QMP_MAX_SIZE_RX_BUF) {
> +                LOGD(ERROR, ev->domid,
> +                     "QMP receive buffer is too big (%ld > %lld)",
> +                     ev->rx_buf_size, QMP_MAX_SIZE_RX_BUF);
> +                return ERROR_BUFFERFULL;

What if you receive multiple messages (events?), but actually a single
message do fit in a buffer? I think it would be better to stop receiving
the data in the case of buffer full, but try to find a valid message
before erroring out.

> +            }
> +            ev->rx_buf = libxl__realloc(NOGC, ev->rx_buf, ev->rx_buf_size);
> +        }
> +
> +        r = read(fd, ev->rx_buf + ev->rx_buf_used,
> +                 ev->rx_buf_size - ev->rx_buf_used);
> +        if (r < 0) {
> +            if (errno == EINTR) {
> +                continue;
> +            }
> +            if (errno == EWOULDBLOCK) {
> +                break;
> +            }
> +            LOGED(ERROR, ev->domid, "error reading QMP socket");
> +            return ERROR_FAIL;
> +        }
> +
> +        if (r == 0) {
> +            LOGD(ERROR, ev->domid, "Unexpected EOF on QMP socket");
> +            return ERROR_PROTOCOL_ERROR_QMP;
> +        }
> +
> +        LOG_QMP("received %ldB: '%.*s'", r,
> +                (int)r, ev->rx_buf + ev->rx_buf_used);
> +
> +        ev->rx_buf_used += r;
> +        assert(ev->rx_buf_used <= ev->rx_buf_size);
> +    }
> +
> +    return 0;
> +}

(...)

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

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