Richard W.M. Jones
2023-Sep-11 14:09 UTC
[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
Eric Blake
2023-Sep-11 14:19 UTC
[Libguestfs] [PATCH nbdkit 04/10] server: Calculate $uri in one place
On Mon, Sep 11, 2023 at 03:09:21PM +0100, Richard W.M. Jones wrote:> > > - 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 :-)> > > + 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).My point was that pre-patch, we had: nbd+unix://\?socket=... and post-patch, we have: 'nbd+unix://?socket=...' but both forms are equally quoted; after the shell removes \ or '' quoting, we are left with: nbd+unix://?socket=... and neither form allowed the glob expansion of the (unlikely) file nbd+unix:/Xsocket=... My comment was more about why changing the fprintf("\\?socket=") pre-patch to fprintf("?socket=") post-patch was correct. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org