Re: [Libguestfs] URI Handling Patch

On Wed, Jul 01, 2015 at 09:58:05PM +0000, Gabriel Hartmann wrote: 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