Re: [Qemu-devel] [patch 4/5] block stream: add support for partial streaming

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, Jan 04, 2012 at 12:39:57PM +0000, Stefan Hajnoczi wrote:
> On Fri, Dec 30, 2011 at 10:03 AM, Marcelo Tosatti <mtosatti@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.