Re: [libvirt] [PATCH v3] Fixed URI parsing

2012-02-24 Thread Daniel P. Berrange 
On Fri, Feb 24, 2012 at 07:09:49PM +0100, Martin Kletzander wrote:
> +xmlURIPtr
> +virURIParse(const char *uri)

> +unsigned char *
> +virURIFormat(xmlURIPtr uri)

The data types here are wrong compared to the header.

Also the return value should not be unsigned - that is
libxml2 bad practice we shouldn't copy

> diff --git a/src/util/viruri.h b/src/util/viruri.h
> new file mode 100644
> index 000..1315488
> --- /dev/null
> +++ b/src/util/viruri.h
> @@ -0,0 +1,18 @@
> +/*
> + * viruri.h: internal definitions used for URI parsing.
> + */
> +
> +#ifndef __VIR_URI_H__
> +# define __VIR_URI_H__
> +
> +# include 
> +
> +# include "internal.h"
> +
> +typedef xmlURIvirURI;
> +typedef xmlURIPtr virURIPtr;
> +
> +virURIPtrvirURIParse(const char *uri);
> +unsigned char *  virURIFormat(virURIPtr uri);
> +
> +#endif /* __VIR_URI_H__ */


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3] Fixed URI parsing

2012-02-24 Thread Eric Blake 
On 02/24/2012 11:09 AM, Martin Kletzander wrote:
> Function xmlParseURI does not remove square brackets around IPv6
> address when parsing. One of the solutions is making wrappers around
> functions working with xmlURI*. This assures that uri->server will be
> always properly assigned and it doesn't have to be changed when used
> on some new place in the code.
> For this purpose, functions virParseURI and virSaveURI were
> added. These function are wrappers around xmlParseURI and xmlSaveUri
> respectively.
> Also there is one new syntax check function to prohibit these functions
> anywhere else.
> 

> +++ b/src/libvirt_private.syms
> @@ -1430,6 +1430,11 @@ virTypedParameterArrayValidate;
>  virTypedParameterAssign;
> 
> 
> +# viruri.h
> +virURIParse;
> +virURIFormat;

Swap these two lines.

