[Qemu-devel] [PATCH 0/4] Cleanups for block code

[Qemu-devel] [PATCH 0/4] Cleanups for block code

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

Reading through some of the blk code, I noticed a lot of cases where
we mix and match between hard-coded values for the block size of 512
and using BDRV_SECTOR_SIZE. Trying to clean it up a bit and change the
512 constants to BDRV_SECTOR_SIZE as it is more explaning when reading
the code.

In addition it fixes up a case in bdrv_open where we did the division,
just to multiply back to the original value for no real reason.

Cheers,
Jes


Jes Sorensen (4):
  Cleanup: bdrv_open() no need to shift total_size just to shift back.
  Cleanup: Be consistent and use BDRV_SECTOR_SIZE instead of 512
  Cleanup: raw-posix.c: Be more consistent using BDRV_SECTOR_SIZE
    instead of 512
  Cleanup: virtio-blk.c: Be more consistent using BDRV_SECTOR_SIZE
    instead

 block.c           |   17 +++++++++--------
 block/raw-posix.c |   20 +++++++++++---------
 hw/virtio-blk.c   |    7 ++++---
 3 files changed, 24 insertions(+), 20 deletions(-)

[Qemu-devel] [PATCH 1/4] Cleanup: bdrv_open() no need to shift total_size just to shift back.

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

In bdrv_open() there is no need to shift total_size >> 9 just to
multiply it by 512 again just a few lines later, since this is the
only place the variable is used.

Mask with BDRV_SECTOR_MASK to protect against case where we are
passed a corrupted image.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 block.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index cd70730..a094454 100644
--- a/block.c
+++ b/block.c
@@ -521,7 +521,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
             bdrv_delete(bs1);
             return ret;
         }
-        total_size = bdrv_getlength(bs1) >> BDRV_SECTOR_BITS;
+        total_size = bdrv_getlength(bs1) & BDRV_SECTOR_MASK;
 
         if (bs1->drv && bs1->drv->protocol_name)
             is_protocol = 1;
@@ -540,7 +540,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
         bdrv_qcow2 = bdrv_find_format("qcow2");
         options = parse_option_parameters("", bdrv_qcow2->create_options, NULL);
 
