[Buildroot] [PATCH 1/2] package/dropbear: Respect user specific configurations

[Buildroot] [PATCH 1/2] package/dropbear: Respect user specific configurations

[Cyril Bur]

systemd .service file should respect /etc/default/dropbear
---
 package/dropbear/dropbear.service | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/package/dropbear/dropbear.service b/package/dropbear/dropbear.service
index 1eb42f9..66ec9cf 100644
--- a/package/dropbear/dropbear.service
+++ b/package/dropbear/dropbear.service
@@ -19,7 +19,8 @@ if [ -L /etc/dropbear \
         mkdir -p "$(readlink /etc/dropbear)"; \
     fi; \
 fi'
-ExecStart=/usr/sbin/dropbear -F -R
+EnvironmentFile=/etc/default/dropbear
+ExecStart=/usr/sbin/dropbear -F -R $DROPBEAR_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
 
 [Install]
-- 
2.6.2

[Buildroot] [PATCH 2/2] package/systemd: Respect BR2_TARGET_GENERIC_GETTY_BAUDRATE

[Cyril Bur]

Currently systemd getty services ignore baudrates set in buildroot in
favour of a hardcoded 115200. This patch SEDs out that hardcoded value with
what is selected.
---
 package/systemd/systemd.mk | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/package/systemd/systemd.mk b/package/systemd/systemd.mk
index b62fc08..d8a25ed 100644
--- a/package/systemd/systemd.mk
+++ b/package/systemd/systemd.mk
@@ -183,6 +183,7 @@ endef
 
 ifneq ($(call qstrip,$(BR2_TARGET_GENERIC_GETTY_PORT)),)
 # systemd needs getty.service for VTs and serial-getty.service for serial ttys
+# also patch the file to use the correct baud-rate, the default baudrate is 115200 so look for that
 define SYSTEMD_INSTALL_SERVICE_TTY
 	if echo $(BR2_TARGET_GENERIC_GETTY_PORT) | egrep -q 'tty[0-9]*$$'; \
 	then \
@@ -190,8 +191,12 @@ define SYSTEMD_INSTALL_SERVICE_TTY
 	else \
 		SERVICE="serial-getty"; \
 	fi; \
-	ln -fs ../../../../lib/systemd/system/$${SERVICE}@.service \
-		$(TARGET_DIR)/etc/systemd/system/getty.target.wants/$${SERVICE}@$(BR2_TARGET_GENERIC_GETTY_PORT).service
+	ln -fs $(TARGET_DIR)/lib/systemd/system/$${SERVICE}@.service \
+		$(TARGET_DIR)/etc/systemd/system/getty.target.wants/$${SERVICE}@$(BR2_TARGET_GENERIC_GETTY_PORT).service; \
+	if [ $(BR2_TARGET_GENERIC_GETTY_BAUDRATE) -gt 0 ] ; \
+	then \
+		$(SED) 's,115200,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE),' $(TARGET_DIR)/lib/systemd/system/$${SERVICE}@.service; \
+	fi
 endef
 endif
 
-- 
2.6.2

[Buildroot] [PATCH 2/2] package/systemd: Respect BR2_TARGET_GENERIC_GETTY_BAUDRATE

[Cyril Bur]

On Thu, 29 Oct 2015 17:11:42 +1100
Cyril Bur <cyrilbur@gmail.com> wrote:

> Currently systemd getty services ignore baudrates set in buildroot in
> favour of a hardcoded 115200. This patch SEDs out that hardcoded value with
> what is selected.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>

> ---
>  package/systemd/systemd.mk | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/package/systemd/systemd.mk b/package/systemd/systemd.mk
> index b62fc08..d8a25ed 100644
> --- a/package/systemd/systemd.mk
> +++ b/package/systemd/systemd.mk
> @@ -183,6 +183,7 @@ endef
>  
>  ifneq ($(call qstrip,$(BR2_TARGET_GENERIC_GETTY_PORT)),)
>  # systemd needs getty.service for VTs and serial-getty.service for serial ttys
> +# also patch the file to use the correct baud-rate, the default baudrate is 115200 so look for that
>  define SYSTEMD_INSTALL_SERVICE_TTY
>  	if echo $(BR2_TARGET_GENERIC_GETTY_PORT) | egrep -q 'tty[0-9]*$$'; \
>  	then \
> @@ -190,8 +191,12 @@ define SYSTEMD_INSTALL_SERVICE_TTY
>  	else \
>  		SERVICE="serial-getty"; \
>  	fi; \
> -	ln -fs ../../../../lib/systemd/system/$${SERVICE}@.service \
> -		$(TARGET_DIR)/etc/systemd/system/getty.target.wants/$${SERVICE}@$(BR2_TARGET_GENERIC_GETTY_PORT).service
> +	ln -fs $(TARGET_DIR)/lib/systemd/system/$${SERVICE}@.service \
> +		$(TARGET_DIR)/etc/systemd/system/getty.target.wants/$${SERVICE}@$(BR2_TARGET_GENERIC_GETTY_PORT).service; \
> +	if [ $(BR2_TARGET_GENERIC_GETTY_BAUDRATE) -gt 0 ] ; \
> +	then \
> +		$(SED) 's,115200,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE),' $(TARGET_DIR)/lib/systemd/system/$${SERVICE}@.service; \
> +	fi
>  endef
>  endif
>  

[Buildroot] [PATCH 2/2] package/systemd: Respect BR2_TARGET_GENERIC_GETTY_BAUDRATE

[Martin Bark]

Cyril, All,

Some comment inline below

On 29 October 2015 at 06:17, Cyril Bur <cyrilbur@gmail.com> wrote:
> On Thu, 29 Oct 2015 17:11:42 +1100
> Cyril Bur <cyrilbur@gmail.com> wrote:
>
>> Currently systemd getty services ignore baudrates set in buildroot in
>> favour of a hardcoded 115200. This patch SEDs out that hardcoded value with
>> what is selected.
>
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
>
>> ---
>>  package/systemd/systemd.mk | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/package/systemd/systemd.mk b/package/systemd/systemd.mk
>> index b62fc08..d8a25ed 100644
>> --- a/package/systemd/systemd.mk
>> +++ b/package/systemd/systemd.mk
>> @@ -183,6 +183,7 @@ endef
>>
>>  ifneq ($(call qstrip,$(BR2_TARGET_GENERIC_GETTY_PORT)),)
>>  # systemd needs getty.service for VTs and serial-getty.service for serial ttys
>> +# also patch the file to use the correct baud-rate, the default baudrate is 115200 so look for that
>>  define SYSTEMD_INSTALL_SERVICE_TTY
>>       if echo $(BR2_TARGET_GENERIC_GETTY_PORT) | egrep -q 'tty[0-9]*$$'; \
>>       then \
>> @@ -190,8 +191,12 @@ define SYSTEMD_INSTALL_SERVICE_TTY
>>       else \
>>               SERVICE="serial-getty"; \
>>       fi; \
>> -     ln -fs ../../../../lib/systemd/system/$${SERVICE}@.service \
>> -             $(TARGET_DIR)/etc/systemd/system/getty.target.wants/$${SERVICE}@$(BR2_TARGET_GENERIC_GETTY_PORT).service
>> +     ln -fs $(TARGET_DIR)/lib/systemd/system/$${SERVICE}@.service \
>> +             $(TARGET_DIR)/etc/systemd/system/getty.target.wants/$${SERVICE}@$(BR2_TARGET_GENERIC_GETTY_PORT).service; \

Changing the symlink above is not necessary for this patch.  Also i
think the the change is wrong and will cause the service to symlink an
invalid file when run on the target.

>> +     if [ $(BR2_TARGET_GENERIC_GETTY_BAUDRATE) -gt 0 ] ; \
>> +     then \
>> +             $(SED) 's,115200,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE),' $(TARGET_DIR)/lib/systemd/system/$${SERVICE}@.service; \
>> +     fi

$(BR2_TARGET_GENERIC_GETTY_BAUDRATE) will have double quotes around it
which will need to be removed so i don't think the -gt test will ever
work.  Have a look in package/skeleton/skeleton.mk where it uses
$(call qstrip,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE)) to strip the
double quotes before it uses the value, you'll need to do something
similar.

