Skip to contentSkip to conversation

cleanup block driver option handling in vl.c

PATCH Superseded Christoph Hellwig 2 participants 2 messages Apr 5, 2010 – Apr 6, 2010

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 1/1 cleanup block driver option handling in vl.c

Assign directly to the bdrv_flags variable instead of using magic numbers before translating to the BDRV_O_* options.

Signed-off-by Christoph Hellwig <[email protected]>
vl.c +11 -24
Index: qemu/vl.c
===================================================================
@@ -851,10 +851,8 @@ DriveInfo *drive_init(QemuOpts *opts, vo
     QEMUMachine *machine = opaque;
     int max_devs;
     int index;
-    int cache;
-    int aio = 0;
     int ro = 0;
-    int bdrv_flags;
+    int bdrv_flags = 0;
     int on_read_error, on_write_error;
     const char *devaddr;
     DriveInfo *dinfo;

@@ -863,7 +861,6 @@ DriveInfo *drive_init(QemuOpts *opts, vo
     *fatal_error = 1;
 
     translation = BIOS_ATA_TRANSLATION_AUTO;
-    cache = 1;
 
     if (machine && machine->use_scsi) {
         type = IF_SCSI;

@@ -978,11 +975,11 @@ DriveInfo *drive_init(QemuOpts *opts, vo
 
     if ((buf = qemu_opt_get(opts, "cache")) != NULL) {
         if (!strcmp(buf, "off") || !strcmp(buf, "none"))
-            cache = 0;
-        else if (!strcmp(buf, "writethrough"))
-            cache = 1;
+            bdrv_flags |= BDRV_O_NOCACHE;
         else if (!strcmp(buf, "writeback"))
-            cache = 2;
+            bdrv_flags |= BDRV_O_CACHE_WB;
+        else if (!strcmp(buf, "writethrough"))
+            /* this is the default */;
         else {
            fprintf(stderr, "qemu: invalid cache option\n");
            return NULL;

@@ -991,10 +988,10 @@ DriveInfo *drive_init(QemuOpts *opts, vo
 
 #ifdef CONFIG_LINUX_AIO
     if ((buf = qemu_opt_get(opts, "aio")) != NULL) {
+        if (!strcmp(buf, "native"))
+            bdrv_flags |= BDRV_O_NATIVE_AIO;
         if (!strcmp(buf, "threads"))
-            aio = 0;
-        else if (!strcmp(buf, "native"))
-            aio = 1;
+            /* this is the default */;
         else {
            fprintf(stderr, "qemu: invalid aio option\n");
            return NULL;

@@ -1169,20 +1166,10 @@ DriveInfo *drive_init(QemuOpts *opts, vo
         *fatal_error = 0;
         return NULL;
     }
-    bdrv_flags = 0;
     if (snapshot) {
-        bdrv_flags |= BDRV_O_SNAPSHOT;
-        cache = 2; /* always use write-back with snapshot */
-    }
-    if (cache == 0) /* no caching */
-        bdrv_flags |= BDRV_O_NOCACHE;
-    else if (cache == 2) /* write-back */
-        bdrv_flags |= BDRV_O_CACHE_WB;
-
-    if (aio == 1) {
-        bdrv_flags |= BDRV_O_NATIVE_AIO;
-    } else {
-        bdrv_flags &= ~BDRV_O_NATIVE_AIO;
+        /* always use write-back with snapshot */
+        bdrv_flags |= (BDRV_O_SNAPSHOT|BDRV_O_CACHE_WB);
+        bdrv_flags &= ~BDRV_O_NOCACHE;
     }
 
     if (ro == 1) {
#1 lore raw

Am 05.04.2010 16:12, schrieb Christoph Hellwig:

Quoted 32 lines

Assign directly to the bdrv_flags variable instead of using magic numbers before translating to the BDRV_O_* options.

Signed-off-by Christoph Hellwig <[email protected]>

Index: qemu/vl.c
===================================================================

vl.c +1 -4
@@ -851,10 +851,8 @@ DriveInfo *drive_init(QemuOpts *opts, vo
     QEMUMachine *machine = opaque;
     int max_devs;
     int index;
-    int cache;
-    int aio = 0;
     int ro = 0;
-    int bdrv_flags;
+    int bdrv_flags = 0;
     int on_read_error, on_write_error;
     const char *devaddr;
     DriveInfo *dinfo;

@@ -863,7 +861,6 @@ DriveInfo *drive_init(QemuOpts *opts, vo
     *fatal_error = 1;
 
     translation = BIOS_ATA_TRANSLATION_AUTO;
-    cache = 1;
 
     if (machine && machine->use_scsi) {
         type = IF_SCSI;

@@ -978,11 +975,11 @@ DriveInfo *drive_init(QemuOpts *opts, vo
 
     if ((buf = qemu_opt_get(opts, "cache")) != NULL) {

vl.c DriveInfo *drive_init(QemuOpts *opts, vo · line 982

Quoted 10 lines
vl.c +4 -4
         if (!strcmp(buf, "off") || !strcmp(buf, "none"))
-            cache = 0;
-        else if (!strcmp(buf, "writethrough"))
-            cache = 1;
+            bdrv_flags |= BDRV_O_NOCACHE;
         else if (!strcmp(buf, "writeback"))
-            cache = 2;
+            bdrv_flags |= BDRV_O_CACHE_WB;
+        else if (!strcmp(buf, "writethrough"))
+            /* this is the default */;

What about following the coding style and using braces here instead of a semicolon after the comment?

vl.c DriveInfo *drive_init(QemuOpts *opts, vo · line 993

Quoted 10 lines
vl.c +2 -0
         else {
            fprintf(stderr, "qemu: invalid cache option\n");
            return NULL;

@@ -991,10 +988,10 @@ DriveInfo *drive_init(QemuOpts *opts, vo
 
 #ifdef CONFIG_LINUX_AIO
     if ((buf = qemu_opt_get(opts, "aio")) != NULL) {
+        if (!strcmp(buf, "native"))
+            bdrv_flags |= BDRV_O_NATIVE_AIO;
         if (!strcmp(buf, "threads"))

This needs to become an else if, breaks aio=native otherwise.

vl.c DriveInfo *drive_init(QemuOpts *opts, vo · line 994

Quoted 4 lines
vl.c +1 -3
-            aio = 0;
-        else if (!strcmp(buf, "native"))
-            aio = 1;
+            /* this is the default */;

Same as above, but IMHO braces are even mandatory here as every single line of the code is touched.

Quoted 14 lines
vl.c +0 -6
         else {
            fprintf(stderr, "qemu: invalid aio option\n");
            return NULL;

@@ -1169,20 +1166,10 @@ DriveInfo *drive_init(QemuOpts *opts, vo
         *fatal_error = 0;
         return NULL;
     }
-    bdrv_flags = 0;
     if (snapshot) {
-        bdrv_flags |= BDRV_O_SNAPSHOT;
-        cache = 2; /* always use write-back with snapshot */
-    }
-    if (cache == 0) /* no caching */
-        bdrv_flags |= BDRV_O_NOCACHE;

vl.c DriveInfo *drive_init(QemuOpts *opts, vo · line 1172

Quoted 10 lines
vl.c +3 -7
-    else if (cache == 2) /* write-back */
-        bdrv_flags |= BDRV_O_CACHE_WB;
-
-    if (aio == 1) {
-        bdrv_flags |= BDRV_O_NATIVE_AIO;
-    } else {
-        bdrv_flags &= ~BDRV_O_NATIVE_AIO;
+        /* always use write-back with snapshot */
+        bdrv_flags |= (BDRV_O_SNAPSHOT|BDRV_O_CACHE_WB);
+        bdrv_flags &= ~BDRV_O_NOCACHE;

I'd prefer to have a bdrv_flags &= ~BDRV_O_CACHE_MASK before the |= line instead of clearing some random flag that happens to be the only one.

Kevin