On Tue, Aug 18, 2015 at 1:17 PM, Ben Hildred <426...@gmail.com> wrote:
>
>
> On Tue, Aug 18, 2015 at 12:56 PM, Dann Frazier <dann.fraz...@canonical.com>
> wrote:
>>
>> On Tue, Aug 18, 2015 at 12:03 PM, Ben Hildred <426...@gmail.com> 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?
I'm not sure a boolean context makes sense here. Any return from a
bootloader could be interpreted as some kind of error, so what would a
generic success imply?. What I really want is a way to give hints to
the platform for what to do next, if the platform supports them. And
that has me now leaning more towards flags (see below).
>>
>> 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?
In this architecture, that'd be up to the platform grub_exit()
implementation. If
that code knows the platform does not support a "next" equivalent, it would
presumably return an error (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?
If the platform doesn't support that feature, it should return a
reasonable value for that platform. Presumably an error, if the
platform supports them.
>>
>> > 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?
Perhaps this would be better handled as one or more grub variables
that can be set before exit; e.g. 'set exit_try_next=1'. That would
also allow for multiple hints to be set if that ever became
interesting.
-dann
>> -dann
>>
>> > On Tue, Aug 18, 2015 at 11:30 AM, dann frazier
>> > <dann.fraz...@canonical.com>
>> > 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 <dann.fraz...@canonical.com>
>> >> ---
>> >> 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
>> >> Grub-devel@gnu.org
>> >> https://lists.gnu.org/mailman/listinfo/grub-devel
>> >
>> >
>> >
>> >
>> > --
>> > --
>> > Ben Hildred
>> > Automation Support Services
>> > 303 815 6721
>> >
>> > _______________________________________________
>> > Grub-devel mailing list
>> > Grub-devel@gnu.org
>> > https://lists.gnu.org/mailman/listinfo/grub-devel
>> >
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>
>
>
>
> --
> --
> Ben Hildred
> Automation Support Services
> 303 815 6721
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
