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

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