diff mbox series

[v5,09/23] block: Switch BdrvCoGetBlockStatusData to byte-based

Message ID 20171004020048.26379-10-eblake@redhat.com
State New
Headers show
Series make bdrv_get_block_status byte-based | expand

Commit Message

Eric Blake Oct. 4, 2017, 2 a.m. UTC 
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based.  Convert another internal
type (no semantic change), and rename it to match the corresponding
public function rename.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>

---
v4-v5: no change
v3: rebase to context conflicts, simple enough to keep R-b
v2: rebase to earlier changes
---
 block/io.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

Comments

Jeff Cody Oct. 9, 2017, 8:07 p.m. UTC | #1 
On Tue, Oct 03, 2017 at 09:00:34PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Convert another internal
> type (no semantic change), and rename it to match the corresponding
> public function rename.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> ---
> v4-v5: no change
> v3: rebase to context conflicts, simple enough to keep R-b
> v2: rebase to earlier changes
> ---
>  block/io.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index b879e26154..1857191187 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1744,17 +1744,17 @@ int bdrv_flush_all(void)
>  }
> 
> 
> -typedef struct BdrvCoGetBlockStatusData {
> +typedef struct BdrvCoBlockStatusData {
>      BlockDriverState *bs;
>      BlockDriverState *base;
>      BlockDriverState **file;
> -    int64_t sector_num;
> -    int nb_sectors;
> -    int *pnum;
> +    int64_t offset;
> +    int64_t bytes;
> +    int64_t *pnum;
>      int64_t ret;
>      bool mapping;
>      bool done;
> -} BdrvCoGetBlockStatusData;
> +} BdrvCoBlockStatusData;
> 
>  int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
>                                                          int64_t sector_num,
> @@ -1983,14 +1983,16 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
>  /* Coroutine wrapper for bdrv_get_block_status_above() */
>  static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)
>  {
> -    BdrvCoGetBlockStatusData *data = opaque;
> +    BdrvCoBlockStatusData *data = opaque;
> +    int n;
> 
>      data->ret = bdrv_co_get_block_status_above(data->bs, data->base,
>                                                 data->mapping,
> -                                               data->sector_num,
> -                                               data->nb_sectors,
> -                                               data->pnum,
> +                                               data->offset >> BDRV_SECTOR_BITS,
> +                                               data->bytes >> BDRV_SECTOR_BITS,
> +                                               &n,
>                                                 data->file);
> +    *data->pnum = n * BDRV_SECTOR_SIZE;

This throws a warning:

 block/io.c: In function ‘bdrv_get_block_status_above_co_entry’:
 block/io.c:1995:21: error: ‘n’ may be used uninitialized in this function 
       [-Werror=maybe-uninitialized] *data->pnum = n * BDRV_SECTOR_SIZE;


It is fixed a couple patches from now, but it will still break git-bisect.


>      data->done = true;
>  }
> 
> @@ -2007,13 +2009,14 @@ static int64_t bdrv_common_block_status_above(BlockDriverState *bs,
>                                                BlockDriverState **file)
>  {
>      Coroutine *co;
> -    BdrvCoGetBlockStatusData data = {
> +    int64_t n;
> +    BdrvCoBlockStatusData data = {
>          .bs = bs,
>          .base = base,
>          .file = file,
> -        .sector_num = sector_num,
> -        .nb_sectors = nb_sectors,
> -        .pnum = pnum,
> +        .offset = sector_num * BDRV_SECTOR_SIZE,
> +        .bytes = nb_sectors * BDRV_SECTOR_SIZE,
> +        .pnum = &n,
>          .mapping = mapping,
>          .done = false,
>      };
> @@ -2027,6 +2030,8 @@ static int64_t bdrv_common_block_status_above(BlockDriverState *bs,
>          bdrv_coroutine_enter(bs, co);
>          BDRV_POLL_WHILE(bs, !data.done);
>      }
> +    assert(data.ret < 0 || QEMU_IS_ALIGNED(n, BDRV_SECTOR_SIZE));
> +    *pnum = n >> BDRV_SECTOR_BITS;
>      return data.ret;
>  }
> 
> -- 
> 2.13.6
> 
>
Eric Blake Oct. 9, 2017, 9:30 p.m. UTC | #2 
On 10/09/2017 03:07 PM, Jeff Cody wrote:
> On Tue, Oct 03, 2017 at 09:00:34PM -0500, Eric Blake wrote:
>> We are gradually converting to byte-based interfaces, as they are
>> easier to reason about than sector-based.  Convert another internal
>> type (no semantic change), and rename it to match the corresponding
>> public function rename.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>>
>> ---

>>  static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)
>>  {
>> -    BdrvCoGetBlockStatusData *data = opaque;
>> +    BdrvCoBlockStatusData *data = opaque;
>> +    int n;
>>
>>      data->ret = bdrv_co_get_block_status_above(data->bs, data->base,
>>                                                 data->mapping,
>> -                                               data->sector_num,
>> -                                               data->nb_sectors,
>> -                                               data->pnum,
>> +                                               data->offset >> BDRV_SECTOR_BITS,
>> +                                               data->bytes >> BDRV_SECTOR_BITS,
>> +                                               &n,
>>                                                 data->file);
>> +    *data->pnum = n * BDRV_SECTOR_SIZE;
> 
> This throws a warning:
> 
>  block/io.c: In function ‘bdrv_get_block_status_above_co_entry’:
>  block/io.c:1995:21: error: ‘n’ may be used uninitialized in this function 
>        [-Werror=maybe-uninitialized] *data->pnum = n * BDRV_SECTOR_SIZE;
> 

Ugh - my curse of compiling development builds with -g instead of -O2,
so I didn't see the warning.  Ideally, it would be nice if all the
various bdrv_*_block_status functions would set *pnum to a sane value on
all exit paths, including errors, but that is not the case in
bdrv_co_get_block_status_above().

> 
> It is fixed a couple patches from now, but it will still break git-bisect.

Indeed.  Will fix on respin, as I have to do a v6 anyways for some minor
rebasing conflict resolution.
diff mbox series

Patch

diff --git a/block/io.c b/block/io.c
index b879e26154..1857191187 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1744,17 +1744,17 @@  int bdrv_flush_all(void)
 }


