Re: [PATCH v2 6/9] net: dhcp: use DHCP options for name and bootfile

[Andre Przywara <andre.przywara@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 5548 bytes --]

On Thu, 21 Feb 2019 19:49:08 +0100
Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Tue, Feb 12, 2019 at 05:46:57PM +0000, Andre Przywara wrote:
> > From: Andrei Borzenkov <arvidjaar@gmail.com>
> >
> > The BOOTP RFC describes the boot file name and the server name as being
> > part of the integral BOOTP data structure, with some limits on the size
> > of them.
> > DHCP extends this by allowing them to be separate DHCP options, which is
> > more flexible.
> >
> > Teach the code dealing with those fields to check for those DHCP options
> > first and use this information, if provided. We fall back to using the
> > BOOTP information if those options are not used.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  grub-core/net/bootp.c | 72 +++++++++++++++++++++++++++++++------------
> >  include/grub/net.h    |  2 ++
> >  2 files changed, 54 insertions(+), 20 deletions(-)
> >
> > diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> > index 42117b72d..0c6756ea0 100644
> > --- a/grub-core/net/bootp.c
> > +++ b/grub-core/net/bootp.c
> > @@ -169,7 +169,9 @@ grub_net_configure_by_dhcp_ack (const char *name,
> >    int mask = -1;
> >    char server_ip[sizeof ("xxx.xxx.xxx.xxx")];
> >    const grub_uint8_t *opt;
> > -  grub_uint8_t opt_len;
> > +  grub_uint8_t opt_len, overload = 0;
> > +  const char *boot_file = 0, *server_name = 0;
> > +  grub_size_t boot_file_len, server_name_len;
> >
> >    addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
> >    addr.ipv4 = bp->your_ip;
> > @@ -188,9 +190,36 @@ grub_net_configure_by_dhcp_ack (const char *name,
> >    if (!inter)
> >      return 0;
> >
> > -  if (size > OFFSET_OF (boot_file, bp))
> > -    grub_env_set_net_property (name, "boot_file", bp->boot_file,
> > -                               sizeof (bp->boot_file));
> > +  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_OVERLOAD, &opt_len);
> > +  if (opt && opt_len == 1)
> > +    overload = *opt;
> > +
> > +  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_TFTP_SERVER_NAME, &opt_len);
> > +  if (opt && opt_len)
> > +    {
> > +      server_name = (const char *) opt;
> > +      server_name_len = opt_len;
> > +    }
> > +  else if (size > OFFSET_OF (server_name, bp) && !(overload & GRUB_DHCP_OPT_OVERLOAD_SNAME) &&
> > +          bp->server_name[0])
> > +    {
> > +      server_name = bp->server_name;
> > +      server_name_len = sizeof (bp->server_name);
> > +    }
> > +
> > +  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_BOOTFILE_NAME, &opt_len);
> > +  if (opt && opt_len)
> > +    {
> > +      boot_file = (const char *) opt;
> > +      boot_file_len = opt_len;
> > +    }
> > +  else if (size > OFFSET_OF (boot_file, bp) && !(overload && GRUB_DHCP_OPT_OVERLOAD_FILE) &&
> > +          bp->boot_file[0])
> > +    {
> > +      boot_file = bp->boot_file;
> > +      boot_file_len = sizeof (bp->boot_file);
> > +    }
> > +
> >    if (bp->server_ip)
> >      {
> >        grub_snprintf (server_ip, sizeof (server_ip), "%d.%d.%d.%d",
> > @@ -221,35 +250,38 @@ grub_net_configure_by_dhcp_ack (const char *name,
> >        *device = grub_xasprintf ("tftp,%s", server_ip);
> >        grub_print_error ();
> >      }
> > -  if (size > OFFSET_OF (server_name, bp)
> > -      && bp->server_name[0])
> > +
> > +  if (server_name)
> >      {
> > -      grub_env_set_net_property (name, "dhcp_server_name", bp->server_name,
> > -                                 sizeof (bp->server_name));
> > +      grub_env_set_net_property (name, "dhcp_server_name", server_name, server_name_len);
> >        if (is_def && !grub_net_default_server)
> >  	{
> > -	  grub_net_default_server = grub_strdup (bp->server_name);
> > +	  grub_net_default_server = grub_strdup (server_name);
> >  	  grub_print_error ();
> >  	}
> >        if (device && !*device)
> >  	{
> > -	  *device = grub_xasprintf ("tftp,%s", bp->server_name);
> > +	  *device = grub_xasprintf ("tftp,%s", server_name);
> >  	  grub_print_error ();
> >  	}
> >      }
> >
> > -  if (size > OFFSET_OF (boot_file, bp) && path)
> > +  if (boot_file)
> >      {
> > -      *path = grub_strndup (bp->boot_file, sizeof (bp->boot_file));
> > -      grub_print_error ();
> > -      if (*path)
> > +      grub_env_set_net_property (name, "boot_file", boot_file, boot_file_len);
> > +      if (path)
> >  	{
> > -	  char *slash;
> > -	  slash = grub_strrchr (*path, '/');
> > -	  if (slash)
> > -	    *slash = 0;
> > -	  else
> > -	    **path = 0;
> > +	  *path = grub_strndup (boot_file, boot_file_len);
> > +	  grub_print_error ();
> > +	  if (*path)
> > +	    {
> > +	      char *slash;
> > +	      slash = grub_strrchr (*path, '/');
> > +	      if (slash)
> > +		*slash = 0;
> > +	      else
> > +		**path = 0;
> > +	    }  
> 
> I am not sure why you are changing the code starting from
> "if (size > OFFSET_OF (boot_file, bp) && path)". Could you
> enlighten me?

The diff is indeed somewhat confusing. So several things are changed here:
- The "boot file" could be specified via *two* ways now, as a DHCP option or using the legacy BOOTP structure field. This is detected at separate places, that's why we change the first line to check for a valid boot_file pointer, instead of hardcoding the BOOTP structure field way here.
- grub_env_set_net_property() was called before when detecting the BOOTP structure field (see above), we do it now here, to cover both ways.
- There is an additional molly guard to check that "path" is not a NULL pointer.

I am attaching a slightly modified wide diff (diff -yb) to make this clearer.

Cheers,
Andre.

[-- Attachment #2: boot_file.diff --]
[-- Type: text/x-patch, Size: 757 bytes --]

  if (size > OFFSET_OF (boot_file, bp) && path)		      |	  if (boot_file)
    {								    {
							      >	      if (path)
							      >		{
							      >           grub_env_set_net_property (name, "boot_file", boot_file
      *path = grub_strndup (bp->boot_file, sizeof (bp->boot_f ~		  *path = grub_strndup (boot_file, boot_file_len);
      grub_print_error ();						  grub_print_error ();
      if (*path)							  if (*path)
	{								    {
	  char *slash;							      char *slash;
	  slash = grub_strrchr (*path, '/');				      slash = grub_strrchr (*path, '/');
	  if (slash)							      if (slash)
	    *slash = 0;								*slash = 0;
	  else								      else
	    **path = 0;								**path = 0;
							      >		    }
	}								}
    }								    }