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
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
> > > 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?>I didn't, it was just moved. In any case see the attached fix.> 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" 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? -- Gabriel
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
+Snesha Foss <sneshaf@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@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 >