Re: [systemd-devel] [PATCH] install: make "reenable" work with templated units

2013-07-13 Thread Zbigniew Jędrzejewski-Szmek
On Sat, Jul 13, 2013 at 10:51:01PM +0200, Ross Lagerwall wrote:
> On Sat, Jul 13, 2013 at 04:12:33PM +0200, Zbigniew Jędrzejewski-Szmek wrote:
> > On Mon, Jun 17, 2013 at 07:11:50PM +0100, Ross Lagerwall wrote:
> > > Before, "systemctl reenable getty@tty1.service" would fail with:
> > > Failed to issue method call: File exists
> > > To fix this, reimplement "reenable" explicitly as a disable followed by
> > > an enable.
> > > This is shorter and is how the man page documents its behavior.
> > > ---
> > >  src/shared/install.c |   38 +-
> > >  1 file changed, 5 insertions(+), 33 deletions(-)
> > Hm, I don't get this error with "reenable", but your patch indeed 
> > simplifies things, so I don't see a reason not to apply it: applied now.
> 
> 
> As far as I can recall, it would fail if
> /etc/systemd/system/getty.target.wants/getty@tty1.service is set up as a
> symlink to /usr/lib/systemd/system/getty@.service and then "systemctl
> reenable getty@tty1.service" is run. 
That's the scenario I tested, and it worked (and works) fine.
There have been some changes to make unit disabling more robust,
so this might have been fixed as a side effect.

We really should grow some test cases for unit enabling/disabling/starting,
it seems that there are some gray areas.

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


Re: [systemd-devel] [PATCH] install: make "reenable" work with templated units

2013-07-13 Thread Ross Lagerwall
On Sat, Jul 13, 2013 at 04:12:33PM +0200, Zbigniew Jędrzejewski-Szmek wrote:
> On Mon, Jun 17, 2013 at 07:11:50PM +0100, Ross Lagerwall wrote:
> > Before, "systemctl reenable getty@tty1.service" would fail with:
> > Failed to issue method call: File exists
> > To fix this, reimplement "reenable" explicitly as a disable followed by
> > an enable.
> > This is shorter and is how the man page documents its behavior.
> > ---
> >  src/shared/install.c |   38 +-
> >  1 file changed, 5 insertions(+), 33 deletions(-)
> Hm, I don't get this error with "reenable", but your patch indeed 
> simplifies things, so I don't see a reason not to apply it: applied now.


As far as I can recall, it would fail if
/etc/systemd/system/getty.target.wants/getty@tty1.service is set up as a
symlink to /usr/lib/systemd/system/getty@.service and then "systemctl
reenable getty@tty1.service" is run.  I can't exactly remember but I
think it would fail because "reenable" would try to delete the
untemplated unit "getty@.service" (which would not exist) but then try
to create the templated unit "getty@tty1.service" which would still
exist hence the "File exists" error.  Thanks for applying the patch
anyway.

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


Re: [systemd-devel] [PATCH] install: make "reenable" work with templated units

2013-07-13 Thread Zbigniew Jędrzejewski-Szmek
On Mon, Jun 17, 2013 at 07:11:50PM +0100, Ross Lagerwall wrote:
> Before, "systemctl reenable getty@tty1.service" would fail with:
> Failed to issue method call: File exists
> To fix this, reimplement "reenable" explicitly as a disable followed by
> an enable.
> This is shorter and is how the man page documents its behavior.
> ---
>  src/shared/install.c |   38 +-
>  1 file changed, 5 insertions(+), 33 deletions(-)
Hm, I don't get this error with "reenable", but your patch indeed 
simplifies things, so I don't see a reason not to apply it: applied now.

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


[systemd-devel] [PATCH] install: make "reenable" work with templated units

2013-06-17 Thread Ross Lagerwall
Before, "systemctl reenable getty@tty1.service" would fail with:
Failed to issue method call: File exists
To fix this, reimplement "reenable" explicitly as a disable followed by
an enable.
This is shorter and is how the man page documents its behavior.
---
 src/shared/install.c |   38 +-
 1 file changed, 5 insertions(+), 33 deletions(-)

diff --git a/src/shared/install.c b/src/shared/install.c
index d2dd276..1bda79a 100644
--- a/src/shared/install.c
+++ b/src/shared/install.c
@@ -1533,43 +1533,15 @@ int unit_file_reenable(
 bool force,
 UnitFileChange **changes,
 unsigned *n_changes) {
+int r;
 
-_cleanup_lookup_paths_free_ LookupPaths paths = {};
-_cleanup_install_context_done_ InstallContext c = {};
-char **i;
-_cleanup_free_ char *config_path = NULL;
-_cleanup_set_free_free_ Set *remove_symlinks_to = NULL;
-int r, q;
-
-assert(scope >= 0);
-assert(scope < _UNIT_FILE_SCOPE_MAX);
-
-r = lookup_paths_init_from_scope(&paths, scope);
-if (r < 0)
-return r;
-
-r = get_config_path(scope, runtime, root_dir, &config_path);
+r = unit_file_disable(scope, runtime, root_dir, files, changes,
+  n_changes);
 if (r < 0)
 return r;
 
-STRV_FOREACH(i, files) {
-r = mark_symlink_for_removal(&remove_symlinks_to, *i);
-if (r < 0)
-return r;
-
-r = install_info_add_auto(&c, *i);
-if (r < 0)
-return r;
-}
-
-r = remove_marked_symlinks(remove_symlinks_to, config_path, changes, 
n_changes, files);
-
-/* Returns number of symlinks that where supposed to be installed. */
-q = install_context_apply(&c, &paths, config_path, root_dir, force, 
changes, n_changes);
-if (r == 0)
-r = q;
-
-return r;
+return unit_file_enable(scope, runtime, root_dir, files, force,
+changes, n_changes);
 }
 
 int unit_file_set_default(
-- 
1.7.10.4

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