[PATCH v4 0/1] Add support for grub-emu to kexec Linux menu entries

[PATCH v4 0/1] Add support for grub-emu to kexec Linux menu entries

[Robbie Harwood]

Address feedback from Raymund's review.  Interdiff attached.

Be well,
--Robbie

Raymund Will (1):
  Add support for grub-emu to kexec Linux menu entries

 grub-core/Makefile.am        |   1 +
 grub-core/Makefile.core.def  |   2 +-
 grub-core/kern/emu/main.c    |   4 +
 grub-core/kern/emu/misc.c    |  18 +++-
 grub-core/loader/emu/linux.c | 181 +++++++++++++++++++++++++++++++++++
 include/grub/emu/exec.h      |   4 +-
 include/grub/emu/hostfile.h  |   3 +-
 include/grub/emu/misc.h      |   3 +
 8 files changed, 212 insertions(+), 4 deletions(-)
 create mode 100644 grub-core/loader/emu/linux.c

Interdiff against v3:
diff --git a/grub-core/kern/emu/main.c b/grub-core/kern/emu/main.c
index b4da9d21cf..855b11c3de 100644
--- a/grub-core/kern/emu/main.c
+++ b/grub-core/kern/emu/main.c
@@ -107,7 +107,7 @@ static struct argp_option options[] = {
    N_("use GRUB files in the directory DIR [default=%s]"), 0},
   {"verbose",     'v', 0,      0, N_("print verbose messages."), 0},
   {"hold",     'H', N_("SECS"),      OPTION_ARG_OPTIONAL, N_("wait until a debugger will attach"), 0},
-  {"kexec",       'X', 0,      0, N_("use kexec to boot Linux kernels."), 0},
+  {"kexec",       'X', 0,      0, N_("use kexec to boot Linux kernels via systemctl (pass twice to enable dangerous fallback to non-systemctl)."), 0},
   { 0, 0, 0, 0, 0, 0 }
 };
 
diff --git a/grub-core/loader/emu/linux.c b/grub-core/loader/emu/linux.c
index b5ea48a677..bdcdbb0ff4 100644
--- a/grub-core/loader/emu/linux.c
+++ b/grub-core/loader/emu/linux.c
@@ -77,9 +77,10 @@ grub_linux_boot (void)
   rc = grub_util_exec (systemctl);
 
   if (kexecute == 1)
-    grub_error (rc, N_("Error trying to perform 'systemctl kexec'"));
+    grub_fatal (N_("Error trying to perform 'systemctl kexec'"));
 
-  /* need to check read-only root before resetting hard!? */
+  /* WARNING: forcible reset should only be used in read-only environments.
+   * grub-emu cannot check for these - users beware. */
   grub_dprintf ("linux", "Performing 'kexec -e -x'");
   kexec[1] = "-e";
   kexec[2] = "-x";
-- 
2.35.1

[PATCH v4 1/1] Add support for grub-emu to kexec Linux menu entries

[Robbie Harwood]

From: Raymund Will <rw@suse.com>

The GRUB emulator is used as a debugging utility but it could also be
used as a user-space bootloader if there is support to boot an operating
system.

The Linux kernel is already able to (re)boot another kernel via the
kexec boot mechanism.  So the grub-emu tool could rely on this feature
and have linux and initrd commands that are used to pass a kernel,
initramfs image and command line parameters to kexec for booting a
selected menu entry.

By default the systemctl kexec option is used so systemd can shutdown
all of the running services before doing a reboot using kexec.  But if
this is not present, it can fall back to executing the kexec user-space
tool directly.  The ability to force a kexec-reboot when systemctl kexec
fails must only be used in controlled environments to avoid possible
filesystem corruption and data loss.

Signed-off-by: Raymund Will <rw@suse.com>
Signed-off-by: John Jolly <jjolly@suse.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 grub-core/Makefile.am        |   1 +
 grub-core/Makefile.core.def  |   2 +-
 grub-core/kern/emu/main.c    |   4 +
 grub-core/kern/emu/misc.c    |  18 +++-
 grub-core/loader/emu/linux.c | 181 +++++++++++++++++++++++++++++++++++
 include/grub/emu/exec.h      |   4 +-
 include/grub/emu/hostfile.h  |   3 +-
 include/grub/emu/misc.h      |   3 +
 8 files changed, 212 insertions(+), 4 deletions(-)
 create mode 100644 grub-core/loader/emu/linux.c

diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index ee88e44e97..80e7a83edf 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -307,6 +307,7 @@ KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/net.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostdisk.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostfile.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/extcmd.h
+KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/exec.h
 if COND_GRUB_EMU_SDL
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/sdl.h
 endif
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 7159948721..5350408601 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1816,9 +1816,9 @@ module = {
   arm64 = loader/arm64/linux.c;
   riscv32 = loader/riscv/linux.c;
   riscv64 = loader/riscv/linux.c;
+  emu = loader/emu/linux.c;
   common = loader/linux.c;
   common = lib/cmdline.c;
-  enable = noemu;
 };
 
 module = {
diff --git a/grub-core/kern/emu/main.c b/grub-core/kern/emu/main.c
index 44e087e988..855b11c3de 100644
--- a/grub-core/kern/emu/main.c
+++ b/grub-core/kern/emu/main.c
@@ -107,6 +107,7 @@ static struct argp_option options[] = {
    N_("use GRUB files in the directory DIR [default=%s]"), 0},
   {"verbose",     'v', 0,      0, N_("print verbose messages."), 0},
   {"hold",     'H', N_("SECS"),      OPTION_ARG_OPTIONAL, N_("wait until a debugger will attach"), 0},
+  {"kexec",       'X', 0,      0, N_("use kexec to boot Linux kernels via systemctl (pass twice to enable dangerous fallback to non-systemctl)."), 0},
   { 0, 0, 0, 0, 0, 0 }
 };
 
@@ -164,6 +165,9 @@ argp_parser (int key, char *arg, struct argp_state *state)
     case 'v':
       verbosity++;
       break;
+    case 'X':
+      grub_util_set_kexecute ();
+      break;
 
     case ARGP_KEY_ARG:
       {
diff --git a/grub-core/kern/emu/misc.c b/grub-core/kern/emu/misc.c
index d0e7a107e7..521220b49d 100644
--- a/grub-core/kern/emu/misc.c
+++ b/grub-core/kern/emu/misc.c
@@ -39,6 +39,7 @@
 #include <grub/emu/misc.h>
 
 int verbosity;
+int kexecute;
 
 void
 grub_util_warn (const char *fmt, ...)
@@ -82,7 +83,7 @@ grub_util_error (const char *fmt, ...)
   vfprintf (stderr, fmt, ap);
   va_end (ap);
   fprintf (stderr, ".\n");
-  exit (1);
+  grub_exit ();
 }
 
 void *
@@ -153,6 +154,9 @@ xasprintf (const char *fmt, ...)
 void
 grub_exit (void)
 {
+#if defined (GRUB_KERNEL)
+  grub_reboot ();
+#endif
   exit (1);
 }
 #endif
@@ -214,3 +218,15 @@ grub_util_load_image (const char *path, char *buf)
 
   fclose (fp);
 }
+
+void
+grub_util_set_kexecute (void)
+{
+  kexecute++;
+}
+
+int
+grub_util_get_kexecute (void)
+{
+  return kexecute;
+}
diff --git a/grub-core/loader/emu/linux.c b/grub-core/loader/emu/linux.c
new file mode 100644
index 0000000000..bdcdbb0ff4
--- /dev/null
+++ b/grub-core/loader/emu/linux.c
@@ -0,0 +1,181 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2006,2007,2008,2009,2010  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB 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 General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/loader.h>
+#include <grub/dl.h>
+#include <grub/command.h>
+#include <grub/time.h>
+
+#include <grub/emu/exec.h>
+#include <grub/emu/hostfile.h>
+#include <grub/emu/misc.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+static grub_dl_t my_mod;
+
+static char *kernel_path;
+static char *initrd_path;
+static char *boot_cmdline;
+
+static grub_err_t
+grub_linux_boot (void)
+{
+  grub_err_t rc = GRUB_ERR_NONE;
+  char *initrd_param;
+  const char *kexec[] = { "kexec", "-la", kernel_path, boot_cmdline, NULL, NULL };
+  const char *systemctl[] = { "systemctl", "kexec", NULL };
+  int kexecute = grub_util_get_kexecute ();
+
+  if (initrd_path)
+    {
+      initrd_param = grub_xasprintf ("--initrd=%s", initrd_path);
+      kexec[3] = initrd_param;
+      kexec[4] = boot_cmdline;
+    }
+  else
+    {
+      initrd_param = grub_xasprintf ("%s", "");
+    }
+
+  grub_dprintf ("linux", "%serforming 'kexec -la %s %s %s'\n",
+                (kexecute) ? "P" : "Not p",
+                kernel_path, initrd_param, boot_cmdline);
+
+  if (kexecute)
+    rc = grub_util_exec (kexec);
+
+  grub_free(initrd_param);
+
+  if (rc != GRUB_ERR_NONE)
+    {
+      grub_error (rc, N_("Error trying to perform kexec load operation."));
+      grub_sleep (3);
+      return rc;
+    }
+
+  if (kexecute < 1)
+    grub_fatal (N_("Use '"PACKAGE"-emu --kexec' to force a system restart."));
+
+  grub_dprintf ("linux", "Performing 'systemctl kexec' (%s) ",
+		(kexecute==1) ? "do-or-die" : "just-in-case");
+  rc = grub_util_exec (systemctl);
+
+  if (kexecute == 1)
+    grub_fatal (N_("Error trying to perform 'systemctl kexec'"));
+
+  /* WARNING: forcible reset should only be used in read-only environments.
+   * grub-emu cannot check for these - users beware. */
+  grub_dprintf ("linux", "Performing 'kexec -e -x'");
+  kexec[1] = "-e";
+  kexec[2] = "-x";
+  kexec[3] = NULL;
+  rc = grub_util_exec (kexec);
+  if ( rc != GRUB_ERR_NONE )
+    grub_fatal (N_("Error trying to directly perform 'kexec -e'."));
+
+  return rc;
+}
+
+static grub_err_t
+grub_linux_unload (void)
+{
+  grub_dl_unref (my_mod);
+  if ( boot_cmdline != NULL )
+    grub_free (boot_cmdline);
+  boot_cmdline = NULL;
+  return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)), int argc, char *argv[])
+{
+  int i;
+  char *tempstr;
+
+  grub_dl_ref (my_mod);
+
+  if (argc == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+
+  if ( !grub_util_is_regular (argv[0]) )
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find kernel file %s"), argv[0]);
+
+  if ( kernel_path != NULL )
+    grub_free (kernel_path);
+
+  kernel_path = grub_xasprintf ("%s", argv[0]);
+
+  if ( boot_cmdline != NULL )
+    {
+      grub_free(boot_cmdline);
+      boot_cmdline = NULL;
+    }
+
+  if ( argc > 1 )
+    {
+      boot_cmdline = grub_xasprintf("--command-line=%s", argv[1]);
+      for ( i = 2; i < argc; i++ )
+        {
+          tempstr = grub_xasprintf("%s %s", boot_cmdline, argv[i]);
+          grub_free(boot_cmdline);
+          boot_cmdline = tempstr;
+        }
+    }
+
+  grub_loader_set (grub_linux_boot, grub_linux_unload, 0);
+
+  return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), int argc, char *argv[])
+{
+  if (argc == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+
+  if ( !grub_util_is_regular (argv[0]) )
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find initrd file %s"), argv[0]);
+
+  if ( initrd_path != NULL )
+    grub_free (initrd_path);
+
+  initrd_path = grub_xasprintf("%s", argv[0]);
+
+  grub_dl_unref (my_mod);
+
+  return GRUB_ERR_NONE;
+}
+
+static grub_command_t cmd_linux, cmd_initrd;
+
+GRUB_MOD_INIT (linux)
+{
+  cmd_linux = grub_register_command ("linux", grub_cmd_linux, 0, N_("Load Linux."));
+  cmd_initrd = grub_register_command ("initrd", grub_cmd_initrd, 0, N_("Load initrd."));
+  my_mod = mod;
+  kernel_path = NULL;
+  initrd_path = NULL;
+  boot_cmdline = NULL;
+}
+
+GRUB_MOD_FINI (linux)
+{
+  grub_unregister_command (cmd_linux);
+  grub_unregister_command (cmd_initrd);
+}
diff --git a/include/grub/emu/exec.h b/include/grub/emu/exec.h
index d1073ef86a..1b61b4a2e5 100644
--- a/include/grub/emu/exec.h
+++ b/include/grub/emu/exec.h
@@ -23,6 +23,8 @@
 #include <stdarg.h>
 
 #include <sys/types.h>
+#include <grub/symbol.h>
+
 pid_t
 grub_util_exec_pipe (const char *const *argv, int *fd);
 pid_t
@@ -32,7 +34,7 @@ int
 grub_util_exec_redirect_all (const char *const *argv, const char *stdin_file,
 			     const char *stdout_file, const char *stderr_file);
 int
-grub_util_exec (const char *const *argv);
+EXPORT_FUNC(grub_util_exec) (const char *const *argv);
 int
 grub_util_exec_redirect (const char *const *argv, const char *stdin_file,
 			 const char *stdout_file);
diff --git a/include/grub/emu/hostfile.h b/include/grub/emu/hostfile.h
index cfb1e2b566..a61568e36e 100644
--- a/include/grub/emu/hostfile.h
+++ b/include/grub/emu/hostfile.h
@@ -22,6 +22,7 @@
 #include <grub/disk.h>
 #include <grub/partition.h>
 #include <sys/types.h>
+#include <grub/symbol.h>
 #include <grub/osdep/hostfile.h>
 
 int
@@ -29,7 +30,7 @@ grub_util_is_directory (const char *path);
 int
 grub_util_is_special_file (const char *path);
 int
-grub_util_is_regular (const char *path);
+EXPORT_FUNC(grub_util_is_regular) (const char *path);
 
 char *
 grub_util_path_concat (size_t n, ...);
diff --git a/include/grub/emu/misc.h b/include/grub/emu/misc.h
index ff9c48a649..01056954b9 100644
--- a/include/grub/emu/misc.h
+++ b/include/grub/emu/misc.h
@@ -57,6 +57,9 @@ void EXPORT_FUNC(grub_util_warn) (const char *fmt, ...) __attribute__ ((format (
 void EXPORT_FUNC(grub_util_info) (const char *fmt, ...) __attribute__ ((format (GNU_PRINTF, 1, 2)));
 void EXPORT_FUNC(grub_util_error) (const char *fmt, ...) __attribute__ ((format (GNU_PRINTF, 1, 2), noreturn));
 
+void EXPORT_FUNC(grub_util_set_kexecute) (void);
+int EXPORT_FUNC(grub_util_get_kexecute) (void) WARN_UNUSED_RESULT;
+
 grub_uint64_t EXPORT_FUNC (grub_util_get_cpu_time_ms) (void);
 
 #ifdef HAVE_DEVICE_MAPPER
-- 
2.35.1

Re: [PATCH v4 1/1] Add support for grub-emu to kexec Linux menu entries

[Daniel Kiper]

On Tue, Oct 04, 2022 at 03:16:48PM -0400, Robbie Harwood wrote:
> From: Raymund Will <rw@suse.com>
>
> The GRUB emulator is used as a debugging utility but it could also be
> used as a user-space bootloader if there is support to boot an operating
> system.
>
> The Linux kernel is already able to (re)boot another kernel via the
> kexec boot mechanism.  So the grub-emu tool could rely on this feature
> and have linux and initrd commands that are used to pass a kernel,
> initramfs image and command line parameters to kexec for booting a
> selected menu entry.
>
> By default the systemctl kexec option is used so systemd can shutdown
> all of the running services before doing a reboot using kexec.  But if
> this is not present, it can fall back to executing the kexec user-space
> tool directly.  The ability to force a kexec-reboot when systemctl kexec
> fails must only be used in controlled environments to avoid possible
> filesystem corruption and data loss.
>
> Signed-off-by: Raymund Will <rw@suse.com>
> Signed-off-by: John Jolly <jjolly@suse.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> ---
>  grub-core/Makefile.am        |   1 +
>  grub-core/Makefile.core.def  |   2 +-
>  grub-core/kern/emu/main.c    |   4 +
>  grub-core/kern/emu/misc.c    |  18 +++-
>  grub-core/loader/emu/linux.c | 181 +++++++++++++++++++++++++++++++++++
>  include/grub/emu/exec.h      |   4 +-
>  include/grub/emu/hostfile.h  |   3 +-
>  include/grub/emu/misc.h      |   3 +

Please document this functionality in the user documentation.

>  8 files changed, 212 insertions(+), 4 deletions(-)
>  create mode 100644 grub-core/loader/emu/linux.c
>
> diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
> index ee88e44e97..80e7a83edf 100644
> --- a/grub-core/Makefile.am
> +++ b/grub-core/Makefile.am
> @@ -307,6 +307,7 @@ KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/net.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostdisk.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostfile.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/extcmd.h
> +KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/exec.h
>  if COND_GRUB_EMU_SDL
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/sdl.h
>  endif
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 7159948721..5350408601 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1816,9 +1816,9 @@ module = {
>    arm64 = loader/arm64/linux.c;
>    riscv32 = loader/riscv/linux.c;
>    riscv64 = loader/riscv/linux.c;
> +  emu = loader/emu/linux.c;
>    common = loader/linux.c;
>    common = lib/cmdline.c;
> -  enable = noemu;
>  };
>
>  module = {
> diff --git a/grub-core/kern/emu/main.c b/grub-core/kern/emu/main.c
> index 44e087e988..855b11c3de 100644
> --- a/grub-core/kern/emu/main.c
> +++ b/grub-core/kern/emu/main.c
> @@ -107,6 +107,7 @@ static struct argp_option options[] = {
>     N_("use GRUB files in the directory DIR [default=%s]"), 0},
>    {"verbose",     'v', 0,      0, N_("print verbose messages."), 0},
>    {"hold",     'H', N_("SECS"),      OPTION_ARG_OPTIONAL, N_("wait until a debugger will attach"), 0},
> +  {"kexec",       'X', 0,      0, N_("use kexec to boot Linux kernels via systemctl (pass twice to enable dangerous fallback to non-systemctl)."), 0},
>    { 0, 0, 0, 0, 0, 0 }
>  };
>
> @@ -164,6 +165,9 @@ argp_parser (int key, char *arg, struct argp_state *state)
>      case 'v':
>        verbosity++;
>        break;
> +    case 'X':
> +      grub_util_set_kexecute ();
> +      break;
>
>      case ARGP_KEY_ARG:
>        {
> diff --git a/grub-core/kern/emu/misc.c b/grub-core/kern/emu/misc.c
> index d0e7a107e7..521220b49d 100644
> --- a/grub-core/kern/emu/misc.c
> +++ b/grub-core/kern/emu/misc.c
> @@ -39,6 +39,7 @@
>  #include <grub/emu/misc.h>
>
>  int verbosity;
> +int kexecute;
>
>  void
>  grub_util_warn (const char *fmt, ...)
> @@ -82,7 +83,7 @@ grub_util_error (const char *fmt, ...)
>    vfprintf (stderr, fmt, ap);
>    va_end (ap);
>    fprintf (stderr, ".\n");
> -  exit (1);
> +  grub_exit ();
>  }
>
>  void *
> @@ -153,6 +154,9 @@ xasprintf (const char *fmt, ...)
>  void
>  grub_exit (void)
>  {
> +#if defined (GRUB_KERNEL)
> +  grub_reboot ();
> +#endif
>    exit (1);
>  }
>  #endif
> @@ -214,3 +218,15 @@ grub_util_load_image (const char *path, char *buf)
>
>    fclose (fp);
>  }
> +
> +void
> +grub_util_set_kexecute (void)
> +{
> +  kexecute++;
> +}
> +
> +int
> +grub_util_get_kexecute (void)
> +{
> +  return kexecute;
> +}
> diff --git a/grub-core/loader/emu/linux.c b/grub-core/loader/emu/linux.c
> new file mode 100644
> index 0000000000..bdcdbb0ff4
> --- /dev/null
> +++ b/grub-core/loader/emu/linux.c
> @@ -0,0 +1,181 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2006,2007,2008,2009,2010  Free Software Foundation, Inc.

s/2006,2007,2008,2009,2010/2022/

> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB 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 General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/loader.h>
> +#include <grub/dl.h>
> +#include <grub/command.h>
> +#include <grub/time.h>
> +
> +#include <grub/emu/exec.h>
> +#include <grub/emu/hostfile.h>
> +#include <grub/emu/misc.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static grub_dl_t my_mod;
> +
> +static char *kernel_path;
> +static char *initrd_path;
> +static char *boot_cmdline;
> +
> +static grub_err_t
> +grub_linux_boot (void)
> +{
> +  grub_err_t rc = GRUB_ERR_NONE;
> +  char *initrd_param;
> +  const char *kexec[] = { "kexec", "-la", kernel_path, boot_cmdline, NULL, NULL };
> +  const char *systemctl[] = { "systemctl", "kexec", NULL };

Please drop spaces after "{" and before "}".

> +  int kexecute = grub_util_get_kexecute ();
> +
> +  if (initrd_path)
> +    {
> +      initrd_param = grub_xasprintf ("--initrd=%s", initrd_path);
> +      kexec[3] = initrd_param;
> +      kexec[4] = boot_cmdline;
> +    }
> +  else
> +    {
> +      initrd_param = grub_xasprintf ("%s", "");
> +    }

Please drop these redundant curly braces.

> +  grub_dprintf ("linux", "%serforming 'kexec -la %s %s %s'\n",
> +                (kexecute) ? "P" : "Not p",
> +                kernel_path, initrd_param, boot_cmdline);
> +
> +  if (kexecute)
> +    rc = grub_util_exec (kexec);
> +
> +  grub_free(initrd_param);
> +
> +  if (rc != GRUB_ERR_NONE)
> +    {
> +      grub_error (rc, N_("Error trying to perform kexec load operation."));

All error messages should start with lowercase and do not contain full
stop at the end. Fix similar issues below too...

> +      grub_sleep (3);
> +      return rc;
> +    }
> +
> +  if (kexecute < 1)
> +    grub_fatal (N_("Use '"PACKAGE"-emu --kexec' to force a system restart."));
> +
> +  grub_dprintf ("linux", "Performing 'systemctl kexec' (%s) ",
> +		(kexecute==1) ? "do-or-die" : "just-in-case");
> +  rc = grub_util_exec (systemctl);
> +
> +  if (kexecute == 1)
> +    grub_fatal (N_("Error trying to perform 'systemctl kexec'"));

I would add '": %d", rc' to the end of fatal message.

> +  /* WARNING: forcible reset should only be used in read-only environments.
> +   * grub-emu cannot check for these - users beware. */

Please format comment properly [1].

> +  grub_dprintf ("linux", "Performing 'kexec -e -x'");
> +  kexec[1] = "-e";
> +  kexec[2] = "-x";
> +  kexec[3] = NULL;
> +  rc = grub_util_exec (kexec);
> +  if ( rc != GRUB_ERR_NONE )

Please drop space after "(" and before ")". Fix similar issues below too...

> +    grub_fatal (N_("Error trying to directly perform 'kexec -e'."));

Please display rc value as above.

> +  return rc;
> +}
> +
> +static grub_err_t
> +grub_linux_unload (void)
> +{
> +  grub_dl_unref (my_mod);

I understand that this marks this module as not in use. Right? If yes
please add a comment before grub_dl_unref() call what exactly happens
here and why. It took me a while to understand why it is needed. Please
do the same below for all grub_dl_ref() and grub_dl_unref() calls.

> +  if ( boot_cmdline != NULL )

This if is redundant.

> +    grub_free (boot_cmdline);
> +  boot_cmdline = NULL;
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)), int argc, char *argv[])
> +{
> +  int i;
> +  char *tempstr;
> +
> +  grub_dl_ref (my_mod);
> +
> +  if (argc == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +
> +  if ( !grub_util_is_regular (argv[0]) )
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find kernel file %s"), argv[0]);
> +
> +  if ( kernel_path != NULL )

This if is redundant too.

> +    grub_free (kernel_path);
> +
> +  kernel_path = grub_xasprintf ("%s", argv[0]);
> +
> +  if ( boot_cmdline != NULL )

Ditto. Please fix similar issues below too...

> +    {
> +      grub_free(boot_cmdline);

Missing space before "(". Same things should be fixed below too...

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments

Re: [PATCH v4 1/1] Add support for grub-emu to kexec Linux menu entries

[Robbie Harwood]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

Daniel Kiper <dkiper@net-space.pl> writes:

> On Tue, Oct 04, 2022 at 03:16:48PM -0400, Robbie Harwood wrote:
>
>> +  return rc;
>> +}
>> +
>> +static grub_err_t
>> +grub_linux_unload (void)
>> +{
>> +  grub_dl_unref (my_mod);
>
> I understand that this marks this module as not in use. Right? If yes
> please add a comment before grub_dl_unref() call what exactly happens
> here and why. It took me a while to understand why it is
> needed. Please do the same below for all grub_dl_ref() and
> grub_dl_unref() calls.

I expect it's modeled after the other linux.c - e.g.,
grub-core/loader/i386/linux.c has an almost identical
grub_linux_unload() function.

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH v4 1/1] Add support for grub-emu to kexec Linux menu entries

[Oskari Pirhonen]

[-- Attachment #1: Type: text/plain, Size: 13446 bytes --]

On Tue, Oct 04, 2022 at 15:16:48 -0400, Robbie Harwood wrote:
> From: Raymund Will <rw@suse.com>
> 
> The GRUB emulator is used as a debugging utility but it could also be
> used as a user-space bootloader if there is support to boot an operating
> system.
> 
> The Linux kernel is already able to (re)boot another kernel via the
> kexec boot mechanism.  So the grub-emu tool could rely on this feature
> and have linux and initrd commands that are used to pass a kernel,
> initramfs image and command line parameters to kexec for booting a
> selected menu entry.
> 
> By default the systemctl kexec option is used so systemd can shutdown
> all of the running services before doing a reboot using kexec.  But if
> this is not present, it can fall back to executing the kexec user-space
> tool directly.  The ability to force a kexec-reboot when systemctl kexec
> fails must only be used in controlled environments to avoid possible
> filesystem corruption and data loss.
> 

Can the existence of systemd/systemctl be checked at configure-time? I
run OpenRC where systemctl is unavailable, so it looks like it would
always hit the error case unless I pass the arg twice. It could also
cause confusion to users if they notice it fails to call systemctl when
on non-systemd systems.

The simplest/safest solution would be to just disable this if the
configure check fails until `systemctl kexec`-equivalent code paths are
added.

I've CC-ed the Gentoo Base System project [1] in case some of them can
provide additional input, such as how to get the safe path working with
other init systems.

- Oskari

[1] https://wiki.gentoo.org/wiki/Project:Base

> Signed-off-by: Raymund Will <rw@suse.com>
> Signed-off-by: John Jolly <jjolly@suse.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> ---
>  grub-core/Makefile.am        |   1 +
>  grub-core/Makefile.core.def  |   2 +-
>  grub-core/kern/emu/main.c    |   4 +
>  grub-core/kern/emu/misc.c    |  18 +++-
>  grub-core/loader/emu/linux.c | 181 +++++++++++++++++++++++++++++++++++
>  include/grub/emu/exec.h      |   4 +-
>  include/grub/emu/hostfile.h  |   3 +-
>  include/grub/emu/misc.h      |   3 +
>  8 files changed, 212 insertions(+), 4 deletions(-)
>  create mode 100644 grub-core/loader/emu/linux.c
> 
> diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
> index ee88e44e97..80e7a83edf 100644
> --- a/grub-core/Makefile.am
> +++ b/grub-core/Makefile.am
> @@ -307,6 +307,7 @@ KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/net.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostdisk.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostfile.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/extcmd.h
> +KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/exec.h
>  if COND_GRUB_EMU_SDL
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/sdl.h
>  endif
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 7159948721..5350408601 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1816,9 +1816,9 @@ module = {
>    arm64 = loader/arm64/linux.c;
>    riscv32 = loader/riscv/linux.c;
>    riscv64 = loader/riscv/linux.c;
> +  emu = loader/emu/linux.c;
>    common = loader/linux.c;
>    common = lib/cmdline.c;
> -  enable = noemu;
>  };
>  
>  module = {
> diff --git a/grub-core/kern/emu/main.c b/grub-core/kern/emu/main.c
> index 44e087e988..855b11c3de 100644
> --- a/grub-core/kern/emu/main.c
> +++ b/grub-core/kern/emu/main.c
> @@ -107,6 +107,7 @@ static struct argp_option options[] = {
>     N_("use GRUB files in the directory DIR [default=%s]"), 0},
>    {"verbose",     'v', 0,      0, N_("print verbose messages."), 0},
>    {"hold",     'H', N_("SECS"),      OPTION_ARG_OPTIONAL, N_("wait until a debugger will attach"), 0},
> +  {"kexec",       'X', 0,      0, N_("use kexec to boot Linux kernels via systemctl (pass twice to enable dangerous fallback to non-systemctl)."), 0},
>    { 0, 0, 0, 0, 0, 0 }
>  };
>  
> @@ -164,6 +165,9 @@ argp_parser (int key, char *arg, struct argp_state *state)
>      case 'v':
>        verbosity++;
>        break;
> +    case 'X':
> +      grub_util_set_kexecute ();
> +      break;
>  
>      case ARGP_KEY_ARG:
>        {
> diff --git a/grub-core/kern/emu/misc.c b/grub-core/kern/emu/misc.c
> index d0e7a107e7..521220b49d 100644
> --- a/grub-core/kern/emu/misc.c
> +++ b/grub-core/kern/emu/misc.c
> @@ -39,6 +39,7 @@
>  #include <grub/emu/misc.h>
>  
>  int verbosity;
> +int kexecute;
>  
>  void
>  grub_util_warn (const char *fmt, ...)
> @@ -82,7 +83,7 @@ grub_util_error (const char *fmt, ...)
>    vfprintf (stderr, fmt, ap);
>    va_end (ap);
>    fprintf (stderr, ".\n");
> -  exit (1);
> +  grub_exit ();
>  }
>  
>  void *
> @@ -153,6 +154,9 @@ xasprintf (const char *fmt, ...)
>  void
>  grub_exit (void)
>  {
> +#if defined (GRUB_KERNEL)
> +  grub_reboot ();
> +#endif
>    exit (1);
>  }
>  #endif
> @@ -214,3 +218,15 @@ grub_util_load_image (const char *path, char *buf)
>  
>    fclose (fp);
>  }
> +
> +void
> +grub_util_set_kexecute (void)
> +{
> +  kexecute++;
> +}
> +
> +int
> +grub_util_get_kexecute (void)
> +{
> +  return kexecute;
> +}
> diff --git a/grub-core/loader/emu/linux.c b/grub-core/loader/emu/linux.c
> new file mode 100644
> index 0000000000..bdcdbb0ff4
> --- /dev/null
> +++ b/grub-core/loader/emu/linux.c
> @@ -0,0 +1,181 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2006,2007,2008,2009,2010  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB 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 General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/loader.h>
> +#include <grub/dl.h>
> +#include <grub/command.h>
> +#include <grub/time.h>
> +
> +#include <grub/emu/exec.h>
> +#include <grub/emu/hostfile.h>
> +#include <grub/emu/misc.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static grub_dl_t my_mod;
> +
> +static char *kernel_path;
> +static char *initrd_path;
> +static char *boot_cmdline;
> +
> +static grub_err_t
> +grub_linux_boot (void)
> +{
> +  grub_err_t rc = GRUB_ERR_NONE;
> +  char *initrd_param;
> +  const char *kexec[] = { "kexec", "-la", kernel_path, boot_cmdline, NULL, NULL };
> +  const char *systemctl[] = { "systemctl", "kexec", NULL };
> +  int kexecute = grub_util_get_kexecute ();
> +
> +  if (initrd_path)
> +    {
> +      initrd_param = grub_xasprintf ("--initrd=%s", initrd_path);
> +      kexec[3] = initrd_param;
> +      kexec[4] = boot_cmdline;
> +    }
> +  else
> +    {
> +      initrd_param = grub_xasprintf ("%s", "");
> +    }
> +
> +  grub_dprintf ("linux", "%serforming 'kexec -la %s %s %s'\n",
> +                (kexecute) ? "P" : "Not p",
> +                kernel_path, initrd_param, boot_cmdline);
> +
> +  if (kexecute)
> +    rc = grub_util_exec (kexec);
> +
> +  grub_free(initrd_param);
> +
> +  if (rc != GRUB_ERR_NONE)
> +    {
> +      grub_error (rc, N_("Error trying to perform kexec load operation."));
> +      grub_sleep (3);
> +      return rc;
> +    }
> +
> +  if (kexecute < 1)
> +    grub_fatal (N_("Use '"PACKAGE"-emu --kexec' to force a system restart."));
> +
> +  grub_dprintf ("linux", "Performing 'systemctl kexec' (%s) ",
> +		(kexecute==1) ? "do-or-die" : "just-in-case");
> +  rc = grub_util_exec (systemctl);
> +
> +  if (kexecute == 1)
> +    grub_fatal (N_("Error trying to perform 'systemctl kexec'"));
> +
> +  /* WARNING: forcible reset should only be used in read-only environments.
> +   * grub-emu cannot check for these - users beware. */
> +  grub_dprintf ("linux", "Performing 'kexec -e -x'");
> +  kexec[1] = "-e";
> +  kexec[2] = "-x";
> +  kexec[3] = NULL;
> +  rc = grub_util_exec (kexec);
> +  if ( rc != GRUB_ERR_NONE )
> +    grub_fatal (N_("Error trying to directly perform 'kexec -e'."));
> +
> +  return rc;
> +}
> +
> +static grub_err_t
> +grub_linux_unload (void)
> +{
> +  grub_dl_unref (my_mod);
> +  if ( boot_cmdline != NULL )
> +    grub_free (boot_cmdline);
> +  boot_cmdline = NULL;
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)), int argc, char *argv[])
> +{
> +  int i;
> +  char *tempstr;
> +
> +  grub_dl_ref (my_mod);
> +
> +  if (argc == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +
> +  if ( !grub_util_is_regular (argv[0]) )
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find kernel file %s"), argv[0]);
> +
> +  if ( kernel_path != NULL )
> +    grub_free (kernel_path);
> +
> +  kernel_path = grub_xasprintf ("%s", argv[0]);
> +
> +  if ( boot_cmdline != NULL )
> +    {
> +      grub_free(boot_cmdline);
> +      boot_cmdline = NULL;
> +    }
> +
> +  if ( argc > 1 )
> +    {
> +      boot_cmdline = grub_xasprintf("--command-line=%s", argv[1]);
> +      for ( i = 2; i < argc; i++ )
> +        {
> +          tempstr = grub_xasprintf("%s %s", boot_cmdline, argv[i]);
> +          grub_free(boot_cmdline);
> +          boot_cmdline = tempstr;
> +        }
> +    }
> +
> +  grub_loader_set (grub_linux_boot, grub_linux_unload, 0);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), int argc, char *argv[])
> +{
> +  if (argc == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +
> +  if ( !grub_util_is_regular (argv[0]) )
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find initrd file %s"), argv[0]);
> +
> +  if ( initrd_path != NULL )
> +    grub_free (initrd_path);
> +
> +  initrd_path = grub_xasprintf("%s", argv[0]);
> +
> +  grub_dl_unref (my_mod);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_command_t cmd_linux, cmd_initrd;
> +
> +GRUB_MOD_INIT (linux)
> +{
> +  cmd_linux = grub_register_command ("linux", grub_cmd_linux, 0, N_("Load Linux."));
> +  cmd_initrd = grub_register_command ("initrd", grub_cmd_initrd, 0, N_("Load initrd."));
> +  my_mod = mod;
> +  kernel_path = NULL;
> +  initrd_path = NULL;
> +  boot_cmdline = NULL;
> +}
> +
> +GRUB_MOD_FINI (linux)
> +{
> +  grub_unregister_command (cmd_linux);
> +  grub_unregister_command (cmd_initrd);
> +}
> diff --git a/include/grub/emu/exec.h b/include/grub/emu/exec.h
> index d1073ef86a..1b61b4a2e5 100644
> --- a/include/grub/emu/exec.h
> +++ b/include/grub/emu/exec.h
> @@ -23,6 +23,8 @@
>  #include <stdarg.h>
>  
>  #include <sys/types.h>
> +#include <grub/symbol.h>
> +
>  pid_t
>  grub_util_exec_pipe (const char *const *argv, int *fd);
>  pid_t
> @@ -32,7 +34,7 @@ int
>  grub_util_exec_redirect_all (const char *const *argv, const char *stdin_file,
>  			     const char *stdout_file, const char *stderr_file);
>  int
> -grub_util_exec (const char *const *argv);
> +EXPORT_FUNC(grub_util_exec) (const char *const *argv);
>  int
>  grub_util_exec_redirect (const char *const *argv, const char *stdin_file,
>  			 const char *stdout_file);
> diff --git a/include/grub/emu/hostfile.h b/include/grub/emu/hostfile.h
> index cfb1e2b566..a61568e36e 100644
> --- a/include/grub/emu/hostfile.h
> +++ b/include/grub/emu/hostfile.h
> @@ -22,6 +22,7 @@
>  #include <grub/disk.h>
>  #include <grub/partition.h>
>  #include <sys/types.h>
> +#include <grub/symbol.h>
>  #include <grub/osdep/hostfile.h>
>  
>  int
> @@ -29,7 +30,7 @@ grub_util_is_directory (const char *path);
>  int
>  grub_util_is_special_file (const char *path);
>  int
> -grub_util_is_regular (const char *path);
> +EXPORT_FUNC(grub_util_is_regular) (const char *path);
>  
>  char *
>  grub_util_path_concat (size_t n, ...);
> diff --git a/include/grub/emu/misc.h b/include/grub/emu/misc.h
> index ff9c48a649..01056954b9 100644
> --- a/include/grub/emu/misc.h
> +++ b/include/grub/emu/misc.h
> @@ -57,6 +57,9 @@ void EXPORT_FUNC(grub_util_warn) (const char *fmt, ...) __attribute__ ((format (
>  void EXPORT_FUNC(grub_util_info) (const char *fmt, ...) __attribute__ ((format (GNU_PRINTF, 1, 2)));
>  void EXPORT_FUNC(grub_util_error) (const char *fmt, ...) __attribute__ ((format (GNU_PRINTF, 1, 2), noreturn));
>  
> +void EXPORT_FUNC(grub_util_set_kexecute) (void);
> +int EXPORT_FUNC(grub_util_get_kexecute) (void) WARN_UNUSED_RESULT;
> +
>  grub_uint64_t EXPORT_FUNC (grub_util_get_cpu_time_ms) (void);
>  
>  #ifdef HAVE_DEVICE_MAPPER
> -- 
> 2.35.1
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 1/1] Add support for grub-emu to kexec Linux menu entries

[Robbie Harwood]

[-- Attachment #1: Type: text/plain, Size: 3139 bytes --]

Oskari Pirhonen <xxc3ncoredxx@gmail.com> writes:

> On Tue, Oct 04, 2022 at 15:16:48 -0400, Robbie Harwood wrote:
>> From: Raymund Will <rw@suse.com>
>> 
>> The GRUB emulator is used as a debugging utility but it could also be
>> used as a user-space bootloader if there is support to boot an operating
>> system.
>> 
>> The Linux kernel is already able to (re)boot another kernel via the
>> kexec boot mechanism.  So the grub-emu tool could rely on this feature
>> and have linux and initrd commands that are used to pass a kernel,
>> initramfs image and command line parameters to kexec for booting a
>> selected menu entry.
>> 
>> By default the systemctl kexec option is used so systemd can shutdown
>> all of the running services before doing a reboot using kexec.  But if
>> this is not present, it can fall back to executing the kexec user-space
>> tool directly.  The ability to force a kexec-reboot when systemctl kexec
>> fails must only be used in controlled environments to avoid possible
>> filesystem corruption and data loss.
>> 
>
> Can the existence of systemd/systemctl be checked at configure-time?

No - think about how binary distros work.  Only on locally built
environments like gentoo is configure time guaranteed to look like
runtime.  Consider Debian, for instance, which can run with multiple
init systems (including openrc, if memory serves): for them, a
configure-time check could make things worse.

> I run OpenRC where systemctl is unavailable, so it looks like it would
> always hit the error case unless I pass the arg twice. It could also
> cause confusion to users if they notice it fails to call systemctl
> when on non-systemd systems.

I don't think this has the impact you're suggesting.  This is new code -
you can't kexec from grub2-emu today.  It's also working as documented
("use kexec to boot Linux kernels via systemctl (pass twice to enable
dangerous fallback to non-systemctl).").  I don't think a user reading
that documentation would expect a system without systemctl functionality
to support this feature.  (Whether users expect their system to have
systemctl functionality is better asked of initsystem developers than of
me.)

I want to be clear here that I support people working on other init
systems, but if there's not an equivalent for this functionality,
there's very little I can do here.

> The simplest/safest solution would be to just disable this if the
> configure check fails until `systemctl kexec`-equivalent code paths are
> added.

Careful with langauge, please.  There is nothing unsafe about attempting
to exec systemctl when it doesn't exist.  And I don't think adding
configuration checks makes code simpler.

> I've CC-ed the Gentoo Base System project [1] in case some of them can
> provide additional input, such as how to get the safe path working
> with other init systems.

Okay.  Any input here is fine, but I think it will be easier for
everyone if it's in the form of separate, follow-up patches.  (I'm happy
to review them once posted, if that's helpful; just CC me.)

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH v4 1/1] Add support for grub-emu to kexec Linux menu entries

[Oskari Pirhonen]

[-- Attachment #1: Type: text/plain, Size: 4272 bytes --]

On Tue, Oct 18, 2022 at 15:26:45 -0400, Robbie Harwood wrote:
> Oskari Pirhonen <xxc3ncoredxx@gmail.com> writes:
> 
> > On Tue, Oct 04, 2022 at 15:16:48 -0400, Robbie Harwood wrote:
> >> From: Raymund Will <rw@suse.com>
> >> 
> >> The GRUB emulator is used as a debugging utility but it could also be
> >> used as a user-space bootloader if there is support to boot an operating
> >> system.
> >> 
> >> The Linux kernel is already able to (re)boot another kernel via the
> >> kexec boot mechanism.  So the grub-emu tool could rely on this feature
> >> and have linux and initrd commands that are used to pass a kernel,
> >> initramfs image and command line parameters to kexec for booting a
> >> selected menu entry.
> >> 
> >> By default the systemctl kexec option is used so systemd can shutdown
> >> all of the running services before doing a reboot using kexec.  But if
> >> this is not present, it can fall back to executing the kexec user-space
> >> tool directly.  The ability to force a kexec-reboot when systemctl kexec
> >> fails must only be used in controlled environments to avoid possible
> >> filesystem corruption and data loss.
> >> 
> >
> > Can the existence of systemd/systemctl be checked at configure-time?
> 
> No - think about how binary distros work.  Only on locally built
> environments like gentoo is configure time guaranteed to look like
> runtime.  Consider Debian, for instance, which can run with multiple
> init systems (including openrc, if memory serves): for them, a
> configure-time check could make things worse.
> 

My idea was something along the lines of --with-kexec=foo in order to
allow for automatic detection as well as manually specifying the method
used. I believe that should cover building for binary distros as well as
locally.

> > I run OpenRC where systemctl is unavailable, so it looks like it would
> > always hit the error case unless I pass the arg twice. It could also
> > cause confusion to users if they notice it fails to call systemctl
> > when on non-systemd systems.
> 
> I don't think this has the impact you're suggesting.  This is new code -
> you can't kexec from grub2-emu today.  It's also working as documented
> ("use kexec to boot Linux kernels via systemctl (pass twice to enable
> dangerous fallback to non-systemctl).").  I don't think a user reading
> that documentation would expect a system without systemctl functionality
> to support this feature.  (Whether users expect their system to have
> systemctl functionality is better asked of initsystem developers than of
> me.)
> 

These are valid points for the current (and possibly final) state of
this patch.

> I want to be clear here that I support people working on other init
> systems, but if there's not an equivalent for this functionality,
> there's very little I can do here.
> 

The most that I would be requesting be added is basic scaffolding to
make future additions easier. But if no one comes forward before the
patch is accepted as ready, saying there's an equally easy method to
`systemctl kexec` for some other init system, then it wouldn't make
sense to add it until said method can be added too.

> > The simplest/safest solution would be to just disable this if the
> > configure check fails until `systemctl kexec`-equivalent code paths are
> > added.
> 
> Careful with langauge, please.  There is nothing unsafe about attempting
> to exec systemctl when it doesn't exist.  And I don't think adding
> configuration checks makes code simpler.
> 

I meant "safe" in the sense that it's a controlled kexec reboot, not
that running a non-existent systemctl is unsafe. Apologies if that was
unclear.

> > I've CC-ed the Gentoo Base System project [1] in case some of them can
> > provide additional input, such as how to get the safe path working
> > with other init systems.
> 
> Okay.  Any input here is fine, but I think it will be easier for
> everyone if it's in the form of separate, follow-up patches.  (I'm happy
> to review them once posted, if that's helpful; just CC me.)
> 

You are probably right on that, which is why I wanted to get the idea
out there at least.

I will do some more digging myself too.

- Oskari

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]