Thanks

Martin

>>  endef
>>  endif
>>
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

[Buildroot] [PATCH 2/2] package/systemd: Respect BR2_TARGET_GENERIC_GETTY_BAUDRATE

[Maxime Hadjinlian]

Hi Cyril, Martin, all

On Thu, Oct 29, 2015 at 11:41 AM, Martin Bark <martin@barkynet.com> wrote:

> Cyril, All,
>
> Some comment inline below
>
> On 29 October 2015 at 06:17, Cyril Bur <cyrilbur@gmail.com> wrote:
> > On Thu, 29 Oct 2015 17:11:42 +1100
> > Cyril Bur <cyrilbur@gmail.com> wrote:
> >
> >> Currently systemd getty services ignore baudrates set in buildroot in
> >> favour of a hardcoded 115200. This patch SEDs out that hardcoded value
> with
> >> what is selected.
> >
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> >
> >> ---
> >>  package/systemd/systemd.mk | 9 +++++++--
> >>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/package/systemd/systemd.mk b/package/systemd/systemd.mk
> >> index b62fc08..d8a25ed 100644
> >> --- a/package/systemd/systemd.mk
> >> +++ b/package/systemd/systemd.mk
> >> @@ -183,6 +183,7 @@ endef
> >>
> >>  ifneq ($(call qstrip,$(BR2_TARGET_GENERIC_GETTY_PORT)),)
> >>  # systemd needs getty.service for VTs and serial-getty.service for
> serial ttys
> >> +# also patch the file to use the correct baud-rate, the default
> baudrate is 115200 so look for that
> >>  define SYSTEMD_INSTALL_SERVICE_TTY
> >>       if echo $(BR2_TARGET_GENERIC_GETTY_PORT) | egrep -q
> 'tty[0-9]*$$'; \
> >>       then \
> >> @@ -190,8 +191,12 @@ define SYSTEMD_INSTALL_SERVICE_TTY
> >>       else \
> >>               SERVICE="serial-getty"; \
> >>       fi; \
> >> -     ln -fs ../../../../lib/systemd/system/$${SERVICE}@.service \
> >> -
>  $(TARGET_DIR)/etc/systemd/system/getty.target.wants/$${SERVICE}@
> $(BR2_TARGET_GENERIC_GETTY_PORT).service
> >> +     ln -fs $(TARGET_DIR)/lib/systemd/system/$${SERVICE}@.service \
> >> +
>  $(TARGET_DIR)/etc/systemd/system/getty.target.wants/$${SERVICE}@$(BR2_TARGET_GENERIC_GETTY_PORT).service;
> \
>
> Changing the symlink above is not necessary for this patch.  Also i
> think the the change is wrong and will cause the service to symlink an
> invalid file when run on the target.
>
Indeed, it's wrong, but not because it'll be wrong on the target itself,
but in the target directory. The relative path allow you to use the symlink
even in your target directory (otherwise you my stumble upon the files on
your hosts and that would start misunderstanding and errors.).

