Re: [PATCH] [RFC] Add exitcode support

On Tue, Aug 18, 2015 at 12:56 PM, Dann Frazier <address@hidden> wrote:

On Tue, Aug 18, 2015 at 12:03 PM, Ben Hildred <address@hidden> wrote:
> I only briefly scanned this, but this is cool (at least the idea is
> something I want to see implemented everywhere it makes sense to do so).

Cool, thanks for the feedback :)

> Just one humdinger of a question: does efi assume nonzero is an error? Does
> platform X assume a nonzero exit is an error? Do we want to assume nonzero
> is an error? What do we do when these assumptions do not agree?

This is the reason for the GRUB_EXITCODE abstraction. I thought we
could abstract out the behaviors users would like to request (e.g.,
try the next boot device, halt, report an error, etc), and then map
those to the proper behavior for the underlying platform (if
supported). Right now I've implemented two behaviors:

- GRUB_EXITCODE_NONE, which retains existing behavior

Is this true or false in a boolean context?

- GRUB_EXITCODE_NEXT (aka exit 1), meaning try the next
firmware-defined boot device.

Is this true or false in a boolean context?

Perhaps this should actually be string arguments to exit ('exit next')
instead of forcing the user to lookup integer codes. And perhaps it'd
be more honest to call them "hints" instead of "codes".

Let's assume for a minute that I have compiled grub as a multiboot image and have called it from another bootloader, say iPXE.Now iPXE assumes that any false return is an error. What happens when grub returns with exit next, does iPXE get a true or false? What about exit fred where fred is not defined by any platform? What if I do an exit config which is only defined for coreboot?

> My off the cuff recommendation is for grub to make no assumptions about how
> exit's value will be used in a general case but to instead have a per
> platform predefined variable (may EXITTYPE) which would provide to the
> script the semantics of the exit value. I would propose that the first three
> possible values be 'ignored', 'errorlevel', and 'zeroerror'. This would
> handle all current models that I am aware of. specifically it gives known
> true and false values everywhere.

I think we're generally on the same page. I do think I prefer passing
down an abstract request to the low level grub_exit() function vs. a
series of platform-specific variables because it gives the platform
code the ability to execute additional code if necessary - e.g.
setting fw environment variable.

I prefer an integer value being passed to exit, with a hint to tell us how to interpret the value specifically indicating if zero is true (like shell) or false (like perl). I would not be specifically opposed to string exits (aside from code size issues), but string to number mapping may introduce error conditions, and what do you do if exit fails?

-dann

