[Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[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 <gang.chen.5i5j@gmail.com>
---
 vl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 842e897..b4f98fa 100644
--- a/vl.c
+++ b/vl.c
@@ -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);
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Marcel Apfelbaum]

On Thu, 2014-03-27 at 09:16 +0800, Chen Gang wrote:
> 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.

> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  vl.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index 842e897..b4f98fa 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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

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

Re: [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Markus Armbruster]

Chen Gang <gang.chen.5i5j@gmail.com> writes:

> 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 <gang.chen.5i5j@gmail.com>
> ---
>  vl.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/vl.c b/vl.c
> index 842e897..b4f98fa 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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.

Re: [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Chen Gang]

On 03/27/2014 03:55 PM, Marcel Apfelbaum wrote:
> On Thu, 2014-03-27 at 09:16 +0800, Chen Gang wrote:
>> 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.
> 

OK, thank you for your work (re-write it before commit it).

Next I will/should notice about it.  :-)

>>
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  vl.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 842e897..b4f98fa 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -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
> 
>>                  }
>>                  *(accel_list[i].allowed) = true;
>>                  ret = accel_list[i].init(machine);
> 
> 
> 

-- 
Chen Gang

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

Re: [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Chen Gang]

On 03/27/2014 04:59 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
> 
>> 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 <gang.chen.5i5j@gmail.com>
>> ---
>>  vl.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 842e897..b4f98fa 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -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.
> 

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.
-- 
Chen Gang

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

Re: [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Chen Gang]

Hello Maintainers:

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


Thanks.

On 03/27/2014 06:01 PM, Chen Gang wrote:
> 
> 
> On 03/27/2014 04:59 PM, Markus Armbruster wrote:
>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>>
>>> 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 <gang.chen.5i5j@gmail.com>
>>> ---
>>>  vl.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index 842e897..b4f98fa 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -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.
>>
> 
> 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.
> 


-- 
Chen Gang

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

Re: [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Markus Armbruster]

Chen Gang <gang.chen.5i5j@gmail.com> writes:

> 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.

Re: [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Chen Gang]

On 03/31/2014 08:38 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
> 
>> 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.
> 

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.

At present, I am just trying to know about Qemu by reading code (start
from 'vl.c' and continue), if I can find related issue, I will try to
fix it and send the related patch for it.


Welcome any additional suggestions and completions for what I am doing.

Thanks.
-- 
Chen Gang

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Peter Maydell]

On 31 March 2014 13:53, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
> On 03/31/2014 08:38 PM, Markus Armbruster wrote:
>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>>
>>> 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.
>>
>
> 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.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Peter Maydell]

On 31 March 2014 13:53, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
> On 03/31/2014 08:38 PM, Markus Armbruster wrote:
>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>>
>>> 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.
>>
>
> 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.)

thanks
-- PMM

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Chen Gang]

On 03/31/2014 09:01 PM, Peter Maydell wrote:
> On 31 March 2014 13:53, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
>> On 03/31/2014 08:38 PM, Markus Armbruster wrote:
>>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>>>
>>>> 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.
>>>
>>
>> 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.)
> 

OK, thanks. And excuse me, my English is not quite well, I guess, I
misunderstood the original replier's meaning. I will/should send patch
v2 for it within this week (2014-04-06).

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).


Thanks.
-- 
Chen Gang

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

Re: [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Chen Gang]

On 03/31/2014 09:01 PM, Peter Maydell wrote:
> On 31 March 2014 13:53, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
>> On 03/31/2014 08:38 PM, Markus Armbruster wrote:
>>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>>>
>>>> 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.
>>>
>>
>> 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.)
> 

OK, thanks. And excuse me, my English is not quite well, I guess, I
misunderstood the original replier's meaning. I will/should send patch
v2 for it within this week (2014-04-06).

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).


Thanks.
-- 
Chen Gang

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Peter Maydell]

On 31 March 2014 14:12, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
> 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

thanks
-- PMM

Re: [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Peter Maydell]

On 31 March 2014 14:12, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
> 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

thanks
-- PMM

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Chen Gang]

On 03/31/2014 09:16 PM, Peter Maydell wrote:
> On 31 March 2014 14:12, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
>> 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
> 

Thank you for your information.

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.  :-)


Thanks.
-- 
Chen Gang

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

Re: [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Chen Gang]

On 03/31/2014 09:16 PM, Peter Maydell wrote:
> On 31 March 2014 14:12, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
>> 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
> 

Thank you for your information.

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.  :-)


Thanks.
-- 
Chen Gang

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Peter Maydell]

On 31 March 2014 14:26, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
> 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.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Peter Maydell]

On 31 March 2014 14:26, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
> 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.

thanks
-- PMM

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Chen Gang]

On 03/31/2014 09:33 PM, Peter Maydell wrote:
> On 31 March 2014 14:26, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
>> 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.
> 

OK, thanks.

And when send trivial patch, I will/should always mark it as "[PATCH
trivial]" to let another members know about it easily, and always cc to
qemu-trivial.


Thanks.
-- 
Chen Gang

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

Re: [Qemu-devel] [PATCH] vl.c: use 'break' instead of 'continue' in configure_accelerator()

[Chen Gang]

On 03/31/2014 09:33 PM, Peter Maydell wrote:
> On 31 March 2014 14:26, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
>> 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.
> 

OK, thanks.

And when send trivial patch, I will/should always mark it as "[PATCH
trivial]" to let another members know about it easily, and always cc to
qemu-trivial.


Thanks.
-- 
Chen Gang

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

[Qemu-trivial] [PATCH-trivial v2] vl: Report accelerator not supported for target more nicely

[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!

Suppress it.


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 vl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 842e897..b4f98fa 100644
--- a/vl.c
+++ b/vl.c
@@ -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);
-- 
1.7.9.5

[Qemu-devel] [PATCH-trivial v2] vl: Report accelerator not supported for target more nicely

[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!

Suppress it.


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 vl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 842e897..b4f98fa 100644
--- a/vl.c
+++ b/vl.c
@@ -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);
-- 
1.7.9.5

Re: [Qemu-trivial] [Qemu-devel] [PATCH-trivial v2] vl: Report accelerator not supported for target more nicely

[Markus Armbruster]

Chen Gang <gang.chen.5i5j@gmail.com> writes:

> 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 <gang.chen.5i5j@gmail.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH-trivial v2] vl: Report accelerator not supported for target more nicely

[Markus Armbruster]

Chen Gang <gang.chen.5i5j@gmail.com> writes:

> 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 <gang.chen.5i5j@gmail.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-trivial] [PATCH-trivial v2] vl: Report accelerator not supported for target more nicely

[Michael Tokarev]

04.04.2014 13:39, Chen Gang wrote:
> 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.

Applied to -trivial, thanks!

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH-trivial v2] vl: Report accelerator not supported for target more nicely

[Michael Tokarev]

04.04.2014 13:39, Chen Gang wrote:
> 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.

Applied to -trivial, thanks!

/mjt

Re: [Qemu-trivial] [Qemu-devel] [PATCH-trivial v2] vl: Report accelerator not supported for target more nicely

[Chen Gang]

On 04/04/2014 06:57 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
> 
>> 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 <gang.chen.5i5j@gmail.com>
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Thanks.

-- 
Chen Gang

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

Re: [Qemu-devel] [PATCH-trivial v2] vl: Report accelerator not supported for target more nicely

[Chen Gang]

On 04/04/2014 06:57 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
> 
>> 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 <gang.chen.5i5j@gmail.com>
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Thanks.

-- 
Chen Gang

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

Re: [Qemu-trivial] [PATCH-trivial v2] vl: Report accelerator not supported for target more nicely

[Chen Gang]

On 04/06/2014 02:32 PM, Michael Tokarev wrote:
> 04.04.2014 13:39, Chen Gang wrote:
>> 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.
> 
> Applied to -trivial, thanks!
> 
> /mjt
> 

Thanks !

-- 
Chen Gang

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

Re: [Qemu-devel] [Qemu-trivial] [PATCH-trivial v2] vl: Report accelerator not supported for target more nicely

[Chen Gang]

On 04/06/2014 02:32 PM, Michael Tokarev wrote:
> 04.04.2014 13:39, Chen Gang wrote:
>> 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.
> 
> Applied to -trivial, thanks!
> 
> /mjt
> 

Thanks !

-- 
Chen Gang

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

[Qemu-trivial] [PATCH trivial 0/3] vl: simplify code for main() and get_boot_device()

[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(...)").


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 vl.c |   23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

[Qemu-devel] [PATCH trivial 0/3] vl: simplify code for main() and get_boot_device()

[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(...)").


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 vl.c |   23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

[Qemu-trivial] [PATCH trivial 1/3] vl: remove useless 'continue'

[Chen Gang]

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

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 vl.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/vl.c b/vl.c
index 9975e5a..7505002 100644
--- a/vl.c
+++ b/vl.c
@@ -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

[Qemu-devel] [PATCH trivial 1/3] vl: remove useless 'continue'

[Chen Gang]

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

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 vl.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/vl.c b/vl.c
index 9975e5a..7505002 100644
--- a/vl.c
+++ b/vl.c
@@ -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

[Qemu-trivial] [PATCH trivial 2/3] vl: remove redundant local variable 'res'

[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.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 vl.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index 7505002..377f962 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;
 }
 
 /*
-- 
1.7.9.5

[Qemu-devel] [PATCH trivial 2/3] vl: remove redundant local variable 'res'

[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.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 vl.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index 7505002..377f962 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;
 }
 
 /*
-- 
1.7.9.5

[Qemu-trivial] [PATCH trivial 3/3] vl: remove local variable 'args' in the middle of code block

[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.


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 vl.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/vl.c b/vl.c
index 377f962..d381443 100644
--- a/vl.c
+++ b/vl.c
@@ -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

[Qemu-devel] [PATCH trivial 3/3] vl: remove local variable 'args' in the middle of code block

[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.


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 vl.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/vl.c b/vl.c
index 377f962..d381443 100644
--- a/vl.c
+++ b/vl.c
@@ -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

Re: [Qemu-trivial] [PATCH trivial 0/3] vl: simplify code for main() and get_boot_device()

[Chen Gang]

May any member help to check them?

And next, I shall try to find bug issues (not code style or document
issues), and fix them. Hope I can succeed.

Thanks.

On 04/08/2014 08:00 PM, Chen Gang wrote:
> 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 <gang.chen.5i5j@gmail.com>
> ---
>  vl.c |   23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 

-- 
Chen Gang

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 1/3] vl: remove useless 'continue'

[Peter Crosthwaite]

On Tue, Apr 8, 2014 at 10:01 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
> 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."

> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>

But patch is good:

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  vl.c |    1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/vl.c b/vl.c
> index 9975e5a..7505002 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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
>

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 2/3] vl: remove redundant local variable 'res'

[Peter Crosthwaite]

On Tue, Apr 8, 2014 at 10:02 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
> 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

> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  vl.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 7505002..377f962 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;
>  }
>
>  /*
> --
> 1.7.9.5
>

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 1/3] vl: remove useless 'continue'

[Chen Gang]

On 04/15/2014 10:11 AM, Peter Crosthwaite wrote:
> On Tue, Apr 8, 2014 at 10:01 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
>> > 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."
> 
>> > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> But patch is good:
> 
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 

OK, thanks.

Thanks.
-- 
Chen Gang

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 2/3] vl: remove redundant local variable 'res'

[Chen Gang]

On 04/15/2014 10:13 AM, Peter Crosthwaite wrote:
> On Tue, Apr 8, 2014 at 10:02 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
>> 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.
> 

Hmm... maybe.

For me, the readers' feeling (let code simple and easy understanding) is
more important than "school rules".

Welcome other members' styling tastes.

> Regards,
> Peter
> 
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  vl.c |    6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 7505002..377f962 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;
>>  }
>>
>>  /*
>> --
>> 1.7.9.5
>>

Thanks.
-- 
Chen Gang

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 2/3] vl: remove redundant local variable 'res'

[Markus Armbruster]

Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> On Tue, Apr 8, 2014 at 10:02 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
>> 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.

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 0/3] vl: simplify code for main() and get_boot_device()

[Markus Armbruster]

Chen Gang <gang.chen.5i5j@gmail.com> writes:

> 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 <gang.chen.5i5j@gmail.com>
> ---
>  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.

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 1/3] vl: remove useless 'continue'

[Markus Armbruster]

Chen Gang <gang.chen.5i5j@gmail.com> writes:

> Normal "if (...) {...} else {...}" is enough in "while(...) {...}", not
> need additional useless 'continue'.
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  vl.c |    1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/vl.c b/vl.c
> index 9975e5a..7505002 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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 <armbru@redhat.com>

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 3/3] vl: remove local variable 'args' in the middle of code block

[Markus Armbruster]

Chen Gang <gang.chen.5i5j@gmail.com> writes:

> 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.

>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  vl.c |   18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 377f962..d381443 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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();

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 2/3] vl: remove redundant local variable 'res'

[Chen Gang]

On 04/15/2014 04:43 PM, Markus Armbruster wrote:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
> 
>> On Tue, Apr 8, 2014 at 10:02 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
>>> 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.
> 

Hmm... after think of, for me, it will be fine to still remain the
original state, it is not quit worth to churn.


Thanks.
-- 
Chen Gang

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 1/3] vl: remove useless 'continue'

[Chen Gang]

On 04/15/2014 04:50 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
> 
>> Normal "if (...) {...} else {...}" is enough in "while(...) {...}", not
>> need additional useless 'continue'.
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  vl.c |    1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 9975e5a..7505002 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -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 <armbru@redhat.com>
> 

OK, thanks.

-- 
Chen Gang

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 3/3] vl: remove local variable 'args' in the middle of code block

[Chen Gang]

On 04/15/2014 04:56 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
> 
>> 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.
> 

That sounds fine to me.

And excuse me, my English is not quite well (so may write confusing and
redundancy comments)

Thanks.
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  vl.c |   18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 377f962..d381443 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -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();
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

-- 
Chen Gang

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 3/3] vl: remove local variable 'args' in the middle of code block

[Peter Crosthwaite]

On Tue, Apr 8, 2014 at 10:05 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
> 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 <gang.chen.5i5j@gmail.com>

With Markus' suggest commit message fixup:

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  vl.c |   18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 377f962..d381443 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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
>

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 3/3] vl: remove local variable 'args' in the middle of code block

[Chen Gang]

On 04/15/2014 08:29 PM, Peter Crosthwaite wrote:
> On Tue, Apr 8, 2014 at 10:05 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
>> 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 <gang.chen.5i5j@gmail.com>
> 
> With Markus' suggest commit message fixup:
> 
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 

OK, thanks. I will use the new commit message in the patch v2.

>> ---
>>  vl.c |   18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 377f962..d381443 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -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
>>

-- 
Chen Gang

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 0/3] vl: simplify code for main() and get_boot_device()

[Chen Gang]

On 04/15/2014 04:49 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
> 
>> 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 <gang.chen.5i5j@gmail.com>
>> ---
>>  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.
> 

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.


Thanks.
-- 
Chen Gang

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 0/3] vl: simplify code for main() and get_boot_device()

[Markus Armbruster]

Chen Gang <gang.chen.5i5j@gmail.com> writes:

> On 04/15/2014 04:49 PM, Markus Armbruster wrote:
>> 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.

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 0/3] vl: simplify code for main() and get_boot_device()

[Chen Gang]

On 04/15/2014 10:51 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
> 
>> On 04/15/2014 04:49 PM, Markus Armbruster wrote:
>>> 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.
> 
> Sounds okay.  But sending with git-send-email is so much easier...
> 

I guess so, too (sending with git-send-email is much easier, which I am
not quite familiar).

I have already familiar with Thunderbird, and at least now, I am not a
maintainer, neither a main patch maker. at present, I have no too many
patches to process. So Thunderbird is enough to me. :-)


Thanks.
-- 
Chen Gang

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial 1/3] vl: remove useless 'continue'

[liang ding]

[-- Attachment #1: Type: text/plain, Size: 1095 bytes --]

我怎样才能退订此邮件列表
How can I unsubscribe from this mailing list


2014-04-15 19:05 GMT+08:00 Chen Gang <gang.chen.5i5j@gmail.com>:

>
>
> On 04/15/2014 04:50 PM, Markus Armbruster wrote:
> > Chen Gang <gang.chen.5i5j@gmail.com> writes:
> >
> >> Normal "if (...) {...} else {...}" is enough in "while(...) {...}", not
> >> need additional useless 'continue'.
> >>
> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> >> ---
> >>  vl.c |    1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/vl.c b/vl.c
> >> index 9975e5a..7505002 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -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 <armbru@redhat.com>
> >
>
> OK, thanks.
>
> --
> Chen Gang
>
> Open, share, and attitude like air, water, and life which God blessed
>
>

[-- Attachment #2: Type: text/html, Size: 2279 bytes --]