diff mbox

[v2,14/17] raw-posix: return get_block_status data and flags

Message ID 1373992168-26043-15-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini July 16, 2013, 4:29 p.m. UTC 
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/raw-posix.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Eric Blake July 19, 2013, 7:48 p.m. UTC | #1 
On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/raw-posix.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 

> +++ b/block/raw-posix.c
> @@ -1089,7 +1089,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
>                                              int nb_sectors, int *pnum)
>  {
>      off_t start, data, hole;
> -    int ret;
> +    int64_t ret;
>  
>      ret = fd_open(bs);
>      if (ret < 0) {
> @@ -1097,6 +1097,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
>      }
>  
>      start = sector_num * BDRV_SECTOR_SIZE;
> +    ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;

off_t is a signed type; if you are compiling on a platform with 32-bit
off_t, is it possible that you will get unintended sign extension for
values of 'start' between 2 and 4 GB?  Or are such files already
impossible to open?  [Or do we intentionally require off_t be 64-bits on
all platforms we care about?]

Reviewed-by: Eric Blake <eblake@redhat.com>
Paolo Bonzini July 25, 2013, 12:16 p.m. UTC | #2 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 19/07/2013 21:48, Eric Blake ha scritto:
>>> start = sector_num * BDRV_SECTOR_SIZE; +    ret =
>>> BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
> off_t is a signed type; if you are compiling on a platform with
> 32-bit off_t, is it possible that you will get unintended sign
> extension for values of 'start' between 2 and 4 GB?  Or are such
> files already impossible to open?  [Or do we intentionally require
> off_t be 64-bits on all platforms we care about?]

Good question.  If we don't, we should.

In the meanwhile, I'll change start/data/hole to be int64_t.

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

iQIcBAEBAgAGBQJR8RcVAAoJEBvWZb6bTYbymN0P/15/doObltN+2vCE1WtQD8IZ
Woqd/Itct/1KXHCLv423ThO23/WpE3KSPna25a19sGeJQSXdZB7E2shDtxNErEob
a0c0n6yMzDI0iU+UX1/mPacoIZt5tiWHzd3W4sTIgVDjHUa1Xctgp8bv3gNFNHzh
Ese6mdNNhJQCPqA67qLhzSGfnWUCxGV0F9HDK23dfYJUQ8yoEnRWqLYjRH0qgEG4
LRydUCyfU3BjvmRlLLRMrJHxSJKQKbYMO94IY1ZPqKn1TxKPnOHdIYOgNP8wFAWA
3uA5kc0r9qEmSyh01dbQ+bBkp9lRzDRgxrmii4LZXiQvF6kAXxrY5fEuUHrewGwC
1KeFccGLkgWH1jI19M8URjzbjQwGm79yYIqlt4ZLDwam88VXlxYse7odaRESGSWY
HQqak7WSlDFPlvNj7eR3lruGN3E9F6XAoMCVwRnPRbrx9G9ezH302VF7IXHvlFJu
yK+iRwoWUSkKPWVBxiMrI5HAWlt4N6Akje2VPhkaJQYBQ0LwLhMiDWfMJySW9M7I
qNz/CjbMljwEGOEjYZ7PUKd0H76mcX1xwDi3ofBkJn3Bu74+C5xs7ZRRNO/tIu2c
mVxxYbP7R3dTDdF6sOTVIjUt9M7ObDJ0I0Y0aev1BSTv5tiZb9KB+ef9DRn2Drhk
cVcuJYJiEvL3JV9J5zDE
=ANdZ
-----END PGP SIGNATURE-----
diff mbox

Patch

diff --git a/block/raw-posix.c b/block/raw-posix.c
index dbc65b0..d011cfd 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1089,7 +1089,7 @@  static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
                                             int nb_sectors, int *pnum)
 {
     off_t start, data, hole;
-    int ret;
+    int64_t ret;
 
     ret = fd_open(bs);
     if (ret < 0) {
@@ -1097,6 +1097,7 @@  static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
     }
 
     start = sector_num * BDRV_SECTOR_SIZE;
+    ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
 
 #ifdef CONFIG_FIEMAP
 
@@ -1114,7 +1115,7 @@  static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
     if (ioctl(s->fd, FS_IOC_FIEMAP, &f) == -1) {
         /* Assume everything is allocated.  */
         *pnum = nb_sectors;
-        return 1;
+        return ret;
     }
 
     if (f.fm.fm_mapped_extents == 0) {
@@ -1141,7 +1142,7 @@  static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
 
         /* Most likely EINVAL.  Assume everything is allocated.  */
         *pnum = nb_sectors;
-        return 1;
+        return ret;
     }
 
     if (hole > start) {
@@ -1154,19 +1155,21 @@  static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
         }
     }
 #else
-    *pnum = nb_sectors;
-    return 1;
+    data = 0;
+    hole = start + nb_sectors * BDRV_SECTOR_SIZE;
 #endif
 
     if (data <= start) {
         /* On a data extent, compute sectors to the end of the extent.  */
         *pnum = MIN(nb_sectors, (hole - start) / BDRV_SECTOR_SIZE);
-        return 1;
     } else {
         /* On a hole, compute sectors to the beginning of the next extent.  */
         *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE);
-        return 0;
+        ret &= ~BDRV_BLOCK_DATA;
+        ret |= BDRV_BLOCK_ZERO;
     }
+
+    return ret;
 }
 
 static coroutine_fn BlockDriverAIOCB *raw_aio_discard(BlockDriverState *bs,