[Libguestfs] URI Handling Patch

Pino Toscano ptoscano at redhat.com
Fri Jun 26 15:11:35 UTC 2015


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




More information about the Libguestfs mailing list