-        set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size * 512);
+        set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size);
         set_option_parameter(options, BLOCK_OPT_BACKING_FILE, backing_filename);
         if (drv) {
             set_option_parameter(options, BLOCK_OPT_BACKING_FMT,
-- 
1.6.5.2

[Qemu-devel] [PATCH 2/4] Cleanup: Be consistent and use BDRV_SECTOR_SIZE instead of 512

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Clean up block.c and use BDRV_SECTOR_SIZE rather than hard coded
numbers (512) when referring to sector size throughout the code.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 block.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index a094454..88806fb 100644
--- a/block.c
+++ b/block.c
@@ -683,7 +683,7 @@ int bdrv_commit(BlockDriverState *bs)
     int64_t i, total_sectors;
     int n, j, ro, open_flags;
     int ret = 0, rw_ret = 0;
-    unsigned char sector[512];
+    unsigned char sector[BDRV_SECTOR_SIZE];
     char filename[1024];
     BlockDriverState *bs_rw, *bs_ro;
 
@@ -823,7 +823,8 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
 static int bdrv_check_request(BlockDriverState *bs, int64_t sector_num,
                               int nb_sectors)
 {
-    return bdrv_check_byte_request(bs, sector_num * 512, nb_sectors * 512);
+    return bdrv_check_byte_request(bs, sector_num * BDRV_SECTOR_SIZE,
+                                   nb_sectors * BDRV_SECTOR_SIZE);
 }
 
 /* return < 0 if error. See bdrv_write() for the return codes */
@@ -1058,7 +1059,7 @@ struct partition {
 static int guess_disk_lchs(BlockDriverState *bs,
                            int *pcylinders, int *pheads, int *psectors)
 {
-    uint8_t buf[512];
+    uint8_t buf[BDRV_SECTOR_SIZE];
     int ret, i, heads, sectors, cylinders;
     struct partition *p;
     uint32_t nr_sects;
@@ -1561,7 +1562,7 @@ static QObject* bdrv_info_stats_bs(BlockDriverState *bs)
                              "} }",
                              bs->rd_bytes, bs->wr_bytes,
                              bs->rd_ops, bs->wr_ops,
-                             bs->wr_highest_sector * 512);
+                             bs->wr_highest_sector * (long)BDRV_SECTOR_SIZE);
     dict  = qobject_to_qdict(res);
 
     if (*bs->device_name) {
@@ -2265,7 +2266,7 @@ static int bdrv_read_em(BlockDriverState *bs, int64_t sector_num,
 
     async_ret = NOT_DONE;
     iov.iov_base = (void *)buf;
-    iov.iov_len = nb_sectors * 512;
+    iov.iov_len = nb_sectors * BDRV_SECTOR_SIZE;
     qemu_iovec_init_external(&qiov, &iov, 1);
     acb = bdrv_aio_readv(bs, sector_num, &qiov, nb_sectors,
         bdrv_rw_em_cb, &async_ret);
@@ -2296,7 +2297,7 @@ static int bdrv_write_em(BlockDriverState *bs, int64_t sector_num,
 
     async_ret = NOT_DONE;
     iov.iov_base = (void *)buf;
-    iov.iov_len = nb_sectors * 512;
+    iov.iov_len = nb_sectors * BDRV_SECTOR_SIZE;
     qemu_iovec_init_external(&qiov, &iov, 1);
     acb = bdrv_aio_writev(bs, sector_num, &qiov, nb_sectors,
         bdrv_rw_em_cb, &async_ret);
-- 
1.6.5.2

[Qemu-devel] [PATCH 3/4] Cleanup: raw-posix.c: Be more consistent using BDRV_SECTOR_SIZE instead of 512

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Clean up raw-posix.c to be more consistent using BDRV_SECTOR_SIZE
instead of hard coded 512 values.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 block/raw-posix.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 7541ed2..3f0701b 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -392,8 +392,9 @@ static int raw_read(BlockDriverState *bs, int64_t sector_num,
 {
     int ret;
 
-    ret = raw_pread(bs, sector_num * 512, buf, nb_sectors * 512);
-    if (ret == (nb_sectors * 512))
+    ret = raw_pread(bs, sector_num * BDRV_SECTOR_SIZE, buf,
+                    nb_sectors * BDRV_SECTOR_SIZE);
+    if (ret == (nb_sectors * BDRV_SECTOR_SIZE))
         ret = 0;
     return ret;
 }
@@ -480,8 +481,9 @@ static int raw_write(BlockDriverState *bs, int64_t sector_num,
                      const uint8_t *buf, int nb_sectors)
 {
     int ret;
-    ret = raw_pwrite(bs, sector_num * 512, buf, nb_sectors * 512);
-    if (ret == (nb_sectors * 512))
+    ret = raw_pwrite(bs, sector_num * BDRV_SECTOR_SIZE, buf,
+                     nb_sectors * BDRV_SECTOR_SIZE);
+    if (ret == (nb_sectors * BDRV_SECTOR_SIZE))
         ret = 0;
     return ret;
 }
@@ -494,7 +496,7 @@ static int qiov_is_aligned(QEMUIOVector *qiov)
     int i;
 
     for (i = 0; i < qiov->niov; i++) {
-        if ((uintptr_t) qiov->iov[i].iov_base % 512) {
+        if ((uintptr_t) qiov->iov[i].iov_base % BDRV_SECTOR_SIZE) {
             return 0;
         }
     }
@@ -703,7 +705,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
     /* Read out options */
     while (options && options->name) {
         if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
-            total_size = options->value.n / 512;
+            total_size = options->value.n / BDRV_SECTOR_SIZE;
         }
         options++;
     }
@@ -713,7 +715,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
     if (fd < 0) {
         result = -errno;
     } else {
-        if (ftruncate(fd, total_size * 512) != 0) {
+        if (ftruncate(fd, total_size * BDRV_SECTOR_SIZE) != 0) {
             result = -errno;
         }
         if (close(fd) != 0) {
@@ -976,7 +978,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
     /* Read out options */
     while (options && options->name) {
         if (!strcmp(options->name, "size")) {
-            total_size = options->value.n / 512;
+            total_size = options->value.n / BDRV_SECTOR_SIZE;
         }
         options++;
     }
@@ -989,7 +991,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
         ret = -errno;
     else if (!S_ISBLK(stat_buf.st_mode) && !S_ISCHR(stat_buf.st_mode))
         ret = -ENODEV;
-    else if (lseek(fd, 0, SEEK_END) < total_size * 512)
+    else if (lseek(fd, 0, SEEK_END) < total_size * BDRV_SECTOR_SIZE)
         ret = -ENOSPC;
 
     close(fd);
-- 
1.6.5.2

[Qemu-devel] [PATCH 4/4] Cleanup: virtio-blk.c: Be more consistent using BDRV_SECTOR_SIZE instead

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Clean up virtio-blk.c to be more consistent using BDRV_SECTOR_SIZE
instead of hard coded 512 values.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 hw/virtio-blk.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 5d7f1a2..80d51c4 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -277,7 +277,7 @@ static void virtio_blk_handle_write(BlockRequest *blkreq, int *num_writes,
     }
 
     blkreq[*num_writes].sector = req->out->sector;
-    blkreq[*num_writes].nb_sectors = req->qiov.size / 512;
+    blkreq[*num_writes].nb_sectors = req->qiov.size / BDRV_SECTOR_SIZE;
     blkreq[*num_writes].qiov = &req->qiov;
     blkreq[*num_writes].cb = virtio_blk_rw_complete;
     blkreq[*num_writes].opaque = req;
@@ -296,7 +296,8 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
     }
 
     acb = bdrv_aio_readv(req->dev->bs, req->out->sector, &req->qiov,
-                         req->qiov.size / 512, virtio_blk_rw_complete, req);
+                         req->qiov.size / BDRV_SECTOR_SIZE,
+                         virtio_blk_rw_complete, req);
     if (!acb) {
         virtio_blk_rw_complete(req, -EIO);
     }
@@ -505,7 +506,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
     s->bs = conf->dinfo->bdrv;
     s->conf = conf;
     s->rq = NULL;
-    s->sector_mask = (s->conf->logical_block_size / 512) - 1;
+    s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
 
     s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
-- 
1.6.5.2

Re: [Qemu-devel] [PATCH 0/4] Cleanups for block code

[Kevin Wolf]

Am 27.05.2010 16:20, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Hi,
> 
> Reading through some of the blk code, I noticed a lot of cases where
> we mix and match between hard-coded values for the block size of 512
> and using BDRV_SECTOR_SIZE. Trying to clean it up a bit and change the
> 512 constants to BDRV_SECTOR_SIZE as it is more explaning when reading
> the code.
> 
> In addition it fixes up a case in bdrv_open where we did the division,
> just to multiply back to the original value for no real reason.
> 
> Cheers,
> Jes
> 
> 
> Jes Sorensen (4):
>   Cleanup: bdrv_open() no need to shift total_size just to shift back.
>   Cleanup: Be consistent and use BDRV_SECTOR_SIZE instead of 512
>   Cleanup: raw-posix.c: Be more consistent using BDRV_SECTOR_SIZE
>     instead of 512
>   Cleanup: virtio-blk.c: Be more consistent using BDRV_SECTOR_SIZE
>     instead

Thanks, applied all to the block branch.

Kevin