At Mon, 21 Feb 2011 17:48:49 +0100, Kevin Wolf wrote: > > Am 21.02.2011 17:31, schrieb 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. > > Right, now that you mention it, I seem to remember this from Sheepdog. I > think I had a discussion with Stefan and he convinced me that we could > get away with it in Sheepdog because of some condition that Sheepdog > meets. Not sure any more what that condition was and if it applies to NBD. > > Was it that Sheepdog has a bounce buffer for all requests?
Sheepdog doesn't use a bounce buffer for any requests, and to me, it seems that Sheepdog also needs to check acb->canceled before reading the response of a read request... > >>> +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. > > Maybe Sheepdog reads only partially from the server if blocks are > unallocated or something. Yes, exactly. Thanks, Kazutaka