[PATCH 0/3] Search for specific config files using UUID, MAC and IP

[PATCH 0/3] Search for specific config files using UUID, MAC and IP

[Javier Martinez Canillas]

Hello,

This series contains some patches that have been carried in Fedora (and
other distros) for a long time. It allows to fetch GRUB configurations
that are customized for each machine by using the client UUID, MAC and
IP addresses as suffixes for the grub.cfg file.

This procedure is similar to what is used by pxelinux and yaboot:
http://www.syslinux.org/wiki/index.php/PXELINUX#config

Patch #1 sets net_<interface>_client{id,uuid} environment variables by
using the DHCP client ID and UUID options if these are found.

Patch #2 adds a %X format specifier to make the grub_printf() function
more similar to printf() and not require to use %x plus grub_toupper().

Finally Patch #3 adds the support to fetch the config files using the
UUID, MAC and IP as suffixes. If these aren't found, the default GRUB
config filename is used as a fallback.

Best regards,
Javier


Paulo Flabiano Smorigo (3):
  Set net_<interface>_client{id,uuid} variables from DHCP options
  Add %X option to printf functions
  Search for specific config files for netboot

 grub-core/kern/misc.c   |   7 ++-
 grub-core/net/bootp.c   |  85 ++++++++++++++++++++++++++---
 grub-core/net/net.c     | 117 ++++++++++++++++++++++++++++++++++++++++
 grub-core/normal/main.c |  17 ++++--
 include/grub/net.h      |   5 ++
 5 files changed, 217 insertions(+), 14 deletions(-)

-- 
2.21.0

[PATCH 1/3] Set net_<interface>_client{id, uuid} variables from DHCP options

[Javier Martinez Canillas]

From: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>

This patch sets a net_<interface>_clientid and net_<interface>_clientuuid
GRUB environment variables, using the DHCP client ID and UUID options if
these are found.

In the same way than net_<interface>_<option> variables are set for other
options such domain name, boot file, next server, etc.

Signed-off-by: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 grub-core/net/bootp.c | 85 +++++++++++++++++++++++++++++++++++++++----
 include/grub/net.h    |  2 +
 2 files changed, 79 insertions(+), 8 deletions(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 04cfbb04504..0e6e41a1699 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -95,6 +95,49 @@ enum
 /* Max timeout when waiting for BOOTP/DHCP reply */
 #define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
 
+static char *
+grub_env_write_readonly (struct grub_env_var *var __attribute__ ((unused)),
+                         const char *val __attribute__ ((unused)))
+{
+  return NULL;
+}
+
+static void
+set_env_limn_ro (const char *intername, const char *suffix,
+                 const char *value, grub_size_t len)
+{
+  char *varname, *varvalue;
+  char *ptr;
+  varname = grub_xasprintf ("net_%s_%s", intername, suffix);
+  if (!varname)
+    return;
+  for (ptr = varname; *ptr; ptr++)
+    if (*ptr == ':')
+      *ptr = '_';
+  varvalue = grub_malloc (len + 1);
+  if (!varvalue)
+    {
+      grub_free (varname);
+      return;
+    }
+
+  grub_memcpy (varvalue, value, len);
+  varvalue[len] = 0;
+  grub_env_set (varname, varvalue);
+  grub_register_variable_hook (varname, 0, grub_env_write_readonly);
+  grub_env_export (varname);
+  grub_free (varname);
+  grub_free (varvalue);
+}
+
+static char
+hexdigit (grub_uint8_t val)
+{
+  if (val < 10)
+    return val + '0';
+  return val + 'a' - 10;
+}
+
 static const void *
 find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
 		  grub_uint8_t opt_code, grub_uint8_t *opt_len)
@@ -152,6 +195,9 @@ again:
       if (i + taglength >= size)
 	return NULL;
 
+      grub_dprintf("net", "DHCP option %u (0x%02x) found with length %u.\n",
+                   tagtype, tagtype, taglength);
+
       /* FIXME RFC 3396 options concatentation */
       if (tagtype == opt_code)
 	{
@@ -354,6 +400,37 @@ grub_net_configure_by_dhcp_ack (const char *name,
     }
   grub_net_add_ipv4_local (inter, mask);
 
+  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_CLIENT_ID, &opt_len);
+  if (opt)
+    {
+      set_env_limn_ro (name, "clientid", (char *) opt, opt_len);
+    }
+
+  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_CLIENT_UUID, &opt_len);
+  if (opt && opt_len == 17)
+    {
+      /* The format is 9cfe245e-d0c8-bd45-a79f-54ea5fbd3d97 */
+
+      opt += 1;
+      opt_len -= 1;
+
+      char *val = grub_malloc (2 * opt_len + 4 + 1);
+      int i = 0;
+      int j = 0;
+      for (i = 0; i < opt_len; i++)
+        {
+          val[2 * i + j] = hexdigit (opt[i] >> 4);
+          val[2 * i + 1 + j] = hexdigit (opt[i] & 0xf);
+
+          if ((i == 3) || (i == 5) || (i == 7) || (i == 9))
+            {
+              j++;
+              val[2 * i + 1+ j] = '-';
+            }
+        }
+      set_env_limn_ro (name, "clientuuid", (char *) val, 2 * opt_len + 4);
+    }
+
   /* We do not implement dead gateway detection and the first entry SHOULD
      be preferred one */
   opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_ROUTER, &opt_len);
