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

2012-02-24 Thread Eric Blake 
On 02/24/2012 06:30 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.
> 

> + */
> +unsigned char *
> +virSaveURI(xmlURIPtr uri)
> +{
> +char *tmpserver, *backupserver = uri->server;
> +unsigned char *ret;
> +bool fixback = false;

Instead of using bool fixback, I'd set tmpserver as NULL here...

> +
> +/* First check: does it make sense to do anything */
> +if (uri != NULL &&
> +uri->server != NULL &&
> +strchr(uri->server, ':') != NULL) {
> +
> +/* To be sure we have enough space for the brackets, we save
> + * the old string and then alocate a new one */

s/alocate/allocate/ - but see below, as I don't think you need this comment.

> +
> + /* the "+ 2" is room for square brackets and + 1 for '\0' */
> +size_t length = strlen(uri->server) + 3;
> +
> +if(VIR_ALLOC_N(tmpserver, length) < 0) {
> +virReportOOMError();

No need to raise OOM error here - all callers of xmlSaveUri were already
raising their own OOM after a NULL return.

> +return NULL;
> +}
> +
> +snprintf(tmpserver, length, "[%s]", uri->server);

I'd just use virAsnprintf(&tmpserver, "[%s]", uri->server); none of this
need to call strlen, VIR_ALLOC_N, or snprintf.

> +
> +uri->server = tmpserver;
> +bool fixback = true;
> +}
> +
> +ret = xmlSaveUri(uri);
> +
> +/* Put the fixed version back */
> +if (fixback) {

...and check 'if (tmpserver)' here (that is, fixback is redundant with
whether tmpserver was assigned at this point).

> +uri->server = backupserver;
> +VIR_FREE(tmpserver);
> +}
> +
> +return ret;
> +}
> 

I'm also a bit worried that we might regress if we don't add a syntax
check; can you look at adding a rule to cfg.mk that poisons xmlParseURI
and xmlSaveUri (wow, libxml2 really does have the awful URI vs. Uri in
its naming conventions), then exempt util/uri.c from the check as the
only file allowed to use them.

-- 
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

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

2012-02-24 Thread Daniel P. Berrange 
On Fri, Feb 24, 2012 at 02:30:11PM +0100, 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.


> diff --git a/src/util/xml.h b/src/util/xml.h
> index a3750fa..4835900 100644
> --- a/src/util/xml.h
> +++ b/src/util/xml.h
> @@ -10,6 +10,7 @@
>  # include 
>  # include 
>  # include 
> +# include 
> 
>  int  virXPathBoolean(const char *xpath,
>   xmlXPathContextPtr ctxt);
> @@ -61,6 +62,10 @@ xmlDocPtr  virXMLParseHelper(int domcode,
>   const char *url,
>   xmlXPathContextPtr *pctxt);
> 
> +xmlURIPtrvirParseURI(const char *uri);
> +
> +unsigned char *   virSaveURI(xmlURIPtr uri);
> +

Can you create new files for these  'util/viruri.{h,c}' and change
to ensure a standard 'virURI' naming prefix. Also we tend to use
the pair  Parse/Format, rather than Parse/Save in libvirt code.

So eg create a file viruri.h containing:

   typedef virURIPtr xmlURIPtr;

   virURIPtr  virURIParse(const char *uri);
   char * virURIFormat(virURIPtr uri);

Regards,
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

[libvirt] [PATCH v2] 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.

File changes:
 - src/util/xml.h   -- declaration
 - src/util/xml.c   -- definition
 - src/libvirt_private.syms -- symbol export
 - all others   -- function call fixed and include added
---
v2:
 - added virSaveURI for building back the original string

 src/esx/esx_driver.c   |3 +-
 src/libvirt.c  |7 ++-
 src/libvirt_private.syms   |2 +
 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  |5 +-
 src/remote/remote_driver.c |5 +-
 src/uml/uml_driver.c   |3 +-
 src/util/xml.c |   94 
 src/util/xml.h |5 ++
 src/vbox/vbox_tmpl.c   |3 +-
 src/vmx/vmx.c  |3 +-
 src/xen/xen_driver.c   |2 +-
 src/xen/xend_internal.c|3 +-
 16 files changed, 129 insertions(+), 17 deletions(-)

diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index f5e1cc7..4375432 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 "xml.h"

 #define VIR_FROM_THIS VIR_FROM_ESX

@@ -3976,7 +3977,7 @@ esxDomainMigratePerform(virDomainPtr domain,
 }

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

 if (parsedUri == NULL) {
 virReportOOMError();
diff --git a/src/libvirt.c b/src/libvirt.c
index 6294196..465d0aa 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -44,6 +44,7 @@
 #include "command.h"
 #include "virnodesuspend.h"
 #include "virrandom.h"
+#include "xml.h"

 #ifndef WITH_DRIVER_MODULES
 # ifdef WITH_TEST
@@ -1127,7 +1128,7 @@ do_open (const char *name,
 virConnectOpenResolveURIAlias(name, &alias) < 0)
 goto failed;

-ret->uri = xmlParseURI (alias ? alias : name);
+ret->uri = virParseURI (alias ? alias : name);
 if (!ret->uri) {
 virLibConnError(VIR_ERR_INVALID_ARG,
 _("could not parse connection URI %s"),
@@ -1729,7 +1730,7 @@ virConnectGetURI (virConnectPtr conn)
 return NULL;
 }

-name = (char *)xmlSaveUri(conn->uri);
+name = (char *)virSaveURI(conn->uri);
 if (!name) {
 virReportOOMError();
 goto error;
@@ -4964,7 +4965,7 @@ virDomainMigratePeer2Peer (virDomainPtr domain,
 return -1;
 }

-tempuri = xmlParseURI(dconnuri);
+tempuri = virParseURI(dconnuri);
 if (!tempuri) {
 virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
 virDispatchError(domain->conn);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9e3573f..595020a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1431,6 +1431,8 @@ virTypedParameterAssign;


 # xml.h
+virParseURI;
+virSaveURI;
 virXMLChildElementCount;
 virXMLParseHelper;
 virXMLPropString;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 6cfc5eb..f1ef5fb 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -44,6 +44,7 @@
 #include "libxl_conf.h"
 #include "xen_xm.h"
 #include "virtypedparam.h"
+#include "xml.h"

 #define VIR_FROM_THIS VIR_FROM_LIBXL

@@ -1043,7 +1044,7 @@ libxlOpen(virConnectPtr conn,
 if (libxl_driver == NULL)
 return VIR_DRV_OPEN_DECLINED;

-conn->uri = xmlParseURI("xen:///");
+conn->uri = virParseURI("xen:///");
 if (!conn->uri) {
 virReportOOMError();
 return VIR_DRV_OPEN_ERROR;
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index b6962cf7..3d25d5e 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -60,6 +60,7 @@
 #include "virnodesuspend.h"
 #include "virtime.h"
 #include "virtypedparam.h"
+#include "xml.h"

 #define VIR_FROM_THIS VIR_FROM_LXC

@@ -139,7 +140,7 @@ static virDrvOpenStatus lxcOpen(virConnectPtr conn,
 if (lxc_driver == NULL)
 return VIR_DRV_OPEN_DECLINED;

-conn->uri = xmlParseURI("lxc:///");
+conn->uri = virParseURI("lxc:///");
 if (!conn->uri) {
 virReportOOMError();
 return VIR_DRV_OPEN_ERROR;
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 833a98d..47fad74 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -56,6 +56,7 @@
 #include "virfile.h"
 #include "logging.h"
 #include "command.h"
+#include "