[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |