[Libguestfs] RFC: Handle query strings for http and https (RHBZ#1092583)

Richard W.M. Jones rjones at redhat.com
Mon Feb 2 12:56:20 UTC 2015


On Mon, Feb 02, 2015 at 01:11:07PM +0100, Pino Toscano wrote:
> Parse the query string from URLs, and pass it as new add_drive optarg;
> use it when building the file= URL for qemu.
> 
> Accept query string only for http and https protocols, for now.
> ---
> Possibly it looks like an ad-hoc solution for http(s), although I'm not
> sure how it could possibly be generalized somehow (maybe "extra params"
> which would be the query string for http(s)?).

A few comments:

It needs test case(s) in fish/test-add-uri.sh.

I think in the API and code we should either consistently call
it 'query' or 'querystring' (whichever, but consistently).

What's quoting/escaping does the querystring require?  Probably the
caller should not have to quote it, but then there are two problems:
(a) does the quoting need to be removed when we parse the guestfish
URL and (b) does quoting need to be added when we call curl at the API
level?

Rich.

> 
>  customize/customize_main.ml |  4 ++--
>  fish/options.c              |  6 ++++++
>  fish/options.h              |  1 +
>  fish/uri.c                  | 27 ++++++++++++++++++++++++---
>  fish/uri.h                  |  1 +
>  generator/actions.ml        | 11 ++++++++++-
>  mllib/uRI.ml                |  1 +
>  mllib/uRI.mli               |  1 +
>  mllib/uri-c.c               | 13 ++++++++++++-
>  resize/resize.ml            |  4 ++--
>  src/drives.c                | 41 +++++++++++++++++++++++++++++++++++++++++
>  src/guestfs-internal.h      |  2 ++
>  src/launch-direct.c         | 24 +++++++++++++-----------
>  sysprep/main.ml             |  4 ++--
>  14 files changed, 118 insertions(+), 22 deletions(-)
> 
> diff --git a/customize/customize_main.ml b/customize/customize_main.ml
> index 5bba71a..9b9613f 100644
> --- a/customize/customize_main.ml
> +++ b/customize/customize_main.ml
> @@ -167,12 +167,12 @@ read the man page virt-customize(1).
>            fun (uri, format) ->
>              let { URI.path = path; protocol = protocol;
>                    server = server; username = username;
> -                  password = password } = uri in
> +                  password = password; query = query } = uri in
>              let discard = if readonly then None else Some "besteffort" in
>              g#add_drive
>                ~readonly ?discard
>                ?format ~protocol ?server ?username ?secret:password
> -              path
> +              ?querystring:query path
>          ) files
>    in
>  
> diff --git a/fish/options.c b/fish/options.c
> index 9ffcc6b..0d5ec2e 100644
> --- a/fish/options.c
> +++ b/fish/options.c
> @@ -68,6 +68,7 @@ option_a (const char *arg, const char *format, struct drv **drvsp)
>      drv->uri.server = uri.server;
>      drv->uri.username = uri.username;
>      drv->uri.password = uri.password;
> +    drv->uri.query = uri.query;
>      drv->uri.format = format;
>      drv->uri.orig_uri = arg;
>    }
> @@ -172,6 +173,10 @@ add_drives_handle (guestfs_h *g, struct drv *drv, char next_drive)
>          ad_optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_SECRET_BITMASK;
>          ad_optargs.secret = drv->uri.password;
>        }
> +      if (drv->uri.query) {
> +        ad_optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_QUERYSTRING_BITMASK;
> +        ad_optargs.querystring = drv->uri.query;
> +      }
>  
>        r = guestfs_add_drive_opts_argv (g, drv->uri.path, &ad_optargs);
>        if (r == -1)
> @@ -307,6 +312,7 @@ free_drives (struct drv *drv)
>      guestfs___free_string_list (drv->uri.server);
>      free (drv->uri.username);
>      free (drv->uri.password);
> +    free (drv->uri.query);
>      break;
>    case drv_d:
>      /* d.filename is optarg, don't free it */
> diff --git a/fish/options.h b/fish/options.h
> index cf68122..d4b0fa5 100644
> --- a/fish/options.h
> +++ b/fish/options.h
> @@ -73,6 +73,7 @@ struct drv {
>        char **server;        /* server(s) - can be NULL */
>        char *username;       /* username - can be NULL */
>        char *password;       /* password - can be NULL */
> +      char *query;          /* query - can be NULL */
>        const char *format;   /* format (NULL == autodetect) */
>        const char *orig_uri; /* original URI (for error messages etc.) */
>      } uri;
> diff --git a/fish/uri.c b/fish/uri.c
> index f45c907..8459a7c 100644
> --- a/fish/uri.c
> +++ b/fish/uri.c
> @@ -34,7 +34,7 @@
>  #include "uri.h"
>  
>  static int is_uri (const char *arg);
> -static int parse (const char *arg, char **path_ret, char **protocol_ret, char ***server_ret, char **username_ret, char **password_ret);
> +static int parse (const char *arg, char **path_ret, char **protocol_ret, char ***server_ret, char **username_ret, char **password_ret, char **query_ret);
>  static char *query_get (xmlURIPtr uri, const char *search_name);
>  static int make_server (xmlURIPtr uri, const char *socket, char ***ret);
>  
> @@ -46,10 +46,11 @@ parse_uri (const char *arg, struct uri *uri_ret)
>    char **server = NULL;
>    char *username = NULL;
>    char *password = NULL;
> +  char *query = NULL;
>  
>    /* Does it look like a URI? */
>    if (is_uri (arg)) {
> -    if (parse (arg, &path, &protocol, &server, &username, &password) == -1)
> +    if (parse (arg, &path, &protocol, &server, &username, &password, &query) == -1)
>        return -1;
>    }
>    else {
> @@ -72,6 +73,7 @@ parse_uri (const char *arg, struct uri *uri_ret)
>    uri_ret->server = server;
>    uri_ret->username = username;
>    uri_ret->password = password;
> +  uri_ret->query = query;
>    return 0;
>  }
>  
> @@ -101,7 +103,8 @@ is_uri (const char *arg)
>  
>  static int
>  parse (const char *arg, char **path_ret, char **protocol_ret,
> -           char ***server_ret, char **username_ret, char **password_ret)
> +           char ***server_ret, char **username_ret, char **password_ret,
> +           char **query_ret)
>  {
>    CLEANUP_XMLFREEURI xmlURIPtr uri = NULL;
>    CLEANUP_FREE char *socket = NULL;
> @@ -201,6 +204,24 @@ parse (const char *arg, char **path_ret, char **protocol_ret,
>      return -1;
>    }
>  
> +  *query_ret = NULL;
> +  /* Copy the query string part only when not building a unix: URI
> +   * (e.g. for nbd).  See logic done in make_server.
> +   */
> +  if (uri->query_raw && STRNEQ (uri->query_raw, "") &&
> +      !(socket && (uri->server == NULL || STREQ (uri->server, "")))) {
> +    *query_ret = strdup (uri->query_raw);
> +    if (*query_ret == NULL) {
> +      perror ("strdup: query");
> +      free (*protocol_ret);
> +      guestfs___free_string_list (*server_ret);
> +      free (*username_ret);
> +      free (*password_ret);
> +      free (*path_ret);
> +      return -1;
> +    }
> +  }
> +
>    return 0;
>  }
>  
> diff --git a/fish/uri.h b/fish/uri.h
> index 9202a70..d9b100a 100644
> --- a/fish/uri.h
> +++ b/fish/uri.h
> @@ -27,6 +27,7 @@ struct uri {
>    char **server;                /* server(s) - can be NULL */
>    char *username;               /* username - can be NULL */
>    char *password;               /* password - can be NULL */
> +  char *query;                  /* query string - can be NULL */
>  };
>  
>  /* Parse the '-a' option parameter 'arg', and place the result in
> diff --git a/generator/actions.ml b/generator/actions.ml
> index c0beaae..18cbfc0 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -1318,7 +1318,7 @@ not all belong to a single logical operating system
>  
>    { defaults with
>      name = "add_drive";
> -    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 "querystring"];
>      once_had_no_optargs = true;
>      blocking = false;
>      fish_alias = ["add"];
> @@ -1575,6 +1575,15 @@ of the same area of disk.
>  
>  The default is false.
>  
> +=item C<querystring>
> +
> +The string parameter C<querystring> specifies the optional query string
> +to be used when accessing some kind of resources, for example:
> +
> + https://example.org/images/?image=test1
> +
> +in such cases, C<image=test1> needs to passed as C<querystring>.
> +
>  =back" };
>  
>    { defaults with
> diff --git a/mllib/uRI.ml b/mllib/uRI.ml
> index d4f7522..9ee53ee 100644
> --- a/mllib/uRI.ml
> +++ b/mllib/uRI.ml
> @@ -22,6 +22,7 @@ type uri = {
>    server : string array option;
>    username : string option;
>    password : string option;
> +  query : string option;
>  }
>  
>  external parse_uri : string -> uri = "virt_resize_parse_uri"
> diff --git a/mllib/uRI.mli b/mllib/uRI.mli
> index 0692f95..7fb9acd 100644
> --- a/mllib/uRI.mli
> +++ b/mllib/uRI.mli
> @@ -24,6 +24,7 @@ type uri = {
>    server : string array option;         (** list of servers *)
>    username : string option;             (** username *)
>    password : string option;             (** password *)
> +  query : string option;                (** query string *)
>  }
>  
>  val parse_uri : string -> uri
> diff --git a/mllib/uri-c.c b/mllib/uri-c.c
> index aa63c48..e649cdb 100644
> --- a/mllib/uri-c.c
> +++ b/mllib/uri-c.c
> @@ -49,7 +49,7 @@ virt_resize_parse_uri (value argv /* arg value, not an array! */)
>      caml_invalid_argument ("URI.parse_uri");
>  
>    /* Convert the struct into an OCaml tuple. */
> -  rv = caml_alloc_tuple (5);
> +  rv = caml_alloc_tuple (6);
>  
>    /* path : string */
>    sv = caml_copy_string (uri.path);
> @@ -94,5 +94,16 @@ virt_resize_parse_uri (value argv /* arg value, not an array! */)
>      ov = Val_int (0);
>    Store_field (rv, 4, ov);
>  
> +  /* query : string option */
> +  if (uri.query) {
> +    sv = caml_copy_string (uri.query);
> +    free (uri.query);
> +    ov = caml_alloc (1, 0);
> +    Store_field (ov, 0, sv);
> +  }
> +  else
> +    ov = Val_int (0);
> +  Store_field (rv, 5, ov);
> +
>    CAMLreturn (rv);
>  }
> diff --git a/resize/resize.ml b/resize/resize.ml
> index 871c6a4..2a2765c 100644
> --- a/resize/resize.ml
> +++ b/resize/resize.ml
> @@ -338,8 +338,8 @@ read the man page virt-resize(1).
>      if verbose then g#set_verbose true;
>      let _, { URI.path = path; protocol = protocol;
>               server = server; username = username;
> -             password = password } = infile in
> -    g#add_drive ?format ~readonly:true ~protocol ?server ?username ?secret:password path;
> +             password = password; query = query } = infile in
> +    g#add_drive ?format ~readonly:true ~protocol ?server ?username ?secret:password ?querystring:query path;
>      (* The output disk is being created, so use cache=unsafe here. *)
>      g#add_drive ?format:output_format ~readonly:false ~cachemode:"unsafe"
>        outfile;
> diff --git a/src/drives.c b/src/drives.c
> index 34bf63d..3668e29 100644
> --- a/src/drives.c
> +++ b/src/drives.c
> @@ -63,6 +63,7 @@ struct drive_create_data {
>    const char *cachemode;
>    enum discard discard;
>    bool copyonread;
> +  const char *query;
>  };
>  
>  COMPILE_REGEXP (re_hostname_port, "(.*):(\\d+)$", 0)
> @@ -144,6 +145,7 @@ create_drive_non_file (guestfs_h *g,
>    drv->src.username = data->username ? safe_strdup (g, data->username) : NULL;
>    drv->src.secret = data->secret ? safe_strdup (g, data->secret) : NULL;
>    drv->src.format = data->format ? safe_strdup (g, data->format) : NULL;
> +  drv->src.query = data->query ? safe_strdup (g, data->query) : NULL;
>  
>    drv->readonly = data->readonly;
>    drv->iface = data->iface ? safe_strdup (g, data->iface) : NULL;
> @@ -171,6 +173,13 @@ static struct drive *
>  create_drive_curl (guestfs_h *g,
>                     const struct drive_create_data *data)
>  {
> +  if (data->query != NULL &&
> +      data->protocol != drive_protocol_http &&
> +      data->protocol != drive_protocol_https) {
> +    error (g, _("curl: you cannot specify a query string with this protocol"));
> +    return NULL;
> +  }
> +
>    if (data->nr_servers != 1) {
>      error (g, _("curl: you must specify exactly one server"));
>      return NULL;
> @@ -207,6 +216,10 @@ create_drive_gluster (guestfs_h *g,
>      error (g, _("gluster: you cannot specify a secret with this protocol"));
>      return NULL;
>    }
> +  if (data->query != NULL) {
> +    error (g, _("gluster: you cannot specify a query string with this protocol"));
> +    return NULL;
> +  }
>  
>    if (data->nr_servers != 1) {
>      error (g, _("gluster: you must specify exactly one server"));
> @@ -250,6 +263,10 @@ create_drive_nbd (guestfs_h *g,
>      error (g, _("nbd: you cannot specify a secret with this protocol"));
>      return NULL;
>    }
> +  if (data->query != NULL) {
> +    error (g, _("nbd: you cannot specify a query string with this protocol"));
> +    return NULL;
> +  }
>  
>    if (data->nr_servers != 1) {
>      error (g, _("nbd: you must specify exactly one server"));
> @@ -268,6 +285,11 @@ create_drive_rbd (guestfs_h *g,
>  {
>    size_t i;
>  
> +  if (data->query != NULL) {
> +    error (g, _("rbd: you cannot specify a query string with this protocol"));
> +    return NULL;
> +  }
> +
>    for (i = 0; i < data->nr_servers; ++i) {
>      if (data->servers[i].transport != drive_transport_none &&
>          data->servers[i].transport != drive_transport_tcp) {
> @@ -307,6 +329,10 @@ create_drive_sheepdog (guestfs_h *g,
>      error (g, _("sheepdog: you cannot specify a secret with this protocol"));
>      return NULL;
>    }
> +  if (data->query != NULL) {
> +    error (g, _("sheepdog: you cannot specify a query string with this protocol"));
> +    return NULL;
> +  }
>  
>    for (i = 0; i < data->nr_servers; ++i) {
>      if (data->servers[i].transport != drive_transport_none &&
> @@ -337,6 +363,11 @@ static struct drive *
>  create_drive_ssh (guestfs_h *g,
>                    const struct drive_create_data *data)
>  {
> +  if (data->query != NULL) {
> +    error (g, _("ssh: you cannot specify a query string with this protocol"));
> +    return NULL;
> +  }
> +
>    if (data->nr_servers != 1) {
>      error (g, _("ssh: you must specify exactly one server"));
>      return NULL;
> @@ -379,6 +410,10 @@ create_drive_iscsi (guestfs_h *g,
>      error (g, _("iscsi: you cannot specify a secret with this protocol"));
>      return NULL;
>    }
> +  if (data->query != NULL) {
> +    error (g, _("iscsi: you cannot specify a query string with this protocol"));
> +    return NULL;
> +  }
>  
>    if (data->nr_servers != 1) {
>      error (g, _("iscsi: you must specify exactly one server"));
> @@ -770,6 +805,8 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename,
>      ? optargs->secret : NULL;
>    data.cachemode = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_CACHEMODE_BITMASK
>      ? optargs->cachemode : NULL;
> +  data.query = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_QUERYSTRING_BITMASK
> +    ? optargs->querystring : NULL;
>  
>    if (optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_DISCARD_BITMASK) {
>      if (STREQ (optargs->discard, "disable"))
> @@ -835,6 +872,10 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename,
>        error (g, _("you cannot specify a secret with file-backed disks"));
>        return -1;
>      }
> +    if (data.query != NULL) {
> +      error (g, _("you cannot specify a query string with file-backed disks"));
> +      return -1;
> +    }
>  
>      if (STREQ (filename, "/dev/null"))
>        drv = create_drive_dev_null (g, &data);
> diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
> index 573c3da..439a80b 100644
> --- a/src/guestfs-internal.h
> +++ b/src/guestfs-internal.h
> @@ -242,6 +242,8 @@ struct drive_source {
>    char *username;
>    /* Optional secret (may be NULL if not specified). */
>    char *secret;
> +  /* Optional query string (may be NULL if not specified). */
> +  char *query;
>  };
>  
>  enum discard {
> diff --git a/src/launch-direct.c b/src/launch-direct.c
> index e6ed54a..00f3190 100644
> --- a/src/launch-direct.c
> +++ b/src/launch-direct.c
> @@ -1189,7 +1189,8 @@ qemu_escape_param (guestfs_h *g, const char *param)
>  static char *
>  make_uri (guestfs_h *g, const char *scheme, const char *user,
>            const char *password,
> -          struct drive_server *server, const char *path)
> +          struct drive_server *server, const char *path,
> +          const char *querystring)
>  {
>    xmlURI uri = { .scheme = (char *) scheme,
>                   .user = (char *) user };
> @@ -1217,6 +1218,7 @@ make_uri (guestfs_h *g, const char *scheme, const char *user,
>    case drive_transport_tcp:
>      uri.server = server->u.hostname;
>      uri.port = server->port;
> +    uri.query_raw = (char *) querystring;
>      break;
>    case drive_transport_unix:
>      query = safe_asprintf (g, "socket=%s", server->u.socket);
> @@ -1258,36 +1260,36 @@ guestfs___drive_source_qemu_param (guestfs_h *g, const struct drive_source *src)
>  
>    case drive_protocol_ftp:
>      return make_uri (g, "ftp", src->username, src->secret,
> -                     &src->servers[0], src->u.exportname);
> +                     &src->servers[0], src->u.exportname, NULL);
>  
>    case drive_protocol_ftps:
>      return make_uri (g, "ftps", src->username, src->secret,
> -                     &src->servers[0], src->u.exportname);
> +                     &src->servers[0], src->u.exportname, NULL);
>  
>    case drive_protocol_gluster:
>      switch (src->servers[0].transport) {
>      case drive_transport_none:
>        return make_uri (g, "gluster", NULL, NULL,
> -                       &src->servers[0], src->u.exportname);
> +                       &src->servers[0], src->u.exportname, NULL);
>      case drive_transport_tcp:
>        return make_uri (g, "gluster+tcp", NULL, NULL,
> -                       &src->servers[0], src->u.exportname);
> +                       &src->servers[0], src->u.exportname, NULL);
>      case drive_transport_unix:
>        return make_uri (g, "gluster+unix", NULL, NULL,
> -                       &src->servers[0], NULL);
> +                       &src->servers[0], NULL, NULL);
>      }
>  
>    case drive_protocol_http:
>      return make_uri (g, "http", src->username, src->secret,
> -                     &src->servers[0], src->u.exportname);
> +                     &src->servers[0], src->u.exportname, src->query);
>  
>    case drive_protocol_https:
>      return make_uri (g, "https", src->username, src->secret,
> -                     &src->servers[0], src->u.exportname);
> +                     &src->servers[0], src->u.exportname, src->query);
>  
>    case drive_protocol_iscsi:
>      return make_uri (g, "iscsi", NULL, NULL,
> -                     &src->servers[0], src->u.exportname);
> +                     &src->servers[0], src->u.exportname, NULL);
>  
>    case drive_protocol_nbd: {
>      CLEANUP_FREE char *p = NULL;
> @@ -1374,11 +1376,11 @@ guestfs___drive_source_qemu_param (guestfs_h *g, const struct drive_source *src)
>  
>    case drive_protocol_ssh:
>      return make_uri (g, "ssh", src->username, src->secret,
> -                     &src->servers[0], src->u.exportname);
> +                     &src->servers[0], src->u.exportname, NULL);
>  
>    case drive_protocol_tftp:
>      return make_uri (g, "tftp", src->username, src->secret,
> -                     &src->servers[0], src->u.exportname);
> +                     &src->servers[0], src->u.exportname, NULL);
>    }
>  
>    abort ();
> diff --git a/sysprep/main.ml b/sysprep/main.ml
> index 249800f..ff9852c 100644
> --- a/sysprep/main.ml
> +++ b/sysprep/main.ml
> @@ -201,12 +201,12 @@ read the man page virt-sysprep(1).
>              fun (uri, format) ->
>                let { URI.path = path; protocol = protocol;
>                      server = server; username = username;
> -                    password = password } = uri in
> +                    password = password; query = query } = uri in
>                let discard = if readonly then None else Some "besteffort" in
>                g#add_drive
>                  ~readonly ?discard
>                  ?format ~protocol ?server ?username ?secret:password
> -                path
> +                ?querystring:query path
>            ) files
>      in
>  
> -- 
> 1.9.3
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top




More information about the Libguestfs mailing list