@@ -631,14 +708,6 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
     }
 }
 
-static char
-hexdigit (grub_uint8_t val)
-{
-  if (val < 10)
-    return val + '0';
-  return val + 'a' - 10;
-}
-
 static grub_err_t
 grub_cmd_dhcpopt (struct grub_command *cmd __attribute__ ((unused)),
 		  int argc, char **args)
diff --git a/include/grub/net.h b/include/grub/net.h
index 4a9069a1474..556c54e579f 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -462,6 +462,8 @@ enum
     GRUB_NET_BOOTP_DOMAIN = 0x0f,
     GRUB_NET_BOOTP_ROOT_PATH = 0x11,
     GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
+    GRUB_NET_BOOTP_CLIENT_ID = 0x3d,
+    GRUB_NET_BOOTP_CLIENT_UUID = 0x61,
     GRUB_NET_DHCP_REQUESTED_IP_ADDRESS = 50,
     GRUB_NET_DHCP_OVERLOAD = 52,
     GRUB_NET_DHCP_MESSAGE_TYPE = 53,
-- 
2.21.0

Re: [PATCH 1/3] Set net_<interface>_client{id, uuid} variables from DHCP options

[Daniel Kiper]

On Sat, Oct 05, 2019 at 12:44:25AM +0200, Javier Martinez Canillas wrote:
> From: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
>
> This patch sets a net_<interface>_clientid and net_<interface>_clientuuid
> GRUB environment variables, using the DHCP client ID and UUID options if
> these are found.
>
> In the same way than net_<interface>_<option> variables are set for other
> options such domain name, boot file, next server, etc.
>
> Signed-off-by: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

In general "Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>"
but a few nit picks below...

> ---
>
>  grub-core/net/bootp.c | 85 +++++++++++++++++++++++++++++++++++++++----
>  include/grub/net.h    |  2 +
>  2 files changed, 79 insertions(+), 8 deletions(-)
>
> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> index 04cfbb04504..0e6e41a1699 100644
> --- a/grub-core/net/bootp.c
> +++ b/grub-core/net/bootp.c
> @@ -95,6 +95,49 @@ enum
>  /* Max timeout when waiting for BOOTP/DHCP reply */
>  #define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
>
> +static char *
> +grub_env_write_readonly (struct grub_env_var *var __attribute__ ((unused)),
> +                         const char *val __attribute__ ((unused)))

s/grub_env_write_readonly/grub_env_ro_write/?

> +{
> +  return NULL;
> +}
> +
> +static void
> +set_env_limn_ro (const char *intername, const char *suffix,
> +                 const char *value, grub_size_t len)

I have hard time reading this. What does "limn" stand for?
Could we make the name of this function less cryptic?

