thr3ads.net - Libguestfs - [Libguestfs] [PATCH nbdkit 04/10] server: Calculate $uri in one place [Sep 2023]

If this information is useful, please help other people find it:
Share via:

Eric Blake

2023-Sep-11 14:01 UTC

[Libguestfs] [PATCH nbdkit 04/10] server: Calculate $uri in one place

On Sat, Sep 09, 2023 at 02:57:52PM +0100, Richard W.M. Jones
wrote:> Move the calculation of $uri to the main function (well, inlined
> there), and out of --run code.
> 
> This is largely code motion.  In theory it changes the content of $uri
> since we now shell quote it after generating it, but this ought not to
> have any practical effect.
> ---
>  server/internal.h |  1 +
>  server/captive.c  | 41 ++----------------------
>  server/main.c     | 79 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 82 insertions(+), 39 deletions(-)
> 
> +++ b/server/captive.c
> @@ -75,45 +75,8 @@ run_command (void)
>  
>    /* Construct $uri. */
>    fprintf (fp, "uri=");
> -  switch (service_mode) {
> -  case SERVICE_MODE_SOCKET_ACTIVATION:
> -  case SERVICE_MODE_LISTEN_STDIN:
> -    break;                      /* can't form a URI, leave it blank */
> -  case SERVICE_MODE_UNIXSOCKET:
> -    fprintf (fp, "nbd%s+unix://", tls == 2 ? "s" :
"");
> -    if (export_name && strcmp (export_name, "") != 0) {
> -      putc ('/', fp);
> -      uri_quote (export_name, fp);
> -    }
> -    fprintf (fp, "\\?socket=");
> -    uri_quote (unixsocket, fp);
Beforehand, we were manually shell-quoting the ? in the Unix URI...
> -    break;
> -  case SERVICE_MODE_VSOCK:
> -    /* 1 = VMADDR_CID_LOCAL */
> -    fprintf (fp, "nbd%s+vsock://1", tls == 2 ? "s" :
"");
> -    if (port) {
> -      putc (':', fp);
> -      shell_quote (port, fp);
> -    }
> -    if (export_name && strcmp (export_name, "") != 0) {
> -      putc ('/', fp);
> -      uri_quote (export_name, fp);
> -    }
> -    break;
> -  case SERVICE_MODE_TCPIP:
> -    fprintf (fp, "nbd%s://localhost", tls == 2 ? "s" :
"");
> -    if (port) {
> -      putc (':', fp);
> -      shell_quote (port, fp);
> -    }
> -    if (export_name && strcmp (export_name, "") != 0) {
> -      putc ('/', fp);
> -      uri_quote (export_name, fp);
> -    }
> -    break;
> -  default:
> -    abort ();
> -  }
> +  if (uri)
> +    shell_quote (uri, fp);
...while here, we shell-quote the entire string...
>    putc ('\n', fp);
>  
>    /* Since nbdkit 1.24, $nbd is a synonym for $uri. */
> diff --git a/server/main.c b/server/main.c
> index 2a332bfdd..54eb348ba 100644
> --- a/server/main.c
> +static char *
> +make_uri (void)
> +
> +  switch (service_mode) {
> +  case SERVICE_MODE_UNIXSOCKET:
> +    fprintf (fp, "nbd%s+unix://", tls == 2 ? "s" :
"");
> +    if (export_name && strcmp (export_name, "") != 0) {
> +      putc ('/', fp);
> +      uri_quote (export_name, fp);
> +    }
> +    fprintf (fp, "?socket=");
> +    uri_quote (unixsocket, fp);
...where the manual shell-quoting is no longer injected.  Yes, this
looks correct (the appearance of the quoting, using '' instead of \,
may be different, but the resulting string as parsed by the shell is
the same).
> +    break;
> +  case SERVICE_MODE_VSOCK:
> +    /* 1 = VMADDR_CID_LOCAL */
> +    fprintf (fp, "nbd%s+vsock://1", tls == 2 ? "s" :
"");
> +    if (port) {
> +      putc (':', fp);
> +      fputs (port, fp);
> +    }
> +    if (export_name && strcmp (export_name, "") != 0) {
> +      putc ('/', fp);
> +      uri_quote (export_name, fp);
> +    }
> +    break;
> +  case SERVICE_MODE_TCPIP:
> +    fprintf (fp, "nbd%s://localhost", tls == 2 ? "s" :
"");
> +    if (port) {
> +      putc (':', fp);
> +      fputs (port, fp);
> +    }
> +    if (export_name && strcmp (export_name, "") != 0) {
> +      putc ('/', fp);
> +      uri_quote (export_name, fp);
> +    }
> +    break;
> +
> +  case SERVICE_MODE_SOCKET_ACTIVATION:
> +  case SERVICE_MODE_LISTEN_STDIN:
> +    abort ();                   /* see above */
> +  default:
> +    abort ();
Could consolidate these labels, although I don't know if a compiler
would be picky about:

  case ...:
    /* comment */
  default:
    abort ();

so I'm also fine leaving it as-is.

Reviewed-by: Eric Blake <eblake at redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Richard W.M. Jones

2023-Sep-11 14:09 UTC

head link

[Libguestfs] [PATCH nbdkit 04/10] server: Calculate $uri in one place

On Mon, Sep 11, 2023 at 09:01:47AM -0500, Eric Blake
wrote:> On Sat, Sep 09, 2023 at 02:57:52PM +0100, Richard W.M. Jones wrote:
> > Move the calculation of $uri to the main function (well, inlined
> > there), and out of --run code.
> > 
> > This is largely code motion.  In theory it changes the content of $uri
> > since we now shell quote it after generating it, but this ought not to
> > have any practical effect.
> > ---
> >  server/internal.h |  1 +
> >  server/captive.c  | 41 ++----------------------
> >  server/main.c     | 79
+++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 82 insertions(+), 39 deletions(-)
> > 
> 
> > +++ b/server/captive.c
> > @@ -75,45 +75,8 @@ run_command (void)
> >  
> >    /* Construct $uri. */
> >    fprintf (fp, "uri=");
> > -  switch (service_mode) {
> > -  case SERVICE_MODE_SOCKET_ACTIVATION:
> > -  case SERVICE_MODE_LISTEN_STDIN:
> > -    break;                      /* can't form a URI, leave it
blank */
> > -  case SERVICE_MODE_UNIXSOCKET:
> > -    fprintf (fp, "nbd%s+unix://", tls == 2 ? "s"
: "");
> > -    if (export_name && strcmp (export_name, "") !=
0) {
> > -      putc ('/', fp);
> > -      uri_quote (export_name, fp);
> > -    }
> > -    fprintf (fp, "\\?socket=");
> > -    uri_quote (unixsocket, fp);
> 
> Beforehand, we were manually shell-quoting the ? in the Unix URI...
The shell quoting here was only marginally useful before this change.
In theory there might be a file in a subdirectory called
'nbd+unix:/Xsocket=' which would match :-)

> > -    break;
> > -  case SERVICE_MODE_VSOCK:
> > -    /* 1 = VMADDR_CID_LOCAL */
> > -    fprintf (fp, "nbd%s+vsock://1", tls == 2 ?
"s" : "");
> > -    if (port) {
> > -      putc (':', fp);
> > -      shell_quote (port, fp);
> > -    }
> > -    if (export_name && strcmp (export_name, "") !=
0) {
> > -      putc ('/', fp);
> > -      uri_quote (export_name, fp);
> > -    }
> > -    break;
> > -  case SERVICE_MODE_TCPIP:
> > -    fprintf (fp, "nbd%s://localhost", tls == 2 ?
"s" : "");
> > -    if (port) {
> > -      putc (':', fp);
> > -      shell_quote (port, fp);
> > -    }
> > -    if (export_name && strcmp (export_name, "") !=
0) {
> > -      putc ('/', fp);
> > -      uri_quote (export_name, fp);
> > -    }
> > -    break;
> > -  default:
> > -    abort ();
> > -  }
> > +  if (uri)
> > +    shell_quote (uri, fp);
> 
> ...while here, we shell-quote the entire string...
Right, and '?' is not listed as a "safe char" so it should be
quoted:

https://gitlab.com/nbdkit/nbdkit/-/blob/ff3c9eb0e2afb24def80950cbfc963c14b037ba5/common/utils/quote.c#L54
> >    putc ('\n', fp);
> >  
> >    /* Since nbdkit 1.24, $nbd is a synonym for $uri. */
> > diff --git a/server/main.c b/server/main.c
> > index 2a332bfdd..54eb348ba 100644
> > --- a/server/main.c
> 
> > +static char *
> > +make_uri (void)
> 
> > +
> > +  switch (service_mode) {
> > +  case SERVICE_MODE_UNIXSOCKET:
> > +    fprintf (fp, "nbd%s+unix://", tls == 2 ? "s"
: "");
> > +    if (export_name && strcmp (export_name, "") !=
0) {
> > +      putc ('/', fp);
> > +      uri_quote (export_name, fp);
> > +    }
> > +    fprintf (fp, "?socket=");
> > +    uri_quote (unixsocket, fp);
> 
> ...where the manual shell-quoting is no longer injected.  Yes, this
> looks correct (the appearance of the quoting, using '' instead of
\,
> may be different, but the resulting string as parsed by the shell is
> the same).
> 
> > +    break;
> > +  case SERVICE_MODE_VSOCK:
> > +    /* 1 = VMADDR_CID_LOCAL */
> > +    fprintf (fp, "nbd%s+vsock://1", tls == 2 ?
"s" : "");
> > +    if (port) {
> > +      putc (':', fp);
> > +      fputs (port, fp);
> > +    }
> > +    if (export_name && strcmp (export_name, "") !=
0) {
> > +      putc ('/', fp);
> > +      uri_quote (export_name, fp);
> > +    }
> > +    break;
> > +  case SERVICE_MODE_TCPIP:
> > +    fprintf (fp, "nbd%s://localhost", tls == 2 ?
"s" : "");
> > +    if (port) {
> > +      putc (':', fp);
> > +      fputs (port, fp);
> > +    }
> > +    if (export_name && strcmp (export_name, "") !=
0) {
> > +      putc ('/', fp);
> > +      uri_quote (export_name, fp);
> > +    }
> > +    break;
> > +
> > +  case SERVICE_MODE_SOCKET_ACTIVATION:
> > +  case SERVICE_MODE_LISTEN_STDIN:
> > +    abort ();                   /* see above */
> > +  default:
> > +    abort ();
> 
> Could consolidate these labels, although I don't know if a compiler
> would be picky about:
> 
>   case ...:
>     /* comment */
>   default:
>     abort ();
> 
> so I'm also fine leaving it as-is.
> 
> Reviewed-by: Eric Blake <eblake at redhat.com>
Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Libguestfs - Sep 2023 - [PATCH nbdkit 04/10] server: Calculate $uri in one place

[Libguestfs] [PATCH nbdkit 04/10] server: Calculate $uri in one place

[Libguestfs] [PATCH nbdkit 04/10] server: Calculate $uri in one place