>
> >> +     if [ $(BR2_TARGET_GENERIC_GETTY_BAUDRATE) -gt 0 ] ; \
> >> +     then \
> >> +             $(SED) 's,115200,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE),'
> $(TARGET_DIR)/lib/systemd/system/$${SERVICE}@.service; \
> >> +     fi
>
> $(BR2_TARGET_GENERIC_GETTY_BAUDRATE) will have double quotes around it
> which will need to be removed so i don't think the -gt test will ever
> work.  Have a look in package/skeleton/skeleton.mk where it uses
> $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE)) to strip the
> double quotes before it uses the value, you'll need to do something
> similar.
>
> Thanks
>
> Martin
>
> >>  endef
> >>  endif
> >>
> >
> > _______________________________________________
> > buildroot mailing list
> > buildroot at busybox.net
> > http://lists.busybox.net/mailman/listinfo/buildroot
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20151104/43f883a2/attachment.html>

[Buildroot] [PATCH 2/2] package/systemd: Respect BR2_TARGET_GENERIC_GETTY_BAUDRATE

[Cyril Bur]

On Wed, 4 Nov 2015 10:22:51 +0100
Maxime Hadjinlian <maxime.hadjinlian@gmail.com> wrote:

> Hi Cyril, Martin, all
> 
> On Thu, Oct 29, 2015 at 11:41 AM, Martin Bark <martin@barkynet.com> wrote:
> 
> > Cyril, All,
> >
> > Some comment inline below
> >
> > On 29 October 2015 at 06:17, Cyril Bur <cyrilbur@gmail.com> wrote:  
> > > On Thu, 29 Oct 2015 17:11:42 +1100
> > > Cyril Bur <cyrilbur@gmail.com> wrote:
> > >  
> > >> Currently systemd getty services ignore baudrates set in buildroot in
> > >> favour of a hardcoded 115200. This patch SEDs out that hardcoded value  
> > with  
> > >> what is selected.  
> > >
> > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > >  
> > >> ---
> > >>  package/systemd/systemd.mk | 9 +++++++--
> > >>  1 file changed, 7 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/package/systemd/systemd.mk b/package/systemd/systemd.mk
> > >> index b62fc08..d8a25ed 100644
> > >> --- a/package/systemd/systemd.mk
> > >> +++ b/package/systemd/systemd.mk
> > >> @@ -183,6 +183,7 @@ endef
> > >>
> > >>  ifneq ($(call qstrip,$(BR2_TARGET_GENERIC_GETTY_PORT)),)
> > >>  # systemd needs getty.service for VTs and serial-getty.service for  
> > serial ttys  
> > >> +# also patch the file to use the correct baud-rate, the default  
> > baudrate is 115200 so look for that  
> > >>  define SYSTEMD_INSTALL_SERVICE_TTY
> > >>       if echo $(BR2_TARGET_GENERIC_GETTY_PORT) | egrep -q  
> > 'tty[0-9]*$$'; \  
> > >>       then \
> > >> @@ -190,8 +191,12 @@ define SYSTEMD_INSTALL_SERVICE_TTY
> > >>       else \
> > >>               SERVICE="serial-getty"; \
> > >>       fi; \
> > >> -     ln -fs ../../../../lib/systemd/system/$${SERVICE}@.service \
> > >> -  
> >  $(TARGET_DIR)/etc/systemd/system/getty.target.wants/$${SERVICE}@
> > $(BR2_TARGET_GENERIC_GETTY_PORT).service  
> > >> +     ln -fs $(TARGET_DIR)/lib/systemd/system/$${SERVICE}@.service \
> > >> +  
> >  $(TARGET_DIR)/etc/systemd/system/getty.target.wants/$${SERVICE}@$(BR2_TARGET_GENERIC_GETTY_PORT).service;
> > \
> >
> > Changing the symlink above is not necessary for this patch.  Also i
> > think the the change is wrong and will cause the service to symlink an
> > invalid file when run on the target.
> >  
> Indeed, it's wrong, but not because it'll be wrong on the target itself,
> but in the target directory. The relative path allow you to use the symlink
> even in your target directory (otherwise you my stumble upon the files on
> your hosts and that would start misunderstanding and errors.).
> 

