[systemd-devel] [PATCH] shutdown: add kexec loading, avoid calling `kexec` binary unnessecarily
Zbigniew Jędrzejewski-Szmek
zbyszek at in.waw.pl
Sun Mar 15 11:31:03 PDT 2015
On Wed, Mar 11, 2015 at 05:22:18PM -0700, Shawn Landden wrote:
> Still use helper when Xen Dom0, to avoid duplicating some hairy code.
>
> I think the rbtree version was far more understandable as greedy_realloc0()
> is very messy to do correctly.
>
> Take fopenat() from lsof.
> Add opendirat()
>
> Future: generate BootLoaderSpec files for other kernel install locations
>
> v5: fix syscall invocation from draft version
> v6: usr either /boot/efi or /boot
> ---
> Makefile.am | 4 +-
> TODO | 3 -
> man/systemctl.xml | 15 ++-
> src/core/shutdown.c | 30 ++++--
> src/shared/fileio.c | 69 +++++++++++++
> src/shared/fileio.h | 5 +
> src/shared/missing.h | 11 +++
> src/shared/rpmvercmp.c | 26 +++--
> src/shared/strv.c | 9 +-
> src/systemctl/bootspec.c | 247 ++++++++++++++++++++++++++++++++++++++++++++++
> src/systemctl/bootspec.h | 48 +++++++++
> src/systemctl/systemctl.c | 57 ++++++++++-
> 12 files changed, 495 insertions(+), 29 deletions(-)
> create mode 100644 src/systemctl/bootspec.c
> create mode 100644 src/systemctl/bootspec.h
>
> diff --git a/Makefile.am b/Makefile.am
> index 353a7de..20a6484 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -2736,7 +2736,9 @@ systemd_escape_LDADD = \
>
> # -----------------------------------------------------------------------------
> systemctl_SOURCES = \
> - src/systemctl/systemctl.c
> + src/systemctl/systemctl.c \
> + src/systemctl/bootspec.c \
> + src/systemctl/bootspec.h
>
> systemctl_LDADD = \
> libsystemd-units.la \
> diff --git a/TODO b/TODO
> index 6180077..9ba7be0 100644
> --- a/TODO
> +++ b/TODO
> @@ -86,9 +86,6 @@ Features:
> * maybe introduce WantsMountsFor=? Usecase:
> http://lists.freedesktop.org/archives/systemd-devel/2015-January/027729.html
>
> -* rework kexec logic to use new kexec_file_load() syscall, so that we
> - don't have to call kexec tool anymore.
> -
> * The udev blkid built-in should expose a property that reflects
> whether media was sensed in USB CF/SD card readers. This should then
> be used to control SYSTEMD_READY=1/0 so that USB card readers aren't
> diff --git a/man/systemctl.xml b/man/systemctl.xml
> index 338c1d3..c550339 100644
> --- a/man/systemctl.xml
> +++ b/man/systemctl.xml
> @@ -607,6 +607,15 @@ kobject-uevent 1 systemd-udevd-kernel.socket systemd-udevd.service
> </varlistentry>
>
> <varlistentry>
> + <term><command>list-kernels</command></term>
> +
> + <listitem>
> + <para>List kernels ordered by version.
> + </para>
> + </listitem>
> + </varlistentry>
> +
> + <varlistentry>
> <term><command>start <replaceable>PATTERN</replaceable>...</command></term>
>
> <listitem>
> @@ -1550,7 +1559,7 @@ kobject-uevent 1 systemd-udevd-kernel.socket systemd-udevd.service
> </varlistentry>
>
> <varlistentry>
> - <term><command>kexec</command></term>
> + <term><command>kexec <optional><replaceable>VERSION</replaceable></optional><optional><replaceable>KERNEL-ARG</replaceable>...</optional></command></term>
>
> <listitem>
> <para>Shut down and reboot the system via kexec. This is
> @@ -1560,6 +1569,10 @@ kobject-uevent 1 systemd-udevd-kernel.socket systemd-udevd.service
> services is skipped, however all processes are killed and
> all file systems are unmounted or mounted read-only,
> immediately followed by the reboot.</para>
> +
> + <para>Also allows specifying the version and optionally
> + extra kernel parameters to append. If no version is specified
> + then the most recent kernel is booted.</para>
> </listitem>
> </varlistentry>
>
> diff --git a/src/core/shutdown.c b/src/core/shutdown.c
> index 70a461e..616a70a 100644
> --- a/src/core/shutdown.c
> +++ b/src/core/shutdown.c
> @@ -19,6 +19,7 @@
> along with systemd; If not, see <http://www.gnu.org/licenses/>.
> ***/
>
> +#include <ctype.h>
> #include <sys/mman.h>
> #include <sys/reboot.h>
> #include <linux/reboot.h>
> @@ -173,15 +174,21 @@ int main(int argc, char *argv[]) {
> goto error;
> }
>
> + in_container = !!detect_virtualization(NULL);
> +
> if (streq(arg_verb, "reboot"))
> cmd = RB_AUTOBOOT;
> else if (streq(arg_verb, "poweroff"))
> cmd = RB_POWER_OFF;
> else if (streq(arg_verb, "halt"))
> cmd = RB_HALT_SYSTEM;
> - else if (streq(arg_verb, "kexec"))
> - cmd = LINUX_REBOOT_CMD_KEXEC;
> - else {
> + else if (streq(arg_verb, "kexec")) {
> + if (in_container) {
> + log_warning("Can't kexec from container. Rebooting…");
> + cmd = RB_AUTOBOOT;
> + } else
> + cmd = LINUX_REBOOT_CMD_KEXEC;
> + } else {
> r = -EINVAL;
> log_error("Unknown action '%s'.", arg_verb);
> goto error;
> @@ -200,8 +207,6 @@ int main(int argc, char *argv[]) {
> log_info("Sending SIGKILL to remaining processes...");
> broadcast_signal(SIGKILL, true, false);
>
> - in_container = detect_container(NULL) > 0;
> -
> need_umount = !in_container;
> need_swapoff = !in_container;
> need_loop_detach = !in_container;
> @@ -341,11 +346,14 @@ int main(int argc, char *argv[]) {
>
> case LINUX_REBOOT_CMD_KEXEC:
>
> - if (!in_container) {
> - /* We cheat and exec kexec to avoid doing all its work */
> - pid_t pid;
> + log_info("Rebooting with kexec.");
>
> - log_info("Rebooting with kexec.");
> + /* kexec-tools has a bunch of special code to make Xen Dom0 work,
> + * otherwise it is only doing stuff we have already done.
> + * This is true for Dom0 and DomU but we only get Dom0
> + * because of the !in_container check */
> + if (access("/proc/xen", F_OK) == 0) {
> + pid_t pid;
>
> pid = fork();
> if (pid < 0)
> @@ -362,7 +370,9 @@ int main(int argc, char *argv[]) {
> _exit(EXIT_FAILURE);
> } else
> wait_for_terminate_and_warn("kexec", pid, true);
> - }
> +
> + } else
> + reboot(cmd);
>
> cmd = RB_AUTOBOOT;
> /* Fall through */
> diff --git a/src/shared/fileio.c b/src/shared/fileio.c
> index ff6b1a7..0b12dce 100644
> --- a/src/shared/fileio.c
> +++ b/src/shared/fileio.c
> @@ -815,3 +815,72 @@ int get_status_field(const char *filename, const char *pattern, char **field) {
>
> return 0;
> }
> +
> +DIR *opendirat(int dirfd, const char *name) {
> + int fd;
> +
> + fd = openat(dirfd, name, O_DIRECTORY);
> + if (fd < 0)
> + return NULL; /*errno is set */
> +
> + return fdopendir(fd);
> +}
> +
> +/*
> + * Copyright 1997 Purdue Research Foundation, West Lafayette, Indiana
> + * 47907. All rights reserved.
> + *
> + * Written by Victor A. Abell
> + *
> + * This software is not subject to any license of the American Telephone
> + * and Telegraph Company or the Regents of the University of California.
> + *
> + * Permission is granted to anyone to use this software for any purpose on
> + * any computer system, and to alter it and redistribute it freely, subject
> + * to the following restrictions:
> + *
> + * 1. Neither the authors nor Purdue University are responsible for any
> + * consequences of the use of this software.
> + *
> + * 2. The origin of this software must not be misrepresented, either by
> + * explicit claim or by omission. Credit to the authors and Purdue
> + * University must appear in documentation and sources.
> + *
> + * 3. Altered versions must be plainly marked as such, and must not be
> + * misrepresented as being the original software.
> + *
> + * 4. This notice may not be removed or altered.
> + */
> +
> +static int fopen_to_open(const char *m) {
> + int flags;
> +
> + if (strchr(m, '+'))
> + flags = O_RDWR;
> + else if (m[0] == 'r')
> + flags = O_RDONLY;
> + else if (m[0] == 'w' || m[0] == 'a')
> + flags = O_WRONLY;
> + else
> + return(0);
> +
> + if (m[0] == 'a')
> + flags |= O_APPEND|O_CREAT;
> + if (m[0] == 'w')
> + flags |= O_TRUNC|O_CREAT;
> +
> + flags |= O_NONBLOCK;
> +
> + return flags;
> +}
I really dislike this part. The open() interface is so much better
than fopen().
> +FILE *fopenat(int dirfd, const char *path, const char *mode) {
> + int fd;
> + int flags = fopen_to_open(mode);
> +
> + fd = openat(dirfd, path, flags);
> + if (fd < 0)
> + return NULL;
> +
> + return fdopen(fd, mode);
> +}
> diff --git a/src/shared/fileio.h b/src/shared/fileio.h
> index 5ae51c1..df174e2 100644
> --- a/src/shared/fileio.h
> +++ b/src/shared/fileio.h
> @@ -22,6 +22,8 @@
> ***/
> #include <stddef.h>
> #include <stdio.h>
> +#include <sys/types.h>
> +#include <dirent.h>
>
> #include "macro.h"
>
> @@ -43,3 +45,6 @@ int write_env_file(const char *fname, char **l);
> int executable_is_script(const char *path, char **interpreter);
>
> int get_status_field(const char *filename, const char *pattern, char **field);
> +
> +DIR *opendirat(int dirfd, const char *name);
> +FILE *fopenat(int dirfd, const char *path, const char *mode);
> diff --git a/src/shared/missing.h b/src/shared/missing.h
> index 802b495..4416a51 100644
> --- a/src/shared/missing.h
> +++ b/src/shared/missing.h
> @@ -36,6 +36,7 @@
> #include <linux/audit.h>
> #include <linux/capability.h>
> #include <linux/neighbour.h>
> +#include <linux/kexec.h>
>
> #ifdef HAVE_AUDIT
> #include <libaudit.h>
> @@ -789,3 +790,13 @@ static inline int kcmp(pid_t pid1, pid_t pid2, int type, unsigned long idx1, uns
> #ifndef KCMP_FILE
> #define KCMP_FILE 0
> #endif
> +
> +/* v3.17 */
> +#ifndef __NR_kexec_file_load
> +#ifdef __x86_64__
> +#define __NR_kexec_file_load 320
> +#endif
> +#endif
> +#ifndef KEXEC_FILE_NO_INITRAMFS
> +#define KEXEC_FILE_NO_INITRAMFS 0x00000004
> +#endif
> diff --git a/src/shared/rpmvercmp.c b/src/shared/rpmvercmp.c
> index c69c2e3..1649929 100644
> --- a/src/shared/rpmvercmp.c
> +++ b/src/shared/rpmvercmp.c
> @@ -13,18 +13,26 @@
> /* return 1: a is newer than b */
> /* 0: a and b are the same version */
> /* -1: b is newer than a */
> -int rpmvercmp(const char * a, const char * b)
> -{
> +int rpmvercmp(const char * a, const char * b) {
> + char oldch1, oldch2;
> + char abuf[strlen(a)+1], bbuf[strlen(b)+1];
> + char *str1 = abuf, *str2 = bbuf;
> + char * one, * two;
> + int rc;
> + int isnum;
> +
> + if (!a) {
> + if (b)
> + return -1;
> + else
> + return 0;
> + }
> + if (!b)
> + return 1;
> +
> /* easy comparison to see if versions are identical */
> if (streq_ptr(a, b)) return 0;
>
> - char oldch1, oldch2;
> - char abuf[strlen(a)+1], bbuf[strlen(b)+1];
> - char *str1 = abuf, *str2 = bbuf;
> - char * one, * two;
> - int rc;
> - int isnum;
> -
> strcpy(str1, a);
> strcpy(str2, b);
>
> diff --git a/src/shared/strv.c b/src/shared/strv.c
> index ee45ad1..ed28b95 100644
> --- a/src/shared/strv.c
> +++ b/src/shared/strv.c
> @@ -81,7 +81,14 @@ void strv_clear(char **l) {
> }
>
> void strv_free(char **l) {
> - strv_clear(l);
> + char **k;
> +
> + if (!l)
> + return;
> +
> + for (k = l; *k; k++)
> + free(*k);
> +
> free(l);
> }
>
> diff --git a/src/systemctl/bootspec.c b/src/systemctl/bootspec.c
> new file mode 100644
> index 0000000..8194eaf
> --- /dev/null
> +++ b/src/systemctl/bootspec.c
> @@ -0,0 +1,247 @@
> +/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
> +
> +/***
> + This file is part of systemd.
> +
> + Copyright 2015 Shawn Landden
> +
> + systemd is free software; you can redistribute it and/or modify it
> + under the terms of the GNU Lesser General Public License as published by
> + the Free Software Foundation; either version 2.1 of the License, or
> + (at your option) any later version.
> +
> + systemd is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public License
> + along with systemd; If not, see <http://www.gnu.org/licenses/>.
> +***/
> +
> +/*
> + * Implements http://freedesktop.org/wiki/Specifications/BootLoaderSpec/
> + * for use with kexec
> + */
> +
> +#include <ctype.h>
> +#include <dirent.h>
> +#include <sys/utsname.h>
> +#include <linux/kexec.h>
> +
> +#include "bootspec.h"
> +#include "strv.h"
> +#include "fileio.h"
> +#include "rpmvercmp.h"
> +
> +void bootspec_free(struct BootSpec *s) {
> + if (!s)
> + return;
> +
> + free(s->conf);
> + free(s);
> +}
> +
> +static int bootspec_cmp(const void *left, const void *right) {
> + const struct BootSpec *l = left,
> + *r = right;
> +
> + /* reverse sort to put highest version first */
> + return rpmvercmp(r->version, l->version);
> +}
> +
> +int bootspec_getkernels(struct BootSpec **ret, size_t *cl, int *bootdir_fd_ret) {
> + int r = 0;
> + struct BootSpec *c;
> + _cleanup_closedir_ DIR *d = NULL;
> + struct dirent *dent;
> + size_t i = 1, ii = 2;
> + _cleanup_close_ int midfd = -1;
> + int bootdir_fd = -1;
> + ssize_t ss;
> + char mid_char[32 + 1];
> + sd_id128_t machine_id;
> +
> + assert(ret);
> + assert(cl);
> +
> + /* we assume that the EFI partition is mounted
> + * and at /boot/efi or /boot
> + */
> + errno = 0;
> + if (access("/boot/efi/loader/entries", R_OK|X_OK) == 0)
> + bootdir_fd = open("/boot/efi", O_DIRECTORY);
> + else if (access("/boot/loader/entries", R_OK|X_OK) == 0)
> + bootdir_fd = open("/boot", O_DIRECTORY);
> + if (bootdir_fd < 0)
> + return IN_SET(errno, 0, ENOENT) ? 0 : -errno;
It seems that error reporting has to be moved into this function.
You really want to be able to say "opening "/boot/efi/loader/entries" failed: %m"
rathen then print a generic error from the calling funtion.
With error reporting moved to this function...
Playing games with errno like this is just asking for trouble.
What about:
NULSTR_FOREACH(dirname, "/boot/efi\0"
"/boot\0") {
dir = open(dirname, O_DIRECTORY);
if (dir < 0 && errno != ENOENT)
return return log_error_errno(errno, "Opening \"%s\" failed: %m", dirname);
if (dir >= 0) {
entries = openat(dir, "loader/entries", O_DIRECTORY);
if (entries >= 0)
break;
if (errno != ENOENT)
return return log_error_errno(errno, "Opening /boot/efi failed: %m");
else
dir = safe_close(dir);
}
}
if (entries < 0)
return log_error_errno(ENOENT, "Neither /boot/efi not /boot ... blah");
It seems nicer to use open everything at once, without using access()
and effectively checking twice.
> + midfd = open("/etc/machine-id", O_RDONLY);
> + if (midfd < 0)
> + return -errno;
> + ss = read(midfd, &mid_char, sizeof(mid_char));
> + if (ss < 0)
> + return -errno;
> + else if (ss != 33)
> + return -EBADF;
> + if (mid_char[32] == '\n')
> + mid_char[32] = '\0';
> + else
> + return -EBADF;
> + r = sd_id128_from_string(mid_char, &machine_id);
> + if (r < 0)
> + return r;
Isn't this what sd_id128_get_machine does?
> +
> + c = new0(struct BootSpec, i + 1);
> + if (!c)
> + return -ENOMEM;
Replace this with
struct BootSpec *specs = NULL;
size_t allocated = 0;
int count = 0;
> + d = opendirat(bootdir_fd, "loader/entries");
> + if (!d) {
> + if (errno == ENOENT)
> + return 0;
> + else
> + return -errno;
> + }
> +
> + for (dent = readdir(d); dent != NULL; dent = readdir(d)) {
> + struct BootSpec *bs;
... and this ...
> + char *m, *l, *k;
> + _cleanup_fclose_ FILE *f = NULL;
> +
> + if (!endswith(dent->d_name, ".conf"))
> + continue;
> +
> + c = greedy_realloc0((void **)&c, &ii, i + 2, sizeof(struct BootSpec));
> + if (!c)
> + return -ENOMEM;
> + bs = &c[i - 1];
... and here use
if (!GREEDY_REALLOC(specs, allocated, count + 1))
return log_oom();
and then specs[count] refers to the new entry you can now fill in.
> + f = fopenat(dirfd(d), dent->d_name, "r");
> + if (!f)
> + return -errno;
> +
I already mentioned this before: reading in of a single entry should
be a separate function. For readability and testability.
> + r = read_full_stream(f, &bs->conf, NULL);
> + if (r < 0)
> + return r;
> +
> + for (m = bs->conf; ; m = k + 1) {
> + if (m[0] == '#')
> + continue;
> +
> + k = strchr(m, '\n');
> +
> + if (k)
> + *k = '\0';
> + else
> + break;
> +
> + if ((l = startswith(m, "title ")))
> + bs->title = l + strspn(l, WHITESPACE);
> + else if ((l = startswith(m, "version ")))
> + bs->version = l + strspn(l, WHITESPACE);
> + else if ((l = startswith(m, "machine-id ")))
> + (void)sd_id128_from_string(l + strspn(l, WHITESPACE), &bs->machine_id);
> + else if ((l = startswith(m, "options ")))
> + bs->options = l + strspn(l, WHITESPACE);
> + else if ((l = startswith(m, "linux ")))
> + bs->linux_loc = l + strspn(l, WHITESPACE);
> + else if ((l = startswith(m, "initrd ")))
> + bs->initrd = l + strspn(l, WHITESPACE);
> + else if ((l = startswith(m, "efi ")))
> + bs->efi = l + strspn(l, WHITESPACE);
> + else if ((l = startswith(m, "devicetree ")))
> + bs->devicetree = l + strspn(l, WHITESPACE);
> + else
> + continue;
> + }
> +
> + /* not interested in EFI programs or kernels for other roots */
> + if (!bs->linux_loc || !sd_id128_equal(bs->machine_id, machine_id)) {
> + free(bs->conf);
> + zero(bs);
> + continue;
> + }
> +
> + i++;
> + }
> +
> + i--;
> +
> + qsort(c, i, sizeof(struct BootSpec), bootspec_cmp);
qsort_safe
> + *cl = i;
> + *ret = c;
> + if (bootdir_fd_ret)
> + *bootdir_fd_ret = bootdir_fd;
> +
> + return r;
> +}
> +
> +int kernel_load(char *version, char *append_cmdline, bool overwrite) {
> + int r = -ENOTSUP;
David replaced all ENOTSUP with EOPNOTSUPP recently.
> +
> +/* only x86_64 and x32 in 3.18 */
> +#ifdef __NR_kexec_file_load
> + if (!overwrite && !kexec_loaded()) {
> + struct utsname u;
> + char *cmdline = NULL;
> + _cleanup_close_ int vmlinuz_fd = -1, initrd_fd = -1, bootdir = -1;
> + struct BootSpec *a = NULL;
> + size_t al = 0;
> + struct BootSpec *c;
> +
> + r = uname(&u);
> + if (r < 0)
> + return -errno;
> +
> + r = bootspec_getkernels(&a, &al, &bootdir);
> + if (r < 0)
> + return r;
Loading the list of kernels should not be done from inside of the function
to load a single kernel... Please split out the function to load
a kernel based on a struct BootSpec out.
> + if (version) {
> + struct BootSpec q;
> +
> + q.version = version;
> + c = bsearch(&q, a, al, sizeof(struct BootSpec), bootspec_cmp);
> + if (!c)
> + return -ENOENT;
> + } else
> + c = &a[0];
> +
> + cmdline = strjoina(c->options, " ", append_cmdline);
> +
> + vmlinuz_fd = openat(bootdir, c->linux_loc, O_RDONLY);
> + if (vmlinuz_fd < 0)
> + return -errno;
> +
> + if (c->initrd) {
> + initrd_fd = openat(bootdir, c->initrd, O_RDONLY);
> + if (initrd_fd < 0)
> + return -errno;
> + }
> +
> + if (initrd_fd < 0)
> + log_info("kexec: kexec -l %s --command-line=\"%s\"",
> + c->linux_loc,
> + cmdline);
> + else
> + log_info("kexec: kexec -l %s --initrd=%s --command-line=\"%s\"",
> + c->linux_loc,
> + c->initrd,
> + cmdline);
> +
> + r = syscall(__NR_kexec_file_load, vmlinuz_fd, initrd_fd,
> + (unsigned long) strlen(cmdline) + 1, cmdline, initrd_fd < 0 ? KEXEC_FILE_NO_INITRAMFS : 0);
> + if (r < 0)
> + return -errno;
> +
> + /* free array */
> + for (unsigned i=0;i<al;i++)
> + free((a[i]).conf);
> +
> + free(a);
> + } else
> + r = 0;
> +#endif
> + return r;
> +}
> diff --git a/src/systemctl/bootspec.h b/src/systemctl/bootspec.h
> new file mode 100644
> index 0000000..d8893a0
> --- /dev/null
> +++ b/src/systemctl/bootspec.h
> @@ -0,0 +1,48 @@
> +/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
> +
> +#pragma once
> +
> +/***
> + This file is part of systemd.
> +
> + Copyright 2015 Shawn Landden
> +
> + systemd is free software; you can redistribute it and/or modify it
> + under the terms of the GNU Lesser General Public License as published by
> + the Free Software Foundation; either version 2.1 of the License, or
> + (at your option) any later version.
> +
> + systemd is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public License
> + along with systemd; If not, see <http://www.gnu.org/licenses/>.
> +***/
> +
> +#include <stdbool.h>
> +#include <errno.h>
> +
> +#include "sd-id128.h"
> +
> +struct BootSpec {
> + /* The others are just pointers into malloc()ed conf */
> + char *conf;
> +
All those below should be const char* probably.
> + char *title;
> + char *version;
> + sd_id128_t machine_id;
> + char *options;
> + char *linux_loc; /* linux is a reserved keyword with gcc! (and clang!)
> + https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65128 */
> + char *initrd;
> + char *efi;
> + char *devicetree;
> +};
> +
> +void bootspec_free(struct BootSpec *l);
> +
> +/* returns an array of struct BootSpec */
> +int bootspec_getkernels(struct BootSpec **a, size_t *l, int *bootdir_fd_ret);
> +int kernel_load(char *version, char *append_cmdline, bool overwrite);
> diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
> index 41f7b9f..1f283c8 100644
> --- a/src/systemctl/systemctl.c
> +++ b/src/systemctl/systemctl.c
> @@ -68,6 +68,7 @@
> #include "bus-common-errors.h"
> #include "mkdir.h"
> #include "dropin.h"
> +#include "bootspec.h"
>
> static char **arg_types = NULL;
> static char **arg_states = NULL;
> @@ -222,6 +223,27 @@ static int translate_bus_error_to_exit_status(int r, const sd_bus_error *error)
> return EXIT_FAILURE;
> }
>
> +static int list_kernels(sd_bus *bus, char **args) {
> + int r;
> + _cleanup_free_ struct BootSpec *a = NULL;
> + size_t al = 0;
> + struct BootSpec *b;
> +
> + r = bootspec_getkernels(&a, &al, NULL);
> + if (r < 0) {
> + log_error_errno(r, "Failed to enumerate boot entries: %m");
> + return r;
> + }
> +
> + for (unsigned i=0;i<al;i++) {
We use the-olde-style variable declarations on top of block.
Also please call the number of entries "count". And the array
"specs" or so.
And spaces: for (i = 0; i < count; i++)
> + b = &a[i];
> + printf("%s - %s\n", b->version, b->title);
Why this indirection? You can just say:
specs[i].version, specs[i].title
> + free(b->conf);
Please don't mix printing and freeing. Since freeing is needed in two places,
add bootspec_kernel_list_free() function, and preferably call it from _cleanup_.
> static void warn_wall(enum action a) {
> static const char *table[_ACTION_MAX] = {
> [ACTION_HALT] = "The system is going down for system halt NOW!",
> @@ -2934,10 +2956,24 @@ static int start_special(sd_bus *bus, char **args) {
> return r;
> }
>
> + if (a == ACTION_KEXEC) {
> + char *cmd_append = NULL;
> +
> + if (args[1])
> + cmd_append = strv_join(&args[2], " ");
args + 2, please.
> + r = kernel_load(args[1], cmd_append, false);
> + if (r < 0) {
> + log_error_errno(r, "Failed to load kernel: %m");
> + return r;
> + }
> + }
> +
Zbyszek
More information about the systemd-devel
mailing list