Hi, Scott

The patch is in the attachment for your review. Below is some notes:

1) Basically I wouldn't like to change any logic of the original code.
2) -serial stdio and -nographic options are removed since they're be covered by the extra parameters.
3) User input would be $poky-qemu qemux86 "<-nographic -m 300>"
4) -m input will be checked still. If it exceeds 128 for arm, it will be changed back to
128M, same logic as before. And after parsing, -m option will be removed and replaced by
Kernel options mem=128M for avoiding some instability issue.

Generally I modified very few, just add an extra parameters with least intrusion of
Current logic.

Any comments are welcomed.
I will conduct more test with latest code in parallel.

Thanks a lot for your help!
criping

On 12/09/2010 12:44 AM, Ke, Liping wrote:

Hi, Scott

The patch is in the attachment for your review. Below is some notes:

1) Basically I wouldn't like to change any logic of the original code.
2) -serial stdio and -nographic options are removed since they're be covered by the extra parameters.
3) User input would be $poky-qemu qemux86 "<-nographic -m 300>"
4) -m input will be checked still. If it exceeds 128 for arm, it will be changed back to
128M, same logic as before. And after parsing, -m option will be removed and replaced by
Kernel options mem=128M for avoiding some instability issue.

Generally I modified very few, just add an extra parameters with least intrusion of
Current logic.

Any comments are welcomed.
I will conduct more test with latest code in parallel.

Hi Criping,

Thanks for the patch. Overall I think this looks good.

However, one thing I feel the need to run by Richard, as he expressed some preferences with how the poky-qemu script would work with regard to options.

Richard: Criping's patch would remove the standalone options we had to the poky-qemu script (e.g, nographic, serial) and instead requires the user to specify them in one command option which can take any qemu command switch.

So for example:

poky-qemu qemux86 nographic

would become:

poky-qemu qemux86 "<-nographic>"

The benefit of this is that this allows the user to specify any qemu option they wish. Previously they were limited by the options we allowed them to specify (which were quite limited).

I tend to feel that this approach is more flexible, and scales better than having to support each and every qemu option with our own script syntax. Is this acceptable, or should we continue to support our own custom options in addition to Criping's new approach?

Scott

--
Scott Garman
Embedded Linux Distro Engineer - Yocto Project

Garman, Scott A wrote:

On 12/09/2010 12:44 AM, Ke, Liping wrote:

Hi, Scott

The patch is in the attachment for your review. Below is some notes:

1) Basically I wouldn't like to change any logic of the original
code. 2) -serial stdio and -nographic options are removed since
they're be covered by the extra parameters. 3) User input would be
$poky-qemu qemux86 "<-nographic -m 300>" 4) -m input will be checked
still. If it exceeds 128 for arm, it will be changed back to
128M, same logic as before. And after parsing, -m option will
be removed and replaced by Kernel options mem=128M for avoiding some
instability issue.

Generally I modified very few, just add an extra parameters with
least intrusion of
Current logic.

Any comments are welcomed.
I will conduct more test with latest code in parallel.

Hi Criping,

Thanks for the patch. Overall I think this looks good.

However, one thing I feel the need to run by Richard, as he expressed
some preferences with how the poky-qemu script would work with regard
to options.

Richard: Criping's patch would remove the standalone options we had to
the poky-qemu script (e.g, nographic, serial) and instead requires the
user to specify them in one command option which can take any qemu
command switch.

So for example:

poky-qemu qemux86 nographic

would become:

poky-qemu qemux86 "<-nographic>"

The benefit of this is that this allows the user to specify any qemu
option they wish. Previously they were limited by the options we
allowed them to specify (which were quite limited).

I tend to feel that this approach is more flexible, and scales better
than having to support each and every qemu option with our own script
syntax. Is this acceptable, or should we continue to support our own
custom options in addition to Criping's new approach?

Liping and I had the discussion regarding this before she went on taking the
approach. Since we now provide this capability to allow user to provide
their qemu params, then why we still in our qemu scripts isolate certain
qemu params out, which may make things complicated unnecessarily and be more
error prone...

Scott

On Thu, 2010-12-09 at 12:44 -0800, Garman, Scott A wrote:

However, one thing I feel the need to run by Richard, as he expressed
some preferences with how the poky-qemu script would work with regard to
options.

Richard: Criping's patch would remove the standalone options we had to
the poky-qemu script (e.g, nographic, serial) and instead requires the
user to specify them in one command option which can take any qemu
command switch.

So for example:

poky-qemu qemux86 nographic

would become:

poky-qemu qemux86 "<-nographic>"

The benefit of this is that this allows the user to specify any qemu
option they wish. Previously they were limited by the options we allowed
them to specify (which were quite limited).