Ah yes, I'm not sure why I did this anymore, I do agree, I'll resend without
this hunk.

> >  
> > >> +     if [ $(BR2_TARGET_GENERIC_GETTY_BAUDRATE) -gt 0 ] ; \
> > >> +     then \
> > >> +             $(SED) 's,115200,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE),'  
> > $(TARGET_DIR)/lib/systemd/system/$${SERVICE}@.service; \  
> > >> +     fi  
> >
> > $(BR2_TARGET_GENERIC_GETTY_BAUDRATE) will have double quotes around it
> > which will need to be removed so i don't think the -gt test will ever
> > work.  Have a look in package/skeleton/skeleton.mk where it uses
> > $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE)) to strip the
> > double quotes before it uses the value, you'll need to do something
> > similar.
> >

I believe it will work, Nicolas Cavallari provided a good explanation as to
why:

This test will work just fine, it will be expanded to e.g.

if [ "38400" -gt 0 ];

Which is a perfectly valid shell condition.  However, the sed will
introduce the double quotes in the systemd unit file.  Which,
according to the systemd documentation, is also fine in an ExecStart
statement, which somewhat mimic the behavior of the shell.

 
> > Thanks
> >
> > Martin
> >  
> > >>  endef
> > >>  endif
> > >>  
> > >
> > > _______________________________________________
> > > buildroot mailing list
> > > buildroot at busybox.net
> > > http://lists.busybox.net/mailman/listinfo/buildroot  
> > _______________________________________________
> > buildroot mailing list
> > buildroot at busybox.net
> > http://lists.busybox.net/mailman/listinfo/buildroot
> >  

[Buildroot] [PATCH 2/2] package/systemd: Respect BR2_TARGET_GENERIC_GETTY_BAUDRATE

[Arnout Vandecappelle]

On 13-11-15 02:08, Cyril Bur wrote:
> On Wed, 4 Nov 2015 10:22:51 +0100
> Maxime Hadjinlian <maxime.hadjinlian@gmail.com> wrote:
> 
>> Hi Cyril, Martin, all
>>
>> On Thu, Oct 29, 2015 at 11:41 AM, Martin Bark <martin@barkynet.com> wrote:
>>
>>> Cyril, All,
>>>
>>> Some comment inline below
>>>
>>> On 29 October 2015 at 06:17, Cyril Bur <cyrilbur@gmail.com> wrote:  
>>>> On Thu, 29 Oct 2015 17:11:42 +1100
>>>> Cyril Bur <cyrilbur@gmail.com> wrote:
[snip]
>>>>> +     if [ $(BR2_TARGET_GENERIC_GETTY_BAUDRATE) -gt 0 ] ; \
>>>>> +     then \
>>>>> +             $(SED) 's,115200,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE),'  
>>> $(TARGET_DIR)/lib/systemd/system/$${SERVICE}@.service; \  
>>>>> +     fi  
>>>
>>> $(BR2_TARGET_GENERIC_GETTY_BAUDRATE) will have double quotes around it
>>> which will need to be removed so i don't think the -gt test will ever
>>> work.  Have a look in package/skeleton/skeleton.mk where it uses
>>> $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE)) to strip the
>>> double quotes before it uses the value, you'll need to do something
>>> similar.
>>>
> 
> I believe it will work, Nicolas Cavallari provided a good explanation as to
> why:
> 
> This test will work just fine, it will be expanded to e.g.
> 
> if [ "38400" -gt 0 ];
> 
> Which is a perfectly valid shell condition.  However, the sed will
> introduce the double quotes in the systemd unit file.  Which,
> according to the systemd documentation, is also fine in an ExecStart
> statement, which somewhat mimic the behavior of the shell.

 Even though it works without the qstrip, I would prefer to use qstrip after all