-typedef struct BdrvCoGetBlockStatusData {
+typedef struct BdrvCoBlockStatusData {
     BlockDriverState *bs;
     BlockDriverState *base;
     BlockDriverState **file;
-    int64_t sector_num;
-    int nb_sectors;
-    int *pnum;
+    int64_t offset;
+    int64_t bytes;
+    int64_t *pnum;
     int64_t ret;
     bool mapping;
     bool done;
-} BdrvCoGetBlockStatusData;
+} BdrvCoBlockStatusData;

 int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
                                                         int64_t sector_num,
@@ -1983,14 +1983,16 @@  static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
 /* Coroutine wrapper for bdrv_get_block_status_above() */
 static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)
 {
-    BdrvCoGetBlockStatusData *data = opaque;
+    BdrvCoBlockStatusData *data = opaque;
+    int n;

     data->ret = bdrv_co_get_block_status_above(data->bs, data->base,
                                                data->mapping,
-                                               data->sector_num,
-                                               data->nb_sectors,
-                                               data->pnum,
+                                               data->offset >> BDRV_SECTOR_BITS,
+                                               data->bytes >> BDRV_SECTOR_BITS,
+                                               &n,
                                                data->file);
+    *data->pnum = n * BDRV_SECTOR_SIZE;
     data->done = true;
 }

@@ -2007,13 +2009,14 @@  static int64_t bdrv_common_block_status_above(BlockDriverState *bs,
                                               BlockDriverState **file)
 {
     Coroutine *co;
-    BdrvCoGetBlockStatusData data = {
+    int64_t n;
+    BdrvCoBlockStatusData data = {
         .bs = bs,
         .base = base,
         .file = file,
-        .sector_num = sector_num,
-        .nb_sectors = nb_sectors,
-        .pnum = pnum,
+        .offset = sector_num * BDRV_SECTOR_SIZE,
+        .bytes = nb_sectors * BDRV_SECTOR_SIZE,
+        .pnum = &n,
         .mapping = mapping,
         .done = false,
     };
@@ -2027,6 +2030,8 @@  static int64_t bdrv_common_block_status_above(BlockDriverState *bs,
         bdrv_coroutine_enter(bs, co);
         BDRV_POLL_WHILE(bs, !data.done);
     }
+    assert(data.ret < 0 || QEMU_IS_ALIGNED(n, BDRV_SECTOR_SIZE));
+    *pnum = n >> BDRV_SECTOR_BITS;
     return data.ret;
 }