Re: [libvirt] [PATCH 04/14] Centralize error reporting for URI parsing/formatting problems

From: "Daniel P. Berrange&quot;&lt;berrange(a)redhat.com&gt; Move error reporting out of the callers, into virURIParse and virURIFormat, to get consistency. * include/libvirt/virterror.h, src/util/virterror.c: Add VIR_FROM_URI * src/util/viruri.c, src/util/viruri.h: Add error reporting * src/esx/esx_driver.c, src/libvirt.c, src/libxl/libxl_driver.c, src/lxc/lxc_driver.c, src/openvz/openvz_driver.c, src/qemu/qemu_driver.c, src/qemu/qemu_migration.c, src/remote/remote_driver.c, src/uml/uml_driver.c, src/vbox/vbox_tmpl.c, src/vmx/vmx.c, src/xen/xen_driver.c, src/xen/xend_internal.c, tests/viruritest.c: Remove error reporting --- include/libvirt/virterror.h | 1 + src/esx/esx_driver.c | 6 +----- src/libvirt.c | 16 ++++------------ src/libxl/libxl_driver.c | 5 +---- src/lxc/lxc_driver.c | 5 +---- src/openvz/openvz_driver.c | 5 +---- src/qemu/qemu_driver.c | 9 +++------ src/qemu/qemu_migration.c | 5 +---- src/remote/remote_driver.c | 18 ++++++++---------- src/uml/uml_driver.c | 9 +++------ src/util/virterror.c | 3 +++ src/util/viruri.c | 26 ++++++++++++++++++++++---- src/util/viruri.h | 6 ++++-- src/vbox/vbox_tmpl.c | 10 +++------- src/vmx/vmx.c | 6 +----- src/xen/xen_driver.c | 5 +---- src/xen/xend_internal.c | 8 +++----- tests/viruritest.c | 8 ++------ 18 files changed, 63 insertions(+), 88 deletions(-) diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 38ac15e..c8ec8d4 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -85,6 +85,7 @@ typedef enum { VIR_FROM_LOCKING = 42, /* Error from lock manager */ VIR_FROM_HYPERV = 43, /* Error from Hyper-V driver */ VIR_FROM_CAPABILITIES = 44, /* Error from capabilities */ + VIR_FROM_URI = 45, /* Error from URI handling */ } virErrorDomain; diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index dbf95f4..7e41fa3 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3977,12 +3977,8 @@ esxDomainMigratePerform(virDomainPtr domain, } /* Parse migration URI */ - parsedUri = virURIParse(uri); - - if (parsedUri == NULL) { - virReportOOMError(); + if (!(parsedUri = virURIParse(uri))) return -1; - } if (parsedUri->scheme == NULL || STRCASENEQ(parsedUri->scheme, "vpxmigr")) { ESX_ERROR(VIR_ERR_INVALID_ARG, "%s", diff --git a/src/libvirt.c b/src/libvirt.c index f58623a..f7590e0 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1166,11 +1166,7 @@ do_open (const char *name, virConnectOpenResolveURIAlias(conf, name,&alias)< 0) goto failed; - ret->uri = virURIParse (alias ? alias : name); - if (!ret->uri) { - virLibConnError(VIR_ERR_INVALID_ARG, - _("could not parse connection URI %s"), - alias ? alias : name); + if (!(ret->uri = virURIParse (alias ? alias : name))) { VIR_FREE(alias); goto failed; } @@ -1771,11 +1767,9 @@ virConnectGetURI (virConnectPtr conn) return NULL; } - name = virURIFormat(conn->uri); - if (!name) { - virReportOOMError(); + if (!(name = virURIFormat(conn->uri))) goto error; - } + return name; error: @@ -5062,9 +5056,7 @@ virDomainMigratePeer2Peer (virDomainPtr domain, return -1; } - tempuri = virURIParse(dconnuri); - if (!tempuri) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!(tempuri = virURIParse(dconnuri))) { virDispatchError(domain->conn); return -1; } diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 177b218..eccd198 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1044,11 +1044,8 @@ libxlOpen(virConnectPtr conn, if (libxl_driver == NULL) return VIR_DRV_OPEN_DECLINED; - conn->uri = virURIParse("xen:///"); - if (!conn->uri) { - virReportOOMError(); + if (!(conn->uri = virURIParse("xen:///"))) return VIR_DRV_OPEN_ERROR; - } } else { /* Only xen scheme */ if (conn->uri->scheme == NULL || STRNEQ(conn->uri->scheme, "xen")) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 3af8084..29842a5 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -140,11 +140,8 @@ static virDrvOpenStatus lxcOpen(virConnectPtr conn, if (lxc_driver == NULL) return VIR_DRV_OPEN_DECLINED; - conn->uri = virURIParse("lxc:///"); - if (!conn->uri) { - virReportOOMError(); + if (!(conn->uri = virURIParse("lxc:///"))) return VIR_DRV_OPEN_ERROR; - } } else { if (conn->uri->scheme == NULL || STRNEQ(conn->uri->scheme, "lxc")) diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 4e369ea..be30640 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1336,11 +1336,8 @@ static virDrvOpenStatus openvzOpen(virConnectPtr conn, if (access("/proc/vz", W_OK)< 0) return VIR_DRV_OPEN_DECLINED; - conn->uri = virURIParse("openvz:///system"); - if (conn->uri == NULL) { - virReportOOMError(); + if (!(conn->uri = virURIParse("openvz:///system"))) return VIR_DRV_OPEN_ERROR; - } } else { /* If scheme isn't 'openvz', then its for another driver */ if (conn->uri->scheme == NULL || diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2c467ab..d90f5fa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -860,13 +860,10 @@ static virDrvOpenStatus qemudOpen(virConnectPtr conn, if (qemu_driver == NULL) return VIR_DRV_OPEN_DECLINED; - conn->uri = virURIParse(qemu_driver->privileged ? - "qemu:///system" : - "qemu:///session"); - if (!conn->uri) { - virReportOOMError(); + if (!(conn->uri = virURIParse(qemu_driver->privileged ? + "qemu:///system" : + "qemu:///session"))) return VIR_DRV_OPEN_ERROR; - } } else { /* If URI isn't 'qemu' its definitely not for us */ if (conn->uri->scheme == NULL || diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 6f274ba..4b17a61 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1855,11 +1855,8 @@ static int doNativeMigrate(struct qemud_driver *driver, } else { uribits = virURIParse(uri); } - if (!uribits) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse URI %s"), uri); + if (!uribits) return -1; - } if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) spec.destType = MIGRATION_DEST_CONNECT_HOST; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 4ddebcb..c6c5809 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -485,7 +485,8 @@ doRemoteOpen (virConnectPtr conn, (STREQ(conn->uri->scheme, "remote") || STRPREFIX(conn->uri->scheme, "remote+"))) { /* Allow remote serve to probe */ - name = strdup(""); + if (!(name = strdup(""))) + goto out_of_memory; } else { virURI tmpuri = { .scheme = conn->uri->scheme, @@ -515,6 +516,9 @@ doRemoteOpen (virConnectPtr conn, /* Restore transport scheme */ if (transport_str) transport_str[-1] = '+'; + + if (!name) + goto failed; } } @@ -522,12 +526,8 @@ doRemoteOpen (virConnectPtr conn, vars = NULL; } else { /* Probe URI server side */ - name = strdup(""); - } - - if (!name) { - virReportOOMError(); - goto failed; + if (!(name = strdup(""))) + goto out_of_memory; } VIR_DEBUG("proceeding with name = %s", name); @@ -720,10 +720,8 @@ doRemoteOpen (virConnectPtr conn, VIR_DEBUG("Auto-probed URI is %s", uriret.uri); conn->uri = virURIParse(uriret.uri); VIR_FREE(uriret.uri); - if (!conn->uri) { - virReportOOMError(); + if (!conn->uri) goto failed; - } } if (!(priv->domainEventState = virDomainEventStateNew())) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 16818cd..d86652c 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1139,13 +1139,10 @@ static virDrvOpenStatus umlOpen(virConnectPtr conn, if (uml_driver == NULL) return VIR_DRV_OPEN_DECLINED; - conn->uri = virURIParse(uml_driver->privileged ? - "uml:///system" : - "uml:///session"); - if (!conn->uri) { - virReportOOMError(); + if (!(conn->uri = virURIParse(uml_driver->privileged ? + "uml:///system" : + "uml:///session"))) return VIR_DRV_OPEN_ERROR; - } } else { if (conn->uri->scheme == NULL || STRNEQ (conn->uri->scheme, "uml")) diff --git a/src/util/virterror.c b/src/util/virterror.c index b4e496a..e1fe522 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -178,6 +178,9 @@ static const char *virErrorDomainName(virErrorDomain domain) { case VIR_FROM_CAPABILITIES: dom = "Capabilities "; break; + case VIR_FROM_URI: + dom = "URI "; + break; } return(dom); } diff --git a/src/util/viruri.c b/src/util/viruri.c index 1d2dca4..bbd8742 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -12,6 +12,14 @@ #include "memory.h" #include "util.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_URI + +#define virURIReportError(code, ...) \ + virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) + /** * virURIParse: @@ -30,9 +38,15 @@ virURIParse(const char *uri) { virURIPtr ret = xmlParseURI(uri); + if (!ret) { + /* libxml2 does not tell us what failed. Grr :-( */ + virURIReportError(VIR_ERR_INTERNAL_ERROR, + "Unable to parse URI %s", uri); + return NULL; + } + /* First check: does it even make sense to jump inside */ - if (ret != NULL&& - ret->server != NULL&& + if (ret->server != NULL&& ret->server[0] == '[') { size_t length = strlen(ret->server); @@ -70,8 +84,7 @@ virURIFormat(virURIPtr uri) char *ret; /* First check: does it make sense to do anything */ - if (uri != NULL&& - uri->server != NULL&& + if (uri->server != NULL&& strchr(uri->server, ':') != NULL) { backupserver = uri->server; @@ -82,7 +95,12 @@ virURIFormat(virURIPtr uri) } ret = (char *) xmlSaveUri(uri); + if (!ret) { + virReportOOMError(); + goto cleanup; + } +cleanup:

/* Put the fixed version back */ if (tmpserver) { uri->server = backupserver; diff --git a/src/util/viruri.h b/src/util/viruri.h index 80369be..5773dda 100644 --- a/src/util/viruri.h +++ b/src/util/viruri.h @@ -16,8 +16,10 @@ typedef xmlURI virURI; typedef xmlURIPtr virURIPtr; -virURIPtr virURIParse(const char *uri); -char *virURIFormat(virURIPtr uri); +virURIPtr virURIParse(const char *uri) + ATTRIBUTE_NONNULL(1); +char *virURIFormat(virURIPtr uri) + ATTRIBUTE_NONNULL(1); void virURIFree(virURIPtr uri); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 853a599..7769b0c 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -980,13 +980,9 @@ static virDrvOpenStatus vboxOpen(virConnectPtr conn, virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); - if (conn->uri == NULL) { - conn->uri = virURIParse(uid ? "vbox:///session" : "vbox:///system"); - if (conn->uri == NULL) { - virReportOOMError(); - return VIR_DRV_OPEN_ERROR; - } - } + if (conn->uri == NULL&& + !(conn->uri = virURIParse(uid ? "vbox:///session" : "vbox:///system"))) + return VIR_DRV_OPEN_ERROR; if (conn->uri->scheme == NULL || STRNEQ (conn->uri->scheme, "vbox")) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 3179688..4324bb8 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2617,12 +2617,8 @@ virVMXParseSerial(virVMXContext *ctx, virConfPtr conf, int port, (*def)->target.port = port; (*def)->source.type = VIR_DOMAIN_CHR_TYPE_TCP; - parsedUri = virURIParse(fileName); - - if (parsedUri == NULL) { - virReportOOMError(); + if (!(parsedUri = virURIParse(fileName))) goto cleanup; - } if (parsedUri->port == 0) { VMX_ERROR(VIR_ERR_INTERNAL_ERROR, diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 0238ed7..4011913 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -270,11 +270,8 @@ xenUnifiedOpen (virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) if (!xenUnifiedProbe()) return VIR_DRV_OPEN_DECLINED; - conn->uri = virURIParse("xen:///"); - if (!conn->uri) { - virReportOOMError(); + if (!(conn->uri = virURIParse("xen:///"))) return VIR_DRV_OPEN_ERROR; - } } else { if (conn->uri->scheme) { /* Decline any scheme which isn't "xen://" or "http://". */ diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index df6b1bb..55bb5db 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -3224,12 +3224,10 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, * "hostname", "hostname:port" or "xenmigr://hostname[:port]/". */ if (strstr (uri, "//")) { /* Full URI. */ - virURIPtr uriptr = virURIParse (uri); - if (!uriptr) { - virXendError(VIR_ERR_INVALID_ARG, - "%s", _("xenDaemonDomainMigrate: invalid URI")); + virURIPtr uriptr; + if (!(uriptr = virURIParse (uri))) return -1; - } + if (uriptr->scheme&& STRCASENEQ (uriptr->scheme, "xenmigr")) { virXendError(VIR_ERR_INVALID_ARG, "%s", _("xenDaemonDomainMigrate: only xenmigr://" diff --git a/tests/viruritest.c b/tests/viruritest.c index 0b38219..fea5ddd 100644 --- a/tests/viruritest.c +++ b/tests/viruritest.c @@ -50,15 +50,11 @@ static int testURIParse(const void *args) const struct URIParseData *data = args; char *uristr; - if (!(uri = virURIParse(data->uri))) { - virReportOOMError(); + if (!(uri = virURIParse(data->uri))) goto cleanup;

- } - if (!(uristr = virURIFormat(uri))) { - virReportOOMError(); + if (!(uristr = virURIFormat(uri))) goto cleanup; - } if (!STREQ(uristr, data->uri)) { VIR_DEBUG("URI did not roundtrip, expect '%s', actual '%s'",