Re: [systemd-devel] [PATCH 2/3] install: don't allow to enable/disable templates

2013-08-09 Thread Michal Sekletar 
On Thu, Aug 08, 2013 at 06:11:42PM +0200, Zbigniew Jędrzejewski-Szmek wrote:
> On Thu, Aug 08, 2013 at 03:19:09PM +0200, Michal Sekletar wrote:
> > Calling enable on template units doesn't make sense since it is possible
> > to enable instances directly and users are not forced to use Alias=
> > trickery anymore.
> Hm, I'm having trouble with parsing this commit message. Can you
> explicitly say what this patch changes (remembering that git log
> is read by people who are not familiar with the codebase and just
> care about the changes in semantics)?

Hi Zbyszek, 

there is no need to change commit message, since from discussion above
in this thread it looks like that my patch can't be merged.
Anyways, thank you for looking at this.

> 
> Zbyszek

Michal
> 
> > ---
> >  src/shared/install.c | 20 ++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/shared/install.c b/src/shared/install.c
> > index 07e06c4..5cda794 100644
> > --- a/src/shared/install.c
> > +++ b/src/shared/install.c
> > @@ -1470,9 +1470,11 @@ int unit_file_enable(
> >  
> >  _cleanup_lookup_paths_free_ LookupPaths paths = {};
> >  _cleanup_install_context_done_ InstallContext c = {};
> > -char **i;
> > +InstallInfo *v;
> > +char **i, *k;
> >  _cleanup_free_ char *config_path = NULL;
> >  int r;
> > +Iterator j;
> >  
> >  assert(scope >= 0);
> >  assert(scope < _UNIT_FILE_SCOPE_MAX);
> > @@ -1491,6 +1493,12 @@ int unit_file_enable(
> >  return r;
> >  }
> >  
> > +HASHMAP_FOREACH_KEY(v, k, c.will_install, j) {
> > +r = unit_name_is_valid(k, false);
> > +if (!r)
> > +return -EINVAL;
> > +}
> > +
> >  /* This will return the number of symlink rules that were
> >  supposed to be created, not the ones actually created. This is
> >  useful to determine whether the passed files had any
> > @@ -1509,10 +1517,12 @@ int unit_file_disable(
> >  
> >  _cleanup_lookup_paths_free_ LookupPaths paths = {};
> >  _cleanup_install_context_done_ InstallContext c = {};
> > -char **i;
> > +InstallInfo *v;
> > +char **i, *k;
> >  _cleanup_free_ char *config_path = NULL;
> >  _cleanup_set_free_free_ Set *remove_symlinks_to = NULL;
> >  int r, q;
> > +Iterator j;
> >  
> >  assert(scope >= 0);
> >  assert(scope < _UNIT_FILE_SCOPE_MAX);
> > @@ -1531,6 +1541,12 @@ int unit_file_disable(
> >  return r;
> >  }
> >  
> > +HASHMAP_FOREACH_KEY(v, k, c.will_install, j) {
> > +r = unit_name_is_valid(k, false);
> > +if (!r)
> > +return -EINVAL;
> > +}
> > +
> >  r = install_context_mark_for_removal(&c, &paths, 
> > &remove_symlinks_to, config_path, root_dir);
> >  
> >  q = remove_marked_symlinks(remove_symlinks_to, config_path, 
> > changes, n_changes, files);
> > -- 
> > 1.8.3.1
> > 
> > ___
> > systemd-devel mailing list
> > systemd-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/systemd-devel
> > 
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/3] install: don't allow to enable/disable templates

2013-08-09 Thread Michal Sekletar 
On Thu, Aug 08, 2013 at 03:35:24PM +0200, Tom Gundersen wrote:
> Hi Michal,

Hello Tom,

> 
> On Thu, Aug 8, 2013 at 3:19 PM, Michal Sekletar  wrote:
> > Calling enable on template units doesn't make sense since it is possible
> > to enable instances directly and users are not forced to use Alias=
> > trickery anymore.
> 
> It actually might make sense to still call enable on a template unit
> with an Alias:

I was too quick doing conclusions here, sorry about that. On the bright
side, at least the discussion started about this topic.

> 
> Imagine foo@.service with Alias=bar@.service. In this case you want to
> be able to do "systemctl enable foo@.service", and then "systemctl
> start bar@baz.service" should start the instance foo@baz.service. An
> example where this would be useful is for "systemctl enable
> kmsconvt@.service" to make the kmsconvt template override the autovt
> template.

Fair enough, it looks like there are use cases for template enablement
after all, thus we shouldn't be so strict as proposed by my patch.

> 
> However, we definitely don't want to allow a template name in
> WantedBy= or RequiredBy= (this doesn't make any sense as "systemctl
> start foo@.service" is invalid).
> 
> Currently we don't really verify what is given in
> Alias/WantedBy/RequiredBy at all at enable time (only at runtime), but
> I think we should do this using unit_name_is_valid(name,true) for
> Alias and unit_name_is_valid(name,false) for the other two.

Doing some validation of installation information encoded in unit file seems
to me as very good idea.

> 
> What do you think?

Once everything is figured out then we should also expand documentation in
relevant section of systemd.unit man page because AFAIK now it doesn't cover
template unit files at all.

> 
> Cheers,
> 
> Tom

Michal
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/3] install: don't allow to enable/disable templates

2013-08-09 Thread Colin Guthrie 
'Twas brillig, and Colin Guthrie at 09/08/13 12:02 did gyre and gimble:
> 'Twas brillig, and Tom Gundersen at 08/08/13 18:08 did gyre and gimble:
>>
>> On 8 Aug 2013 17:57, "Thomas Bächler" > > wrote:
>>>
>>> Am 08.08.2013 15:19, schrieb Michal Sekletar:
 Calling enable on template units doesn't make sense since it is possible
 to enable instances directly and users are not forced to use Alias=
 trickery anymore.
>>>
>>> Actually, it would make sense to do this instead:
>>>
>> http://www.mail-archive.com/systemd-devel@lists.freedesktop.org/msg09244.html
>>
>> Yes, we should do this too. But we still need to solve the case when
>> DefaultInstance is not specified.
> 
> I actually ran into a problem the other day related to this.
> 
> I forgot to update my .spec after updating systemd to 206 and because of
> http://cgit.freedesktop.org/systemd/systemd/commit/?id=7aa4fa34f76b0d9b031f0a5ea941c7fa10cebbee
> it caused an interesting problem.
> 
> Doing "systemctl enable getty@.service simply created a getty@.service
> symlink. When this was run at boot, it resulted in systemd trying to
> start getty@getty.target, i.e. the instance name was filled in with the
> unit name.
> 
> Ultimately this lead to agetty failing (as /dev/getty didn't exist) and
> as it has Restart=always in the unit, it kept going and going and going...
> 
> Not the nicest experience.
> 
> Surely if you try and start a template unit without any instance it
> should simply fail rather than default to the unit name?

I see this specific problem is mentioned elsewhere in the thread, so it
can be covered there...

Col


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited http://www.tribalogic.net/
Open Source:
  Mageia Contributor http://www.mageia.org/
  PulseAudio Hacker http://www.pulseaudio.org/
  Trac Hacker http://trac.edgewall.org/

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/3] install: don't allow to enable/disable templates

2013-08-09 Thread Colin Guthrie 
'Twas brillig, and Tom Gundersen at 08/08/13 18:08 did gyre and gimble:
> 
> On 8 Aug 2013 17:57, "Thomas Bächler"  > wrote:
>>
>> Am 08.08.2013 15:19, schrieb Michal Sekletar:
>> > Calling enable on template units doesn't make sense since it is possible
>> > to enable instances directly and users are not forced to use Alias=
>> > trickery anymore.
>>
>> Actually, it would make sense to do this instead:
>>
> http://www.mail-archive.com/systemd-devel@lists.freedesktop.org/msg09244.html
> 
> Yes, we should do this too. But we still need to solve the case when
> DefaultInstance is not specified.

I actually ran into a problem the other day related to this.

I forgot to update my .spec after updating systemd to 206 and because of
http://cgit.freedesktop.org/systemd/systemd/commit/?id=7aa4fa34f76b0d9b031f0a5ea941c7fa10cebbee
it caused an interesting problem.

Doing "systemctl enable getty@.service simply created a getty@.service
symlink. When this was run at boot, it resulted in systemd trying to
start getty@getty.target, i.e. the instance name was filled in with the
unit name.

Ultimately this lead to agetty failing (as /dev/getty didn't exist) and
as it has Restart=always in the unit, it kept going and going and going...

Not the nicest experience.

Surely if you try and start a template unit without any instance it
should simply fail rather than default to the unit name?

Col

-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited http://www.tribalogic.net/
Open Source:
  Mageia Contributor http://www.mageia.org/
  PulseAudio Hacker http://www.pulseaudio.org/
  Trac Hacker http://trac.edgewall.org/

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/3] install: don't allow to enable/disable templates

2013-08-08 Thread Zbigniew Jędrzejewski-Szmek 
On Thu, Aug 08, 2013 at 03:19:09PM +0200, Michal Sekletar wrote:
> Calling enable on template units doesn't make sense since it is possible
> to enable instances directly and users are not forced to use Alias=
> trickery anymore.
Hm, I'm having trouble with parsing this commit message. Can you
explicitly say what this patch changes (remembering that git log
is read by people who are not familiar with the codebase and just
care about the changes in semantics)?

Zbyszek

> ---
>  src/shared/install.c | 20 ++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/src/shared/install.c b/src/shared/install.c
> index 07e06c4..5cda794 100644
> --- a/src/shared/install.c
> +++ b/src/shared/install.c
> @@ -1470,9 +1470,11 @@ int unit_file_enable(
>  
>  _cleanup_lookup_paths_free_ LookupPaths paths = {};
>  _cleanup_install_context_done_ InstallContext c = {};
> -char **i;
> +InstallInfo *v;
> +char **i, *k;
>  _cleanup_free_ char *config_path = NULL;
>  int r;
> +Iterator j;
>  
>  assert(scope >= 0);
>  assert(scope < _UNIT_FILE_SCOPE_MAX);
> @@ -1491,6 +1493,12 @@ int unit_file_enable(
>  return r;
>  }
>  
> +HASHMAP_FOREACH_KEY(v, k, c.will_install, j) {
> +r = unit_name_is_valid(k, false);
> +if (!r)
> +return -EINVAL;
> +}
> +
>  /* This will return the number of symlink rules that were
>  supposed to be created, not the ones actually created. This is
>  useful to determine whether the passed files had any
> @@ -1509,10 +1517,12 @@ int unit_file_disable(
>  
>  _cleanup_lookup_paths_free_ LookupPaths paths = {};
>  _cleanup_install_context_done_ InstallContext c = {};
> -char **i;
> +InstallInfo *v;
> +char **i, *k;
>  _cleanup_free_ char *config_path = NULL;
>  _cleanup_set_free_free_ Set *remove_symlinks_to = NULL;
>  int r, q;
> +Iterator j;
>  
>  assert(scope >= 0);
>  assert(scope < _UNIT_FILE_SCOPE_MAX);
> @@ -1531,6 +1541,12 @@ int unit_file_disable(
>  return r;
>  }
>  
> +HASHMAP_FOREACH_KEY(v, k, c.will_install, j) {
> +r = unit_name_is_valid(k, false);
> +if (!r)
> +return -EINVAL;
> +}
> +
>  r = install_context_mark_for_removal(&c, &paths, 
> &remove_symlinks_to, config_path, root_dir);
>  
>  q = remove_marked_symlinks(remove_symlinks_to, config_path, changes, 
> n_changes, files);
> -- 
> 1.8.3.1
> 
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
> 
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/3] install: don't allow to enable/disable templates

2013-08-08 Thread Tom Gundersen 
On 8 Aug 2013 17:57, "Thomas Bächler"  wrote:
>
> Am 08.08.2013 15:19, schrieb Michal Sekletar:
> > Calling enable on template units doesn't make sense since it is possible
> > to enable instances directly and users are not forced to use Alias=
> > trickery anymore.
>
> Actually, it would make sense to do this instead:
>
http://www.mail-archive.com/systemd-devel@lists.freedesktop.org/msg09244.html

Yes, we should do this too. But we still need to solve the case when
DefaultInstance is not specified.

Cheers,

Tom
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/3] install: don't allow to enable/disable templates

2013-08-08 Thread Thomas Bächler 
Am 08.08.2013 15:19, schrieb Michal Sekletar:
> Calling enable on template units doesn't make sense since it is possible
> to enable instances directly and users are not forced to use Alias=
> trickery anymore.

Actually, it would make sense to do this instead:
http://www.mail-archive.com/systemd-devel@lists.freedesktop.org/msg09244.html




signature.asc
Description: OpenPGP digital signature
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/3] install: don't allow to enable/disable templates

2013-08-08 Thread Tom Gundersen 
Hi Michal,

On Thu, Aug 8, 2013 at 3:19 PM, Michal Sekletar  wrote:
> Calling enable on template units doesn't make sense since it is possible
> to enable instances directly and users are not forced to use Alias=
> trickery anymore.

It actually might make sense to still call enable on a template unit
with an Alias:

Imagine foo@.service with Alias=bar@.service. In this case you want to
be able to do "systemctl enable foo@.service", and then "systemctl
start bar@baz.service" should start the instance foo@baz.service. An
example where this would be useful is for "systemctl enable
kmsconvt@.service" to make the kmsconvt template override the autovt
template.

However, we definitely don't want to allow a template name in
WantedBy= or RequiredBy= (this doesn't make any sense as "systemctl
start foo@.service" is invalid).

Currently we don't really verify what is given in
Alias/WantedBy/RequiredBy at all at enable time (only at runtime), but
I think we should do this using unit_name_is_valid(name,true) for
Alias and unit_name_is_valid(name,false) for the other two.

What do you think?

Cheers,

Tom
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 2/3] install: don't allow to enable/disable templates

2013-08-08 Thread Michal Sekletar 
Calling enable on template units doesn't make sense since it is possible
to enable instances directly and users are not forced to use Alias=
trickery anymore.
---
 src/shared/install.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/shared/install.c b/src/shared/install.c
index 07e06c4..5cda794 100644
--- a/src/shared/install.c
+++ b/src/shared/install.c
@@ -1470,9 +1470,11 @@ int unit_file_enable(
 
 _cleanup_lookup_paths_free_ LookupPaths paths = {};
 _cleanup_install_context_done_ InstallContext c = {};
-char **i;
+InstallInfo *v;
+char **i, *k;
 _cleanup_free_ char *config_path = NULL;
 int r;
+Iterator j;
 
 assert(scope >= 0);
 assert(scope < _UNIT_FILE_SCOPE_MAX);
@@ -1491,6 +1493,12 @@ int unit_file_enable(
 return r;
 }
 
+HASHMAP_FOREACH_KEY(v, k, c.will_install, j) {
+r = unit_name_is_valid(k, false);
+if (!r)
+return -EINVAL;
+}
+
 /* This will return the number of symlink rules that were
 supposed to be created, not the ones actually created. This is
 useful to determine whether the passed files had any
@@ -1509,10 +1517,12 @@ int unit_file_disable(
 
 _cleanup_lookup_paths_free_ LookupPaths paths = {};
 _cleanup_install_context_done_ InstallContext c = {};
-char **i;
+InstallInfo *v;
+char **i, *k;
 _cleanup_free_ char *config_path = NULL;
 _cleanup_set_free_free_ Set *remove_symlinks_to = NULL;
 int r, q;
+Iterator j;
 
 assert(scope >= 0);
 assert(scope < _UNIT_FILE_SCOPE_MAX);
@@ -1531,6 +1541,12 @@ int unit_file_disable(
 return r;
 }
 
+HASHMAP_FOREACH_KEY(v, k, c.will_install, j) {
+r = unit_name_is_valid(k, false);
+if (!r)
+return -EINVAL;
+}
+
 r = install_context_mark_for_removal(&c, &paths, &remove_symlinks_to, 
config_path, root_dir);
 
 q = remove_marked_symlinks(remove_symlinks_to, config_path, changes, 
n_changes, files);
-- 
1.8.3.1

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel