| Message ID | 20180708195429.9693-1-jeremy.rosen@smile.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | systemd: add an option to use busybox's getty | expand |
Hi Jeremy, Le 08/07/2018 à 21:54, Jérémy Rosen a écrit : > Currently, the patch to replace agetty with busybox's getty is always > applied. > > this patch adds a compile-time option to systemd to choose either agetty > or busybox's getty and chooses the correct option depending on agetty being > selected in busybox > As discussed privately, if you want an example on how apply a patch conditionally you can take a look at the gcc package [1]. This has been introduced by [2] but there is some other place where $(APPLY_PATCHES) is used (conditionally or unconditionally). $(APPLY_PATCHES) is defined in package/Makefile.in but this way to use it is not documented in the manual [3]. Best regards, Romain [1] https://git.busybox.net/buildroot/tree/package/gcc/gcc.mk#n41 [2] https://git.busybox.net/buildroot/commit/?id=197006a41c1a0450bf6350d5742e186b5b0c69de [3] http://nightly.buildroot.org/manual.html#patch-policy
On 09-07-18 10:02, Romain Naour wrote: > Hi Jeremy, > > Le 08/07/2018 à 21:54, Jérémy Rosen a écrit : >> Currently, the patch to replace agetty with busybox's getty is always >> applied. >> >> this patch adds a compile-time option to systemd to choose either agetty >> or busybox's getty and chooses the correct option depending on agetty being >> selected in busybox >> > > As discussed privately, if you want an example on how apply a patch > conditionally you can take a look at the gcc package [1]. I don't think we want to apply patches conditionally. In fact, Jérémy's change is much better, because it makes the patch upstreamable. That said, as discussed on IRC, a much easier approach would be to overwrite systemd's getty unit files with custom buildroot unit files in case UTIL_LINUX_AGETTY is not selected. Then the patch could be dropped entirely. Regards, Arnout > This has been introduced by [2] but there is some other place where > $(APPLY_PATCHES) is used (conditionally or unconditionally). > > $(APPLY_PATCHES) is defined in package/Makefile.in but this way to use it is not > documented in the manual [3]. > > Best regards, > Romain > > [1] https://git.busybox.net/buildroot/tree/package/gcc/gcc.mk#n41 > [2] > https://git.busybox.net/buildroot/commit/?id=197006a41c1a0450bf6350d5742e186b5b0c69de > [3] http://nightly.buildroot.org/manual.html#patch-policy > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot >
On 10/07/2018 00:21, Arnout Vandecappelle wrote: > > On 09-07-18 10:02, Romain Naour wrote: >> Hi Jeremy, >> >> Le 08/07/2018 à 21:54, Jérémy Rosen a écrit : >>> Currently, the patch to replace agetty with busybox's getty is always >>> applied. >>> >>> this patch adds a compile-time option to systemd to choose either agetty >>> or busybox's getty and chooses the correct option depending on agetty being >>> selected in busybox >>> >> As discussed privately, if you want an example on how apply a patch >> conditionally you can take a look at the gcc package [1]. > I don't think we want to apply patches conditionally. In fact, Jérémy's change > is much better, because it makes the patch upstreamable. systemd upstream is not interested in supporting busybox AFAIU... > That said, as discussed on IRC, a much easier approach would be to overwrite > systemd's getty unit files with custom buildroot unit files in case > UTIL_LINUX_AGETTY is not selected. Then the patch could be dropped entirely. The patch approch has the advantage of "breaking" if upstream changes, but I can do a followup in that direction, if that's what the project wants... > Regards, > Arnout > >> This has been introduced by [2] but there is some other place where >> $(APPLY_PATCHES) is used (conditionally or unconditionally). >> >> $(APPLY_PATCHES) is defined in package/Makefile.in but this way to use it is not >> documented in the manual [3]. >> >> Best regards, >> Romain >> >> [1] https://git.busybox.net/buildroot/tree/package/gcc/gcc.mk#n41 >> [2] >> https://git.busybox.net/buildroot/commit/?id=197006a41c1a0450bf6350d5742e186b5b0c69de >> [3] http://nightly.buildroot.org/manual.html#patch-policy >> _______________________________________________ >> buildroot mailing list >> buildroot@busybox.net >> http://lists.busybox.net/mailman/listinfo/buildroot >>
On 10-07-18 10:05, Jérémy Rosen wrote: > > > On 10/07/2018 00:21, Arnout Vandecappelle wrote: >> On 09-07-18 10:02, Romain Naour wrote: >>> Hi Jeremy, >>> >>> Le 08/07/2018 à 21:54, Jérémy Rosen a écrit : >>>> Currently, the patch to replace agetty with busybox's getty is always >>>> applied. >>>> >>>> this patch adds a compile-time option to systemd to choose either agetty >>>> or busybox's getty and chooses the correct option depending on agetty being >>>> selected in busybox >>>> >>> As discussed privately, if you want an example on how apply a patch >>> conditionally you can take a look at the gcc package [1]. >> I don't think we want to apply patches conditionally. In fact, Jérémy's change >> is much better, because it makes the patch upstreamable. > systemd upstream is not interested in supporting busybox AFAIU... Really? There are a few commits in systemd that mention support for busybox... I do believe that Denys (the busybox maintainer) at some point made some negative comments about adding support for systemd (I guess about communicating with journald). I don't remember ever seeing systemd people saying anything about busybox. But I'm not really following the systemd world. >> That said, as discussed on IRC, a much easier approach would be to overwrite >> systemd's getty unit files with custom buildroot unit files in case >> UTIL_LINUX_AGETTY is not selected. Then the patch could be dropped entirely. > The patch approch has the advantage of "breaking" if upstream changes, but I can > do a followup in that direction, if that's what the project wants... Good point. My first thought was: nothing important is ever going to change in those getty files. But then again, it has some things that might change, like dependencies and conditions. So yes, the patch is probably the better approach. Could you make an attempt at upstreaming it though? One comment on that patch, though: the changequote using a non-7-bit-ascii character is pretty ugly IMO, and also the characters you choose are not a typical open-close pair. Could you replace it with [] or {} or suchlike? Also, I think (but I'm no m4 expert) you can make it a bit more readable by doing: {m4_dnl # The '-o' option ... instead of {# The '-o' option ... Come to think of it, isn't it sufficient to just double-quote instead of doing changequote? So m4_ifdef(`ENABLE_BUSYBOX_GETTY', ``ExecStart=-/sbin/getty -L %I 115200 vt100'', ``m4_dnl # The '-o' option value tells agetty to replace 'login' arguments with an ... '')m4_dnl Regards, Arnout >> Regards, >> Arnout >> >>> This has been introduced by [2] but there is some other place where >>> $(APPLY_PATCHES) is used (conditionally or unconditionally). >>> >>> $(APPLY_PATCHES) is defined in package/Makefile.in but this way to use it is not >>> documented in the manual [3]. >>> >>> Best regards, >>> Romain >>> >>> [1] https://git.busybox.net/buildroot/tree/package/gcc/gcc.mk#n41 >>> [2] >>> https://git.busybox.net/buildroot/commit/?id=197006a41c1a0450bf6350d5742e186b5b0c69de >>> [3] http://nightly.buildroot.org/manual.html#patch-policy >>> _______________________________________________ >>> buildroot mailing list >>> buildroot@busybox.net >>> http://lists.busybox.net/mailman/listinfo/buildroot >>> > > -- > SMILE <http://www.smile.eu/> > > 20 rue des Jardins > 92600 Asnières-sur-Seine > > > *Jérémy ROSEN* > Architecte technique > Responsable de l'expertise Smile-ECS > > email jeremy.rosen@smile.fr <mailto:jeremy.rosen@smile.fr> > phone +33141402967 > url http://www.smile.eu > > Twitter <https://twitter.com/GroupeSmile> Facebook > <https://www.facebook.com/smileopensource> LinkedIn > <https://www.linkedin.com/company/smile> Github <https://github.com/Smile-SA> > > > Découvrez l’univers Smile, rendez-vous sur smile.eu > <http://smile.eu/?utm_source=signature&utm_medium=email&utm_campaign=signature> > > eco Pour la planète, n'imprimez ce mail que si c'est nécessaire
On 10/07/2018 11:12, Arnout Vandecappelle wrote: > > On 10-07-18 10:05, Jérémy Rosen wrote: >> >> On 10/07/2018 00:21, Arnout Vandecappelle wrote: >>> On 09-07-18 10:02, Romain Naour wrote: >>>> Hi Jeremy, >>>> >>>> Le 08/07/2018 à 21:54, Jérémy Rosen a écrit : >>>>> Currently, the patch to replace agetty with busybox's getty is always >>>>> applied. >>>>> >>>>> this patch adds a compile-time option to systemd to choose either agetty >>>>> or busybox's getty and chooses the correct option depending on agetty being >>>>> selected in busybox >>>>> >>>> As discussed privately, if you want an example on how apply a patch >>>> conditionally you can take a look at the gcc package [1]. >>> I don't think we want to apply patches conditionally. In fact, Jérémy's change >>> is much better, because it makes the patch upstreamable. >> systemd upstream is not interested in supporting busybox AFAIU... > Really? There are a few commits in systemd that mention support for busybox... > > I do believe that Denys (the busybox maintainer) at some point made some > negative comments about adding support for systemd (I guess about communicating > with journald). I don't remember ever seeing systemd people saying anything > about busybox. But I'm not really following the systemd world. > That was my understanding, but I can submit a PR, we'll see if they are interested... >>> That said, as discussed on IRC, a much easier approach would be to overwrite >>> systemd's getty unit files with custom buildroot unit files in case >>> UTIL_LINUX_AGETTY is not selected. Then the patch could be dropped entirely. >> The patch approch has the advantage of "breaking" if upstream changes, but I can >> do a followup in that direction, if that's what the project wants... > Good point. My first thought was: nothing important is ever going to change in > those getty files. But then again, it has some things that might change, like > dependencies and conditions. > > So yes, the patch is probably the better approach. Could you make an attempt at > upstreaming it though? will do (not right away, i'm away for a week) > > One comment on that patch, though: the changequote using a non-7-bit-ascii > character is pretty ugly IMO, and also the characters you choose are not a > typical open-close pair. Could you replace it with [] or {} or suchlike? I'll try to find a pair that is not used in the block I quote :) > > Also, I think (but I'm no m4 expert) you can make it a bit more readable by doing: > > {m4_dnl > # The '-o' option ... > > instead of > > {# The '-o' option ... > > Come to think of it, isn't it sufficient to just double-quote instead of doing > changequote? So > > m4_ifdef(`ENABLE_BUSYBOX_GETTY', > ``ExecStart=-/sbin/getty -L %I 115200 vt100'', > ``m4_dnl > # The '-o' option value tells agetty to replace 'login' arguments with an > ... > '')m4_dnl Ok, your m4-fu is way better than mine... i'll have a look at that too. Thx for the review Jérémy > > Regards, > Arnout > > >>> Regards, >>> Arnout >>> >>>> This has been introduced by [2] but there is some other place where >>>> $(APPLY_PATCHES) is used (conditionally or unconditionally). >>>> >>>> $(APPLY_PATCHES) is defined in package/Makefile.in but this way to use it is not >>>> documented in the manual [3]. >>>> >>>> Best regards, >>>> Romain >>>> >>>> [1] https://git.busybox.net/buildroot/tree/package/gcc/gcc.mk#n41 >>>> [2] >>>> https://git.busybox.net/buildroot/commit/?id=197006a41c1a0450bf6350d5742e186b5b0c69de >>>> [3] http://nightly.buildroot.org/manual.html#patch-policy >>>> _______________________________________________ >>>> buildroot mailing list >>>> buildroot@busybox.net >>>> http://lists.busybox.net/mailman/listinfo/buildroot >>>> >> -- >> SMILE <http://www.smile.eu/> >> >> 20 rue des Jardins >> 92600 Asnières-sur-Seine >> >> >> *Jérémy ROSEN* >> Architecte technique >> Responsable de l'expertise Smile-ECS >> >> email jeremy.rosen@smile.fr <mailto:jeremy.rosen@smile.fr> >> phone +33141402967 >> url http://www.smile.eu >> >> Twitter <https://twitter.com/GroupeSmile> Facebook >> <https://www.facebook.com/smileopensource> LinkedIn >> <https://www.linkedin.com/company/smile> Github <https://github.com/Smile-SA> >> >> >> Découvrez l’univers Smile, rendez-vous sur smile.eu >> <http://smile.eu/?utm_source=signature&utm_medium=email&utm_campaign=signature> >> >> eco Pour la planète, n'imprimez ce mail que si c'est nécessaire
diff --git a/package/systemd/0001-fix-getty-unit.patch b/package/systemd/0001-fix-getty-unit.patch index d546e1ab32..cdbcf38b93 100644 --- a/package/systemd/0001-fix-getty-unit.patch +++ b/package/systemd/0001-fix-getty-unit.patch @@ -1,4 +1,4 @@ -From 69e440f9b7a0e9a43ef582d4bb521722b448a7c2 Mon Sep 17 00:00:00 2001 +From 5f0534a9464717f1cde3f9c1c76f3bff6b9ae6a1 Mon Sep 17 00:00:00 2001 From: Maxime Ripard <maxime.ripard@free-electrons.com> Date: Mon, 31 Jul 2017 10:08:46 -0400 Subject: [PATCH] fix-getty-unit @@ -6,7 +6,7 @@ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit -Prefer getty to agetty in console setup systemd units +Add an option to select getty instead of agetty Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Signed-off-by: Eric Le Bihan <eric.le.bihan.dev@free.fr> @@ -14,68 +14,130 @@ Signed-off-by: Eric Le Bihan <eric.le.bihan.dev@free.fr> Signed-off-by: Adam Duskett <aduskett@gmail.com> [Jérémy: replace additional usage of agetty by getty.] Signed-off-by: Jérémy Rosen <jeremy.rosen@smile.fr> +[Jérémy: transform into a systemd compile-time option.] +Signed-off-by: Jérémy Rosen <jeremy.rosen@smile.fr> --- - units/console-getty.service.m4 | 2 +- - units/container-getty@.service.m4 | 2 +- - units/getty@.service.m4 | 5 +---- - units/serial-getty@.service.m4 | 2 +- - 4 files changed, 4 insertions(+), 7 deletions(-) + meson.build | 4 +++- + meson_options.txt | 3 +++ + units/console-getty.service.m4 | 7 +++++-- + units/container-getty@.service.m4 | 7 +++++-- + units/getty@.service.m4 | 7 +++++-- + units/serial-getty@.service.m4 | 7 +++++-- + 6 files changed, 26 insertions(+), 9 deletions(-) +diff --git a/meson.build b/meson.build +index 04331dd41..b95e74a54 100644 +--- a/meson.build ++++ b/meson.build +@@ -1221,7 +1221,8 @@ foreach term : ['utmp', + 'smack', + 'gshadow', + 'idn', +- 'nss-systemd'] ++ 'nss-systemd', ++ 'busybox-getty'] + have = get_option(term) + name = 'ENABLE_' + term.underscorify().to_upper() + conf.set10(name, have) +@@ -2985,6 +2986,7 @@ foreach tuple : [ + ['debug hashmap'], + ['debug mmap cache'], + ['valgrind', conf.get('VALGRIND') == 1], ++ ['busybox-getty'] + ] + + if tuple.length() >= 2 +diff --git a/meson_options.txt b/meson_options.txt +index 16c1f2b2f..8ccdd6c7d 100644 +--- a/meson_options.txt ++++ b/meson_options.txt +@@ -315,3 +315,6 @@ option('oss-fuzz', type : 'boolean', value : 'false', + description : 'build against oss-fuzz') + option('llvm-fuzz', type : 'boolean', value : 'false', + description : 'build against LLVM libFuzzer') ++ ++option('busybox-getty',type : 'boolean', value : 'false', ++ description : 'Use busybox implementation of getty') diff --git a/units/console-getty.service.m4 b/units/console-getty.service.m4 -index 3c553240a..fd5ad9456 100644 +index 3c553240a..7271336f1 100644 --- a/units/console-getty.service.m4 +++ b/units/console-getty.service.m4 -@@ -23,7 +23,7 @@ ConditionPathExists=/dev/console - # The '-o' option value tells agetty to replace 'login' arguments with an +@@ -20,10 +20,13 @@ Before=getty.target + ConditionPathExists=/dev/console + + [Service] +-# The '-o' option value tells agetty to replace 'login' arguments with an ++m4_changequote(£,@)m4_ifdef(£ENABLE_BUSYBOX_GETTY@, ++£ExecStart=-/sbin/getty -L %I 115200 vt100@, ++£# The '-o' option value tells agetty to replace 'login' arguments with an # option to preserve environment (-p), followed by '--' for safety, and then # the entered username. -ExecStart=-/sbin/agetty -o '-p -- \\u' --noclear --keep-baud console 115200,38400,9600 $TERM -+ExecStart=-/sbin/getty -L %I 115200 vt100 ++ExecStart=-/sbin/agetty -o '-p -- \\u' --noclear --keep-baud console 115200,38400,9600 $TERM@ ++)m4_changequote()m4_dnl Type=idle Restart=always UtmpIdentifier=cons diff --git a/units/container-getty@.service.m4 b/units/container-getty@.service.m4 -index 087ab7f9b..30f7b66fe 100644 +index 087ab7f9b..f534375bd 100644 --- a/units/container-getty@.service.m4 +++ b/units/container-getty@.service.m4 -@@ -28,7 +28,7 @@ Before=rescue.service - # The '-o' option value tells agetty to replace 'login' arguments with an +@@ -25,10 +25,13 @@ Conflicts=rescue.service + Before=rescue.service + + [Service] +-# The '-o' option value tells agetty to replace 'login' arguments with an ++m4_changequote(£,@)m4_ifdef(£ENABLE_BUSYBOX_GETTY@, ++£ExecStart=-/sbin/getty -L %I 115200 vt100@, ++£# The '-o' option value tells agetty to replace 'login' arguments with an # option to preserve environment (-p), followed by '--' for safety, and then # the entered username. -ExecStart=-/sbin/agetty -o '-p -- \\u' --noclear --keep-baud pts/%I 115200,38400,9600 $TERM -+ExecStart=-/sbin/getty -L %I 115200 vt100 ++ExecStart=-/sbin/agetty -o '-p -- \\u' --noclear --keep-baud pts/%I 115200,38400,9600 $TERM@ ++)m4_changequote()m4_dnl Type=idle Restart=always RestartSec=0 diff --git a/units/getty@.service.m4 b/units/getty@.service.m4 -index 80e793bb7..385758c61 100644 +index 80e793bb7..2370e94f3 100644 --- a/units/getty@.service.m4 +++ b/units/getty@.service.m4 -@@ -35,10 +35,7 @@ ConditionPathExists=/dev/tty0 +@@ -35,10 +35,13 @@ ConditionPathExists=/dev/tty0 [Service] # the VT is cleared by TTYVTDisallocate -# The '-o' option value tells agetty to replace 'login' arguments with an --# option to preserve environment (-p), followed by '--' for safety, and then --# the entered username. ++m4_changequote(£,@)m4_ifdef(£ENABLE_BUSYBOX_GETTY@, ++£ExecStart=-/sbin/getty -L %I 115200 vt100@, ++£# The '-o' option value tells agetty to replace 'login' arguments with an + # option to preserve environment (-p), followed by '--' for safety, and then + # the entered username. -ExecStart=-/sbin/agetty -o '-p -- \\u' --noclear %I $TERM -+ExecStart=-/sbin/getty -L %I 115200 vt100 ++ExecStart=-/sbin/agetty -o '-p -- \\u' --noclear %I $TERM@ ++)m4_changequote()m4_dnl Type=idle Restart=always RestartSec=0 diff --git a/units/serial-getty@.service.m4 b/units/serial-getty@.service.m4 -index 757b86ab2..3d60efdb6 100644 +index 757b86ab2..fea646f77 100644 --- a/units/serial-getty@.service.m4 +++ b/units/serial-getty@.service.m4 -@@ -33,7 +33,7 @@ Before=rescue.service - # The '-o' option value tells agetty to replace 'login' arguments with an +@@ -30,10 +30,13 @@ Conflicts=rescue.service + Before=rescue.service + + [Service] +-# The '-o' option value tells agetty to replace 'login' arguments with an ++m4_changequote(£,@)m4_ifdef(£ENABLE_BUSYBOX_GETTY@, ++£ExecStart=-/sbin/getty -L %I 115200 vt100@, ++£# The '-o' option value tells agetty to replace 'login' arguments with an # option to preserve environment (-p), followed by '--' for safety, and then # the entered username. -ExecStart=-/sbin/agetty -o '-p -- \\u' --keep-baud 115200,38400,9600 %I $TERM -+ExecStart=-/sbin/getty -L %I 115200 vt100 ++ExecStart=-/sbin/agetty -o '-p -- \\u' --keep-baud 115200,38400,9600 %I $TERM@ ++)m4_changequote()m4_dnl Type=idle Restart=always UtmpIdentifier=%I -- -2.14.4 +2.18.0 diff --git a/package/systemd/systemd.mk b/package/systemd/systemd.mk index d7031ed21d..e723a59f33 100644 --- a/package/systemd/systemd.mk +++ b/package/systemd/systemd.mk @@ -332,6 +332,12 @@ else SYSTEMD_CONF_OPTS += -Dhibernate=false endif +ifeq ($(BR2_PACKAGE_UTIL_LINUX_AGETTY),y) +SYSTEMD_CONF_OPTS += -Dbusybox-getty=false +else +SYSTEMD_CONF_OPTS += -Dbusybox-getty=true +endif + SYSTEMD_FALLBACK_HOSTNAME = $(call qstrip,$(BR2_TARGET_GENERIC_HOSTNAME)) ifneq ($(SYSTEMD_FALLBACK_HOSTNAME),) SYSTEMD_CONF_OPTS += -Dfallback-hostname=$(SYSTEMD_FALLBACK_HOSTNAME)
Currently, the patch to replace agetty with busybox's getty is always applied. this patch adds a compile-time option to systemd to choose either agetty or busybox's getty and chooses the correct option depending on agetty being selected in busybox Signed-off-by: Jérémy Rosen <jeremy.rosen@smile.fr> --- package/systemd/0001-fix-getty-unit.patch | 112 +++++++++++++++++----- package/systemd/systemd.mk | 6 ++ 2 files changed, 93 insertions(+), 25 deletions(-)