Re: [Qemu-devel] [PATCH 07/10] qcow2: Implement .bdrv_inactivate

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2818 bytes --]

On 12/22/2015 09:46 AM, Kevin Wolf wrote:
> The callback has to ensure that closing or flushing the image afterwards
> wouldn't cause a write access to the image files. This means that just
> the caches have to be written out, which is part of the existing
> .bdrv_close implementation.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 45 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 17 deletions(-)
> 

Mostly code motion, but I still ended up with questions.

>  
> +static int qcow2_inactivate(BlockDriverState *bs)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    int ret, result = 0;
> +
> +    ret = qcow2_cache_flush(bs, s->l2_table_cache);
> +    if (ret) {
> +        result = ret;
> +        error_report("Failed to flush the L2 table cache: %s",
> +                     strerror(-ret));

I asked Markus if we want error_report_errno() - and ever since then, I
keep finding more and more uses that would benefit from it :)

> +    }
> +
> +    ret = qcow2_cache_flush(bs, s->refcount_block_cache);
> +    if (ret) {
> +        result = ret;
> +        error_report("Failed to flush the refcount block cache: %s",
> +                     strerror(-ret));
> +    }
> +

If the media fails in between these two statements,...

> +    if (result == 0) {
> +        qcow2_mark_clean(bs);

...can't qcow2_mark_clean() fail due to an EIO or other write error?  Do
we care?  I guess the worst is that we didn't mark the image clean after
all, which is no worse than if qemu[-img] had been SIGKILL'd at the same
point where I hypothesized that the media could fail.

> +    }
> +
> +    return result;

If both flushes failed, you return the result set to the return value of
the second flush.  Is it ever possible that the return value of the
first flush might be more useful?

> @@ -1693,23 +1719,7 @@ static void qcow2_close(BlockDriverState *bs)
>      s->l1_table = NULL;
>  
>      if (!(bs->open_flags & BDRV_O_INCOMING)) {

> -        if (!ret1 && !ret2) {
> -            qcow2_mark_clean(bs);
> -        }
> +        qcow2_inactivate(bs);
>      }

Then again, the lone existing caller in this addition doesn't even care
about the return value.  The only other caller was your new code added
earlier in the series; in 5/10 migration_completion(), which uses the
value as a conditional but doesn't try to call strerror(-ret).

Since this is mostly code motion, any semantic changes you would want to
make based on my questions above probably belong in their own patches.
Therefore, for this patch as written,

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]