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

[Andrei Borzenkov]

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.