[Qemu-devel] Re: [PATCH 3/3] block/nbd: Make the NBD block device use the AIO interface

[Nicholas Thomas]

Hi again,

Thanks for looking through the patches. I'm just going through and
making the suggested changes now. I've also got qemu-nbd and block/nbd.c
working over IPv6 :) - hopefully I'll be able to provide patches in a
couple of days. Just a few questions about some of the changes...

Canceled requests: 
> > +
> > +
> > +static void nbd_aio_cancel(BlockDriverAIOCB *blockacb)
> > +{
> > +    NBDAIOCB *acb = (NBDAIOCB *)blockacb;
> > +
> > +    /*
> > +     * We cannot cancel the requests which are already sent to
> > +     * the servers, so we just complete the request with -EIO here.
> > +     */
> > +    acb->common.cb(acb->common.opaque, -EIO);
> > +    acb->canceled = 1;
> > +}
> 
> I think you need to check for acb->canceled before you write to the
> associated buffer when receiving the reply for a read request. The
> buffer might not exist any more after the request is cancelled.

I "borrowed" this code from block/sheepdog.c (along with a fair few
other bits ;) ) - which doesn't seem to do any special checking for
cancelled write requests. So if there is a potential SIGSEGV here, I
guess sheepdog is also vulnerable.

Presumably, in aio_read_response, I'd need to allocate a buffer of the
appropriate size, read the data from the socket, then deallocate the
buffer? Or would fsetpos(fd, ftell(fd)+data_len) be sufficient?

qemu-io doesn't seem to have any provisions for testing this particular
code path - can you think of any tests I could run to ensure
correctness? I've no idea under what situations an IO request might be
cancelled.


> > +static BlockDriverAIOCB *nbd_aio_readv(BlockDriverState *bs,
> > +        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
> > +        BlockDriverCompletionFunc *cb, void *opaque)
> > +{
> > [...]
> > +    for (i = 0; i < qiov->niov; i++) {
> > +        memset(qiov->iov[i].iov_base, 0, qiov->iov[i].iov_len);
> > +    }
> 
> qemu_iovec_memset?
> 
> What is this even for? Aren't these zeros overwritten anyway?

Again, present in sheepdog - but it does seem to work fine without it.
I'll remove it from NBD.