to make things consistent.


 Ideally, by the way, I would like the getty handling to be done together for
systemd and old init. But unfortunately, moving it to skeleton is not going to
work since this patch requires systemd to be installed and systemd requires
skeleton to be installed. So we'll have to keep it split up like this until we
find a better solution.


 Regards,
 Arnout

[snip]


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 2/2] package/systemd: Respect BR2_TARGET_GENERIC_GETTY_BAUDRATE

[Nicolas Cavallari]

On 29/10/2015 11:41, Martin Bark wrote:
>>> +     if [ $(BR2_TARGET_GENERIC_GETTY_BAUDRATE) -gt 0 ] ; \
>>> +     then \
>>> +             $(SED) 's,115200,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE),' $(TARGET_DIR)/lib/systemd/system/$${SERVICE}@.service; \
>>> +     fi
> 
> $(BR2_TARGET_GENERIC_GETTY_BAUDRATE) will have double quotes around it
> which will need to be removed so i don't think the -gt test will ever
> work.  Have a look in package/skeleton/skeleton.mk where it uses
> $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_BAUDRATE)) to strip the
> double quotes before it uses the value, you'll need to do something
> similar.

This test will work just fine, it will be expanded to e.g.

if [ "38400" -gt 0 ];

Which is a perfectly valid shell condition.  However, the sed will
introduce the double quotes in the systemd unit file.  Which,
according to the systemd documentation, is also fine in an ExecStart
statement, which somewhat mimic the behavior of the shell.

[Buildroot] [PATCH 2/2] package/systemd: Respect BR2_TARGET_GENERIC_GETTY_BAUDRATE

[Thomas Petazzoni]

Dear Cyril Bur,

On Thu, 29 Oct 2015 17:11:42 +1100, Cyril Bur wrote:
> Currently systemd getty services ignore baudrates set in buildroot in
> favour of a hardcoded 115200. This patch SEDs out that hardcoded value with
> what is selected.
> ---
>  package/systemd/systemd.mk | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Both Martin Bark and Maxime Hadjinlian raised issues about your
patches. Therefore, I have for now marked your patch as "Rejected" in
our patch tracking system. However, if you don't agree with what Martin
and Maxime said, do not hesitate to reply to them, find a solution
that works fine for everyone and post an updated patch.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 2/2] package/systemd: Respect BR2_TARGET_GENERIC_GETTY_BAUDRATE

[Cyril Bur]

On Thu, 5 Nov 2015 13:32:18 +0100
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Dear Cyril Bur,
> 
> On Thu, 29 Oct 2015 17:11:42 +1100, Cyril Bur wrote:
> > Currently systemd getty services ignore baudrates set in buildroot in
> > favour of a hardcoded 115200. This patch SEDs out that hardcoded value with
> > what is selected.
> > ---
> >  package/systemd/systemd.mk | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)  
> 
> Both Martin Bark and Maxime Hadjinlian raised issues about your
> patches. Therefore, I have for now marked your patch as "Rejected" in
> our patch tracking system. However, if you don't agree with what Martin
> and Maxime said, do not hesitate to reply to them, find a solution
> that works fine for everyone and post an updated patch.
> 

Hi Thomas,

Thanks for taking the time to respond, these emails got lost somewhere,
somehow. Apologies!

The patches did work for me but one specific case means nothing. I'll
incorporate feedback and send you an updated version.

Cyril

> Thanks!
> 
> Thomas

[Buildroot] [PATCH 1/2] package/dropbear: Respect user specific configurations

[Cyril Bur]

On Thu, 29 Oct 2015 17:11:41 +1100
Cyril Bur <cyrilbur@gmail.com> wrote:

Apologies, missing sign off, don't try to rush patches out!

> systemd .service file should respect /etc/default/dropbear

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>

> ---
>  package/dropbear/dropbear.service | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/package/dropbear/dropbear.service b/package/dropbear/dropbear.service
> index 1eb42f9..66ec9cf 100644
> --- a/package/dropbear/dropbear.service
> +++ b/package/dropbear/dropbear.service
> @@ -19,7 +19,8 @@ if [ -L /etc/dropbear \
>          mkdir -p "$(readlink /etc/dropbear)"; \
>      fi; \
>  fi'
> -ExecStart=/usr/sbin/dropbear -F -R
> +EnvironmentFile=/etc/default/dropbear
> +ExecStart=/usr/sbin/dropbear -F -R $DROPBEAR_ARGS
>  ExecReload=/bin/kill -HUP $MAINPID
>  
>  [Install]

[Buildroot] [PATCH 1/2] package/dropbear: Respect user specific configurations

[Thomas Petazzoni]

Dear Cyril Bur,

On Thu, 29 Oct 2015 17:11:41 +1100, Cyril Bur wrote:
> systemd .service file should respect /etc/default/dropbear
> ---
>  package/dropbear/dropbear.service | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Thanks, applied.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/2] package/dropbear: Respect user specific configurations

[Gabe Evans]

Hi Cyril, Thomas,

This patch results in dropbear being broken out-of-the-box for systemd
users. The EnvironmentFile= line sets a requirement that
/etc/default/dropbear exist (it doesn't, by default).

I'm not so sure having an environment file by default is a good practice
for systemd units. The general consensus is that things effecting the
execution of a service should always be found in the appropriate service
unit. Environment= would be better anyway since there's only one variable
($DROPBEAR_ARGS) being used in this particular case.

An even better solution would be to move the dropbear.service symlink from
/etc/systemd/system/multi-user.target.wants/ to
/usr/lib/systemd/system/multi-user.target.wants/. That would allow users to
shadow the unit completely with their own customizations or override
individual options through the use of "drop-in" units. The manual explains
this in more detail:
http://www.freedesktop.org/software/systemd/man/systemd.unit.html#id-1.10.3

Thanks,
Gabe

On Mon, Nov 2, 2015 at 1:43 PM, Thomas Petazzoni <
thomas.petazzoni@free-electrons.com> wrote:

> Dear Cyril Bur,
>
> On Thu, 29 Oct 2015 17:11:41 +1100, Cyril Bur wrote:
> > systemd .service file should respect /etc/default/dropbear
> > ---
> >  package/dropbear/dropbear.service | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
>
> Thanks, applied.
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
>



-- 
Gabe Evans | Co-Founder & CTO
hashrabbit.co ? angel.co/hashrabbit ? github.com/gevans
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20151103/c94dba14/attachment.html>

[Buildroot] [PATCH 1/2] package/dropbear: Respect user specific configurations

[Thomas Petazzoni]

Dear Gabe Evans,

On Tue, 3 Nov 2015 13:17:05 -0800, Gabe Evans wrote:

> This patch results in dropbear being broken out-of-the-box for systemd
> users. The EnvironmentFile= line sets a requirement that
> /etc/default/dropbear exist (it doesn't, by default).
> 
> I'm not so sure having an environment file by default is a good practice
> for systemd units. The general consensus is that things effecting the
> execution of a service should always be found in the appropriate service
> unit. Environment= would be better anyway since there's only one variable
> ($DROPBEAR_ARGS) being used in this particular case.

Right. I'll revert the patch from Cyril then, unless he suggests a
better solution. I really don't know much about systemd, so I can only
rely on people reviewing patches touching systemd topics. If no-one
does, all I can do is a guess that the change looks somewhat reasonable.

So your report is definitely useful. But if I may suggest, it would be
even more useful if you could review systemd related patches before
they are applied.

> An even better solution would be to move the dropbear.service symlink from
> /etc/systemd/system/multi-user.target.wants/ to
> /usr/lib/systemd/system/multi-user.target.wants/. That would allow users to
> shadow the unit completely with their own customizations or override
> individual options through the use of "drop-in" units. The manual explains
> this in more detail:
> http://www.freedesktop.org/software/systemd/man/systemd.unit.html#id-1.10.3

*All* our packages are creating the symlink
in /etc/systemd/system/multi-user.target.wants/, so changing that
really needs to be discussed with the other people interested in
systemd support in Buildroot. I believe we used to have some symlinks
in /usr, some in /etc, and we settled on /etc with some
${justification}.

Maxime ?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/2] package/dropbear: Respect user specific configurations

[Gabe Evans]

Hi Thomas,

On Tue, Nov 3, 2015 at 1:53 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> So your report is definitely useful. But if I may suggest, it would be
> even more useful if you could review systemd related patches before
> they are applied.

This one slipped past me but I'd be more than happy to review patches
in the future.

>> An even better solution would be to move the dropbear.service symlink from
>> /etc/systemd/system/multi-user.target.wants/ to
>> /usr/lib/systemd/system/multi-user.target.wants/. That would allow users to
>> shadow the unit completely with their own customizations or override
>> individual options through the use of "drop-in" units. The manual explains
>> this in more detail:
>> http://www.freedesktop.org/software/systemd/man/systemd.unit.html#id-1.10.3
>
> *All* our packages are creating the symlink
> in /etc/systemd/system/multi-user.target.wants/, so changing that
> really needs to be discussed with the other people interested in
> systemd support in Buildroot. I believe we used to have some symlinks
> in /usr, some in /etc, and we settled on /etc with some
> ${justification}.

It might also be worth clarifying that other Linux distributions
follow the practice of reserving /etc/systemd for users and
/usr/lib/systemd for maintainers.

This is definitely a big change that should be discussed in more
detail. I'm still getting familiar with the mailing list so your help
in starting this conversation would be appreciated. :)

