[systemd-devel] [PATCH v3] systemctl: add add-wants and add-requires verbs

[Lennart Poettering]

On Thu, 25.09.14 15:26, Lukas Nykryn (lnykryn at redhat.com) wrote:

> ---
> Changes in v3
> - move "don't load logic" to unit_file_load
> - --help message should be finally fine
> 
>  TODO                                   |  1 -
>  man/systemctl.xml                      | 21 ++++++++
>  src/core/dbus-manager.c                | 84 ++++++++++++++++++-----------
>  src/core/org.freedesktop.systemd1.conf |  4 ++
>  src/core/selinux-access.c              | 29 ++++++++++
>  src/core/selinux-access.h              |  3 ++
>  src/shared/install.c                   | 96 ++++++++++++++++++++++++++++++----
>  src/shared/install.h                   | 11 ++++
>  src/systemctl/systemctl.c              | 95 +++++++++++++++++++++++++++++++++
>  9 files changed, 304 insertions(+), 40 deletions(-)
> 
> diff --git a/TODO b/TODO
> index 9ac6fac..923cbb8 100644
> --- a/TODO
> +++ b/TODO
> @@ -450,7 +450,6 @@ Features:
>    - "systemctl mask" should find all names by which a unit is accessible
>      (i.e. by scanning for symlinks to it) and link them all to /dev/null
>    - systemctl list-unit-files should list generated files (and probably with a new state "generated" for them, or so)
> -  - systemctl: maybe add "systemctl add-wants" or so...
>  
>  * timer units:
>    - timer units should get the ability to trigger when:
> diff --git a/man/systemctl.xml b/man/systemctl.xml
> index b28a3b7..c4ee308 100644
> --- a/man/systemctl.xml
> +++ b/man/systemctl.xml
> @@ -1098,6 +1098,27 @@ kobject-uevent 1 systemd-udevd-kernel.socket systemd-udevd.service
>          </varlistentry>
>  
>          <varlistentry>
> +          <term><command>add-wants|add-requires <replaceable>TARGET</replaceable>
> +          <replaceable>NAME</replaceable>...</command>
> +          </term>

Please use two <term> lines here, instead of merging them manually via
the "|". Like this:

          <term><command>add-wants <replaceable>TARGET</replaceable> <replaceable>NAME</replaceable>...</command></term>
          <term><command>add-requires <replaceable>TARGET</replaceable> <replaceable>NAME</replaceable>...</command></term>

> +
> +          <listitem>
> +            <para>Enables one or more units similarly to
> <command>enable</command>,

This is very much unlike "enable" I'd say. I wouldn't mention this
really.

Instead, just say that this adds a Wants=/Requires= type dependency
from the target to the specified units.

> +            but instead of looking to <literal>[Install]</literal> section of
> +            the unit file, adds <literal>Wants</literal>
> +            resp. <literal>Requires</literal> dependency to the specified
> +            <replaceable>TARGET</replaceable>.
> +            </para>


Enable is really only about applying [Install], and if you say this is
like enable without [install], then exactly nothing is left, hence
don't mention it please.
>  
> +static int method_add_install_dependency_unit_files(sd_bus *bus, sd_bus_message *message, void *userdata, sd_bus_error *error) {
> +        _cleanup_strv_free_ char **l = NULL;
> +        Manager *m = userdata;
> +        UnitFileChange *changes = NULL;
> +        unsigned n_changes = 0;
> +        UnitFileScope scope;
> +        int runtime, force, r;
> +        char *target;
> +        char *type;
> +        UnitFileInstallDependency dep;
> +
> +        assert(bus);
> +        assert(message);
> +        assert(m);
> +
> +        r = bus_verify_manage_unit_files_async(m, message, error);
> +        if (r < 0)
> +                return r;
> +        if (r == 0)
> +                return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
> +
> +        r = sd_bus_message_read_strv(message, &l);
> +        if (r < 0)
> +                return r;
> +
> +        r = sd_bus_message_read(message, "ssbb", &target, &type, &runtime, &force);
> +        if (r < 0)
> +                return r;
> +
> +        dep = unit_file_install_dependency_from_string(type);
> +

Spurious newline.

> +        if (dep < 0)
> +                return -EINVAL;
> +
> +        r = selinux_unit_access_check_strv(l, message, m, "enable", error);
> +        if (r < 0)
> +                return r;
> +
> +        scope = m->running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : UNIT_FILE_USER;
> +
> +        r = unit_file_add_install_dependency(scope, runtime, NULL, l, target, dep, force, &changes, &n_changes);
> +        if (r < 0)
> +                return r;

Why the word "install" in this? unit_file_add_dependency() sounds OK
too?

> +
> +        return reply_unit_file_changes_and_free(m, bus, message, -1, changes, n_changes);
> +}
> +
>  const sd_bus_vtable bus_manager_vtable[] = {
>          SD_BUS_VTABLE_START(0),
>  
> @@ -1918,6 +1941,7 @@ const sd_bus_vtable bus_manager_vtable[] = {
>          SD_BUS_METHOD("SetDefaultTarget", "sb", "a(sss)", method_set_default_target, SD_BUS_VTABLE_UNPRIVILEGED),
>          SD_BUS_METHOD("GetDefaultTarget", NULL, "s", method_get_default_target, SD_BUS_VTABLE_UNPRIVILEGED),
>          SD_BUS_METHOD("PresetAllUnitFiles", "sbb", "a(sss)", method_preset_all_unit_files, SD_BUS_VTABLE_UNPRIVILEGED),
> +        SD_BUS_METHOD("AddInstallDependencyUnitFiles", "asssbb",
>          "a(sss)", method_add_install_dependency_unit_files,
>          SD_BUS_VTABLE_UNPRIVILEGED),

similar here.

> +int unit_file_add_install_dependency(
> +                UnitFileScope scope,
> +                bool runtime,
> +                const char *root_dir,
> +                char **files,
> +                char *target,
> +                UnitFileInstallDependency dep,

Instead of introducing a new enum here, I think it would be nicer to
just bite the bullet and move UnitDependency from src/core/unit.h into
src/shared/unit-name.h, along with its to_string()/from_string()
calls. Then, simply reuse the enum here...

Otherwise looks pretty OK to me, but I didn't test it...

Lennart

-- 
Lennart Poettering, Red Hat