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

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

[Javier Martinez Canillas]

Hello,

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

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

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

Patch #2 is just a cleanup of the bootp/dhcp options enum to consistently use
hexadecimal numbers for all the values.

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

Patch #4 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.

Patch #5 adds documentation that specifies the order netboot clients use to
select a grub configuration file.

Best regards,
Javier

Changes in v4:
- Use decimal instead of hexadecimal numbers to be consistent with RFC 2132.
- Mention that the UUID is unique to the machine and not the NIC and also
  refer to the RFC 4578 where the client UUID DHCP option is specified.

Changes in v3:
- Add patch to cleanup bootp/dhcp options enum.
- Use decimal numbers for the newly added DHCP client ID and UUID options.
- Add Reviewed-by tag from Daniel Kiper.
- Add a patch documenting the grub.cfg netboot selection order.

Changes in v2:
- Put patch that adds the %X format specifier first in the series.
- Use the existing grub_env_set_net_property() and remove duplicated code.

Javier Martinez Canillas (1):
  net/dhcp: Consistently use decimal numbers for DHCP/BOOTP options enum

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

Robert Marshall (1):
  docs: Update info with grub.cfg netboot selection order

 docs/grub.texi          |  46 ++++++++++++++++
 grub-core/kern/misc.c   |   7 ++-
 grub-core/net/bootp.c   |  48 ++++++++++++++---
 grub-core/net/net.c     | 117 ++++++++++++++++++++++++++++++++++++++++
 grub-core/normal/main.c |  17 ++++--
 include/grub/net.h      |  24 +++++----
 6 files changed, 236 insertions(+), 23 deletions(-)

-- 
2.21.0

[PATCH v4 1/5] 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>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

---

Changes in v4: None
Changes in v3: None
Changes in v2:
- Put patch that adds the %X format specifier first in the series.

 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

[PATCH v4 2/5] net/dhcp: Consistently use decimal numbers for DHCP/BOOTP options enum

[Javier Martinez Canillas]

The DHCP Options and BOOTP Vendor Extensions enum values are a mixture of
decimal and hexadecimal numbers. Change this to consistently use decimal
numbers for all since that's how these values are defined by RFC 2132.

Suggested-by: Daniel Kiper <daniel.kiper@oracle.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

---

Changes in v4:
- Use decimal instead of hexadecimal numbers to be consistent with RFC 2132.

Changes in v3:
- Add patch to cleanup bootp/dhcp options enum.

Changes in v2: None

 include/grub/net.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/grub/net.h b/include/grub/net.h
index 4a9069a1474..b5fedb28b9c 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -454,14 +454,14 @@ struct grub_net_bootp_packet
 
 enum
   {
-    GRUB_NET_BOOTP_PAD = 0x00,
-    GRUB_NET_BOOTP_NETMASK = 0x01,
-    GRUB_NET_BOOTP_ROUTER = 0x03,
-    GRUB_NET_BOOTP_DNS = 0x06,
-    GRUB_NET_BOOTP_HOSTNAME = 0x0c,
-    GRUB_NET_BOOTP_DOMAIN = 0x0f,
-    GRUB_NET_BOOTP_ROOT_PATH = 0x11,
-    GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
+    GRUB_NET_BOOTP_PAD = 0,
+    GRUB_NET_BOOTP_NETMASK = 1,
+    GRUB_NET_BOOTP_ROUTER = 3,
+    GRUB_NET_BOOTP_DNS = 6,
+    GRUB_NET_BOOTP_HOSTNAME = 12,
+    GRUB_NET_BOOTP_DOMAIN = 15,
+    GRUB_NET_BOOTP_ROOT_PATH = 17,
+    GRUB_NET_BOOTP_EXTENSIONS_PATH = 18,
     GRUB_NET_DHCP_REQUESTED_IP_ADDRESS = 50,
     GRUB_NET_DHCP_OVERLOAD = 52,
     GRUB_NET_DHCP_MESSAGE_TYPE = 53,
@@ -469,7 +469,7 @@ enum
     GRUB_NET_DHCP_PARAMETER_REQUEST_LIST = 55,
     GRUB_NET_DHCP_TFTP_SERVER_NAME = 66,
     GRUB_NET_DHCP_BOOTFILE_NAME = 67,
-    GRUB_NET_BOOTP_END = 0xff
+    GRUB_NET_BOOTP_END = 255
   };
 
 struct grub_net_network_level_interface *
