Re: [Xen-devel] [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH v5 03/15] libxl_qmp: Implement fd callback and 
read data"):
> First step into taking care of the input from QEMU's QMP socket. For
> now, we read data and store them in a buffer.
...
> +    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);
> +    }

I think this is unnecessarily complex.  I think you should set buf_*
to 0 in _init, and arrange to realloc(0, new_size) if the buffer is
not big enough for "some space" plus a nul.

Also, you should increase the buffer size exponentially.  And you
should put a limit on the buffer size, in case the sender goes mad.

> +    /* 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);

Lines too long again.  (I will stop complaining about this now but can
you pleae fix it throughout?)

> +    if (r < 0) {
> +        if (errno == EINTR)
> +            return 0;

No, you need to go round again.  I'm afraid the whole of this function
needs to be in a loop.  This is because you are not guaranteed that
you will get more than one call to your readable callback per
transition from not-readable to readable.

> +        assert(errno);
> +        if (errno == EWOULDBLOCK)
> +            return 0;

And again.

> +        LOGED(ERROR, ev->domid, "error reading QMP socket");

I think you need to look up the error handling for (possibly
long-running) connect operation fails on a stream socket which were in
nonblocking mode when you called connect().  You may need to handle
EINPROGRESS from the connect() syscall itself (that was in the
previous patch).

> +    if (r == 0) {
> +        LOGD(ERROR, ev->domid, "No data read on QMP socket");

You mean "unexpected EOF" not "no data read".  Normally this would
mean that qemu died or crashed.

> +        return 0;

Err, and then this needs to be handled as an error, not simply
ignored.  If it happens, it will keep happening.

> +    LOG_QMP("received %ldB: '%.*s'", r, (int)r, ev->rx_buf + ev->buf_used);
> +
> +    ev->buf_used += r;
> +    assert(ev->buf_used < ev->buf_size);

It would be really helpful if these partial functions were to contain
an `xxx' or something where there is missing code.

As it is I know that more code will occur here, but nothing
straightforward in the review will process will spot it the mistake if
you forget to add it...

The loop I mentioned could be in qmp_ev_fd_callback.

TBH I find the split between qmp_ev_fd_callback and
qmp_ev_callback_readable a bit confusing but I know that I'm unusual
in preferring longer functions.

> +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);

Missing /* xxx */ to report the error upwards.


Hrm.  I realise this is going to be annoying, but I think I should
probably stop now because of the lack of xxx's means it's hard for me
to review.

What do you think would be best:
 (i) I should, in my own tree, squash several of your commits down
     so I can review them together
 (ii) You repost with a lot of XXXs added
 (iii) We intend to squash the commits in xen.git ?

I think (ii) is a fair amount of work for polishing an intermediate
state, and probably a waste.  I'm inclined to (i) (iii).  Can you tell
me which patches I should sqaush ?

I think I need up to `libxl_qmp: Respond to QMP greeting' ?

Regards,
Ian.

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