grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Andre Przywara
Subject: Re: [PATCH v2 6/9] net: dhcp: use DHCP options for name and bootfile
Date: Thu, 7 Mar 2019 10:36:12 +0000

On Thu, 21 Feb 2019 19:49:08 +0100
Daniel Kiper <address@hidden> wrote:

> On Tue, Feb 12, 2019 at 05:46:57PM +0000, Andre Przywara wrote:
> > From: Andrei Borzenkov <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> >  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: boot_file.diff
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]