> +{
> +  char *varname, *varvalue;
> +  char *ptr;

Please add empty line here.

> +  varname = grub_xasprintf ("net_%s_%s", intername, suffix);
> +  if (!varname)
> +    return;
> +  for (ptr = varname; *ptr; ptr++)
> +    if (*ptr == ':')
> +      *ptr = '_';
> +  varvalue = grub_malloc (len + 1);

grub_zalloc() and then you can...

> +  if (!varvalue)
> +    {
> +      grub_free (varname);
> +      return;
> +    }
> +
> +  grub_memcpy (varvalue, value, len);
> +  varvalue[len] = 0;

...drop this...

Daniel

> +  grub_env_set (varname, varvalue);
> +  grub_register_variable_hook (varname, 0, grub_env_write_readonly);
> +  grub_env_export (varname);
> +  grub_free (varname);
> +  grub_free (varvalue);
> +}
> +
> +static char
> +hexdigit (grub_uint8_t val)
> +{
> +  if (val < 10)
> +    return val + '0';
> +  return val + 'a' - 10;
> +}
> +
>  static const void *
>  find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
>  		  grub_uint8_t opt_code, grub_uint8_t *opt_len)
> @@ -152,6 +195,9 @@ again:
>        if (i + taglength >= size)
>  	return NULL;
>
> +      grub_dprintf("net", "DHCP option %u (0x%02x) found with length %u.\n",
> +                   tagtype, tagtype, taglength);
> +
>        /* FIXME RFC 3396 options concatentation */
>        if (tagtype == opt_code)
>  	{
> @@ -354,6 +400,37 @@ grub_net_configure_by_dhcp_ack (const char *name,
>      }
>    grub_net_add_ipv4_local (inter, mask);
>
> +  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_CLIENT_ID, &opt_len);
> +  if (opt)
> +    {
> +      set_env_limn_ro (name, "clientid", (char *) opt, opt_len);
> +    }
> +
> +  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_CLIENT_UUID, &opt_len);
> +  if (opt && opt_len == 17)
> +    {
> +      /* The format is 9cfe245e-d0c8-bd45-a79f-54ea5fbd3d97 */
> +
> +      opt += 1;
> +      opt_len -= 1;
> +
> +      char *val = grub_malloc (2 * opt_len + 4 + 1);
> +      int i = 0;
> +      int j = 0;
> +      for (i = 0; i < opt_len; i++)
> +        {
> +          val[2 * i + j] = hexdigit (opt[i] >> 4);
> +          val[2 * i + 1 + j] = hexdigit (opt[i] & 0xf);
> +
> +          if ((i == 3) || (i == 5) || (i == 7) || (i == 9))
> +            {
> +              j++;
> +              val[2 * i + 1+ j] = '-';
> +            }
> +        }
> +      set_env_limn_ro (name, "clientuuid", (char *) val, 2 * opt_len + 4);
> +    }
> +
>    /* We do not implement dead gateway detection and the first entry SHOULD
>       be preferred one */
>    opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_ROUTER, &opt_len);
> @@ -631,14 +708,6 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
>      }
>  }
>
> -static char
> -hexdigit (grub_uint8_t val)
> -{
> -  if (val < 10)
> -    return val + '0';
> -  return val + 'a' - 10;
> -}
> -
>  static grub_err_t
>  grub_cmd_dhcpopt (struct grub_command *cmd __attribute__ ((unused)),
>  		  int argc, char **args)
> diff --git a/include/grub/net.h b/include/grub/net.h
> index 4a9069a1474..556c54e579f 100644
> --- a/include/grub/net.h
> +++ b/include/grub/net.h
> @@ -462,6 +462,8 @@ enum
>      GRUB_NET_BOOTP_DOMAIN = 0x0f,
>      GRUB_NET_BOOTP_ROOT_PATH = 0x11,
>      GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
> +    GRUB_NET_BOOTP_CLIENT_ID = 0x3d,
> +    GRUB_NET_BOOTP_CLIENT_UUID = 0x61,
>      GRUB_NET_DHCP_REQUESTED_IP_ADDRESS = 50,
>      GRUB_NET_DHCP_OVERLOAD = 52,
>      GRUB_NET_DHCP_MESSAGE_TYPE = 53,
> --
> 2.21.0

Re: [PATCH 1/3] Set net_<interface>_client{id, uuid} variables from DHCP options

[Javier Martinez Canillas]

Hello Daniel,

On 10/15/19 4:19 PM, Daniel Kiper wrote:
> On Sat, Oct 05, 2019 at 12:44:25AM +0200, Javier Martinez Canillas wrote:
>> From: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
>>
>> This patch sets a net_<interface>_clientid and net_<interface>_clientuuid
>> GRUB environment variables, using the DHCP client ID and UUID options if
>> these are found.
>>
>> In the same way than net_<interface>_<option> variables are set for other
>> options such domain name, boot file, next server, etc.
>>
>> Signed-off-by: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> In general "Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>"
> but a few nit picks below...
>

Thanks for the review.

>> ---
>>
>>  grub-core/net/bootp.c | 85 +++++++++++++++++++++++++++++++++++++++----
>>  include/grub/net.h    |  2 +
>>  2 files changed, 79 insertions(+), 8 deletions(-)
>>
>> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
>> index 04cfbb04504..0e6e41a1699 100644
>> --- a/grub-core/net/bootp.c
>> +++ b/grub-core/net/bootp.c
>> @@ -95,6 +95,49 @@ enum
>>  /* Max timeout when waiting for BOOTP/DHCP reply */
>>  #define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
>>
>> +static char *
>> +grub_env_write_readonly (struct grub_env_var *var __attribute__ ((unused)),
>> +                         const char *val __attribute__ ((unused)))
> 
> s/grub_env_write_readonly/grub_env_ro_write/?
>

I noticed that this function already exists in grub-core/net/net.c, take a
look to commit e4dbf247b65 ("add grub_env_set_net_property function").

So I'll just remove this and the cryptic set_env_limn_ro() function.

>> +{
>> +  return NULL;
>> +}
>> +
>> +static void
>> +set_env_limn_ro (const char *intername, const char *suffix,
>> +                 const char *value, grub_size_t len)
> 
> I have hard time reading this. What does "limn" stand for?
> Could we make the name of this function less cryptic?
>

Yes, same. But this function also already exists in grub-core/net/net.c as
mentioned. It's called grub_env_set_net_property() and I'll use that.

Sorry for missing that this patch could be much more simpler after a rebase
to the latest GRUB version.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

[PATCH 2/3] Add %X option to printf functions

[Javier Martinez Canillas]

From: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>

The printf(3) function has support for the %X format specifier, to output
an unsigned hexadecimal integer in uppercase.

This can be achived in GRUB using the %x format specifier in grub_printf()
and calling grub_toupper(), but it is more convenient if there is support
for %X in grub_printf().

Signed-off-by: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 grub-core/kern/misc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 3b633d51f4c..76e7fb22872 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -588,7 +588,7 @@ grub_divmod64 (grub_uint64_t n, grub_uint64_t d, grub_uint64_t *r)
 static inline char *
 grub_lltoa (char *str, int c, unsigned long long n)
 {
-  unsigned base = (c == 'x') ? 16 : 10;
+  unsigned base = ((c == 'x') || (c == 'X')) ? 16 : 10;
   char *p;
 
   if ((long long) n < 0 && c == 'd')
@@ -603,7 +603,7 @@ grub_lltoa (char *str, int c, unsigned long long n)
     do
       {
 	unsigned d = (unsigned) (n & 0xf);
-	*p++ = (d > 9) ? d + 'a' - 10 : d + '0';
+	*p++ = (d > 9) ? d + ((c == 'x') ? 'a' : 'A') - 10 : d + '0';
       }
     while (n >>= 4);
   else
@@ -676,6 +676,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
 	{
 	case 'p':
 	case 'x':
+	case 'X':
 	case 'u':
 	case 'd':
 	case 'c':
@@ -762,6 +763,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
       switch (c)
 	{
 	case 'x':
+	case 'X':
 	case 'u':
 	  args->ptr[curn].type = UNSIGNED_INT + longfmt;
 	  break;
@@ -900,6 +902,7 @@ grub_vsnprintf_real (char *str, grub_size_t max_len, const char *fmt0,
 	  c = 'x';
 	  /* Fall through. */
 	case 'x':
+	case 'X':
 	case 'u':
 	case 'd':
 	  {
-- 
2.21.0

Re: [PATCH 2/3] Add %X option to printf functions

[Steve McIntyre]

On Sat, Oct 05, 2019 at 12:44:26AM +0200, Javier Martinez Canillas wrote:
>From: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
>
>The printf(3) function has support for the %X format specifier, to output
>an unsigned hexadecimal integer in uppercase.
>
>This can be achived in GRUB using the %x format specifier in grub_printf()
>and calling grub_toupper(), but it is more convenient if there is support
>for %X in grub_printf().
>
>Signed-off-by: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
>Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>---
>
> grub-core/kern/misc.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
>index 3b633d51f4c..76e7fb22872 100644
>--- a/grub-core/kern/misc.c
>+++ b/grub-core/kern/misc.c
>@@ -588,7 +588,7 @@ grub_divmod64 (grub_uint64_t n, grub_uint64_t d, grub_uint64_t *r)
> static inline char *
> grub_lltoa (char *str, int c, unsigned long long n)
> {
>-  unsigned base = (c == 'x') ? 16 : 10;
>+  unsigned base = ((c == 'x') || (c == 'X')) ? 16 : 10;
>   char *p;
> 
>   if ((long long) n < 0 && c == 'd')
>@@ -603,7 +603,7 @@ grub_lltoa (char *str, int c, unsigned long long n)
>     do
>       {
> 	unsigned d = (unsigned) (n & 0xf);
>-	*p++ = (d > 9) ? d + 'a' - 10 : d + '0';
>+	*p++ = (d > 9) ? d + ((c == 'x') ? 'a' : 'A') - 10 : d + '0';
>       }
>     while (n >>= 4);
>   else
>@@ -676,6 +676,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
> 	{
> 	case 'p':
> 	case 'x':
>+	case 'X':
> 	case 'u':
> 	case 'd':
> 	case 'c':
>@@ -762,6 +763,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
>       switch (c)
> 	{
> 	case 'x':
>+	case 'X':
> 	case 'u':
> 	  args->ptr[curn].type = UNSIGNED_INT + longfmt;
> 	  break;
>@@ -900,6 +902,7 @@ grub_vsnprintf_real (char *str, grub_size_t max_len, const char *fmt0,
> 	  c = 'x';
> 	  /* Fall through. */
> 	case 'x':
>+	case 'X':
> 	case 'u':
> 	case 'd':
> 	  {

Ummm, isn't this exactly the same patch that Colin Watson proposed and
I reviewed back in March, as part of his changeset for UEFI?

https://lists.gnu.org/archive/html/grub-devel/2019-03/msg00121.html

Oh, that never got applied. :-(

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
  Mature Sporty Personal
  More Innovation More Adult
  A Man in Dandism
  Powered Midship Specialty

Re: [PATCH 2/3] Add %X option to printf functions

[Javier Martinez Canillas]

Hello Steve,

On 10/5/19 3:47 AM, Steve McIntyre wrote:
> On Sat, Oct 05, 2019 at 12:44:26AM +0200, Javier Martinez Canillas wrote:

[snip]

>> 	  c = 'x';
>> 	  /* Fall through. */
>> 	case 'x':
>> +	case 'X':
>> 	case 'u':
>> 	case 'd':
>> 	  {
> 
> Ummm, isn't this exactly the same patch that Colin Watson proposed and
> I reviewed back in March, as part of his changeset for UEFI?
> 
> https://lists.gnu.org/archive/html/grub-devel/2019-03/msg00121.html
> 
> Oh, that never got applied. :-(
> 

That patch does the same indeed. If that gets applied then
I could just drop this patch from the series.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH 2/3] Add %X option to printf functions

[Daniel Kiper]

On Sat, Oct 05, 2019 at 02:47:23AM +0100, Steve McIntyre wrote:
> On Sat, Oct 05, 2019 at 12:44:26AM +0200, Javier Martinez Canillas wrote:
> >From: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
> >
> >The printf(3) function has support for the %X format specifier, to output
> >an unsigned hexadecimal integer in uppercase.
> >
> >This can be achived in GRUB using the %x format specifier in grub_printf()
> >and calling grub_toupper(), but it is more convenient if there is support
> >for %X in grub_printf().
> >
> >Signed-off-by: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
> >Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >---
> >
> > grub-core/kern/misc.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> >diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
> >index 3b633d51f4c..76e7fb22872 100644
> >--- a/grub-core/kern/misc.c
> >+++ b/grub-core/kern/misc.c
> >@@ -588,7 +588,7 @@ grub_divmod64 (grub_uint64_t n, grub_uint64_t d, grub_uint64_t *r)
> > static inline char *
> > grub_lltoa (char *str, int c, unsigned long long n)
> > {
> >-  unsigned base = (c == 'x') ? 16 : 10;
> >+  unsigned base = ((c == 'x') || (c == 'X')) ? 16 : 10;
> >   char *p;
> >
> >   if ((long long) n < 0 && c == 'd')
> >@@ -603,7 +603,7 @@ grub_lltoa (char *str, int c, unsigned long long n)
> >     do
> >       {
> > 	unsigned d = (unsigned) (n & 0xf);
> >-	*p++ = (d > 9) ? d + 'a' - 10 : d + '0';
> >+	*p++ = (d > 9) ? d + ((c == 'x') ? 'a' : 'A') - 10 : d + '0';
> >       }
> >     while (n >>= 4);
> >   else
> >@@ -676,6 +676,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
> > 	{
> > 	case 'p':
> > 	case 'x':
> >+	case 'X':
> > 	case 'u':
> > 	case 'd':
> > 	case 'c':
> >@@ -762,6 +763,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
> >       switch (c)
> > 	{
> > 	case 'x':
> >+	case 'X':
> > 	case 'u':
> > 	  args->ptr[curn].type = UNSIGNED_INT + longfmt;
> > 	  break;
> >@@ -900,6 +902,7 @@ grub_vsnprintf_real (char *str, grub_size_t max_len, const char *fmt0,
> > 	  c = 'x';
> > 	  /* Fall through. */
> > 	case 'x':
> >+	case 'X':
> > 	case 'u':
> > 	case 'd':
> > 	  {
>
> Ummm, isn't this exactly the same patch that Colin Watson proposed and
> I reviewed back in March, as part of his changeset for UEFI?
>
> https://lists.gnu.org/archive/html/grub-devel/2019-03/msg00121.html
>
> Oh, that never got applied. :-(

Well, IIRC because the whole patch series was not taken... :-(((
I hope that this time we will take it.

Daniel

Re: [PATCH 2/3] Add %X option to printf functions

[Daniel Kiper]

On Sat, Oct 05, 2019 at 12:44:26AM +0200, Javier Martinez Canillas wrote:
> From: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
>
> The printf(3) function has support for the %X format specifier, to output
> an unsigned hexadecimal integer in uppercase.
>
> This can be achived in GRUB using the %x format specifier in grub_printf()
> and calling grub_toupper(), but it is more convenient if there is support
> for %X in grub_printf().
>
> Signed-off-by: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

But I would like to see this as a first patch...

Daniel

[PATCH 3/3] Search for specific config files for netboot

[Javier Martinez Canillas]

From: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>

This patch implements a search for a specific configuration when the config
file is on a remoteserver. It uses the following order:
   1) DHCP client UUID option.
   2) MAC address (in lower case hexadecimal with dash separators);
   3) IP (in upper case hexadecimal) or IPv6;
   4) The original grub.cfg file.

This procedure is similar to what is used by pxelinux and yaboot:
http://www.syslinux.org/wiki/index.php/PXELINUX#config

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=873406

Signed-off-by: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

---

 grub-core/net/net.c     | 117 ++++++++++++++++++++++++++++++++++++++++
 grub-core/normal/main.c |  17 ++++--
 include/grub/net.h      |   3 ++
 3 files changed, 133 insertions(+), 4 deletions(-)

diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index d5d726a315e..6492bb74274 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -1735,6 +1735,123 @@ grub_net_restore_hw (void)
   return GRUB_ERR_NONE;
 }
 
+grub_err_t
+grub_net_search_configfile (char *config)
+{
+  grub_size_t config_len;
+  char *suffix;
+
+  auto int search_through (grub_size_t num_tries, grub_size_t slice_size);
+  int search_through (grub_size_t num_tries, grub_size_t slice_size)
+  {
+    while (num_tries-- > 0)
+      {
+        grub_dprintf ("net", "attempt to fetch config %s\n", config);
+
+        grub_file_t file;
+        file = grub_file_open (config, GRUB_FILE_TYPE_CONFIG);
+
+        if (file)
+          {
+            grub_file_close (file);
+            return 0;
+          }
+        else
+          {
+            if (grub_errno == GRUB_ERR_IO)
+              grub_errno = GRUB_ERR_NONE;
+          }
+
+        if (grub_strlen (suffix) < slice_size)
+          break;
+
+        config[grub_strlen (config) - slice_size] = '\0';
+      }
+
+    return 1;
+  }
+
+  config_len = grub_strlen (config);
+  config[config_len] = '-';
+  suffix = config + config_len + 1;
+
+  struct grub_net_network_level_interface *inf;
+  FOR_NET_NETWORK_LEVEL_INTERFACES (inf)
+    {
+      /* By the Client UUID. */
+
+      char client_uuid_var[sizeof ("net_") + grub_strlen (inf->name) +
+                           sizeof ("_clientuuid") + 1];
+      grub_snprintf (client_uuid_var, sizeof (client_uuid_var),
+                     "net_%s_clientuuid", inf->name);
+
+      const char *client_uuid;
+      client_uuid = grub_env_get (client_uuid_var);
+
+      if (client_uuid)
+        {
+          grub_strcpy (suffix, client_uuid);
+          if (search_through (1, 0) == 0) return GRUB_ERR_NONE;
+        }
+
+      /* By the MAC address. */
+
+      /* Add ethernet type */
+      grub_strcpy (suffix, "01-");
+
+      grub_net_hwaddr_to_str (&inf->hwaddress, suffix + 3);
+
+      char *ptr;
+      for (ptr = suffix; *ptr; ptr++)
+        if (*ptr == ':')
+          *ptr = '-';
+
+      if (search_through (1, 0) == 0) return GRUB_ERR_NONE;
+
+      /* By IP address */
+
+      switch ((&inf->address)->type)
+        {
+        case GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4:
+          {
+            grub_uint32_t n = grub_be_to_cpu32 ((&inf->address)->ipv4);
+            grub_snprintf (suffix, GRUB_NET_MAX_STR_ADDR_LEN, "%02X%02X%02X%02X", \
+                           ((n >> 24) & 0xff), ((n >> 16) & 0xff),      \
+                           ((n >> 8) & 0xff), ((n >> 0) & 0xff));
+
+            if (search_through (8, 1) == 0) return GRUB_ERR_NONE;
+            break;
+          }
+        case GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6:
+          {
+            char buf[GRUB_NET_MAX_STR_ADDR_LEN];
+            struct grub_net_network_level_address base;
+            base.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
+            grub_memcpy (&base.ipv6, ((&inf->address)->ipv6), 16);
+            grub_net_addr_to_str (&base, buf);
+
+            for (ptr = buf; *ptr; ptr++)
+              if (*ptr == ':')
+                *ptr = '-';
+
+            grub_snprintf (suffix, GRUB_NET_MAX_STR_ADDR_LEN, "%s", buf);
+            if (search_through (1, 0) == 0) return GRUB_ERR_NONE;
+            break;
+          }
+        case GRUB_NET_NETWORK_LEVEL_PROTOCOL_DHCP_RECV:
+          return grub_error (GRUB_ERR_BUG, "shouldn't reach here");
+        default:
+          return grub_error (GRUB_ERR_BUG,
+                             "unsupported address type %d", (&inf->address)->type);
+        }
+    }
+
+  /* Remove the remaining minus sign at the end. */
+  config[config_len] = '\0';
+
+  return GRUB_ERR_NONE;
+}
+
 static struct grub_preboot *fini_hnd;
 
 static grub_command_t cmd_addaddr, cmd_deladdr, cmd_addroute, cmd_delroute;
diff --git a/grub-core/normal/main.c b/grub-core/normal/main.c
index 1b03dfd57b9..62ad546f4f7 100644
--- a/grub-core/normal/main.c
+++ b/grub-core/normal/main.c
@@ -323,10 +323,19 @@ grub_cmd_normal (struct grub_command *cmd __attribute__ ((unused)),
 
       prefix = grub_env_get ("prefix");
       if (prefix)
-	{
-	  config = grub_xasprintf ("%s/grub.cfg", prefix);
-	  if (! config)
-	    goto quit;
+        {
+          grub_size_t config_len;
+          config_len = grub_strlen (prefix) +
+                      sizeof ("/grub.cfg-XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX");
+          config = grub_malloc (config_len);
+
+          if (! config)
+            goto quit;
+
+          grub_snprintf (config, config_len, "%s/grub.cfg", prefix);
+
+          if (grub_strncmp (prefix + 1, "tftp", sizeof ("tftp") - 1) == 0)
+            grub_net_search_configfile (config);
 
 	  grub_enter_normal_mode (config);
 	  grub_free (config);
diff --git a/include/grub/net.h b/include/grub/net.h
index 556c54e579f..ff6d347f7da 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -578,4 +578,7 @@ extern char *grub_net_default_server;
 
 #define VLANTAG_IDENTIFIER 0x8100
 
+grub_err_t
+grub_net_search_configfile (char *config);
+
 #endif /* ! GRUB_NET_HEADER */
-- 
2.21.0

Re: [PATCH 3/3] Search for specific config files for netboot

[Daniel Kiper]

On Sat, Oct 05, 2019 at 12:44:27AM +0200, Javier Martinez Canillas wrote:
> From: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
>
> This patch implements a search for a specific configuration when the config
> file is on a remoteserver. It uses the following order:
>    1) DHCP client UUID option.
>    2) MAC address (in lower case hexadecimal with dash separators);
>    3) IP (in upper case hexadecimal) or IPv6;
>    4) The original grub.cfg file.
>
> This procedure is similar to what is used by pxelinux and yaboot:
> http://www.syslinux.org/wiki/index.php/PXELINUX#config
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=873406
>
> Signed-off-by: Paulo Flabiano Smorigo <pfsmorigo@br.ibm.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

However, one nitpick below...

> ---
>
>  grub-core/net/net.c     | 117 ++++++++++++++++++++++++++++++++++++++++
>  grub-core/normal/main.c |  17 ++++--
>  include/grub/net.h      |   3 ++
>  3 files changed, 133 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index d5d726a315e..6492bb74274 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -1735,6 +1735,123 @@ grub_net_restore_hw (void)
>    return GRUB_ERR_NONE;
>  }
>
> +grub_err_t
> +grub_net_search_configfile (char *config)

s/grub_net_search_configfile/grub_net_search_config_file/

Daniel

> +{
> +  grub_size_t config_len;
> +  char *suffix;
> +
> +  auto int search_through (grub_size_t num_tries, grub_size_t slice_size);
> +  int search_through (grub_size_t num_tries, grub_size_t slice_size)
> +  {
> +    while (num_tries-- > 0)
> +      {
> +        grub_dprintf ("net", "attempt to fetch config %s\n", config);
> +
> +        grub_file_t file;
> +        file = grub_file_open (config, GRUB_FILE_TYPE_CONFIG);
> +
> +        if (file)
> +          {
> +            grub_file_close (file);
> +            return 0;
> +          }
> +        else
> +          {
> +            if (grub_errno == GRUB_ERR_IO)
> +              grub_errno = GRUB_ERR_NONE;
> +          }
> +
> +        if (grub_strlen (suffix) < slice_size)
> +          break;
> +
> +        config[grub_strlen (config) - slice_size] = '\0';
> +      }
> +
> +    return 1;
> +  }
> +
> +  config_len = grub_strlen (config);
> +  config[config_len] = '-';
> +  suffix = config + config_len + 1;
> +
> +  struct grub_net_network_level_interface *inf;
> +  FOR_NET_NETWORK_LEVEL_INTERFACES (inf)
> +    {
> +      /* By the Client UUID. */
> +
> +      char client_uuid_var[sizeof ("net_") + grub_strlen (inf->name) +
> +                           sizeof ("_clientuuid") + 1];
> +      grub_snprintf (client_uuid_var, sizeof (client_uuid_var),
> +                     "net_%s_clientuuid", inf->name);
> +
> +      const char *client_uuid;
> +      client_uuid = grub_env_get (client_uuid_var);
> +
> +      if (client_uuid)
> +        {
> +          grub_strcpy (suffix, client_uuid);
> +          if (search_through (1, 0) == 0) return GRUB_ERR_NONE;
> +        }
> +
> +      /* By the MAC address. */
> +
> +      /* Add ethernet type */
> +      grub_strcpy (suffix, "01-");
> +
> +      grub_net_hwaddr_to_str (&inf->hwaddress, suffix + 3);
> +
> +      char *ptr;
> +      for (ptr = suffix; *ptr; ptr++)
> +        if (*ptr == ':')
> +          *ptr = '-';
> +
> +      if (search_through (1, 0) == 0) return GRUB_ERR_NONE;
> +
> +      /* By IP address */
> +
> +      switch ((&inf->address)->type)
> +        {
> +        case GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4:
> +          {
> +            grub_uint32_t n = grub_be_to_cpu32 ((&inf->address)->ipv4);
> +            grub_snprintf (suffix, GRUB_NET_MAX_STR_ADDR_LEN, "%02X%02X%02X%02X", \
> +                           ((n >> 24) & 0xff), ((n >> 16) & 0xff),      \
> +                           ((n >> 8) & 0xff), ((n >> 0) & 0xff));
> +
> +            if (search_through (8, 1) == 0) return GRUB_ERR_NONE;
> +            break;
> +          }
> +        case GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6:
> +          {
> +            char buf[GRUB_NET_MAX_STR_ADDR_LEN];
> +            struct grub_net_network_level_address base;
> +            base.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> +            grub_memcpy (&base.ipv6, ((&inf->address)->ipv6), 16);
> +            grub_net_addr_to_str (&base, buf);
> +
> +            for (ptr = buf; *ptr; ptr++)
> +              if (*ptr == ':')
> +                *ptr = '-';
> +
> +            grub_snprintf (suffix, GRUB_NET_MAX_STR_ADDR_LEN, "%s", buf);
> +            if (search_through (1, 0) == 0) return GRUB_ERR_NONE;
> +            break;
> +          }
> +        case GRUB_NET_NETWORK_LEVEL_PROTOCOL_DHCP_RECV:
> +          return grub_error (GRUB_ERR_BUG, "shouldn't reach here");
> +        default:
> +          return grub_error (GRUB_ERR_BUG,
> +                             "unsupported address type %d", (&inf->address)->type);
> +        }
> +    }
> +
> +  /* Remove the remaining minus sign at the end. */
> +  config[config_len] = '\0';
> +
> +  return GRUB_ERR_NONE;
> +}
> +
>  static struct grub_preboot *fini_hnd;
>
>  static grub_command_t cmd_addaddr, cmd_deladdr, cmd_addroute, cmd_delroute;
> diff --git a/grub-core/normal/main.c b/grub-core/normal/main.c
> index 1b03dfd57b9..62ad546f4f7 100644
> --- a/grub-core/normal/main.c
> +++ b/grub-core/normal/main.c
> @@ -323,10 +323,19 @@ grub_cmd_normal (struct grub_command *cmd __attribute__ ((unused)),
>
>        prefix = grub_env_get ("prefix");
>        if (prefix)
> -	{
> -	  config = grub_xasprintf ("%s/grub.cfg", prefix);
> -	  if (! config)
> -	    goto quit;
> +        {
> +          grub_size_t config_len;
> +          config_len = grub_strlen (prefix) +
> +                      sizeof ("/grub.cfg-XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX");
> +          config = grub_malloc (config_len);
> +
> +          if (! config)
> +            goto quit;
> +
> +          grub_snprintf (config, config_len, "%s/grub.cfg", prefix);
> +
> +          if (grub_strncmp (prefix + 1, "tftp", sizeof ("tftp") - 1) == 0)
> +            grub_net_search_configfile (config);
>
>  	  grub_enter_normal_mode (config);
>  	  grub_free (config);
> diff --git a/include/grub/net.h b/include/grub/net.h
> index 556c54e579f..ff6d347f7da 100644
> --- a/include/grub/net.h
> +++ b/include/grub/net.h
> @@ -578,4 +578,7 @@ extern char *grub_net_default_server;
>
>  #define VLANTAG_IDENTIFIER 0x8100
>
> +grub_err_t
> +grub_net_search_configfile (char *config);
> +
>  #endif /* ! GRUB_NET_HEADER */
> --
> 2.21.0

Re: [PATCH 0/3] Search for specific config files using UUID, MAC and IP

[Daniel Kiper]

On Sat, Oct 05, 2019 at 12:44:24AM +0200, Javier Martinez Canillas wrote:
> Hello,
>
> This series contains some patches that have been carried in Fedora (and
> other distros) for a long time. It allows to fetch GRUB configurations
> that are customized for each machine by using the client UUID, MAC and
> IP addresses as suffixes for the grub.cfg file.
>
> This procedure is similar to what is used by pxelinux and yaboot:
> http://www.syslinux.org/wiki/index.php/PXELINUX#config
>
> Patch #1 sets net_<interface>_client{id,uuid} environment variables by
> using the DHCP client ID and UUID options if these are found.
>
> Patch #2 adds a %X format specifier to make the grub_printf() function
> more similar to printf() and not require to use %x plus grub_toupper().
>
> Finally Patch #3 adds the support to fetch the config files using the
> UUID, MAC and IP as suffixes. If these aren't found, the default GRUB
> config filename is used as a fallback.

Except some nitpicks LGTM. You can repost the patch series or I can
fix these minor things before applying.

Daniel

Re: [PATCH 0/3] Search for specific config files using UUID, MAC and IP

[Javier Martinez Canillas]

Hello Daniel,

On 10/15/19 4:37 PM, Daniel Kiper wrote:
> On Sat, Oct 05, 2019 at 12:44:24AM +0200, Javier Martinez Canillas wrote:
>> Hello,
>>
>> This series contains some patches that have been carried in Fedora (and
>> other distros) for a long time. It allows to fetch GRUB configurations
>> that are customized for each machine by using the client UUID, MAC and
>> IP addresses as suffixes for the grub.cfg file.
>>
>> This procedure is similar to what is used by pxelinux and yaboot:
>> http://www.syslinux.org/wiki/index.php/PXELINUX#config
>>
>> Patch #1 sets net_<interface>_client{id,uuid} environment variables by
>> using the DHCP client ID and UUID options if these are found.
>>
>> Patch #2 adds a %X format specifier to make the grub_printf() function
>> more similar to printf() and not require to use %x plus grub_toupper().
>>
>> Finally Patch #3 adds the support to fetch the config files using the
>> UUID, MAC and IP as suffixes. If these aren't found, the default GRUB
>> config filename is used as a fallback.
> 
> Except some nitpicks LGTM. You can repost the patch series or I can
> fix these minor things before applying.
>

Thanks for the feedback. I've posted a v2 that addresses all the issues
you pointed out. Please let me know if anything else is missing.

> Daniel
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat