[systemd-devel] [PATCH] systemctl: add command default-target
Zbigniew Jędrzejewski-Szmek
zbyszek at in.waw.pl
Tue May 21 11:39:55 PDT 2013
On Fri, May 17, 2013 at 03:14:15PM +0200, Vaclav Pavlin wrote:
> From: Václav Pavlín <vpavlin at redhat.com>
>
> systemctl default-target NAME will set the default target.
> If the NAME is not specified, the default target is printed out.
Hi,
this departs from current command structure in systemctl, where
there are always separate "status" and "modification" verbs:
start / status
set-cgroup-attr / get-cgroup-attr
set-cgroup / unset-cgroup
enable / is-enabled
set-environment / show-environment
Partially this is because there are multiple ways to display, and
multiple ways to set/unset, so it would be harder to fit whole functionality
under one verb. In case of 'default-target' there's less ambiguity.
Nevertheless, I think systemctl is already quite complicated and
consistency is very important. 'set-default' and 'get-default' seem
like a better fit. Also, there's already 'default', and if 'default-target'
was added, it would be unobvious what is the difference between them
without reading documentation.
> ---
> man/systemctl.xml | 11 ++++++
> shell-completion/bash/systemctl | 4 ++
> src/core/dbus-manager.c | 15 +++++++-
> src/shared/install.c | 82 +++++++++++++++++++++++++++++++++++++++++
> src/shared/install.h | 2 +
> src/systemctl/systemctl.c | 39 +++++++++++++++-----
> 6 files changed, 142 insertions(+), 11 deletions(-)
>
> diff --git a/man/systemctl.xml b/man/systemctl.xml
> index 0afb0cc..459444b 100644
> --- a/man/systemctl.xml
> +++ b/man/systemctl.xml
> @@ -998,6 +998,17 @@ kobject-uevent 1 systemd-udevd-kernel.socket systemd-udevd.service
> </varlistentry>
>
> <varlistentry>
> + <term><command>default-target [<replaceable>NAME</replaceable>]</command></term>
> +
> + <listitem>
> + <para>Set/get the default target to boot into. If
> + the name is specified the default.target is linked
> + to the unit file. If the command is used without the name,
> + current default.target is printed.</para>
> + </listitem>
> + </varlistentry>
> +
> + <varlistentry>
> <term><command>load <replaceable>NAME</replaceable>...</command></term>
>
> <listitem>
> diff --git a/shell-completion/bash/systemctl b/shell-completion/bash/systemctl
> index 191b8d1..ff59dbc 100644
> --- a/shell-completion/bash/systemctl
> +++ b/shell-completion/bash/systemctl
> @@ -137,6 +137,7 @@ _systemctl () {
> show-environment suspend'
> [NAME]='snapshot load'
> [FILE]='link'
> + [TARGETS]='default-target'
> )
>
> for ((i=0; $i <= $COMP_CWORD; i++)); do
> @@ -210,6 +211,9 @@ _systemctl () {
> elif __contains_word "$verb" ${VERBS[FILE]}; then
> comps=$( compgen -A file -- "$cur" )
> compopt -o filenames
> + elif __contains_word "$verb" ${VERBS[TARGETS]}; then
> + comps=$( __systemctl $mode list-unit-files --type target --full --all \
> + | { while read -r a b; do echo " $a"; done; } )
> fi
>
> COMPREPLY=( $(compgen -W '$comps' -- "$cur") )
> diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c
> index 56b02a1..22449c5 100644
> --- a/src/core/dbus-manager.c
> +++ b/src/core/dbus-manager.c
> @@ -227,6 +227,13 @@
> " <arg name=\"files\" type=\"as\" direction=\"in\"/>\n" \
> " <arg name=\"runtime\" type=\"b\" direction=\"in\"/>\n" \
> " <arg name=\"changes\" type=\"a(sss)\" direction=\"out\"/>\n" \
> + " </method>\n" \
> + " <method name=\"SetDefaultTarget\">\n" \
> + " <arg name=\"files\" type=\"as\" direction=\"in\"/>\n" \
> + " <arg name=\"changes\" type=\"a(sss)\" direction=\"out\"/>\n" \
> + " </method>\n" \
> + " <method name=\"GetDefaultTarget\">\n" \
> + " <arg name=\"changes\" type=\"a(sss)\" direction=\"out\"/>\n" \
> " </method>\n"
>
> #define BUS_MANAGER_INTERFACE_SIGNALS \
> @@ -1728,7 +1735,9 @@ static DBusHandlerResult bus_manager_message_handler(DBusConnection *connection,
> dbus_message_is_method_call(message, "org.freedesktop.systemd1.Manager", "ReenableUnitFiles") ||
> dbus_message_is_method_call(message, "org.freedesktop.systemd1.Manager", "LinkUnitFiles") ||
> dbus_message_is_method_call(message, "org.freedesktop.systemd1.Manager", "PresetUnitFiles") ||
> - dbus_message_is_method_call(message, "org.freedesktop.systemd1.Manager", "MaskUnitFiles")) {
> + dbus_message_is_method_call(message, "org.freedesktop.systemd1.Manager", "MaskUnitFiles") ||
> + dbus_message_is_method_call(message, "org.freedesktop.systemd1.Manager", "SetDefaultTarget") ||
> + dbus_message_is_method_call(message, "org.freedesktop.systemd1.Manager", "GetDefaultTarget")) {
>
> char **l = NULL;
> DBusMessageIter iter;
> @@ -1771,6 +1780,10 @@ static DBusHandlerResult bus_manager_message_handler(DBusConnection *connection,
> carries_install_info = r;
> } else if (streq(member, "MaskUnitFiles"))
> r = unit_file_mask(scope, runtime, NULL, l, force, &changes, &n_changes);
> + else if (streq(member, "SetDefaultTarget"))
> + r = unit_file_set_default(scope, NULL, l[0], &changes, &n_changes);
> + else if (streq(member, "GetDefaultTarget"))
> + r = unit_file_get_default(scope, NULL, &changes, &n_changes);
> else
> assert_not_reached("Uh? Wrong method");
>
> diff --git a/src/shared/install.c b/src/shared/install.c
> index edf4d2a..a411e62 100644
> --- a/src/shared/install.c
> +++ b/src/shared/install.c
> @@ -1570,6 +1570,88 @@ int unit_file_reenable(
> return r;
> }
>
> +int unit_file_set_default(
> + UnitFileScope scope,
> + const char *root_dir,
> + char *file,
> + UnitFileChange **changes,
> + unsigned *n_changes) {
> +
> + _cleanup_lookup_paths_free_ LookupPaths paths = {};
> + _cleanup_install_context_done_ InstallContext c = {};
> + _cleanup_free_ char *config_path = NULL;
> + _cleanup_free_ char *path = NULL;
> + int r;
> + const char *e = NULL;
> + InstallInfo *i = NULL;
> +
> + assert(scope >= 0);
> + assert(scope < _UNIT_FILE_SCOPE_MAX);
> +
> +
> + e = strrchr(file, '.');
Are you sure that there's always a dot in there? Otherwise we'll get a NULL.
> + if (unit_type_from_string(e + 1) != UNIT_TARGET)
> + return -EINVAL;
> +
> + r = lookup_paths_init_from_scope(&paths, scope);
> + if (r < 0)
> + return r;
> +
> + r = get_config_path(scope, false, root_dir, &config_path);
> + if (r < 0)
> + return r;
> +
> + r = install_info_add_auto(&c, file);
> + if (r < 0)
> + return r;
> +
> + i = (InstallInfo*)hashmap_first(c.will_install);
> +
> + r = unit_file_search(&c, i, &paths, root_dir, false);
> + if (r < 0)
> + return r;
> +
> + if (asprintf(&path, "%s/%s", config_path, "default.target") < 0)
> + return -ENOMEM;
I think we try to use strjoin or strappenda in newer code... strappenda
will likely be inifinitesimally faster, and doesn't require error checking,
so code is shorter.
> + r = create_symlink(i->path, path, true, changes, n_changes);
> + if (r < 0)
> + return r;
> +
> + return 0;
> +
> +
> +}
> +
> +int unit_file_get_default(
> + UnitFileScope scope,
> + const char *root_dir,
> + UnitFileChange **changes,
> + unsigned *n_changes) {
> +
> + _cleanup_free_ char *path;
> + _cleanup_free_ char *config_path;
> + _cleanup_free_ char *dest;
> + int r;
> +
> + r = get_config_path(scope, false, root_dir, &config_path);
> + if (r < 0)
> + return r;
> +
> + if (asprintf(&path, "%s/%s", config_path, "default.target") < 0)
> + return -ENOMEM;
strappenda...
> + r = readlink_and_make_absolute(path, &dest);
> + if (r < 0)
> + return r;
> +
> + r = add_file_change(changes, n_changes, UNIT_FILE_SYMLINK, dest, NULL);
> + if (r < 0)
> + return r;
> +
> + return 0;
> +}
This ignores the fact the target can be specified on the command line. I guess
that's fine, but it should be explicitly mentioned in the man page, that
the output is the default default.target, possibly not the actual one.
Also, src/core/main.c simply uses manager_load_unit to load default.target,
so it'll load from /etc/, /run, etc. Is it enough to just check in one
place?
> UnitFileState unit_file_get_state(
> UnitFileScope scope,
> const char *root_dir,
> diff --git a/src/shared/install.h b/src/shared/install.h
> index 94516c9..1789482 100644
> --- a/src/shared/install.h
> +++ b/src/shared/install.h
> @@ -80,6 +80,8 @@ int unit_file_link(UnitFileScope scope, bool runtime, const char *root_dir, char
> int unit_file_preset(UnitFileScope scope, bool runtime, const char *root_dir, char *files[], bool force, UnitFileChange **changes, unsigned *n_changes);
> int unit_file_mask(UnitFileScope scope, bool runtime, const char *root_dir, char *files[], bool force, UnitFileChange **changes, unsigned *n_changes);
> int unit_file_unmask(UnitFileScope scope, bool runtime, const char *root_dir, char *files[], UnitFileChange **changes, unsigned *n_changes);
> +int unit_file_set_default(UnitFileScope scope, const char *root_dir, char *file, UnitFileChange **changes, unsigned *n_changes);
> +int unit_file_get_default(UnitFileScope scope, const char *root_dir, UnitFileChange **changes, unsigned *n_changes);
>
> UnitFileState unit_file_get_state(UnitFileScope scope, const char *root_dir, const char *filename);
>
> diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
> index 3cca861..8aadf85 100644
> --- a/src/systemctl/systemctl.c
> +++ b/src/systemctl/systemctl.c
> @@ -4214,7 +4214,7 @@ static int enable_unit(DBusConnection *bus, char **args) {
> if (r < 0)
> return r;
>
> - if (!args[1])
> + if (!args[1] && !streq(verb, "default-target"))
> return 0;
>
> if (!bus || avoid_bus()) {
> @@ -4235,7 +4235,15 @@ static int enable_unit(DBusConnection *bus, char **args) {
> r = unit_file_mask(arg_scope, arg_runtime, arg_root, args+1, arg_force, &changes, &n_changes);
> else if (streq(verb, "unmask"))
> r = unit_file_unmask(arg_scope, arg_runtime, arg_root, args+1, &changes, &n_changes);
> - else
> + else if (streq(verb, "default-target")) {
> + if(args[1])
> + r = unit_file_set_default(arg_scope, arg_root, args[1], &changes, &n_changes);
> + else
> + r = unit_file_get_default(arg_scope, arg_root, &changes, &n_changes);
> +
> + if (!args[1] && n_changes > 0)
> + log_info("%s\n", changes[n_changes-1].path);
> + } else
> assert_not_reached("Unknown verb");
>
> if (r < 0) {
> @@ -4243,7 +4251,7 @@ static int enable_unit(DBusConnection *bus, char **args) {
> goto finish;
> }
>
> - if (!arg_quiet) {
> + if (!arg_quiet && args[1]) {
> for (i = 0; i < n_changes; i++) {
> if (changes[i].type == UNIT_FILE_SYMLINK)
> log_info("ln -s '%s' '%s'", changes[i].source, changes[i].path);
> @@ -4278,6 +4286,11 @@ static int enable_unit(DBusConnection *bus, char **args) {
> else if (streq(verb, "unmask")) {
> method = "UnmaskUnitFiles";
> send_force = false;
> + } else if (streq(verb, "default-target")) {
> + if (args[1])
> + method = "SetDefaultTarget";
> + else
> + method = "GetDefaultTarget";
> } else
> assert_not_reached("Unknown verb");
>
> @@ -4294,7 +4307,7 @@ static int enable_unit(DBusConnection *bus, char **args) {
> dbus_message_iter_init_append(m, &iter);
>
> r = mangle_names(args+1, &mangled_names);
> - if(r < 0)
> + if (r < 0)
> goto finish;
>
> r = bus_append_strv_iter(&iter, mangled_names);
> @@ -4369,11 +4382,15 @@ static int enable_unit(DBusConnection *bus, char **args) {
> goto finish;
> }
>
> - if (!arg_quiet) {
> - if (streq(type, "symlink"))
> - log_info("ln -s '%s' '%s'", source, path);
> - else
> - log_info("rm '%s'", path);
> + if (streq(method, "GetDefaultTarget"))
> + log_info("%s", path);
I don't think that log_* should be used for that. In other places normal stdio is
used to output the output.
> + else {
> + if (!arg_quiet) {
> + if (streq(type, "symlink"))
> + log_info("ln -s '%s' '%s'", source, path);
> + else
> + log_info("rm '%s'", path);
> + }
This else + if can be collapsed into 'else if'.
> }
>
> dbus_message_iter_next(&sub);
> @@ -5642,6 +5659,7 @@ static int systemctl_main(DBusConnection *bus, int argc, char *argv[], DBusError
> { "link", MORE, 2, enable_unit },
> { "switch-root", MORE, 2, switch_root },
> { "list-dependencies", LESS, 2, list_dependencies },
> + { "default-target", LESS, 2, enable_unit },
> };
>
> int left;
> @@ -5713,7 +5731,8 @@ static int systemctl_main(DBusConnection *bus, int argc, char *argv[], DBusError
> !streq(verbs[i].verb, "preset") &&
> !streq(verbs[i].verb, "mask") &&
> !streq(verbs[i].verb, "unmask") &&
> - !streq(verbs[i].verb, "link")) {
> + !streq(verbs[i].verb, "link") &&
> + !streq(verbs[i].verb, "default-target")) {
>
> if (running_in_chroot() > 0) {
> log_info("Running in chroot, ignoring request.");
Zbyszek
More information about the systemd-devel
mailing list