Re: [Qemu-block] [PATCH 03/16] blkverify: Convert s->test_file to BdrvChild

2015-09-23 Thread Alberto Garcia 
On Thu 17 Sep 2015 03:48:07 PM CEST, Kevin Wolf wrote:

> @@ -151,7 +151,7 @@ static void blkverify_close(BlockDriverState *bs)
>  {
>  BDRVBlkverifyState *s = bs->opaque;
>  
> -bdrv_unref(s->test_file);
> +bdrv_unref_child(bs, s->test_file);
>  s->test_file = NULL;
>  }

You are using bdrv_unref_child() here whereas in quorum you kept
bdrv_unref().

In principle both seem correct because if you don't detach the children
in the driver's close function then bdrv_close() will take care of doing
it, but is there a reason why you are using different methods?

Berto

Re: [Qemu-block] [PATCH 03/16] blkverify: Convert s->test_file to BdrvChild

2015-09-23 Thread Alberto Garcia 
On Wed 23 Sep 2015 03:58:23 PM CEST, Kevin Wolf wrote:

>> > @@ -151,7 +151,7 @@ static void blkverify_close(BlockDriverState *bs)
>> >  {
>> >  BDRVBlkverifyState *s = bs->opaque;
>> >  
>> > -bdrv_unref(s->test_file);
>> > +bdrv_unref_child(bs, s->test_file);
>> >  s->test_file = NULL;
>> >  }
>> 
>> You are using bdrv_unref_child() here whereas in quorum you kept
>> bdrv_unref().
>> 
>> In principle both seem correct because if you don't detach the
>> children in the driver's close function then bdrv_close() will take
>> care of doing it, but is there a reason why you are using different
>> methods?
>
> Because consistency is overrated? Or simply carelessness?

Ah ok :-)

> VMDK uses bdrv_unref_child() as well, so I guess quorum is the one
> that should be changed?

You can keep my R-b if you do so.

Berto

Re: [Qemu-block] [PATCH 03/16] blkverify: Convert s->test_file to BdrvChild

2015-09-23 Thread Alberto Garcia 
On Thu 17 Sep 2015 03:48:07 PM CEST, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  block/blkverify.c | 41 +
>  1 file changed, 21 insertions(+), 20 deletions(-)
>

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH 03/16] blkverify: Convert s->test_file to BdrvChild

2015-09-23 Thread Kevin Wolf 
Am 23.09.2015 um 15:01 hat Alberto Garcia geschrieben:
> On Thu 17 Sep 2015 03:48:07 PM CEST, Kevin Wolf wrote:
> 
> > @@ -151,7 +151,7 @@ static void blkverify_close(BlockDriverState *bs)
> >  {
> >  BDRVBlkverifyState *s = bs->opaque;
> >  
> > -bdrv_unref(s->test_file);
> > +bdrv_unref_child(bs, s->test_file);
> >  s->test_file = NULL;
> >  }
> 
> You are using bdrv_unref_child() here whereas in quorum you kept
> bdrv_unref().
> 
> In principle both seem correct because if you don't detach the children
> in the driver's close function then bdrv_close() will take care of doing
> it, but is there a reason why you are using different methods?

Because consistency is overrated? Or simply carelessness?

In the end (not in this series), I'd like to remove all of this from the
close functions (as the comment in block.c says) and let it all be
handled in bdrv_close(). But until then, we should stay reasonably
consistent indeed.

VMDK uses bdrv_unref_child() as well, so I guess quorum is the one that
should be changed?

Kevin

Re: [Qemu-block] [PATCH 03/16] blkverify: Convert s->test_file to BdrvChild

2015-09-22 Thread Max Reitz 
On 17.09.2015 15:48, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  block/blkverify.c | 41 +
>  1 file changed, 21 insertions(+), 20 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature