[v2,1/2] getty: Create specific getty config + cleanups

Submitted by Assaf Inbal on Aug. 25, 2013, 11:13 a.m.

Details

Message ID 1377429219-25140-1-git-send-email-shmuelzon@gmail.com
State Superseded
Headers show

Commit Message

Assaf Inbal Aug. 25, 2013, 11:13 a.m.
This commit introduces a specific BR2_TARGET_GENERIC_GETTY configuration flag.
This eliminates the need for checking if BR2_TARGET_GENERIC_GETTY_PORT is an
empty string or not. It also allows hiding various getty options when getty
isn't enabled.

Signed-off-by: Assaf Inbal <shmuelzon@gmail.com>
---
 system/Config.in |    9 +++++++--
 system/system.mk |   27 ++++++++++++++-------------
 2 files changed, 21 insertions(+), 15 deletions(-)

Comments

Thomas De Schampheleire Aug. 27, 2013, 10:33 a.m.
Hi Assaf,

Thanks for your patch. Some comments below...

On Sun, Aug 25, 2013 at 1:13 PM, Assaf Inbal <shmuelzon@gmail.com> wrote:
> This commit introduces a specific BR2_TARGET_GENERIC_GETTY configuration flag.
> This eliminates the need for checking if BR2_TARGET_GENERIC_GETTY_PORT is an
> empty string or not. It also allows hiding various getty options when getty
> isn't enabled.
>
> Signed-off-by: Assaf Inbal <shmuelzon@gmail.com>
> ---
>  system/Config.in |    9 +++++++--
>  system/system.mk |   27 ++++++++++++++-------------
>  2 files changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/system/Config.in b/system/Config.in
> index d41f184..403b229 100644
> --- a/system/Config.in
> +++ b/system/Config.in
> @@ -194,12 +194,16 @@ config BR2_TARGET_GENERIC_ROOT_PASSWD
>           in the build log! Avoid using a valuable password if either the
>           .config file or the build log may be distributed!
>
> +config BR2_TARGET_GENERIC_GETTY
> +       bool "Run a getty (login prompt) after boot"
> +       default y
> +
> +if BR2_TARGET_GENERIC_GETTY
>  config BR2_TARGET_GENERIC_GETTY_PORT
>         string "Port to run a getty (login prompt) on"
>         default "ttyS0"
>         help
> -         Specify a port to run a getty (login prompt) on.
> -         Set to the empty string to not run a getty.
> +         Specify a port to run a getty on.
>
>  choice
>         prompt "Baudrate to use"
> @@ -235,6 +239,7 @@ config BR2_TARGET_GENERIC_GETTY_TERM
>         default "vt100"
>         help
>           Specify a TERM type.
> +endif

The text of the suboptions to the new getty top-level option are still
somewhat chaotic:
[*] Run a getty (login prompt) after boot
(ttyS0) Port to run a getty (login prompt) on
Baudrate to use (115200)  --->
(vt100) Value to assign the TERM environment variable
()    other options to pass to getty


My proposal is to simplify the suboptions:
[*] Run a getty (login prompt) after boot
(ttyS0) tty port
baudrate (115200)  --->
(vt100) TERM environment variable
()    other options to pass to getty

>
>  config BR2_TARGET_GENERIC_REMOUNT_ROOTFS_RW
>         bool "remount root filesystem read-write during boot"
> diff --git a/system/system.mk b/system/system.mk
> index b4ddc3e..c9e7cf7 100644
> --- a/system/system.mk
> +++ b/system/system.mk
> @@ -1,14 +1,15 @@
> -TARGET_GENERIC_HOSTNAME := $(call qstrip,$(BR2_TARGET_GENERIC_HOSTNAME))
> -TARGET_GENERIC_ISSUE := $(call qstrip,$(BR2_TARGET_GENERIC_ISSUE))
> -TARGET_GENERIC_ROOT_PASSWD := $(call qstrip,$(BR2_TARGET_GENERIC_ROOT_PASSWD))
> -TARGET_GENERIC_PASSWD_METHOD := $(call qstrip,$(BR2_TARGET_GENERIC_PASSWD_METHOD))
> -TARGET_GENERIC_GETTY := $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_PORT))
> -TARGET_GENERIC_GETTY_BAUDRATE := $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE))
> -TARGET_GENERIC_GETTY_TERM := $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_TERM))
> +TARGET_GENERIC_HOSTNAME = $(call qstrip,$(BR2_TARGET_GENERIC_HOSTNAME))
> +TARGET_GENERIC_ISSUE = $(call qstrip,$(BR2_TARGET_GENERIC_ISSUE))
> +TARGET_GENERIC_ROOT_PASSWD = $(call qstrip,$(BR2_TARGET_GENERIC_ROOT_PASSWD))
> +TARGET_GENERIC_PASSWD_METHOD = $(call qstrip,$(BR2_TARGET_GENERIC_PASSWD_METHOD))
> +TARGET_GENERIC_GETTY_PORT = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_PORT))
> +TARGET_GENERIC_GETTY_BAUDRATE = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE))
> +TARGET_GENERIC_GETTY_TERM = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_TERM))
> +TARGET_GENERIC_GETTY_OPTIONS = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_OPTIONS))

Although in general = is favored over := in buildroot, I'm now
wondering if this is the right choice here: variable
TARGET_GENERIC_GETTY_PORT is used several times in .mk. If = is used,
the qstrip expansion is done each time, while := will do it once.

Arnout: isn't this ($(call), $(shell), ...) a case where we should
rather use := iso = ?

>
>  target-generic-securetty:
> -       grep -q '^$(TARGET_GENERIC_GETTY)$$' $(TARGET_DIR)/etc/securetty || \
> -               echo '$(TARGET_GENERIC_GETTY)' >> $(TARGET_DIR)/etc/securetty
> +       grep -q '^$(TARGET_GENERIC_GETTY_PORT)$$' $(TARGET_DIR)/etc/securetty || \
> +               echo '$(TARGET_GENERIC_GETTY_PORT)' >> $(TARGET_DIR)/etc/securetty
>
>  target-generic-hostname:
>         mkdir -p $(TARGET_DIR)/etc
> @@ -29,13 +30,13 @@ target-root-passwd:
>         $(SED) "s,^root:[^:]*:,root:$$TARGET_GENERIC_ROOT_PASSWD_HASH:," $(TARGET_DIR)/etc/shadow
>
>  target-generic-getty-busybox:
> -       $(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(TARGET_GENERIC_GETTY)::respawn:/sbin/getty -L $(TARGET_GENERIC_GETTY) $(TARGET_GENERIC_GETTY_BAUDRATE) $(TARGET_GENERIC_GETTY_TERM) #~' \
> +       $(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(TARGET_GENERIC_GETTY_PORT)::respawn:/sbin/getty -L $(TARGET_GENERIC_GETTY_PORT) $(TARGET_GENERIC_GETTY_BAUDRATE) $(TARGET_GENERIC_GETTY_TERM) #~' \
>                 $(TARGET_DIR)/etc/inittab
>
>  # In sysvinit inittab, the "id" must not be longer than 4 bytes, so we
>  # skip the "tty" part and keep only the remaining.
>  target-generic-getty-sysvinit:
> -       $(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(shell echo $(TARGET_GENERIC_GETTY) | tail -c+4)::respawn:/sbin/getty -L $(TARGET_GENERIC_GETTY) $(TARGET_GENERIC_GETTY_BAUDRATE) $(TARGET_GENERIC_GETTY_TERM) #~' \
> +       $(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(shell echo $(TARGET_GENERIC_GETTY_PORT) | tail -c+4)::respawn:/sbin/getty -L $(TARGET_GENERIC_GETTY_PORT) $(TARGET_GENERIC_GETTY_BAUDRATE) $(TARGET_GENERIC_GETTY_TERM) #~' \
>                 $(TARGET_DIR)/etc/inittab
>
>  # Find commented line, if any, and remove leading '#'s
> @@ -46,7 +47,7 @@ target-generic-do-remount-rw:
>  target-generic-dont-remount-rw:
>         $(SED) '/^[^#].*# REMOUNT_ROOTFS_RW$$/s~^~#~' $(TARGET_DIR)/etc/inittab
>
> -ifneq ($(TARGET_GENERIC_GETTY),)
> +ifeq ($(BR2_TARGET_GENERIC_GETTY),y)
>  TARGETS += target-generic-securetty
>  endif
>
> @@ -61,7 +62,7 @@ endif
>  ifeq ($(BR2_ROOTFS_SKELETON_DEFAULT),y)
>  TARGETS += target-root-passwd
>
> -ifneq ($(TARGET_GENERIC_GETTY),)
> +ifeq ($(BR2_TARGET_GENERIC_GETTY),y)
>  TARGETS += target-generic-getty-$(if $(BR2_PACKAGE_SYSVINIT),sysvinit,busybox)
>  endif
>
> --
> 1.7.9.5

Best regards,
Thomas
Arnout Vandecappelle Aug. 27, 2013, 12:27 p.m.
On 08/27/13 12:33, Thomas De Schampheleire wrote:
>> >+TARGET_GENERIC_HOSTNAME = $(call qstrip,$(BR2_TARGET_GENERIC_HOSTNAME))
>> >+TARGET_GENERIC_ISSUE = $(call qstrip,$(BR2_TARGET_GENERIC_ISSUE))
>> >+TARGET_GENERIC_ROOT_PASSWD = $(call qstrip,$(BR2_TARGET_GENERIC_ROOT_PASSWD))
>> >+TARGET_GENERIC_PASSWD_METHOD = $(call qstrip,$(BR2_TARGET_GENERIC_PASSWD_METHOD))
>> >+TARGET_GENERIC_GETTY_PORT = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_PORT))
>> >+TARGET_GENERIC_GETTY_BAUDRATE = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE))
>> >+TARGET_GENERIC_GETTY_TERM = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_TERM))
>> >+TARGET_GENERIC_GETTY_OPTIONS = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_OPTIONS))
> Although in general = is favored over := in buildroot, I'm now
> wondering if this is the right choice here: variable
> TARGET_GENERIC_GETTY_PORT is used several times in .mk. If = is used,
> the qstrip expansion is done each time, while := will do it once.

  Remember that := will expand once, even if it is never used. So using = 
is sometimes even more efficient.


> Arnout: isn't this ($(call), $(shell), ...) a case where we should
> rather use := iso = ?

  For $(shell), yes (on condition that it is _always_ evaluated). But the 
overhead of $(call) isn't very high. These variables are used a couple of 
times, but every time it is within a separate sub-shell - the overhead of 
starting that sub-shell is way higher than the overhead of running a $(call).

  So no, this is not a case where := would be more appropriate. It's not 
even a case where := is necessarily faster.

  Regards,
  Arnout
Assaf Inbal Aug. 28, 2013, 4:41 a.m.
Hey Thomas,

The text of the suboptions to the new getty top-level option are still
> somewhat chaotic:
> [*] Run a getty (login prompt) after boot
> (ttyS0) Port to run a getty (login prompt) on
> Baudrate to use (115200)  --->
> (vt100) Value to assign the TERM environment variable
> ()    other options to pass to getty
>
>
> My proposal is to simplify the suboptions:
> [*] Run a getty (login prompt) after boot
> (ttyS0) tty port
> baudrate (115200)  --->
> (vt100) TERM environment variable
> ()    other options to pass to getty
>

Would you like to me submit another version of the patch with your wording,
or will you do this yourself when applying the patch?

Good day,
Assaf
Thomas De Schampheleire Aug. 28, 2013, 6:36 a.m.
Hi Assaf,

On Wed, Aug 28, 2013 at 6:41 AM, Assaf Inbal <shmuelzon@gmail.com> wrote:
> Hey Thomas,
>
>> The text of the suboptions to the new getty top-level option are still
>> somewhat chaotic:
>> [*] Run a getty (login prompt) after boot
>> (ttyS0) Port to run a getty (login prompt) on
>> Baudrate to use (115200)  --->
>> (vt100) Value to assign the TERM environment variable
>> ()    other options to pass to getty
>>
>>
>> My proposal is to simplify the suboptions:
>> [*] Run a getty (login prompt) after boot
>> (ttyS0) tty port
>> baudrate (115200)  --->
>> (vt100) TERM environment variable
>> ()    other options to pass to getty
>
>
> Would you like to me submit another version of the patch with your wording,
> or will you do this yourself when applying the patch?

I'm just a 'regular' developer like you, I cannot apply the patch to
the buildroot repo. Only the maintainer Peter Korsgaard can do that
(or recently the maintainer-ad-interim Thomas Petazzoni)
I think it is best if you submit a new version, to minimize the work
he has to do.

Thanks again for your contributions.

Best regards,
Thomas
Thomas De Schampheleire Sept. 4, 2013, 9:31 a.m.
Hi Assaf,

On Wed, Aug 28, 2013 at 8:43 AM, Assaf Inbal <shmuelzon@gmail.com> wrote:
> Hey,
>
>> I'm just a 'regular' developer like you,
>
>
> Must have gotten my Thomas's mixed up :)
> Sorry about that.
> I'll send in a new patch shortly.

I haven't seen a new version yet. When you have some time, could you
do it? I think the patch series is a nice improvement so I don't want
to loose this...

Thanks,
Thomas
Assaf Inbal Sept. 6, 2013, 6:59 a.m.
Hey,

> I haven't seen a new version yet. When you have some time, could you
> do it? I think the patch series is a nice improvement so I don't want
> to loose this...

Sorry, got caught up by my actual day job :)
Two quick questions:
- Would it be better to create a sub menu called "getty options"? I
think that otherwise it would be clear what these options refer to. It
would also clean up the main "system" menu.
- In order to avoid the new thread that started with the 2nd version
of the patch, how should I configure git send-email to reply to this
thread? I altered the "reply-to" field but that didn't seem to work.

Good day,
Assaf
Thomas De Schampheleire Sept. 6, 2013, 8:18 a.m.
On Fri, Sep 6, 2013 at 8:59 AM, Assaf Inbal <shmuelzon@gmail.com> wrote:
> Hey,
>
>> I haven't seen a new version yet. When you have some time, could you
>> do it? I think the patch series is a nice improvement so I don't want
>> to loose this...
>
> Sorry, got caught up by my actual day job :)
> Two quick questions:
> - Would it be better to create a sub menu called "getty options"? I
> think that otherwise it would be clear what these options refer to. It
> would also clean up the main "system" menu.

We don't usually have real submenus in buildroot, but I don't have a
very strong opinion on this.

> - In order to avoid the new thread that started with the 2nd version
> of the patch, how should I configure git send-email to reply to this
> thread? I altered the "reply-to" field but that didn't seem to work.

There are different opinions in the buildroot community regarding the
desirability of such in-reply-to patches. I'm personally not an
opponent of it, for the following reasons:

1. from the list of e-mails (in gmail for example) it is unclear if
the new reply is just a comment on the current version of the patch,
or if it is a new version. You really have to check the reply.

2. it doesn't work well for patch series: the first patch would indeed
be a reply to the previous thread, but all other patches would not.
This makes it very annoying to read all patches in a series.

Therefore, I always send new versions as new standalone e-mails, with
a marker vX to indicate the version. Several other buildroot
contributors do the same.

Best regards,
Thomas

Patch hide | download patch | download mbox

diff --git a/system/Config.in b/system/Config.in
index d41f184..403b229 100644
--- a/system/Config.in
+++ b/system/Config.in
@@ -194,12 +194,16 @@  config BR2_TARGET_GENERIC_ROOT_PASSWD
 	  in the build log! Avoid using a valuable password if either the
 	  .config file or the build log may be distributed!
 
+config BR2_TARGET_GENERIC_GETTY
+	bool "Run a getty (login prompt) after boot"
+	default y
+
+if BR2_TARGET_GENERIC_GETTY
 config BR2_TARGET_GENERIC_GETTY_PORT
 	string "Port to run a getty (login prompt) on"
 	default "ttyS0"
 	help
-	  Specify a port to run a getty (login prompt) on.
-	  Set to the empty string to not run a getty.
+	  Specify a port to run a getty on.
 
 choice
 	prompt "Baudrate to use"
@@ -235,6 +239,7 @@  config BR2_TARGET_GENERIC_GETTY_TERM
 	default "vt100"
 	help
 	  Specify a TERM type.
+endif
 
 config BR2_TARGET_GENERIC_REMOUNT_ROOTFS_RW
 	bool "remount root filesystem read-write during boot"
