From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:33783) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RiRH6-0003Uh-GH for qemu-devel@nongnu.org; Wed, 04 Jan 2012 08:53:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RiRGz-0006Dh-Ux for qemu-devel@nongnu.org; Wed, 04 Jan 2012 08:53:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38445) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RiRGz-0006DR-Ni for qemu-devel@nongnu.org; Wed, 04 Jan 2012 08:52:57 -0500 Date: Wed, 4 Jan 2012 11:52:07 -0200 From: Marcelo Tosatti Message-ID: <20120104135206.GA15452@amt.cnet> References: <20111230100337.226685961@redhat.com> <20111230100503.809473621@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [patch 4/5] block stream: add support for partial streaming List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, stefanha@linux.vnet.ibm.com, qemu-devel@nongnu.org On Wed, Jan 04, 2012 at 12:39:57PM +0000, Stefan Hajnoczi wrote: > On Fri, Dec 30, 2011 at 10:03 AM, Marcelo Tosatti = wrote: > > +int bdrv_is_allocated_base(BlockDriverState *top, > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 BlockDriverStat= e *base, > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int64_t sector_= num, > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int nb_sectors,= int *pnum) >=20 > Since this function runs in coroutine context it should be marked: > int coroutine_fn bdrv_co_is_allocated_base(...) >=20 > The _co_ in the filename is just a convention to identify block layer > functions that execute in coroutine context. The coroutine_fn > annotation is useful if we ever want static checker support that > verifies that coroutine functions are always called from coroutine > context. Done. > > +{ > > + =A0 =A0BlockDriverState *intermediate; > > + =A0 =A0int ret, n; > > + > > + =A0 =A0ret =3D bdrv_co_is_allocated(top, sector_num, nb_sectors, &n= ); > > + =A0 =A0if (ret) { > > + =A0 =A0 =A0 =A0*pnum =3D n; > > + =A0 =A0 =A0 =A0return ret; > > + =A0 =A0} > > + > > + =A0 =A0/* > > + =A0 =A0 * Is the unallocated chunk [sector_num, n] also > > + =A0 =A0 * unallocated between base and top? > > + =A0 =A0 */ > > + =A0 =A0intermediate =3D top->backing_hd; >=20 > This reaches into BlockDriverState->backing_hd. In practice this is > probably the simplest and best way to do it. But if we're going to do > this then do we even need the BlockDriver .bdrv_find_backing_file() > abstraction? We could implement generic code which traverses > ->backing_hd in block.c and if you don't use > BlockDriverState->backing_hd then you're out of luck. Right. Then it becomes necessary for drivers to fill in=20 ->backing_hd and ->backing_file properly, which is reasonable. Will drop the abstractions. > > @@ -129,7 +141,10 @@ retry: > > =A0 =A0 bdrv_disable_zero_detection(bs); > > > > =A0 =A0 if (sector_num =3D=3D end && ret =3D=3D 0) { > > - =A0 =A0 =A0 =A0bdrv_change_backing_file(bs, NULL, NULL); > > + =A0 =A0 =A0 =A0const char *base_id =3D NULL; > > + =A0 =A0 =A0 =A0if (base) > > + =A0 =A0 =A0 =A0 =A0 =A0base_id =3D s->backing_file_id; > > + =A0 =A0 =A0 =A0bdrv_change_backing_file(bs, base_id, NULL); >=20 > This makes me want to move the bdrv_change_backing_file() call out to > blockdev.c where we would perform it on successful completion. That > way we don't need to pass base_id into stream.c at all. A streaming > block job would no longer automatically change the backing file on > completion. Well, it is safer to perform the backing file change under refcounting=20 protection (i don't see the advantage of your suggestion other than the cosmetic aspect of saving a variable). > > @@ -166,6 +182,8 @@ int stream_start(BlockDriverState *bs, B > > > > =A0 =A0 s =3D block_job_create(&stream_job_type, bs, cb, opaque); > > =A0 =A0 s->base =3D base; > > + =A0 =A0if (base_id) > > + =A0 =A0 =A0 =A0strncpy(s->backing_file_id, base_id, sizeof(s->backi= ng_file_id) - 1); >=20 > cutils.c:pstrcpy() is nicer than strncpy(), it does not need the size -= 1. Done.