Thanks,
Gabe

-- 
Gabe Evans | Co-Founder & CTO
hashrabbit.co ? angel.co/hashrabbit ? github.com/gevans

[Buildroot] [PATCH 1/2] package/dropbear: Respect user specific configurations

[Maxime Hadjinlian]

Hi Gabe, Thomas, all,

On Tue, Nov 3, 2015 at 11:21 PM, Gabe Evans <gabe@hashrabbit.co> wrote:

> Hi Thomas,
>
> On Tue, Nov 3, 2015 at 1:53 PM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > So your report is definitely useful. But if I may suggest, it would be
> > even more useful if you could review systemd related patches before
> > they are applied.
>
> This one slipped past me but I'd be more than happy to review patches
> in the future.
>
> >> An even better solution would be to move the dropbear.service symlink
> from
> >> /etc/systemd/system/multi-user.target.wants/ to
> >> /usr/lib/systemd/system/multi-user.target.wants/. That would allow
> users to
> >> shadow the unit completely with their own customizations or override
> >> individual options through the use of "drop-in" units. The manual
> explains
> >> this in more detail:
> >>
> http://www.freedesktop.org/software/systemd/man/systemd.unit.html#id-1.10.3
> >
> > *All* our packages are creating the symlink
> > in /etc/systemd/system/multi-user.target.wants/, so changing that
> > really needs to be discussed with the other people interested in
> > systemd support in Buildroot. I believe we used to have some symlinks
> > in /usr, some in /etc, and we settled on /etc with some
> > ${justification}.
>
> It might also be worth clarifying that other Linux distributions
> follow the practice of reserving /etc/systemd for users and
> /usr/lib/systemd for maintainers.
>

We organized the files the way they are after what Debian did. They install
everything in '/usr/lib/systemd/system' and simply create symlinks in
'/etc/systemd/system/....', the way systemctl enable/disable does it (with
regards of the targets). It's also easy to test/tweak or the target without
loosing the original file.

Regarding the user replacing the unit files, we strongly suggest that
people that wants to customize anything to their needs, use a
post-build/post-image scripts.
We don't really provide an image as a 'vendor' would do, that you would
customize after. You build your own image, so you can customize it the way
you want it to at build time.

And if you want to customize only a little, the EnvironmentFile is there to
do that (that's the way I see it at least).

The simpler Environment is tricker to use in Buildroot, to customize the
env for a specific process, you would have to tweak systemd's configuration
file so it would evaluate a file containing all the env variable you would
use. What happens if there's collisions in the name ?
Environment is good on a distro, if you quickly want to hack something.

'EnvironmentFile' is the way to provide users with a way to customize the
process (or using the templates system, but that would requires more effort
to maintain, again since the file is used by both init, it's pretty
straightforward).
But since you don't want a process to stop functioning because it's lacking
options (since they are optionnal, it must work without them), using the
'=-' mechanisms is kind of mandatory for us I would say.

So I kind of like the way things are right now but if you see something
that doesn't sit right with you, let's discuss it :)

>
> This is definitely a big change that should be discussed in more
> detail. I'm still getting familiar with the mailing list so your help
> in starting this conversation would be appreciated. :)
>
> Thanks,
> Gabe
>
> --
> Gabe Evans | Co-Founder & CTO
> hashrabbit.co ? angel.co/hashrabbit ? github.com/gevans
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20151104/77edc9f6/attachment.html>

[Buildroot] [PATCH 1/2] package/dropbear: Respect user specific configurations

[Gabe Evans]

Hi Maxime, Thomas, all,

On Wed, Nov 4, 2015 at 2:46 AM, Maxime Hadjinlian
<maxime.hadjinlian@gmail.com> wrote:
> We organized the files the way they are after what Debian did. They install
> everything in '/usr/lib/systemd/system' and simply create symlinks in
> '/etc/systemd/system/....', the way systemctl enable/disable does it (with
> regards of the targets). It's also easy to test/tweak or the target without
> loosing the original file.
>
> Regarding the user replacing the unit files, we strongly suggest that people
> that wants to customize anything to their needs, use a post-build/post-image
> scripts.
> We don't really provide an image as a 'vendor' would do, that you would
> customize after. You build your own image, so you can customize it the way
> you want it to at build time.

My reasoning for linking services to targets in
'/usr/lib/systemd/system' is more for endusers than it is for
developers using Buildroot. Also, being able to link to
'../something.service' instead of
'../../../../usr/lib/systemd/system/something.service' leaves less
room for error. In the long run it offers more flexibility to users
who may not have the access or ability to make modifications to the
build system. Even though the project isn't much of a 'vendor' the
developers using it are in most cases.

