Patchwork [v2] Introduce new SERIAL_CONSOLES to add multiple consoles for your MACHINE

login
register
mail settings
Submitter Matthew McClintock
Date Nov. 4, 2011, 9:25 p.m.
Message ID <1320441941-30850-1-git-send-email-msm@freescale.com>
Download mbox | patch
Permalink /patch/14315/
State Deferred
Headers show

Comments

Matthew McClintock - Nov. 4, 2011, 9:25 p.m.
Just define additional serial consoles like so:

SERIAL_CONSOLES="115200;ttyS0 115200;ttyS1 ... 115200;ttySN"

Also be sure to remove SERIAL_CONSOLE (lacking the S) from your
machine as they can conflict.

Signed-off-by: Matthew McClintock <msm@freescale.com>
---
v2: handle case where SERIAL_CONSOLES is not defined

 .../sysvinit/sysvinit-inittab_2.88dsf.bb           |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
Phil Blundell - Nov. 4, 2011, 9:34 p.m.
On Fri, 2011-11-04 at 16:25 -0500, Matthew McClintock wrote:
> v2: handle case where SERIAL_CONSOLES is not defined
> 
>  .../sysvinit/sysvinit-inittab_2.88dsf.bb           |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb b/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb
> index ba60c74..adab262 100644
> --- a/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb
> +++ b/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb
> @@ -25,6 +25,18 @@ do_install() {
>      if [ ! -z "${SERIAL_CONSOLE}" ]; then
>          echo "S:2345:respawn:${base_sbindir}/getty ${SERIAL_CONSOLE}" >> ${D}${sysconfdir}/inittab
>      fi
> +
> +    idx=0
> +    tmp="${SERIAL_CONSOLES}"
> +    if [ "x" != "x$tmp" ]; then
> +	for i in $tmp
> +	do
> +	    j=`echo ${i} | sed s/\;/\ /g`
> +	    echo "${idx}:2345:respawn:${base_sbindir}/getty ${j}" >> ${D}${sysconfdir}/inittab
> +	    idx=`expr $idx + 1`
> +	done
> +    fi
> +

Did you test that this actually solves the problem?  It looks to me as
though it will still do the wrong thing if the variable is not defined
at all (as opposed to being defined to the empty string).  The added
"if" statement doesn't actually achieve much since, if $tmp is the empty
string, the for-loop wouldn't have executed anyway and there is no need
for an extra guard.

Also, for what it's worth, the "x$tmp" idiom is unnecessary if you're
going to quote the strings.

p.
McClintock Matthew-B29882 - Nov. 4, 2011, 10:12 p.m.
On Nov 4, 2011 4:35 PM, "Phil Blundell" <philb@gnu.org<mailto:philb@gnu.org>> wrote:
>
> On Fri, 2011-11-04 at 16:25 -0500, Matthew McClintock wrote:
> > v2: handle case where SERIAL_CONSOLES is not defined
> >
> >  .../sysvinit/sysvinit-inittab_2.88dsf.bb<http://sysvinit-inittab_2.88dsf.bb>           |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> >
> > diff --git a/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb<http://sysvinit-inittab_2.88dsf.bb> b/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb<http://sysvinit-inittab_2.88dsf.bb>
> > index ba60c74..adab262 100644
> > --- a/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb<http://sysvinit-inittab_2.88dsf.bb>
> > +++ b/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb<http://sysvinit-inittab_2.88dsf.bb>
> > @@ -25,6 +25,18 @@ do_install() {
> >      if [ ! -z "${SERIAL_CONSOLE}" ]; then
> >          echo "S:2345:respawn:${base_sbindir}/getty ${SERIAL_CONSOLE}" >> ${D}${sysconfdir}/inittab
> >      fi
> > +
> > +    idx=0
> > +    tmp="${SERIAL_CONSOLES}"
> > +    if [ "x" != "x$tmp" ]; then
> > +     for i in $tmp
> > +     do
> > +         j=`echo ${i} | sed s/\;/\ /g`
> > +         echo "${idx}:2345:respawn:${base_sbindir}/getty ${j}" >> ${D}${sysconfdir}/inittab
> > +         idx=`expr $idx + 1`
> > +     done
> > +    fi
> > +
>
> Did you test that this actually solves the problem?  It looks to me as
> though it will still do the wrong thing if the variable is not defined
> at all (as opposed to being defined to the empty string).  The added
> "if" statement doesn't actually achieve much since, if $tmp is the empty
> string, the for-loop wouldn't have executed anyway and there is no need
> for an extra guard.
>
> Also, for what it's worth, the "x$tmp" idiom is unnecessary if you're
> going to quote the strings.
>
> p.
>
>

Thanks for the feedback. Ill look at this again when I'm fresh.

-M

>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org<mailto:Openembedded-core@lists.openembedded.org>
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
McClintock Matthew-B29882 - Nov. 7, 2011, 11:03 p.m.
On Fri, Nov 4, 2011 at 4:34 PM, Phil Blundell <philb@gnu.org> wrote:
>> --- a/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb
>> +++ b/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb
>> @@ -25,6 +25,18 @@ do_install() {
>>      if [ ! -z "${SERIAL_CONSOLE}" ]; then
>>          echo "S:2345:respawn:${base_sbindir}/getty ${SERIAL_CONSOLE}" >> ${D}${sysconfdir}/inittab
>>      fi
>> +
>> +    idx=0
>> +    tmp="${SERIAL_CONSOLES}"
>> +    if [ "x" != "x$tmp" ]; then
>> +     for i in $tmp
>> +     do
>> +         j=`echo ${i} | sed s/\;/\ /g`
>> +         echo "${idx}:2345:respawn:${base_sbindir}/getty ${j}" >> ${D}${sysconfdir}/inittab
>> +         idx=`expr $idx + 1`
>> +     done
>> +    fi
>> +
>
> Did you test that this actually solves the problem?  It looks to me as
> though it will still do the wrong thing if the variable is not defined
> at all (as opposed to being defined to the empty string).  The added
> "if" statement doesn't actually achieve much since, if $tmp is the empty
> string, the for-loop wouldn't have executed anyway and there is no need
> for an extra guard.
>
> Also, for what it's worth, the "x$tmp" idiom is unnecessary if you're
> going to quote the strings.

I don't get it. I think the first version was still OK. The first
version is pasted below.

> +    idx=0
> +    tmp="${SERIAL_CONSOLES}" <- If it's not defined this will catch it.
> +    for i in $tmp
> +    do
> +     j=`echo ${i} | sed s/\;/\ /g`
> +        echo "${idx}:2345:respawn:${base_sbindir}/getty ${j}" >> ${D}${sysconfdir}/inittab
> +     idx=`expr $idx + 1`
> +    done

See my inline comment above, but if SERIAL_CONSOLES is not defined
tmp="" and the for loop won't execute. Not sure if there is a more
standard way to do this though.

-M
McClintock Matthew-B29882 - Nov. 8, 2011, 7:36 p.m.
On Mon, Nov 7, 2011 at 5:03 PM, Matthew McClintock <msm@freescale.com> wrote:
> I don't get it. I think the first version was still OK. The first
> version is pasted below.
>
>> +    idx=0
>> +    tmp="${SERIAL_CONSOLES}" <- If it's not defined this will catch it.
>> +    for i in $tmp
>> +    do
>> +     j=`echo ${i} | sed s/\;/\ /g`
>> +        echo "${idx}:2345:respawn:${base_sbindir}/getty ${j}" >> ${D}${sysconfdir}/inittab
>> +     idx=`expr $idx + 1`
>> +    done
>
> See my inline comment above, but if SERIAL_CONSOLES is not defined
> tmp="" and the for loop won't execute. Not sure if there is a more
> standard way to do this though.

ping..

-M
Phil Blundell - Nov. 8, 2011, 8:47 p.m.
On Tue, 2011-11-08 at 19:36 +0000, McClintock Matthew-B29882 wrote:
> On Mon, Nov 7, 2011 at 5:03 PM, Matthew McClintock <msm@freescale.com> wrote:
> > I don't get it. I think the first version was still OK. The first
> > version is pasted below.
> >
> >> +    idx=0
> >> +    tmp="${SERIAL_CONSOLES}" <- If it's not defined this will catch it.
> >> +    for i in $tmp
> >> +    do
> >> +     j=`echo ${i} | sed s/\;/\ /g`
> >> +        echo "${idx}:2345:respawn:${base_sbindir}/getty ${j}" >> ${D}${sysconfdir}/inittab
> >> +     idx=`expr $idx + 1`
> >> +    done
> >
> > See my inline comment above, but if SERIAL_CONSOLES is not defined
> > tmp="" and the for loop won't execute. Not sure if there is a more
> > standard way to do this though.
> 
> ping..

Yeah, now I look at this again I think it is probably OK.  Bitbake won't
expand ${SERIAL_CONSOLES} if it's unset, but the shell will and I think
you probably will get the right results.  Assuming you've actually
tested that case and it does work, I think I'm happy with what you have
above.

p.
McClintock Matthew-B29882 - Nov. 8, 2011, 8:50 p.m.
On Tue, Nov 8, 2011 at 2:47 PM, Phil Blundell <philb@gnu.org> wrote:
> On Tue, 2011-11-08 at 19:36 +0000, McClintock Matthew-B29882 wrote:
>> On Mon, Nov 7, 2011 at 5:03 PM, Matthew McClintock <msm@freescale.com> wrote:
>> > I don't get it. I think the first version was still OK. The first
>> > version is pasted below.
>> >
>> >> +    idx=0
>> >> +    tmp="${SERIAL_CONSOLES}" <- If it's not defined this will catch it.
>> >> +    for i in $tmp
>> >> +    do
>> >> +     j=`echo ${i} | sed s/\;/\ /g`
>> >> +        echo "${idx}:2345:respawn:${base_sbindir}/getty ${j}" >> ${D}${sysconfdir}/inittab
>> >> +     idx=`expr $idx + 1`
>> >> +    done
>> >
>> > See my inline comment above, but if SERIAL_CONSOLES is not defined
>> > tmp="" and the for loop won't execute. Not sure if there is a more
>> > standard way to do this though.
>>
>> ping..
>
> Yeah, now I look at this again I think it is probably OK.  Bitbake won't
> expand ${SERIAL_CONSOLES} if it's unset, but the shell will and I think
> you probably will get the right results.  Assuming you've actually
> tested that case and it does work, I think I'm happy with what you have
> above.

Thanks Phil, I wanted to make sure I was not missing something obvious ;).

Maintainers, please consider v1 of this patch.

Thanks,
Matthew
Saul Wold - Nov. 8, 2011, 9:06 p.m.
On 11/08/2011 12:50 PM, McClintock Matthew-B29882 wrote:
> On Tue, Nov 8, 2011 at 2:47 PM, Phil Blundell<philb@gnu.org>  wrote:
>> On Tue, 2011-11-08 at 19:36 +0000, McClintock Matthew-B29882 wrote:
>>> On Mon, Nov 7, 2011 at 5:03 PM, Matthew McClintock<msm@freescale.com>  wrote:
>>>> I don't get it. I think the first version was still OK. The first
>>>> version is pasted below.
>>>>
>>>>> +    idx=0
>>>>> +    tmp="${SERIAL_CONSOLES}"<- If it's not defined this will catch it.
>>>>> +    for i in $tmp
>>>>> +    do
>>>>> +     j=`echo ${i} | sed s/\;/\ /g`
>>>>> +        echo "${idx}:2345:respawn:${base_sbindir}/getty ${j}">>  ${D}${sysconfdir}/inittab
>>>>> +     idx=`expr $idx + 1`
>>>>> +    done
>>>>
>>>> See my inline comment above, but if SERIAL_CONSOLES is not defined
>>>> tmp="" and the for loop won't execute. Not sure if there is a more
>>>> standard way to do this though.
>>>
>>> ping..
>>
>> Yeah, now I look at this again I think it is probably OK.  Bitbake won't
>> expand ${SERIAL_CONSOLES} if it's unset, but the shell will and I think
>> you probably will get the right results.  Assuming you've actually
>> tested that case and it does work, I think I'm happy with what you have
>> above.
>
> Thanks Phil, I wanted to make sure I was not missing something obvious ;).
>
> Maintainers, please consider v1 of this patch.
>
Just to follow up, this is now in my queue.

Sau!

> Thanks,
> Matthew
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>
Saul Wold - Nov. 10, 2011, 5:21 p.m.
On 11/04/2011 02:25 PM, Matthew McClintock wrote:
> Just define additional serial consoles like so:
>
> SERIAL_CONSOLES="115200;ttyS0 115200;ttyS1 ... 115200;ttySN"
>
> Also be sure to remove SERIAL_CONSOLE (lacking the S) from your
> machine as they can conflict.
>
> Signed-off-by: Matthew McClintock<msm@freescale.com>
> ---
> v2: handle case where SERIAL_CONSOLES is not defined
>
>   .../sysvinit/sysvinit-inittab_2.88dsf.bb           |   12 ++++++++++++
>   1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb b/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb
> index ba60c74..adab262 100644
> --- a/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb
> +++ b/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb
> @@ -25,6 +25,18 @@ do_install() {
>       if [ ! -z "${SERIAL_CONSOLE}" ]; then
>           echo "S:2345:respawn:${base_sbindir}/getty ${SERIAL_CONSOLE}">>  ${D}${sysconfdir}/inittab
>       fi
> +
> +    idx=0
> +    tmp="${SERIAL_CONSOLES}"
> +    if [ "x" != "x$tmp" ]; then
> +	for i in $tmp
> +	do
> +	    j=`echo ${i} | sed s/\;/\ /g`
> +	    echo "${idx}:2345:respawn:${base_sbindir}/getty ${j}">>  ${D}${sysconfdir}/inittab
> +	    idx=`expr $idx + 1`
> +	done
> +    fi
> +
>       if [ "${USE_VT}" = "1" ]; then
>           cat<<EOF>>${D}${sysconfdir}/inittab
>   # ${base_sbindir}/getty invocations for the runlevels.

Merged into OE-Core

Thanks
	Sau!
Chris Larson - Nov. 11, 2011, 1:06 a.m.
On Tue, Nov 8, 2011 at 1:50 PM, McClintock Matthew-B29882
<B29882@freescale.com> wrote:
> On Tue, Nov 8, 2011 at 2:47 PM, Phil Blundell <philb@gnu.org> wrote:
>> On Tue, 2011-11-08 at 19:36 +0000, McClintock Matthew-B29882 wrote:
>>> On Mon, Nov 7, 2011 at 5:03 PM, Matthew McClintock <msm@freescale.com> wrote:
>>> > I don't get it. I think the first version was still OK. The first
>>> > version is pasted below.
>>> >
>>> >> +    idx=0
>>> >> +    tmp="${SERIAL_CONSOLES}" <- If it's not defined this will catch it.
>>> >> +    for i in $tmp
>>> >> +    do
>>> >> +     j=`echo ${i} | sed s/\;/\ /g`
>>> >> +        echo "${idx}:2345:respawn:${base_sbindir}/getty ${j}" >> ${D}${sysconfdir}/inittab
>>> >> +     idx=`expr $idx + 1`
>>> >> +    done
>>> >
>>> > See my inline comment above, but if SERIAL_CONSOLES is not defined
>>> > tmp="" and the for loop won't execute. Not sure if there is a more
>>> > standard way to do this though.
>>>
>>> ping..
>>
>> Yeah, now I look at this again I think it is probably OK.  Bitbake won't
>> expand ${SERIAL_CONSOLES} if it's unset, but the shell will and I think
>> you probably will get the right results.  Assuming you've actually
>> tested that case and it does work, I think I'm happy with what you have
>> above.
>
> Thanks Phil, I wanted to make sure I was not missing something obvious ;).
>

Note that one could always define SERIAL_CONSOLES ?= "" somewhere
appropriate. Wouldn't hurt anything to define defaults for things we
use. Maybe someday we can make referencing undefined variables raise
an error rather than this whole 'if bitbake doesn't expand it, let the
shell do it' behavior which is unintuitive. Not easily knowing when
something will be evaluated is problematic.

Patch

diff --git a/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb b/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb
index ba60c74..adab262 100644
--- a/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb
+++ b/meta/recipes-core/sysvinit/sysvinit-inittab_2.88dsf.bb
@@ -25,6 +25,18 @@  do_install() {
     if [ ! -z "${SERIAL_CONSOLE}" ]; then
         echo "S:2345:respawn:${base_sbindir}/getty ${SERIAL_CONSOLE}" >> ${D}${sysconfdir}/inittab
     fi
+
+    idx=0
+    tmp="${SERIAL_CONSOLES}"
+    if [ "x" != "x$tmp" ]; then
+	for i in $tmp
+	do
+	    j=`echo ${i} | sed s/\;/\ /g`
+	    echo "${idx}:2345:respawn:${base_sbindir}/getty ${j}" >> ${D}${sysconfdir}/inittab
+	    idx=`expr $idx + 1`
+	done
+    fi
+
     if [ "${USE_VT}" = "1" ]; then
         cat <<EOF >>${D}${sysconfdir}/inittab
 # ${base_sbindir}/getty invocations for the runlevels.