[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] efinet: get bootstrap info from proxy offer packet
From: |
chen zhihui |
Subject: |
Re: [PATCH] efinet: get bootstrap info from proxy offer packet |
Date: |
Fri, 22 Apr 2016 02:30:38 +0000 |
On 04/21/2016 09:02 PM, Andrei Borzenkov wrote:
> On Thu, Apr 21, 2016 at 11:36 AM, chen zhihui <address@hidden> wrote:
>> From: chenzhihui <address@hidden>
>>
>> Bootstrap server ip address and boot file name maybe come from
>> a separate proxy DHCP server, check the proxy_offer packet if
>> failed with dhcp_ack packet.
>>
> Yes, this came up before. Thank you for a patch. This is likely
> post-2.02 material though.
Thanks for your comments!
>
>> Signed-off-by: chenzhihui <address@hidden>
>> Tested-by: Jerome Forissier <address@hidden>
>> ---
>> grub-core/net/bootp.c | 170
>> ++++++++++++++++++++++++++++++++++++-
>> grub-core/net/drivers/efi/efinet.c | 23 ++++-
>> include/grub/net.h | 10 +++
>> 3 files changed, 200 insertions(+), 3 deletions(-)
>>
>> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
>> index 4fdeac3..52f4051 100644
>> --- a/grub-core/net/bootp.c
>> +++ b/grub-core/net/bootp.c
>> @@ -186,7 +186,7 @@ grub_net_configure_by_dhcp_ack (const char *name,
>> }
>> #endif
>>
>> - if (size > OFFSET_OF (boot_file, bp))
>> + if (size > OFFSET_OF (boot_file, bp) && bp->boot_file[0])
>> grub_env_set_net_property (name, "boot_file", bp->boot_file,
>> sizeof (bp->boot_file));
>> if (is_def)
>> @@ -233,7 +233,7 @@ grub_net_configure_by_dhcp_ack (const char *name,
>> }
>> }
>>
>> - if (size > OFFSET_OF (boot_file, bp) && path)
>> + if (size > OFFSET_OF (boot_file, bp) && bp->boot_file[0] && path)
>> {
>> *path = grub_strndup (bp->boot_file, sizeof (bp->boot_file));
>> grub_print_error ();
>> @@ -263,6 +263,172 @@ grub_net_configure_by_dhcp_ack (const char *name,
>> return inter;
>> }
>>
>> +struct dhcp4_packet_option {
>> + grub_uint8_t code;
>> + grub_uint8_t length;
>> + grub_uint8_t data[0];
>> +};
>> +
>> +/*
>> + * Get specified option from DHCP extension data
>> + *
>> + * from PxeBcDhcp.c of UEFI
>> + *
>> + */
>> +static struct dhcp4_packet_option *
>> +dhcp_proxy_extension_option (const grub_uint8_t *buf,
>> + grub_size_t size,
>> + grub_uint8_t code)
>> +{
>> + struct dhcp4_packet_option *option = (struct dhcp4_packet_option
>> *)buf;
>> + grub_size_t offset = 0;
>> +
>> + while (offset < size && option->code != GRUB_NET_BOOTP_END) {
> OK, could you please rebase it on top of
> http://lists.gnu.org/archive/html/grub-devel/2016-03/msg00280.html. Of
> course comments to this patch are welcome. Without comments it will
> land here at some point and it implements DHCP options processing
> already. If you find it easier, I can create branch with this patch.
No thanks, maybe I'll send v2 patch when your patch is accepted by upstream.
> ...
>> +void
>> +grub_net_configure_by_proxy_offer (const struct grub_net_bootp_packet *bp,
>> + grub_size_t size,
>> + char **device,
>> + char **path)
>> +{
>> + const grub_uint8_t *buf = bp->vendor + sizeof (grub_uint32_t);
>> + grub_uint32_t option_size =
>> + size - OFFSET_OF(vendor, bp) - sizeof (grub_uint32_t);
>> + struct dhcp4_packet_option *option;
>> +
>> + if (device == NULL)
>> + return;
>> +
>> + if (!proxy_offer_is_valid(bp, size))
>> + return;
>> +
>> + if (!*device && bp->server_ip)
>> + {
>> + *device = grub_xasprintf ("tftp,%d.%d.%d.%d",
>> + ((grub_uint8_t *) &bp->server_ip)[0],
>> + ((grub_uint8_t *) &bp->server_ip)[1],
>> + ((grub_uint8_t *) &bp->server_ip)[2],
>> + ((grub_uint8_t *) &bp->server_ip)[3]);
>> + grub_print_error ();
>> + }
>> +
>> + option = dhcp_proxy_extension_option(buf,
>> + option_size, GRUB_NET_DHCP_OVERLOAD);
>> +
>> + if ((option == NULL || option->data[0] == 1) && !*device &&
>> bp->server_name[0])
>> + {
>> + *device = grub_xasprintf ("tftp,%s", bp->server_name);
>> + grub_print_error ();
>> + }
>> +
>> + if (!*device)
>> + {
>> + option = dhcp_proxy_extension_option(buf,
>> + option_size, GRUB_NET_DHCP_SERVER_ID);
>> +
>> + if (option) {
>> + *device = grub_xasprintf("tftp,%d.%d.%d.%d",
>> + option->data[0],
>> + option->data[1],
>> + option->data[2],
>> + option->data[3]);
> DHCP_SERVER_ID (option 54) is defined for DHCP client/server
> communication only. There is no implied usage of this option as next
> server (i.e. boot server) so I do not think this is correct.
Yes, I'll remove this in v2.
>
> ...
>> diff --git a/grub-core/net/drivers/efi/efinet.c
>> b/grub-core/net/drivers/efi/efinet.c
>> index 5388f95..ef0ccd9 100644
>> --- a/grub-core/net/drivers/efi/efinet.c
>> +++ b/grub-core/net/drivers/efi/efinet.c
>> @@ -338,6 +338,7 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char
>> **device,
>> FOR_NET_CARDS (card)
>> {
>> grub_efi_device_path_t *cdp;
>> + struct grub_net_network_level_interface *inter;
>> struct grub_efi_pxe *pxe;
>> struct grub_efi_pxe_mode *pxe_mode;
>> if (card->driver != &efidriver)
>> @@ -378,11 +379,31 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char
>> **device,
>> if (! pxe)
>> continue;
>> pxe_mode = pxe->mode;
>> - grub_net_configure_by_dhcp_ack (card->name, card, 0,
>> + inter = grub_net_configure_by_dhcp_ack (card->name, card, 0,
>> (struct grub_net_bootp_packet *)
>> &pxe_mode->dhcp_ack,
>> sizeof (pxe_mode->dhcp_ack),
>> 1, device, path);
>> +
>> + /*
>> + * Bootstrap server ip address and file name maybe
>> + * come from a separate proxy DHCP server,
>> + * so check the proxy_offer DHCP packet
>> + *
>> + */
>> + if (inter && *path == NULL) {
>> + if (*device) {
>> + grub_free(*device);
>> + *device = NULL;
>> + }
>> +
>> + grub_net_configure_by_proxy_offer(
>> + (struct grub_net_bootp_packet
>> *)&pxe_mode->proxy_offer,
> Please check ProxyOfferReceived as required by UEFI. I know we do not
> do it currently but let's do it right at least in new code.
You're right, I'll change in v2
>
>> + sizeof (pxe_mode->proxy_offer),
>> + device,
>> + path);
>> + }
>> +
> Well, this opens up the question about precedence of data from
> different packets. With my patch (that abstracts away options
> processing) we can actually simply pass both packets at once and fetch
> IP information from (original) DHCPACK and other information from
> proxy DHCPOFFER as required (falling back to DHCPACK if proxy is
> NULL).
Great! If you can do that in your v3 patch, I'll has nothing to do.
>> return;
>> }
>> }
>> diff --git a/include/grub/net.h b/include/grub/net.h
>> index b62643a..f107a23 100644
>> --- a/include/grub/net.h
>> +++ b/include/grub/net.h
>> @@ -433,6 +433,10 @@ enum
>> GRUB_NET_BOOTP_DOMAIN = 0x0f,
>> GRUB_NET_BOOTP_ROOT_PATH = 0x11,
>> GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
>> + GRUB_NET_DHCP_OVERLOAD = 0x34,
>> + GRUB_NET_DHCP_SERVER_ID = 0x36,
>> + GRUB_NET_DHCP_CLASS_ID = 0x3c,
>> + GRUB_NET_DHCP_BOOTFILE = 0x43,
> Please use decimal option numbers for any new option you add. They are
> defined as decimal in RFC and it makes it easier to cross-reference
> with RFC later.
OK.
--
==========================================
Best Regards
陈志辉(Chenzhihui)
华为海思图灵软件架构组
Huawei Hisilicon Turing Software Architecture Team
http://www.hisilicon.com/