Re: [PATCH 2/2] Add net_set_vlan command

[Daniel Kiper]

On Fri, Mar 04, 2022 at 10:46:36PM -0500, Chad Kimes via Grub-devel wrote:
> Previously there was no way to set the 802.1Q VLAN identifier, despite
> support for vlantag in the net module. The only location vlantag was
> being populated was from PXE boot and only for Open Firmware hardware.
> This commit allows users to manually configure VLAN information for any
> interface.
>
> Example usage:
>   grub> net_ls_addr
>   efinet1 00:11:22:33:44:55 192.168.0.100
>   grub> net_set_vlan efinet1 100
>   grub> net_ls_addr
>   efinet1 00:11:22:33:44:55 192.168.0.100 vlan100
>   grub> net_set_vlan efinet1 0
>   efinet1 00:11:22:33:44:55 192.168.0.100

The same comment as for the patch #1.

> Signed-off-by: Chad Kimes <chkimes@github.com>
> ---
>  docs/grub.texi      |  9 +++++++++
>  grub-core/net/net.c | 34 +++++++++++++++++++++++++++++++++-
>  2 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index caba8befb..5758ec285 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -5553,6 +5553,7 @@ This command is only available on AArch64 systems.
>  * net_ls_cards::                List network cards
>  * net_ls_dns::                  List DNS servers
>  * net_ls_routes::               List routing entries
> +* net_set_vlan::                Set vlan id on an interface
>  * net_nslookup::                Perform a DNS lookup
>  @end menu
>
> @@ -5721,6 +5722,14 @@ List routing entries.
>  @end deffn
>
>
> +@node net_set_vlan
> +@subsection net_set_vlan
> +
> +@deffn Command net_set_vlan @var{interface} @var{vlanid}
> +Set the 802.1Q VLAN identifier on @var{interface} to @var{vlanid}.
> +@end deffn

May I ask you to add an example here?

>  @node net_nslookup
>  @subsection net_nslookup
>
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index 33e35d5b5..f2acd2ecf 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -1176,6 +1176,35 @@ grub_cmd_addroute (struct grub_command *cmd 
> __attribute__ ((unused)),
>      }
>  }
>
> +static grub_err_t
> +grub_cmd_setvlan (struct grub_command *cmd __attribute__ ((unused)),
> +               int argc, char **args)
> +{
> +  if (argc < 2)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments expected"));
> +
> +  const char *vlanptr = args[1];

Please declare all variables at the beginning of the function.

> +  unsigned long vlantag;
> +  vlantag = grub_strtoul (vlanptr, &vlanptr, 10);
> +
> +  if (vlantag > 4094)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid vlan id"));

This check is bogus. Please take a look at commit ac8a37dda (net/http:
Allow use of non-standard TCP/IP ports) how it should be done properly.

> +  struct grub_net_network_level_interface *inter;

Please move this to the beginning of the function.

> +  FOR_NET_NETWORK_LEVEL_INTERFACES (inter)
> +    {
> +      if (grub_strcmp (inter->name, args[0]) != 0)
> +     continue;
> +
> +      inter->vlantag = vlantag;
> +      return GRUB_ERR_NONE;
> +    }
> +
> +  return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                     N_("network interface not found"));
> +}
> +
>  static void
>  print_net_address (const grub_net_network_level_netaddress_t *target)
>  {
> @@ -1892,7 +1921,7 @@ static struct grub_preboot *fini_hnd;
>
>  static grub_command_t cmd_addaddr, cmd_deladdr, cmd_addroute, cmd_delroute;
>  static grub_command_t cmd_lsroutes, cmd_lscards;
> -static grub_command_t cmd_lsaddr, cmd_slaac;
> +static grub_command_t cmd_lsaddr, cmd_slaac, cmd_setvlan;
>
>  GRUB_MOD_INIT(net)
>  {
> @@ -1935,6 +1964,9 @@ GRUB_MOD_INIT(net)
>                                      "", N_("list network cards"));
>    cmd_lsaddr = grub_register_command ("net_ls_addr", grub_cmd_listaddrs,
>                                      "", N_("list network addresses"));
> +  cmd_setvlan = grub_register_command ("net_set_vlan", grub_cmd_setvlan,
> +                                    N_("SHORTNAME VLANID"),

Why do we need that if other commands have empty string here?

> +                                    N_("Set an interace's vlan id."));

Please be consistent with earlier grub_register_command() calls and
start sentence with lower case and drop full stop at the end of it.

Daniel