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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback 
and read data"):
> On Wed, Oct 10, 2018 at 04:47:08PM +0100, Ian Jackson wrote:
> > 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!

It's less than 80 but not less than 75.  I have sent a patch to change
this in CODING_STYLE.  We can argue about this in that other thread.
(In practical terms these lines do result in wrap damage on my
screen, which makes it significantly harder to read, so I'm not just
quibbling for no reason.)

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

You're right that this is not properly documented.  You'll see that I
have sent a patch to fix that.

In practice, if the application has its own event loop which uses
epoll and EPOLLET then indeed you'll only get the callback once per
transition from unready to ready.  Some event-like descriptor
readiness reporting mechanisms support only those semantics.

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

Err, sorry, yes.

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

I think you might not get POLLHUP.  For example if qemu did
shutdown(,SHUT_WR);  Thanks.

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

I will do that and patchbomb myself with the resulting email and then
reply as if you had sent it to me.

Thanks,
Ian.

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