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 <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Javier Martinez Canillas <address@hidden>
> Reviewed-by: Daniel Kiper <address@hidden>

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