From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1oQnjS-0005Co-Kq for mharc-grub-devel@gnu.org; Wed, 24 Aug 2022 06:35:34 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:54976) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oQnjN-00052K-CS for grub-devel@gnu.org; Wed, 24 Aug 2022 06:35:29 -0400 Received: from smtp-out2.suse.de ([2001:67c:2178:6::1d]:51402) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oQnjL-0001d0-BB for grub-devel@gnu.org; Wed, 24 Aug 2022 06:35:29 -0400 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id C53A21FA4E; Wed, 24 Aug 2022 10:35:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1661337323; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=NbCfDnB1YE4tq35LVCQP+LZUrozLOj/T8NhZSsyyO90=; b=ur2G/BGWeWAFtu0haZ9EBmCCJKLWW28x1xEXiiB0Q0kxndr9VxI1F988H2DeILZPGO3bhN CQFXSqKRM66k+IID4q94t+zGneXfw15YyGb66FPRqHU/cdF1FdvfA57JyMfQ1cO7kqPyQg Rd1r2eZxk5xh3PZsXw9mI2q4PnyP+wk= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1661337323; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=NbCfDnB1YE4tq35LVCQP+LZUrozLOj/T8NhZSsyyO90=; b=wcUlJx0h7fyL5oWjkRIO7JZyEK6flYdwxpgGF9UN8SrJVlyJM01IVhl9rM9xAuFWy/zNzT 6F7OUa85vow7nLBg== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id AAB442C141; Wed, 24 Aug 2022 10:35:22 +0000 (UTC) Received: by wotan.suse.de (Postfix, from userid 16060) id B3DFF678B; Wed, 24 Aug 2022 10:35:22 +0000 (UTC) Date: Wed, 24 Aug 2022 12:35:22 +0200 From: Raymund Will To: Robbie Harwood Cc: grub-devel@gnu.org, dkiper@net-space.pl, Javier Martinez Canillas Subject: Re: [PATCH v3 1/1] Add support for grub-emu to kexec Linux menu entries Message-ID: <20220824103522.GG7668@suse.de> References: <20220823211542.167373-1-rharwood@redhat.com> <20220823211542.167373-2-rharwood@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220823211542.167373-2-rharwood@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) Received-SPF: pass client-ip=2001:67c:2178:6::1d; envelope-from=rw@suse.de; helo=smtp-out2.suse.de X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Aug 2022 10:35:29 -0000 Robbie Harwood wrote on 2022-08-23T17:15:42 -0400: > From: Raymund Will [...] > 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 fallbacks to executing the kexec user-space tool directly. The last sentence should probably read more like: The provision to force a kexec-reboot, in case systemctl kexec fails, must only be used in controlled environments to avoid possible file-system corruption and data-loss. [...] > --- /dev/null > +++ b/grub-core/loader/emu/linux.c > @@ -0,0 +1,180 @@ [...] > +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 }; You might have noticed the change in the first parameter to kexec, which makes a perfect argument for Daniel's request to have that configurable! But implementation would be quite expensive, maybe unless it's strictly restricted to non-whitespace- bearing parameters. Would that be sufficient and viable? > + 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); Well, the original grub_printf() in this case was very helpful to "create" a kexec-load command for cut-n-paste. Is it really necessary to bury it in a ton of debug messages? > + > + 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_error (rc, N_("Error trying to perform 'systemctl kexec'")); This grub_error() needs to be a grub_fatal() to achieve the intended behavior, right? If kexecute is 1 it should bail out here. Only if it's bigger the forced kexec should be tried! (Note, that "< 1" is already covered above!) > + > + /* need to check read-only root before resetting hard!? */ This comment probably needs to be replaced with a strict one (reflected in GRUB's docs) explaining, that the user takes full responsiblity in "force" being exclusively used in read-only environments, as grub-emu itself simply can't guarantee this. (Any check here would hardly scratch the surface.) > + grub_dprintf ("linux", "Performing 'kexec -e -x'"); > + kexec[1] = "-e"; > + kexec[2] = "-x"; > + kexec[3] = NULL; Provided the kexec-load gets a tunable, this kexec-exec probably deserves one as well (as this '-ex' initially was only a '-e' (see ----vv)). > + rc = grub_util_exec (kexec); > + if ( rc != GRUB_ERR_NONE ) > + grub_fatal (N_("Error trying to directly perform 'kexec -e'.")); > + > + return rc; > +} [...] Thanks Robbie for driving this, as I'm lacking the time... Best, -- Raymund WILL rw@SUSE.de SUSE Software Solutions Germany GmbH, Frankenstr. 146, D-90461 Nuernberg Geschaeftsfuehrer: Ivo Totev, et al (HRB 36809, AG Nuernberg)