On Wed, Jan 04, 2012 at 12:39:57PM +0000, Stefan Hajnoczi wrote:
> On Fri, Dec 30, 2011 at 10:03 AM, Marcelo Tosatti <mtosa...@redhat.com> wrote:
> > +int bdrv_is_allocated_base(BlockDriverState *top,
> > + BlockDriverState *base,
> > + int64_t sector_num,
> > + int nb_sectors, int *pnum)
>
> Since this function runs in coroutine context it should be marked:
> int coroutine_fn bdrv_co_is_allocated_base(...)
>
> 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.
> > +{
> > + BlockDriverState *intermediate;
> > + int ret, n;
> > +
> > + ret = bdrv_co_is_allocated(top, sector_num, nb_sectors, &n);
> > + if (ret) {
> > + *pnum = n;
> > + return ret;
> > + }
> > +
> > + /*
> > + * Is the unallocated chunk [sector_num, n] also
> > + * unallocated between base and top?
> > + */
> > + intermediate = top->backing_hd;
>
> 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
->backing_hd and ->backing_file properly, which is reasonable.
Will drop the abstractions.
> > @@ -129,7 +141,10 @@ retry:
> > bdrv_disable_zero_detection(bs);
> >
> > if (sector_num == end && ret == 0) {
> > - bdrv_change_backing_file(bs, NULL, NULL);
> > + const char *base_id = NULL;
> > + if (base)
> > + base_id = s->backing_file_id;
> > + bdrv_change_backing_file(bs, base_id, NULL);
>
> 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
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
> >
> > s = block_job_create(&stream_job_type, bs, cb, opaque);
> > s->base = base;
> > + if (base_id)
> > + strncpy(s->backing_file_id, base_id, sizeof(s->backing_file_id) -
> > 1);
>
> cutils.c:pstrcpy() is nicer than strncpy(), it does not need the size - 1.
Done.
