From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=52174 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1PrYg2-00039D-Uj
	for qemu-devel@nongnu.org; Mon, 21 Feb 2011 11:31:59 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <nick@bytemark.co.uk>) id 1PrYg1-00045m-Jc
	for qemu-devel@nongnu.org; Mon, 21 Feb 2011 11:31:58 -0500
Received: from egg.sh.bytemark.co.uk ([212.110.161.171]:43113)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <nick@bytemark.co.uk>) id 1PrYg1-00044z-FU
	for qemu-devel@nongnu.org; Mon, 21 Feb 2011 11:31:57 -0500
From: Nicholas Thomas <nick@bytemark.co.uk>
In-Reply-To: <4D6261B2.4060108@redhat.com>
References: <1298033729-17945-1-git-send-email-nick@bytemark.co.uk>
	<1298033729-17945-3-git-send-email-nick@bytemark.co.uk>
	<4D6261B2.4060108@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Date: Mon, 21 Feb 2011 16:31:49 +0000
Message-ID: <1298305909.17006.111.camel@desk4.office.bytemark.co.uk>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: [Qemu-devel] Re: [PATCH 3/3] block/nbd: Make the NBD block device
 use the AIO interface
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Kevin Wolf <kwolf@redhat.com>
Cc: stefanha@gmail.com, qemu-devel@nongnu.org

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.