Hi again Kevin, all, Thanks for applying the first four patches, and apologies for taking so long to get back to you. I've found the time to take your comments on-board and re-do the last patch, + the string-leak patch; I'll send them on shortly, I just wanted to make a few notes on yours, first.
> > + * If there'an unrecoverable error reading from the socket, [...] > > Something's missing here? ;-) Documentation on how reconnections happen ;) - big fat TODO. [...] > > > + free_aio_req(s, a); > > + } > > + nbd_finish_aiocb(acb); > > + } > > + > > + nbd_teardown_connection(s); > > And now there's no way to get the disk back to life without a reboot? Do > I understand correctly that now trying to access the disk will always > return -EBADF? That's right - this is the case with the current code too. I want to put the reconnection logic into a separate patch, but haven't actually written it yet. > > +static void nbd_aio_read_response(void *opaque) > > +{ > > + BDRVNBDState *s = opaque; > > + AIOReq *aio_req = NULL; > > + NBDAIOCB *acb; > > + NBDReply rsp; > > + > > + size_t total; > > + ssize_t ret; > > + int rest; > > + > > + if (s->current_req == NULL && QTAILQ_EMPTY(&s->reqs_for_reply_head)) { > > + return; > > + } > > + > > + /* Build our nbd_reply object if we've got it */ > > + if (s->current_req && (s->nbd_rsp_offset == sizeof(NBDReply))) { > > + nbd_buf_to_reply((uint8_t *)&s->nbd_rsp_buf, &rsp); > > + } > > + > > + if (s->current_req == NULL) { > > + /* Try to read a header */ > > Factor this whole block out in its own function? I wasn't sure here if you meant the preceding, or following, code - I guessed the latter, anyway, as that made the most sense. Done in the following patch. > > + QEMUIOVector hdr; > > + qemu_iovec_init(&hdr, 1); > > + qemu_iovec_add(&hdr, ((&s->nbd_rsp_buf) + s->nbd_rsp_offset), > > + (sizeof(NBDReply) - s->nbd_rsp_offset)); > > + ret = readv(s->sock, hdr.iov, hdr.niov); > > + qemu_iovec_destroy(&hdr); > > + > > + if (ret == -1) { > > + nbd_handle_io_err(s, aio_req, socket_error()); > > + return; > > + } [...] > > +ssize_t nbd_wr_aio(int fd, QEMUIOVector *qiov, size_t len, off_t offset, > > + bool do_read) > > Isn't this name misleading? The function is completely synchronous. It > just happens not to block because it's only called when the socket is ready. As far as I understand it, a read/write may still block (or return EAGAIN, EWOULDBLOCK, EINTR, etc) even if select has marked the socket as ready for reads or writes. So it's aio, because we don't loop on those errors. Anyway, I've renamed it to nbd_qiov_wr, since that's just as relevant.