> +xmlURIPtr
> +virURIParse(const char *uri)
> +{
> +xmlURIPtr ret = xmlParseURI(uri);
> +
> +/* First check: does it even make sense to jump inside */
> +if (ret != NULL &&
> +ret->server != NULL &&
> +ret->server[0] == '[') {
> +size_t length = strlen(ret->server);
> +
> +/* We want to modify the server string only if there are
> + * square brackets on both ends and inside there is IPv6
> + * address. Otherwise we could make a mistake by modifying
> + * something else than IPv6 address. */

s/else than/other than an/

> +unsigned char *
> +virURIFormat(xmlURIPtr uri)
> +{
> +char *tmpserver = NULL, *backupserver = uri->server;

NULL deref...

> +unsigned char *ret;
> +
> +/* First check: does it make sense to do anything */
> +if (uri != NULL &&

...since you allow uri == NULL on input.  Reorder the assignment to
backupserver to come after you know uri is not NULL.

> +uri->server != NULL &&
> +strchr(uri->server, ':') != NULL) {
> +
> +if (virAsprintf(&tmpserver, "[%s]", uri->server) == -1)

It's more idiomatic to use '< 0', not '== -1'.

> +++ b/src/util/viruri.h
> @@ -0,0 +1,18 @@
> +/*
> + * viruri.h: internal definitions used for URI parsing.

Needs a copyright header.

ACK with those nits fixed; I think we're close enough that you can push
without having to get a review on a v4.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v3] Fixed URI parsing

2012-02-24 Thread Martin Kletzander 
Function xmlParseURI does not remove square brackets around IPv6
address when parsing. One of the solutions is making wrappers around
functions working with xmlURI*. This assures that uri->server will be
always properly assigned and it doesn't have to be changed when used
on some new place in the code.
For this purpose, functions virParseURI and virSaveURI were
added. These function are wrappers around xmlParseURI and xmlSaveUri
respectively.
Also there is one new syntax check function to prohibit these functions
anywhere else.

File changes:
 - src/util/viruri.h-- declaration
 - src/util/viruri.c-- definition
 - src/libvirt_private.syms -- symbol export
 - src/Makefile.am  -- added source and header files
 - cfg.mk   -- added sc_prohibit_xmlURI
 - all others   -- ID name and include fixes
---
v3:
 - wrappers moved to new files
 - syntax check added
 - virAsprintf used instead of nasty memory mechanics

v2:
 - added virSaveURI for building back the original string

 cfg.mk |8 
 src/Makefile.am|3 +-
 src/datatypes.h|2 +-
 src/driver.h   |2 +-
 src/esx/esx_driver.c   |5 +-
 src/esx/esx_util.c |2 +-
 src/esx/esx_util.h |4 +-
 src/hyperv/hyperv_util.c   |2 +-
 src/hyperv/hyperv_util.h   |5 +-
 src/libvirt.c  |   10 ++--
 src/libvirt_private.syms   |5 ++
 src/libxl/libxl_driver.c   |3 +-
 src/lxc/lxc_driver.c   |3 +-
 src/openvz/openvz_driver.c |3 +-
 src/qemu/qemu_driver.c |2 +-
 src/qemu/qemu_migration.c  |7 ++-
 src/remote/remote_driver.c |9 ++--
 src/uml/uml_driver.c   |3 +-
 src/util/qparams.c |3 +-
 src/util/viruri.c  |   91 
 src/util/viruri.h  |   18 +
 src/vbox/vbox_tmpl.c   |3 +-
 src/vmx/vmx.c  |6 +-
 src/xen/xen_driver.c   |4 +-
 src/xen/xen_hypervisor.h   |3 +-
 src/xen/xend_internal.c|4 +-
 src/xen/xend_internal.h|2 +-
 src/xenapi/xenapi_driver.c |2 +-
 src/xenapi/xenapi_utils.c  |4 +-
 src/xenapi/xenapi_utils.h  |4 +-
 30 files changed, 174 insertions(+), 48 deletions(-)
 create mode 100644 src/util/viruri.c
 create mode 100644 src/util/viruri.h

diff --git a/cfg.mk b/cfg.mk
index dcf44bb..9759d87 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -457,6 +457,12 @@ sc_prohibit_xmlGetProp:
halt='use virXMLPropString, not xmlGetProp' \
  $(_sc_search_regexp)

+# xml(ParseURI|SaveUri) doesn't handle IPv6 URIs well
+sc_prohibit_xmlURI:
+   @prohibit='\
-# include 

 # include "internal.h"
+# include "viruri.h"
 /*
  * List of registered drivers numbers
  */
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index f5e1cc7..b6b22f8 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -44,6 +44,7 @@
 #include "esx_vi.h"
 #include "esx_vi_methods.h"
 #include "esx_util.h"
+#include "viruri.h"

 #define VIR_FROM_THIS VIR_FROM_ESX

@@ -3945,7 +3946,7 @@ esxDomainMigratePerform(virDomainPtr domain,
 {
 int result = -1;
 esxPrivate *priv = domain->conn->privateData;
-xmlURIPtr parsedUri = NULL;
+virURIPtr parsedUri = NULL;
 char *saveptr;
 char *path_resourcePool;
 char *path_hostSystem;
@@ -3976,7 +3977,7 @@ esxDomainMigratePerform(virDomainPtr domain,
 }

 /* Parse migration URI */
-parsedUri = xmlParseURI(uri);
+parsedUri = virURIParse(uri);

 if (parsedUri == NULL) {
 virReportOOMError();
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c
index 2c5ac1a..7d4b908 100644
--- a/src/esx/esx_util.c
+++ b/src/esx/esx_util.c
@@ -42,7 +42,7 @@


 int
-esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, xmlURIPtr uri)
+esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri)
 {
 int result = -1;
 struct qparam_set *queryParamSet = NULL;
diff --git a/src/esx/esx_util.h b/src/esx/esx_util.h
index 2bee510..a69b3f4 100644
--- a/src/esx/esx_util.h
+++ b/src/esx/esx_util.h
@@ -22,9 +22,9 @@
 #ifndef __ESX_UTIL_H__
 # define __ESX_UTIL_H__

-# include 
 # include 
 # include "internal.h"
+# include "viruri.h"

 typedef struct _esxUtil_ParsedUri esxUtil_ParsedUri;

@@ -40,7 +40,7 @@ struct _esxUtil_ParsedUri {
 char *path;
 };

-int esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, xmlURIPtr uri);
+int esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri);

 void esxUtil_FreeParsedUri(esxUtil_ParsedUri **parsedUri);

diff --git a/src/hyperv/hyperv_util.c b/src/hyperv/hyperv_util.c
index 298cfe0..2e6a2d4 100644
--- a/src/hyperv/hyperv_util.c
+++ b/src/hyperv/hyperv_util.c
@@ -37,7 +37,7 @@


 int
-hypervParseUri(hypervParsedUri **parsedUri, xmlURIPtr uri)
+hypervParseUri(hypervParsedUri **parsedUri, virURIPtr uri)
 {
 int result = -1;
 struct qparam_set *queryParamSet = NULL;
diff --git a/src/hyperv/hyperv_util.h b/src/hyperv/hyperv_ut