| Message ID | 1446099102-5205-1-git-send-email-cyrilbur@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
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]
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
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@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot >
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
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
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 >
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
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@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@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
>>>>> "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.
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]