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

[Stefan Hajnoczi]

On Fri, Dec 30, 2011 at 10:03 AM, Marcelo Tosatti <address@hidden> 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.

> +{
> +    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.

> @@ -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.

> @@ -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.