Re: [2.02][PATCH] bootp: export server IP as environment variable

[Daniel Kiper]

On Tue, Nov 22, 2016 at 08:21:12PM +0300, Andrei Borzenkov wrote:
> 22.11.2016 01:02, Daniel Kiper ??????????:
> > On Mon, Nov 21, 2016 at 10:52:24PM +0300, Andrei Borzenkov wrote:
> >> 21.11.2016 16:09, Daniel Kiper ??????????:
> >>> On Fri, Nov 18, 2016 at 09:11:47PM +0300, Andrei Borzenkov wrote:
> >>>> 17.11.2016 22:02, Daniel Kiper ??????????:
> >>>>> On Fri, Mar 18, 2016 at 08:42:00PM +0300, Andrei Borzenkov wrote:
> >>>>>> Network boot autoconfiguration sets default server to next server IP
> >>>>>> (siaddr) from BOOTP/DHCP reply, but manual configuration using 
> >>>>>> net_bootp
> >>>>>> exports only server name. Unfortunately semantic of server name is not
> >>>>>> clearly defined. BOOTP RFC 951 defines it only for client request, and
> >>>>>> DHCP RFC 1541 only mentions it, without any implied usage. It looks 
> >>>>>> like
> >>>>>> this field is mostly empty in server replies.
> >>>>>>
> >>>>>> Export next server IP as net_<interface>_server_ip variable. This 
> >>>>>> allows
> >>>>>> grub configuration script to set $root/$prefix based on information 
> >>>>>> obtained
> >>>>>> by net_bootp.
> >>>>>>
> >>>>>> Reported and tested by: Nikunj A Dadhania <address@hidden>
> >>>>>> Cc: address@hidden
> >>>>>>
> >>>>>> ---
> >>>>>>
> >>>>>> @Nikunj: cosmetic changes comparing with version you tested - no need 
> >>>>>> to
> >>>>>> allocate small buffer dynamically.
> >>>>>>
> >>>>>> @Vladimir: I think this is useful; I will update networking 
> >>>>>> documentation
> >>>>>> in followup patch.
> >>>>>
> >>>>> Vladimir, Andrei, could you take care of it?
> >>>>>
> >>>>> Daniel
> >>>>>
> >>>>
> >>>> I added documentation (not worth separate patch); if there are no
> >>>> objections I commit.
> >>>>
> >>>>
> >>>> From: Andrei Borzenkov <address@hidden>
> >>>> Subject: [2.02][PATCH] bootp: export server IP as environment variable
> >>>>
> >>>> Network boot autoconfiguration sets default server to next server IP
> >>>> (siaddr) from BOOTP/DHCP reply, but manual configuration using net_bootp
> >>>> exports only server name. Unfortunately semantic of server name is not
> >>>> clearly defined. BOOTP RFC 951 defines it only for client request, and
> >>>> DHCP RFC 1541 only mentions it, without any implied usage. It looks like
> >>>> this field is mostly empty in server replies.
> >>>>
> >>>> Export next server IP as net_<interface>_server_ip variable. This allows
> >>>> grub configuration script to set $root/$prefix based on information 
> >>>> obtained
> >>>> by net_bootp.
> >>>>
> >>>> Reported and tested by: Nikunj A Dadhania <address@hidden>
> >>>> Cc: address@hidden
> >>>>
> >>>> ---
> >>>>  docs/grub.texi        | 11 +++++++++++
> >>>>  grub-core/net/bootp.c | 24 ++++++++++++++----------
> >>>>  2 files changed, 25 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/docs/grub.texi b/docs/grub.texi
> >>>> index 82f6fa4..bb03875 100644
> >>>> --- a/docs/grub.texi
> >>>> +++ b/docs/grub.texi
> >>>> @@ -2446,6 +2446,10 @@ The boot file name provided by DHCP.  Read-only.
> >>>>  The name of the DHCP server responsible for these boot parameters.
> >>>>  Read-only.
> >>>>
> >>>> address@hidden address@hidden<interface>}_server_ip
> >>>> +The IP address of the next (usually, TFTP) server provided by DHCP.
> >>>
> >>> BOOTP instead of DHCP? However, then we should change DHCP everywhere 
> >>> too...
> >>>
> >>
> >> Yes. Sorry, it was probably my fault to refer to DHCP in the first place.
> >>
> >>>> +Read-only.
> >>>> +
> >>>>  @item net_default_interface
> >>>>  Initially set to name of network interface that was used to load grub.
> >>>>  Read-write, although setting it affects only interpretation of
> >>>> @@ -3062,6 +3066,7 @@ These variables have special meaning to GRUB.
> >>>>  * address@hidden<interface>}_hostname::
> >>>>  * address@hidden<interface>}_ip::
> >>>>  * address@hidden<interface>}_mac::
> >>>> +* address@hidden<interface>}_server_ip::
> >>>
> >>> There is something known as net_default_server which contains next IP 
> >>> address.
> >>> Should not we be more consistent and use 
> >>> "address@hidden<interface>}_server" instead
> >>> of "address@hidden<interface>}_server_ip" here and there.
> >>>
> >>
> >> Well, net_default_server is not really tied to any BOOTP/DHCP at all,
> >> although it may be initially set based on content of ACK packet. I was
> >> confused by dhcp_server_name, but they it is unrelated to next server.
> >>
> >> I'd rather used net_<interface>_next_server for clarity, both "server"
> >> and "server_ip" are indeed too vague in this case.
> >
> > net_<interface>_next_server is OK for me. Should not we introduce
> > net_default_next_server as RO then too? At first its value will be
> > equal net_default_server but only net_default_server could be
> > changed by user. Does it make sense?
>
> I would wait until someone has use case for it. I do not see it now.

OK, so, let's leave net_<interface>_next_server only right now.
Thanks for committing this stuff.

Daniel