vl.c: use 'break' instead of 'continue' in configure_accelerator()

At present, each 'opt_name' of 'accel_list' is uniq with each other, so 'buf' can only match one 'opt_name'.

When drop into the matching code block, can 'break' outside related 'for' looping after finish processing it (just like the other 'break' within the matching block).

After print "... not support for this target", it can avoid to print "... accelerator does not exist".

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

---
 vl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

vl.c +0 -0

index 842e897..b4f98fa 100644

vl.c +0 -0

                 }
                 *(accel_list[i].allowed) = true;
                 ret = accel_list[i].init(machine);

At present, each 'opt_name' of 'accel_list' is uniq with each other, so 'buf' can only match one 'opt_name'.

When drop into the matching code block, can 'break' outside related 'for' looping after finish processing it (just like the other 'break' within the matching block).

After print "... not support for this target", it can avoid to print "... accelerator does not exist".

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

---
 vl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

vl.c +0 -0

index 842e897..b4f98fa 100644

Quoted 9 lines · Chen Gang

At present, each 'opt_name' of 'accel_list' is uniq with each other, so 'buf' can only match one 'opt_name'.

When drop into the matching code block, can 'break' outside related 'for' looping after finish processing it (just like the other 'break' within the matching block).

After print "... not support for this target", it can avoid to print "... accelerator does not exist".

Hi,
Thanks for the patch!

I would re-write
1. The commit subject to:
Fix wrong warning on configure_accelerator
2. The commit message to:
No need to continue to iterate over the accelerators list after a match is found, even if it is not supported.

Quoted 12 lines

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

---
 vl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

vl.c +0 -0

index 842e897..b4f98fa 100644

vl.c static int configure_accelerator(QEMUMachine *machine) · line 2712

Quoted 5 lines

vl.c +1 -1

@@ -2709,7 +2709,7 @@ static int configure_accelerator(QEMUMachine *machine)
                 if (!accel_list[i].available()) {
                     printf("%s not supported for this target\n",
                            accel_list[i].name);
-                    continue;
+                    break;

Seems like the right thing to do.

Thanks,
Marcel

Quoted 3 lines

vl.c +0 -0

                 }
                 *(accel_list[i].allowed) = true;
                 ret = accel_list[i].init(machine);

Quoted 21 lines · Chen Gang

At present, each 'opt_name' of 'accel_list' is uniq with each other, so 'buf' can only match one 'opt_name'.

When drop into the matching code block, can 'break' outside related 'for' looping after finish processing it (just like the other 'break' within the matching block).

After print "... not support for this target", it can avoid to print "... accelerator does not exist".

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

---
 vl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

vl.c +0 -0

index 842e897..b4f98fa 100644

vl.c static int configure_accelerator(QEMUMachine *machine) · line 2715

Quoted 8 lines

vl.c +1 -1

@@ -2709,7 +2709,7 @@ static int configure_accelerator(QEMUMachine *machine)
                 if (!accel_list[i].available()) {
                     printf("%s not supported for this target\n",
                            accel_list[i].name);
-                    continue;
+                    break;
                 }
                 *(accel_list[i].allowed) = true;
                 ret = accel_list[i].init(machine);

Works, because the opt_name in accel_list[] are distinct, as you explained in your commit message.

Apropos commit message. You first explain what you do, and only then state the problem you're trying to solve. That's backwards. Start with the problem. Only then explain your solution, if it needs explaining. This one, I think, doesn't.

Suggested commit message:

vl: Report accelerator not supported for target more nicely

When you ask for an accelerator not supported for your target, you get a bogus "accelerator does not exist" message:

$ qemu-system-arm -machine none,accel=kvm
KVM not supported for this target
"kvm" accelerator does not exist.
No accelerator found!

Suppress it.

Quoted 47 lines · Markus Armbruster

Quoted 21 lines · Chen Gang

At present, each 'opt_name' of 'accel_list' is uniq with each other, so 'buf' can only match one 'opt_name'.

When drop into the matching code block, can 'break' outside related 'for' looping after finish processing it (just like the other 'break' within the matching block).

After print "... not support for this target", it can avoid to print "... accelerator does not exist".

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

---
 vl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

vl.c +0 -0

index 842e897..b4f98fa 100644

vl.c static int configure_accelerator(QEMUMachine *machine) · line 2715

Quoted 8 lines

vl.c +1 -1

@@ -2709,7 +2709,7 @@ static int configure_accelerator(QEMUMachine *machine)
                 if (!accel_list[i].available()) {
                     printf("%s not supported for this target\n",
                            accel_list[i].name);
-                    continue;
+                    break;
                 }
                 *(accel_list[i].allowed) = true;
                 ret = accel_list[i].init(machine);

Works, because the opt_name in accel_list[] are distinct, as you explained in your commit message.

Apropos commit message. You first explain what you do, and only then state the problem you're trying to solve. That's backwards. Start with the problem. Only then explain your solution, if it needs explaining. This one, I think, doesn't.

Suggested commit message:

vl: Report accelerator not supported for target more nicely

When you ask for an accelerator not supported for your target, you get a bogus "accelerator does not exist" message:

Quoted 7 lines

$ qemu-system-arm -machine none,accel=kvm
KVM not supported for this target
"kvm" accelerator does not exist.
No accelerator found!

Suppress it.

Thank you for your details, that sounds reasonable to me.

Next, I will/should notice start with the issue, and then explain the solution.

Thanks.

Hello Maintainers:

If it is necessary to send patch v2 by me, please let me know, I will/should send.

Quoted 4 lines · Chen Gang

Hello Maintainers:

If it is necessary to send patch v2 by me, please let me know, I will/should send.

Not a maintainer, but if you send a v2 with an improved commit message, I'll R-by it, which can only help getting it merged.

Quoted 7 lines · Markus Armbruster

Quoted 4 lines · Chen Gang

Hello Maintainers:

If it is necessary to send patch v2 by me, please let me know, I will/should send.

Quoted 3 lines

Not a maintainer, but if you send a v2 with an improved commit message, I'll R-by it, which can only help getting it merged.

I guess your meaning is "not quite necessary" (for me, minor useful patches almost like spam). So if sending patch v2 is really required, please let me know, thanks.

Quoted 8 lines · Chen Gang

Quoted 7 lines · Markus Armbruster

Quoted 4 lines · Chen Gang

Hello Maintainers:

If it is necessary to send patch v2 by me, please let me know, I will/should send.

Quoted 7 lines

Quoted 3 lines

Not a maintainer, but if you send a v2 with an improved commit message, I'll R-by it, which can only help getting it merged.

I guess your meaning is "not quite necessary" (for me, minor useful patches almost like spam). So if sending patch v2 is really required, please let me know, thanks.

Basically, asking a maintainer to make changes to a patch as they apply it is asking them to do extra work beyond what they would normally do. Sometimes people will agree to do this, but in general it's better just to send a fixed version of the patch yourself.

(I've cc'd qemu-trivial since that's probably the best tree to take this patch.)

Next, when I send trivial patches, I will/should cc to qemu-trivial. I guess, most of my future patches will be trivial patches (and for me, trivial != minor).

Quoted 3 lines · Chen Gang

Next, when I send trivial patches, I will/should cc to qemu-trivial. I guess, most of my future patches will be trivial patches (and for me, trivial != minor).

We describe on the wiki what we mean by 'trivial':
http://wiki.qemu.org/Contribute/TrivialPatches

Next, when I send trivial patches, I will only send to qemu-trivial (not send/cc to qemu-devel again), that will be more efficient. :-)

Quoted 2 lines · Chen Gang

Next, when I send trivial patches, I will only send to qemu-trivial (not send/cc to qemu-devel again), that will be more efficient. :-)

No, please always send to qemu-devel; just also cc qemu-trivial (or the relevant subsystem maintainers as listed in MAINTAINERS) if appropriate. Not everybody subscribes to qemu-trivial.

When you ask for an accelerator not supported for your target, you get a bogus "accelerator does not exist" message:

  $ qemu-system-arm -machine none,accel=kvm
  KVM not supported for this target
  "kvm" accelerator does not exist.
  No accelerator found!

Suppress it.

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

When you ask for an accelerator not supported for your target, you get a bogus "accelerator does not exist" message:

  $ qemu-system-arm -machine none,accel=kvm
  KVM not supported for this target
  "kvm" accelerator does not exist.
  No accelerator found!

Suppress it.

Quoted 8 lines · Chen Gang

When you ask for an accelerator not supported for your target, you get a bogus "accelerator does not exist" message:

  $ qemu-system-arm -machine none,accel=kvm
  KVM not supported for this target
  "kvm" accelerator does not exist.
  No accelerator found!

Quoted 4 lines

Suppress it.

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

Reviewed-by Markus Armbruster <[email protected]>

Quoted 8 lines · Chen Gang

When you ask for an accelerator not supported for your target, you get a bogus "accelerator does not exist" message:

  $ qemu-system-arm -machine none,accel=kvm
  KVM not supported for this target
  "kvm" accelerator does not exist.
  No accelerator found!

Quoted 1 line

Suppress it.

