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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Oct 10, 2018 at 04:47:08PM +0100, Ian Jackson wrote:
> 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.

Ok, I'll see what I can do.

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

But the comment is less than 70chr long, and the next line follow the
coding style, which is "Lines are limited to 75-80 characters.", and
I'm pretty sure that 77 is less that 75-80!

If the coding style described in CODING_STYLE change before my next
submition, I can wrap too long lines ;-).

> > +    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.

I've did it without a loop because POLLIN means "data may be read
without blocking", and I can't find anything speaking about a relation
with transition from one state to the other.

But I can add a loop around the code, as that seems to confuse everyone
that there is none.

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

Surely here I need to return on EWOULDBLOCK. The fd is obviously in
non-readable state, so there is going to be a transition.

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

Looks like it, I'll have a look at the error handling of the connect()
call.

> > +    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.

I think I'm been lazy and have just let this error been handle in the
next event where POLLHUP should be set. I can try to handle this error
better here.

> > +    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' ?

Squashing all the relevant commits might not be that bad, since all
commits implement different functions.

There is one patch that can be reviewed separatly, and then yes, you
need up to 'libxl_qmp: Respond to QMP greeting'. That give us:

pick      libxl_qmp: Separate QMP message generation from qmp_send_prepare
pick      libxl_qmp: Connect to QMP socket
squash    libxl_qmp: Implement fd callback and read data
squash    libxl_qmp: Parse JSON input from QMP
squash    libxl_qmp: Prepare the command to be sent
squash    libxl_qmp: Handle write to QMP socket
squash    libxl_qmp: Handle messages from QEMU
squash    libxl_qmp: Respond to QMP greeting

That give us a patch with only 500 insertions.

Thanks,

-- 
Anthony PERARD

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