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