| Message ID | 1445734779-7212-2-git-send-email-benoit.thebaudeau.dev@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
Hi Benoit, all On Sun, Oct 25, 2015 at 2:59 AM, Benoît Thébaudeau < benoit.thebaudeau.dev@gmail.com> wrote: > Use the same EnvironmentFile name as the SysV init script for > consistency. The filenames under /etc/default/ are usually just the > package/daemon/service/feature name without any extension, so remove the > ".conf" extension from EnvironmentFile. > > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau.dev@gmail.com> > > --- > Changes v2 -> v3: new patch. > --- > package/dhcp/dhcpd.service | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/package/dhcp/dhcpd.service b/package/dhcp/dhcpd.service > index 7b265cb..5989506 100644 > --- a/package/dhcp/dhcpd.service > +++ b/package/dhcp/dhcpd.service > @@ -7,7 +7,7 @@ Type=forking > PIDFile=/run/dhcpd.pid > ExecStart=/usr/sbin/dhcpd -q -pf /run/dhcpd.pid $INTERFACES > KillSignal=SIGINT > -EnvironmentFile=/etc/default/dhcpd.conf > +EnvironmentFile=/etc/default/dhcpd > Maybe this should be: EnvironmentFile-=/etc/default/dhcpd Notice the '-' before the '=', in case the file doesn't exists, it won't print a warning or error the whole service. When I build dhcp, I did not find a /etc/default folder in my target directory, am I missing something there ? > > [Install] > WantedBy=multi-user.target > -- > 2.1.4 > >
On Wed, Nov 4, 2015 at 10:52 AM, Maxime Hadjinlian < maxime.hadjinlian@gmail.com> wrote: > Hi Benoit, all > > On Sun, Oct 25, 2015 at 2:59 AM, Benoît Thébaudeau < > benoit.thebaudeau.dev@gmail.com> wrote: > >> Use the same EnvironmentFile name as the SysV init script for >> consistency. The filenames under /etc/default/ are usually just the >> package/daemon/service/feature name without any extension, so remove the >> ".conf" extension from EnvironmentFile. >> >> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau.dev@gmail.com> >> >> --- >> Changes v2 -> v3: new patch. >> --- >> package/dhcp/dhcpd.service | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/package/dhcp/dhcpd.service b/package/dhcp/dhcpd.service >> index 7b265cb..5989506 100644 >> --- a/package/dhcp/dhcpd.service >> +++ b/package/dhcp/dhcpd.service >> @@ -7,7 +7,7 @@ Type=forking >> PIDFile=/run/dhcpd.pid >> ExecStart=/usr/sbin/dhcpd -q -pf /run/dhcpd.pid $INTERFACES >> KillSignal=SIGINT >> -EnvironmentFile=/etc/default/dhcpd.conf >> +EnvironmentFile=/etc/default/dhcpd >> > Maybe this should be: > EnvironmentFile-=/etc/default/dhcpd > > Notice the '-' before the '=', in case the file doesn't exists, it won't > print a warning or error the whole service. > > My mistake, the '-' should be *AFTER* the '='. > When I build dhcp, I did not find a /etc/default folder in my target > directory, am I missing something there ? > >> >> [Install] >> WantedBy=multi-user.target >> -- >> 2.1.4 >> >> >
Hi Maxime, all, On 04/11/2015 10:54, Maxime Hadjinlian wrote: > > > On Wed, Nov 4, 2015 at 10:52 AM, Maxime Hadjinlian <maxime.hadjinlian@gmail.com <mailto:maxime.hadjinlian@gmail.com>> wrote: > > Hi Benoit, all > > On Sun, Oct 25, 2015 at 2:59 AM, Benoît Thébaudeau <benoit.thebaudeau.dev@gmail.com <mailto:benoit.thebaudeau.dev@gmail.com>> wrote: > > Use the same EnvironmentFile name as the SysV init script for > consistency. The filenames under /etc/default/ are usually just the > package/daemon/service/feature name without any extension, so remove the > ".conf" extension from EnvironmentFile. > > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau.dev@gmail.com <mailto:benoit.thebaudeau.dev@gmail.com>> > > --- > Changes v2 -> v3: new patch. > --- > package/dhcp/dhcpd.service | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/package/dhcp/dhcpd.service b/package/dhcp/dhcpd.service > index 7b265cb..5989506 100644 > --- a/package/dhcp/dhcpd.service > +++ b/package/dhcp/dhcpd.service > @@ -7,7 +7,7 @@ Type=forking > PIDFile=/run/dhcpd.pid > ExecStart=/usr/sbin/dhcpd -q -pf /run/dhcpd.pid $INTERFACES > KillSignal=SIGINT > -EnvironmentFile=/etc/default/dhcpd.conf > +EnvironmentFile=/etc/default/dhcpd > > Maybe this should be: > EnvironmentFile-=/etc/default/dhcpd > > Notice the '-' before the '=', in case the file doesn't exists, it won't print a warning or error the whole service. > > My mistake, the '-' should be *AFTER* the '='. See 06/13. > When I build dhcp, I did not find a /etc/default folder in my target directory, am I missing something there ? You might have this question about 13/13 which requires a file in /etc/default, but not here because of 06/13. But a default file provided by Buildroot would not make sense for 13/13 since the file contents would be application-specific though required. In this case, it is up to users to add a rootfs overlay with such files. > [Install] > WantedBy=multi-user.target > -- > 2.1.4 Best regards, Benoît
On Wed, Nov 4, 2015 at 11:01 AM, Benoît Thébaudeau <benoit@wsystem.com> wrote: > Hi Maxime, all, > > On 04/11/2015 10:54, Maxime Hadjinlian wrote: > > > > > > On Wed, Nov 4, 2015 at 10:52 AM, Maxime Hadjinlian < > maxime.hadjinlian@gmail.com <mailto:maxime.hadjinlian@gmail.com>> wrote: > > > > Hi Benoit, all > > > > On Sun, Oct 25, 2015 at 2:59 AM, Benoît Thébaudeau < > benoit.thebaudeau.dev@gmail.com <mailto:benoit.thebaudeau.dev@gmail.com>> > wrote: > > > > Use the same EnvironmentFile name as the SysV init script for > > consistency. The filenames under /etc/default/ are usually just > the > > package/daemon/service/feature name without any extension, so > remove the > > ".conf" extension from EnvironmentFile. > > > > Signed-off-by: Benoît Thébaudeau < > benoit.thebaudeau.dev@gmail.com <mailto:benoit.thebaudeau.dev@gmail.com>> > > > > --- > > Changes v2 -> v3: new patch. > > --- > > package/dhcp/dhcpd.service | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/package/dhcp/dhcpd.service > b/package/dhcp/dhcpd.service > > index 7b265cb..5989506 100644 > > --- a/package/dhcp/dhcpd.service > > +++ b/package/dhcp/dhcpd.service > > @@ -7,7 +7,7 @@ Type=forking > > PIDFile=/run/dhcpd.pid > > ExecStart=/usr/sbin/dhcpd -q -pf /run/dhcpd.pid $INTERFACES > > KillSignal=SIGINT > > -EnvironmentFile=/etc/default/dhcpd.conf > > +EnvironmentFile=/etc/default/dhcpd > > > > Maybe this should be: > > EnvironmentFile-=/etc/default/dhcpd > > > > Notice the '-' before the '=', in case the file doesn't exists, it > won't print a warning or error the whole service. > > > > My mistake, the '-' should be *AFTER* the '='. > > See 06/13. > Ah yes, should they be squashed ? > > > When I build dhcp, I did not find a /etc/default folder in my target > directory, am I missing something there ? > > You might have this question about 13/13 which requires a file in > /etc/default, > but not here because of 06/13. But a default file provided by Buildroot > would > not make sense for 13/13 since the file contents would be > application-specific > though required. In this case, it is up to users to add a rootfs overlay > with > such files. > Ok, that's fair enough. > > > [Install] > > WantedBy=multi-user.target > > -- > > 2.1.4 > > Best regards, > Benoît >
On 04/11/2015 11:04, Maxime Hadjinlian wrote: > > > On Wed, Nov 4, 2015 at 11:01 AM, Benoît Thébaudeau <benoit@wsystem.com <mailto:benoit@wsystem.com>> wrote: > > Hi Maxime, all, > > On 04/11/2015 10:54, Maxime Hadjinlian wrote: > > > > > > On Wed, Nov 4, 2015 at 10:52 AM, Maxime Hadjinlian <maxime.hadjinlian@gmail.com <mailto:maxime.hadjinlian@gmail.com> <mailto:maxime.hadjinlian@gmail.com <mailto:maxime.hadjinlian@gmail.com>>> wrote: > > > > Hi Benoit, all > > > > On Sun, Oct 25, 2015 at 2:59 AM, Benoît Thébaudeau <benoit.thebaudeau.dev@gmail.com <mailto:benoit.thebaudeau.dev@gmail.com> <mailto:benoit.thebaudeau.dev@gmail.com <mailto:benoit.thebaudeau.dev@gmail.com>>> wrote: > > > > Use the same EnvironmentFile name as the SysV init script for > > consistency. The filenames under /etc/default/ are usually just the > > package/daemon/service/feature name without any extension, so remove the > > ".conf" extension from EnvironmentFile. > > > > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau.dev@gmail.com <mailto:benoit.thebaudeau.dev@gmail.com> <mailto:benoit.thebaudeau.dev@gmail.com <mailto:benoit.thebaudeau.dev@gmail.com>>> > > > > --- > > Changes v2 -> v3: new patch. > > --- > > package/dhcp/dhcpd.service | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/package/dhcp/dhcpd.service b/package/dhcp/dhcpd.service > > index 7b265cb..5989506 100644 > > --- a/package/dhcp/dhcpd.service > > +++ b/package/dhcp/dhcpd.service > > @@ -7,7 +7,7 @@ Type=forking > > PIDFile=/run/dhcpd.pid > > ExecStart=/usr/sbin/dhcpd -q -pf /run/dhcpd.pid $INTERFACES > > KillSignal=SIGINT > > -EnvironmentFile=/etc/default/dhcpd.conf > > +EnvironmentFile=/etc/default/dhcpd > > > > Maybe this should be: > > EnvironmentFile-=/etc/default/dhcpd > > > > Notice the '-' before the '=', in case the file doesn't exists, it won't print a warning or error the whole service. > > > > My mistake, the '-' should be *AFTER* the '='. > > See 06/13. > > Ah yes, should they be squashed ? No: these patches have different purposes. > > When I build dhcp, I did not find a /etc/default folder in my target directory, am I missing something there ? > > You might have this question about 13/13 which requires a file in /etc/default, > but not here because of 06/13. But a default file provided by Buildroot would > not make sense for 13/13 since the file contents would be application-specific > though required. In this case, it is up to users to add a rootfs overlay with > such files. > > Ok, that's fair enough. > > > > [Install] > > WantedBy=multi-user.target > > -- > > 2.1.4 Best regards, Benoît
[-SNIP-] > > > Maybe this should be: > > > EnvironmentFile-=/etc/default/dhcpd > > > > > > Notice the '-' before the '=', in case the file doesn't > exists, it won't print a warning or error the whole service. > > > > > > My mistake, the '-' should be *AFTER* the '='. > > > > See 06/13. > > > > Ah yes, should they be squashed ? > > No: these patches have different purposes. > Yes, on that bombshell Reviewed-by: "Maxime Hadjinlian" <maxime.hadjinlian@gmail.com> [-SNIP-]
Maxime, Benoît, Gabe, On Wed, 4 Nov 2015 10:52:10 +0100, Maxime Hadjinlian wrote: > > diff --git a/package/dhcp/dhcpd.service b/package/dhcp/dhcpd.service > > index 7b265cb..5989506 100644 > > --- a/package/dhcp/dhcpd.service > > +++ b/package/dhcp/dhcpd.service > > @@ -7,7 +7,7 @@ Type=forking > > PIDFile=/run/dhcpd.pid > > ExecStart=/usr/sbin/dhcpd -q -pf /run/dhcpd.pid $INTERFACES > > KillSignal=SIGINT > > -EnvironmentFile=/etc/default/dhcpd.conf > > +EnvironmentFile=/etc/default/dhcpd > > > Maybe this should be: > EnvironmentFile-=/etc/default/dhcpd On the dropbear package, Gabe Evans said it may not be such a good idea to do this EnvironmentFile thing. Can we settle on a common decision ? (From my PoV, using this EnvironmentFile looked good as it would be a common mechanism between the systemd and Busybox/sysvinit cases to tune the configuration of services. But Gabe seemed to disagree.) Could systemd people in Buildroot decide what to do ? :-) Thomas
Thomas, Gabe, Maxime, all, On 04/11/2015 11:18, Thomas Petazzoni wrote: > On Wed, 4 Nov 2015 10:52:10 +0100, Maxime Hadjinlian wrote: > >>> diff --git a/package/dhcp/dhcpd.service b/package/dhcp/dhcpd.service >>> index 7b265cb..5989506 100644 >>> --- a/package/dhcp/dhcpd.service >>> +++ b/package/dhcp/dhcpd.service >>> @@ -7,7 +7,7 @@ Type=forking >>> PIDFile=/run/dhcpd.pid >>> ExecStart=/usr/sbin/dhcpd -q -pf /run/dhcpd.pid $INTERFACES >>> KillSignal=SIGINT >>> -EnvironmentFile=/etc/default/dhcpd.conf >>> +EnvironmentFile=/etc/default/dhcpd >>> >> Maybe this should be: >> EnvironmentFile-=/etc/default/dhcpd > > On the dropbear package, Gabe Evans said it may not be such a good idea > to do this EnvironmentFile thing. Why? What was the rationale? What would be the alternatives? > Can we settle on a common decision ? > > (From my PoV, using this EnvironmentFile looked good as it would be a > common mechanism between the systemd and Busybox/sysvinit cases to tune > the configuration of services. But Gabe seemed to disagree.) > > Could systemd people in Buildroot decide what to do ? :-) Benoît
Hi Thomas, all On Wed, Nov 4, 2015 at 11:18 AM, Thomas Petazzoni < thomas.petazzoni@free-electrons.com> wrote: > Maxime, Benoît, Gabe, > > On Wed, 4 Nov 2015 10:52:10 +0100, Maxime Hadjinlian wrote: > > > > diff --git a/package/dhcp/dhcpd.service b/package/dhcp/dhcpd.service > > > index 7b265cb..5989506 100644 > > > --- a/package/dhcp/dhcpd.service > > > +++ b/package/dhcp/dhcpd.service > > > @@ -7,7 +7,7 @@ Type=forking > > > PIDFile=/run/dhcpd.pid > > > ExecStart=/usr/sbin/dhcpd -q -pf /run/dhcpd.pid $INTERFACES > > > KillSignal=SIGINT > > > -EnvironmentFile=/etc/default/dhcpd.conf > > > +EnvironmentFile=/etc/default/dhcpd > > > > > Maybe this should be: > > EnvironmentFile-=/etc/default/dhcpd > > On the dropbear package, Gabe Evans said it may not be such a good idea > to do this EnvironmentFile thing. > > Can we settle on a common decision ? > > (From my PoV, using this EnvironmentFile looked good as it would be a > common mechanism between the systemd and Busybox/sysvinit cases to tune > the configuration of services. But Gabe seemed to disagree.) > I still have to read that thread, I'll get to it in a few minutes. The mechanism in itself is good, adding the '-' simply makes the file optional. For me, a program must be able to start without options, or they are not options but mandatory parameters (which is a bit contradictory) > > Could systemd people in Buildroot decide what to do ? :-) > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com >
Hi Thomas, all, On Wed, Nov 4, 2015 at 2:18 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Maxime, Benoît, Gabe, > > On Wed, 4 Nov 2015 10:52:10 +0100, Maxime Hadjinlian wrote: > >> > diff --git a/package/dhcp/dhcpd.service b/package/dhcp/dhcpd.service >> > index 7b265cb..5989506 100644 >> > --- a/package/dhcp/dhcpd.service >> > +++ b/package/dhcp/dhcpd.service >> > @@ -7,7 +7,7 @@ Type=forking >> > PIDFile=/run/dhcpd.pid >> > ExecStart=/usr/sbin/dhcpd -q -pf /run/dhcpd.pid $INTERFACES >> > KillSignal=SIGINT >> > -EnvironmentFile=/etc/default/dhcpd.conf >> > +EnvironmentFile=/etc/default/dhcpd >> > >> Maybe this should be: >> EnvironmentFile-=/etc/default/dhcpd > > On the dropbear package, Gabe Evans said it may not be such a good idea > to do this EnvironmentFile thing. > > Can we settle on a common decision ? > > (From my PoV, using this EnvironmentFile looked good as it would be a > common mechanism between the systemd and Busybox/sysvinit cases to tune > the configuration of services. But Gabe seemed to disagree.) > > Could systemd people in Buildroot decide what to do ? :-) > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com Maxime cleared everything up for me. I'm okay with this as long as the environment file is optional with: 'EnvironmentFile=-/etc/default/dhcpd'. Under the rationale of sharing configuration between Busybox/SysVinit it makes the most sense. Thanks, Gabe
Dear Benoît Thébaudeau, On Sun, 25 Oct 2015 02:59:28 +0200, Benoît Thébaudeau wrote: > Use the same EnvironmentFile name as the SysV init script for > consistency. The filenames under /etc/default/ are usually just the > package/daemon/service/feature name without any extension, so remove the > ".conf" extension from EnvironmentFile. > > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau.dev@gmail.com> I already did this change in a commit that also adjusted S80dhcp-server: https://git.busybox.net/buildroot/commit/?id=6f81baaf47e3f47b131ec2b1c6c5f7d062a48d84 Thanks, Thomas
diff --git a/package/dhcp/dhcpd.service b/package/dhcp/dhcpd.service index 7b265cb..5989506 100644 --- a/package/dhcp/dhcpd.service +++ b/package/dhcp/dhcpd.service @@ -7,7 +7,7 @@ Type=forking PIDFile=/run/dhcpd.pid ExecStart=/usr/sbin/dhcpd -q -pf /run/dhcpd.pid $INTERFACES KillSignal=SIGINT -EnvironmentFile=/etc/default/dhcpd.conf +EnvironmentFile=/etc/default/dhcpd [Install] WantedBy=multi-user.target
Use the same EnvironmentFile name as the SysV init script for consistency. The filenames under /etc/default/ are usually just the package/daemon/service/feature name without any extension, so remove the ".conf" extension from EnvironmentFile. Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau.dev@gmail.com> --- Changes v2 -> v3: new patch. --- package/dhcp/dhcpd.service | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)