[Qemu-devel] [PATCH 0/2] Refine some vdi code

[Qemu-devel] [PATCH 0/2] Refine some vdi code

[yuchenlin]

From: yuchenlin <yuchenlin@synology.com>

This series refine some code in vdi.c, includes:

* Remvoe CONFIG_VDI_WRITE because there is no reason to leave an always on
  and cannot configure option in the code-side.
* decouple if else if chain to get more readability.

Thanks,
yuchenlin

yuchenlin (2):
  vdi: remove CONFIG_VDI_WRITE
  vdi: refine code for vdi_open

 block/vdi.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH 1/2] vdi: remove CONFIG_VDI_WRITE

[yuchenlin]

From: yuchenlin <yuchenlin@synology.com>

The CONFIG_VDI_WRITE is here when the first time vdi is added.
But there is no reason to leave an always on and cannot configure option
in the code-side.

Signed-off-by: yuchenlin <yuchenlin@synology.com>
---
 block/vdi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 6555cffb88..12f92e7891 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -70,9 +70,6 @@
 /* Enable debug messages. */
 //~ #define CONFIG_VDI_DEBUG
 
-/* Support write operations on VDI images. */
-#define CONFIG_VDI_WRITE
-
 /* Support non-standard block (cluster) size. This is untested.
  * Maybe it will be needed for very large images.
  */
@@ -1016,9 +1013,7 @@ static BlockDriver bdrv_vdi = {
     .bdrv_make_empty = vdi_make_empty,
 
     .bdrv_co_preadv     = vdi_co_preadv,
-#if defined(CONFIG_VDI_WRITE)
     .bdrv_co_pwritev    = vdi_co_pwritev,
-#endif
 
     .bdrv_get_info = vdi_get_info,
 
-- 
2.17.0

[Qemu-devel] [PATCH 2/2] vdi: refine code for vdi_open

[yuchenlin]

From: yuchenlin <yuchenlin@synology.com>

When the condition of each if or else if is true,
the code flow will goto fail. Which means we can decouple
if else if chain to get some readability.

Signed-off-by: yuchenlin <yuchenlin@synology.com>
---
 block/vdi.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 12f92e7891..28fc6210a7 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -405,35 +405,41 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
                    ")", header.signature);
         ret = -EINVAL;
         goto fail;
