Re: [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(a)redhat.com&gt;