[libvirt] [PATCH] Fixed URI parsing
Function virParseURI was added. This function is wrapper around xmlParseURI. In this wrapper we are fixing what doesn't seems to be fixed in libxml2 in the near future. The wrapper is written in such way that if anything gets fixed in libxml2, it'll still work. 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 --- src/esx/esx_driver.c | 3 ++- src/libvirt.c | 10 ++++++---- src/libvirt_private.syms | 1 + 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 | 3 ++- src/uml/uml_driver.c | 3 ++- src/util/xml.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/xml.h | 3 +++ 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, 70 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 8035add..eeca680 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 @@ -1117,8 +1118,9 @@ do_open (const char *name, if (STRCASEEQ(name, "xen")) name = "xen:///"; - /* Convert xen:// -> xen:/// because xmlParseURI cannot parse the - * former. This allows URIs such as xen://localhost to work. + /* Convert xen:// -> xen:/// because xmlParseURI (and thus virParseURI + * as well) cannot parse the former. This allows URIs such as + * xen://localhost to work. */ if (STREQ (name, "xen://")) name = "xen:///"; @@ -1127,7 +1129,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"), @@ -4964,7 +4966,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 a5f9433..7975f94 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1425,6 +1425,7 @@ virTypedParameterAssign; # xml.h +virParseURI; 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 b712da4..9c3c005 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -59,6 +59,7 @@ #include "virnodesuspend.h" #include "virtime.h" #include "virtypedparam.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -138,7 +139,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 "xml.h" #define VIR_FROM_THIS VIR_FROM_OPENVZ @@ -1335,7 +1336,7 @@ static virDrvOpenStatus openvzOpen(virConnectPtr conn, if (access("/proc/vz", W_OK) < 0) return VIR_DRV_OPEN_DECLINED; - conn->uri = xmlParseURI("openvz:///system"); + conn->uri = virParseURI("openvz:///system"); if (conn->uri == NULL) { virReportOOMError(); return VIR_DRV_OPEN_ERROR; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 45c4100..5ff1313 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -857,7 +857,7 @@ static virDrvOpenStatus qemudOpen(virConnectPtr conn, if (qemu_driver == NULL) return VIR_DRV_OPEN_DECLINED; - conn->uri = xmlParseURI(qemu_driver->privileged ? + conn->uri = virParseURI(qemu_driver->privileged ? "qemu:///system" : "qemu:///session"); if (!conn->uri) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 12cfbde..35a2ebc 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -45,6 +45,7 @@ #include "virtime.h" #include "locking/domain_lock.h" #include "rpc/virnetsocket.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -1773,10 +1774,10 @@ static int doNativeMigrate(struct qemud_driver *driver, virReportOOMError(); return -1; } - uribits = xmlParseURI(tmp); + uribits = virParseURI(tmp); VIR_FREE(tmp); } else { - uribits = xmlParseURI(uri); + uribits = virParseURI(uri); } if (!uribits) { qemuReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index e068126..116cf12 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -47,6 +47,7 @@ #include "command.h" #include "intprops.h" #include "virtypedparam.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_REMOTE @@ -719,7 +720,7 @@ doRemoteOpen (virConnectPtr conn, goto failed; VIR_DEBUG("Auto-probed URI is %s", uriret.uri); - conn->uri = xmlParseURI(uriret.uri); + conn->uri = virParseURI(uriret.uri); VIR_FREE(uriret.uri); if (!conn->uri) { virReportOOMError(); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index a4cf945..f1e290a 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -63,6 +63,7 @@ #include "configmake.h" #include "virnetdevtap.h" #include "virnodesuspend.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_UML @@ -1138,7 +1139,7 @@ static virDrvOpenStatus umlOpen(virConnectPtr conn, if (uml_driver == NULL) return VIR_DRV_OPEN_DECLINED; - conn->uri = xmlParseURI(uml_driver->privileged ? + conn->uri = virParseURI(uml_driver->privileged ? "uml:///system" : "uml:///session"); if (!conn->uri) { diff --git a/src/util/xml.c b/src/util/xml.c index b426653..982ce9a 100644 --- a/src/util/xml.c +++ b/src/util/xml.c @@ -800,6 +800,43 @@ error: goto cleanup; } +/** + * virParseURI: + * @uri: URI to parse + * + * Wrapper for xmlParseURI + * + * Unfortunately there are few things that should be managed after + * parsing the URI. First example is removing the square brackets + * around IPv6 addresses. + * + * @returns the parsed uri object with some fixes + */ +xmlURIPtr +virParseURI(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 last = strlen(ret->server) - 1; /* Index of last char */ + + /* 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. */ + if (ret->server[last] == ']' && strchr(ret->server, ':')) { + for (size_t i = 0; i < last; i++) + ret->server[i] = ret->server[i + 1]; + + ret->server[last - 1] = '\0'; + } + /* Even after such modification, it is completely ok to free + * the uri with xmlFreeURI() */ + } + + return ret; +} static int virXMLEmitWarning(int fd, const char *name, diff --git a/src/util/xml.h b/src/util/xml.h index a3750fa..c0fbbca 100644 --- a/src/util/xml.h +++ b/src/util/xml.h @@ -10,6 +10,7 @@ # include <libxml/parser.h> # include <libxml/tree.h> # include <libxml/xpath.h> +# include <libxml/uri.h> int virXPathBoolean(const char *xpath, xmlXPathContextPtr ctxt); @@ -61,6 +62,8 @@ xmlDocPtr virXMLParseHelper(int domcode, const char *url, xmlXPathContextPtr *pctxt); +xmlURIPtr virParseURI(const char *uri); + /** * virXMLParse: * @filename: file to parse, or NULL for string parsing diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index b168c7d..11fa995 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -56,6 +56,7 @@ #include "configmake.h" #include "virfile.h" #include "fdstream.h" +#include "xml.h" /* This one changes from version to version. */ #if VBOX_API_VERSION == 2002 @@ -980,7 +981,7 @@ static virDrvOpenStatus vboxOpen(virConnectPtr conn, virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); if (conn->uri == NULL) { - conn->uri = xmlParseURI(uid ? "vbox:///session" : "vbox:///system"); + conn->uri = virParseURI(uid ? "vbox:///session" : "vbox:///system"); if (conn->uri == NULL) { virReportOOMError(); return VIR_DRV_OPEN_ERROR; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 1fdbd50..866b150 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -33,6 +33,7 @@ #include "logging.h" #include "uuid.h" #include "vmx.h" +#include "xml.h" /* @@ -2610,7 +2611,7 @@ virVMXParseSerial(virVMXContext *ctx, virConfPtr conf, int port, (*def)->target.port = port; (*def)->source.type = VIR_DOMAIN_CHR_TYPE_TCP; - parsedUri = xmlParseURI(fileName); + parsedUri = virParseURI(fileName); if (parsedUri == NULL) { virReportOOMError(); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 94cafde..c101e11 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -270,7 +270,7 @@ xenUnifiedOpen (virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) if (!xenUnifiedProbe()) 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/xen/xend_internal.c b/src/xen/xend_internal.c index 5c3838f..e346f5f 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -46,6 +46,7 @@ #include "memory.h" #include "count-one-bits.h" #include "virfile.h" +#include "xml.h" /* required for cpumap_t */ #include <xen/dom0_ops.h> @@ -3224,7 +3225,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, * "hostname", "hostname:port" or "xenmigr://hostname[:port]/". */ if (strstr (uri, "//")) { /* Full URI. */ - xmlURIPtr uriptr = xmlParseURI (uri); + xmlURIPtr uriptr = virParseURI (uri); if (!uriptr) { virXendError(VIR_ERR_INVALID_ARG, "%s", _("xenDaemonDomainMigrate: invalid URI")); -- 1.7.3.4
Function virParseURI was added. This function is wrapper around xmlParseURI. In this wrapper we are fixing what doesn't seems to be fixed in libxml2 in the near future. The wrapper is written in such way that if anything gets fixed in libxml2, it'll still work. The problem alluded to here is that when xmlParseURI encounters a numeric IPv6 address in brackets, it returns the entire string including
On 02/09/2012 09:43 AM, Martin Kletzander wrote: the brackets. getaddrxx(), by contrast, expects the brackets to not be there. And yet the brackets *must* be included to specify a numeric IP address, according to section 6 of RFC5952 (why am I ready with this information? Because I looked it up when commenting on a qemu bug last night :-) (https://bugzilla.redhat.com/show_bug.cgi?id=680356 if you're interested) I almost ACKed this patch (with one small change to replace the loop with memmove), but then thought about what happens if we need to reconstruct the URI from this modified xmlURIPtr (e.g., with xmlSaveUri, which libvirt calls in two places). Since we've modified the server string, calling xmlSaveUri will give us incorrect results - let's say you start off with: http://[2098:2da:335b::1]:123 The original xmlParseURI would return with server == "[2098:2da:335b::1]", and port == 123. If we called xmlSaveUri on that, we would get back the original string. But if we have modified the server string to remove the brackets, calling xmlSaveUri gives us: http://2098:2da:335b::1:123 Which is a completely different address. So, I think that the correct solution here, rather than permanently modifying the server string returned from xmlParseURI, is to look at the places where the server string is used for something other than just checking for != NULL (that leaves very few places) and modify a copy of the string to remove the brackets in those cases. This way you always have the exact original server string so that the original URI can be reconstructed. So, NACK. (note that I still made some comments on the code that modifies server, which you'll want to look at before making a helper function to return an unbracketed copy (or however you end up fixing it without modifying the original).
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 --- src/esx/esx_driver.c | 3 ++- src/libvirt.c | 10 ++++++---- src/libvirt_private.syms | 1 + 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 | 3 ++- src/uml/uml_driver.c | 3 ++- src/util/xml.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/xml.h | 3 +++ 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, 70 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 8035add..eeca680 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 @@ -1117,8 +1118,9 @@ do_open (const char *name, if (STRCASEEQ(name, "xen")) name = "xen:///";
- /* Convert xen:// -> xen:/// because xmlParseURI cannot parse the - * former. This allows URIs such as xen://localhost to work. + /* Convert xen:// -> xen:/// because xmlParseURI (and thus virParseURI + * as well) cannot parse the former. This allows URIs such as + * xen://localhost to work. */ if (STREQ (name, "xen://")) name = "xen:///"; @@ -1127,7 +1129,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"), @@ -4964,7 +4966,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 a5f9433..7975f94 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1425,6 +1425,7 @@ virTypedParameterAssign;
# xml.h +virParseURI; 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 b712da4..9c3c005 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -59,6 +59,7 @@ #include "virnodesuspend.h" #include "virtime.h" #include "virtypedparam.h" +#include "xml.h"
#define VIR_FROM_THIS VIR_FROM_LXC
@@ -138,7 +139,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 "xml.h"
#define VIR_FROM_THIS VIR_FROM_OPENVZ
@@ -1335,7 +1336,7 @@ static virDrvOpenStatus openvzOpen(virConnectPtr conn, if (access("/proc/vz", W_OK)< 0) return VIR_DRV_OPEN_DECLINED;
- conn->uri = xmlParseURI("openvz:///system"); + conn->uri = virParseURI("openvz:///system"); if (conn->uri == NULL) { virReportOOMError(); return VIR_DRV_OPEN_ERROR; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 45c4100..5ff1313 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -857,7 +857,7 @@ static virDrvOpenStatus qemudOpen(virConnectPtr conn, if (qemu_driver == NULL) return VIR_DRV_OPEN_DECLINED;
- conn->uri = xmlParseURI(qemu_driver->privileged ? + conn->uri = virParseURI(qemu_driver->privileged ? "qemu:///system" : "qemu:///session"); if (!conn->uri) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 12cfbde..35a2ebc 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -45,6 +45,7 @@ #include "virtime.h" #include "locking/domain_lock.h" #include "rpc/virnetsocket.h" +#include "xml.h"
#define VIR_FROM_THIS VIR_FROM_QEMU @@ -1773,10 +1774,10 @@ static int doNativeMigrate(struct qemud_driver *driver, virReportOOMError(); return -1; } - uribits = xmlParseURI(tmp); + uribits = virParseURI(tmp); VIR_FREE(tmp); } else { - uribits = xmlParseURI(uri); + uribits = virParseURI(uri); } if (!uribits) { qemuReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index e068126..116cf12 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -47,6 +47,7 @@ #include "command.h" #include "intprops.h" #include "virtypedparam.h" +#include "xml.h"
#define VIR_FROM_THIS VIR_FROM_REMOTE
@@ -719,7 +720,7 @@ doRemoteOpen (virConnectPtr conn, goto failed;
VIR_DEBUG("Auto-probed URI is %s", uriret.uri); - conn->uri = xmlParseURI(uriret.uri); + conn->uri = virParseURI(uriret.uri); VIR_FREE(uriret.uri); if (!conn->uri) { virReportOOMError(); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index a4cf945..f1e290a 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -63,6 +63,7 @@ #include "configmake.h" #include "virnetdevtap.h" #include "virnodesuspend.h" +#include "xml.h"
#define VIR_FROM_THIS VIR_FROM_UML
@@ -1138,7 +1139,7 @@ static virDrvOpenStatus umlOpen(virConnectPtr conn, if (uml_driver == NULL) return VIR_DRV_OPEN_DECLINED;
- conn->uri = xmlParseURI(uml_driver->privileged ? + conn->uri = virParseURI(uml_driver->privileged ? "uml:///system" : "uml:///session"); if (!conn->uri) { diff --git a/src/util/xml.c b/src/util/xml.c index b426653..982ce9a 100644 --- a/src/util/xml.c +++ b/src/util/xml.c @@ -800,6 +800,43 @@ error: goto cleanup; }
+/** + * virParseURI: + * @uri: URI to parse + * + * Wrapper for xmlParseURI + * + * Unfortunately there are few things that should be managed after + * parsing the URI. First example is removing the square brackets + * around IPv6 addresses. + * + * @returns the parsed uri object with some fixes + */ +xmlURIPtr +virParseURI(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 last = strlen(ret->server) - 1; /* Index of last char */ + + /* 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. */ + if (ret->server[last] == ']'&& strchr(ret->server, ':')) {
I've never found a document that said [domainname]:port was legal, so I guess it's okay to check for at least one ":" in the string. I would have probably left that out though, as I'm fairly certain that "[" and "]" still aren't allowed in domain names (even after the expansion to support UTF characters).
+ for (size_t i = 0; i< last; i++) + ret->server[i] = ret->server[i + 1];
(this actually copies one character too many, but that's harmless). Just replace this with: memmove(ret->server, ret->server+1, last-1)
+ + ret->server[last - 1] = '\0'; + } + /* Even after such modification, it is completely ok to free + * the uri with xmlFreeURI() */ + } + + return ret; +}
static int virXMLEmitWarning(int fd, const char *name, diff --git a/src/util/xml.h b/src/util/xml.h index a3750fa..c0fbbca 100644 --- a/src/util/xml.h +++ b/src/util/xml.h @@ -10,6 +10,7 @@ # include<libxml/parser.h> # include<libxml/tree.h> # include<libxml/xpath.h> +# include<libxml/uri.h>
int virXPathBoolean(const char *xpath, xmlXPathContextPtr ctxt); @@ -61,6 +62,8 @@ xmlDocPtr virXMLParseHelper(int domcode, const char *url, xmlXPathContextPtr *pctxt);
+xmlURIPtr virParseURI(const char *uri); + /** * virXMLParse: * @filename: file to parse, or NULL for string parsing diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index b168c7d..11fa995 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -56,6 +56,7 @@ #include "configmake.h" #include "virfile.h" #include "fdstream.h" +#include "xml.h"
/* This one changes from version to version. */ #if VBOX_API_VERSION == 2002 @@ -980,7 +981,7 @@ static virDrvOpenStatus vboxOpen(virConnectPtr conn, virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
if (conn->uri == NULL) { - conn->uri = xmlParseURI(uid ? "vbox:///session" : "vbox:///system"); + conn->uri = virParseURI(uid ? "vbox:///session" : "vbox:///system"); if (conn->uri == NULL) { virReportOOMError(); return VIR_DRV_OPEN_ERROR; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 1fdbd50..866b150 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -33,6 +33,7 @@ #include "logging.h" #include "uuid.h" #include "vmx.h" +#include "xml.h"
/*
@@ -2610,7 +2611,7 @@ virVMXParseSerial(virVMXContext *ctx, virConfPtr conf, int port, (*def)->target.port = port; (*def)->source.type = VIR_DOMAIN_CHR_TYPE_TCP;
- parsedUri = xmlParseURI(fileName); + parsedUri = virParseURI(fileName);
if (parsedUri == NULL) { virReportOOMError(); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 94cafde..c101e11 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -270,7 +270,7 @@ xenUnifiedOpen (virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) if (!xenUnifiedProbe()) 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/xen/xend_internal.c b/src/xen/xend_internal.c index 5c3838f..e346f5f 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -46,6 +46,7 @@ #include "memory.h" #include "count-one-bits.h" #include "virfile.h" +#include "xml.h"
/* required for cpumap_t */ #include<xen/dom0_ops.h> @@ -3224,7 +3225,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, * "hostname", "hostname:port" or "xenmigr://hostname[:port]/". */ if (strstr (uri, "//")) { /* Full URI. */ - xmlURIPtr uriptr = xmlParseURI (uri); + xmlURIPtr uriptr = virParseURI (uri); if (!uriptr) { virXendError(VIR_ERR_INVALID_ARG, "%s", _("xenDaemonDomainMigrate: invalid URI")); -- 1.7.3.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/09/2012 04:38 PM, Laine Stump wrote:
Function virParseURI was added. This function is wrapper around xmlParseURI. In this wrapper we are fixing what doesn't seems to be fixed in libxml2 in the near future. The wrapper is written in such way that if anything gets fixed in libxml2, it'll still work. The problem alluded to here is that when xmlParseURI encounters a numeric IPv6 address in brackets, it returns the entire string including
On 02/09/2012 09:43 AM, Martin Kletzander wrote: the brackets. getaddrxx(), by contrast, expects the brackets to not be there. And yet the brackets *must* be included to specify a numeric IP address, according to section 6 of RFC5952 (why am I ready with this information? Because I looked it up when commenting on a qemu bug last night :-)
(https://bugzilla.redhat.com/show_bug.cgi?id=680356 if you're interested)
I'll definitely check this out, thanks for the info.
I almost ACKed this patch (with one small change to replace the loop with memmove), but then thought about what happens if we need to reconstruct the URI from this modified xmlURIPtr (e.g., with xmlSaveUri, which libvirt calls in two places).
Jiri told me about this, it's just my fault that I've forgot to modify that part as well. What is your opinion for me setting up one more function (virSaveURI let's say) that constructs the string back as it is supposed to (again just a wrapper for xmlSaveURI)?
So, I think that the correct solution here, rather than permanently modifying the server string returned from xmlParseURI, is to look at the places where the server string is used for something other than just checking for != NULL (that leaves very few places) and modify a copy of the string to remove the brackets in those cases. This way you always have the exact original server string so that the original URI can be reconstructed.
That was the first thing I wanted to change but unfortunately I did that only for ssh so it was unusable.
+ for (size_t i = 0; i< last; i++) + ret->server[i] = ret->server[i + 1];
(this actually copies one character too many, but that's harmless). Just replace this with:
memmove(ret->server, ret->server+1, last-1)
I know about memmove(), it's just that in this case I really didn't like that it copes the string somewhere else and than again back (in case of overlapping) but no problem to change this :-)
On 02/09/2012 11:48 AM, Martin Kletzander wrote:
On 02/09/2012 04:38 PM, Laine Stump wrote:
Function virParseURI was added. This function is wrapper around xmlParseURI. In this wrapper we are fixing what doesn't seems to be fixed in libxml2 in the near future. The wrapper is written in such way that if anything gets fixed in libxml2, it'll still work. The problem alluded to here is that when xmlParseURI encounters a numeric IPv6 address in brackets, it returns the entire string including
On 02/09/2012 09:43 AM, Martin Kletzander wrote: the brackets. getaddrxx(), by contrast, expects the brackets to not be there. And yet the brackets *must* be included to specify a numeric IP address, according to section 6 of RFC5952 (why am I ready with this information? Because I looked it up when commenting on a qemu bug last night :-)
(https://bugzilla.redhat.com/show_bug.cgi?id=680356 if you're interested) I'll definitely check this out, thanks for the info.
I almost ACKed this patch (with one small change to replace the loop with memmove), but then thought about what happens if we need to reconstruct the URI from this modified xmlURIPtr (e.g., with xmlSaveUri, which libvirt calls in two places). Jiri told me about this, it's just my fault that I've forgot to modify that part as well. What is your opinion for me setting up one more function (virSaveURI let's say) that constructs the string back as it is supposed to (again just a wrapper for xmlSaveURI)?
The problems I can see with that: 1) That would require you to once again modify the server string in-place, but you can't rely on it still having the 2 extra bytes, so you would have to create a new string. 2) If somebody decided in the future to use different libxml2 API on the URI (e.g. xmlPrintURI), or wrote their own code to do something with the string, they would have to know to provide a wrapper for that as well. 3) Just the oddness of determining whether or not to add brackets back based on whether or not you find a ":" in the string. On the other hand, if you leave the brackets in, then everybody who uses uri->server directly needs to know that it may have extraneous brackets around it, so I'm undecided. Maybe someone else has an opinion that will take it over the edge.
So, I think that the correct solution here, rather than permanently modifying the server string returned from xmlParseURI, is to look at the places where the server string is used for something other than just checking for != NULL (that leaves very few places) and modify a copy of the string to remove the brackets in those cases. This way you always have the exact original server string so that the original URI can be reconstructed.
That was the first thing I wanted to change but unfortunately I did that only for ssh so it was unusable.
You can just do a cscope search for "server", then look within those results for the string "->server".
+ for (size_t i = 0; i< last; i++) + ret->server[i] = ret->server[i + 1]; (this actually copies one character too many, but that's harmless). Just replace this with:
memmove(ret->server, ret->server+1, last-1) I know about memmove(), it's just that in this case I really didn't like that it copes the string somewhere else and than again back (in case of overlapping) but no problem to change this :-)
I would be surprised if gcc's memmove was that braindead. Especially when moving memory to a lower address, they can still use the same rep movs (or whatever) and it goes in the correct direction already - don't even have to reverse it. The manpage doesn't say that the bytes *are* copied into a temporary array and back, it just says "copying takes place *as if* the bytes are first copied into a temporary array".
On 02/09/2012 10:53 AM, Laine Stump wrote:
memmove(ret->server, ret->server+1, last-1)
I know about memmove(), it's just that in this case I really didn't like that it copes the string somewhere else and than again back (in case of overlapping) but no problem to change this :-)
I would be surprised if gcc's memmove was that braindead. Especially when moving memory to a lower address, they can still use the same rep movs (or whatever) and it goes in the correct direction already - don't even have to reverse it. The manpage doesn't say that the bytes *are* copied into a temporary array and back, it just says "copying takes place *as if* the bytes are first copied into a temporary array".
Agreed. Any libc worth their salt implements memmove by first checking for overlap, and if there is overlap, then choosing to start the move from the direction that won't be self-destructive, without using any secondary bank; and furthermore libc will optimize things into word-size chunks for faster than byte-wise motion. Furthermore, while some hardware is tuned only for forward copies, my recollection is that x86 has a rich enough instruction set to iterate the copy in either direction with equal speed. You're almost always better off calling into libc than doing a hand-rolled implementation, except in one case: POSIX currently says that memmove, memcpy, strstr, and other functions in <string.h> are not async-signal-safe. If you are paranoid, you cannot call these functions, but can instead call a hand-rolled implementation, from inside a signal handler or between fork and exec. Or, if you are like me, you will assume that this is a bug in POSIX, and that it was oversight in the standard for omitting functions that are obviously implementable in a signal-safe manner. (Actually, I think there may be a real reason why they are not listed as signal-safe - if there is hardware like x86 that has shortcut instructions like rep movs, but where the shortcut cannot be resumed if it gets interrupted midstream; but I've never seen a web site stating that such an awful hardware design exists in practice). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/09/2012 06:53 PM, Laine Stump wrote:
The problems I can see with that:
1) That would require you to once again modify the server string in-place, but you can't rely on it still having the 2 extra bytes, so you would have to create a new string.
Modifying the server is not a problem. If you allocate new string it is freed properly with xmlFreeURI(). I know it's a little bit clumsy but it works. Also the modification is needed only for calling the function once.
2) If somebody decided in the future to use different libxml2 API on the URI (e.g. xmlPrintURI), or wrote their own code to do something with the string, they would have to know to provide a wrapper for that as well.
Yes, this is a little problem at first. However, on the other hand, if somebody wants to use the uri->server fro anything, they would have to know to use the function returning proper hostname/ip.
3) Just the oddness of determining whether or not to add brackets back based on whether or not you find a ":" in the string.
This seems very reasonable to me. The brackets are there just for the purpose of determining what is the IPv6 address and what's the port. It is exactly the reason why it is needed :)
On the other hand, if you leave the brackets in, then everybody who uses uri->server directly needs to know that it may have extraneous brackets around it, so I'm undecided. Maybe someone else has an opinion that will take it over the edge.
There (maybe) is one more way how to do this. I'm saying "maybe" because while handling all the cons we discussed, it is also the worst option to deal with such a problem, I guess. We can create new structure (virURI with virURIPtr etc.) that encapsulates xmlURI and also has it's own members that would help determining what to do in what function. There would be one more layer of functions accessing this structure and there could even be a pointer to xmlURIPtr that one can use for unimplemented functions around the new structure. But as I say, I think this is the worst option considering the real problem is with "brackets around IPv6 in libxml2" and fixing it there would save us from dealing with lots of bugs.
On 02/13/2012 10:57 AM, Martin Kletzander wrote:
But as I say, I think this is the worst option considering the real problem is with "brackets around IPv6 in libxml2" and fixing it there would save us from dealing with lots of bugs.
Let's suppose bug #624626 [1] will be fixed soon. Is there a need to support older libxml2 as well (those versions without the fix) in this case? Martin [1] https://bugzilla.redhat.com/show_bug.cgi?id=624626
On Wed, Feb 15, 2012 at 03:05:34PM +0100, Martin Kletzander wrote:
On 02/13/2012 10:57 AM, Martin Kletzander wrote:
But as I say, I think this is the worst option considering the real problem is with "brackets around IPv6 in libxml2" and fixing it there would save us from dealing with lots of bugs.
Let's suppose bug #624626 [1] will be fixed soon. Is there a need to support older libxml2 as well (those versions without the fix) in this case?
Martin
Hum .... I completely missed this, I'm not sure I understand comment 2: paphio:~/XML -> testURI --debug 'qemu+ssh://[3ffe::102]/system' scheme: qemu+ssh server: [3ffe::102] path: /system qemu+ssh://[3ffe::102]/system paphio:~/XML -> Seems to me that libxml2 URI parsing works as expected if I look at http://www.ietf.org/rfc/rfc3986.txt which is the level of the URI spec that libxml2 implements: authority = [ userinfo "@" ] host [ ":" port ] host = IP-literal / IPv4address / reg-name IP-literal = "[" ( IPv6address / IPvFuture ) "]" IPv6address = 6( h16 ":" ) ls32 / "::" 5( h16 ":" ) ls32 / [ h16 ] "::" 4( h16 ":" ) ls32 / [ *1( h16 ":" ) h16 ] "::" 3( h16 ":" ) ls32 / [ *2( h16 ":" ) h16 ] "::" 2( h16 ":" ) ls32 / [ *3( h16 ":" ) h16 ] "::" h16 ":" ls32 / [ *4( h16 ":" ) h16 ] "::" ls32 / [ *5( h16 ":" ) h16 ] "::" h16 / [ *6( h16 ":" ) h16 ] "::" ls32 = ( h16 ":" h16 ) / IPv4address ; least-significant 32 bits of address h16 = 1*4HEXDIG ; 16 bits of address represented in hexadecimal so it seems to me that the "[" and "]" are part of the host address per the RFC spec and well libxml2 parsing is adequate. I'm not sure I understand the parsing problem, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 02/15/2012 08:01 AM, Daniel Veillard wrote:
I completely missed this, I'm not sure I understand comment 2:
paphio:~/XML -> testURI --debug 'qemu+ssh://[3ffe::102]/system' scheme: qemu+ssh server: [3ffe::102] path: /system qemu+ssh://[3ffe::102]/system paphio:~/XML ->
Seems to me that libxml2 URI parsing works as expected if I look at http://www.ietf.org/rfc/rfc3986.txt which is the level of the URI spec that libxml2 implements:
so it seems to me that the "[" and "]" are part of the host address per the RFC spec and well libxml2 parsing is adequate. I'm not sure I understand the parsing problem,
It boils down to an ease-of-use question. Within a URI, I agree with you that IPv6 addresses in the 'host' element of 'authority' have a mandatory []. Outside of a URI, IPv6 addresses must not have [] for use in any other libc functions. So either all clients of libxml2 must strip [] when reading the server: field of the broken-down struct, and reinstate [] before asking libxml to reconstruct a valid URI, or libxml2 could be taught to provide some convenient hooks: server: remains as is server_cooked: new field, which matches server for IPv4 and hostnames, and which for IPv6 strips the [] and when constructing a URI from user-provided elements, automatically add in the needed [] around IPv6 addresses (which are otherwise invalid as they are not IPv4 nor hostnames) to create a valid URI without the user having to track []. However, since any ease-of-use improvements to libxml2 will only show up in new libxml2 versions, and we must still interoperate with old versions, I think this patch is on the right track of abstracting the fact that libvirt must manage [] (strip it on parse, re-add it on format) rather than relying on libxml2. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 "xml.h" #define VIR_FROM_THIS VIR_FROM_OPENVZ @@ -1335,7 +1336,7 @@ static virDrvOpenStatus openvzOpen(virConnectPtr conn, if (access("/proc/vz", W_OK) < 0) return VIR_DRV_OPEN_DECLINED; - conn->uri = xmlParseURI("openvz:///system"); + conn->uri = virParseURI("openvz:///system"); if (conn->uri == NULL) { virReportOOMError(); return VIR_DRV_OPEN_ERROR; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 717bdf1..3dcf461 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -857,7 +857,7 @@ static virDrvOpenStatus qemudOpen(virConnectPtr conn, if (qemu_driver == NULL) return VIR_DRV_OPEN_DECLINED; - conn->uri = xmlParseURI(qemu_driver->privileged ? + conn->uri = virParseURI(qemu_driver->privileged ? "qemu:///system" : "qemu:///session"); if (!conn->uri) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f0af494..d338066 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -45,6 +45,7 @@ #include "virtime.h" #include "locking/domain_lock.h" #include "rpc/virnetsocket.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -1775,10 +1776,10 @@ static int doNativeMigrate(struct qemud_driver *driver, virReportOOMError(); return -1; } - uribits = xmlParseURI(tmp); + uribits = virParseURI(tmp); VIR_FREE(tmp); } else { - uribits = xmlParseURI(uri); + uribits = virParseURI(uri); } if (!uribits) { qemuReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 2dacb70..96f5f8c 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -47,6 +47,7 @@ #include "command.h" #include "intprops.h" #include "virtypedparam.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_REMOTE @@ -505,7 +506,7 @@ doRemoteOpen (virConnectPtr conn, transport_str[-1] = '\0'; } - name = (char *) xmlSaveUri (&tmpuri); + name = (char *) virSaveURI (&tmpuri); #ifdef HAVE_XMLURI_QUERY_RAW VIR_FREE(tmpuri.query_raw); @@ -719,7 +720,7 @@ doRemoteOpen (virConnectPtr conn, goto failed; VIR_DEBUG("Auto-probed URI is %s", uriret.uri); - conn->uri = xmlParseURI(uriret.uri); + conn->uri = virParseURI(uriret.uri); VIR_FREE(uriret.uri); if (!conn->uri) { virReportOOMError(); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index a4cf945..f1e290a 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -63,6 +63,7 @@ #include "configmake.h" #include "virnetdevtap.h" #include "virnodesuspend.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_UML @@ -1138,7 +1139,7 @@ static virDrvOpenStatus umlOpen(virConnectPtr conn, if (uml_driver == NULL) return VIR_DRV_OPEN_DECLINED; - conn->uri = xmlParseURI(uml_driver->privileged ? + conn->uri = virParseURI(uml_driver->privileged ? "uml:///system" : "uml:///session"); if (!conn->uri) { diff --git a/src/util/xml.c b/src/util/xml.c index b426653..36f3a7f 100644 --- a/src/util/xml.c +++ b/src/util/xml.c @@ -800,6 +800,100 @@ error: goto cleanup; } +/************************************************************************ + * * + * Wrappers around libxml2 URI specific functions * + * * + ************************************************************************/ + +/** + * virParseURI: + * @uri: URI to parse + * + * Wrapper for xmlParseURI + * + * Unfortunately there are few things that should be managed after + * parsing the URI. Fortunately there is only one thing now and its + * removing of square brackets around IPv6 addresses. + * + * @returns the parsed uri object with some fixes + */ +xmlURIPtr +virParseURI(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. */ + if (ret->server[length - 1] == ']' && strchr(ret->server, ':')) { + memmove(&ret->server[0], &ret->server[1], length - 2); + ret->server[length - 2] = '\0'; + } + /* Even after such modification, it is completely ok to free + * the uri with xmlFreeURI() */ + } + + return ret; +} + +/** + * virSaveURI: + * @uri: URI to save + * + * Wrapper for xmlSaveUri + * + * This function constructs back everything that @ref virParseURI + * changes after parsing + * + * @returns the constructed uri as a string + */ +unsigned char * +virSaveURI(xmlURIPtr uri) +{ + char *tmpserver, *backupserver = uri->server; + unsigned char *ret; + bool fixback = false; + + /* 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 */ + + /* 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(); + return NULL; + } + + snprintf(tmpserver, length, "[%s]", uri->server); + + uri->server = tmpserver; + bool fixback = true; + } + + ret = xmlSaveUri(uri); + + /* Put the fixed version back */ + if (fixback) { + uri->server = backupserver; + VIR_FREE(tmpserver); + } + + return ret; +} static int virXMLEmitWarning(int fd, const char *name, 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 <libxml/parser.h> # include <libxml/tree.h> # include <libxml/xpath.h> +# include <libxml/uri.h> int virXPathBoolean(const char *xpath, xmlXPathContextPtr ctxt); @@ -61,6 +62,10 @@ xmlDocPtr virXMLParseHelper(int domcode, const char *url, xmlXPathContextPtr *pctxt); +xmlURIPtr virParseURI(const char *uri); + +unsigned char * virSaveURI(xmlURIPtr uri); + /** * virXMLParse: * @filename: file to parse, or NULL for string parsing diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index b168c7d..11fa995 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -56,6 +56,7 @@ #include "configmake.h" #include "virfile.h" #include "fdstream.h" +#include "xml.h" /* This one changes from version to version. */ #if VBOX_API_VERSION == 2002 @@ -980,7 +981,7 @@ static virDrvOpenStatus vboxOpen(virConnectPtr conn, virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); if (conn->uri == NULL) { - conn->uri = xmlParseURI(uid ? "vbox:///session" : "vbox:///system"); + conn->uri = virParseURI(uid ? "vbox:///session" : "vbox:///system"); if (conn->uri == NULL) { virReportOOMError(); return VIR_DRV_OPEN_ERROR; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 823d5df..92a6e1c 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -33,6 +33,7 @@ #include "logging.h" #include "uuid.h" #include "vmx.h" +#include "xml.h" /* @@ -2608,7 +2609,7 @@ virVMXParseSerial(virVMXContext *ctx, virConfPtr conf, int port, (*def)->target.port = port; (*def)->source.type = VIR_DOMAIN_CHR_TYPE_TCP; - parsedUri = xmlParseURI(fileName); + parsedUri = virParseURI(fileName); if (parsedUri == NULL) { virReportOOMError(); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 635f468..5ba3f1c 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -270,7 +270,7 @@ xenUnifiedOpen (virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) if (!xenUnifiedProbe()) 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/xen/xend_internal.c b/src/xen/xend_internal.c index 5c3838f..e346f5f 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -46,6 +46,7 @@ #include "memory.h" #include "count-one-bits.h" #include "virfile.h" +#include "xml.h" /* required for cpumap_t */ #include <xen/dom0_ops.h> @@ -3224,7 +3225,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, * "hostname", "hostname:port" or "xenmigr://hostname[:port]/". */ if (strstr (uri, "//")) { /* Full URI. */ - xmlURIPtr uriptr = xmlParseURI (uri); + xmlURIPtr uriptr = virParseURI (uri); if (!uriptr) { virXendError(VIR_ERR_INVALID_ARG, "%s", _("xenDaemonDomainMigrate: invalid URI")); -- 1.7.3.4
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 <libxml/parser.h> # include <libxml/tree.h> # include <libxml/xpath.h> +# include <libxml/uri.h>
int virXPathBoolean(const char *xpath, xmlXPathContextPtr ctxt); @@ -61,6 +62,10 @@ xmlDocPtr virXMLParseHelper(int domcode, const char *url, xmlXPathContextPtr *pctxt);
+xmlURIPtr virParseURI(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 :|
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 eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Laine Stump -
Martin Kletzander