I tend to feel that this approach is more flexible, and scales better
than having to support each and every qemu option with our own script
syntax. Is this acceptable, or should we continue to support our own
custom options in addition to Criping's new approach?

My gut feeling is that having some simplified way to trigger possibly
complex option combinations is still desirable but adding a way to pass
additional custom commandline is equally good. This gives us the maximum
flexibility moving forwards but keeps the script easy to use?

Cheers,

Richard

I tend to feel that this approach is more flexible, and scales better

than having to support each and every qemu option with our own script
syntax. Is this acceptable, or should we continue to support our own
custom options in addition to Criping's new approach?

My gut feeling is that having some simplified way to trigger possibly
complex option combinations is still desirable but adding a way to pass
additional custom commandline is equally good. This gives us the
maximum
flexibility moving forwards but keeps the script easy to use?

Hi, Scott

So the conclusion is that I should keep the old (serial nographic) option while
adding the new "<-XXX -XXX -XXX>" option?
OK. I will send out the modified patch to you for review later.

Thanks& Regards,
criping

Cheers,

Richard

On 12/12/2010 05:43 PM, Ke, Liping wrote:

I tend to feel that this approach is more flexible, and scales better

than having to support each and every qemu option with our own script
syntax. Is this acceptable, or should we continue to support our own
custom options in addition to Criping's new approach?

My gut feeling is that having some simplified way to trigger possibly
complex option combinations is still desirable but adding a way to pass
additional custom commandline is equally good. This gives us the
maximum
flexibility moving forwards but keeps the script easy to use?

Hi, Scott

So the conclusion is that I should keep the old (serial nographic) option while
adding the new "<-XXX -XXX -XXX>" option?
OK. I will send out the modified patch to you for review later.

Yes, please respin the patch with those changes.

Thanks!

Scott

--
Scott Garman
Embedded Linux Distro Engineer - Yocto Project

Hi, Scott

I have updated the patch and tested with poky-tree mode (arm, x86).
Since kvm and serial needs special processing, so for avoiding repeating the code, I will exclude serial and kvm in permitted extra-option, user need to use (serial, kvm) it they want to use it.

For "<-m XXX>" options, I will keep the original logic. If it's arm, the > 128M memory will be forced back to 128M.

It's the high-level user's responsibility to make sure other params are valid.

Any problem, just let me know.

Thanks a lot for your help!

criping

On 12/13/2010 07:41 PM, Ke, Liping wrote:

Hi, Scott

I have updated the patch and tested with poky-tree mode (arm, x86).
Since kvm and serial needs special processing, so for avoiding repeating the code, I will exclude serial and kvm in permitted extra-option, user need to use (serial, kvm) it they want to use it.

For "<-m XXX>" options, I will keep the original logic. If it's arm, the> 128M memory will be forced back to 128M.

It's the high-level user's responsibility to make sure other params are valid.

Any problem, just let me know.

Thanks a lot for your help!

Thanks for the patch, Criping.

This patch includes the addition of a do_configure_prepend step for the libxfixes recipe? That looks like some debugging info crept in that may not have been intended.

In the usage() function, please use $MYNAME instead of $0 for consistency - see the other help echo lines.

The rest of the patch looks ok. Please respin one last time with the above minor changes and I'll accept it.

Thanks,

Scott
--
Scott Garman
Embedded Linux Distro Engineer - Yocto Project

On 12/14/2010 02:27 PM, Scott Garman wrote:

On 12/13/2010 07:41 PM, Ke, Liping wrote:

Hi, Scott

I have updated the patch and tested with poky-tree mode (arm, x86).
Since kvm and serial needs special processing, so for avoiding repeating the code, I will exclude serial and kvm in permitted extra-option, user need to use (serial, kvm) it they want to use it.

For "<-m XXX>" options, I will keep the original logic. If it's arm, the> 128M memory will be forced back to 128M.

It's the high-level user's responsibility to make sure other params are valid.

Any problem, just let me know.

Thanks a lot for your help!

Thanks for the patch, Criping.

This patch includes the addition of a do_configure_prepend step for the
libxfixes recipe? That looks like some debugging info crept in that may
not have been intended.

In the usage() function, please use $MYNAME instead of $0 for
consistency - see the other help echo lines.

The rest of the patch looks ok. Please respin one last time with the
above minor changes and I'll accept it.

One final thing - when you resubmit your patch this time, could you use the create-pull-request/send-pull-request scripts? That way the patch can be pulled in by Richard or Saul with minimal effort, and it will help ensure you get proper credit in the commit log.

Scott

--
Scott Garman
Embedded Linux Distro Engineer - Yocto Project

Hi, Scott

Thanks for the review! Sure, I will use the script following distro-team conventions.

And also, $MYNAME will be used in the final patch. And the debug line for building will be
removed when I send the final patch.

Thanks & Regards,
criping