Re: [Qemu-devel] [PATCH v2 09/17] block: introduce bdrv_get_block_status API

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 19/07/2013 15:43, Eric Blake ha scritto:
> On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
>> For now, bdrv_get_block_status is just another name for
>> bdrv_is_allocated. The next patches will add more flags.
>> 
>> This also touches all block drivers with a mostly mechanical
>> rename.  The sole exception is cow; because it calls
>> cow_co_is_allocated from the read code, we keep that function and
>> make cow_co_get_block_status a wrapper.
>> 
>> Signed-off-by: Paolo Bonzini <address@hidden> --- v1->v2:
>> rebase after vmdk changes
> 
> Reviewed-by: Eric Blake <address@hidden>
> 
>> 
>> @@ -370,7 +376,7 @@ static BlockDriver bdrv_cow = {
>> 
>> .bdrv_read              = cow_co_read, .bdrv_write             =
>> cow_co_write, -    .bdrv_co_is_allocated   =
>> cow_co_is_allocated, +    .bdrv_co_get_block_status   =
>> cow_co_get_block_status,
> 
> Is it worth realigning indentation now that you have a longer
> name?
> 
>> +++ b/block/qcow.c @@ -395,7 +395,7 @@ static uint64_t
>> get_cluster_offset(BlockDriverState *bs, return cluster_offset; 
>> }
>> 
>> -static int coroutine_fn qcow_co_is_allocated(BlockDriverState
>> *bs, +static int64_t coroutine_fn
>> qcow_co_get_block_status(BlockDriverState *bs, int64_t
>> sector_num, int nb_sectors, int *pnum)
> 
> Is it worth fixing alignment while you touch this?
> 
>> { BDRVQcowState *s = bs->opaque; @@ -896,7 +896,7 @@ static
>> BlockDriver bdrv_qcow = {
>> 
>> .bdrv_co_readv          = qcow_co_readv, .bdrv_co_writev
>> = qcow_co_writev, -    .bdrv_co_is_allocated   =
>> qcow_co_is_allocated, +    .bdrv_co_get_block_status   =
>> qcow_co_get_block_status,
> 
> Another spot for realignment?
> 
>> +++ b/block/qcow2.c @@ -640,7 +640,7 @@ static int
>> qcow2_reopen_prepare(BDRVReopenState *state, return 0; }
>> 
>> -static int coroutine_fn qcow2_co_is_allocated(BlockDriverState
>> *bs, +static int64_t coroutine_fn
>> qcow2_co_get_block_status(BlockDriverState *bs, int64_t
>> sector_num, int nb_sectors, int *pnum)
> 
> alignment?
> 
>> { BDRVQcowState *s = bs->opaque; @@ -1784,7 +1784,7 @@ static
>> BlockDriver bdrv_qcow2 = { .bdrv_reopen_prepare  =
>> qcow2_reopen_prepare, .bdrv_create        = qcow2_create, 
>> .bdrv_has_zero_init = bdrv_has_zero_init_1, -
>> .bdrv_co_is_allocated = qcow2_co_is_allocated, +
>> .bdrv_co_get_block_status = qcow2_co_get_block_status, 
>> .bdrv_set_key       = qcow2_set_key, .bdrv_make_empty    =
>> qcow2_make_empty,
>> 
> 
> wow, this is already an alignment mess before your change
> 
>> +++ b/block/qed.c @@ -667,7 +667,7 @@ static void
>> qed_is_allocated_cb(void *opaque, int ret, uint64_t offset,
>> size_t l } }
>> 
>> -static int coroutine_fn
>> bdrv_qed_co_is_allocated(BlockDriverState *bs, +static int64_t
>> coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs, 
>> int64_t sector_num, int nb_sectors, int *pnum)
> 
> alignment
> 
>> +++ b/block/raw-posix.c @@ -1084,7 +1084,7 @@ static int
>> raw_create(const char *filename, QEMUOptionParameter *options) *
>> 'nb_sectors' is the max value 'pnum' should be set to.  If
>> nb_sectors goes * beyond the end of the disk image it will be
>> clamped. */ -static int coroutine_fn
>> raw_co_is_allocated(BlockDriverState *bs, +static int64_t
>> coroutine_fn raw_co_get_block_status(BlockDriverState *bs, 
>> int64_t sector_num, int nb_sectors, int *pnum)
> 
> and again
> 
>> { @@ -1200,7 +1200,7 @@ static BlockDriver bdrv_file = { 
>> .bdrv_close = raw_close, .bdrv_create = raw_create, 
>> .bdrv_has_zero_init = bdrv_has_zero_init_1, -
>> .bdrv_co_is_allocated = raw_co_is_allocated, +
>> .bdrv_co_get_block_status = raw_co_get_block_status,
>> 
>> .bdrv_aio_readv = raw_aio_readv, .bdrv_aio_writev =
>> raw_aio_writev,
> 
> here, nothing was aligned, so you actually met status quo :)
> 
> But that raises a question of consistency between drivers...
> 
>> +++ b/block/raw.c @@ -35,11 +35,11 @@ static void
>> raw_close(BlockDriverState *bs) { }
>> 
>> -static int coroutine_fn raw_co_is_allocated(BlockDriverState
>> *bs, +static int64_t coroutine_fn
>> raw_co_get_block_status(BlockDriverState *bs, int64_t
>> sector_num, int nb_sectors, int *pnum)
> 
> alignment.  You get the picture; I'll quit pointing it out, since
> it doesn't affect semantics.

Got it, let's make everything aligned in a separate patch if deemed
useful.  I didn't want to taint "git blame".

Paolo

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJR8RZUAAoJEBvWZb6bTYby2OYP/jDW+qofVnm/sEE0x6j3p/JD
9JlK3kBKIJ3IQLFBWWz7/scNNF4Y0mPZ1U1ylU95F/BZzyntURsCPLGIBrScgx43
L5V1auiHV/aNEWnAD2sHDfkumiS0x8nzq6yEu8go2ldZBQR+2m9gn5wfWTmNs3nD
OEhg9Fzz7Lka5KwtGDezcjthKV92eBBEQCJAkx0qa/Jf4BrwQo3P4wCn+4nOJoii
veQz809EnQ5VAA1GJ6xIq7iRySLAlbeaG1Km+iB/1gDtD46oRoYOOZGjtvzXQWcY
NIVihNsGx9PUYD/uhGhKqxoOJ9Z9Wx6qLadFYi50vFrvBbZ31+Fl5IRrkEgNZb/G
eWwPmXJIfqoEyQ4MNdJSQuQhBjd0Y3KCvSBOSi+jQR05osyzvHF5iSWL6SshPocN
Ck3blVSxjcZtT2ZhwKxcG0Jk1p9YmnaNGOC8yUxxpgMtJCz7U8G/ki1RaK17VVHU
yFXnvvyxfbLl6tpQtPg0MI/XLpyO1BQeJD8C1+q5F16naVczxIB+N/ugKvhWKxNo
A7Ou9LgiXq09R6hVrh1Qlz65TbSRwP/j7y07fYj74n14IXwoyt1ybi4RjmM6d0Po
vA8uRDb7AVxX4XidFlDe5KTQy3ANGFuhX8Du3JwMvc/KqbqK3rFTT+slDdbJISKJ
AeuJHNxM3AYyK53EK4C3
=SxhS
-----END PGP SIGNATURE-----