vl.c: clean up code

in get_boot_device()

- remove 'res' to simplify code

in main():

- remove useless 'continue'.

- in main switch():

- remove or adjust all useless 'break'.

- remove useless '{' and '}'.

- use assignment directly to replace useless 'args' (which is defined in the middle of code block).

Signed-off-by Chen Gang <[email protected]>

---
 vl.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

vl.c +13 -23

index 9975e5a..9c733cb 100644
@@ -1188,18 +1188,16 @@ DeviceState *get_boot_device(uint32_t position)
 {
     uint32_t counter = 0;
     FWBootEntry *i = NULL;
-    DeviceState *res = NULL;
 
     if (!QTAILQ_EMPTY(&fw_boot_order)) {
         QTAILQ_FOREACH(i, &fw_boot_order, link) {
             if (counter == position) {
-                res = i->dev;
-                break;
+                return i->dev;
             }
             counter++;
         }
     }
-    return res;
+    return NULL;
 }
 
 /*

@@ -3034,7 +3032,6 @@ int main(int argc, char **argv, char **envp)
         if (argv[optind][0] != '-') {
             /* disk image */
             optind++;
-            continue;
         } else {
             const QEMUOption *popt;
 

@@ -3204,11 +3201,11 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_curses:
 #ifdef CONFIG_CURSES
                 display_type = DT_CURSES;
+                break;
 #else
                 fprintf(stderr, "Curses support is disabled\n");
                 exit(1);
 #endif
-                break;
             case QEMU_OPTION_portrait:
                 graphic_rotate = 90;
                 break;

@@ -3286,7 +3283,6 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_audio_help:
                 AUD_help ();
                 exit (0);
-                break;
             case QEMU_OPTION_soundhw:
                 select_soundhw (optarg);
                 break;

@@ -3296,7 +3292,6 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_version:
                 version();
                 exit(0);
-                break;
             case QEMU_OPTION_m: {
                 int64_t value;
                 uint64_t sz;

@@ -3638,11 +3633,10 @@ int main(int argc, char **argv, char **envp)
                 olist = qemu_find_opts("machine");
                 qemu_opts_parse(olist, "accel=tcg", 0);
                 break;
-            case QEMU_OPTION_no_kvm_pit: {
+            case QEMU_OPTION_no_kvm_pit:
                 fprintf(stderr, "Warning: KVM PIT can no longer be disabled "
                                 "separately.\n");
                 break;
-            }
             case QEMU_OPTION_no_kvm_pit_reinjection: {
                 static GlobalProperty kvm_pit_lost_tick_policy[] = {
                     {

@@ -3681,11 +3675,11 @@ int main(int argc, char **argv, char **envp)
 #ifdef CONFIG_VNC
                 display_remote++;
                 vnc_display = optarg;
+                break;
 #else
                 fprintf(stderr, "VNC support is disabled\n");
                 exit(1);
 #endif
-                break;
             case QEMU_OPTION_no_acpi:
                 acpi_enabled = 0;
                 break;

@@ -3811,7 +3805,6 @@ int main(int argc, char **argv, char **envp)
                 xen_mode = XEN_ATTACH;
                 break;
             case QEMU_OPTION_trace:
-            {
                 opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 0);
                 if (!opts) {
                     exit(1);

@@ -3819,7 +3812,6 @@ int main(int argc, char **argv, char **envp)
                 trace_events = qemu_opt_get(opts, "events");
                 trace_file = qemu_opt_get(opts, "file");
                 break;
-            }
             case QEMU_OPTION_readconfig:
                 {
                     int ret = qemu_read_config_file(optarg);

@@ -3876,12 +3868,12 @@ int main(int argc, char **argv, char **envp)
                 if (!opts) {
                     exit(1);
                 }
+                break;
 #else
                 error_report("File descriptor passing is disabled on this "
                              "platform");
                 exit(1);
 #endif
-                break;
             case QEMU_OPTION_object:
                 opts = qemu_opts_parse(qemu_find_opts("object"), optarg, 1);
                 if (!opts) {

@@ -4371,15 +4363,13 @@ int main(int argc, char **argv, char **envp)
 
     qdev_machine_init();
 
-    QEMUMachineInitArgs args = { .machine = machine,
-                                 .ram_size = ram_size,
-                                 .boot_order = boot_order,
-                                 .kernel_filename = kernel_filename,
-                                 .kernel_cmdline = kernel_cmdline,
-                                 .initrd_filename = initrd_filename,
-                                 .cpu_model = cpu_model };
-
-    current_machine->init_args = args;
+    current_machine->init_args.machine = machine;
+    current_machine->init_args.ram_size = ram_size;
+    current_machine->init_args.boot_order = boot_order;
+    current_machine->init_args.kernel_filename = kernel_filename;
+    current_machine->init_args.kernel_cmdline = kernel_cmdline;
+    current_machine->init_args.initrd_filename = initrd_filename;
+    current_machine->init_args.cpu_model = cpu_model;
     machine->init(&current_machine->init_args);
 
     audio_init();

Hello Maintainers:

In main switch of main(), it contents several styles for "{...}" code block.

If it is necessary to use unique style within a function, please let me know, I will/should clean up it. And also better to tell me which style we need choose -- for me, I don't know which style is the best to Qemu.

Thanks.

Quoted 165 lines · Chen Gang

in get_boot_device()

- remove 'res' to simplify code

in main():

- remove useless 'continue'.

- in main switch():

- remove or adjust all useless 'break'.

- remove useless '{' and '}'.

- use assignment directly to replace useless 'args' (which is defined in the middle of code block).

Signed-off-by Chen Gang <[email protected]>

---
 vl.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

vl.c +13 -23

index 9975e5a..9c733cb 100644
@@ -1188,18 +1188,16 @@ DeviceState *get_boot_device(uint32_t position)
 {
     uint32_t counter = 0;
     FWBootEntry *i = NULL;
-    DeviceState *res = NULL;
 
     if (!QTAILQ_EMPTY(&fw_boot_order)) {
         QTAILQ_FOREACH(i, &fw_boot_order, link) {
             if (counter == position) {
-                res = i->dev;
-                break;
+                return i->dev;
             }
             counter++;
         }
     }
-    return res;
+    return NULL;
 }
 
 /*

@@ -3034,7 +3032,6 @@ int main(int argc, char **argv, char **envp)
         if (argv[optind][0] != '-') {
             /* disk image */
             optind++;
-            continue;
         } else {
             const QEMUOption *popt;
 

@@ -3204,11 +3201,11 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_curses:
 #ifdef CONFIG_CURSES
                 display_type = DT_CURSES;
+                break;
 #else
                 fprintf(stderr, "Curses support is disabled\n");
                 exit(1);
 #endif
-                break;
             case QEMU_OPTION_portrait:
                 graphic_rotate = 90;
                 break;

@@ -3286,7 +3283,6 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_audio_help:
                 AUD_help ();
                 exit (0);
-                break;
             case QEMU_OPTION_soundhw:
                 select_soundhw (optarg);
                 break;

@@ -3296,7 +3292,6 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_version:
                 version();
                 exit(0);
-                break;
             case QEMU_OPTION_m: {
                 int64_t value;
                 uint64_t sz;

@@ -3638,11 +3633,10 @@ int main(int argc, char **argv, char **envp)
                 olist = qemu_find_opts("machine");
                 qemu_opts_parse(olist, "accel=tcg", 0);
                 break;
-            case QEMU_OPTION_no_kvm_pit: {
+            case QEMU_OPTION_no_kvm_pit:
                 fprintf(stderr, "Warning: KVM PIT can no longer be disabled "
                                 "separately.\n");
                 break;
-            }
             case QEMU_OPTION_no_kvm_pit_reinjection: {
                 static GlobalProperty kvm_pit_lost_tick_policy[] = {
                     {

@@ -3681,11 +3675,11 @@ int main(int argc, char **argv, char **envp)
 #ifdef CONFIG_VNC
                 display_remote++;
                 vnc_display = optarg;
+                break;
 #else
                 fprintf(stderr, "VNC support is disabled\n");
                 exit(1);
 #endif
-                break;
             case QEMU_OPTION_no_acpi:
                 acpi_enabled = 0;
                 break;

@@ -3811,7 +3805,6 @@ int main(int argc, char **argv, char **envp)
                 xen_mode = XEN_ATTACH;
                 break;
             case QEMU_OPTION_trace:
-            {
                 opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 0);
                 if (!opts) {
                     exit(1);

@@ -3819,7 +3812,6 @@ int main(int argc, char **argv, char **envp)
                 trace_events = qemu_opt_get(opts, "events");
                 trace_file = qemu_opt_get(opts, "file");
                 break;
-            }
             case QEMU_OPTION_readconfig:
                 {
                     int ret = qemu_read_config_file(optarg);

@@ -3876,12 +3868,12 @@ int main(int argc, char **argv, char **envp)
                 if (!opts) {
                     exit(1);
                 }
+                break;
 #else
                 error_report("File descriptor passing is disabled on this "
                              "platform");
                 exit(1);
 #endif
-                break;
             case QEMU_OPTION_object:
                 opts = qemu_opts_parse(qemu_find_opts("object"), optarg, 1);
                 if (!opts) {

@@ -4371,15 +4363,13 @@ int main(int argc, char **argv, char **envp)
 
     qdev_machine_init();
 
-    QEMUMachineInitArgs args = { .machine = machine,
-                                 .ram_size = ram_size,
-                                 .boot_order = boot_order,
-                                 .kernel_filename = kernel_filename,
-                                 .kernel_cmdline = kernel_cmdline,
-                                 .initrd_filename = initrd_filename,
-                                 .cpu_model = cpu_model };
-
-    current_machine->init_args = args;
+    current_machine->init_args.machine = machine;
+    current_machine->init_args.ram_size = ram_size;
+    current_machine->init_args.boot_order = boot_order;
+    current_machine->init_args.kernel_filename = kernel_filename;
+    current_machine->init_args.kernel_cmdline = kernel_cmdline;
+    current_machine->init_args.initrd_filename = initrd_filename;
+    current_machine->init_args.cpu_model = cpu_model;
     machine->init(&current_machine->init_args);
 
     audio_init();

Quoted 4 lines · Chen Gang

Hello Maintainers:

In main switch of main(), it contents several styles for "{...}" code block.

Quoted 4 lines

If it is necessary to use unique style within a function, please let me know, I will/should clean up it. And also better to tell me which style we need choose -- for me, I don't know which style is the best to Qemu.

The correct block style is described in CODING_STYLE Section 4. However chunks of the code base violate the style guidelines and should be cleaned up as other fixes are made.

in get_boot_device()

- remove 'res' to simplify code

in main():

- remove useless 'continue'.

- in main switch():

- remove or adjust all useless 'break'.

- remove useless '{' and '}'.

- use assignment directly to replace useless 'args' (which is defined in the middle of code block).

Signed-off-by Chen Gang <[email protected]>

---
 vl.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

vl.c +2 -5

index 9975e5a..9c733cb 100644
@@ -1188,18 +1188,16 @@ DeviceState *get_boot_device(uint32_t position)
 {
     uint32_t counter = 0;
     FWBootEntry *i = NULL;
-    DeviceState *res = NULL;
 
     if (!QTAILQ_EMPTY(&fw_boot_order)) {
         QTAILQ_FOREACH(i, &fw_boot_order, link) {
             if (counter == position) {
-                res = i->dev;
-                break;
+                return i->dev;
             }
             counter++;
         }
     }
-    return res;
+    return NULL;
 }
 
 /*

@@ -3034,7 +3032,6 @@ int main(int argc, char **argv, char **envp)
         if (argv[optind][0] != '-') {
             /* disk image */
             optind++;
-            continue;
         } else {
             const QEMUOption *popt;
 

@@ -3204,11 +3201,11 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_curses:
 #ifdef CONFIG_CURSES

vl.c +3 -17

@@ -3286,7 +3283,6 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_audio_help:
                 AUD_help ();
                 exit (0);
-                break;
             case QEMU_OPTION_soundhw:
                 select_soundhw (optarg);
                 break;

@@ -3296,7 +3292,6 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_version:
                 version();
                 exit(0);
-                break;
             case QEMU_OPTION_m: {
                 int64_t value;
                 uint64_t sz;

@@ -3638,11 +3633,10 @@ int main(int argc, char **argv, char **envp)
                 olist = qemu_find_opts("machine");
                 qemu_opts_parse(olist, "accel=tcg", 0);
                 break;
-            case QEMU_OPTION_no_kvm_pit: {
+            case QEMU_OPTION_no_kvm_pit:
                 fprintf(stderr, "Warning: KVM PIT can no longer be disabled "
                                 "separately.\n");
                 break;
-            }
             case QEMU_OPTION_no_kvm_pit_reinjection: {
                 static GlobalProperty kvm_pit_lost_tick_policy[] = {
                     {

@@ -3681,11 +3675,11 @@ int main(int argc, char **argv, char **envp)
 #ifdef CONFIG_VNC
                 display_remote++;
                 vnc_display = optarg;
+                break;
 #else
                 fprintf(stderr, "VNC support is disabled\n");
                 exit(1);
 #endif
-                break;
             case QEMU_OPTION_no_acpi:
                 acpi_enabled = 0;
                 break;

@@ -3811,7 +3805,6 @@ int main(int argc, char **argv, char **envp)
                 xen_mode = XEN_ATTACH;
                 break;
             case QEMU_OPTION_trace:
-            {
                 opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 0);
                 if (!opts) {
                     exit(1);

@@ -3819,7 +3812,6 @@ int main(int argc, char **argv, char **envp)
                 trace_events = qemu_opt_get(opts, "events");
                 trace_file = qemu_opt_get(opts, "file");
                 break;
-            }
             case QEMU_OPTION_readconfig:
                 {
                     int ret = qemu_read_config_file(optarg);

@@ -3876,12 +3868,12 @@ int main(int argc, char **argv, char **envp)
                 if (!opts) {
                     exit(1);
                 }
+                break;
 #else
                 error_report("File descriptor passing is disabled on this "
                              "platform");
                 exit(1);
 #endif
-                break;
             case QEMU_OPTION_object:
                 opts = qemu_opts_parse(qemu_find_opts("object"), optarg, 1);
                 if (!opts) {

@@ -4371,15 +4363,13 @@ int main(int argc, char **argv, char **envp)
 
     qdev_machine_init();
 
-    QEMUMachineInitArgs args = { .machine = machine,
-                                 .ram_size = ram_size,
-                                 .boot_order = boot_order,
-                                 .kernel_filename = kernel_filename,
-                                 .kernel_cmdline = kernel_cmdline,
-                                 .initrd_filename = initrd_filename,
-                                 .cpu_model = cpu_model };
-
-    current_machine->init_args = args;

Quoted 10 lines · Chen Gang

in get_boot_device()

- remove 'res' to simplify code

in main():

- remove useless 'continue'.

- in main switch():

Quoted 6 lines

- remove or adjust all useless 'break'.

- remove useless '{' and '}'.

- use assignment directly to replace useless 'args' (which is defined in the middle of code block).

Suggest to have one patch per item in this list.

vl.c int main(int argc, char **argv, char **envp) · line 3211

Quoted 13 lines

vl.c +1 -1

@@ -3204,11 +3201,11 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_curses:
 #ifdef CONFIG_CURSES
                 display_type = DT_CURSES;
+                break;
 #else
                 fprintf(stderr, "Curses support is disabled\n");
                 exit(1);
 #endif
-                break;
             case QEMU_OPTION_portrait:
                 graphic_rotate = 90;
                 break;

I'm not sure eliding the break after exit is worthwhile.

In theory, it could cause false positives with tools smart enough to diagnose fall through without comment, but too stupid to see that exit() never returns. No idea whether such tools exist.

The missing break might give human readers slight pause, until they recognize exit. Especially with such ifdeffery.

Quoted 13 lines

vl.c +0 -9

@@ -4371,15 +4363,13 @@ int main(int argc, char **argv, char **envp)
 
     qdev_machine_init();
 
-    QEMUMachineInitArgs args = { .machine = machine,
-                                 .ram_size = ram_size,
-                                 .boot_order = boot_order,
-                                 .kernel_filename = kernel_filename,
-                                 .kernel_cmdline = kernel_cmdline,
-                                 .initrd_filename = initrd_filename,
-                                 .cpu_model = cpu_model };
-
-    current_machine->init_args = args;

vl.c int main(int argc, char **argv, char **envp) · line 4375

Quoted 10 lines

vl.c +7 -0

+    current_machine->init_args.machine = machine;
+    current_machine->init_args.ram_size = ram_size;
+    current_machine->init_args.boot_order = boot_order;
+    current_machine->init_args.kernel_filename = kernel_filename;
+    current_machine->init_args.kernel_cmdline = kernel_cmdline;
+    current_machine->init_args.initrd_filename = initrd_filename;
+    current_machine->init_args.cpu_model = cpu_model;
     machine->init(&current_machine->init_args);
 
     audio_init();

I agree with dropping useless variable args. However, you don't have to replace the assignment of a compound literal by multiple simple assignments for that. You could write

current_machine->init_args = (QEMUMachineInitArgs){
.machine = machine,
.ram_size = ram_size,
.boot_order = boot_order, .kernel_filename = kernel_filename, .kernel_cmdline = kernel_cmdline, .initrd_filename = initrd_filename, .cpu_model = cpu_model };

Not exactly the same; it this implicitly zeroes members not mentioned. No such members exist now.

Quoted 16 lines · Markus Armbruster

Quoted 14 lines · Chen Gang

in get_boot_device()

- remove 'res' to simplify code

in main():

- remove useless 'continue'.

- in main switch():

- remove or adjust all useless 'break'.

- remove useless '{' and '}'.

Quoted 5 lines

Quoted 2 lines

- use assignment directly to replace useless 'args' (which is defined in the middle of code block).

Suggest to have one patch per item in this list.

OK, thanks. I will/should send again in this week (within 2014-04-06).

[...]

Quoted 16 lines

vl.c int main(int argc, char **argv, char **envp) · line 3211

Quoted 13 lines

vl.c +1 -1

@@ -3204,11 +3201,11 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_curses:
 #ifdef CONFIG_CURSES
                 display_type = DT_CURSES;
+                break;
 #else
                 fprintf(stderr, "Curses support is disabled\n");
                 exit(1);
 #endif
-                break;
             case QEMU_OPTION_portrait:
                 graphic_rotate = 90;
                 break;

I'm not sure eliding the break after exit is worthwhile.

Quoted 7 lines

In theory, it could cause false positives with tools smart enough to diagnose fall through without comment, but too stupid to see that exit() never returns. No idea whether such tools exist.

The missing break might give human readers slight pause, until they recognize exit. Especially with such ifdeffery.

That sounds reasonable to me.

"removing 'break' after exit()" will not be good for C code readers:
exit() is not like 'return' which can get enough focus by C code readers (especially in 'vim' or other latest editor).

How about to let 'return' instead of exit() in main(), and also remove useless 'break'? Welcome any additional suggestions, discussions or completions, thanks.

Quoted 20 lines · Chen Gang

Quoted 19 lines · Markus Armbruster

Quoted 16 lines · Chen Gang

in get_boot_device()

- remove 'res' to simplify code

in main():

- remove useless 'continue'.

- in main switch():

- remove or adjust all useless 'break'.

- remove useless '{' and '}'.

- use assignment directly to replace useless 'args' (which is defined in the middle of code block).

Quoted 4 lines

Quoted 2 lines

Suggest to have one patch per item in this list.

OK, thanks. I will/should send again in this week (within 2014-04-06).

No rush :)

Quoted 27 lines

[...]

Quoted 16 lines

vl.c int main(int argc, char **argv, char **envp) · line 3211

Quoted 13 lines

vl.c +1 -1

@@ -3204,11 +3201,11 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_curses:
 #ifdef CONFIG_CURSES
                 display_type = DT_CURSES;
+                break;
 #else
                 fprintf(stderr, "Curses support is disabled\n");
                 exit(1);
 #endif
-                break;
             case QEMU_OPTION_portrait:
                 graphic_rotate = 90;
                 break;

I'm not sure eliding the break after exit is worthwhile.

Quoted 7 lines

In theory, it could cause false positives with tools smart enough to diagnose fall through without comment, but too stupid to see that exit() never returns. No idea whether such tools exist.

The missing break might give human readers slight pause, until they recognize exit. Especially with such ifdeffery.

That sounds reasonable to me.

Quoted 7 lines

"removing 'break' after exit()" will not be good for C code readers:
exit() is not like 'return' which can get enough focus by C code readers (especially in 'vim' or other latest editor).

How about to let 'return' instead of exit() in main(), and also remove useless 'break'? Welcome any additional suggestions, discussions or completions, thanks.

When main() is short, return instead of exit() is just fine with me. But when it's as long as this one, I prefer exit() to make the process termination locally obvious.

I think the code is okay as it is.

Quoted 25 lines · Markus Armbruster

Quoted 23 lines · Chen Gang

Quoted 21 lines · Markus Armbruster

Quoted 8 lines · Chen Gang

in get_boot_device()

- remove 'res' to simplify code

in main():

- remove useless 'continue'.

Quoted 8 lines

- in main switch():

- remove or adjust all useless 'break'.

- remove useless '{' and '}'.

- use assignment directly to replace useless 'args' (which is defined in the middle of code block).

Suggest to have one patch per item in this list.

Quoted 4 lines

Quoted 1 line

OK, thanks. I will/should send again in this week (within 2014-04-06).

No rush :)

Yeah, more discussions, and more thinking of, before implementations.

Quoted 28 lines · Chen Gang

Quoted 27 lines · Markus Armbruster

Quoted 24 lines · Chen Gang

Quoted 12 lines · Markus Armbruster

Quoted 10 lines · Chen Gang

in get_boot_device()

- remove 'res' to simplify code

in main():

- remove useless 'continue'.

- in main switch():

Quoted 9 lines

Quoted 6 lines

- remove or adjust all useless 'break'.

- remove useless '{' and '}'.

- use assignment directly to replace useless 'args' (which is defined in the middle of code block).

Suggest to have one patch per item in this list.

OK, thanks. I will/should send again in this week (within 2014-04-06).

Quoted 4 lines

Quoted 2 lines

No rush :)

Yeah, more discussions, and more thinking of, before implementations.

What I meant to say is "next week is fine, we're not in a big hurry to get this done".