Re: [PATCH 2/2] Add net_set_vlan command

[Chad Kimes]

Thanks for the review! I'll re-submit this and the other patch series shortly.

On Thu, Mar 17, 2022 at 7:03 PM Daniel Kiper <dkiper@net-space.pl> wrote:
>
> 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?

The format is following the other net commands above this that modify data
instead of just list data. For example:

  cmd_addroute = grub_register_command ("net_add_route", grub_cmd_addroute,
                                        /* TRANSLATORS: "gw" is a keyword.  */
                                        N_("SHORTNAME NET [INTERFACE|
gw GATEWAY]"),
                                        N_("Add a network route."));
  cmd_delroute = grub_register_command ("net_del_route", grub_cmd_delroute,
                                        N_("SHORTNAME"),
                                        N_("Delete a network route."));

I'll move the new command after these lines and before the ls commands so that
it's more clear.

>
> > +                                    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.

Same as above.

>
> Daniel