Re: [PATCH v2 2/2] Configure VLAN from UEFI device used for PXE

[Daniel Kiper]

On Mon, Mar 21, 2022 at 06:07:32PM -0400, Chad Kimes via Grub-devel wrote:

I would prefer if you copy some text from the cover letter here.

> Signed-off-by: Chad Kimes <chkimes@github.com>
> ---
>  grub-core/net/drivers/efi/efinet.c | 38 ++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/net/drivers/efi/efinet.c 
> b/grub-core/net/drivers/efi/efinet.c
> index 381c138db..107e1f09e 100644
> --- a/grub-core/net/drivers/efi/efinet.c
> +++ b/grub-core/net/drivers/efi/efinet.c
> @@ -339,6 +339,10 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char 
> **device,
>  {
>    struct grub_net_card *card;
>    grub_efi_device_path_t *dp;
> +  struct grub_net_network_level_interface *inter;
> +  grub_efi_device_path_t *vlan_dp;
> +  grub_efi_uint16_t vlan_dp_len;
> +  grub_efi_vlan_device_path_t *vlan;
>
>    dp = grub_efi_get_device_path (hnd);
>    if (! dp)
> @@ -387,11 +391,35 @@ 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,
> -                                 (struct grub_net_bootp_packet *)
> -                                 &pxe_mode->dhcp_ack,
> -                                 sizeof (pxe_mode->dhcp_ack),
> -                                 1, device, path);
> +
> +    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);
> +
> +    if (inter)

I prefer "if (inter != NULL)".

> +      {
> +     /*
> +      * search the device path for any VLAN subtype and use it
> +      * to configure the interface
> +      */
> +     vlan_dp = dp;
> +
> +     while (!GRUB_EFI_END_ENTIRE_DEVICE_PATH (vlan_dp))
> +     {
> +       if (GRUB_EFI_DEVICE_PATH_TYPE (vlan_dp) == 
> GRUB_EFI_MESSAGING_DEVICE_PATH_TYPE
> +           && GRUB_EFI_DEVICE_PATH_SUBTYPE (vlan_dp) == 
> GRUB_EFI_VLAN_DEVICE_PATH_SUBTYPE)
> +         {
> +           vlan = (grub_efi_vlan_device_path_t *) vlan_dp;
> +           inter->vlantag = vlan->vlan_id;
> +           break;
> +         }
> +
> +       vlan_dp_len = GRUB_EFI_DEVICE_PATH_LENGTH (vlan_dp);
> +       vlan_dp = (grub_efi_device_path_t *) ((char *) vlan_dp + vlan_dp_len);

s/char */grub_uint8_t */ even if that translates to almost the same type.
Simply I think grub_uint8_t is more natural here.

Anyway, I will fix all these minor issues this time for you.

So, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Thank you for adding this feature!

Daniel