[libvirt] [PATCH 04/14] Centralize error reporting for URI parsing/formatting problems
Osier Yang
jyang at redhat.com
Thu Mar 22 04:03:11 UTC 2012
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"<berrange at redhat.com>
>
> 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:
The cleanup label doesn't make any sense. Or it's for follow up
pacthes use? but it should be together with the follow up patch
if so.
> /* 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;
Forgot my doubt on 1/14 now, :-)
> - }
>
> - 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'",
ACK with the "cleanup" fixed.
Osier
More information about the libvir-list
mailing list