diff mbox

[2/2] package/systemd: Respect BR2_TARGET_GENERIC_GETTY_BAUDRATE

Message ID 1446099102-5205-2-git-send-email-cyrilbur@gmail.com
State Rejected
Headers show

Commit Message

Cyril Bur Oct. 29, 2015, 6:11 a.m. UTC 
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(-)

Comments

Cyril Bur Oct. 29, 2015, 6:17 a.m. UTC | #1 
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
>
Martin Bark Oct. 29, 2015, 10:41 a.m. UTC | #2 
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@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Maxime Hadjinlian Nov. 4, 2015, 9:22 a.m. UTC | #3 
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@busybox.net
> > http://lists.busybox.net/mailman/listinfo/buildroot
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
>
Thomas Petazzoni Nov. 5, 2015, 12:32 p.m. UTC | #4 
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
Nicolas Cavallari Nov. 6, 2015, 8:41 a.m. UTC | #5 
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.
Cyril Bur Nov. 13, 2015, 12:57 a.m. UTC | #6 
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
Cyril Bur Nov. 13, 2015, 1:08 a.m. UTC | #7 
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@busybox.net
> > > http://lists.busybox.net/mailman/listinfo/buildroot  
> > _______________________________________________
> > buildroot mailing list
> > buildroot@busybox.net
> > http://lists.busybox.net/mailman/listinfo/buildroot
> >
Arnout Vandecappelle Nov. 13, 2015, 6:42 a.m. UTC | #8 
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]
diff mbox

Patch

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