> On Tue, Aug 18, 2015 at 11:30 AM, dann frazier <address@hidden>
> wrote:
>>
>> Some platforms are capable of changing behavior based on the bootloader
>> exit code. In particular, UEFI boot managers use this code to determine
>> if they should try the next boot entry or not. I'd like to use this as
>> a way to make my PXE server tell clients to attempt to boot from their
>> next entry (presumably an on-disk OS), providing something akin to
>> pxelinux's "localboot".
>>
>> The implementation here is to add a new optional argument to the exit
>> command. The platform-specific grub_exit() implementations can then
>> translate it into a platform-appropriate exit code.
>>
>> Signed-off-by: dann frazier <address@hidden>
>> ---
>> grub-core/commands/minicmd.c | 12 ++++++++----
>> grub-core/kern/efi/efi.c | 20 ++++++++++++++++++--
>> grub-core/kern/emu/misc.c | 3 ++-
>> grub-core/kern/i386/coreboot/init.c | 3 ++-
>> grub-core/kern/i386/qemu/init.c | 3 ++-
>> grub-core/kern/ieee1275/init.c | 3 ++-
>> grub-core/kern/mips/arc/init.c | 3 ++-
>> grub-core/kern/mips/loongson/init.c | 3 ++-
>> grub-core/kern/mips/qemu_mips/init.c | 3 ++-
>> grub-core/kern/misc.c | 3 ++-
>> grub-core/kern/uboot/init.c | 5 +++--
>> grub-core/kern/xen/init.c | 2 +-
>> include/grub/exitcode.h | 30 ++++++++++++++++++++++++++++++
>> include/grub/misc.h | 3 ++-
>> 14 files changed, 78 insertions(+), 18 deletions(-)
>> create mode 100644 include/grub/exitcode.h
>>
>> diff --git a/grub-core/commands/minicmd.c b/grub-core/commands/minicmd.c
>> index a3a1182..d67cdf1 100644
>> --- a/grub-core/commands/minicmd.c
>> +++ b/grub-core/commands/minicmd.c
>> @@ -28,6 +28,7 @@
>> #include <grub/loader.h>
>> #include <grub/command.h>
>> #include <grub/i18n.h>
>> +#include <grub/exitcode.h>
>>
>> GRUB_MOD_LICENSE ("GPLv3+");
>>
>> @@ -178,10 +179,13 @@ grub_mini_cmd_lsmod (struct grub_command *cmd
>> __attribute__ ((unused)),
>> /* exit */
>> static grub_err_t __attribute__ ((noreturn))
>> grub_mini_cmd_exit (struct grub_command *cmd __attribute__ ((unused)),
>> - int argc __attribute__ ((unused)),
>> - char *argv[] __attribute__ ((unused)))
>> + int argc, char *argv[])
>> +
>> {
>> - grub_exit ();
>> + grub_exitcode_t exitcode;
>> +
>> + exitcode = argc ? grub_strtoul(argv[0], 0, 10) : GRUB_EXITCODE_NONE;
>> + grub_exit (exitcode);
>> /* Not reached. */
>> }
>>
>> @@ -207,7 +211,7 @@ GRUB_MOD_INIT(minicmd)
>> 0, N_("Show loaded modules."));
>> cmd_exit =
>> grub_register_command ("exit", grub_mini_cmd_exit,
>> - 0, N_("Exit from GRUB."));
>> + N_("[EXITCODE]"), N_("Exit from GRUB."));
>> }
>>
>> GRUB_MOD_FINI(minicmd)
>> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
>> index caf9bcc..33fb737 100644
>> --- a/grub-core/kern/efi/efi.c
>> +++ b/grub-core/kern/efi/efi.c
>> @@ -28,6 +28,7 @@
>> #include <grub/kernel.h>
>> #include <grub/mm.h>
>> #include <grub/loader.h>
>> +#include <grub/exitcode.h>
>>
>> /* The handle of GRUB itself. Filled in by the startup code. */
>> grub_efi_handle_t grub_efi_image_handle;
>> @@ -155,11 +156,26 @@ grub_efi_get_loaded_image (grub_efi_handle_t
>> image_handle)
>> }
>>
>> void
>> -grub_exit (void)
>> +grub_exit (grub_exitcode_t exitcode)
>> {
>> + grub_efi_status_t efi_status;
>> +
>> grub_machine_fini (GRUB_LOADER_FLAG_NORETURN);
>> +
>> + /* Map the GRUB exitcode to a suitable EFI status */
>> + switch (exitcode)
>> + {
>> + case GRUB_EXITCODE_NEXT:
>> + /* Anything other than EFI_SUCCESS will do (UEFI 2.5 section 3.1.1)
>> */
>> + efi_status = GRUB_EFI_LOAD_ERROR;
>> + break;
>> + default:
>> + efi_status = GRUB_EFI_SUCCESS;
>> + break;
>> + }
>> +
>> efi_call_4 (grub_efi_system_table->boot_services->exit,
>> - grub_efi_image_handle, GRUB_EFI_SUCCESS, 0, 0);
>> + grub_efi_image_handle, efi_status, 0, 0);
>> for (;;) ;
>> }
>>
>> diff --git a/grub-core/kern/emu/misc.c b/grub-core/kern/emu/misc.c
>> index bb606da..4f345d6 100644
>> --- a/grub-core/kern/emu/misc.c
>> +++ b/grub-core/kern/emu/misc.c
>> @@ -35,6 +35,7 @@
>> #include <grub/i18n.h>
>> #include <grub/time.h>
>> #include <grub/emu/misc.h>
>> +#include <grub/exitcode.h>
>>
>> int verbosity;
>>
>> @@ -135,7 +136,7 @@ xasprintf (const char *fmt, ...)
>> #endif
>>
>> void
>> -grub_exit (void)
>> +grub_exit (grub_exitcode_t exitcode __attribute__ ((unused)))
>> {
>> exit (1);
>> }
>> diff --git a/grub-core/kern/i386/coreboot/init.c
>> b/grub-core/kern/i386/coreboot/init.c
>> index 3314f02..07294a1 100644
>> --- a/grub-core/kern/i386/coreboot/init.c
>> +++ b/grub-core/kern/i386/coreboot/init.c
>> @@ -35,13 +35,14 @@
>> #include <grub/cpu/floppy.h>
>> #include <grub/cpu/tsc.h>
>> #include <grub/video.h>
>> +#include <grub/exitcode.h>
>>
>> extern grub_uint8_t _start[];
>> extern grub_uint8_t _end[];
>> extern grub_uint8_t _edata[];
>>
>> void __attribute__ ((noreturn))
>> -grub_exit (void)
>> +grub_exit (grub_exitcode_t exitcode __attribute__ ((unused)))
>> {
>> /* We can't use grub_fatal() in this function. This would create an
>> infinite
>> loop, since grub_fatal() calls grub_abort() which in turn calls
>> grub_exit(). */
>> diff --git a/grub-core/kern/i386/qemu/init.c
>> b/grub-core/kern/i386/qemu/init.c
>> index 271b6fb..67e155c 100644
>> --- a/grub-core/kern/i386/qemu/init.c
>> +++ b/grub-core/kern/i386/qemu/init.c
>> @@ -36,13 +36,14 @@
>> #include <grub/cpu/tsc.h>
>> #include <grub/machine/kernel.h>
>> #include <grub/pci.h>
>> +#include <grub/exitcode.h>
>>
>> extern grub_uint8_t _start[];
>> extern grub_uint8_t _end[];
>> extern grub_uint8_t _edata[];
>>
>> void __attribute__ ((noreturn))
>> -grub_exit (void)
>> +grub_exit (grub_exitcode_t exitcode __attribute__ ((unused)))
>> {
>> /* We can't use grub_fatal() in this function. This would create an
>> infinite
>> loop, since grub_fatal() calls grub_abort() which in turn calls
>> grub_exit(). */
>> diff --git a/grub-core/kern/ieee1275/init.c
>> b/grub-core/kern/ieee1275/init.c
>> index d5bd74d..996663f 100644
>> --- a/grub-core/kern/ieee1275/init.c
>> +++ b/grub-core/kern/ieee1275/init.c
>> @@ -35,6 +35,7 @@
>> #include <grub/offsets.h>
>> #include <grub/memory.h>
>> #include <grub/loader.h>
>> +#include <grub/exitcode.h>
>> #ifdef __i386__
>> #include <grub/cpu/tsc.h>
>> #endif
>> @@ -60,7 +61,7 @@ grub_addr_t grub_ieee1275_original_stack;
>> #endif
>>
>> void
>> -grub_exit (void)
>> +grub_exit (grub_exitcode_t exitcode __attribute__ ((unused)))
>> {
>> grub_ieee1275_exit ();
>> }
>> diff --git a/grub-core/kern/mips/arc/init.c
>> b/grub-core/kern/mips/arc/init.c
>> index 3834a14..28189b4 100644
>> --- a/grub-core/kern/mips/arc/init.c
>> +++ b/grub-core/kern/mips/arc/init.c
>> @@ -36,6 +36,7 @@
>> #include <grub/i18n.h>
>> #include <grub/disk.h>
>> #include <grub/partition.h>
>> +#include <grub/exitcode.h>
>>
>> const char *type_names[] = {
>> #ifdef GRUB_CPU_WORDS_BIGENDIAN
>> @@ -276,7 +277,7 @@ grub_halt (void)
>> }
>>
>> void
>> -grub_exit (void)
>> +grub_exit (grub_exitcode_t exitcode __attribute__ ((unused)))
>> {
>> GRUB_ARC_FIRMWARE_VECTOR->exit ();
>>
>> diff --git a/grub-core/kern/mips/loongson/init.c
>> b/grub-core/kern/mips/loongson/init.c
>> index 7b96531..5958653 100644
>> --- a/grub-core/kern/mips/loongson/init.c
>> +++ b/grub-core/kern/mips/loongson/init.c
>> @@ -38,6 +38,7 @@
>> #include <grub/serial.h>
>> #include <grub/loader.h>
>> #include <grub/at_keyboard.h>
>> +#include <grub/exitcode.h>
>>
>> grub_err_t
>> grub_machine_mmap_iterate (grub_memory_hook_t hook, void *hook_data)
>> @@ -304,7 +305,7 @@ grub_halt (void)
>> }
>>
>> void
>> -grub_exit (void)
>> +grub_exit (grub_exitcode_t exitcode __attribute__ ((unused)))
>> {
>> grub_halt ();
>> }
>> diff --git a/grub-core/kern/mips/qemu_mips/init.c
>> b/grub-core/kern/mips/qemu_mips/init.c
>> index be88b77..b807b78 100644
>> --- a/grub-core/kern/mips/qemu_mips/init.c
>> +++ b/grub-core/kern/mips/qemu_mips/init.c
>> @@ -17,6 +17,7 @@
>> #include <grub/serial.h>
>> #include <grub/loader.h>
>> #include <grub/at_keyboard.h>
>> +#include <grub/exitcode.h>
>>
>> static inline int
>> probe_mem (grub_addr_t addr)
>> @@ -75,7 +76,7 @@ grub_machine_fini (int flags __attribute__ ((unused)))
>> }
>>
>> void
>> -grub_exit (void)
>> +grub_exit (grub_exitcode_t exitcode __attribute__ ((unused)))
>> {
>> grub_halt ();
>> }
>> diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
>> index 906d2c2..0db98cc 100644
>> --- a/grub-core/kern/misc.c
>> +++ b/grub-core/kern/misc.c
>> @@ -24,6 +24,7 @@
>> #include <grub/term.h>
>> #include <grub/env.h>
>> #include <grub/i18n.h>
>> +#include <grub/exitcode.h>
>>
>> union printf_arg
>> {
>> @@ -1087,7 +1088,7 @@ grub_abort (void)
>> grub_getkey ();
>> }
>>
>> - grub_exit ();
>> + grub_exit (GRUB_EXITCODE_NONE);
>> }
>>
>> void
>> diff --git a/grub-core/kern/uboot/init.c b/grub-core/kern/uboot/init.c
>> index 5dcc106..1f27fa9 100644
>> --- a/grub-core/kern/uboot/init.c
>> +++ b/grub-core/kern/uboot/init.c
>> @@ -32,6 +32,7 @@
>> #include <grub/uboot/api_public.h>
>> #include <grub/cpu/system.h>
>> #include <grub/cache.h>
>> +#include <grub/exitcode.h>
>>
>> extern char __bss_start[];
>> extern char _end[];
>> @@ -43,7 +44,7 @@ extern grub_uint32_t grub_uboot_machine_type;
>> extern grub_addr_t grub_uboot_boot_data;
>>
>> void
>> -grub_exit (void)
>> +grub_exit (grub_exitcode_t exitcode __attribute__ ((unused)))
>> {
>> grub_uboot_return (0);
>> }
>> @@ -94,7 +95,7 @@ grub_machine_init (void)
>> if (!ver)
>> {
>> /* Don't even have a console to log errors to... */
>> - grub_exit ();
>> + grub_exit (GRUB_EXITCODE_NONE);
>> }
>> else if (ver > API_SIG_VERSION)
>> {
>> diff --git a/grub-core/kern/xen/init.c b/grub-core/kern/xen/init.c
>> index 0559c03..0fc3723 100644
>> --- a/grub-core/kern/xen/init.c
>> +++ b/grub-core/kern/xen/init.c
>> @@ -549,7 +549,7 @@ grub_machine_init (void)
>> }
>>
>> void
>> -grub_exit (void)
>> +grub_exit (grub_exitcode_t exitcode __attribute__ ((unused)))
>> {
>> struct sched_shutdown arg;
>>
>> diff --git a/include/grub/exitcode.h b/include/grub/exitcode.h
>> new file mode 100644
>> index 0000000..7f179f4
>> --- /dev/null
>> +++ b/include/grub/exitcode.h
>> @@ -0,0 +1,30 @@
>> +/* exitcode.h - declare exitcode types */
>> +/*
>> + * GRUB -- GRand Unified Bootloader
>> + * Copyright (C) 2015 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/>.
>> + */
>> +
>> +#ifndef GRUB_EXITCODE_HEADER
>> +#define GRUB_EXITCODE_HEADER 1
>> +
>> +typedef enum
>> + {
>> + GRUB_EXITCODE_NONE = 0,
>> + GRUB_EXITCODE_NEXT,
>> + }
>> +grub_exitcode_t;
>> +
>> +#endif /* ! GRUB_EXITCODE_HEADER */
>> diff --git a/include/grub/misc.h b/include/grub/misc.h
>> index 2a9f87c..18dc77c 100644
>> --- a/include/grub/misc.h
>> +++ b/include/grub/misc.h
>> @@ -26,6 +26,7 @@
>> #include <grub/err.h>
>> #include <grub/i18n.h>
>> #include <grub/compiler.h>
>> +#include <grub/exitcode.h>
>>
>> #define ALIGN_UP(addr, align) \
>> ((addr + (typeof (addr)) align - 1) & ~((typeof (addr)) align -
>> 1))
>> @@ -334,7 +335,7 @@ int EXPORT_FUNC(grub_vsnprintf) (char *str,
>> grub_size_t n, const char *fmt,
>> char *EXPORT_FUNC(grub_xasprintf) (const char *fmt, ...)
>> __attribute__ ((format (GNU_PRINTF, 1, 2))) WARN_UNUSED_RESULT;
>> char *EXPORT_FUNC(grub_xvasprintf) (const char *fmt, va_list args)
>> WARN_UNUSED_RESULT;
>> -void EXPORT_FUNC(grub_exit) (void) __attribute__ ((noreturn));
>> +void EXPORT_FUNC(grub_exit) (grub_exitcode_t exitcode) __attribute__
>> ((noreturn));
>> grub_uint64_t EXPORT_FUNC(grub_divmod64) (grub_uint64_t n,
>> grub_uint64_t d,
>> grub_uint64_t *r);
>> --
>> 2.5.0
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> address@hidden
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>
>
>
>
> --
> --
> Ben Hildred
> Automation Support Services
> 303 815 6721
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

_______________________________________________
Grub-devel mailing list
address@hidden
https://lists.gnu.org/mailman/listinfo/grub-devel

Ben Hildred

Automation Support Services

303 815 6721

From:	Ben Hildred
Subject:	Re: [PATCH] [RFC] Add exitcode support
Date:	Tue, 18 Aug 2015 13:17:05 -0600