diff --git a/system/system.mk b/system/system.mk
index b4ddc3e..c9e7cf7 100644
--- a/system/system.mk
+++ b/system/system.mk
@@ -1,14 +1,15 @@ 
-TARGET_GENERIC_HOSTNAME := $(call qstrip,$(BR2_TARGET_GENERIC_HOSTNAME))
-TARGET_GENERIC_ISSUE := $(call qstrip,$(BR2_TARGET_GENERIC_ISSUE))
-TARGET_GENERIC_ROOT_PASSWD := $(call qstrip,$(BR2_TARGET_GENERIC_ROOT_PASSWD))
-TARGET_GENERIC_PASSWD_METHOD := $(call qstrip,$(BR2_TARGET_GENERIC_PASSWD_METHOD))
-TARGET_GENERIC_GETTY := $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_PORT))
-TARGET_GENERIC_GETTY_BAUDRATE := $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE))
-TARGET_GENERIC_GETTY_TERM := $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_TERM))
+TARGET_GENERIC_HOSTNAME = $(call qstrip,$(BR2_TARGET_GENERIC_HOSTNAME))
+TARGET_GENERIC_ISSUE = $(call qstrip,$(BR2_TARGET_GENERIC_ISSUE))
+TARGET_GENERIC_ROOT_PASSWD = $(call qstrip,$(BR2_TARGET_GENERIC_ROOT_PASSWD))
+TARGET_GENERIC_PASSWD_METHOD = $(call qstrip,$(BR2_TARGET_GENERIC_PASSWD_METHOD))
+TARGET_GENERIC_GETTY_PORT = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_PORT))
+TARGET_GENERIC_GETTY_BAUDRATE = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE))
+TARGET_GENERIC_GETTY_TERM = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_TERM))
+TARGET_GENERIC_GETTY_OPTIONS = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_OPTIONS))
 
 target-generic-securetty:
-	grep -q '^$(TARGET_GENERIC_GETTY)$$' $(TARGET_DIR)/etc/securetty || \
-		echo '$(TARGET_GENERIC_GETTY)' >> $(TARGET_DIR)/etc/securetty
+	grep -q '^$(TARGET_GENERIC_GETTY_PORT)$$' $(TARGET_DIR)/etc/securetty || \
+		echo '$(TARGET_GENERIC_GETTY_PORT)' >> $(TARGET_DIR)/etc/securetty
 
 target-generic-hostname:
 	mkdir -p $(TARGET_DIR)/etc
@@ -29,13 +30,13 @@  target-root-passwd:
 	$(SED) "s,^root:[^:]*:,root:$$TARGET_GENERIC_ROOT_PASSWD_HASH:," $(TARGET_DIR)/etc/shadow
 
 target-generic-getty-busybox:
-	$(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(TARGET_GENERIC_GETTY)::respawn:/sbin/getty -L $(TARGET_GENERIC_GETTY) $(TARGET_GENERIC_GETTY_BAUDRATE) $(TARGET_GENERIC_GETTY_TERM) #~' \
+	$(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(TARGET_GENERIC_GETTY_PORT)::respawn:/sbin/getty -L $(TARGET_GENERIC_GETTY_PORT) $(TARGET_GENERIC_GETTY_BAUDRATE) $(TARGET_GENERIC_GETTY_TERM) #~' \
 		$(TARGET_DIR)/etc/inittab
 
 # In sysvinit inittab, the "id" must not be longer than 4 bytes, so we
 # skip the "tty" part and keep only the remaining.
 target-generic-getty-sysvinit:
-	$(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(shell echo $(TARGET_GENERIC_GETTY) | tail -c+4)::respawn:/sbin/getty -L $(TARGET_GENERIC_GETTY) $(TARGET_GENERIC_GETTY_BAUDRATE) $(TARGET_GENERIC_GETTY_TERM) #~' \
+	$(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(shell echo $(TARGET_GENERIC_GETTY_PORT) | tail -c+4)::respawn:/sbin/getty -L $(TARGET_GENERIC_GETTY_PORT) $(TARGET_GENERIC_GETTY_BAUDRATE) $(TARGET_GENERIC_GETTY_TERM) #~' \
 		$(TARGET_DIR)/etc/inittab
 
 # Find commented line, if any, and remove leading '#'s
@@ -46,7 +47,7 @@  target-generic-do-remount-rw:
 target-generic-dont-remount-rw:
 	$(SED) '/^[^#].*# REMOUNT_ROOTFS_RW$$/s~^~#~' $(TARGET_DIR)/etc/inittab
 
-ifneq ($(TARGET_GENERIC_GETTY),)
+ifeq ($(BR2_TARGET_GENERIC_GETTY),y)
 TARGETS += target-generic-securetty
 endif
 
@@ -61,7 +62,7 @@  endif
 ifeq ($(BR2_ROOTFS_SKELETON_DEFAULT),y)
 TARGETS += target-root-passwd
 
-ifneq ($(TARGET_GENERIC_GETTY),)
+ifeq ($(BR2_TARGET_GENERIC_GETTY),y)
 TARGETS += target-generic-getty-$(if $(BR2_PACKAGE_SYSVINIT),sysvinit,busybox)
 endif