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

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

[Victor Lowther]

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)

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

---
 ChangeLog            |  11 ++++
 docs/grub.texi       |  10 ++-
 grub-core/net/http.c |   3 +-
 grub-core/net/net.c  | 167 ++++++++++++++++++++++++++-------------------------
 grub-core/net/tftp.c |   3 +-
 include/grub/net.h   |   2 +
 6 files changed, 110 insertions(+), 86 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c38917b..4f08e29 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2014-12-11  Victor Lowther  <victor.lowther@gmail.com>
+
+ * grub-core/net/net.c: Allow grub_net_open_real to handle parsing
+ an optional port number for tftp and http network protocols.
+ * grub-core/net/http.c: Expose default port for HTTP in the
+ grub_http_protocol struct.
+ * grub-core/net/tftp.c: Expose default port for TFTP in the
+ grub_tftp_protocol struct.
+ * includegrub/net.h: Extend grub_net_app_protocol and grub_net_t
+ to include the port number.
+
 2014-12-09  Andrei Borzenkov  <arvidjaar@gmail.com>

  * grub-core/term/serial.c (grub_cmd_serial): Fix --rtscts
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.
+If @var{port} is omitted, it defaults to the IANA assigned port for
+that protocol.
+
+@example
+(tftp)
+(http,,8091)
+(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/http.c b/grub-core/net/http.c
index 4684f8b..2562d5e 100644
--- a/grub-core/net/http.c
+++ b/grub-core/net/http.c
@@ -392,7 +392,7 @@ http_establish (struct grub_file *file, grub_off_t
offset, int initial)
   grub_memcpy (ptr, "\r\n", 2);

   data->sock = grub_net_tcp_open (file->device->net->server,
-  HTTP_PORT, http_receive,
+  file->device->net->port, http_receive,
   http_err, http_err,
   file);
   if (!data->sock)
@@ -545,6 +545,7 @@ http_packets_pulled (struct grub_file *file)
 static struct grub_net_app_protocol grub_http_protocol =
   {
     .name = "http",
+    .port = HTTP_PORT,
     .open = http_open,
     .close = http_close,
     .seek = http_seek,
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)
+    comma = grub_strchr (work, ',');
+  if (!comma)
     {
-      protname = "tftp";
-      protnamelen = sizeof ("tftp") - 1;
-      server = grub_net_default_server;
+      protname = grub_strdup(work);
+      server = grub_strdup(grub_net_default_server);
     }
   else
     {
-      const char *comma;
-      comma = grub_strchr (name, ',');
-      if (comma)
- {
-  protnamelen = comma - name;
-  server = comma + 1;
-  protname = name;
- }
+      protname = grub_strndup(work,comma - work);
+      if (!comma[1])
+        {
+          grub_error(GRUB_ERR_NET_BAD_ADDRESS,N_("no server is specified"));
+          goto out;
+        }
+      else if (comma[1] == ',')
+        {
+          /* There is no server, just a port.  Let the server be the
default. */
+          server = grub_strdup(grub_net_default_server);
+          port = grub_strtoul(comma + 2,0,10);
+        }
       else
- {
-  protnamelen = grub_strlen (name);
-  server = grub_net_default_server;
-  protname = name;
- }
+        {
+          /* We have a server and maybe a port. */
+          work = comma + 1;
+          comma = grub_strchr(work,',');
+          if (!comma)
+            server = grub_strdup(work);
+          else
+            {
+              server = grub_strndup(work,comma - work);
+              port = grub_strtoul(comma + 1,0,10);
+            }
+        }
     }
-  if (!server)
+  /* By now, we have to have a server and a port.
+     If we do not, just die. */
+  if (!server || !protname)
     {
-      grub_error (GRUB_ERR_NET_BAD_ADDRESS,
-  N_("no server is specified"));
-      return NULL;
-    }
-
-  for (try = 0; try < 2; try++)
+      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)
+      ret->port = proto->port;
+    else
+      ret->port = port;
+    grub_free(protname);
+    return ret;
+  }
+ out:
+  if (ret)
+    grub_free(ret);
+  if (server)
+    grub_free(server);
+  if (protname)
+    grub_free(protname);
   return NULL;
 }

diff --git a/grub-core/net/tftp.c b/grub-core/net/tftp.c
index 1319671..d047a86 100644
--- a/grub-core/net/tftp.c
+++ b/grub-core/net/tftp.c
@@ -378,7 +378,7 @@ tftp_open (struct grub_file *file, const char *filename)
     }

   data->sock = grub_net_udp_open (addr,
-  TFTP_SERVER_PORT, tftp_receive,
+  file->device->net->port, tftp_receive,
   file);
   if (!data->sock)
     {
@@ -475,6 +475,7 @@ tftp_packets_pulled (struct grub_file *file)
 static struct grub_net_app_protocol grub_tftp_protocol =
   {
     .name = "tftp",
+    .port = TFTP_SERVER_PORT,
     .open = tftp_open,
     .close = tftp_close,
     .packets_pulled = tftp_packets_pulled
diff --git a/include/grub/net.h b/include/grub/net.h
index 538baa3..2249b10 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -245,6 +245,7 @@ struct grub_net_app_protocol
   struct grub_net_app_protocol *next;
   struct grub_net_app_protocol **prev;
   const char *name;
+  grub_uint16_t port;
   grub_err_t (*dir) (grub_device_t device, const char *path,
      int (*hook) (const char *filename,
   const struct grub_dirhook_info *info));
@@ -264,6 +265,7 @@ typedef struct grub_net
   grub_fs_t fs;
   int eof;
   int stall;
+  grub_uint16_t port;
 } *grub_net_t;

 extern grub_net_t (*EXPORT_VAR (grub_net_open)) (const char *name);
-- 
2.1.3

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

[Andrei Borzenkov]

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

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

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]


> @@ -545,6 +545,7 @@ http_packets_pulled (struct grub_file *file)
>  static struct grub_net_app_protocol grub_http_protocol =
>    {
>      .name = "http",
> +    .port = HTTP_PORT,
Wrong. This way you'll modify the default port for subsequent calls.
Port should be passed as an additional argument to open. Use int and
value -1 to indicate that no port is specified and change code to
replace it by right port. Don't use 0 as 0 is a valid port.
> +  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)
> +    comma = grub_strchr (work, ',');
This makes colon equivalent to comma. We don't want another separator.
"pxe:" is for compatibility. Leave it at that, don't add new functions
to it.
> +  if (!comma)
>      {
> -      protname = "tftp";
> -      protnamelen = sizeof ("tftp") - 1;
> -      server = grub_net_default_server;
> +      protname = grub_strdup(work);
> +      server = grub_strdup(grub_net_default_server);
>      }
>    else
>      {
> -      const char *comma;
> -      comma = grub_strchr (name, ',');
> -      if (comma)
> - {
> -  protnamelen = comma - name;
> -  server = comma + 1;
> -  protname = name;
> - }
Please follow whitespace conventions (although it's minor concern).
> +  if (!grub_dl_load(protname))
> +    {
> +      grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("disk `%s' not found"),
> +                  name);
> +      goto out;
> +    }
Please don't jumble unrelated changes. This has nothing to do with
parser changes.
The change you intend to make should be more like 20 lines of modified
code, probably even less, not a jumbled case of many unrelated changes.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]