-- 
2.21.0

[PATCH v4 3/5] net/dhcp: 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>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

---

Changes in v4: None
Changes in v3:
- Use decimal numbers for the newly added DHCP client ID and UUID options.

Changes in v2:
- Use the existing grub_env_set_net_property() and remove duplicated code.

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

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 04cfbb04504..377006da938 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -95,6 +95,14 @@ enum
 /* Max timeout when waiting for BOOTP/DHCP reply */
 #define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
 
+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 +160,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)
 	{
@@ -406,6 +417,35 @@ grub_net_configure_by_dhcp_ack (const char *name,
   if (opt && opt_len)
     grub_env_set_net_property (name, "extensionspath", (const char *) opt, opt_len);
   
+  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_CLIENT_ID, &opt_len);
+  if (opt && opt_len)
+    grub_env_set_net_property (name, "clientid", (const 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] = '-';
+            }
+        }
+      grub_env_set_net_property (name, "clientuuid", (char *) val, 2 * opt_len + 4);
+    }
+
   inter->dhcp_ack = grub_malloc (size);
   if (inter->dhcp_ack)
     {
@@ -631,14 +671,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 b5fedb28b9c..9668808dbca 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -454,6 +454,7 @@ struct grub_net_bootp_packet
 
 enum
   {
+
     GRUB_NET_BOOTP_PAD = 0,
     GRUB_NET_BOOTP_NETMASK = 1,
     GRUB_NET_BOOTP_ROUTER = 3,
@@ -467,8 +468,10 @@ enum
     GRUB_NET_DHCP_MESSAGE_TYPE = 53,
     GRUB_NET_DHCP_SERVER_IDENTIFIER = 54,
     GRUB_NET_DHCP_PARAMETER_REQUEST_LIST = 55,
+    GRUB_NET_BOOTP_CLIENT_ID = 61,
     GRUB_NET_DHCP_TFTP_SERVER_NAME = 66,
     GRUB_NET_DHCP_BOOTFILE_NAME = 67,
+    GRUB_NET_BOOTP_CLIENT_UUID = 97,
     GRUB_NET_BOOTP_END = 255
   };
 
-- 
2.21.0

[PATCH v4 4/5] normal/main: 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>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

---

Changes in v4: None
Changes in v3:
- Add Reviewed-by tag from Daniel Kiper.

Changes in v2: None

 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..f066b02ebad 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_config_file (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..cdeb58f1a1e 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_config_file (config);
 
 	  grub_enter_normal_mode (config);
 	  grub_free (config);
diff --git a/include/grub/net.h b/include/grub/net.h
index 9668808dbca..d7a89bd39d1 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -579,4 +579,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

[PATCH v4 5/5] docs: Update info with grub.cfg netboot selection order

[Javier Martinez Canillas]

From: Robert Marshall <rmarshall@redhat.com>

Add documentation to the grub info page that specifies the order
netboot clients use to select a grub configuration file.

Signed-off-by: Robert Marshall <rmarshall@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

---

Changes in v4:
- Mention that the UUID is unique to the machine and not the NIC and also
  refer to the RFC 4578 where the client UUID DHCP option is specified.

Changes in v3:
- Add a patch documenting the grub.cfg netboot selection order.

Changes in v2: None

 docs/grub.texi | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/docs/grub.texi b/docs/grub.texi
index 5ac61c09d1b..2f69c47175e 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -2486,6 +2486,52 @@ grub-mknetdir --net-directory=/srv/tftp --subdir=/boot/grub -d /usr/lib/grub/i38
 Then follow instructions printed out by grub-mknetdir on configuring your DHCP
 server.
 
+The grub.cfg file is placed in the same directory as the path output by
+grub-mknetdir hereafter referred to as FWPATH. GRUB will search for its
+configuration files in order using the following rules where the appended
+value corresponds to a value on the client machine.
+
+@example
+@group
+@samp{(FWPATH)}/grub.cfg-@samp{(UUID OF MACHINE)}
+@samp{(FWPATH)}/grub.cfg-@samp{(MAC ADDRESS OF NIC)}
+@samp{(FWPATH)}/grub.cfg-@samp{(IPv4 OR IPv6 ADDRESS)}
+@samp{(FWPATH)}/grub.cfg
+@end group
+@end example
+
+The UUID is the Client Machine Identifier Option Definition as specified in
+RFC 4578. The client will only attempt to loouk up a UUID config file if it
+was provided by the DHCP server.
+
+The client will only attempt to look up an IPv6 address config once, however,
+it will try the IPv4 multiple times. The concrete example below shows what
+would happen under the IPv4 case.
+
+@example
+@group
+UUID: 7726a678-7fc0-4853-a4f6-c85ac36a120a
+MAC:  52:54:00:ec:33:81
+IPV4: 10.0.0.130 (0A000082)
+@end group
+@end example
+
+@example
+@group
+@samp{(FWPATH)}/grub.cfg-7726a678-7fc0-4853-a4f6-c85ac36a120a
+@samp{(FWPATH)}/grub.cfg-52-54-00-ec-33-81
+@samp{(FWPATH)}/grub.cfg-0A000082
+@samp{(FWPATH)}/grub.cfg-0A00008
+@samp{(FWPATH)}/grub.cfg-0A0000
+@samp{(FWPATH)}/grub.cfg-0A000
+@samp{(FWPATH)}/grub.cfg-0A00
+@samp{(FWPATH)}/grub.cfg-0A0
+@samp{(FWPATH)}/grub.cfg-0A
+@samp{(FWPATH)}/grub.cfg-0
+@samp{(FWPATH)}/grub.cfg
+@end group
+@end example
+
 After GRUB has started, files on the TFTP server will be accessible via the
 @samp{(tftp)} device.
 
-- 
2.21.0

Re: [PATCH v4 2/5] net/dhcp: Consistently use decimal numbers for DHCP/BOOTP options enum

[Daniel Kiper]

On Wed, Oct 23, 2019 at 03:00:30PM +0200, Javier Martinez Canillas wrote:
> The DHCP Options and BOOTP Vendor Extensions enum values are a mixture of
> decimal and hexadecimal numbers. Change this to consistently use decimal
> numbers for all since that's how these values are defined by RFC 2132.
>
> Suggested-by: Daniel Kiper <daniel.kiper@oracle.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

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

Daniel

Re: [PATCH v4 5/5] docs: Update info with grub.cfg netboot selection order

[Daniel Kiper]

On Wed, Oct 23, 2019 at 03:00:33PM +0200, Javier Martinez Canillas wrote:
> From: Robert Marshall <rmarshall@redhat.com>
>
> Add documentation to the grub info page that specifies the order
> netboot clients use to select a grub configuration file.
>
> Signed-off-by: Robert Marshall <rmarshall@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

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

Daniel

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

[Daniel Kiper]

On Wed, Oct 23, 2019 at 03:00:31PM +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>
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

I was going to push this today but I have found a few issues with this
patchset... :-((( Details below...

>
> ---
>
> Changes in v4: None
> Changes in v3:
> - Use decimal numbers for the newly added DHCP client ID and UUID options.
>
> Changes in v2:
> - Use the existing grub_env_set_net_property() and remove duplicated code.
>
>  grub-core/net/bootp.c | 48 +++++++++++++++++++++++++++++++++++--------
>  include/grub/net.h    |  3 +++
>  2 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> index 04cfbb04504..377006da938 100644
> --- a/grub-core/net/bootp.c
> +++ b/grub-core/net/bootp.c
> @@ -95,6 +95,14 @@ enum
>  /* Max timeout when waiting for BOOTP/DHCP reply */
>  #define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
>
> +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 +160,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)
>  	{
> @@ -406,6 +417,35 @@ grub_net_configure_by_dhcp_ack (const char *name,
>    if (opt && opt_len)
>      grub_env_set_net_property (name, "extensionspath", (const char *) opt, opt_len);
>
> +  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_CLIENT_ID, &opt_len);
> +  if (opt && opt_len)
> +    grub_env_set_net_property (name, "clientid", (const char *) opt, opt_len);
> +
> +  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_CLIENT_UUID, &opt_len);
> +  if (opt && opt_len == 17)
> +    {

Move here all variable declarations from below. And "i" does not need to
be initialized to 0 in the declaration. It is done in for().

> +      /* The format is 9cfe245e-d0c8-bd45-a79f-54ea5fbd3d97 */
> +
> +      opt += 1;
> +      opt_len -= 1;
> +
> +      char *val = grub_malloc (2 * opt_len + 4 + 1);

Please check for NULL like below...

if (!val) {
  grub_errno = GRUB_ERR_OUT_OF_MEMORY;
  return inter;
}

> +      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] = '-';
> +            }
> +        }
> +      grub_env_set_net_property (name, "clientuuid", (char *) val, 2 * opt_len + 4);

Missing grub_free (val)...

> +    }
> +
>    inter->dhcp_ack = grub_malloc (size);
>    if (inter->dhcp_ack)
>      {
> @@ -631,14 +671,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 b5fedb28b9c..9668808dbca 100644
> --- a/include/grub/net.h
> +++ b/include/grub/net.h
> @@ -454,6 +454,7 @@ struct grub_net_bootp_packet
>
>  enum
>    {
> +

Spurious empty line...

Daniel

Re: [PATCH v4 4/5] normal/main: Search for specific config files for netboot

[Daniel Kiper]

On Wed, Oct 23, 2019 at 03:00:32PM +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>

Did you at least build test whole patchset? I do not think so.
Please do that before posting any patch on grub-devel. I do not
accept any patch which at least does not build. Details below...
Plus some nitpicks...

> ---
>
> Changes in v4: None
> Changes in v3:
> - Add Reviewed-by tag from Daniel Kiper.
>
> Changes in v2: None
>
>  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..f066b02ebad 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_config_file (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];

net/net.c:1784:28: error: ISO C90 forbids variable length array ‘client_uuid_var’ [-Werror=vla]
                            sizeof ("_clientuuid") + 1];
                            ^~~~~~

Well, message looks misleading. I think that it complains about
grub_strlen(). Anyway, could you use grub_malloc() or something
like that here?

> +      grub_snprintf (client_uuid_var, sizeof (client_uuid_var),
> +                     "net_%s_clientuuid", inf->name);
> +
> +      const char *client_uuid;

Please put all variables in one place... E.g. just before or behind client_uuid_var.

> +      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;

Wrong syntax here and below... Please fix as below...

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);

I would like to see empty line here.

> +            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;

Ditto...

> +            break;
> +          }
> +        case GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6:
> +          {
> +            char buf[GRUB_NET_MAX_STR_ADDR_LEN];
> +            struct grub_net_network_level_address base;

Ditto...

> +            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;

Ditto...

> +            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..cdeb58f1a1e 100644
> --- a/grub-core/normal/main.c
> +++ b/grub-core/normal/main.c

Missing "#include <grub/net.h>".

> @@ -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_config_file (config);
>
>  	  grub_enter_normal_mode (config);
>  	  grub_free (config);
> diff --git a/include/grub/net.h b/include/grub/net.h
> index 9668808dbca..d7a89bd39d1 100644
> --- a/include/grub/net.h
> +++ b/include/grub/net.h
> @@ -579,4 +579,7 @@ extern char *grub_net_default_server;
>
>  #define VLANTAG_IDENTIFIER 0x8100
>
> +grub_err_t
> +grub_net_search_configfile (char *config);

s/grub_net_search_configfile/grub_net_search_config_file/g

And I want to ask you to put this prototype behind
grub_net_remove_dns_server() after one empty line.

Daniel

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

[Javier Martinez Canillas]

On 10/28/19 3:54 PM, Daniel Kiper wrote:
> On Wed, Oct 23, 2019 at 03:00:31PM +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>
>> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> 
> I was going to push this today but I have found a few issues with this
> patchset... :-((( Details below...
>

Thanks a lot for pointing them out. I'll fix these issues when posting a v5.

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

Re: [PATCH v4 4/5] normal/main: Search for specific config files for netboot

[Javier Martinez Canillas]

On 10/28/19 4:13 PM, Daniel Kiper wrote:
> On Wed, Oct 23, 2019 at 03:00:32PM +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>
> 
> Did you at least build test whole patchset? I do not think so.
> Please do that before posting any patch on grub-devel. I do not
> accept any patch which at least does not build. Details below..

I do build and test the functionality for all the posted patches but just
noticed that I was calling configure with the --disable-werror option on
my build script... and that's why I missed these issues.

Sorry about that, I fixed my script now so it won't happen again.

> Plus some nitpicks...
>

I'll address these also in v5, thanks again for your feedback and review.
 
Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat