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