> And if you want to customize only a little, the EnvironmentFile is there to
> do that (that's the way I see it at least).
>
> The simpler Environment is tricker to use in Buildroot, to customize the env
> for a specific process, you would have to tweak systemd's configuration file
> so it would evaluate a file containing all the env variable you would use.
> What happens if there's collisions in the name ?
> Environment is good on a distro, if you quickly want to hack something.
>
> 'EnvironmentFile' is the way to provide users with a way to customize the
> process (or using the templates system, but that would requires more effort
> to maintain, again since the file is used by both init, it's pretty
> straightforward).
> But since you don't want a process to stop functioning because it's lacking
> options (since they are optionnal, it must work without them), using the
> '=-' mechanisms is kind of mandatory for us I would say.

I completely agree with you here. My original suggestion of using
'Environment=' doesn't work as effectively as 'EnvironmentFile=-' if
the only way to change a unit is to modify or replace the original
file.

> So I kind of like the way things are right now but if you see something that
> doesn't sit right with you, let's discuss it :)

My only other concern would be making sure other packages follow the
pattern of having an optional environment file in a common location
and use common arguments. In this particular case, additional
arguments are specified as '$DROPBEAR_ARGS' meanwhile, the other
proposed patch for rng-tools names this variable '$DAEMON_ARGS'.

That aside, I'm okay with this solution given your reasoning. :)

Thanks,
Gabe

-- 
Gabe Evans | Co-Founder & CTO
hashrabbit.co ? angel.co/hashrabbit ? github.com/gevans

[Buildroot] [PATCH 1/2] package/dropbear: Respect user specific configurations

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

 > Dear Gabe Evans,
 > On Tue, 3 Nov 2015 13:17:05 -0800, Gabe Evans wrote:

 >> This patch results in dropbear being broken out-of-the-box for systemd
 >> users. The EnvironmentFile= line sets a requirement that
 >> /etc/default/dropbear exist (it doesn't, by default).
 >> 
 >> I'm not so sure having an environment file by default is a good practice
 >> for systemd units. The general consensus is that things effecting the
 >> execution of a service should always be found in the appropriate service
 >> unit. Environment= would be better anyway since there's only one variable
 >> ($DROPBEAR_ARGS) being used in this particular case.

 > Right. I'll revert the patch from Cyril then, unless he suggests a
 > better solution. I really don't know much about systemd, so I can only
 > rely on people reviewing patches touching systemd topics. If no-one
 > does, all I can do is a guess that the change looks somewhat reasonable.

I have instead changed it to use =- as suggested by Maxime and Steven
and fixed a similar issue with out dhcp package.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/2] package/dropbear: Respect user specific configurations

[Steven Noonan]

On Tue, Nov 3, 2015 at 1:17 PM, Gabe Evans <gabe@hashrabbit.co> wrote:
> Hi Cyril, Thomas,
>
> This patch results in dropbear being broken out-of-the-box for systemd
> users. The EnvironmentFile= line sets a requirement that
> /etc/default/dropbear exist (it doesn't, by default).

Make it "EnvironmentFile=-/some/path" (note the dash) and it will
ignore cases where the environment file is not present.

> I'm not so sure having an environment file by default is a good practice for
> systemd units. The general consensus is that things effecting the execution
> of a service should always be found in the appropriate service unit.
> Environment= would be better anyway since there's only one variable
> ($DROPBEAR_ARGS) being used in this particular case.
>
> An even better solution would be to move the dropbear.service symlink from
> /etc/systemd/system/multi-user.target.wants/ to
> /usr/lib/systemd/system/multi-user.target.wants/. That would allow users to
> shadow the unit completely with their own customizations or override
> individual options through the use of "drop-in" units. The manual explains
> this in more detail:
> http://www.freedesktop.org/software/systemd/man/systemd.unit.html#id-1.10.3
>
> Thanks,
> Gabe
>
> On Mon, Nov 2, 2015 at 1:43 PM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
>>
>> Dear Cyril Bur,
>>
>> On Thu, 29 Oct 2015 17:11:41 +1100, Cyril Bur wrote:
>> > systemd .service file should respect /etc/default/dropbear
>> > ---
>> >  package/dropbear/dropbear.service | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> Thanks, applied.
>>
>> Thomas
>> --
>> Thomas Petazzoni, CTO, Free Electrons
>> Embedded Linux, Kernel and Android engineering
>> http://free-electrons.com
>> _______________________________________________
>> buildroot mailing list
>> buildroot at busybox.net
>> http://lists.busybox.net/mailman/listinfo/buildroot
>
>
>
>
> --
> Gabe Evans | Co-Founder & CTO
> hashrabbit.co ? angel.co/hashrabbit ? github.com/gevans
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot