Re: [PATCH] net: Allow use of non-standard TCP/IP ports for HTTP protocol.

[Stephen Balousek]

Hi Daniel,

Thank you for another detailed response. Please let me know how that 
section related to IPv6 should be commented, and I will submit an 
updated patch.

- Steve

On 10/26/2021 8:52 AM, Daniel Kiper wrote:
> Hi Stephen,
>
> On Sun, Oct 10, 2021 at 03:15:34PM -0700, Stephen Balousek wrote:
> > Hi Daniel,
> >
> > Thank you for the review. This is my first submission to the project, and I
> > apologize for my lack of familiarity with the conventions and guidelines.
> No worries!
>
> > I reworked the patch with your suggested changes, and I also included an
> > attempt at some changes for the documentation.
> Thanks!
>
> > I am not completely sure how to submit a revised patch to the mailing list
> > and still maintain the email thread, so I am appending the new patch to this
> > message.
> Next time please create a new thread and mark the patch as v2, v3, etc. 
> respectively.
> You can use "git format-patch" and "git send-email" to do it.
>
> [...]
>
> >  From 3efc9a7f00a3e8f96609ee95262e87e61d42ce44 Mon Sep 17 00:00:00 2001
> > From: Stephen Balousek <sbalousek@wickedloop.com>
> > Date: Sun, 10 Oct 2021 15:12:20 -0700
> > Subject: [PATCH] http: Allow use of non-standard TCP/IP ports
> > To: grub-devel@gnu.org
> >
> > Allow the use of HTTP servers listening on ports other 80. This is done
> > with an extension to the http notation:
> >
> >    (http[,server[,port]])
> >
> >   - or -
> >
> >    (http[,server[:port]])
> >
> > Signed-off-by: Stephen Balousek <sbalousek@wickedloop.com>
> > ---
> >   docs/grub.texi       | 12 ++++++++++++
> >   grub-core/net/http.c | 37 +++++++++++++++++++++++++++++++++++--
> >   2 files changed, 47 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/grub.texi b/docs/grub.texi
> > index 99d0a0149..dbafe80d2 100644
> > --- a/docs/grub.texi
> > +++ b/docs/grub.texi
> > @@ -3011,6 +3011,18 @@ environment variable @samp{net_default_server} is
> > used.
> >   Before using the network drive, you must initialize the network.
> >   @xref{Network}, for more information.
> >
> > +For the @samp{http} network protocol, @code{@var{server}} may specify a
> > +port number other than the default value of @samp{80}. The server name
> > +and port number are separated by either @samp{,} or @samp{:}:
> > +
> > +@itemize @bullet
> > +@item
> > +@code{(http,@var{server},@var{port})}
> > +
> > +@item
> > +@code{(http,@var{server}:@var{port})}
> > +@end itemize
> > +
> >   If you boot GRUB from a CD-ROM, @samp{(cd)} is available. @xref{Making
> >   a GRUB bootable CD-ROM}, for details.
> >
> > diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> > index b616cf40b..eb07e333a 100644
> > --- a/grub-core/net/http.c
> > +++ b/grub-core/net/http.c
> > @@ -312,6 +312,9 @@ http_establish (struct grub_file *file, grub_off_t
> > offset, int initial)
> >     int i;
> >     struct grub_net_buff *nb;
> >     grub_err_t err;
> > +  char *server_name;
> > +  char *port_string;
> > +  unsigned long port_number;
> >
> >     nb = grub_netbuff_alloc (GRUB_NET_TCP_RESERVE_SIZE
> >                  + sizeof ("GET ") - 1
> > @@ -390,10 +393,40 @@ http_establish (struct grub_file *file, grub_off_t
> > offset, int initial)
> >     grub_netbuff_put (nb, 2);
> >     grub_memcpy (ptr, "\r\n", 2);
> >
> > -  data->sock = grub_net_tcp_open (file->device->net->server,
> > -                  HTTP_PORT, http_receive,
> > +  port_string = grub_strrchr (file->device->net->server, ',');
> > +  if (port_string == NULL)
> > +    {
> > +      port_string = grub_strrchr (file->device->net->server, ':');
> > +      if (port_string != NULL && grub_strchr (port_string + 1, ']') !=
> I am not sure why you are looking for ']' here. Could you enlighten me?

IPv6. The idea is to capture the port number '8080' from '[::1]:8080' 
but not '1' from '[::1]'. I can add a comment here, since I agree the 
intent is not clear.

> > NULL)
> > +      port_string = NULL;
> > +    }
> > +  if (port_string != NULL)
> > +    {
> > +      port_number = grub_strtoul (port_string + 1, 0, 10);
> > +      if (port_number == 0 && grub_errno == GRUB_ERR_BAD_NUMBER)
> I think it should be "port_number == GRUB_ULONG_MAX" instead of
> "port_number == 0" here.
Right. I don't know what I was thinking here.
> > +      return grub_error (GRUB_ERR_BAD_NUMBER, N_("non-numeric or invalid
> > port number `%s'"), port_string + 1);
> > +      if (port_number == 0 || port_number > 65535)
> > +      return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("port number `%s' not in
> > the range of 1 to 65535"), port_string + 1);
> > +
> > +      server_name = grub_strdup (file->device->net->server);
> > +      if (server_name == NULL)
> > +      return grub_errno;
> > +      server_name[port_string - file->device->net->server] = '\0';
> > +    }
> > +  else
> > +    {
> > +      port_number = HTTP_PORT;
> > +      server_name = file->device->net->server;
> > +    }
> Otherwise patch looks much better.
Thank you.
> Thanks,
>
> Daniel

--
- Steve