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 <ebl...@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 mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs