Code review comment for ~juliank/grub/+git/ubuntu:ubuntu

No, s390x does not use grub. It uses zipl (z itial program load).

PowerPC uses prep partitions & petiboot.

Some armhf, arm64, riscv64 use grub.

Amd64 uses grub.

i386 is dead to us.

On Wed, 11 Mar 2020, 17:00 Seth Arnold, <email address hidden> wrote:

> I've marked a spot where a char comparison may not necessarily be useful
> on systems with signed-by-default chars like the s390. Does this code ever
> run on any of those architectures?
>
> Thanks
>
> Diff comments:
>
> > diff --git
> a/debian/patches/0076-smbios-Add-a-linux-argument-to-apply-linux-modalias-.patch
> b/debian/patches/0076-smbios-Add-a-linux-argument-to-apply-linux-modalias-.patch
> > new file mode 100644
> > index 0000000..f656509
> > --- /dev/null
> > +++
> b/debian/patches/0076-smbios-Add-a-linux-argument-to-apply-linux-modalias-.patch
> > @@ -0,0 +1,86 @@
> > +From 79d4ae1df7b63ef106a85ad18bfedb6d709ef29a Mon Sep 17 00:00:00 2001
> > +From: Julian Andres Klode <email address hidden>
> > +Date: Tue, 3 Mar 2020 16:06:34 +0100
> > +Subject: smbios: Add a --linux argument to apply linux modalias-like
> filtering
> > +
> > +Linux creates modalias strings by filtering out non-ASCII, space,
> > +and colon characters. Provide an option that does the same filtering
> > +so people can create a modalias string in GRUB, and then match their
> > +modalias patterns against it.
> > +
> > +Signed-off-by: Julian Andres Klode <email address hidden>
> > +Reviewed-by: Daniel Kiper <email address hidden>
> > +Origin: upstream,
> https://git.savannah.gnu.org/cgit/grub.git/commit/?id=87049f9716fb095aecb595fb8f45497bbbb1b4a2
> > +---
> > + grub-core/commands/smbios.c | 24 ++++++++++++++++++++++++
> > + 1 file changed, 24 insertions(+)
> > +
> > +diff --git a/grub-core/commands/smbios.c b/grub-core/commands/smbios.c
> > +index 7a6a391fc..1a9086ddd 100644
> > +--- a/grub-core/commands/smbios.c
> > ++++ b/grub-core/commands/smbios.c
> > +@@ -64,6 +64,21 @@ grub_smbios_get_eps3 (void)
> > + return eps;
> > + }
> > +
> > ++static char *
> > ++linux_string (const char *value)
> > ++{
> > ++ char *out = grub_malloc( grub_strlen (value) + 1);
> > ++ const char *src = value;
> > ++ char *dst = out;
> > ++
> > ++ for (; *src; src++)
> > ++ if (*src > ' ' && *src < 127 && *src != ':')
>
> Is grub ever used on architectures with signed-by-default chars like
> s390x? (Are there others?)
>
> > ++ *dst++ = *src;
> > ++
> > ++ *dst = 0;
> > ++ return out;
> > ++}
> > ++
> > + /*
> > + * These functions convert values from the various SMBIOS structure
> field types
> > + * into a string formatted to be returned to the user. They expect
> that the
> > +@@ -176,6 +191,7 @@ static const struct {
> > + /* List command options, with structure field getters ordered as
> above. */
> > + #define FIRST_GETTER_OPT (3)
> > + #define SETTER_OPT (FIRST_GETTER_OPT + ARRAY_SIZE(field_extractors))
> > ++#define LINUX_OPT (FIRST_GETTER_OPT + ARRAY_SIZE(field_extractors) + 1)
> > +
> > + static const struct grub_arg_option options[] = {
> > + {"type", 't', 0, N_("Match structures with the given type."),
> > +@@ -198,6 +214,8 @@ static const struct grub_arg_option options[] = {
> > + N_("offset"), ARG_TYPE_INT},
> > + {"set", '\0', 0, N_("Store the value in the given variable
> name."),
> > + N_("variable"), ARG_TYPE_STRING},
> > ++ {"linux", '\0', 0, N_("Filter the result like linux does."),
> > ++ N_("variable"), ARG_TYPE_NONE},
> > + {0, 0, 0, 0, 0, 0}
> > + };
> > +
> > +@@ -261,6 +279,7 @@ grub_cmd_smbios (grub_extcmd_context_t ctxt,
> > +
> > + const grub_uint8_t *structure;
> > + const char *value;
> > ++ char *modified_value = NULL;
> > + grub_int32_t option;
> > + grub_int8_t field_type = -1;
> > + grub_uint8_t i;
> > +@@ -334,12 +353,17 @@ grub_cmd_smbios (grub_extcmd_context_t ctxt,
> > + return grub_error (GRUB_ERR_IO,
> > + N_("failed to retrieve the structure field"));
> > +
> > ++ if (state[LINUX_OPT].set)
> > ++ value = modified_value = linux_string (value);
> > ++
> > + /* Store or print the formatted value. */
> > + if (state[SETTER_OPT].set)
> > + grub_env_set (state[SETTER_OPT].arg, value);
> > + else
> > + grub_printf ("%s\n", value);
> > +
> > ++ grub_free(modified_value);
> > ++
> > + return GRUB_ERR_NONE;
> > + }
> > +
>
>
> --
> https://code.launchpad.net/~juliank/grub/+git/ubuntu/+merge/380548
> You are reviewing the proposed merge of ~juliank/grub/+git/ubuntu:ubuntu
> into ~ubuntu-core-dev/grub/+git/ubuntu:ubuntu.
>