* [Qemu-devel] [PATCH trival] vl.c: clean up code
@ 2014-03-30 14:34 Chen Gang
2014-03-30 14:43 ` Chen Gang
2014-03-31 15:49 ` Markus Armbruster
0 siblings, 2 replies; 10+ messages in thread
From: Chen Gang @ 2014-03-30 14:34 UTC (permalink / raw)
To: aliguori, QEMU Developers
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 <gang.chen.5i5j@gmail.com>
---
vl.c | 36 +++++++++++++-----------------------
1 file changed, 13 insertions(+), 23 deletions(-)
diff --git a/vl.c b/vl.c
index 9975e5a..9c733cb 100644
--- a/vl.c
+++ b/vl.c
@@ -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(¤t_machine->init_args);
audio_init();
--
1.7.11.7
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH trival] vl.c: clean up code
2014-03-30 14:34 [Qemu-devel] [PATCH trival] vl.c: clean up code Chen Gang
@ 2014-03-30 14:43 ` Chen Gang
2014-04-01 12:36 ` Alex Bennée
2014-03-31 15:49 ` Markus Armbruster
1 sibling, 1 reply; 10+ messages in thread
From: Chen Gang @ 2014-03-30 14:43 UTC (permalink / raw)
To: aliguori, QEMU Developers
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.
On 03/30/2014 10:34 PM, Chen Gang wrote:
> 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 <gang.chen.5i5j@gmail.com>
> ---
> vl.c | 36 +++++++++++++-----------------------
> 1 file changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 9975e5a..9c733cb 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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(¤t_machine->init_args);
>
> audio_init();
>
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH trival] vl.c: clean up code
2014-03-30 14:34 [Qemu-devel] [PATCH trival] vl.c: clean up code Chen Gang
2014-03-30 14:43 ` Chen Gang
@ 2014-03-31 15:49 ` Markus Armbruster
2014-04-01 0:11 ` Chen Gang
1 sibling, 1 reply; 10+ messages in thread
From: Markus Armbruster @ 2014-03-31 15:49 UTC (permalink / raw)
To: Chen Gang; +Cc: QEMU Developers, aliguori
Chen Gang <gang.chen.5i5j@gmail.com> writes:
> 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).
Suggest to have one patch per item in this list.
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
> vl.c | 36 +++++++++++++-----------------------
> 1 file changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 9975e5a..9c733cb 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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;
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.
Dunno.
> @@ -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(¤t_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.
Matter of taste, I guess.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH trival] vl.c: clean up code
2014-03-31 15:49 ` Markus Armbruster
@ 2014-04-01 0:11 ` Chen Gang
2014-04-01 8:13 ` Markus Armbruster
0 siblings, 1 reply; 10+ messages in thread
From: Chen Gang @ 2014-04-01 0:11 UTC (permalink / raw)
To: Markus Armbruster; +Cc: QEMU Developers, aliguori
On 03/31/2014 11:49 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>
>> 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).
>
> Suggest to have one patch per item in this list.
>
OK, thanks. I will/should send again in this week (within 2014-04-06).
[...]
>> @@ -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.
>
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.
[...]
>> @@ -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(¤t_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.
>
That sounds good to me, I will/should send related patch v2.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH trival] vl.c: clean up code
2014-04-01 0:11 ` Chen Gang
@ 2014-04-01 8:13 ` Markus Armbruster
2014-04-01 9:01 ` Chen Gang
0 siblings, 1 reply; 10+ messages in thread
From: Markus Armbruster @ 2014-04-01 8:13 UTC (permalink / raw)
To: Chen Gang; +Cc: QEMU Developers, aliguori
Chen Gang <gang.chen.5i5j@gmail.com> writes:
> On 03/31/2014 11:49 PM, Markus Armbruster wrote:
>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>>
>>> 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).
>>
>> 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 :)
> [...]
>>> @@ -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.
>>
>
> 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.
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.
[...]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH trival] vl.c: clean up code
2014-04-01 8:13 ` Markus Armbruster
@ 2014-04-01 9:01 ` Chen Gang
2014-04-01 13:33 ` Markus Armbruster
0 siblings, 1 reply; 10+ messages in thread
From: Chen Gang @ 2014-04-01 9:01 UTC (permalink / raw)
To: Markus Armbruster; +Cc: QEMU Developers, aliguori
On 04/01/2014 04:13 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>
>> On 03/31/2014 11:49 PM, Markus Armbruster wrote:
>>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>>>
>>>> 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).
>>>
>>> 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 :)
>
Yeah, more discussions, and more thinking of, before implementations.
>> [...]
>>>> @@ -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.
>>>
>>
>> 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.
>
> 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.
>
I guess, the root cause is the main() is too long, and need be split
into several small functions, then use 'return' instead of exit(), and
then remove "'break' after exit()".
And after split into small functions, we also can by pass the several
"{...}" styles within switch() in main() -- let the related code block
be in individual functions.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH trival] vl.c: clean up code
2014-03-30 14:43 ` Chen Gang
@ 2014-04-01 12:36 ` Alex Bennée
2014-04-01 13:08 ` Chen Gang
0 siblings, 1 reply; 10+ messages in thread
From: Alex Bennée @ 2014-04-01 12:36 UTC (permalink / raw)
To: Chen Gang; +Cc: QEMU Developers, aliguori
Chen Gang <gang.chen.5i5j@gmail.com> writes:
> 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.
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.
>
>
> Thanks.
>
> On 03/30/2014 10:34 PM, Chen Gang wrote:
>> 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 <gang.chen.5i5j@gmail.com>
>> ---
>> vl.c | 36 +++++++++++++-----------------------
>> 1 file changed, 13 insertions(+), 23 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 9975e5a..9c733cb 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -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(¤t_machine->init_args);
>>
>> audio_init();
>>
--
Alex Bennée
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH trival] vl.c: clean up code
2014-04-01 12:36 ` Alex Bennée
@ 2014-04-01 13:08 ` Chen Gang
0 siblings, 0 replies; 10+ messages in thread
From: Chen Gang @ 2014-04-01 13:08 UTC (permalink / raw)
To: Alex Bennée; +Cc: QEMU Developers, aliguori
On 04/01/2014 08:36 PM, Alex Bennée wrote:
>
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>
>> 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.
>
> 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 CODING_STYLE Section 4, has no demo for "{...}" within switch, I
guess your meaning is "the demo below is the correct block style for Qemu":
switch(...) {
case 'x': {
char a;
...
break;
}
case 'y':
...
break;
default:
break;
}
If it is OK, suggest to complete the CODING_STYLE Section 4 with one
'switch' demo.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH trival] vl.c: clean up code
2014-04-01 9:01 ` Chen Gang
@ 2014-04-01 13:33 ` Markus Armbruster
2014-04-01 13:42 ` Chen Gang
0 siblings, 1 reply; 10+ messages in thread
From: Markus Armbruster @ 2014-04-01 13:33 UTC (permalink / raw)
To: Chen Gang; +Cc: QEMU Developers, aliguori
Chen Gang <gang.chen.5i5j@gmail.com> writes:
> On 04/01/2014 04:13 PM, Markus Armbruster wrote:
>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>>
>>> On 03/31/2014 11:49 PM, Markus Armbruster wrote:
>>>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>>>>
>>>>> 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).
>>>>
>>>> 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 :)
>>
>
> 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".
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH trival] vl.c: clean up code
2014-04-01 13:33 ` Markus Armbruster
@ 2014-04-01 13:42 ` Chen Gang
0 siblings, 0 replies; 10+ messages in thread
From: Chen Gang @ 2014-04-01 13:42 UTC (permalink / raw)
To: Markus Armbruster; +Cc: QEMU Developers, aliguori
On 04/01/2014 09:33 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>
>> On 04/01/2014 04:13 PM, Markus Armbruster wrote:
>>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>>>
>>>> On 03/31/2014 11:49 PM, Markus Armbruster wrote:
>>>>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>>>>>
>>>>>> 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).
>>>>>
>>>>> 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 :)
>>>
>>
>> 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".
>
OK, thanks. :-)
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-04-01 13:42 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-30 14:34 [Qemu-devel] [PATCH trival] vl.c: clean up code Chen Gang
2014-03-30 14:43 ` Chen Gang
2014-04-01 12:36 ` Alex Bennée
2014-04-01 13:08 ` Chen Gang
2014-03-31 15:49 ` Markus Armbruster
2014-04-01 0:11 ` Chen Gang
2014-04-01 8:13 ` Markus Armbruster
2014-04-01 9:01 ` Chen Gang
2014-04-01 13:33 ` Markus Armbruster
2014-04-01 13:42 ` Chen Gang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.