[Libguestfs] URI Handling Patch

[Pino Toscano]

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 at 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