Hi, On Wednesday 01 July 2015 21:58:05 Gabriel Hartmann wrote: > Here's the latest patch. I think this should address the problem. The > query string is now only appended to the end of a URI in the HTTP and HTTPS > cases. > > The add-uri test now passes, and 'make check' still passes.
Other than what Rich said, I have few more notes: > From dcf395ddb16ed6cbef4214d273f21b32b183f0eb Mon Sep 17 00:00:00 2001 > From: Ubuntu <[email protected]> > Date: Thu, 25 Jun 2015 22:15:05 +0000 Please put your name and email. > Subject: [PATCH] Append the query string portion of a URI to the drive path > when in HTTP or HTTPS protocols Can you please append also the reference to the bug? That is, "... protocols (RHBZ#1092583)". > diff --git a/fish/options.c b/fish/options.c > index b1be711..0e4b468 100644 > --- a/fish/options.c > +++ b/fish/options.c > @@ -64,6 +64,7 @@ option_a (const char *arg, const char *format, struct drv > **drvsp) > drv->type = drv_uri; > drv->nr_drives = -1; > drv->uri.path = uri.path; > + drv->uri.query = uri.query; This should be free'd below, in free_drives. > @@ -100,12 +102,11 @@ is_uri (const char *arg) > } > > static int > -parse (const char *arg, char **path_ret, char **protocol_ret, > +parse (const char *arg, char **path_ret, char **query_ret, char > **protocol_ret, > char ***server_ret, char **username_ret, char **password_ret) > { > CLEANUP_XMLFREEURI xmlURIPtr uri = NULL; > CLEANUP_FREE char *socket = NULL; > - char *path; This change, together with the renaming of it to tmpPath, is not needed. > diff --git a/generator/actions.ml b/generator/actions.ml > index d5e5ccf..75b52ea 100644 > --- a/generator/actions.ml > +++ b/generator/actions.ml > @@ -1336,7 +1336,7 @@ not all belong to a single logical operating system > > { defaults with > name = "add_drive"; added = (0, 0, 3); > - style = RErr, [String "filename"], [OBool "readonly"; OString "format"; > OString "iface"; OString "name"; OString "label"; OString "protocol"; > OStringList "server"; OString "username"; OString "secret"; OString > "cachemode"; OString "discard"; OBool "copyonread"]; > + style = RErr, [String "filename"], [OBool "readonly"; OString "format"; > OString "iface"; OString "name"; OString "label"; OString "protocol"; > OStringList "server"; OString "username"; OString "secret"; OString > "cachemode"; OString "discard"; OBool "copyonread"; OString "query"]; > once_had_no_optargs = true; The new parameter should be described, even briefly, in the documentation below. > diff --git a/src/drives.c b/src/drives.c > index ad747ab..1e6c95d 100644 > --- a/src/drives.c > +++ b/src/drives.c > @@ -744,9 +744,8 @@ guestfs_impl_add_drive_opts (guestfs_h *g, const char > *filename, > > data.nr_servers = 0; > data.servers = NULL; > - data.exportname = filename; > > - data.readonly = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_READONLY_BITMASK > + data.readonly = optargs->bitmask & > GUESTFS_ADD_DRIVE_OPTS_READONLY_BITMASK Extra indentation change. > ? optargs->readonly : false; > data.format = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_FORMAT_BITMASK > ? optargs->format : NULL; > @@ -771,6 +770,25 @@ guestfs_impl_add_drive_opts (guestfs_h *g, const char > *filename, > data.cachemode = optargs->bitmask & > GUESTFS_ADD_DRIVE_OPTS_CACHEMODE_BITMASK > ? optargs->cachemode : NULL; > > + /* If http or https are being used then the full path should > + * be the path + query string. > + */ > + char *fullPath = NULL; This is leaked, so need to be CLEANUP_FREE. > + if ((STREQ (protocol, "http") || STREQ (protocol, "https")) && > + optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_QUERY_BITMASK ) { > + > + if (asprintf(&fullPath, "%s?%s", filename, optargs->query) != -1) { Please use safe_asprintf, which returns the string and never fails (does abort() on memory allocation failures). > + data.exportname = fullPath; > + } else { > + error (g, _("path and query_string concatenation must not fail.")); > + free_drive_servers (data.servers, data.nr_servers); > + return -1; > + } > + > + } else { > + data.exportname = filename; > + } > + > if (optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_DISCARD_BITMASK) { > if (STREQ (optargs->discard, "disable")) > data.discard = discard_disable; > diff --git a/src/launch-direct.c b/src/launch-direct.c > index ea67ec9..cb82f20 100644 > --- a/src/launch-direct.c > +++ b/src/launch-direct.c > @@ -1224,7 +1224,7 @@ make_uri (guestfs_h *g, const char *scheme, const char > *user, > break; > } > > - return (char *) xmlSaveUri (&uri); > + return xmlURIUnescapeString((char *) xmlSaveUri (&uri), -1, NULL); I think this, if really needed, is in the wrong place. IMHO it would be better to keep the query string separate in the drive_source struct, and put it back as proper query in make_uri: with have a separate parameter for make_uri, you would then pass the query only for the protocols requiring it. Thanks, -- Pino Toscano _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
