Re: [Libguestfs] URI Handling Patch
Hi,
In data giovedì 25 giugno 2015 18:44:50, Gabriel Hartmann ha scritto:
I have written a patch (please see attached) which fixes both of
these bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1092583
https://bugzilla.redhat.com/show_bug.cgi?id=1232477
By default, when saving a URI using xmlSaveUri it escapes everything in the
URI. QEMU doesn't want anything escaped, so now I unescape everything
after the URI is generated. Unfortunately there's no flag to change the
default behavior.
I'm not sure that's the actual issue here, but I'm somehow included to
think this is another consequence of the lack of query string handling
for http/https URIs.
In any case, do you have a simple reproducer for this escaping handling
for qemu?
The other problem was that only the "path" portion of the
URI struct was
being used to indicate the path. That's natural enough, but that practice
is what was dropping the query string. The query string is kept separately
from the path. I now concat the query string back onto the URI with a
separating '?'.
I don't think that appending the query string to the path is a good idea.
For example, we do a minimum of parsing of the URI, and for some
protocols (like the "We may have to adjust the path ..." comment says)
we adjust path according to the elements in the query string.
I've successfully mounted remote vhds in Azure with this new
code, and the
basic set of tests pass. If there's anything else I can do by way of
verification, please let me know.
diff --git a/fish/uri.c b/fish/uri.c
index 593e62a..1566cdf 100644
--- a/fish/uri.c
+++ b/fish/uri.c
@@ -178,12 +178,20 @@ parse (const char *arg, char **path_ret, char **protocol_ret,
}
}
+ if (asprintf(&path, "%s?%s", uri->path, uri->query_raw) == -1) {
+ perror ("asprintf: path + query_raw");
+ free (*protocol_ret);
+ guestfs_int_free_string_list (*server_ret);
+ free (*username_ret);
+ free (*password_ret);
+ return -1;
+ }
'path' created here is leaked. Also, this needs to take into account
that either uri->path or uri->query_raw may be null.
/* We may have to adjust the path depending on the protocol. For
* example ceph/rbd URIs look like rbd:///pool/disk, but the
* exportname expected will be "pool/disk". Here, uri->path will be
* "/pool/disk" so we have to knock off the leading '/' character.
*/
- path = uri->path;
if (path && path[0] == '/' &&
(STREQ (uri->scheme, "gluster") ||
STREQ (uri->scheme, "iscsi") ||
@@ -192,15 +200,6 @@ parse (const char *arg, char **path_ret, char **protocol_ret,
path++;
*path_ret = strdup (path ? path : "");
- if (*path_ret == NULL) {
- perror ("strdup: path");
- free (*protocol_ret);
- guestfs_int_free_string_list (*server_ret);
- free (*username_ret);
- free (*password_ret);
- return -1;
- }
Why did you remove the error checking?
Ad additional checking for URIs, we have fish/test-add-uri.sh, which
fails with your patch. You might want to also add additional checks with
for query string http/https URIs.
Thanks,
--
Pino Toscano