-    } else if (header.version != VDI_VERSION_1_1) {
+    }
+    if (header.version != VDI_VERSION_1_1) {
         error_setg(errp, "unsupported VDI image (version %" PRIu32 ".%" PRIu32
                    ")", header.version >> 16, header.version & 0xffff);
         ret = -ENOTSUP;
         goto fail;
-    } else if (header.offset_bmap % SECTOR_SIZE != 0) {
+    }
+    if (header.offset_bmap % SECTOR_SIZE != 0) {
         /* We only support block maps which start on a sector boundary. */
         error_setg(errp, "unsupported VDI image (unaligned block map offset "
                    "0x%" PRIx32 ")", header.offset_bmap);
         ret = -ENOTSUP;
         goto fail;
-    } else if (header.offset_data % SECTOR_SIZE != 0) {
+    }
+    if (header.offset_data % SECTOR_SIZE != 0) {
         /* We only support data blocks which start on a sector boundary. */
         error_setg(errp, "unsupported VDI image (unaligned data offset 0x%"
                    PRIx32 ")", header.offset_data);
         ret = -ENOTSUP;
         goto fail;
-    } else if (header.sector_size != SECTOR_SIZE) {
+    }
+    if (header.sector_size != SECTOR_SIZE) {
         error_setg(errp, "unsupported VDI image (sector size %" PRIu32
                    " is not %u)", header.sector_size, SECTOR_SIZE);
         ret = -ENOTSUP;
         goto fail;
-    } else if (header.block_size != DEFAULT_CLUSTER_SIZE) {
+    }
+    if (header.block_size != DEFAULT_CLUSTER_SIZE) {
         error_setg(errp, "unsupported VDI image (block size %" PRIu32
                          " is not %" PRIu64 ")",
                    header.block_size, DEFAULT_CLUSTER_SIZE);
         ret = -ENOTSUP;
         goto fail;
-    } else if (header.disk_size >
+    }
+    if (header.disk_size >
                (uint64_t)header.blocks_in_image * header.block_size) {
         error_setg(errp, "unsupported VDI image (disk size %" PRIu64 ", "
                    "image bitmap has room for %" PRIu64 ")",
@@ -441,15 +447,18 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
                    (uint64_t)header.blocks_in_image * header.block_size);
         ret = -ENOTSUP;
         goto fail;
-    } else if (!qemu_uuid_is_null(&header.uuid_link)) {
+    }
+    if (!qemu_uuid_is_null(&header.uuid_link)) {
         error_setg(errp, "unsupported VDI image (non-NULL link UUID)");
         ret = -ENOTSUP;
         goto fail;
-    } else if (!qemu_uuid_is_null(&header.uuid_parent)) {
+    }
+    if (!qemu_uuid_is_null(&header.uuid_parent)) {
         error_setg(errp, "unsupported VDI image (non-NULL parent UUID)");
         ret = -ENOTSUP;
         goto fail;
-    } else if (header.blocks_in_image > VDI_BLOCKS_IN_IMAGE_MAX) {
+    }
+    if (header.blocks_in_image > VDI_BLOCKS_IN_IMAGE_MAX) {
         error_setg(errp, "unsupported VDI image "
                          "(too many blocks %u, max is %u)",
                           header.blocks_in_image, VDI_BLOCKS_IN_IMAGE_MAX);
-- 
2.17.0

Re: [Qemu-devel] [PATCH 0/2] Refine some vdi code

[Stefan Weil]

Am 30.07.2018 um 04:46 schrieb yuchenlin@synology.com:
> From: yuchenlin <yuchenlin@synology.com>
> 
> This series refine some code in vdi.c, includes:
> 
> * Remvoe CONFIG_VDI_WRITE because there is no reason to leave an always on
>   and cannot configure option in the code-side.
> * decouple if else if chain to get more readability.
> 
> Thanks,
> yuchenlin
> 
> yuchenlin (2):
>   vdi: remove CONFIG_VDI_WRITE
>   vdi: refine code for vdi_open
> 
>  block/vdi.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)


Technically these changes are fine, but personally I prefer my old code.
If else is rendundant here, but redundancy helps humans to understand
the code. CONFIG_VDI_WRITE still has a similar function as it documents
which code parts are relevant for write support.

Stefan

Re: [Qemu-devel] [PATCH 0/2] Refine some vdi code

[yuchenlin]

Hi, Stefan

I agree that redundancy of If else may helps people to understand the code.

However, CONFIG_VDI_WRITE only contributes:

#if defined(CONFIG_VDI_WRITE)
.bdrv_co_pwritev = vdi_co_pwritev,
#endif

I think we don't need CONFIG_VDI_WRITE to document the code.
As its name implies, vdi_co_pwritev shows the code parts for vdi write support.


I appreciated your time and effort for reviews.

Regards,
yuchenlin


Stefan Weil <sw@weilnetz.de> 於 2018-07-30 15:13 寫道：
> Am 30.07.2018 um 04:46 schrieb yuchenlin@synology.com: > From: yuchenlin <yuchenlin@synology.com> > > This series refine some code in vdi.c, includes: > > * Remvoe CONFIG_VDI_WRITE because there is no reason to leave an always on > and cannot configure option in the code-side. > * decouple if else if chain to get more readability. > > Thanks, > yuchenlin > > yuchenlin (2): > vdi: remove CONFIG_VDI_WRITE > vdi: refine code for vdi_open > > block/vdi.c | 32 ++++++++++++++++++-------------- > 1 file changed, 18 insertions(+), 14 deletions(-) Technically these changes are fine, but personally I prefer my old code. If else is rendundant here, but redundancy helps humans to understand the code. CONFIG_VDI_WRITE still has a similar function as it documents which code parts are relevant for write support. Stefan