Re: [PATCH] Allow nonstandard ports when specifying network protocols. (take 2)

[Andrei Borzenkov <arvidjaar@gmail.com>]

В Fri, 12 Dec 2014 20:00:10 -0600
Victor Lowther <victor.lowther@gmail.com> пишет:

> There are usecases for running TFTP and HTTP on nonstandard ports.  This
> patch allows you to specify nonstandard ports with the following syntax:
> 
>     set root=(http,$net_default_server,portno)
> or
>     set root=(http,,portno)

I do not think that is something that needs to be supported. The notion
of "default server" is mostly legacy one. Today it is exposed and if
needed can always be stated explicitly.

> 
> It also allows an initial : where a , should be for pxe: backwards compatibility
> 

This breaks with IPv6 and suddenly makes (http:server) valid syntax
which it is not.

> diff --git a/docs/grub.texi b/docs/grub.texi
> index 46b9e7f..9d0b538 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -2822,9 +2822,17 @@ of the partition when installing GRUB).
>  @end example
> 
>  If you enabled the network support, the special drives
> -@code{(@var{protocol}[,@var{server}])} are also available. Supported protocols
> +@code{(@var{protocol}[,@var{server}][,@var{port}])} are also
> available. Supported protocols
>  are @samp{http} and @samp{tftp}. If @var{server} is omitted, value of
>  environment variable @samp{net_default_server} is used.

According to syntax above it is impossible to omit server without
creating ambiguity. Just make it "protocol[,server[,port]]".

> +If @var{port} is omitted, it defaults to the IANA assigned port for
> +that protocol.
> +
> +@example
> +(tftp)
> +(http,,8091)

See above.

> +(http,boot.example.com)
> +
>  Before using the network drive, you must initialize the network.
>  @xref{Network}, for more information.
> 

> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index 21a4e94..d2d1d4a 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -1271,98 +1271,99 @@ static grub_net_t
>  grub_net_open_real (const char *name)
>  {
>    grub_net_app_level_t proto;
> -  const char *protname, *server;
> -  grub_size_t protnamelen;
> -  int try;
> -
> -  if (grub_strncmp (name, "pxe:", sizeof ("pxe:") - 1) == 0)
> -    {
> -      protname = "tftp";
> -      protnamelen = sizeof ("tftp") - 1;
> -      server = name + sizeof ("pxe:") - 1;
> -    }
> -  else if (grub_strcmp (name, "pxe") == 0)
> +  char *protname, *server;
> +  const char *work, *comma;
> +  grub_uint16_t port;
> +  grub_net_t ret;
> +  port = 0;
> +  work = name;
> +  server = NULL;
> +  protname = NULL;
> +  ret = NULL;
> +
> +  /* Pick off the protocol first. */
> +  comma = grub_strchr (work, ':');
> +  if (!comma)

No. ":" is not separator. (pxe) and (pxe:server) are legacy syntax - it
is not going to be extended. Leave this part as it is. You just need to
add additional check for a port in

   {
      const char *comma;
      comma = grub_strchr (name, ',');
      if (comma)
        {
          protnamelen = comma - name;
          server = comma + 1;
=== add check for port here ===
          protname = name;
        }

...
> -
> -  for (try = 0; try < 2; try++)

Please do not reshuffle code unnecessary. It makes it more difficult to
understand.

> +      grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("disk `%s' not found"),
> +                  name);
> +      goto out;
> +    }
> +  if (grub_strcmp(protname,"pxe") == 0)
>      {
> -      FOR_NET_APP_LEVEL (proto)
> -      {
> - if (grub_memcmp (proto->name, protname, protnamelen) == 0
> -    && proto->name[protnamelen] == 0)
> -  {
> -    grub_net_t ret = grub_zalloc (sizeof (*ret));
> -    if (!ret)
> -      return NULL;
> -    ret->protocol = proto;
> -    if (server)
> -      {
> - ret->server = grub_strdup (server);
> - if (!ret->server)
> -  {
> -    grub_free (ret);
> -    return NULL;
> -  }
> -      }
> -    else
> -      ret->server = NULL;
> -    ret->fs = &grub_net_fs;
> -    ret->offset = 0;
> -    ret->eof = 0;
> -    return ret;
> -  }
> -      }
> -      if (try == 0)
> - {
> -  if (sizeof ("http") - 1 == protnamelen
> -      && grub_memcmp ("http", protname, protnamelen) == 0)
> -    {
> -      grub_dl_load ("http");
> -      grub_errno = GRUB_ERR_NONE;
> -      continue;
> -    }
> -  if (sizeof ("tftp") - 1 == protnamelen
> -      && grub_memcmp ("tftp", protname, protnamelen) == 0)
> -    {
> -      grub_dl_load ("tftp");
> -      grub_errno = GRUB_ERR_NONE;
> -      continue;
> -    }
> - }
> -      break;
> +      grub_free(protname);
> +      protname = grub_strdup("tftp");
>      }
> -
> -  /* Restore original error.  */
> -  grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("disk `%s' not found"),
> -      name);
> -
> +  if (!grub_dl_load(protname))
> +    {
> +      grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("disk `%s' not found"),
> +                  name);
> +      goto out;
> +    }
> +  grub_errno = GRUB_ERR_NONE;
> +  ret = grub_zalloc (sizeof (*ret));
> +  if (!ret)
> +    goto out;
> +  ret->server = server;
> +  ret->fs = &grub_net_fs;
> +  ret->offset = 0;
> +  ret->eof = 0;
> +  FOR_NET_APP_LEVEL (proto)
> +  {
> +    if (grub_strcmp (proto->name, protname) != 0)
> +      continue;
> +    ret->protocol = proto;
> +    if (!port)

Port 0 is valid TCP port; you cannot assume it was not specified only
because it is zero.