Applied to -trivial, thanks!

/mjt

In "vl.c", at least, we can simplify the code below, so can let readers read professional C code (especially for new readers, which often start reading code at main function).

- remove useless 'continue' in main().

- remove redundant local variable 'res' in get_boot_device().

- remove local variable 'args' in the middle of code block in main().

The following 3 patches are for the 3 'remove' above.

And "vl.c" has a very long function main() which is about 17K lines. Next, it can be split into sub-functions (so can bypass another code style issue: multiple "{...}" styles within "swith(...)").

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

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

Normal "if (...) {...} else {...}" is enough in "while(...) {...}", not need additional useless 'continue'.

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

---
 vl.c |    1 -
 1 file changed, 1 deletion(-)

vl.c +0 -2

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

1.7.9.5

In function, if no additional resources to free before quit, commonly, need not use additional local variable 'res' to notice about it. So remove it to simplify code.

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

---
 vl.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

vl.c +2 -5

index 7505002..377f962 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;
 }
 
 /*
--

1.7.9.5

Quoted 3 lines · Chen Gang

Quoted 3 lines

Normal "if (...) {...} else {...}" is enough in "while(...) {...}", not need additional useless 'continue'.

Only in the case where the if-else is not followed by any code. Which is the case here. I found this sentance slightly confusing and TBH id just drop it or say something less codey like:

"This if else has no code between it and the end of the enclosing while loop. This makes this continue redundant."

Quoted 1 line

Quoted 1 line

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

But patch is good:

Reviewed-by Peter Crosthwaite <[email protected]>

Quoted 4 lines · Chen Gang

In function, if no additional resources to free before quit, commonly, need not use additional local variable 'res' to notice about it. So remove it to simplify code.

Styling wise, there is a school of thought that functions should only have one return statement which is probably the original authors intention.

Regards,
Peter

Quoted 29 lines

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

---
 vl.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

vl.c +2 -4

index 7505002..377f962 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;
 }
 

Quoted 4 lines

vl.c +0 -1

 /*
--

1.7.9.5

Quoted 4 lines · Chen Gang

In function, if no additional resources to free before quit, commonly, need not use additional local variable 'res' to notice about it. So remove it to simplify code.

Styling wise, there is a school of thought that functions should only have one return statement which is probably the original authors intention.

In "vl.c", at least, we can simplify the code below, so can let readers read professional C code (especially for new readers, which often start reading code at main function).

- remove useless 'continue' in main().

- remove redundant local variable 'res' in get_boot_device().

- remove local variable 'args' in the middle of code block in main().

The following 3 patches are for the 3 'remove' above.

And "vl.c" has a very long function main() which is about 17K lines. Next, it can be split into sub-functions (so can bypass another code style issue: multiple "{...}" styles within "swith(...)").

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

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

Normal "if (...) {...} else {...}" is enough in "while(...) {...}", not need additional useless 'continue'.

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

---
 vl.c |    1 -
 1 file changed, 1 deletion(-)

vl.c +0 -0

index 9975e5a..7505002 100644

For C code, it does not recommend to define a local variable in the middle of code block without "{...}". The original author may want to zero members not mentioned in structure assignment.

So recommend to use structure initializing block "{...}" for structure assignment in the middle of code block.

And at present, we can assume that all related gcc versions will be latest enough to notice about this grammar.

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

---
 vl.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

vl.c +2 -9

index 377f962..d381443 100644
@@ -4368,15 +4368,15 @@ 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 = (QEMUMachineInitArgs) {
+        .machine = machine,

Quoted 9 lines · Peter Crosthwaite

Quoted 4 lines · Chen Gang

In function, if no additional resources to free before quit, commonly, need not use additional local variable 'res' to notice about it. So remove it to simplify code.

Styling wise, there is a school of thought that functions should only have one return statement which is probably the original authors intention.

Plausible. But what matters here is whether we think the patch is worthwhile or not.

I find Chen's version a bit clearer, but I'm not sure it's worth the churn.

Quoted 8 lines · Chen Gang

Normal "if (...) {...} else {...}" is enough in "while(...) {...}", not need additional useless 'continue'.

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

---
 vl.c |    1 -
 1 file changed, 1 deletion(-)

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

Quoted 11 lines

vl.c +0 -1

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

Reviewed-by Markus Armbruster <[email protected]>

Quoted 9 lines · Chen Gang

For C code, it does not recommend to define a local variable in the middle of code block without "{...}". The original author may want to zero members not mentioned in structure assignment.

So recommend to use structure initializing block "{...}" for structure assignment in the middle of code block.

And at present, we can assume that all related gcc versions will be latest enough to notice about this grammar.

Commit message is a bit confusing :) I'd write something like this:

vl: Eliminate a superfluous local variable

CODING_STYLE frowns upon mixing declarations and statements. main() has such a declaration. Clean up by eliminating the variable.

Quoted 25 lines

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

---
 vl.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

vl.c +2 -9

index 377f962..d381443 100644
@@ -4368,15 +4368,15 @@ 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 = (QEMUMachineInitArgs) {
+        .machine = machine,

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

Quoted 10 lines

vl.c +7 -0

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

Reviewed-by Markus Armbruster <[email protected]>

For C code, it does not recommend to define a local variable in the middle of code block without "{...}". The original author may want to zero members not mentioned in structure assignment.

So recommend to use structure initializing block "{...}" for structure assignment in the middle of code block.

And at present, we can assume that all related gcc versions will be latest enough to notice about this grammar.

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

---
 vl.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

vl.c +9 -10

index 377f962..d381443 100644
@@ -4368,15 +4368,15 @@ 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 = (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 };
+
     machine->init(&current_machine->init_args);
 
     audio_init();
--

1.7.9.5

Quoted 11 lines · Chen Gang

For C code, it does not recommend to define a local variable in the middle of code block without "{...}". The original author may want to zero members not mentioned in structure assignment.

So recommend to use structure initializing block "{...}" for structure assignment in the middle of code block.

And at present, we can assume that all related gcc versions will be latest enough to notice about this grammar.

Quoted 1 line

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

With Markus' suggest commit message fixup:

Reviewed-by Peter Crosthwaite <[email protected]>

Quoted 36 lines

---
 vl.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

vl.c +9 -10

index 377f962..d381443 100644
@@ -4368,15 +4368,15 @@ 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 = (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 };
+
     machine->init(&current_machine->init_args);
 
     audio_init();
--

1.7.9.5

Quoted 17 lines · Chen Gang

In "vl.c", at least, we can simplify the code below, so can let readers read professional C code (especially for new readers, which often start reading code at main function).

- remove useless 'continue' in main().

- remove redundant local variable 'res' in get_boot_device().

- remove local variable 'args' in the middle of code block in main().

The following 3 patches are for the 3 'remove' above.

And "vl.c" has a very long function main() which is about 17K lines. Next, it can be split into sub-functions (so can bypass another code style issue: multiple "{...}" styles within "swith(...)").

Quoted 4 lines

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

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

In future submissions, please send the patches in-reply-to the cover letter, not chained together in-reply-to the previous part. Check out git-send-email --no-chain-reply-to.

Quoted 4 lines · Markus Armbruster

In future submissions, please send the patches in-reply-to the cover letter, not chained together in-reply-to the previous part. Check out git-send-email --no-chain-reply-to.

OK, thanks. But excuse me, I use thunderbird client to send/recv mail (not use git send-mail), so not quit understand what you said. And I guess what your meaning is:

- start a new thread "[PATH 0/3]..."

- "[PATH 1/3]..." need "reply all" the "[PATH 0/3]..."

- "[PATH 2/3]..." need "reply all" the "[PATH 0/3]..."

- "[PATH 3/3]..." need "reply all" the "[PATH 0/3]..."

If what I guess is incorrect, please let me know, and it will be better to provide more detials.

Quoted 14 lines · Chen Gang

Quoted 4 lines · Markus Armbruster

In future submissions, please send the patches in-reply-to the cover letter, not chained together in-reply-to the previous part. Check out git-send-email --no-chain-reply-to.

OK, thanks. But excuse me, I use thunderbird client to send/recv mail (not use git send-mail), so not quit understand what you said. And I guess what your meaning is:

- start a new thread "[PATH 0/3]..."

- "[PATH 1/3]..." need "reply all" the "[PATH 0/3]..."

Quoted 6 lines

- "[PATH 2/3]..." need "reply all" the "[PATH 0/3]..."

- "[PATH 3/3]..." need "reply all" the "[PATH 0/3]..."

If what I guess is incorrect, please let me know, and it will be better to provide more detials.

Sounds okay. But sending with git-send-email is so much easier...

Quoted 10 lines

Quoted 8 lines · Chen Gang

Normal "if (...) {...} else {...}" is enough in "while(...) {...}", not need additional useless 'continue'.

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

---
 vl.c |    1 -
 1 file changed, 1 deletion(-)

Quoted 14 lines

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

Quoted 11 lines

vl.c +0 -1

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

Reviewed-by Markus Armbruster <[email protected]>

OK, thanks.

--
Chen Gang

Open, share, and attitude like air, water, and life which God blessed