1:44 p.m.
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.
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'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.
-- Gabriel
10:11 a.m.
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
1:50 p.m.
> 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?
I don't have a simple repro for qemu, but this is pretty close. In
guestfish do this:
add
"/vhds/osdiskforconsul0-osdisk.vhd?se=2016-01-01T00%3A00%3A00Z&sp=r&sv=2014-02-14&sr=b&sig=LOlrHXrQeaqlSEP51hRi7E5KDa9lnkqSvLTaZBmTkrQ%3D"
readonly:true protocol:https server:
gabhartswarmstorage.blob.core.windows.net
After you execute 'run', you should see qemu complaining about something
like a 404 Error and the file not existing. After the unescape change the
above will start working.
> 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.
Are you talking about this comment?
/* 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.
*/
This is talking about removing leading the '/' in the path sometimes
depending on protocol. Nothing to do with the query string.
> + 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.
Good point. Fixed. See attached patch.
> *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?
10:37 a.m.
In data venerdì 26 giugno 2015 18:50:16, Gabriel Hartmann ha scritto:
>
> > 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?
>
I don't have a simple repro for qemu, but this is pretty close. In
guestfish do this:
add
"/vhds/osdiskforconsul0-osdisk.vhd?se=2016-01-01T00%3A00%3A00Z&sp=r&sv=2014-02-14&sr=b&sig=LOlrHXrQeaqlSEP51hRi7E5KDa9lnkqSvLTaZBmTkrQ%3D"
readonly:true protocol:https server:
gabhartswarmstorage.blob.core.windows.net
After you execute 'run', you should see qemu complaining about something
like a 404 Error and the file not existing. After the unescape change the
above will start working.
In this case, you can check in a simplier way:
$ guestfish -x -a '/vhds/etc...'
you'll see a "libguestfs: trace: add_drive ..." line contaning the
actual add_drive call which guestfish will do to the library, and the
parameters passed to it.
Also, I'm thinking we should focus on making sure to have the proper
query string for http(s) URIs to add_drive, we'll deal with the
percent-encoding on the way.
> > 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.
Are you talking about this comment?
/* 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.
*/
This is talking about removing leading the '/' in the path sometimes
depending on protocol. Nothing to do with the query string.
Yes, although it was not directly to the specified code snippet. The
advice here is that some protocols don't need a query string in the
path, because parts of it might be handled differently. See also below.
> 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.
>
The test is now failing on this case:
+ guestfish -x -a 'nbd:///export?socket=/sk'
+ grep -sq 'add_drive *"/export"* "protocol:nbd"
"server:unix:/sk"'
test-add-uri.out
because I am now appending the query string as in:
add_drive* "/export?socket=/sk"* "protocol:nbd"
"server:unix:/sk"
It didn't expect a query string before, and it must not take it now,
as the parameters are not related to the actual path, but they are
connection parameters.
In which cases should the query string be appended and in what cases
should
it be dropped? I believe in the http and https case it should be
appended. I'm not familiar with the nbd protocol. Is dropping the query
string always the right thing to do for this protocol? Other protocols?
Surely http(s) drives need to have the query string that was specified
in guestfish URIs, just like the add_drive API need to properly deal
with such extra parameter, and manage them depending on the protocol.
We have few possibilities at this point, in order of complexity:
(a) split the query string into an "hash table" (i.e. a C list of
key/value items), pass that as new parameter to add_drive, and rebuild
a query string depending on the backend (direct/qemu, libvirt, etc) and
on the protocol (http, nbd, etc)
(b) pass the query string as-is (percent-encoded or not) to add_drive,
and add it back depending on the protocol
(c) join the query string to the path, depending on the protocol, in
guestfish
Your solution so far is (c), while IMHO a better one would be (b) or
even (a).
Also, something more to check is how e.g. libvirt needs such kind of
URI parameter after the path, and how. You can check whether your
change also work with libvirt by exporting LIBGUESTFS_BACKEND=libvirt
(or also using "set-backend libvirt" in guestfish), provided you have
built libguestfs with libvirt support.
Thanks,
--
Pino Toscano
1:03 p.m.
(apologies if you see this twice, forgot the CC the first time)
In data venerdì 26 giugno 2015 18:50:16, Gabriel Hartmann ha scritto:
>
> > 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?
>
I don't have a simple repro for qemu, but this is pretty close. In
guestfish do this:
add
"/vhds/osdiskforconsul0-osdisk.vhd?se=2016-01-01T00%3A00%3A00Z&sp=r&sv=2014-02-14&sr=b&sig=LOlrHXrQeaqlSEP51hRi7E5KDa9lnkqSvLTaZBmTkrQ%3D"
readonly:true protocol:https server:
gabhartswarmstorage.blob.core.windows.net
After you execute 'run', you should see qemu complaining about something
like a 404 Error and the file not existing. After the unescape change the
above will start working.
In this case, you can check in a simplier way:
$ guestfish -x -a '/vhds/etc...'
you'll see a "libguestfs: trace: add_drive ..." line contaning the
actual add_drive call which guestfish will do to the library, and the
parameters passed to it.
Also, I'm thinking we should focus on making sure to have the proper
query string for http(s) URIs to add_drive, we'll deal with the
percent-encoding on the way.
> > 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.
Are you talking about this comment?
/* 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.
*/
This is talking about removing leading the '/' in the path sometimes
depending on protocol. Nothing to do with the query string.
Yes, although it was not directly to the specified code snippet. The
advice here is that some protocols don't need a query string in the
path, because parts of it might be handled differently. See also below.
> 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.
>
The test is now failing on this case:
+ guestfish -x -a 'nbd:///export?socket=/sk'
+ grep -sq 'add_drive *"/export"* "protocol:nbd"
"server:unix:/sk"'
test-add-uri.out
because I am now appending the query string as in:
add_drive* "/export?socket=/sk"* "protocol:nbd"
"server:unix:/sk"
It didn't expect a query string before, and it must not take it now,
as the parameters are not related to the actual path, but they are
connection parameters.
In which cases should the query string be appended and in what cases
should
it be dropped? I believe in the http and https case it should be
appended. I'm not familiar with the nbd protocol. Is dropping the query
string always the right thing to do for this protocol? Other protocols?
Surely http(s) drives need to have the query string that was specified
in guestfish URIs, just like the add_drive API need to properly deal
with such extra parameter, and manage them depending on the protocol.
We have few possibilities at this point, in order of complexity:
(a) split the query string into an "hash table" (i.e. a C list of
key/value items), pass that as new parameter to add_drive, and rebuild
a query string depending on the backend (direct/qemu, libvirt, etc) and
on the protocol (http, nbd, etc)
(b) pass the query string as-is (percent-encoded or not) to add_drive,
and add it back depending on the protocol
(c) join the query string to the path, depending on the protocol, in
guestfish
Your solution so far is (c), while IMHO a better one would be (b) or
even (a).
Also, something more to check is how e.g. libvirt needs such kind of
URI parameter after the path, and how. You can check whether your
change also work with libvirt by exporting LIBGUESTFS_BACKEND=libvirt
(or also using "set-backend libvirt" in guestfish), provided you have
built libguestfs with libvirt support.
Thanks,
--
Pino Toscano
1:09 p.m.
Re-adding Snesha: +sneshaf(a)microsoft.com <sneshaf(a)microsoft.com>
Hi Pino,
Thanks, this looks like a good list of alternatives. I think (b) is
probably the way to go. I'm willing to just add the query string to the
path in only the HTTP and HTTPS cases for now. If other protocols run into
some problems around lacking query strings we can probably re-visit the
issue. The (a) option seems needlessly complex and bug prone to me.
-- Gabriel
We have few possibilities at this point, in order of complexity:
(a) split the query string into an "hash table" (i.e. a C list of
key/value items), pass that as new parameter to add_drive, and rebuild
a query string depending on the backend (direct/qemu, libvirt, etc) and
on the protocol (http, nbd, etc)
(b) pass the query string as-is (percent-encoded or not) to add_drive,
and add it back depending on the protocol
(c) join the query string to the path, depending on the protocol, in
guestfish
Your solution so far is (c), while IMHO a better one would be (b) or
even (a).
Also, something more to check is how e.g. libvirt needs such kind of
URI parameter after the path, and how. You can check whether your
change also work with libvirt by exporting LIBGUESTFS_BACKEND=libvirt
(or also using "set-backend libvirt" in guestfish), provided you have
built libguestfs with libvirt support.
6:03 a.m.
On Thu, Jun 25, 2015 at 06:44:50PM +0000, Gabriel Hartmann wrote:
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.
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'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.
As Pino says, we can't just append the path and the query string.
I think the way to go is to add an extra parameter to the
guestfs_add_drive_opts API which would contain an OStringList of (key,
value) pairs (unescaped).
See generator/actions.ml add_drive
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
12:57 p.m.
+Snesha Foss <sneshaf(a)microsoft.com> who is taking over this work.
It definitely looks like adding the query string to every path is just
wrong.
I'm not sure I understand why we'd want to parse, deconstruct key value
pairs and then reconstruct the query string from these values and append
them to the path selectively. This seems like added complexity for a
benefit I don't understand.
I think in all http and https cases appending the query string is
required. It is part of the file path. I think possibly having a flag for
turning on and off query string appending would be a good idea for
protocols where this is not the case.
The question I asked in the previous e-mail, which still needs to be
answered no matter what scheme is ultimately chosen, is this:
Under what circumstances should the query string be appended to the path?
-- Gabriel
On Mon, Jun 29, 2015 at 4:03 AM Richard W.M. Jones <rjones(a)redhat.com>
wrote:
On Thu, Jun 25, 2015 at 06:44:50PM +0000, Gabriel Hartmann wrote:
> 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.
>
> 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'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.
As Pino says, we can't just append the path and the query string.
I think the way to go is to add an extra parameter to the
guestfs_add_drive_opts API which would contain an OStringList of (key,
value) pairs (unescaped).
See generator/actions.ml add_drive
Rich.
--
Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
1:14 p.m.
On Mon, Jun 29, 2015 at 05:57:38PM +0000, Gabriel Hartmann wrote:
+Snesha Foss <sneshaf(a)microsoft.com> who is taking over this
work.
It definitely looks like adding the query string to every path is just
wrong.
I'm not sure I understand why we'd want to parse, deconstruct key value
pairs and then reconstruct the query string from these values and append
them to the path selectively. This seems like added complexity for a
benefit I don't understand.
Yes, you're probably right that we shouldn't do that. However an
extra query string parameter (as an OString) would still be needed to
the add_drive call.
I think in all http and https cases appending the query string is
required. It is part of the file path. I think possibly having a flag for
turning on and off query string appending would be a good idea for
protocols where this is not the case.
There's what happens at the C API level (which is what I'm talking
about here), and what guestfish does when it separately parses up
URIs.
I think there is value in keeping the path and query separate because
they require different escaping, or perhaps more accurate to say,
currently we escape '?' in the path part, and changing that would
break the API for existing users.
Guestfish complicates everything by parsing the query for one
parameter (?socket=...) although that parameter is not used by the
http protocol backend.
The question I asked in the previous e-mail, which still needs to be
answered no matter what scheme is ultimately chosen, is this:
Under what circumstances should the query string be appended to the path?
If there are two separate parameters (path, optional query string) to
guestfs_add_drive then I hope it's clear when - you'd append the query
string when the query string is passed by the caller.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
4:58 p.m.
Hi All,
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.
-- Gabriel
5:20 p.m.
On Wed, Jul 01, 2015 at 09:58:05PM +0000, Gabriel Hartmann wrote:
Hi All,
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.
As a general note, it's easier for us to review patches if you send
them using 'git send-email'.
Also I think you should split up the commit into changes made to the
core library, and changes made to guestfish.
---
diff --git a/.gitignore b/.gitignore
index 6f14915..efa7179 100644
--- a/.gitignore
+++ b/.gitignore
@@ -27,6 +27,7 @@ cscope.out
.libs
Makefile
Makefile.in
+tags
^ This should be in a separate commit.
[...]
+ if ((STREQ (protocol, "http") || STREQ (protocol, "https"))
&&
+ optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_QUERY_BITMASK ) {
^ Probably better to put parentheses around the & expression, although
I don't believe it is strictly necessary.
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 suspect this is wrong. What is it supposed to do?
---
The general patch looks OK, however it desperately needs unit tests.
There are two places where unit tests might be added:
- tests/c-api/test-add-drive-opts.c (for the core library)
- fish/test-add-uri.sh (for guestfish URI parsing)
The tests should be included in the corresponding patch, and you
should also run them to ensure they really work (I know this sounds
obvious, but many people don't do this).
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
2:59 a.m.
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(a)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
3430
days inactive
3437
days old
11 comments
3 participants
participants (3)
-
Gabriel Hartmann -
Pino Toscano -
Richard W.M. Jones