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 <gabhart@gabhart-ubuntu.gabhart-ubuntu.d2.internal.cloudapp.net> 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