[libvirt] [PATCH 00/14] Support reading auth credentials from a config file
When connecting to a remote libvirtd, or hypervisor that requires authentication we have callbacks which can be used to prompt for various credentials. This is tedious if you are looking to automate stuff though. While we could let each application implement a custom auth callback for fetching credentials from a config file, it is nicer if we provide this ability directly in libvirt, so that all apps can benefit from it. So this series introduces a new config file in which authentication credentials can be placed. It supports setting multiple sets of credentials, of varying types in one file. I use the '.ini' config file syntax, since this makes it easy to reuse the same config file in virt-viewer & similar apps to store VNC / SPICE credentials. To find the config file we check for each of - Path from LIBVIRT_AUTH_FILE env var - Query param 'authfile=PATH' in the URI params - /etc/libvirt/auth.conf - $HOME/.libvirt/auth.conf if no config file is present, everything should carry on as normal. If the config is present, then it is consulted to fill in credentials first. Only if there are still missing credentials, will the auth callbacks then be invoked. See the docs in patch 12 of this series for the config file syntax examples. This is supported for all drivers that do auth, remote, esx, xenapi, hyperv and phyp, though I have only tested it with the remote driver. I would appreciate someone giving it a go with the other drivers. The first 7 patches are actually all related to the viruri.[ch] file, expanding its functionality & adding tests. This is to make sure the auth config patches can have easy access to the query params. The last 7 patches are related to the auth config file support again including tests
From: "Daniel P. Berrange" <berrange@redhat.com> To ensure we properly escape & unescape IPv6 numeric addresses, add a test case * tests/Makefile.am, tests/viruritest.c: URI parsing test --- tests/Makefile.am | 7 ++- tests/viruritest.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 1 deletions(-) create mode 100644 tests/viruritest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 0c8cf37..035c8c6 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -96,7 +96,7 @@ check_PROGRAMS = virshtest conftest sockettest \ commandtest commandhelper seclabeltest \ virhashtest virnetmessagetest virnetsockettest ssh \ utiltest virnettlscontexttest shunloadtest \ - virtimetest + virtimetest viruritest check_LTLIBRARIES = libshunload.la @@ -219,6 +219,7 @@ TESTS = virshtest \ virnetsockettest \ virnettlscontexttest \ virtimetest \ + viruritest \ shunloadtest \ utiltest \ $(test_scripts) @@ -506,6 +507,10 @@ virtimetest_SOURCES = \ virtimetest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) virtimetest_LDADD = ../src/libvirt-net-rpc.la $(LDADDS) +viruritest_SOURCES = \ + viruritest.c testutils.h testutils.c +viruritest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) +viruritest_LDADD = ../src/libvirt-net-rpc.la $(LDADDS) seclabeltest_SOURCES = \ seclabeltest.c diff --git a/tests/viruritest.c b/tests/viruritest.c new file mode 100644 index 0000000..a5de50a --- /dev/null +++ b/tests/viruritest.c @@ -0,0 +1,139 @@ +/* + * Copyright (C) 2011 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include <stdlib.h> +#include <signal.h> + +#include "testutils.h" +#include "util.h" +#include "virterror_internal.h" +#include "memory.h" +#include "logging.h" + +#include "viruri.h" + +#define VIR_FROM_THIS VIR_FROM_RPC + +struct URIParseData { + const char *uri; + const char *scheme; + const char *server; + int port; + const char *path; + const char *query; + const char *fragment; +}; + +static int testURIParse(const void *args) +{ + int ret = -1; + virURIPtr uri = NULL; + const struct URIParseData *data = args; + char *uristr; + + if (!(uri = virURIParse(data->uri))) { + virReportOOMError(); + goto cleanup; + } + + if (!(uristr = virURIFormat(uri))) { + virReportOOMError(); + goto cleanup; + } + + if (!STREQ(uristr, data->uri)) { + VIR_DEBUG("URI did not roundtrip, expect '%s', actual '%s'", + data->uri, uristr); + goto cleanup; + } + + if (!STREQ(uri->scheme, data->scheme)) { + VIR_DEBUG("Expected scheme '%s', actual '%s'", + data->scheme, uri->scheme); + goto cleanup; + } + + if (!STREQ(uri->server, data->server)) { + VIR_DEBUG("Expected server '%s', actual '%s'", + data->server, uri->server); + goto cleanup; + } + + if (uri->port != data->port) { + VIR_DEBUG("Expected port '%d', actual '%d'", + data->port, uri->port); + goto cleanup; + } + + if (!STREQ_NULLABLE(uri->path, data->path)) { + VIR_DEBUG("Expected path '%s', actual '%s'", + data->path, uri->path); + goto cleanup; + } + + if (!STREQ_NULLABLE(uri->query, data->query)) { + VIR_DEBUG("Expected query '%s', actual '%s'", + data->query, uri->query); + goto cleanup; + } + + if (!STREQ_NULLABLE(uri->fragment, data->fragment)) { + VIR_DEBUG("Expected fragment '%s', actual '%s'", + data->fragment, uri->fragment); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(uristr); + xmlFreeURI(uri); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + signal(SIGPIPE, SIG_IGN); + +#define TEST_PARSE(uri, scheme, server, port, path, query, fragment) \ + do { \ + const struct URIParseData data = { \ + uri, scheme, server, port, path, query, fragment \ + }; \ + if (virtTestRun("Test IPv6 " # uri, 1, testURIParse, &data) < 0) \ + ret = -1; \ + } while (0) + + TEST_PARSE("test://example.com", "test", "example.com", 0, NULL, NULL, NULL); + TEST_PARSE("test://example.com:123", "test", "example.com", 123, NULL, NULL, NULL); + TEST_PARSE("test://example.com:123/system?name=value#foo", "test", "example.com", 123, "/system", "name=value", "foo"); + TEST_PARSE("test://127.0.0.1:123/system", "test", "127.0.0.1", 123, "/system", NULL, NULL); + TEST_PARSE("test://[::1]:123/system", "test", "::1", 123, "/system", NULL, NULL); + TEST_PARSE("test://[2001:41c8:1:4fd4::2]:123/system", "test", "2001:41c8:1:4fd4::2", 123, "/system", NULL, NULL); + + return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); +} + +VIRT_TEST_MAIN(mymain) -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
To ensure we properly escape& unescape IPv6 numeric addresses, add a test case
* tests/Makefile.am, tests/viruritest.c: URI parsing test --- tests/Makefile.am | 7 ++- tests/viruritest.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 1 deletions(-) create mode 100644 tests/viruritest.c
diff --git a/tests/Makefile.am b/tests/Makefile.am index 0c8cf37..035c8c6 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -96,7 +96,7 @@ check_PROGRAMS = virshtest conftest sockettest \ commandtest commandhelper seclabeltest \ virhashtest virnetmessagetest virnetsockettest ssh \ utiltest virnettlscontexttest shunloadtest \ - virtimetest + virtimetest viruritest
check_LTLIBRARIES = libshunload.la
@@ -219,6 +219,7 @@ TESTS = virshtest \ virnetsockettest \ virnettlscontexttest \ virtimetest \ + viruritest \
Seems like this uses spaces?
shunloadtest \ utiltest \ $(test_scripts) @@ -506,6 +507,10 @@ virtimetest_SOURCES = \ virtimetest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) virtimetest_LDADD = ../src/libvirt-net-rpc.la $(LDADDS)
+viruritest_SOURCES = \ + viruritest.c testutils.h testutils.c +viruritest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) +viruritest_LDADD = ../src/libvirt-net-rpc.la $(LDADDS)
seclabeltest_SOURCES = \ seclabeltest.c diff --git a/tests/viruritest.c b/tests/viruritest.c new file mode 100644 index 0000000..a5de50a --- /dev/null +++ b/tests/viruritest.c @@ -0,0 +1,139 @@ +/* + * Copyright (C) 2011 Red Hat, Inc.
s/2011/2012/
+ * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange<berrange@redhat.com> + */ + +#include<config.h> + +#include<stdlib.h> +#include<signal.h> + +#include "testutils.h" +#include "util.h" +#include "virterror_internal.h" +#include "memory.h" +#include "logging.h" + +#include "viruri.h" + +#define VIR_FROM_THIS VIR_FROM_RPC + +struct URIParseData { + const char *uri; + const char *scheme; + const char *server; + int port; + const char *path; + const char *query; + const char *fragment; +}; + +static int testURIParse(const void *args) +{ + int ret = -1; + virURIPtr uri = NULL; + const struct URIParseData *data = args; + char *uristr; + + if (!(uri = virURIParse(data->uri))) { + virReportOOMError();
It's not neccessary to be out of memory error, perhaps VIR_DEBUG with a msg is better here.
+ goto cleanup; + } + + if (!(uristr = virURIFormat(uri))) { + virReportOOMError();
likewise
+ goto cleanup; + } + + if (!STREQ(uristr, data->uri)) { + VIR_DEBUG("URI did not roundtrip, expect '%s', actual '%s'", + data->uri, uristr); + goto cleanup; + } + + if (!STREQ(uri->scheme, data->scheme)) { + VIR_DEBUG("Expected scheme '%s', actual '%s'", + data->scheme, uri->scheme); + goto cleanup; + } + + if (!STREQ(uri->server, data->server)) { + VIR_DEBUG("Expected server '%s', actual '%s'", + data->server, uri->server); + goto cleanup; + } + + if (uri->port != data->port) { + VIR_DEBUG("Expected port '%d', actual '%d'", + data->port, uri->port); + goto cleanup; + } + + if (!STREQ_NULLABLE(uri->path, data->path)) { + VIR_DEBUG("Expected path '%s', actual '%s'", + data->path, uri->path); + goto cleanup; + } + + if (!STREQ_NULLABLE(uri->query, data->query)) { + VIR_DEBUG("Expected query '%s', actual '%s'", + data->query, uri->query); + goto cleanup; + } + + if (!STREQ_NULLABLE(uri->fragment, data->fragment)) { + VIR_DEBUG("Expected fragment '%s', actual '%s'", + data->fragment, uri->fragment); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(uristr); + xmlFreeURI(uri); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + signal(SIGPIPE, SIG_IGN); + +#define TEST_PARSE(uri, scheme, server, port, path, query, fragment) \ + do { \ + const struct URIParseData data = { \
Indention
+ uri, scheme, server, port, path, query, fragment \ + }; \ + if (virtTestRun("Test IPv6 " # uri, 1, testURIParse,&data)< 0) \ + ret = -1; \ + } while (0) + + TEST_PARSE("test://example.com", "test", "example.com", 0, NULL, NULL, NULL); + TEST_PARSE("test://example.com:123", "test", "example.com", 123, NULL, NULL, NULL); + TEST_PARSE("test://example.com:123/system?name=value#foo", "test", "example.com", 123, "/system", "name=value", "foo"); + TEST_PARSE("test://127.0.0.1:123/system", "test", "127.0.0.1", 123, "/system", NULL, NULL); + TEST_PARSE("test://[::1]:123/system", "test", "::1", 123, "/system", NULL, NULL); + TEST_PARSE("test://[2001:41c8:1:4fd4::2]:123/system", "test", "2001:41c8:1:4fd4::2", 123, "/system", NULL, NULL); + + return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); +} + +VIRT_TEST_MAIN(mymain)
Others look good. ACK with those fixed.
On Thu, Mar 22, 2012 at 11:54:07AM +0800, Osier Yang wrote:
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
To ensure we properly escape& unescape IPv6 numeric addresses, add a test case
* tests/Makefile.am, tests/viruritest.c: URI parsing test --- tests/Makefile.am | 7 ++- tests/viruritest.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 1 deletions(-) create mode 100644 tests/viruritest.c
+static int testURIParse(const void *args) +{ + int ret = -1; + virURIPtr uri = NULL; + const struct URIParseData *data = args; + char *uristr; + + if (!(uri = virURIParse(data->uri))) { + virReportOOMError();
It's not neccessary to be out of memory error, perhaps VIR_DEBUG with a msg is better here.
libxml doesn't do any usefull error reporting here so we can't find out what is actually wrong :-( 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 :|
From: "Daniel P. Berrange" <berrange@redhat.com> Since we defined a custom virURIPtr type, we should use a virURIFree method instead of assuming it will always be a typedef for xmlURIPtr * src/util/viruri.c, src/util/viruri.h, src/libvirt_private.syms: Add a virURIFree method * src/datatypes.c, src/esx/esx_driver.c, src/libvirt.c, src/qemu/qemu_migration.c, src/vmx/vmx.c, src/xen/xend_internal.c, tests/viruritest.c: s/xmlFreeURI/virURIFree/ --- src/datatypes.c | 2 +- src/esx/esx_driver.c | 2 +- src/libvirt.c | 4 ++-- src/libvirt_private.syms | 1 + src/qemu/qemu_migration.c | 2 +- src/util/viruri.c | 12 ++++++++++++ src/util/viruri.h | 2 ++ src/vmx/vmx.c | 2 +- src/xen/xend_internal.c | 8 ++++---- tests/viruritest.c | 2 +- 10 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index f0b5025..5ad988b 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -117,7 +117,7 @@ virReleaseConnect(virConnectPtr conn) { virResetError(&conn->err); - xmlFreeURI(conn->uri); + virURIFree(conn->uri); virMutexUnlock(&conn->lock); virMutexDestroy(&conn->lock); diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 6943534..dbf95f4 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4072,7 +4072,7 @@ esxDomainMigratePerform(virDomainPtr domain, result = 0; cleanup: - xmlFreeURI(parsedUri); + virURIFree(parsedUri); esxVI_ObjectContent_Free(&virtualMachine); esxVI_Event_Free(&eventList); esxVI_ManagedObjectReference_Free(&task); diff --git a/src/libvirt.c b/src/libvirt.c index 7f8d42c..f58623a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5072,10 +5072,10 @@ virDomainMigratePeer2Peer (virDomainPtr domain, if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) { virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); virDispatchError(domain->conn); - xmlFreeURI(tempuri); + virURIFree(tempuri); return -1; } - xmlFreeURI(tempuri); + virURIFree(tempuri); /* Perform the migration. The driver isn't supposed to return * until the migration is complete. diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e40c80e..7cd6a96 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1469,6 +1469,7 @@ virTypedParameterAssign; # viruri.h +virURIFree; virURIFormat; virURIParse; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 81b2d5b..6f274ba 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1875,7 +1875,7 @@ static int doNativeMigrate(struct qemud_driver *driver, if (spec.destType == MIGRATION_DEST_FD) VIR_FORCE_CLOSE(spec.dest.fd.qemu); - xmlFreeURI(uribits); + virURIFree(uribits); return ret; } diff --git a/src/util/viruri.c b/src/util/viruri.c index 6c4dfe3..0b25679 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -91,3 +91,15 @@ virURIFormat(xmlURIPtr uri) return ret; } + + +/** + * virURIFree: + * @uri: uri to free + * + * Frees the URI + */ +void virURIFree(virURIPtr uri) +{ + xmlFreeURI(uri); +} diff --git a/src/util/viruri.h b/src/util/viruri.h index 5215e42..80369be 100644 --- a/src/util/viruri.h +++ b/src/util/viruri.h @@ -19,4 +19,6 @@ typedef xmlURIPtr virURIPtr; virURIPtr virURIParse(const char *uri); char *virURIFormat(virURIPtr uri); +void virURIFree(virURIPtr uri); + #endif /* __VIR_URI_H__ */ diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index cc426da..3179688 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2696,7 +2696,7 @@ virVMXParseSerial(virVMXContext *ctx, virConfPtr conf, int port, VIR_FREE(fileType); VIR_FREE(fileName); VIR_FREE(network_endPoint); - xmlFreeURI(parsedUri); + virURIFree(parsedUri); return result; diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index a19d055..df6b1bb 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -3234,25 +3234,25 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, virXendError(VIR_ERR_INVALID_ARG, "%s", _("xenDaemonDomainMigrate: only xenmigr://" " migrations are supported by Xen")); - xmlFreeURI (uriptr); + virURIFree (uriptr); return -1; } if (!uriptr->server) { virXendError(VIR_ERR_INVALID_ARG, "%s", _("xenDaemonDomainMigrate: a hostname must be" " specified in the URI")); - xmlFreeURI (uriptr); + virURIFree (uriptr); return -1; } hostname = strdup (uriptr->server); if (!hostname) { virReportOOMError(); - xmlFreeURI (uriptr); + virURIFree (uriptr); return -1; } if (uriptr->port) snprintf (port, sizeof port, "%d", uriptr->port); - xmlFreeURI (uriptr); + virURIFree (uriptr); } else if ((p = strrchr (uri, ':')) != NULL) { /* "hostname:port" */ int port_nr, n; diff --git a/tests/viruritest.c b/tests/viruritest.c index a5de50a..0b38219 100644 --- a/tests/viruritest.c +++ b/tests/viruritest.c @@ -105,7 +105,7 @@ static int testURIParse(const void *args) ret = 0; cleanup: VIR_FREE(uristr); - xmlFreeURI(uri); + virURIFree(uri); return ret; } -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
Since we defined a custom virURIPtr type, we should use a virURIFree method instead of assuming it will always be a typedef for xmlURIPtr
* src/util/viruri.c, src/util/viruri.h, src/libvirt_private.syms: Add a virURIFree method * src/datatypes.c, src/esx/esx_driver.c, src/libvirt.c, src/qemu/qemu_migration.c, src/vmx/vmx.c, src/xen/xend_internal.c, tests/viruritest.c: s/xmlFreeURI/virURIFree/ --- src/datatypes.c | 2 +- src/esx/esx_driver.c | 2 +- src/libvirt.c | 4 ++-- src/libvirt_private.syms | 1 + src/qemu/qemu_migration.c | 2 +- src/util/viruri.c | 12 ++++++++++++ src/util/viruri.h | 2 ++ src/vmx/vmx.c | 2 +- src/xen/xend_internal.c | 8 ++++---- tests/viruritest.c | 2 +- 10 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/src/datatypes.c b/src/datatypes.c index f0b5025..5ad988b 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -117,7 +117,7 @@ virReleaseConnect(virConnectPtr conn) {
virResetError(&conn->err);
- xmlFreeURI(conn->uri); + virURIFree(conn->uri);
virMutexUnlock(&conn->lock); virMutexDestroy(&conn->lock); diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 6943534..dbf95f4 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4072,7 +4072,7 @@ esxDomainMigratePerform(virDomainPtr domain, result = 0;
cleanup: - xmlFreeURI(parsedUri); + virURIFree(parsedUri); esxVI_ObjectContent_Free(&virtualMachine); esxVI_Event_Free(&eventList); esxVI_ManagedObjectReference_Free(&task); diff --git a/src/libvirt.c b/src/libvirt.c index 7f8d42c..f58623a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5072,10 +5072,10 @@ virDomainMigratePeer2Peer (virDomainPtr domain, if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) { virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); virDispatchError(domain->conn); - xmlFreeURI(tempuri); + virURIFree(tempuri); return -1; } - xmlFreeURI(tempuri); + virURIFree(tempuri);
/* Perform the migration. The driver isn't supposed to return * until the migration is complete. diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e40c80e..7cd6a96 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1469,6 +1469,7 @@ virTypedParameterAssign;
# viruri.h +virURIFree; virURIFormat;
it should be after virURIFormat. ACK with this fixed.
From: "Daniel P. Berrange" <berrange@redhat.com> The parameter in the virURIFormat impl mistakenly used the xmlURIPtr type, instead of virURIPtr. Since they will soon cease to be identical, this needs fixing --- src/util/viruri.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/util/viruri.c b/src/util/viruri.c index 0b25679..1d2dca4 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -63,7 +63,7 @@ virURIParse(const char *uri) * @returns the constructed uri as a string */ char * -virURIFormat(xmlURIPtr uri) +virURIFormat(virURIPtr uri) { char *backupserver = NULL; char *tmpserver = NULL; -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
The parameter in the virURIFormat impl mistakenly used the xmlURIPtr type, instead of virURIPtr. Since they will soon cease to be identical, this needs fixing --- src/util/viruri.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/viruri.c b/src/util/viruri.c index 0b25679..1d2dca4 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -63,7 +63,7 @@ virURIParse(const char *uri) * @returns the constructed uri as a string */ char * -virURIFormat(xmlURIPtr uri) +virURIFormat(virURIPtr uri) { char *backupserver = NULL; char *tmpserver = NULL;
ACK
From: "Daniel P. Berrange" <berrange@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: /* 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'", -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@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
On Thu, Mar 22, 2012 at 12:03:11PM +0800, Osier Yang wrote:
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com> @@ -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.
I think it is preferrable to always have an explicit cleanup: statement in this scenario, rather than relying on fallthrough. It avoids future code additionals introducing cleanup bugs. 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 :|
From: "Daniel P. Berrange" <berrange@redhat.com> Instead of just typedef'ing the xmlURIPtr struct for virURIPtr, use a custom libvirt struct. This allows us to fix various problems with libxml2. This initially just fixes the query vs query_raw handling problems. --- src/esx/esx_util.c | 4 -- src/hyperv/hyperv_util.c | 4 -- src/libvirt.c | 5 +-- src/remote/remote_driver.c | 16 +-------- src/util/viruri.c | 79 +++++++++++++++++++++++++++++++++++-------- src/util/viruri.h | 14 +++++++- 6 files changed, 78 insertions(+), 44 deletions(-) diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 7d4b908..67b07b7 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -62,11 +62,7 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri) return -1; } -#ifdef HAVE_XMLURI_QUERY_RAW - queryParamSet = qparam_query_parse(uri->query_raw); -#else queryParamSet = qparam_query_parse(uri->query); -#endif if (queryParamSet == NULL) { goto cleanup; diff --git a/src/hyperv/hyperv_util.c b/src/hyperv/hyperv_util.c index 2e6a2d4..63c761b 100644 --- a/src/hyperv/hyperv_util.c +++ b/src/hyperv/hyperv_util.c @@ -54,11 +54,7 @@ hypervParseUri(hypervParsedUri **parsedUri, virURIPtr uri) return -1; } -#ifdef HAVE_XMLURI_QUERY_RAW - queryParamSet = qparam_query_parse(uri->query_raw); -#else queryParamSet = qparam_query_parse(uri->query); -#endif if (queryParamSet == NULL) { goto cleanup; diff --git a/src/libvirt.c b/src/libvirt.c index f7590e0..fb7885f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1173,15 +1173,12 @@ do_open (const char *name, VIR_DEBUG("name \"%s\" to URI components:\n" " scheme %s\n" - " opaque %s\n" - " authority %s\n" " server %s\n" " user %s\n" " port %d\n" " path %s\n", alias ? alias : name, - NULLSTR(ret->uri->scheme), NULLSTR(ret->uri->opaque), - NULLSTR(ret->uri->authority), NULLSTR(ret->uri->server), + NULLSTR(ret->uri->scheme), NULLSTR(ret->uri->server), NULLSTR(ret->uri->user), ret->uri->port, NULLSTR(ret->uri->path)); diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index c6c5809..9de966f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -417,15 +417,9 @@ doRemoteOpen (virConnectPtr conn, */ struct qparam *var; int i; - char *query; if (conn->uri) { -#ifdef HAVE_XMLURI_QUERY_RAW - query = conn->uri->query_raw; -#else - query = conn->uri->query; -#endif - vars = qparam_query_parse (query); + vars = qparam_query_parse (conn->uri->query); if (vars == NULL) goto failed; for (i = 0; i < vars->n; i++) { @@ -490,11 +484,7 @@ doRemoteOpen (virConnectPtr conn, } else { virURI tmpuri = { .scheme = conn->uri->scheme, -#ifdef HAVE_XMLURI_QUERY_RAW - .query_raw = qparam_get_query (vars), -#else .query = qparam_get_query (vars), -#endif .path = conn->uri->path, .fragment = conn->uri->fragment, }; @@ -507,11 +497,7 @@ doRemoteOpen (virConnectPtr conn, name = virURIFormat(&tmpuri); -#ifdef HAVE_XMLURI_QUERY_RAW - VIR_FREE(tmpuri.query_raw); -#else VIR_FREE(tmpuri.query); -#endif /* Restore transport scheme */ if (transport_str) diff --git a/src/util/viruri.c b/src/util/viruri.c index bbd8742..d8618d1 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -36,15 +36,45 @@ virURIPtr virURIParse(const char *uri) { - virURIPtr ret = xmlParseURI(uri); + xmlURIPtr xmluri; + virURIPtr ret = NULL; - if (!ret) { + xmluri = xmlParseURI(uri); + + if (!uri) { /* libxml2 does not tell us what failed. Grr :-( */ virURIReportError(VIR_ERR_INTERNAL_ERROR, "Unable to parse URI %s", uri); return NULL; } + if (VIR_ALLOC(ret) < 0) + goto no_memory; + + if (xmluri->scheme && + !(ret->scheme = strdup(xmluri->scheme))) + goto no_memory; + if (xmluri->server && + !(ret->server = strdup(xmluri->server))) + goto no_memory; + ret->port = xmluri->port; + if (xmluri->path && + !(ret->path = strdup(xmluri->path))) + goto no_memory; +#ifdef HAVE_XMLURI_QUERY_RAW + if (xmluri->query_raw && + !(ret->query = strdup(xmluri->query_raw))) + goto no_memory; +#else + if (xmluri->query && + !(ret->query = strdup(xmluri->query))) + goto no_memory; +#endif + if (xmluri->fragment && + !(ret->fragment = strdup(xmluri->fragment))) + goto no_memory; + + /* First check: does it even make sense to jump inside */ if (ret->server != NULL && ret->server[0] == '[') { @@ -62,7 +92,15 @@ virURIParse(const char *uri) * the uri with xmlFreeURI() */ } + xmlFreeURI(xmluri); + return ret; + +no_memory: + virReportOOMError(); + xmlFreeURI(xmluri); + virURIFree(ret); + return NULL; } /** @@ -79,33 +117,37 @@ virURIParse(const char *uri) char * virURIFormat(virURIPtr uri) { - char *backupserver = NULL; + xmlURI xmluri; char *tmpserver = NULL; char *ret; + memset(&xmluri, 0, sizeof(xmluri)); + + xmluri.scheme = uri->scheme; + xmluri.server = uri->server; + xmluri.port = uri->port; + xmluri.path = uri->path; + xmluri.query = uri->query; + xmluri.fragment = uri->fragment; + /* First check: does it make sense to do anything */ - if (uri->server != NULL && - strchr(uri->server, ':') != NULL) { + if (xmluri.server != NULL && + strchr(xmluri.server, ':') != NULL) { - backupserver = uri->server; - if (virAsprintf(&tmpserver, "[%s]", uri->server) < 0) + if (virAsprintf(&tmpserver, "[%s]", xmluri.server) < 0) return NULL; - uri->server = tmpserver; + xmluri.server = tmpserver; } - ret = (char *) xmlSaveUri(uri); + ret = (char *)xmlSaveUri(&xmluri); if (!ret) { virReportOOMError(); goto cleanup; } cleanup: - /* Put the fixed version back */ - if (tmpserver) { - uri->server = backupserver; - VIR_FREE(tmpserver); - } + VIR_FREE(tmpserver); return ret; } @@ -119,5 +161,12 @@ cleanup: */ void virURIFree(virURIPtr uri) { - xmlFreeURI(uri); + if (!uri) + return; + + VIR_FREE(uri->scheme); + VIR_FREE(uri->server); + VIR_FREE(uri->path); + VIR_FREE(uri->query); + VIR_FREE(uri); } diff --git a/src/util/viruri.h b/src/util/viruri.h index 5773dda..dd270de 100644 --- a/src/util/viruri.h +++ b/src/util/viruri.h @@ -13,8 +13,18 @@ # include "internal.h" -typedef xmlURI virURI; -typedef xmlURIPtr virURIPtr; +typedef struct _virURI virURI; +typedef virURI *virURIPtr; + +struct _virURI { + char *scheme; /* the URI scheme */ + char *server; /* the server part */ + char *user; /* the user part */ + int port; /* the port number */ + char *path; /* the path string */ + char *query; /* the query string */ + char *fragment; /* the fragment string */ +}; virURIPtr virURIParse(const char *uri) ATTRIBUTE_NONNULL(1); -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
Instead of just typedef'ing the xmlURIPtr struct for virURIPtr, use a custom libvirt struct. This allows us to fix various problems with libxml2. This initially just fixes the query vs query_raw handling problems. --- src/esx/esx_util.c | 4 -- src/hyperv/hyperv_util.c | 4 -- src/libvirt.c | 5 +-- src/remote/remote_driver.c | 16 +-------- src/util/viruri.c | 79 +++++++++++++++++++++++++++++++++++-------- src/util/viruri.h | 14 +++++++- 6 files changed, 78 insertions(+), 44 deletions(-)
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 7d4b908..67b07b7 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -62,11 +62,7 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri) return -1; }
-#ifdef HAVE_XMLURI_QUERY_RAW - queryParamSet = qparam_query_parse(uri->query_raw); -#else queryParamSet = qparam_query_parse(uri->query); -#endif
if (queryParamSet == NULL) { goto cleanup; diff --git a/src/hyperv/hyperv_util.c b/src/hyperv/hyperv_util.c index 2e6a2d4..63c761b 100644 --- a/src/hyperv/hyperv_util.c +++ b/src/hyperv/hyperv_util.c @@ -54,11 +54,7 @@ hypervParseUri(hypervParsedUri **parsedUri, virURIPtr uri) return -1; }
-#ifdef HAVE_XMLURI_QUERY_RAW - queryParamSet = qparam_query_parse(uri->query_raw); -#else queryParamSet = qparam_query_parse(uri->query); -#endif
if (queryParamSet == NULL) { goto cleanup; diff --git a/src/libvirt.c b/src/libvirt.c index f7590e0..fb7885f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1173,15 +1173,12 @@ do_open (const char *name,
VIR_DEBUG("name \"%s\" to URI components:\n" " scheme %s\n" - " opaque %s\n" - " authority %s\n" " server %s\n" " user %s\n" " port %d\n" " path %s\n", alias ? alias : name, - NULLSTR(ret->uri->scheme), NULLSTR(ret->uri->opaque), - NULLSTR(ret->uri->authority), NULLSTR(ret->uri->server), + NULLSTR(ret->uri->scheme), NULLSTR(ret->uri->server), NULLSTR(ret->uri->user), ret->uri->port, NULLSTR(ret->uri->path));
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index c6c5809..9de966f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -417,15 +417,9 @@ doRemoteOpen (virConnectPtr conn, */ struct qparam *var; int i; - char *query;
if (conn->uri) { -#ifdef HAVE_XMLURI_QUERY_RAW - query = conn->uri->query_raw; -#else - query = conn->uri->query; -#endif - vars = qparam_query_parse (query); + vars = qparam_query_parse (conn->uri->query); if (vars == NULL) goto failed;
for (i = 0; i< vars->n; i++) { @@ -490,11 +484,7 @@ doRemoteOpen (virConnectPtr conn, } else { virURI tmpuri = { .scheme = conn->uri->scheme, -#ifdef HAVE_XMLURI_QUERY_RAW - .query_raw = qparam_get_query (vars), -#else .query = qparam_get_query (vars), -#endif .path = conn->uri->path, .fragment = conn->uri->fragment, }; @@ -507,11 +497,7 @@ doRemoteOpen (virConnectPtr conn,
name = virURIFormat(&tmpuri);
-#ifdef HAVE_XMLURI_QUERY_RAW - VIR_FREE(tmpuri.query_raw); -#else VIR_FREE(tmpuri.query); -#endif
/* Restore transport scheme */ if (transport_str) diff --git a/src/util/viruri.c b/src/util/viruri.c index bbd8742..d8618d1 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -36,15 +36,45 @@ virURIPtr virURIParse(const char *uri) { - virURIPtr ret = xmlParseURI(uri); + xmlURIPtr xmluri; + virURIPtr ret = NULL;
- if (!ret) { + xmluri = xmlParseURI(uri); + + if (!uri) {
s/uri/xmluri/
/* libxml2 does not tell us what failed. Grr :-( */ virURIReportError(VIR_ERR_INTERNAL_ERROR, "Unable to parse URI %s", uri); return NULL; }
+ if (VIR_ALLOC(ret)< 0) + goto no_memory; + + if (xmluri->scheme&& + !(ret->scheme = strdup(xmluri->scheme))) + goto no_memory; + if (xmluri->server&& + !(ret->server = strdup(xmluri->server))) + goto no_memory; + ret->port = xmluri->port; + if (xmluri->path&& + !(ret->path = strdup(xmluri->path))) + goto no_memory; +#ifdef HAVE_XMLURI_QUERY_RAW + if (xmluri->query_raw&& + !(ret->query = strdup(xmluri->query_raw))) + goto no_memory; +#else + if (xmluri->query&& + !(ret->query = strdup(xmluri->query))) + goto no_memory; +#endif + if (xmluri->fragment&& + !(ret->fragment = strdup(xmluri->fragment))) + goto no_memory; + + /* First check: does it even make sense to jump inside */ if (ret->server != NULL&& ret->server[0] == '[') { @@ -62,7 +92,15 @@ virURIParse(const char *uri) * the uri with xmlFreeURI() */ }
+ xmlFreeURI(xmluri); + return ret; + +no_memory: + virReportOOMError(); + xmlFreeURI(xmluri); + virURIFree(ret); + return NULL; }
/** @@ -79,33 +117,37 @@ virURIParse(const char *uri) char * virURIFormat(virURIPtr uri) { - char *backupserver = NULL; + xmlURI xmluri; char *tmpserver = NULL; char *ret;
+ memset(&xmluri, 0, sizeof(xmluri)); + + xmluri.scheme = uri->scheme; + xmluri.server = uri->server; + xmluri.port = uri->port; + xmluri.path = uri->path; + xmluri.query = uri->query; + xmluri.fragment = uri->fragment; + /* First check: does it make sense to do anything */ - if (uri->server != NULL&& - strchr(uri->server, ':') != NULL) { + if (xmluri.server != NULL&& + strchr(xmluri.server, ':') != NULL) {
- backupserver = uri->server; - if (virAsprintf(&tmpserver, "[%s]", uri->server)< 0) + if (virAsprintf(&tmpserver, "[%s]", xmluri.server)< 0) return NULL;
- uri->server = tmpserver; + xmluri.server = tmpserver; }
- ret = (char *) xmlSaveUri(uri); + ret = (char *)xmlSaveUri(&xmluri); if (!ret) { virReportOOMError(); goto cleanup; }
cleanup: - /* Put the fixed version back */ - if (tmpserver) { - uri->server = backupserver; - VIR_FREE(tmpserver); - } + VIR_FREE(tmpserver);
No use of "cleanup" yet.
return ret; } @@ -119,5 +161,12 @@ cleanup: */ void virURIFree(virURIPtr uri) { - xmlFreeURI(uri); + if (!uri) + return; + + VIR_FREE(uri->scheme); + VIR_FREE(uri->server); + VIR_FREE(uri->path); + VIR_FREE(uri->query); + VIR_FREE(uri); } diff --git a/src/util/viruri.h b/src/util/viruri.h index 5773dda..dd270de 100644 --- a/src/util/viruri.h +++ b/src/util/viruri.h @@ -13,8 +13,18 @@
# include "internal.h"
-typedef xmlURI virURI; -typedef xmlURIPtr virURIPtr; +typedef struct _virURI virURI; +typedef virURI *virURIPtr; + +struct _virURI { + char *scheme; /* the URI scheme */ + char *server; /* the server part */ + char *user; /* the user part */ + int port; /* the port number */ + char *path; /* the path string */ + char *query; /* the query string */ + char *fragment; /* the fragment string */ +};
virURIPtr virURIParse(const char *uri) ATTRIBUTE_NONNULL(1);
ACK with s/uri/xmluri/
From: "Daniel P. Berrange" <berrange@redhat.com> Avoid the need for each driver to parse query parameters itself by storing them directly in the virURIPtr struct. The parsing code is a copy of that from src/util/qparams.c The latter will be removed in a later patch * src/util/viruri.h: Add query params to virURIPtr * src/util/viruri.c: Parse query parameters when creating virURIPtr * tests/viruritest.c: Expand test to cover params --- src/libvirt_private.syms | 1 + src/util/viruri.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/viruri.h | 15 +++++ tests/viruritest.c | 46 +++++++++++++--- 4 files changed, 193 insertions(+), 8 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7cd6a96..49fb2ee 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1471,6 +1471,7 @@ virTypedParameterAssign; # viruri.h virURIFree; virURIFormat; +virURIFormatQuery; virURIParse; diff --git a/src/util/viruri.c b/src/util/viruri.c index d8618d1..f5adca5 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -13,6 +13,7 @@ #include "memory.h" #include "util.h" #include "virterror_internal.h" +#include "buf.h" #define VIR_FROM_THIS VIR_FROM_URI @@ -21,6 +22,117 @@ __FUNCTION__, __LINE__, __VA_ARGS__) +static int +virURIParamAppend(virURIPtr uri, + const char *name, + const char *value) +{ + char *pname = NULL; + char *pvalue = NULL; + + if (!(pname = strdup(name))) + goto no_memory; + if (!(pvalue = strdup (value))) + goto no_memory; + + if (VIR_RESIZE_N(uri->params, uri->paramsAlloc, uri->paramsCount, 1) < 0) + goto no_memory; + + uri->params[uri->paramsCount].name = pname; + uri->params[uri->paramsCount].value = pvalue; + uri->params[uri->paramsCount].ignore = 0; + uri->paramsCount++; + + return 0; + +no_memory: + VIR_FREE(pname); + VIR_FREE(pvalue); + virReportOOMError(); + return -1; +} + + +static int +virURIParseParams(virURIPtr uri) +{ + const char *end, *eq; + const char *query = uri->query; + + if (!query || query[0] == '\0') + return 0; + + while (*query) { + char *name = NULL, *value = NULL; + + /* Find the next separator, or end of the string. */ + end = strchr (query, '&'); + if (!end) + end = strchr (query, ';'); + if (!end) + end = query + strlen (query); + + /* Find the first '=' character between here and end. */ + eq = strchr (query, '='); + if (eq && eq >= end) eq = NULL; + + /* Empty section (eg. "&&"). */ + if (end == query) + goto next; + + /* If there is no '=' character, then we have just "name" + * and consistent with CGI.pm we assume value is "". + */ + else if (!eq) { + name = xmlURIUnescapeString (query, end - query, NULL); + if (!name) goto no_memory; + } + /* Or if we have "name=" here (works around annoying + * problem when calling xmlURIUnescapeString with len = 0). + */ + else if (eq+1 == end) { + name = xmlURIUnescapeString (query, eq - query, NULL); + if (!name) goto no_memory; + } + /* If the '=' character is at the beginning then we have + * "=value" and consistent with CGI.pm we _ignore_ this. + */ + else if (query == eq) + goto next; + + /* Otherwise it's "name=value". */ + else { + name = xmlURIUnescapeString (query, eq - query, NULL); + if (!name) + goto no_memory; + value = xmlURIUnescapeString (eq+1, end - (eq+1), NULL); + if (!value) { + VIR_FREE(name); + goto no_memory; + } + } + + /* Append to the parameter set. */ + if (virURIParamAppend(uri, name, value ? value : "") < 0) { + VIR_FREE(name); + VIR_FREE(value); + goto no_memory; + } + VIR_FREE(name); + VIR_FREE(value); + + next: + query = end; + if (*query) query ++; /* skip '&' separator */ + } + + return 0; + + no_memory: + virReportOOMError(); + return -1; +} + /** * virURIParse: * @uri: URI to parse @@ -92,12 +204,16 @@ virURIParse(const char *uri) * the uri with xmlFreeURI() */ } + if (virURIParseParams(ret) < 0) + goto error; + xmlFreeURI(xmluri); return ret; no_memory: virReportOOMError(); +error: xmlFreeURI(xmluri); virURIFree(ret); return NULL; @@ -153,6 +269,29 @@ cleanup: } +char *virURIFormatQuery(virURIPtr uri) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + int i, amp = 0; + + for (i = 0; i < uri->paramsCount; ++i) { + if (!uri->params[i].ignore) { + if (amp) virBufferAddChar (&buf, '&'); + virBufferStrcat (&buf, uri->params[i].name, "=", NULL); + virBufferURIEncodeString (&buf, uri->params[i].value); + amp = 1; + } + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + /** * virURIFree: * @uri: uri to free diff --git a/src/util/viruri.h b/src/util/viruri.h index dd270de..6fe0b2e 100644 --- a/src/util/viruri.h +++ b/src/util/viruri.h @@ -16,6 +16,15 @@ typedef struct _virURI virURI; typedef virURI *virURIPtr; +typedef struct _virURIParam virURIParam; +typedef virURIParam *virURIParamPtr; + +struct _virURIParam { + char *name; /* Name (unescaped). */ + char *value; /* Value (unescaped). */ + bool ignore; /* Ignore this field in qparam_get_query */ +}; + struct _virURI { char *scheme; /* the URI scheme */ char *server; /* the server part */ @@ -24,6 +33,10 @@ struct _virURI { char *path; /* the path string */ char *query; /* the query string */ char *fragment; /* the fragment string */ + + size_t paramsCount; + size_t paramsAlloc; + virURIParamPtr params; }; virURIPtr virURIParse(const char *uri) @@ -31,6 +44,8 @@ virURIPtr virURIParse(const char *uri) char *virURIFormat(virURIPtr uri) ATTRIBUTE_NONNULL(1); +char *virURIFormatQuery(virURIPtr uri); + void virURIFree(virURIPtr uri); #endif /* __VIR_URI_H__ */ diff --git a/tests/viruritest.c b/tests/viruritest.c index fea5ddd..1d27697 100644 --- a/tests/viruritest.c +++ b/tests/viruritest.c @@ -41,6 +41,7 @@ struct URIParseData { const char *path; const char *query; const char *fragment; + virURIParamPtr params; }; static int testURIParse(const void *args) @@ -49,6 +50,7 @@ static int testURIParse(const void *args) virURIPtr uri = NULL; const struct URIParseData *data = args; char *uristr; + size_t i; if (!(uri = virURIParse(data->uri))) goto cleanup; @@ -98,6 +100,29 @@ static int testURIParse(const void *args) goto cleanup; } + for (i = 0 ; data->params && data->params[i].name && i < uri->paramsCount ; i++) { + if (!STREQ_NULLABLE(data->params[i].name, uri->params[i].name)) { + VIR_DEBUG("Expected param name %zu '%s', actual '%s'", + i, data->params[i].name, uri->params[i].name); + goto cleanup; + } + if (!STREQ_NULLABLE(data->params[i].value, uri->params[i].value)) { + VIR_DEBUG("Expected param value %zu '%s', actual '%s'", + i, data->params[i].value, uri->params[i].value); + goto cleanup; + } + } + if (data->params && data->params[i].name) { + VIR_DEBUG("Missing parameter %zu %s=%s", + i, data->params[i].name, data->params[i].value); + goto cleanup; + } + if (i != uri->paramsCount) { + VIR_DEBUG("Unexpected parameter %zu %s=%s", + i, uri->params[i].name, uri->params[i].value); + goto cleanup; + } + ret = 0; cleanup: VIR_FREE(uristr); @@ -113,21 +138,26 @@ mymain(void) signal(SIGPIPE, SIG_IGN); -#define TEST_PARSE(uri, scheme, server, port, path, query, fragment) \ +#define TEST_PARSE(uri, scheme, server, port, path, query, fragment, params) \ do { \ const struct URIParseData data = { \ - uri, scheme, server, port, path, query, fragment \ + uri, scheme, server, port, path, query, fragment, params \ }; \ if (virtTestRun("Test IPv6 " # uri, 1, testURIParse, &data) < 0) \ ret = -1; \ } while (0) - TEST_PARSE("test://example.com", "test", "example.com", 0, NULL, NULL, NULL); - TEST_PARSE("test://example.com:123", "test", "example.com", 123, NULL, NULL, NULL); - TEST_PARSE("test://example.com:123/system?name=value#foo", "test", "example.com", 123, "/system", "name=value", "foo"); - TEST_PARSE("test://127.0.0.1:123/system", "test", "127.0.0.1", 123, "/system", NULL, NULL); - TEST_PARSE("test://[::1]:123/system", "test", "::1", 123, "/system", NULL, NULL); - TEST_PARSE("test://[2001:41c8:1:4fd4::2]:123/system", "test", "2001:41c8:1:4fd4::2", 123, "/system", NULL, NULL); + virURIParam params[] = { + { (char*)"name", (char*)"value" }, + { NULL, NULL }, + }; + + TEST_PARSE("test://example.com", "test", "example.com", 0, NULL, NULL, NULL, NULL); + TEST_PARSE("test://example.com:123", "test", "example.com", 123, NULL, NULL, NULL, NULL); + TEST_PARSE("test://example.com:123/system?name=value#foo", "test", "example.com", 123, "/system", "name=value", "foo", params); + TEST_PARSE("test://127.0.0.1:123/system", "test", "127.0.0.1", 123, "/system", NULL, NULL, NULL); + TEST_PARSE("test://[::1]:123/system", "test", "::1", 123, "/system", NULL, NULL, NULL); + TEST_PARSE("test://[2001:41c8:1:4fd4::2]:123/system", "test", "2001:41c8:1:4fd4::2", 123, "/system", NULL, NULL, NULL); return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); } -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
Avoid the need for each driver to parse query parameters itself by storing them directly in the virURIPtr struct. The parsing code is a copy of that from src/util/qparams.c The latter will be removed in a later patch
* src/util/viruri.h: Add query params to virURIPtr * src/util/viruri.c: Parse query parameters when creating virURIPtr * tests/viruritest.c: Expand test to cover params --- src/libvirt_private.syms | 1 + src/util/viruri.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/viruri.h | 15 +++++ tests/viruritest.c | 46 +++++++++++++--- 4 files changed, 193 insertions(+), 8 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7cd6a96..49fb2ee 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1471,6 +1471,7 @@ virTypedParameterAssign; # viruri.h virURIFree; virURIFormat; +virURIFormatQuery; virURIParse;
diff --git a/src/util/viruri.c b/src/util/viruri.c index d8618d1..f5adca5 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -13,6 +13,7 @@ #include "memory.h" #include "util.h" #include "virterror_internal.h" +#include "buf.h"
#define VIR_FROM_THIS VIR_FROM_URI
@@ -21,6 +22,117 @@ __FUNCTION__, __LINE__, __VA_ARGS__)
+static int +virURIParamAppend(virURIPtr uri, + const char *name, + const char *value) +{ + char *pname = NULL; + char *pvalue = NULL; + + if (!(pname = strdup(name))) + goto no_memory; + if (!(pvalue = strdup (value))) + goto no_memory; + + if (VIR_RESIZE_N(uri->params, uri->paramsAlloc, uri->paramsCount, 1)< 0) + goto no_memory; + + uri->params[uri->paramsCount].name = pname; + uri->params[uri->paramsCount].value = pvalue; + uri->params[uri->paramsCount].ignore = 0; + uri->paramsCount++; + + return 0; + +no_memory: + VIR_FREE(pname); + VIR_FREE(pvalue); + virReportOOMError(); + return -1; +} + + +static int +virURIParseParams(virURIPtr uri) +{ + const char *end, *eq; + const char *query = uri->query; + + if (!query || query[0] == '\0') + return 0; + + while (*query) { + char *name = NULL, *value = NULL; + + /* Find the next separator, or end of the string. */ + end = strchr (query, '&'); + if (!end) + end = strchr (query, ';'); + if (!end) + end = query + strlen (query); + + /* Find the first '=' character between here and end. */ + eq = strchr (query, '='); + if (eq&& eq>= end) eq = NULL; + + /* Empty section (eg. "&&"). */ + if (end == query) + goto next; + + /* If there is no '=' character, then we have just "name" + * and consistent with CGI.pm we assume value is "". + */ + else if (!eq) { + name = xmlURIUnescapeString (query, end - query, NULL); + if (!name) goto no_memory; + } + /* Or if we have "name=" here (works around annoying + * problem when calling xmlURIUnescapeString with len = 0). + */ + else if (eq+1 == end) { + name = xmlURIUnescapeString (query, eq - query, NULL); + if (!name) goto no_memory; + } + /* If the '=' character is at the beginning then we have + * "=value" and consistent with CGI.pm we _ignore_ this. + */ + else if (query == eq) + goto next; + + /* Otherwise it's "name=value". */ + else { + name = xmlURIUnescapeString (query, eq - query, NULL); + if (!name) + goto no_memory; + value = xmlURIUnescapeString (eq+1, end - (eq+1), NULL); + if (!value) { + VIR_FREE(name); + goto no_memory; + } + } + + /* Append to the parameter set. */ + if (virURIParamAppend(uri, name, value ? value : "")< 0) { + VIR_FREE(name); + VIR_FREE(value); + goto no_memory; + } + VIR_FREE(name); + VIR_FREE(value); + + next: + query = end; + if (*query) query ++; /* skip '&' separator */ + } + + return 0; + + no_memory: + virReportOOMError(); + return -1; +} + /** * virURIParse: * @uri: URI to parse @@ -92,12 +204,16 @@ virURIParse(const char *uri) * the uri with xmlFreeURI() */ }
+ if (virURIParseParams(ret)< 0) + goto error; + xmlFreeURI(xmluri);
return ret;
no_memory: virReportOOMError(); +error: xmlFreeURI(xmluri); virURIFree(ret); return NULL; @@ -153,6 +269,29 @@ cleanup: }
+char *virURIFormatQuery(virURIPtr uri)
Should we name the function as virURIFormatParams instead? Per params is used in the context everywhere, and virURIParseParams.
+{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + int i, amp = 0; + + for (i = 0; i< uri->paramsCount; ++i) { + if (!uri->params[i].ignore) { + if (amp) virBufferAddChar (&buf, '&'); + virBufferStrcat (&buf, uri->params[i].name, "=", NULL); + virBufferURIEncodeString (&buf, uri->params[i].value); + amp = 1; + } + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + /** * virURIFree: * @uri: uri to free diff --git a/src/util/viruri.h b/src/util/viruri.h index dd270de..6fe0b2e 100644 --- a/src/util/viruri.h +++ b/src/util/viruri.h @@ -16,6 +16,15 @@ typedef struct _virURI virURI; typedef virURI *virURIPtr;
+typedef struct _virURIParam virURIParam; +typedef virURIParam *virURIParamPtr; + +struct _virURIParam { + char *name; /* Name (unescaped). */ + char *value; /* Value (unescaped). */ + bool ignore; /* Ignore this field in qparam_get_query */
s/qparam_get_query/virURIFormatParams/
+}; + struct _virURI { char *scheme; /* the URI scheme */ char *server; /* the server part */ @@ -24,6 +33,10 @@ struct _virURI { char *path; /* the path string */ char *query; /* the query string */ char *fragment; /* the fragment string */ + + size_t paramsCount; + size_t paramsAlloc; + virURIParamPtr params; };
virURIPtr virURIParse(const char *uri) @@ -31,6 +44,8 @@ virURIPtr virURIParse(const char *uri) char *virURIFormat(virURIPtr uri) ATTRIBUTE_NONNULL(1);
+char *virURIFormatQuery(virURIPtr uri); + void virURIFree(virURIPtr uri);
#endif /* __VIR_URI_H__ */ diff --git a/tests/viruritest.c b/tests/viruritest.c index fea5ddd..1d27697 100644 --- a/tests/viruritest.c +++ b/tests/viruritest.c @@ -41,6 +41,7 @@ struct URIParseData { const char *path; const char *query; const char *fragment; + virURIParamPtr params; };
static int testURIParse(const void *args) @@ -49,6 +50,7 @@ static int testURIParse(const void *args) virURIPtr uri = NULL; const struct URIParseData *data = args; char *uristr; + size_t i;
if (!(uri = virURIParse(data->uri))) goto cleanup; @@ -98,6 +100,29 @@ static int testURIParse(const void *args) goto cleanup; }
+ for (i = 0 ; data->params&& data->params[i].name&& i< uri->paramsCount ; i++) { + if (!STREQ_NULLABLE(data->params[i].name, uri->params[i].name)) { + VIR_DEBUG("Expected param name %zu '%s', actual '%s'", + i, data->params[i].name, uri->params[i].name); + goto cleanup; + } + if (!STREQ_NULLABLE(data->params[i].value, uri->params[i].value)) { + VIR_DEBUG("Expected param value %zu '%s', actual '%s'", + i, data->params[i].value, uri->params[i].value); + goto cleanup; + } + } + if (data->params&& data->params[i].name) { + VIR_DEBUG("Missing parameter %zu %s=%s", + i, data->params[i].name, data->params[i].value); + goto cleanup; + } + if (i != uri->paramsCount) { + VIR_DEBUG("Unexpected parameter %zu %s=%s", + i, uri->params[i].name, uri->params[i].value); + goto cleanup; + } + ret = 0; cleanup: VIR_FREE(uristr); @@ -113,21 +138,26 @@ mymain(void)
signal(SIGPIPE, SIG_IGN);
-#define TEST_PARSE(uri, scheme, server, port, path, query, fragment) \ +#define TEST_PARSE(uri, scheme, server, port, path, query, fragment, params) \ do { \ const struct URIParseData data = { \ - uri, scheme, server, port, path, query, fragment \ + uri, scheme, server, port, path, query, fragment, params \ }; \ if (virtTestRun("Test IPv6 " # uri, 1, testURIParse,&data)< 0) \ ret = -1; \ } while (0)
- TEST_PARSE("test://example.com", "test", "example.com", 0, NULL, NULL, NULL); - TEST_PARSE("test://example.com:123", "test", "example.com", 123, NULL, NULL, NULL); - TEST_PARSE("test://example.com:123/system?name=value#foo", "test", "example.com", 123, "/system", "name=value", "foo"); - TEST_PARSE("test://127.0.0.1:123/system", "test", "127.0.0.1", 123, "/system", NULL, NULL); - TEST_PARSE("test://[::1]:123/system", "test", "::1", 123, "/system", NULL, NULL); - TEST_PARSE("test://[2001:41c8:1:4fd4::2]:123/system", "test", "2001:41c8:1:4fd4::2", 123, "/system", NULL, NULL); + virURIParam params[] = { + { (char*)"name", (char*)"value" }, + { NULL, NULL }, + }; + + TEST_PARSE("test://example.com", "test", "example.com", 0, NULL, NULL, NULL, NULL); + TEST_PARSE("test://example.com:123", "test", "example.com", 123, NULL, NULL, NULL, NULL); + TEST_PARSE("test://example.com:123/system?name=value#foo", "test", "example.com", 123, "/system", "name=value", "foo", params); + TEST_PARSE("test://127.0.0.1:123/system", "test", "127.0.0.1", 123, "/system", NULL, NULL, NULL); + TEST_PARSE("test://[::1]:123/system", "test", "::1", 123, "/system", NULL, NULL, NULL); + TEST_PARSE("test://[2001:41c8:1:4fd4::2]:123/system", "test", "2001:41c8:1:4fd4::2", 123, "/system", NULL, NULL, NULL);
return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); }
Others look fine, ACK with the function name changed and copy & paste comment fixed. Osier
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
Avoid the need for each driver to parse query parameters itself by storing them directly in the virURIPtr struct. The parsing code is a copy of that from src/util/qparams.c The latter will be removed in a later patch
* src/util/viruri.h: Add query params to virURIPtr * src/util/viruri.c: Parse query parameters when creating virURIPtr * tests/viruritest.c: Expand test to cover params --- src/libvirt_private.syms | 1 + src/util/viruri.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/viruri.h | 15 +++++ tests/viruritest.c | 46 +++++++++++++--- 4 files changed, 193 insertions(+), 8 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7cd6a96..49fb2ee 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1471,6 +1471,7 @@ virTypedParameterAssign; # viruri.h virURIFree; virURIFormat; +virURIFormatQuery; virURIParse;
diff --git a/src/util/viruri.c b/src/util/viruri.c index d8618d1..f5adca5 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -13,6 +13,7 @@ #include "memory.h" #include "util.h" #include "virterror_internal.h" +#include "buf.h"
#define VIR_FROM_THIS VIR_FROM_URI
@@ -21,6 +22,117 @@ __FUNCTION__, __LINE__, __VA_ARGS__)
+static int +virURIParamAppend(virURIPtr uri, + const char *name, + const char *value) +{ + char *pname = NULL; + char *pvalue = NULL; + + if (!(pname = strdup(name))) + goto no_memory; + if (!(pvalue = strdup (value))) + goto no_memory; + + if (VIR_RESIZE_N(uri->params, uri->paramsAlloc, uri->paramsCount, 1)< 0) + goto no_memory; + + uri->params[uri->paramsCount].name = pname; + uri->params[uri->paramsCount].value = pvalue; + uri->params[uri->paramsCount].ignore = 0; + uri->paramsCount++; + + return 0; + +no_memory: + VIR_FREE(pname); + VIR_FREE(pvalue); + virReportOOMError(); + return -1; +} + + +static int +virURIParseParams(virURIPtr uri) +{ + const char *end, *eq; + const char *query = uri->query; + + if (!query || query[0] == '\0') + return 0; + + while (*query) { + char *name = NULL, *value = NULL; + + /* Find the next separator, or end of the string. */ + end = strchr (query, '&'); + if (!end) + end = strchr (query, ';'); + if (!end) + end = query + strlen (query); + + /* Find the first '=' character between here and end. */ + eq = strchr (query, '='); + if (eq&& eq>= end) eq = NULL; + + /* Empty section (eg. "&&"). */ + if (end == query) + goto next; + + /* If there is no '=' character, then we have just "name" + * and consistent with CGI.pm we assume value is "". + */ + else if (!eq) { + name = xmlURIUnescapeString (query, end - query, NULL); + if (!name) goto no_memory; + } + /* Or if we have "name=" here (works around annoying + * problem when calling xmlURIUnescapeString with len = 0). + */ + else if (eq+1 == end) { + name = xmlURIUnescapeString (query, eq - query, NULL); + if (!name) goto no_memory; + } + /* If the '=' character is at the beginning then we have + * "=value" and consistent with CGI.pm we _ignore_ this. + */ + else if (query == eq) + goto next; + + /* Otherwise it's "name=value". */ + else { + name = xmlURIUnescapeString (query, eq - query, NULL); + if (!name) + goto no_memory; + value = xmlURIUnescapeString (eq+1, end - (eq+1), NULL); + if (!value) { + VIR_FREE(name); + goto no_memory; + } + } + + /* Append to the parameter set. */ + if (virURIParamAppend(uri, name, value ? value : "")< 0) { + VIR_FREE(name); + VIR_FREE(value); + goto no_memory; + } + VIR_FREE(name); + VIR_FREE(value); + + next: + query = end; + if (*query) query ++; /* skip '&' separator */ + } + + return 0; + + no_memory: + virReportOOMError(); + return -1; +} + /** * virURIParse: * @uri: URI to parse @@ -92,12 +204,16 @@ virURIParse(const char *uri) * the uri with xmlFreeURI() */ }
+ if (virURIParseParams(ret)< 0) + goto error; + xmlFreeURI(xmluri);
return ret;
no_memory: virReportOOMError(); +error: xmlFreeURI(xmluri); virURIFree(ret); return NULL; @@ -153,6 +269,29 @@ cleanup: }
+char *virURIFormatQuery(virURIPtr uri) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + int i, amp = 0; + + for (i = 0; i< uri->paramsCount; ++i) { + if (!uri->params[i].ignore) { + if (amp) virBufferAddChar (&buf, '&'); + virBufferStrcat (&buf, uri->params[i].name, "=", NULL); + virBufferURIEncodeString (&buf, uri->params[i].value); + amp = 1; + } + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + /** * virURIFree: * @uri: uri to free diff --git a/src/util/viruri.h b/src/util/viruri.h index dd270de..6fe0b2e 100644 --- a/src/util/viruri.h +++ b/src/util/viruri.h @@ -16,6 +16,15 @@ typedef struct _virURI virURI; typedef virURI *virURIPtr;
+typedef struct _virURIParam virURIParam; +typedef virURIParam *virURIParamPtr; + +struct _virURIParam { + char *name; /* Name (unescaped). */ + char *value; /* Value (unescaped). */ + bool ignore; /* Ignore this field in qparam_get_query */ +}; + struct _virURI { char *scheme; /* the URI scheme */ char *server; /* the server part */ @@ -24,6 +33,10 @@ struct _virURI { char *path; /* the path string */ char *query; /* the query string */ char *fragment; /* the fragment string */ + + size_t paramsCount; + size_t paramsAlloc; + virURIParamPtr params; };
virURIPtr virURIParse(const char *uri) @@ -31,6 +44,8 @@ virURIPtr virURIParse(const char *uri) char *virURIFormat(virURIPtr uri) ATTRIBUTE_NONNULL(1);
+char *virURIFormatQuery(virURIPtr uri); + void virURIFree(virURIPtr uri);
Oh, the params array is not free()'ed in virURIFree()
On 03/20/2012 11:33 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Avoid the need for each driver to parse query parameters itself by storing them directly in the virURIPtr struct. The parsing code is a copy of that from src/util/qparams.c The latter will be removed in a later patch
* src/util/viruri.h: Add query params to virURIPtr * src/util/viruri.c: Parse query parameters when creating virURIPtr * tests/viruritest.c: Expand test to cover params --- src/libvirt_private.syms | 1 + src/util/viruri.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/viruri.h | 15 +++++ tests/viruritest.c | 46 +++++++++++++--- 4 files changed, 193 insertions(+), 8 deletions(-)
This patch does not round-trip queries that include % escapes. The old qparamtest.c tried to roundtrip foo=one%20two (the literal value of 'one two' when not URI-encoded); but now virURIFormat re-escapes the % into one%2520two. I caught this with my attempt to enhance viruritest to the same level as qparamtest, but didn't want to spend any further time in fixing viruri.c. https://www.redhat.com/archives/libvir-list/2012-March/msg01043.html -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> Convert drivers currently using the qparams APIs, to instead use the virURIPtr query parameters directly. * src/esx/esx_util.c, src/hyperv/hyperv_util.c, src/remote/remote_driver.c, src/xenapi/xenapi_utils.c: Remove use of qparams * src/util/qparams.h, src/util/qparams.c: Delete * src/Makefile.am, src/libvirt_private.syms: Remove qparams --- src/Makefile.am | 1 - src/esx/esx_util.c | 17 +--- src/hyperv/hyperv_util.c | 17 +--- src/libvirt_private.syms | 6 - src/remote/remote_driver.c | 17 +--- src/util/qparams.c | 265 -------------------------------------------- src/util/qparams.h | 58 ---------- src/xenapi/xenapi_utils.c | 20 +--- 8 files changed, 9 insertions(+), 392 deletions(-) delete mode 100644 src/util/qparams.c delete mode 100644 src/util/qparams.h diff --git a/src/Makefile.am b/src/Makefile.am index e57eca2..39076cc 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -70,7 +70,6 @@ UTIL_SOURCES = \ util/pci.c util/pci.h \ util/processinfo.c util/processinfo.h \ util/hostusb.c util/hostusb.h \ - util/qparams.c util/qparams.h \ util/sexpr.c util/sexpr.h \ util/stats_linux.c util/stats_linux.h \ util/storage_file.c util/storage_file.h \ diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 67b07b7..a08ca19 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -28,7 +28,6 @@ #include "internal.h" #include "datatypes.h" -#include "qparams.h" #include "util.h" #include "memory.h" #include "logging.h" @@ -45,8 +44,6 @@ int esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri) { int result = -1; - struct qparam_set *queryParamSet = NULL; - struct qparam *queryParam = NULL; int i; int noVerify; int autoAnswer; @@ -62,14 +59,8 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri) return -1; } - queryParamSet = qparam_query_parse(uri->query); - - if (queryParamSet == NULL) { - goto cleanup; - } - - for (i = 0; i < queryParamSet->n; i++) { - queryParam = &queryParamSet->p[i]; + for (i = 0; i < uri->paramsCount; i++) { + virURIParamPtr queryParam = &uri->params[i]; if (STRCASEEQ(queryParam->name, "transport")) { VIR_FREE((*parsedUri)->transport); @@ -204,10 +195,6 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri) esxUtil_FreeParsedUri(parsedUri); } - if (queryParamSet != NULL) { - free_qparam_set(queryParamSet); - } - return result; } diff --git a/src/hyperv/hyperv_util.c b/src/hyperv/hyperv_util.c index 63c761b..81c087e 100644 --- a/src/hyperv/hyperv_util.c +++ b/src/hyperv/hyperv_util.c @@ -24,7 +24,6 @@ #include "internal.h" #include "datatypes.h" -#include "qparams.h" #include "util.h" #include "memory.h" #include "logging.h" @@ -40,8 +39,6 @@ int hypervParseUri(hypervParsedUri **parsedUri, virURIPtr uri) { int result = -1; - struct qparam_set *queryParamSet = NULL; - struct qparam *queryParam = NULL; int i; if (parsedUri == NULL || *parsedUri != NULL) { @@ -54,14 +51,8 @@ hypervParseUri(hypervParsedUri **parsedUri, virURIPtr uri) return -1; } - queryParamSet = qparam_query_parse(uri->query); - - if (queryParamSet == NULL) { - goto cleanup; - } - - for (i = 0; i < queryParamSet->n; i++) { - queryParam = &queryParamSet->p[i]; + for (i = 0; i < uri->paramsCount; i++) { + virURIParamPtr queryParam = &uri->params[i]; if (STRCASEEQ(queryParam->name, "transport")) { VIR_FREE((*parsedUri)->transport); @@ -103,10 +94,6 @@ hypervParseUri(hypervParsedUri **parsedUri, virURIPtr uri) hypervFreeParsedUri(parsedUri); } - if (queryParamSet != NULL) { - free_qparam_set(queryParamSet); - } - return result; } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 49fb2ee..8a14838 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -917,12 +917,6 @@ virProcessInfoGetAffinity; virProcessInfoSetAffinity; -# qparams.h -free_qparam_set; -qparam_get_query; -qparam_query_parse; - - # secret_conf.h virSecretDefFormat; virSecretDefFree; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9de966f..bc6fea2 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -35,7 +35,6 @@ #include "domain_event.h" #include "driver.h" #include "buf.h" -#include "qparams.h" #include "remote_driver.h" #include "remote_protocol.h" #include "qemu_protocol.h" @@ -311,7 +310,6 @@ doRemoteOpen (virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags) { - struct qparam_set *vars = NULL; char *transport_str = NULL; enum { trans_tls, @@ -415,15 +413,11 @@ doRemoteOpen (virConnectPtr conn, * feasibly it might contain variables needed by the real driver, * although that won't be the case for now). */ - struct qparam *var; int i; if (conn->uri) { - vars = qparam_query_parse (conn->uri->query); - if (vars == NULL) goto failed; - - for (i = 0; i < vars->n; i++) { - var = &vars->p[i]; + for (i = 0; i < conn->uri->paramsCount ; i++) { + virURIParamPtr var = &conn->uri->params[i]; if (STRCASEEQ (var->name, "name")) { VIR_FREE(name); name = strdup (var->value); @@ -484,7 +478,7 @@ doRemoteOpen (virConnectPtr conn, } else { virURI tmpuri = { .scheme = conn->uri->scheme, - .query = qparam_get_query (vars), + .query = virURIFormatQuery(conn->uri), .path = conn->uri->path, .fragment = conn->uri->fragment, }; @@ -507,9 +501,6 @@ doRemoteOpen (virConnectPtr conn, goto failed; } } - - free_qparam_set (vars); - vars = NULL; } else { /* Probe URI server side */ if (!(name = strdup(""))) @@ -732,8 +723,6 @@ doRemoteOpen (virConnectPtr conn, out_of_memory: virReportOOMError(); - if (vars) - free_qparam_set (vars); failed: virNetClientProgramFree(priv->remoteProgram); diff --git a/src/util/qparams.c b/src/util/qparams.c deleted file mode 100644 index 83b568e..0000000 --- a/src/util/qparams.c +++ /dev/null @@ -1,265 +0,0 @@ -/* Copyright (C) 2007, 2009-2010 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - * Authors: - * Richard W.M. Jones <rjones@redhat.com> - * - * Utility functions to help parse and assemble query strings. - */ - -#include <config.h> - -#include <stdio.h> -#include <stdlib.h> -#include <stdarg.h> - -#include "virterror_internal.h" -#include "buf.h" -#include "memory.h" -#include "qparams.h" -#include "viruri.h" - -#define VIR_FROM_THIS VIR_FROM_NONE - -struct qparam_set * -new_qparam_set (int init_alloc, ...) -{ - va_list args; - struct qparam_set *ps; - const char *pname, *pvalue; - - if (init_alloc <= 0) init_alloc = 1; - - if (VIR_ALLOC(ps) < 0) { - virReportOOMError(); - return NULL; - } - ps->n = 0; - ps->alloc = init_alloc; - if (VIR_ALLOC_N(ps->p, ps->alloc) < 0) { - VIR_FREE (ps); - virReportOOMError(); - return NULL; - } - - va_start (args, init_alloc); - while ((pname = va_arg (args, char *)) != NULL) { - pvalue = va_arg (args, char *); - - if (append_qparam (ps, pname, pvalue) == -1) { - free_qparam_set (ps); - ps = NULL; - break; - } - } - va_end (args); - - return ps; -} - -int -append_qparams (struct qparam_set *ps, ...) -{ - va_list args; - const char *pname, *pvalue; - int ret = 0; - - va_start (args, ps); - while ((pname = va_arg (args, char *)) != NULL) { - pvalue = va_arg (args, char *); - - if (append_qparam (ps, pname, pvalue) == -1) { - ret = -1; - break; - } - } - va_end (args); - - return ret; -} - -/* Ensure there is space to store at least one more parameter - * at the end of the set. - */ -static int -grow_qparam_set (struct qparam_set *ps) -{ - if (ps->n >= ps->alloc) { - if (VIR_REALLOC_N(ps->p, ps->alloc * 2) < 0) { - virReportOOMError(); - return -1; - } - ps->alloc *= 2; - } - - return 0; -} - -int -append_qparam (struct qparam_set *ps, - const char *name, const char *value) -{ - char *pname, *pvalue; - - pname = strdup (name); - if (!pname) { - virReportOOMError(); - return -1; - } - - pvalue = strdup (value); - if (!pvalue) { - VIR_FREE (pname); - virReportOOMError(); - return -1; - } - - if (grow_qparam_set (ps) == -1) { - VIR_FREE (pname); - VIR_FREE (pvalue); - return -1; - } - - ps->p[ps->n].name = pname; - ps->p[ps->n].value = pvalue; - ps->p[ps->n].ignore = 0; - ps->n++; - - return 0; -} - -char * -qparam_get_query (const struct qparam_set *ps) -{ - virBuffer buf = VIR_BUFFER_INITIALIZER; - int i, amp = 0; - - for (i = 0; i < ps->n; ++i) { - if (!ps->p[i].ignore) { - if (amp) virBufferAddChar (&buf, '&'); - virBufferStrcat (&buf, ps->p[i].name, "=", NULL); - virBufferURIEncodeString (&buf, ps->p[i].value); - amp = 1; - } - } - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - - return virBufferContentAndReset(&buf); -} - -void -free_qparam_set (struct qparam_set *ps) -{ - int i; - - for (i = 0; i < ps->n; ++i) { - VIR_FREE (ps->p[i].name); - VIR_FREE (ps->p[i].value); - } - VIR_FREE (ps->p); - VIR_FREE (ps); -} - -struct qparam_set * -qparam_query_parse (const char *query) -{ - struct qparam_set *ps; - const char *end, *eq; - - ps = new_qparam_set (0, NULL); - if (!ps) { - virReportOOMError(); - return NULL; - } - - if (!query || query[0] == '\0') return ps; - - while (*query) { - char *name = NULL, *value = NULL; - - /* Find the next separator, or end of the string. */ - end = strchr (query, '&'); - if (!end) - end = strchr (query, ';'); - if (!end) - end = query + strlen (query); - - /* Find the first '=' character between here and end. */ - eq = strchr (query, '='); - if (eq && eq >= end) eq = NULL; - - /* Empty section (eg. "&&"). */ - if (end == query) - goto next; - - /* If there is no '=' character, then we have just "name" - * and consistent with CGI.pm we assume value is "". - */ - else if (!eq) { - name = xmlURIUnescapeString (query, end - query, NULL); - if (!name) goto out_of_memory; - } - /* Or if we have "name=" here (works around annoying - * problem when calling xmlURIUnescapeString with len = 0). - */ - else if (eq+1 == end) { - name = xmlURIUnescapeString (query, eq - query, NULL); - if (!name) goto out_of_memory; - } - /* If the '=' character is at the beginning then we have - * "=value" and consistent with CGI.pm we _ignore_ this. - */ - else if (query == eq) - goto next; - - /* Otherwise it's "name=value". */ - else { - name = xmlURIUnescapeString (query, eq - query, NULL); - if (!name) - goto out_of_memory; - value = xmlURIUnescapeString (eq+1, end - (eq+1), NULL); - if (!value) { - VIR_FREE(name); - goto out_of_memory; - } - } - - /* Append to the parameter set. */ - if (append_qparam (ps, name, value ? value : "") == -1) { - VIR_FREE(name); - VIR_FREE(value); - goto out_of_memory; - } - VIR_FREE(name); - VIR_FREE(value); - - next: - query = end; - if (*query) query ++; /* skip '&' separator */ - } - - return ps; - - out_of_memory: - virReportOOMError(); - free_qparam_set (ps); - return NULL; -} diff --git a/src/util/qparams.h b/src/util/qparams.h deleted file mode 100644 index 6a3d790..0000000 --- a/src/util/qparams.h +++ /dev/null @@ -1,58 +0,0 @@ -/* Copyright (C) 2007 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - * Authors: - * Richard W.M. Jones <rjones@redhat.com> - * - * Utility functions to help parse and assemble query strings. - */ - -#ifndef _QPARAMS_H_ -# define _QPARAMS_H_ - -/* Single web service query parameter 'name=value'. */ -struct qparam { - char *name; /* Name (unescaped). */ - char *value; /* Value (unescaped). */ - int ignore; /* Ignore this field in qparam_get_query */ -}; - -/* Set of parameters. */ -struct qparam_set { - int n; /* number of parameters used */ - int alloc; /* allocated space */ - struct qparam *p; /* array of parameters */ -}; - -/* New parameter set. */ -extern struct qparam_set *new_qparam_set (int init_alloc, ...) - ATTRIBUTE_SENTINEL; - -/* Appending parameters. */ -extern int append_qparams (struct qparam_set *ps, ...) - ATTRIBUTE_SENTINEL; -extern int append_qparam (struct qparam_set *ps, - const char *name, const char *value); - -/* Get a query string ("name=value&name=value&...") */ -extern char *qparam_get_query (const struct qparam_set *ps); - -/* Parse a query string into a parameter set. */ -extern struct qparam_set *qparam_query_parse (const char *query); - -extern void free_qparam_set (struct qparam_set *ps); - -#endif /* _QPARAMS_H_ */ diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index 943b6c0..516cf8f 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -35,7 +35,6 @@ #include "memory.h" #include "buf.h" #include "logging.h" -#include "qparams.h" #include "viruri.h" #include "xenapi_driver_private.h" #include "xenapi_utils.h" @@ -98,21 +97,9 @@ xenapiUtil_ParseQuery(virConnectPtr conn, virURIPtr uri, int *noVerify) { int result = 0; int i; - struct qparam_set *queryParamSet = NULL; - struct qparam *queryParam = NULL; -#ifdef HAVE_XMLURI_QUERY_RAW - queryParamSet = qparam_query_parse(uri->query_raw); -#else - queryParamSet = qparam_query_parse(uri->query); -#endif - - if (queryParamSet == NULL) { - goto failure; - } - - for (i = 0; i < queryParamSet->n; i++) { - queryParam = &queryParamSet->p[i]; + for (i = 0; i < uri->paramsCount; i++) { + virURIParamPtr queryParam = &uri->params[i]; if (STRCASEEQ(queryParam->name, "no_verify")) { if (noVerify == NULL) { continue; @@ -127,9 +114,6 @@ xenapiUtil_ParseQuery(virConnectPtr conn, virURIPtr uri, int *noVerify) } cleanup: - if (queryParamSet != NULL) { - free_qparam_set(queryParamSet); - } return result; -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
Convert drivers currently using the qparams APIs, to instead use the virURIPtr query parameters directly.
* src/esx/esx_util.c, src/hyperv/hyperv_util.c, src/remote/remote_driver.c, src/xenapi/xenapi_utils.c: Remove use of qparams * src/util/qparams.h, src/util/qparams.c: Delete * src/Makefile.am, src/libvirt_private.syms: Remove qparams --- src/Makefile.am | 1 - src/esx/esx_util.c | 17 +--- src/hyperv/hyperv_util.c | 17 +--- src/libvirt_private.syms | 6 - src/remote/remote_driver.c | 17 +--- src/util/qparams.c | 265 -------------------------------------------- src/util/qparams.h | 58 ---------- src/xenapi/xenapi_utils.c | 20 +--- 8 files changed, 9 insertions(+), 392 deletions(-) delete mode 100644 src/util/qparams.c delete mode 100644 src/util/qparams.h
diff --git a/src/Makefile.am b/src/Makefile.am index e57eca2..39076cc 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -70,7 +70,6 @@ UTIL_SOURCES = \ util/pci.c util/pci.h \ util/processinfo.c util/processinfo.h \ util/hostusb.c util/hostusb.h \ - util/qparams.c util/qparams.h \ util/sexpr.c util/sexpr.h \ util/stats_linux.c util/stats_linux.h \ util/storage_file.c util/storage_file.h \ diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 67b07b7..a08ca19 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -28,7 +28,6 @@
#include "internal.h" #include "datatypes.h" -#include "qparams.h" #include "util.h" #include "memory.h" #include "logging.h" @@ -45,8 +44,6 @@ int esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri) { int result = -1; - struct qparam_set *queryParamSet = NULL; - struct qparam *queryParam = NULL; int i; int noVerify; int autoAnswer; @@ -62,14 +59,8 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri) return -1; }
- queryParamSet = qparam_query_parse(uri->query); - - if (queryParamSet == NULL) { - goto cleanup; - } - - for (i = 0; i< queryParamSet->n; i++) { - queryParam =&queryParamSet->p[i]; + for (i = 0; i< uri->paramsCount; i++) { + virURIParamPtr queryParam =&uri->params[i];
if (STRCASEEQ(queryParam->name, "transport")) { VIR_FREE((*parsedUri)->transport); @@ -204,10 +195,6 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri) esxUtil_FreeParsedUri(parsedUri); }
- if (queryParamSet != NULL) { - free_qparam_set(queryParamSet); - } - return result; }
diff --git a/src/hyperv/hyperv_util.c b/src/hyperv/hyperv_util.c index 63c761b..81c087e 100644 --- a/src/hyperv/hyperv_util.c +++ b/src/hyperv/hyperv_util.c @@ -24,7 +24,6 @@
#include "internal.h" #include "datatypes.h" -#include "qparams.h" #include "util.h" #include "memory.h" #include "logging.h" @@ -40,8 +39,6 @@ int hypervParseUri(hypervParsedUri **parsedUri, virURIPtr uri) { int result = -1; - struct qparam_set *queryParamSet = NULL; - struct qparam *queryParam = NULL; int i;
if (parsedUri == NULL || *parsedUri != NULL) { @@ -54,14 +51,8 @@ hypervParseUri(hypervParsedUri **parsedUri, virURIPtr uri) return -1; }
- queryParamSet = qparam_query_parse(uri->query); - - if (queryParamSet == NULL) { - goto cleanup; - } - - for (i = 0; i< queryParamSet->n; i++) { - queryParam =&queryParamSet->p[i]; + for (i = 0; i< uri->paramsCount; i++) { + virURIParamPtr queryParam =&uri->params[i];
if (STRCASEEQ(queryParam->name, "transport")) { VIR_FREE((*parsedUri)->transport); @@ -103,10 +94,6 @@ hypervParseUri(hypervParsedUri **parsedUri, virURIPtr uri) hypervFreeParsedUri(parsedUri); }
- if (queryParamSet != NULL) { - free_qparam_set(queryParamSet); - } - return result; }
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 49fb2ee..8a14838 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -917,12 +917,6 @@ virProcessInfoGetAffinity; virProcessInfoSetAffinity;
-# qparams.h -free_qparam_set; -qparam_get_query; -qparam_query_parse; - - # secret_conf.h virSecretDefFormat; virSecretDefFree; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9de966f..bc6fea2 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -35,7 +35,6 @@ #include "domain_event.h" #include "driver.h" #include "buf.h" -#include "qparams.h" #include "remote_driver.h" #include "remote_protocol.h" #include "qemu_protocol.h" @@ -311,7 +310,6 @@ doRemoteOpen (virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags) { - struct qparam_set *vars = NULL; char *transport_str = NULL; enum { trans_tls, @@ -415,15 +413,11 @@ doRemoteOpen (virConnectPtr conn, * feasibly it might contain variables needed by the real driver, * although that won't be the case for now). */ - struct qparam *var; int i;
if (conn->uri) { - vars = qparam_query_parse (conn->uri->query); - if (vars == NULL) goto failed; - - for (i = 0; i< vars->n; i++) { - var =&vars->p[i]; + for (i = 0; i< conn->uri->paramsCount ; i++) { + virURIParamPtr var =&conn->uri->params[i]; if (STRCASEEQ (var->name, "name")) { VIR_FREE(name); name = strdup (var->value); @@ -484,7 +478,7 @@ doRemoteOpen (virConnectPtr conn, } else { virURI tmpuri = { .scheme = conn->uri->scheme, - .query = qparam_get_query (vars), + .query = virURIFormatQuery(conn->uri), .path = conn->uri->path, .fragment = conn->uri->fragment, }; @@ -507,9 +501,6 @@ doRemoteOpen (virConnectPtr conn, goto failed; } } - - free_qparam_set (vars); - vars = NULL; } else { /* Probe URI server side */ if (!(name = strdup(""))) @@ -732,8 +723,6 @@ doRemoteOpen (virConnectPtr conn,
out_of_memory: virReportOOMError(); - if (vars) - free_qparam_set (vars);
failed: virNetClientProgramFree(priv->remoteProgram); diff --git a/src/util/qparams.c b/src/util/qparams.c deleted file mode 100644 index 83b568e..0000000 --- a/src/util/qparams.c +++ /dev/null @@ -1,265 +0,0 @@ -/* Copyright (C) 2007, 2009-2010 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - * Authors: - * Richard W.M. Jones<rjones@redhat.com> - * - * Utility functions to help parse and assemble query strings. - */ - -#include<config.h> - -#include<stdio.h> -#include<stdlib.h> -#include<stdarg.h> - -#include "virterror_internal.h" -#include "buf.h" -#include "memory.h" -#include "qparams.h" -#include "viruri.h" - -#define VIR_FROM_THIS VIR_FROM_NONE - -struct qparam_set * -new_qparam_set (int init_alloc, ...) -{ - va_list args; - struct qparam_set *ps; - const char *pname, *pvalue; - - if (init_alloc<= 0) init_alloc = 1; - - if (VIR_ALLOC(ps)< 0) { - virReportOOMError(); - return NULL; - } - ps->n = 0; - ps->alloc = init_alloc; - if (VIR_ALLOC_N(ps->p, ps->alloc)< 0) { - VIR_FREE (ps); - virReportOOMError(); - return NULL; - } - - va_start (args, init_alloc); - while ((pname = va_arg (args, char *)) != NULL) { - pvalue = va_arg (args, char *); - - if (append_qparam (ps, pname, pvalue) == -1) { - free_qparam_set (ps); - ps = NULL; - break; - } - } - va_end (args); - - return ps; -} - -int -append_qparams (struct qparam_set *ps, ...) -{ - va_list args; - const char *pname, *pvalue; - int ret = 0; - - va_start (args, ps); - while ((pname = va_arg (args, char *)) != NULL) { - pvalue = va_arg (args, char *); - - if (append_qparam (ps, pname, pvalue) == -1) { - ret = -1; - break; - } - } - va_end (args); - - return ret; -} - -/* Ensure there is space to store at least one more parameter - * at the end of the set. - */ -static int -grow_qparam_set (struct qparam_set *ps) -{ - if (ps->n>= ps->alloc) { - if (VIR_REALLOC_N(ps->p, ps->alloc * 2)< 0) { - virReportOOMError(); - return -1; - } - ps->alloc *= 2; - } - - return 0; -} - -int -append_qparam (struct qparam_set *ps, - const char *name, const char *value) -{ - char *pname, *pvalue; - - pname = strdup (name); - if (!pname) { - virReportOOMError(); - return -1; - } - - pvalue = strdup (value); - if (!pvalue) { - VIR_FREE (pname); - virReportOOMError(); - return -1; - } - - if (grow_qparam_set (ps) == -1) { - VIR_FREE (pname); - VIR_FREE (pvalue); - return -1; - } - - ps->p[ps->n].name = pname; - ps->p[ps->n].value = pvalue; - ps->p[ps->n].ignore = 0; - ps->n++; - - return 0; -} - -char * -qparam_get_query (const struct qparam_set *ps) -{ - virBuffer buf = VIR_BUFFER_INITIALIZER; - int i, amp = 0; - - for (i = 0; i< ps->n; ++i) { - if (!ps->p[i].ignore) { - if (amp) virBufferAddChar (&buf, '&'); - virBufferStrcat (&buf, ps->p[i].name, "=", NULL); - virBufferURIEncodeString (&buf, ps->p[i].value); - amp = 1; - } - } - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - - return virBufferContentAndReset(&buf); -} - -void -free_qparam_set (struct qparam_set *ps) -{ - int i; - - for (i = 0; i< ps->n; ++i) { - VIR_FREE (ps->p[i].name); - VIR_FREE (ps->p[i].value); - } - VIR_FREE (ps->p); - VIR_FREE (ps); -} - -struct qparam_set * -qparam_query_parse (const char *query) -{ - struct qparam_set *ps; - const char *end, *eq; - - ps = new_qparam_set (0, NULL); - if (!ps) { - virReportOOMError(); - return NULL; - } - - if (!query || query[0] == '\0') return ps; - - while (*query) { - char *name = NULL, *value = NULL; - - /* Find the next separator, or end of the string. */ - end = strchr (query, '&'); - if (!end) - end = strchr (query, ';'); - if (!end) - end = query + strlen (query); - - /* Find the first '=' character between here and end. */ - eq = strchr (query, '='); - if (eq&& eq>= end) eq = NULL; - - /* Empty section (eg. "&&"). */ - if (end == query) - goto next; - - /* If there is no '=' character, then we have just "name" - * and consistent with CGI.pm we assume value is "". - */ - else if (!eq) { - name = xmlURIUnescapeString (query, end - query, NULL); - if (!name) goto out_of_memory; - } - /* Or if we have "name=" here (works around annoying - * problem when calling xmlURIUnescapeString with len = 0). - */ - else if (eq+1 == end) { - name = xmlURIUnescapeString (query, eq - query, NULL); - if (!name) goto out_of_memory; - } - /* If the '=' character is at the beginning then we have - * "=value" and consistent with CGI.pm we _ignore_ this. - */ - else if (query == eq) - goto next; - - /* Otherwise it's "name=value". */ - else { - name = xmlURIUnescapeString (query, eq - query, NULL); - if (!name) - goto out_of_memory; - value = xmlURIUnescapeString (eq+1, end - (eq+1), NULL); - if (!value) { - VIR_FREE(name); - goto out_of_memory; - } - } - - /* Append to the parameter set. */ - if (append_qparam (ps, name, value ? value : "") == -1) { - VIR_FREE(name); - VIR_FREE(value); - goto out_of_memory; - } - VIR_FREE(name); - VIR_FREE(value); - - next: - query = end; - if (*query) query ++; /* skip '&' separator */ - } - - return ps; - - out_of_memory: - virReportOOMError(); - free_qparam_set (ps); - return NULL; -} diff --git a/src/util/qparams.h b/src/util/qparams.h deleted file mode 100644 index 6a3d790..0000000 --- a/src/util/qparams.h +++ /dev/null @@ -1,58 +0,0 @@ -/* Copyright (C) 2007 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - * Authors: - * Richard W.M. Jones<rjones@redhat.com> - * - * Utility functions to help parse and assemble query strings. - */ - -#ifndef _QPARAMS_H_ -# define _QPARAMS_H_ - -/* Single web service query parameter 'name=value'. */ -struct qparam { - char *name; /* Name (unescaped). */ - char *value; /* Value (unescaped). */ - int ignore; /* Ignore this field in qparam_get_query */ -}; - -/* Set of parameters. */ -struct qparam_set { - int n; /* number of parameters used */ - int alloc; /* allocated space */ - struct qparam *p; /* array of parameters */ -}; - -/* New parameter set. */ -extern struct qparam_set *new_qparam_set (int init_alloc, ...) - ATTRIBUTE_SENTINEL; - -/* Appending parameters. */ -extern int append_qparams (struct qparam_set *ps, ...) - ATTRIBUTE_SENTINEL; -extern int append_qparam (struct qparam_set *ps, - const char *name, const char *value); - -/* Get a query string ("name=value&name=value&...") */ -extern char *qparam_get_query (const struct qparam_set *ps); - -/* Parse a query string into a parameter set. */ -extern struct qparam_set *qparam_query_parse (const char *query); - -extern void free_qparam_set (struct qparam_set *ps); - -#endif /* _QPARAMS_H_ */ diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index 943b6c0..516cf8f 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -35,7 +35,6 @@ #include "memory.h" #include "buf.h" #include "logging.h" -#include "qparams.h" #include "viruri.h" #include "xenapi_driver_private.h" #include "xenapi_utils.h" @@ -98,21 +97,9 @@ xenapiUtil_ParseQuery(virConnectPtr conn, virURIPtr uri, int *noVerify) { int result = 0; int i; - struct qparam_set *queryParamSet = NULL; - struct qparam *queryParam = NULL;
-#ifdef HAVE_XMLURI_QUERY_RAW - queryParamSet = qparam_query_parse(uri->query_raw); -#else - queryParamSet = qparam_query_parse(uri->query); -#endif - - if (queryParamSet == NULL) { - goto failure; - } - - for (i = 0; i< queryParamSet->n; i++) { - queryParam =&queryParamSet->p[i]; + for (i = 0; i< uri->paramsCount; i++) { + virURIParamPtr queryParam =&uri->params[i]; if (STRCASEEQ(queryParam->name, "no_verify")) { if (noVerify == NULL) { continue; @@ -127,9 +114,6 @@ xenapiUtil_ParseQuery(virConnectPtr conn, virURIPtr uri, int *noVerify) }
cleanup: - if (queryParamSet != NULL) { - free_qparam_set(queryParamSet); - }
return result;
Looks fine, ACK
From: "Daniel P. Berrange" <berrange@redhat.com> The '.ini' file format is a useful alternative to the existing config file style, when you need to have config files which are hashes of hashes. The 'virKeyFilePtr' object provides a way to parse these file types. * src/Makefile.am, src/util/virkeyfile.c, src/util/virkeyfile.h: Add .ini file parser * tests/Makefile.am, tests/virkeyfiletest.c: Test basic parsing capabilities --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 10 ++ src/util/virkeyfile.c | 367 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virkeyfile.h | 64 ++++++++ tests/Makefile.am | 8 +- tests/virkeyfiletest.c | 123 ++++++++++++++++ 7 files changed, 573 insertions(+), 1 deletions(-) create mode 100644 src/util/virkeyfile.c create mode 100644 src/util/virkeyfile.h create mode 100644 tests/virkeyfiletest.c diff --git a/po/POTFILES.in b/po/POTFILES.in index 16a3f9e..8354c09 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -127,6 +127,7 @@ src/util/util.c src/util/viraudit.c src/util/virfile.c src/util/virhash.c +src/util/virkeyfile.c src/util/virnetdev.c src/util/virnetdevbridge.c src/util/virnetdevmacvlan.c diff --git a/src/Makefile.am b/src/Makefile.am index 39076cc..07d7faa 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -90,6 +90,7 @@ UTIL_SOURCES = \ util/virhash.c util/virhash.h \ util/virhashcode.c util/virhashcode.h \ util/virkeycode.c util/virkeycode.h \ + util/virkeyfile.c util/virkeyfile.h \ util/virkeymaps.h \ util/virmacaddr.h util/virmacaddr.c \ util/virnetdev.h util/virnetdev.c \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8a14838..3f69ec1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1198,6 +1198,16 @@ virKeycodeValueFromString; virKeycodeValueTranslate; +# virkeyfile.h +virKeyFileNew; +virKeyFileLoadFile; +virKeyFileLoadData; +virKeyFileFree; +virKeyFileHasValue; +virKeyFileHasGroup; +virKeyFileGetValueString; + + # virmacaddr.h virMacAddrCompare; virMacAddrFormat; diff --git a/src/util/virkeyfile.c b/src/util/virkeyfile.c new file mode 100644 index 0000000..3dd4960 --- /dev/null +++ b/src/util/virkeyfile.c @@ -0,0 +1,367 @@ +/* + * virkeyfile.c: "ini"-style configuration file handling + * + * Copyright (C) 2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Authors: + * Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include <stdio.h> + +#include "c-ctype.h" +#include "logging.h" +#include "memory.h" +#include "util.h" +#include "virhash.h" +#include "virkeyfile.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_CONF + +typedef struct _virKeyFileGroup virKeyFileGroup; +typedef virKeyFileGroup *virKeyFileGroupPtr; + +typedef struct _virKeyFileParserCtxt virKeyFileParserCtxt; +typedef virKeyFileParserCtxt *virKeyFileParserCtxtPtr; + +struct _virKeyFile { + virHashTablePtr groups; +}; + +struct _virKeyFileParserCtxt { + virKeyFilePtr conf; + + const char *filename; + + const char *base; + const char *cur; + const char *end; + size_t line; + + char *groupname; + virHashTablePtr group; +}; + +/* + * The grammar for the keyfile + * + * KEYFILE = (GROUP | COMMENT | BLANK )* + * + * COMMENT = ('#' | ';') [^\n]* '\n' + * BLANK = (' ' | '\t' )* '\n' + * + * GROUP = '[' GROUPNAME ']' '\n' (ENTRY ) * + * GROUPNAME = [^[]\n]+ + * + * ENTRY = KEYNAME '=' VALUE + * VALUE = [^\n]* '\n' + * KEYNAME = [-a-zA-Z0-9]+ + */ + +#define IS_EOF (ctxt->cur >= ctxt->end) +#define IS_EOL(c) (((c) == '\n') || ((c) == '\r')) +#define CUR (*ctxt->cur) +#define NEXT if (!IS_EOF) ctxt->cur++; + + +#define virKeyFileError(ctxt, error, info) \ + virKeyFileErrorHelper(__FILE__, __FUNCTION__, __LINE__, ctxt, error, info) +static void +virKeyFileErrorHelper(const char *file, const char *func, size_t line, + virKeyFileParserCtxtPtr ctxt, + virErrorNumber error, const char *info) +{ + /* Construct the string 'filename:line: info' if we have that. */ + if (ctxt && ctxt->filename) { + virReportErrorHelper(VIR_FROM_CONF, error, file, func, line, + _("%s:%zu: %s '%s'"), ctxt->filename, ctxt->line, info, ctxt->cur); + } else { + virReportErrorHelper(VIR_FROM_CONF, error, file, func, line, + "%s", info); + } +} + + +static void virKeyFileValueFree(void *value, const void *name ATTRIBUTE_UNUSED) +{ + VIR_FREE(value); +} + +static int virKeyFileParseGroup(virKeyFileParserCtxtPtr ctxt) +{ + int ret = -1; + const char *name; + NEXT; + + ctxt->group = NULL; + VIR_FREE(ctxt->groupname); + + name = ctxt->cur; + while (!IS_EOF && c_isascii(CUR) && CUR != ']') + ctxt->cur++; + if (CUR != ']') { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "cannot find end of group name, expected ']'"); + return -1; + } + + if (!(ctxt->groupname = strndup(name, ctxt->cur - name))) { + virReportOOMError(); + return -1; + } + + NEXT; + + if (!(ctxt->group = virHashCreate(10, virKeyFileValueFree))) + goto cleanup; + + if (virHashAddEntry(ctxt->conf->groups, ctxt->groupname, ctxt->group) < 0) + goto cleanup; + + ret = 0; +cleanup: + if (ret != 0) { + virHashFree(ctxt->group); + ctxt->group = NULL; + VIR_FREE(ctxt->groupname); + } + + return ret; +} + +static int virKeyFileParseValue(virKeyFileParserCtxtPtr ctxt) +{ + int ret = -1; + const char *keystart; + const char *valuestart; + char *key = NULL; + char *value = NULL; + size_t len; + + if (!ctxt->groupname || !ctxt->group) { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "value found before first group"); + return -1; + } + + keystart = ctxt->cur; + while (!IS_EOF && c_isalnum(CUR) && CUR != '=') + ctxt->cur++; + if (CUR != '=') { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "expected end of value name, expected '='"); + return -1; + } + + if (!(key = strndup(keystart, ctxt->cur - keystart))) { + virReportOOMError(); + return -1; + } + + NEXT; + valuestart = ctxt->cur; + while (!IS_EOF && !IS_EOL(CUR)) + ctxt->cur++; + if (!(IS_EOF || IS_EOL(CUR))) { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "unexpected end of value"); + goto cleanup; + } + len = ctxt->cur - valuestart; + if (IS_EOF && !IS_EOL(CUR)) + len++; + if (!(value = strndup(valuestart, len))) { + virReportOOMError(); + goto cleanup; + } + + if (virHashAddEntry(ctxt->group, key, value) < 0) + goto cleanup; + + NEXT; + + ret = 0; + +cleanup: + VIR_FREE(key); + return ret; +} + +static int virKeyFileParseComment(virKeyFileParserCtxtPtr ctxt) +{ + NEXT; + + while (!IS_EOF && !IS_EOL(CUR)) + ctxt->cur++; + + NEXT; + + return 0; +} + +static int virKeyFileParseBlank(virKeyFileParserCtxtPtr ctxt) +{ + while ((ctxt->cur < ctxt->end) && c_isblank(CUR)) + ctxt->cur++; + + if (!((ctxt->cur == ctxt->end) || IS_EOL(CUR))) { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "expected newline"); + return -1; + } + NEXT; + return 0; +} + +static int virKeyFileParseStatement(virKeyFileParserCtxtPtr ctxt) +{ + int ret = -1; + + if (CUR == '[') { + ret = virKeyFileParseGroup(ctxt); + } else if (c_isalnum(CUR)) { + ret = virKeyFileParseValue(ctxt); + } else if (CUR == '#' || CUR == ';') { + ret = virKeyFileParseComment(ctxt); + } else if (c_isblank(CUR) || IS_EOL(CUR)) { + ret = virKeyFileParseBlank(ctxt); + } else { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "unexpected statement"); + } + + return ret; +} + +static int virKeyFileParse(virKeyFilePtr conf, + const char *filename, + const char *data, + size_t len) +{ + virKeyFileParserCtxt ctxt; + int ret = -1; + + VIR_DEBUG("Parse %p '%s' %p %zu", conf, filename, data, len); + + memset(&ctxt, 0, sizeof(ctxt)); + + ctxt.filename = filename; + ctxt.base = ctxt.cur = data; + ctxt.end = data + len - 1; + ctxt.line = 1; + ctxt.conf = conf; + + while (ctxt.cur < ctxt.end) { + if (virKeyFileParseStatement(&ctxt) < 0) + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(ctxt.groupname); + return ret; +} + + +static void virKeyFileEntryFree(void *payload, const void *name ATTRIBUTE_UNUSED) +{ + virHashFree(payload); +} + + +virKeyFilePtr virKeyFileNew(void) +{ + virKeyFilePtr conf; + + if (VIR_ALLOC(conf) < 0) { + virReportOOMError(); + goto error; + } + + if (!(conf->groups = virHashCreate(10, + virKeyFileEntryFree))) + goto error; + + return conf; + +error: + virKeyFileFree(conf); + return NULL; +} + + +#define MAX_CONFIG_FILE_SIZE (1024 * 1024) + +int virKeyFileLoadFile(virKeyFilePtr conf, + const char *filename) +{ + char *data = NULL; + ssize_t len; + int ret; + + if ((len = virFileReadAll(filename, MAX_CONFIG_FILE_SIZE, &data)) < 0) + return -1; + + ret = virKeyFileParse(conf, filename, data, len); + + VIR_FREE(data); + + return ret; +} + + +int virKeyFileLoadData(virKeyFilePtr conf, + const char *path, + const char *data, + size_t len) +{ + return virKeyFileParse(conf, path, data, len); +} + + +void virKeyFileFree(virKeyFilePtr conf) +{ + if (!conf) + return; + + virHashFree(conf->groups); + VIR_FREE(conf); +} + + +bool virKeyFileHasGroup(virKeyFilePtr conf, + const char *groupname) +{ + VIR_DEBUG("conf=%p groupname=%s", conf, groupname); + return virHashLookup(conf->groups, groupname) != NULL; +} + + +bool virKeyFileHasValue(virKeyFilePtr conf, + const char *groupname, + const char *valuename) +{ + virHashTablePtr group = virHashLookup(conf->groups, groupname); + VIR_DEBUG("conf=%p groupname=%s valuename=%s", conf, groupname, valuename); + return group && virHashLookup(group, valuename) != NULL; +} + +const char *virKeyFileGetValueString(virKeyFilePtr conf, + const char *groupname, + const char *valuename) +{ + virHashTablePtr group = virHashLookup(conf->groups, groupname); + VIR_DEBUG("conf=%p groupname=%s valuename=%s", conf, groupname, valuename); + return virHashLookup(group, valuename); +} diff --git a/src/util/virkeyfile.h b/src/util/virkeyfile.h new file mode 100644 index 0000000..098ef59 --- /dev/null +++ b/src/util/virkeyfile.h @@ -0,0 +1,64 @@ +/* + * virkeyfile.h: "ini"-style configuration file handling + * + * Copyright (C) 2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Authors: + * Daniel P. Berrange <berrange@redhat.com> + */ + +#ifndef __VIR_KEYFILE_H__ +# define __VIR_KEYFILE_H__ + +# include "internal.h" + +/** + * virKeyFilePtr: + * a pointer to a parsed configuration file + */ +typedef struct _virKeyFile virKeyFile; +typedef virKeyFile *virKeyFilePtr; + +virKeyFilePtr virKeyFileNew(void); + +int virKeyFileLoadFile(virKeyFilePtr conf, + const char *filename) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + +int virKeyFileLoadData(virKeyFilePtr conf, + const char *filename, + const char *data, + size_t len) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + +void virKeyFileFree(virKeyFilePtr conf); + +bool virKeyFileHasGroup(virKeyFilePtr conf, + const char *groupname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +bool virKeyFileHasValue(virKeyFilePtr conf, + const char *groupname, + const char *valuename) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + +const char *virKeyFileGetValueString(virKeyFilePtr conf, + const char *groupname, + const char *valuename) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + +#endif /* __VIR_KEYFILE_H__ */ diff --git a/tests/Makefile.am b/tests/Makefile.am index 035c8c6..4dde3e9 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -96,7 +96,7 @@ check_PROGRAMS = virshtest conftest sockettest \ commandtest commandhelper seclabeltest \ virhashtest virnetmessagetest virnetsockettest ssh \ utiltest virnettlscontexttest shunloadtest \ - virtimetest viruritest + virtimetest viruritest virkeyfiletest check_LTLIBRARIES = libshunload.la @@ -220,6 +220,7 @@ TESTS = virshtest \ virnettlscontexttest \ virtimetest \ viruritest \ + virkeyfiletest \ shunloadtest \ utiltest \ $(test_scripts) @@ -512,6 +513,11 @@ viruritest_SOURCES = \ viruritest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) viruritest_LDADD = ../src/libvirt-net-rpc.la $(LDADDS) +virkeyfiletest_SOURCES = \ + virkeyfiletest.c testutils.h testutils.c +virkeyfiletest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) +virkeyfiletest_LDADD = ../src/libvirt-net-rpc.la $(LDADDS) + seclabeltest_SOURCES = \ seclabeltest.c seclabeltest_LDADD = ../src/libvirt_driver_security.la $(LDADDS) diff --git a/tests/virkeyfiletest.c b/tests/virkeyfiletest.c new file mode 100644 index 0000000..34dd267 --- /dev/null +++ b/tests/virkeyfiletest.c @@ -0,0 +1,123 @@ +/* + * Copyright (C) 2011 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include <stdlib.h> +#include <signal.h> + +#include "testutils.h" +#include "util.h" +#include "virterror_internal.h" +#include "memory.h" +#include "logging.h" + +#include "virkeyfile.h" + +#define VIR_FROM_THIS VIR_FROM_RPC + + +static int testParse(const void *args ATTRIBUTE_UNUSED) +{ + static const char *cfg1 = + "# Some config\n" + "\n" + "# The first group\n" + "[Foo]\n" + "one=The first entry is here\n" + "two=The second entry\n" + " \n" + "three=The third entry\n" + "[Bar]\n" + "; Another comment\n" + "one=The first entry in second group"; + virKeyFilePtr kf = virKeyFileNew(); + int ret = -1; + + if (virKeyFileLoadData(kf, "demo.conf", cfg1, strlen(cfg1)) < 0) + goto cleanup; + + if (!virKeyFileHasGroup(kf, "Foo")) { + VIR_DEBUG("Missing group 'Foo'"); + goto cleanup; + } + if (!virKeyFileHasValue(kf, "Foo", "one")) { + VIR_DEBUG("Missing Value 'Foo.one'"); + goto cleanup; + } + if (!virKeyFileHasValue(kf, "Foo", "two")) { + VIR_DEBUG("Missing Value 'Foo.two'"); + goto cleanup; + } + if (!virKeyFileHasValue(kf, "Foo", "three")) { + VIR_DEBUG("Missing Value 'Foo.three'"); + goto cleanup; + } + if (!STREQ(virKeyFileGetValueString(kf, "Foo", "one"), + "The first entry is here")) { + VIR_DEBUG("Wrong value for 'Foo.one'"); + goto cleanup; + } + if (!STREQ(virKeyFileGetValueString(kf, "Foo", "two"), + "The second entry")) { + VIR_DEBUG("Wrong value for 'Foo.one'"); + goto cleanup; + } + if (!STREQ(virKeyFileGetValueString(kf, "Foo", "three"), + "The third entry")) { + VIR_DEBUG("Wrong value for 'Foo.one'"); + goto cleanup; + } + + if (!virKeyFileHasGroup(kf, "Bar")) { + VIR_DEBUG("Missing group 'Bar'"); + goto cleanup; + } + if (!virKeyFileHasValue(kf, "Bar", "one")) { + VIR_DEBUG("Missing Value 'Bar.one'"); + goto cleanup; + } + if (!STREQ(virKeyFileGetValueString(kf, "Bar", "one"), + "The first entry in second group")) { + VIR_DEBUG("Wrong value for 'Bar.one'"); + goto cleanup; + } + + ret = 0; +cleanup: + virKeyFileFree(kf); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + signal(SIGPIPE, SIG_IGN); + + if (virtTestRun("Test parse", 1, testParse, NULL) < 0) + ret = -1; + + return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); +} + +VIRT_TEST_MAIN(mymain) -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
The '.ini' file format is a useful alternative to the existing config file style, when you need to have config files which are hashes of hashes. The 'virKeyFilePtr' object provides a way to parse these file types.
* src/Makefile.am, src/util/virkeyfile.c, src/util/virkeyfile.h: Add .ini file parser * tests/Makefile.am, tests/virkeyfiletest.c: Test basic parsing capabilities --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 10 ++ src/util/virkeyfile.c | 367 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virkeyfile.h | 64 ++++++++ tests/Makefile.am | 8 +- tests/virkeyfiletest.c | 123 ++++++++++++++++ 7 files changed, 573 insertions(+), 1 deletions(-) create mode 100644 src/util/virkeyfile.c create mode 100644 src/util/virkeyfile.h create mode 100644 tests/virkeyfiletest.c
diff --git a/po/POTFILES.in b/po/POTFILES.in index 16a3f9e..8354c09 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -127,6 +127,7 @@ src/util/util.c src/util/viraudit.c src/util/virfile.c src/util/virhash.c +src/util/virkeyfile.c src/util/virnetdev.c src/util/virnetdevbridge.c src/util/virnetdevmacvlan.c diff --git a/src/Makefile.am b/src/Makefile.am index 39076cc..07d7faa 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -90,6 +90,7 @@ UTIL_SOURCES = \ util/virhash.c util/virhash.h \ util/virhashcode.c util/virhashcode.h \ util/virkeycode.c util/virkeycode.h \ + util/virkeyfile.c util/virkeyfile.h \ util/virkeymaps.h \ util/virmacaddr.h util/virmacaddr.c \ util/virnetdev.h util/virnetdev.c \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8a14838..3f69ec1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1198,6 +1198,16 @@ virKeycodeValueFromString; virKeycodeValueTranslate;
+# virkeyfile.h +virKeyFileNew; +virKeyFileLoadFile; +virKeyFileLoadData; +virKeyFileFree; +virKeyFileHasValue; +virKeyFileHasGroup; +virKeyFileGetValueString;
Again, the sorting, per all the strings are sorted now. :-)
+ + # virmacaddr.h virMacAddrCompare; virMacAddrFormat; diff --git a/src/util/virkeyfile.c b/src/util/virkeyfile.c new file mode 100644 index 0000000..3dd4960 --- /dev/null +++ b/src/util/virkeyfile.c @@ -0,0 +1,367 @@ +/* + * virkeyfile.c: "ini"-style configuration file handling + * + * Copyright (C) 2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Authors: + * Daniel P. Berrange<berrange@redhat.com> + */ + +#include<config.h> + +#include<stdio.h> + +#include "c-ctype.h" +#include "logging.h" +#include "memory.h" +#include "util.h" +#include "virhash.h" +#include "virkeyfile.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_CONF + +typedef struct _virKeyFileGroup virKeyFileGroup; +typedef virKeyFileGroup *virKeyFileGroupPtr; + +typedef struct _virKeyFileParserCtxt virKeyFileParserCtxt; +typedef virKeyFileParserCtxt *virKeyFileParserCtxtPtr; + +struct _virKeyFile { + virHashTablePtr groups; +}; + +struct _virKeyFileParserCtxt { + virKeyFilePtr conf; + + const char *filename; + + const char *base; + const char *cur; + const char *end; + size_t line; + + char *groupname; + virHashTablePtr group; +}; + +/* + * The grammar for the keyfile + * + * KEYFILE = (GROUP | COMMENT | BLANK )* + * + * COMMENT = ('#' | ';') [^\n]* '\n' + * BLANK = (' ' | '\t' )* '\n' + * + * GROUP = '[' GROUPNAME ']' '\n' (ENTRY ) * + * GROUPNAME = [^[]\n]+ + * + * ENTRY = KEYNAME '=' VALUE + * VALUE = [^\n]* '\n' + * KEYNAME = [-a-zA-Z0-9]+ + */ + +#define IS_EOF (ctxt->cur>= ctxt->end) +#define IS_EOL(c) (((c) == '\n') || ((c) == '\r')) +#define CUR (*ctxt->cur) +#define NEXT if (!IS_EOF) ctxt->cur++; + + +#define virKeyFileError(ctxt, error, info) \ + virKeyFileErrorHelper(__FILE__, __FUNCTION__, __LINE__, ctxt, error, info) +static void +virKeyFileErrorHelper(const char *file, const char *func, size_t line, + virKeyFileParserCtxtPtr ctxt, + virErrorNumber error, const char *info) +{ + /* Construct the string 'filename:line: info' if we have that. */ + if (ctxt&& ctxt->filename) { + virReportErrorHelper(VIR_FROM_CONF, error, file, func, line, + _("%s:%zu: %s '%s'"), ctxt->filename, ctxt->line, info, ctxt->cur); + } else { + virReportErrorHelper(VIR_FROM_CONF, error, file, func, line, + "%s", info); + } +} + + +static void virKeyFileValueFree(void *value, const void *name ATTRIBUTE_UNUSED) +{ + VIR_FREE(value); +} + +static int virKeyFileParseGroup(virKeyFileParserCtxtPtr ctxt) +{ + int ret = -1; + const char *name; + NEXT; + + ctxt->group = NULL; + VIR_FREE(ctxt->groupname); + + name = ctxt->cur; + while (!IS_EOF&& c_isascii(CUR)&& CUR != ']') + ctxt->cur++; + if (CUR != ']') { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "cannot find end of group name, expected ']'"); + return -1; + } + + if (!(ctxt->groupname = strndup(name, ctxt->cur - name))) { + virReportOOMError(); + return -1; + } + + NEXT; + + if (!(ctxt->group = virHashCreate(10, virKeyFileValueFree))) + goto cleanup; + + if (virHashAddEntry(ctxt->conf->groups, ctxt->groupname, ctxt->group)< 0) + goto cleanup; + + ret = 0; +cleanup: + if (ret != 0) { + virHashFree(ctxt->group); + ctxt->group = NULL; + VIR_FREE(ctxt->groupname); + } + + return ret; +} + +static int virKeyFileParseValue(virKeyFileParserCtxtPtr ctxt) +{ + int ret = -1; + const char *keystart; + const char *valuestart; + char *key = NULL; + char *value = NULL; + size_t len; + + if (!ctxt->groupname || !ctxt->group) { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "value found before first group"); + return -1; + } + + keystart = ctxt->cur; + while (!IS_EOF&& c_isalnum(CUR)&& CUR != '=') + ctxt->cur++; + if (CUR != '=') { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "expected end of value name, expected '='"); + return -1; + } + + if (!(key = strndup(keystart, ctxt->cur - keystart))) { + virReportOOMError(); + return -1; + } + + NEXT; + valuestart = ctxt->cur; + while (!IS_EOF&& !IS_EOL(CUR)) + ctxt->cur++; + if (!(IS_EOF || IS_EOL(CUR))) { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "unexpected end of value"); + goto cleanup; + } + len = ctxt->cur - valuestart; + if (IS_EOF&& !IS_EOL(CUR)) + len++; + if (!(value = strndup(valuestart, len))) { + virReportOOMError(); + goto cleanup; + } + + if (virHashAddEntry(ctxt->group, key, value)< 0) + goto cleanup; + + NEXT; + + ret = 0; + +cleanup: + VIR_FREE(key);
Do we need to VIR_FREE(value) too? ACK otherwise.
On Thu, Mar 22, 2012 at 05:34:54PM +0800, Osier Yang wrote:
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com> +static int virKeyFileParseValue(virKeyFileParserCtxtPtr ctxt) +{ + int ret = -1; + const char *keystart; + const char *valuestart; + char *key = NULL; + char *value = NULL; + size_t len; + + if (!ctxt->groupname || !ctxt->group) { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "value found before first group"); + return -1; + } + + keystart = ctxt->cur; + while (!IS_EOF&& c_isalnum(CUR)&& CUR != '=') + ctxt->cur++; + if (CUR != '=') { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "expected end of value name, expected '='"); + return -1; + } + + if (!(key = strndup(keystart, ctxt->cur - keystart))) { + virReportOOMError(); + return -1; + } + + NEXT; + valuestart = ctxt->cur; + while (!IS_EOF&& !IS_EOL(CUR)) + ctxt->cur++; + if (!(IS_EOF || IS_EOL(CUR))) { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "unexpected end of value"); + goto cleanup; + } + len = ctxt->cur - valuestart; + if (IS_EOF&& !IS_EOL(CUR)) + len++; + if (!(value = strndup(valuestart, len))) { + virReportOOMError(); + goto cleanup; + } + + if (virHashAddEntry(ctxt->group, key, value)< 0) + goto cleanup; + + NEXT; + + ret = 0; + +cleanup: + VIR_FREE(key);
Do we need to VIR_FREE(value) too?
No, the value is owned by the hash table now 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 03/23/2012 08:16 PM, Daniel P. Berrange wrote:
On Thu, Mar 22, 2012 at 05:34:54PM +0800, Osier Yang wrote:
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com> +static int virKeyFileParseValue(virKeyFileParserCtxtPtr ctxt) +{ + int ret = -1; + const char *keystart; + const char *valuestart; + char *key = NULL; + char *value = NULL; + size_t len; + + if (!ctxt->groupname || !ctxt->group) { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "value found before first group"); + return -1; + } + + keystart = ctxt->cur; + while (!IS_EOF&& c_isalnum(CUR)&& CUR != '=') + ctxt->cur++; + if (CUR != '=') { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "expected end of value name, expected '='"); + return -1; + } + + if (!(key = strndup(keystart, ctxt->cur - keystart))) { + virReportOOMError(); + return -1; + } + + NEXT; + valuestart = ctxt->cur; + while (!IS_EOF&& !IS_EOL(CUR)) + ctxt->cur++; + if (!(IS_EOF || IS_EOL(CUR))) { + virKeyFileError(ctxt, VIR_ERR_CONF_SYNTAX, "unexpected end of value"); + goto cleanup; + } + len = ctxt->cur - valuestart; + if (IS_EOF&& !IS_EOL(CUR)) + len++; + if (!(value = strndup(valuestart, len))) { + virReportOOMError(); + goto cleanup; + } + + if (virHashAddEntry(ctxt->group, key, value)< 0) + goto cleanup;
I meant here. Perhaps I commented at wrong place.
+ + NEXT; + + ret = 0; + +cleanup: + VIR_FREE(key);
Do we need to VIR_FREE(value) too?
No, the value is owned by the hash table now
How about virHashAddEntry fails? :-) Osier
From: "Daniel P. Berrange" <berrange@redhat.com> To follow latest naming conventions, rename src/util/authhelper.[ch] to src/util/virauth.[ch]. * src/util/authhelper.[ch]: Rename to src/util/virauth.[ch] * src/esx/esx_driver.c, src/hyperv/hyperv_driver.c, src/phyp/phyp_driver.c, src/xenapi/xenapi_driver.c: Update for renamed include files --- po/POTFILES.in | 2 +- src/Makefile.am | 2 +- src/esx/esx_driver.c | 2 +- src/hyperv/hyperv_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/util/{authhelper.c => virauth.c} | 5 ++--- src/util/{authhelper.h => virauth.h} | 9 ++++----- src/xenapi/xenapi_driver.c | 2 +- 8 files changed, 12 insertions(+), 14 deletions(-) rename src/util/{authhelper.c => virauth.c} (97%) rename src/util/{authhelper.h => virauth.h} (87%) diff --git a/po/POTFILES.in b/po/POTFILES.in index 8354c09..8eaa8ad 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -106,7 +106,6 @@ src/storage/storage_driver.c src/test/test_driver.c src/uml/uml_conf.c src/uml/uml_driver.c -src/util/authhelper.c src/util/cgroup.c src/util/command.c src/util/conf.c @@ -124,6 +123,7 @@ src/util/stats_linux.c src/util/storage_file.c src/util/sysinfo.c src/util/util.c +src/util/virauth.c src/util/viraudit.c src/util/virfile.c src/util/virhash.c diff --git a/src/Makefile.am b/src/Makefile.am index 07d7faa..3cbf9d7 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -52,7 +52,6 @@ augeastest_DATA = # These files are not related to driver APIs. Simply generic # helper APIs for various purposes UTIL_SOURCES = \ - util/authhelper.c util/authhelper.h \ util/bitmap.c util/bitmap.h \ util/buf.c util/buf.h \ util/command.c util/command.h \ @@ -81,6 +80,7 @@ UTIL_SOURCES = \ util/uuid.c util/uuid.h \ util/util.c util/util.h \ util/viraudit.c util/viraudit.h \ + util/virauth.c util/virauth.h \ util/virfile.c util/virfile.h \ util/virnodesuspend.c util/virnodesuspend.h \ util/virpidfile.c util/virpidfile.h \ diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 7e41fa3..51bd5b2 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -26,7 +26,7 @@ #include "internal.h" #include "domain_conf.h" -#include "authhelper.h" +#include "virauth.h" #include "util.h" #include "memory.h" #include "logging.h" diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 27c8747..5ca20cf 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -26,7 +26,7 @@ #include "internal.h" #include "datatypes.h" #include "domain_conf.h" -#include "authhelper.h" +#include "virauth.h" #include "util.h" #include "memory.h" #include "logging.h" diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index bdf4a7b..4b99465 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -44,7 +44,7 @@ #include <domain_event.h> #include "internal.h" -#include "authhelper.h" +#include "virauth.h" #include "util.h" #include "datatypes.h" #include "buf.h" diff --git a/src/util/authhelper.c b/src/util/virauth.c similarity index 97% rename from src/util/authhelper.c rename to src/util/virauth.c index 9398cb3..a3c6b94 100644 --- a/src/util/authhelper.c +++ b/src/util/virauth.c @@ -1,6 +1,5 @@ - /* - * authhelper.c: authentication related utility functions + * virauth.c: authentication related utility functions * * Copyright (C) 2010 Matthias Bolte <matthias.bolte@googlemail.com> * @@ -22,7 +21,7 @@ #include <config.h> -#include "authhelper.h" +#include "virauth.h" #include "util.h" #include "memory.h" diff --git a/src/util/authhelper.h b/src/util/virauth.h similarity index 87% rename from src/util/authhelper.h rename to src/util/virauth.h index ca45d16..2c8d80f 100644 --- a/src/util/authhelper.h +++ b/src/util/virauth.h @@ -1,6 +1,5 @@ - /* - * authhelper.h: authentication related utility functions + * virauth.h: authentication related utility functions * * Copyright (C) 2010 Matthias Bolte <matthias.bolte@googlemail.com> * @@ -20,8 +19,8 @@ * */ -#ifndef __VIR_AUTHHELPER_H__ -# define __VIR_AUTHHELPER_H__ +#ifndef __VIR_AUTH_H__ +# define __VIR_AUTH_H__ # include "internal.h" @@ -30,4 +29,4 @@ char *virRequestUsername(virConnectAuthPtr auth, const char *defaultUsername, char *virRequestPassword(virConnectAuthPtr auth, const char *username, const char *hostname); -#endif /* __VIR_AUTHHELPER_H__ */ +#endif /* __VIR_AUTH_H__ */ diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 298db12..8d261ef 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -31,7 +31,7 @@ #include "domain_conf.h" #include "virterror_internal.h" #include "datatypes.h" -#include "authhelper.h" +#include "virauth.h" #include "util.h" #include "uuid.h" #include "memory.h" -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
To follow latest naming conventions, rename src/util/authhelper.[ch] to src/util/virauth.[ch].
* src/util/authhelper.[ch]: Rename to src/util/virauth.[ch] * src/esx/esx_driver.c, src/hyperv/hyperv_driver.c, src/phyp/phyp_driver.c, src/xenapi/xenapi_driver.c: Update for renamed include files --- po/POTFILES.in | 2 +- src/Makefile.am | 2 +- src/esx/esx_driver.c | 2 +- src/hyperv/hyperv_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/util/{authhelper.c => virauth.c} | 5 ++--- src/util/{authhelper.h => virauth.h} | 9 ++++----- src/xenapi/xenapi_driver.c | 2 +- 8 files changed, 12 insertions(+), 14 deletions(-) rename src/util/{authhelper.c => virauth.c} (97%) rename src/util/{authhelper.h => virauth.h} (87%)
diff --git a/po/POTFILES.in b/po/POTFILES.in index 8354c09..8eaa8ad 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -106,7 +106,6 @@ src/storage/storage_driver.c src/test/test_driver.c src/uml/uml_conf.c src/uml/uml_driver.c -src/util/authhelper.c src/util/cgroup.c src/util/command.c src/util/conf.c @@ -124,6 +123,7 @@ src/util/stats_linux.c src/util/storage_file.c src/util/sysinfo.c src/util/util.c +src/util/virauth.c src/util/virauditc
Better to put it after viraudit.c, but it doesn't hurt. ACK
From: "Daniel P. Berrange" <berrange@redhat.com> Ensure that the functions in virauth.h have names matching the file prefix, by renaming virRequest{Username,Password} to virAuthGet{Username,Password} --- src/esx/esx_driver.c | 8 ++++---- src/hyperv/hyperv_driver.c | 4 ++-- src/libvirt_private.syms | 5 +++++ src/phyp/phyp_driver.c | 4 ++-- src/util/virauth.c | 4 ++-- src/util/virauth.h | 4 ++-- src/xenapi/xenapi_driver.c | 4 ++-- 7 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 51bd5b2..7689306 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -700,7 +700,7 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, goto cleanup; } } else { - username = virRequestUsername(auth, "root", hostname); + username = virAuthGetUsername(auth, "root", hostname); if (username == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); @@ -708,7 +708,7 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, } } - unescapedPassword = virRequestPassword(auth, username, hostname); + unescapedPassword = virAuthGetPassword(auth, username, hostname); if (unescapedPassword == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); @@ -830,7 +830,7 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, goto cleanup; } } else { - username = virRequestUsername(auth, "administrator", hostname); + username = virAuthGetUsername(auth, "administrator", hostname); if (username == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); @@ -838,7 +838,7 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, } } - unescapedPassword = virRequestPassword(auth, username, hostname); + unescapedPassword = virAuthGetPassword(auth, username, hostname); if (unescapedPassword == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 5ca20cf..0469e2e 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -147,7 +147,7 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) goto cleanup; } } else { - username = virRequestUsername(auth, "administrator", conn->uri->server); + username = virAuthGetUsername(auth, "administrator", conn->uri->server); if (username == NULL) { HYPERV_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); @@ -155,7 +155,7 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) } } - password = virRequestPassword(auth, username, conn->uri->server); + password = virAuthGetPassword(auth, username, conn->uri->server); if (password == NULL) { HYPERV_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3f69ec1..07302cd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1165,6 +1165,11 @@ virUUIDGenerate; virUUIDParse; +# virauth.h +virAuthGetUsername; +virAuthGetPassword; + + # viraudit.h virAuditClose; virAuditEncode; diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 4b99465..470706d 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1006,7 +1006,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, goto err; } - username = virRequestUsername(auth, NULL, conn->uri->server); + username = virAuthGetUsername(auth, NULL, conn->uri->server); if (username == NULL) { PHYP_ERROR(VIR_ERR_AUTH_FAILED, "%s", @@ -1087,7 +1087,7 @@ keyboard_interactive: goto disconnect; } - password = virRequestPassword(auth, username, conn->uri->server); + password = virAuthGetPassword(auth, username, conn->uri->server); if (password == NULL) { PHYP_ERROR(VIR_ERR_AUTH_FAILED, "%s", diff --git a/src/util/virauth.c b/src/util/virauth.c index a3c6b94..d7375e9 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -27,7 +27,7 @@ char * -virRequestUsername(virConnectAuthPtr auth, const char *defaultUsername, +virAuthGetUsername(virConnectAuthPtr auth, const char *defaultUsername, const char *hostname) { unsigned int ncred; @@ -74,7 +74,7 @@ virRequestUsername(virConnectAuthPtr auth, const char *defaultUsername, char * -virRequestPassword(virConnectAuthPtr auth, const char *username, +virAuthGetPassword(virConnectAuthPtr auth, const char *username, const char *hostname) { unsigned int ncred; diff --git a/src/util/virauth.h b/src/util/virauth.h index 2c8d80f..8856701 100644 --- a/src/util/virauth.h +++ b/src/util/virauth.h @@ -24,9 +24,9 @@ # include "internal.h" -char *virRequestUsername(virConnectAuthPtr auth, const char *defaultUsername, +char *virAuthGetUsername(virConnectAuthPtr auth, const char *defaultUsername, const char *hostname); -char *virRequestPassword(virConnectAuthPtr auth, const char *username, +char *virAuthGetPassword(virConnectAuthPtr auth, const char *username, const char *hostname); #endif /* __VIR_AUTH_H__ */ diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 8d261ef..3f88c91 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -138,7 +138,7 @@ xenapiOpen (virConnectPtr conn, virConnectAuthPtr auth, goto error; } } else { - username = virRequestUsername(auth, NULL, conn->uri->server); + username = virAuthGetUsername(auth, NULL, conn->uri->server); if (username == NULL) { xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED, @@ -147,7 +147,7 @@ xenapiOpen (virConnectPtr conn, virConnectAuthPtr auth, } } - password = virRequestPassword(auth, username, conn->uri->server); + password = virAuthGetPassword(auth, username, conn->uri->server); if (password == NULL) { xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED, -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
Ensure that the functions in virauth.h have names matching the file prefix, by renaming virRequest{Username,Password} to virAuthGet{Username,Password} --- src/esx/esx_driver.c | 8 ++++---- src/hyperv/hyperv_driver.c | 4 ++-- src/libvirt_private.syms | 5 +++++ src/phyp/phyp_driver.c | 4 ++-- src/util/virauth.c | 4 ++-- src/util/virauth.h | 4 ++-- src/xenapi/xenapi_driver.c | 4 ++-- 7 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 51bd5b2..7689306 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -700,7 +700,7 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, goto cleanup; } } else { - username = virRequestUsername(auth, "root", hostname); + username = virAuthGetUsername(auth, "root", hostname);
if (username == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); @@ -708,7 +708,7 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, } }
- unescapedPassword = virRequestPassword(auth, username, hostname); + unescapedPassword = virAuthGetPassword(auth, username, hostname);
if (unescapedPassword == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); @@ -830,7 +830,7 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, goto cleanup; } } else { - username = virRequestUsername(auth, "administrator", hostname); + username = virAuthGetUsername(auth, "administrator", hostname);
if (username == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); @@ -838,7 +838,7 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, } }
- unescapedPassword = virRequestPassword(auth, username, hostname); + unescapedPassword = virAuthGetPassword(auth, username, hostname);
if (unescapedPassword == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 5ca20cf..0469e2e 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -147,7 +147,7 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) goto cleanup; } } else { - username = virRequestUsername(auth, "administrator", conn->uri->server); + username = virAuthGetUsername(auth, "administrator", conn->uri->server);
if (username == NULL) { HYPERV_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); @@ -155,7 +155,7 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) } }
- password = virRequestPassword(auth, username, conn->uri->server); + password = virAuthGetPassword(auth, username, conn->uri->server);
if (password == NULL) { HYPERV_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3f69ec1..07302cd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1165,6 +1165,11 @@ virUUIDGenerate; virUUIDParse;
+# virauth.h +virAuthGetUsername; +virAuthGetPassword;
virRequest{Username,Password} are forgoten to remove. # authhelper.h virRequestPassword; virRequestUsername; ACK with they are removed.
From: "Daniel P. Berrange" <berrange@redhat.com> * src/util/virauth.c, src/util/virauth.h: Add virAuthGetConfigFilePath * include/libvirt/virterror.h, src/util/virterror.c: Add VIR_FROM_AUTH error domain --- include/libvirt/virterror.h | 1 + src/libvirt_private.syms | 1 + src/util/virauth.c | 74 +++++++++++++++++++++++++++++++++++++++++++ src/util/virauth.h | 3 ++ src/util/virterror.c | 3 ++ 5 files changed, 82 insertions(+), 0 deletions(-) diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index c8ec8d4..e04d29e 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -86,6 +86,7 @@ typedef enum { VIR_FROM_HYPERV = 43, /* Error from Hyper-V driver */ VIR_FROM_CAPABILITIES = 44, /* Error from capabilities */ VIR_FROM_URI = 45, /* Error from URI handling */ + VIR_FROM_AUTH = 46, /* Error from auth handling */ } virErrorDomain; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 07302cd..6f53aa8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1168,6 +1168,7 @@ virUUIDParse; # virauth.h virAuthGetUsername; virAuthGetPassword; +virAuthGetConfigFilePath; # viraudit.h diff --git a/src/util/virauth.c b/src/util/virauth.c index d7375e9..150b8e7 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -21,9 +21,83 @@ #include <config.h> +#include <stdlib.h> + #include "virauth.h" #include "util.h" #include "memory.h" +#include "logging.h" +#include "datatypes.h" +#include "virterror_internal.h" +#include "configmake.h" + +#define VIR_FROM_THIS VIR_FROM_AUTH + + +int virAuthGetConfigFilePath(virConnectPtr conn, + char **path) +{ + int ret = -1; + size_t i; + const char *authenv = getenv("LIBVIRT_AUTH_FILE"); + char *userdir = NULL; + + *path = NULL; + + VIR_DEBUG("Determining auth config file path"); + + if (authenv) { + VIR_DEBUG("Using path from env '%s'", authenv); + if (!(*path = strdup(authenv))) + goto no_memory; + return 0; + } + + for (i = 0 ; i < conn->uri->paramsCount ; i++) { + if (STREQ_NULLABLE(conn->uri->params[i].name, "authfile") && + conn->uri->params[i].value) { + VIR_DEBUG("Using path from URI '%s'", + conn->uri->params[i].value); + if (!(*path = strdup(conn->uri->params[i].value))) + goto no_memory; + return 0; + } + } + + if (!(userdir = virGetUserDirectory(geteuid()))) + goto cleanup; + + if (virAsprintf(path, "%s/.libvirt/auth.conf", userdir) < 0) + goto no_memory; + + VIR_DEBUG("Checking for readability of '%s'", *path); + if (access(*path, R_OK) == 0) + goto done; + + VIR_FREE(*path); + + if (!(*path = strdup(SYSCONFDIR "/libvirt/auth.conf"))) + goto no_memory; + + VIR_DEBUG("Checking for readability of '%s'", *path); + if (access(*path, R_OK) == 0) + goto done; + + VIR_FREE(*path); + +done: + ret = 0; + + VIR_DEBUG("Using auth file '%s'", NULLSTR(*path)); +cleanup: + VIR_FREE(userdir); + + return ret; + +no_memory: + virReportOOMError(); + goto cleanup; +} char * diff --git a/src/util/virauth.h b/src/util/virauth.h index 8856701..7f43bee 100644 --- a/src/util/virauth.h +++ b/src/util/virauth.h @@ -24,6 +24,9 @@ # include "internal.h" +int virAuthGetConfigFilePath(virConnectPtr conn, + char **path); + char *virAuthGetUsername(virConnectAuthPtr auth, const char *defaultUsername, const char *hostname); char *virAuthGetPassword(virConnectAuthPtr auth, const char *username, diff --git a/src/util/virterror.c b/src/util/virterror.c index e1fe522..9dc40a8 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -181,6 +181,9 @@ static const char *virErrorDomainName(virErrorDomain domain) { case VIR_FROM_URI: dom = "URI "; break; + case VIR_FROM_AUTH: + dom = "Auth "; + break; } return(dom); } -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
* src/util/virauth.c, src/util/virauth.h: Add virAuthGetConfigFilePath * include/libvirt/virterror.h, src/util/virterror.c: Add VIR_FROM_AUTH error domain --- include/libvirt/virterror.h | 1 + src/libvirt_private.syms | 1 + src/util/virauth.c | 74 +++++++++++++++++++++++++++++++++++++++++++ src/util/virauth.h | 3 ++ src/util/virterror.c | 3 ++ 5 files changed, 82 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index c8ec8d4..e04d29e 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -86,6 +86,7 @@ typedef enum { VIR_FROM_HYPERV = 43, /* Error from Hyper-V driver */ VIR_FROM_CAPABILITIES = 44, /* Error from capabilities */ VIR_FROM_URI = 45, /* Error from URI handling */ + VIR_FROM_AUTH = 46, /* Error from auth handling */ } virErrorDomain;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 07302cd..6f53aa8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1168,6 +1168,7 @@ virUUIDParse; # virauth.h virAuthGetUsername; virAuthGetPassword; +virAuthGetConfigFilePath;
virAuthGetConfigFilePath; virAuthGetPassword; virAuthGetUsername;
# viraudit.h diff --git a/src/util/virauth.c b/src/util/virauth.c index d7375e9..150b8e7 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -21,9 +21,83 @@
#include<config.h>
+#include<stdlib.h> + #include "virauth.h" #include "util.h" #include "memory.h" +#include "logging.h" +#include "datatypes.h" +#include "virterror_internal.h" +#include "configmake.h" + +#define VIR_FROM_THIS VIR_FROM_AUTH + + +int virAuthGetConfigFilePath(virConnectPtr conn, + char **path) +{ + int ret = -1; + size_t i; + const char *authenv = getenv("LIBVIRT_AUTH_FILE"); + char *userdir = NULL; + + *path = NULL; + + VIR_DEBUG("Determining auth config file path"); + + if (authenv) { + VIR_DEBUG("Using path from env '%s'", authenv); + if (!(*path = strdup(authenv))) + goto no_memory; + return 0; + } + + for (i = 0 ; i< conn->uri->paramsCount ; i++) { + if (STREQ_NULLABLE(conn->uri->params[i].name, "authfile")&& + conn->uri->params[i].value) { + VIR_DEBUG("Using path from URI '%s'", + conn->uri->params[i].value); + if (!(*path = strdup(conn->uri->params[i].value))) + goto no_memory; + return 0; + } + } + + if (!(userdir = virGetUserDirectory(geteuid()))) + goto cleanup; + + if (virAsprintf(path, "%s/.libvirt/auth.conf", userdir)< 0) + goto no_memory; + + VIR_DEBUG("Checking for readability of '%s'", *path); + if (access(*path, R_OK) == 0) + goto done; + + VIR_FREE(*path); + + if (!(*path = strdup(SYSCONFDIR "/libvirt/auth.conf"))) + goto no_memory; + + VIR_DEBUG("Checking for readability of '%s'", *path); + if (access(*path, R_OK) == 0) + goto done; + + VIR_FREE(*path);
*path will be NULL if the last choice of authfile (e.g. /etc/libvirt/auth.conf) is not readable, while the function will return 0. ACK with this fixed.
On Thu, Mar 22, 2012 at 04:34:16PM +0800, Osier Yang wrote:
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
+int virAuthGetConfigFilePath(virConnectPtr conn, + char **path) +{ + int ret = -1; + size_t i; + const char *authenv = getenv("LIBVIRT_AUTH_FILE"); + char *userdir = NULL; + + *path = NULL; + + VIR_DEBUG("Determining auth config file path"); + + if (authenv) { + VIR_DEBUG("Using path from env '%s'", authenv); + if (!(*path = strdup(authenv))) + goto no_memory; + return 0; + } + + for (i = 0 ; i< conn->uri->paramsCount ; i++) { + if (STREQ_NULLABLE(conn->uri->params[i].name, "authfile")&& + conn->uri->params[i].value) { + VIR_DEBUG("Using path from URI '%s'", + conn->uri->params[i].value); + if (!(*path = strdup(conn->uri->params[i].value))) + goto no_memory; + return 0; + } + } + + if (!(userdir = virGetUserDirectory(geteuid()))) + goto cleanup; + + if (virAsprintf(path, "%s/.libvirt/auth.conf", userdir)< 0) + goto no_memory; + + VIR_DEBUG("Checking for readability of '%s'", *path); + if (access(*path, R_OK) == 0) + goto done; + + VIR_FREE(*path); + + if (!(*path = strdup(SYSCONFDIR "/libvirt/auth.conf"))) + goto no_memory; + + VIR_DEBUG("Checking for readability of '%s'", *path); + if (access(*path, R_OK) == 0) + goto done; + + VIR_FREE(*path);
*path will be NULL if the last choice of authfile (e.g. /etc/libvirt/auth.conf) is not readable, while the function will return 0.
Yes, that is correct behaviour. We only return '-1' upon some fatal error. We return 0 if we successfully found a path, or if no path exists. This is why we return the path in a out parameter, instead of using the return value. 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 :|
From: "Daniel P. Berrange" <berrange@redhat.com> This defines the format for the auth credential config file and provides APIs to access the data. The config file contains one or more named 'credential' sets [credentials-$NAME] credname1=value1 credname2=value2 eg [credentials-test] authname=fred password=123456 [credentials-prod] authname=bar password=letmein There are then one or more 'auth' sets which match services/hosts and map to credential sets. [auth-$SERVICE-$HOSTNAME] credentials=$CREDENTIALS eg [auth-libvirt-test1.example.com] credentials=test [auth-libvirt-test2.example.com] credentials=test [auth-libvirt-demo3.example.com] credentials=test [auth-libvirt-prod1.example.com] credentials=prod * docs/auth.html.in: Document use of client auth config files * src/Makefile.am, src/libvirt_private.syms, src/util/virauthconfig.c, src/util/virauthconfig.h: Add APIs for processing auth.conf file --- docs/auth.html.in | 118 ++++++++++++++++++++++++++++++- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 7 ++ src/util/virauthconfig.c | 175 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virauthconfig.h | 45 ++++++++++++ tests/Makefile.am | 9 ++- tests/virauthconfigtest.c | 140 ++++++++++++++++++++++++++++++++++++ 8 files changed, 494 insertions(+), 2 deletions(-) create mode 100644 src/util/virauthconfig.c create mode 100644 src/util/virauthconfig.h create mode 100644 tests/virauthconfigtest.c diff --git a/docs/auth.html.in b/docs/auth.html.in index 2163959..ecff0fc 100644 --- a/docs/auth.html.in +++ b/docs/auth.html.in @@ -1,7 +1,7 @@ <?xml version="1.0"?> <html> <body> - <h1 >Access control</h1> + <h1 >Authentication & access control</h1> <p> When connecting to libvirt, some connections may require client authentication before allowing use of the APIs. The set of possible @@ -11,6 +11,122 @@ <ul id="toc"></ul> + <h2><a name="Auth_client_config">Client configuration</a></h2> + + <p> + When connecting to a remote hypervisor which requires authentication, +most libvirt applications will prompt the user for the credentials. It is +also possible to provide a client configuration file containing all the +authentication credentials, avoiding any interaction. Libvirt will look +for the authentication file using the following sequence: + </p> + <ol> + <li>The file path specified by the $LIBVIRT_AUTH_FILE environment + variable.</li> + <li>The file path specified by the "authfile=/some/file" URI + query parameter</li> + <li>The file $HOME/.libvirt/auth.conf</li> + <li>The file /etc/libvirt/auth.conf</li> + </ol> + + <p> + The auth configuration file uses the traditional <code>".ini"</code> + style syntax. There are two types of groups that can be present in + the config. First there are one or more <strong>credential</strong> + sets, which provide the actual authentication credentials. The keys + within the group may be: + </p> + + <ul> + <li><code>username</code>: the user login name to act as. This + is relevant for ESX, Xen, HyperV and SSH, but probably not + the one you want to libvirtd with SASL.</li> + <li><code>authname</code>: the name to authorize as. This is + what is commonly required for libvirtd with SASL.</li> + <li><code>password</code>: the secret password</li> + <li><code>realm</code>: the domain realm for SASL, mostly + unused</li> + </ul> + + <p> + Each set of credentials has a name, which is part of the group + entry name. Overall the syntax is + </p> + + <pre> +[credentials-$NAME] +credname1=value1 +credname2=value2</pre> + + <p> + For example, to define two sets of credentials used for production + and test machines, using libvirtd, and a further ESX server for dev: + </p> +<pre> +[credentials-test] +authname=fred +password=123456 + +[credentials-prod] +authname=bar +password=letmein + +[credentials-dev] +username=joe +password=hello</pre> + + <p> + The second set of groups provide mappings of credentials to + specific machine services. The config file group names compromise + the service type and host: + </p> + + <pre> +[auth-$SERVICE-$HOSTNAME] +credentials=$CREDENTIALS</pre> + + <p> + For example, following the previous example, here is how to + list some machines + </p> + + <pre> +[auth-libvirt-test1.example.com] +credentials=test + +[auth-libvirt-test2.example.com] +credentials=test + +[auth-libvirt-demo3.example.com] +credentials=test + +[auth-libvirt-prod1.example.com] +credentials=prod + +[auth-esx-dev1.example.com] +credentials=dev</pre> + + <p> + The following service types are known to libvirt + </p> + + <ol> + <li><code>libvirt</code> - used for connections to a libvirtd + server, which is configured with SASL auth</li> + <li><code>ssh</code> - used for connections to a Phyp server + over SSH</li> + <li><code>esx</code> - used for connections to an ESX or + VirtualCenter server</li> + <li><code>xen</code> - used for connections to a Xen Enterprise + sever using XenAPI</li> + </ol> + + <p> + Applications using libvirt are free to use this same configuration + file for storing other credentials. For example, it can be used + to storage VNC or SPICE login credentials + </p> + <h2><a name="ACL_server_config">Server configuration</a></h2> <p> The libvirt daemon allows the administrator to choose the authentication diff --git a/po/POTFILES.in b/po/POTFILES.in index 8eaa8ad..6488c4c 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -124,6 +124,7 @@ src/util/storage_file.c src/util/sysinfo.c src/util/util.c src/util/virauth.c +src/util/virauthconfig.c src/util/viraudit.c src/util/virfile.c src/util/virhash.c diff --git a/src/Makefile.am b/src/Makefile.am index 3cbf9d7..a2aae9d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -81,6 +81,7 @@ UTIL_SOURCES = \ util/util.c util/util.h \ util/viraudit.c util/viraudit.h \ util/virauth.c util/virauth.h \ + util/virauthconfig.c util/virauthconfig.h \ util/virfile.c util/virfile.h \ util/virnodesuspend.c util/virnodesuspend.h \ util/virpidfile.c util/virpidfile.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6f53aa8..09e2c35 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1171,6 +1171,13 @@ virAuthGetPassword; virAuthGetConfigFilePath; +# virauthconfig.h +virAuthConfigNew; +virAuthConfigNewData; +virAuthConfigLookup; +virAuthConfigFree; + + # viraudit.h virAuditClose; virAuditEncode; diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c new file mode 100644 index 0000000..ad98959 --- /dev/null +++ b/src/util/virauthconfig.c @@ -0,0 +1,175 @@ +/* + * virauthconfig.c: authentication config handling + * + * Copyright (C) 2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include "virauthconfig.h" + +#include "virkeyfile.h" +#include "memory.h" +#include "util.h" +#include "logging.h" +#include "virterror_internal.h" + + +struct _virAuthConfig { + virKeyFilePtr keyfile; + char *path; +}; + +#define VIR_FROM_THIS VIR_FROM_NONE + +#define virAuthReportError(code, ...) \ + virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) + + +virAuthConfigPtr virAuthConfigNew(const char *path) +{ + virAuthConfigPtr auth; + + if (VIR_ALLOC(auth) < 0) { + virReportOOMError(); + goto error; + } + + if (!(auth->path = strdup(path))) { + virReportOOMError(); + goto error; + } + + if (!(auth->keyfile = virKeyFileNew())) + goto error; + + if (virKeyFileLoadFile(auth->keyfile, path) < 0) + goto error; + + return auth; + +error: + virAuthConfigFree(auth); + return NULL; +} + + +virAuthConfigPtr virAuthConfigNewData(const char *path, + const char *data, + size_t len) +{ + virAuthConfigPtr auth; + + if (VIR_ALLOC(auth) < 0) { + virReportOOMError(); + goto error; + } + + if (!(auth->path = strdup(path))) { + virReportOOMError(); + goto error; + } + + if (!(auth->keyfile = virKeyFileNew())) + goto error; + + if (virKeyFileLoadData(auth->keyfile, path, data, len) < 0) + goto error; + + return auth; + +error: + virAuthConfigFree(auth); + return NULL; +} + + +void virAuthConfigFree(virAuthConfigPtr auth) +{ + if (!auth) + return; + + virKeyFileFree(auth->keyfile); + VIR_FREE(auth->path); + VIR_FREE(auth); +} + + +int virAuthConfigLookup(virAuthConfigPtr auth, + const char *service, + const char *hostname, + const char *credname, + const char **value) +{ + char *authgroup = NULL; + char *credgroup = NULL; + const char *authcred; + int ret = -1; + + *value = NULL; + + VIR_DEBUG("Lookup '%s' '%s' '%s'", service, NULLSTR(hostname), credname); + + if (!hostname) + hostname = "localhost"; + + if (virAsprintf(&authgroup, "auth-%s-%s", service, hostname) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (!virKeyFileHasGroup(auth->keyfile, authgroup)) { + ret = 0; + goto cleanup; + } + + if (!(authcred = virKeyFileGetValueString(auth->keyfile, authgroup, "credentials"))) { + virAuthReportError(VIR_ERR_CONF_SYNTAX, + _("Missing item 'credentials' in group '%s' in '%s'"), + authgroup, auth->path); + goto cleanup; + } + + if (virAsprintf(&credgroup, "credentials-%s", authcred) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (!virKeyFileHasGroup(auth->keyfile, credgroup)) { + virAuthReportError(VIR_ERR_CONF_SYNTAX, + _("Missing group 'credentials-%s' referenced from group '%s' in '%s'"), + authcred, authgroup, auth->path); + goto cleanup; + } + + if (!virKeyFileHasValue(auth->keyfile, credgroup, credname)) { + ret = 0; + goto cleanup; + } + + *value = virKeyFileGetValueString(auth->keyfile, credgroup, credname); + + ret = 0; + +cleanup: + VIR_FREE(authgroup); + VIR_FREE(credgroup); + return ret; +} diff --git a/src/util/virauthconfig.h b/src/util/virauthconfig.h new file mode 100644 index 0000000..cbeef85 --- /dev/null +++ b/src/util/virauthconfig.h @@ -0,0 +1,45 @@ +/* + * virauthconfig.h: authentication config handling + * + * Copyright (C) 2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#ifndef __VIR_AUTHCONFIG_H__ +# define __VIR_AUTHCONFIG_H__ + +# include "internal.h" + +typedef struct _virAuthConfig virAuthConfig; +typedef virAuthConfig *virAuthConfigPtr; + + +virAuthConfigPtr virAuthConfigNew(const char *path); +virAuthConfigPtr virAuthConfigNewData(const char *path, + const char *data, + size_t len); + +void virAuthConfigFree(virAuthConfigPtr auth); + +int virAuthConfigLookup(virAuthConfigPtr auth, + const char *service, + const char *hostname, + const char *credname, + const char **value); + +#endif /* __VIR_AUTHCONFIG_H__ */ diff --git a/tests/Makefile.am b/tests/Makefile.am index 4dde3e9..8ba7e66 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -96,7 +96,8 @@ check_PROGRAMS = virshtest conftest sockettest \ commandtest commandhelper seclabeltest \ virhashtest virnetmessagetest virnetsockettest ssh \ utiltest virnettlscontexttest shunloadtest \ - virtimetest viruritest virkeyfiletest + virtimetest viruritest virkeyfiletest \ + virauthconfigtest check_LTLIBRARIES = libshunload.la @@ -221,6 +222,7 @@ TESTS = virshtest \ virtimetest \ viruritest \ virkeyfiletest \ + virauthconfigtest \ shunloadtest \ utiltest \ $(test_scripts) @@ -518,6 +520,11 @@ virkeyfiletest_SOURCES = \ virkeyfiletest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) virkeyfiletest_LDADD = ../src/libvirt-net-rpc.la $(LDADDS) +virauthconfigtest_SOURCES = \ + virauthconfigtest.c testutils.h testutils.c +virauthconfigtest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) +virauthconfigtest_LDADD = ../src/libvirt-net-rpc.la $(LDADDS) + seclabeltest_SOURCES = \ seclabeltest.c seclabeltest_LDADD = ../src/libvirt_driver_security.la $(LDADDS) diff --git a/tests/virauthconfigtest.c b/tests/virauthconfigtest.c new file mode 100644 index 0000000..b4f08fd --- /dev/null +++ b/tests/virauthconfigtest.c @@ -0,0 +1,140 @@ +/* + * Copyright (C) 2011 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include <stdlib.h> +#include <signal.h> + +#include "testutils.h" +#include "util.h" +#include "virterror_internal.h" +#include "memory.h" +#include "logging.h" + +#include "virauthconfig.h" + +#define VIR_FROM_THIS VIR_FROM_RPC + +struct ConfigLookupData { + virAuthConfigPtr config; + const char *hostname; + const char *service; + const char *credname; + const char *expect; +}; + +static int testAuthLookup(const void *args) +{ + int ret = -1; + const struct ConfigLookupData *data = args; + const char *actual = NULL; + int rv; + + rv = virAuthConfigLookup(data->config, + data->service, + data->hostname, + data->credname, + &actual); + + if (rv < 0) + goto cleanup; + + if (data->expect) { + if (!actual || + !STREQ(actual, data->expect)) { + VIR_WARN("Expected value '%s' for '%s' '%s' '%s', but got '%s'", + data->expect, data->hostname, + data->service, data->credname, + NULLSTR(actual)); + goto cleanup; + } + } else { + if (actual) { + VIR_WARN("Did not expect a value for '%s' '%s' '%s', but got '%s'", + data->hostname, + data->service, data->credname, + actual); + goto cleanup; + } + } + + ret = 0; +cleanup: + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + virAuthConfigPtr config; + + signal(SIGPIPE, SIG_IGN); + +#define TEST_LOOKUP(config, hostname, service, credname, expect) \ + do { \ + const struct ConfigLookupData data = { \ + config, hostname, service, credname, expect \ + }; \ + if (virtTestRun("Test Lookup " hostname "-" service "-" credname, \ + 1, testAuthLookup, &data) < 0) \ + ret = -1; \ + } while (0) + + const char *confdata = + "[credentials-test]\n" + "username=fred\n" + "password=123456\n" + "\n" + "[credentials-prod]\n" + "username=bar\n" + "password=letmein\n" + "\n" + "[auth-libvirt-test1.example.com]\n" + "credentials=test\n" + "\n" + "[auth-libvirt-test2.example.com]\n" + "credentials=test\n" + "\n" + "[auth-libvirt-demo3.example.com]\n" + "credentials=test\n" + "\n" + "[auth-libvirt-prod1.example.com]\n" + "credentials=prod\n"; + + if (!(config = virAuthConfigNewData("auth.conf", confdata, strlen(confdata)))) + return EXIT_FAILURE; + + TEST_LOOKUP(config, "test1.example.com", "libvirt", "username", "fred"); + TEST_LOOKUP(config, "test1.example.com", "vnc", "username", NULL); + TEST_LOOKUP(config, "test1.example.com", "libvirt", "realm", NULL); + TEST_LOOKUP(config, "test66.example.com", "libvirt", "username", NULL); + TEST_LOOKUP(config, "prod1.example.com", "libvirt", "username", "bar"); + TEST_LOOKUP(config, "prod1.example.com", "libvirt", "password", "letmein"); + + virAuthConfigFree(config); + + return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); +} + +VIRT_TEST_MAIN(mymain) -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
This defines the format for the auth credential config file and provides APIs to access the data. The config file contains one or more named 'credential' sets
[credentials-$NAME] credname1=value1 credname2=value2
eg
[credentials-test] authname=fred password=123456
[credentials-prod] authname=bar password=letmein
There are then one or more 'auth' sets which match services/hosts and map to credential sets.
[auth-$SERVICE-$HOSTNAME] credentials=$CREDENTIALS
eg
[auth-libvirt-test1.example.com] credentials=test
[auth-libvirt-test2.example.com] credentials=test
[auth-libvirt-demo3.example.com] credentials=test
[auth-libvirt-prod1.example.com] credentials=prod
* docs/auth.html.in: Document use of client auth config files * src/Makefile.am, src/libvirt_private.syms, src/util/virauthconfig.c, src/util/virauthconfig.h: Add APIs for processing auth.conf file --- docs/auth.html.in | 118 ++++++++++++++++++++++++++++++- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 7 ++ src/util/virauthconfig.c | 175 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virauthconfig.h | 45 ++++++++++++ tests/Makefile.am | 9 ++- tests/virauthconfigtest.c | 140 ++++++++++++++++++++++++++++++++++++ 8 files changed, 494 insertions(+), 2 deletions(-) create mode 100644 src/util/virauthconfig.c create mode 100644 src/util/virauthconfig.h create mode 100644 tests/virauthconfigtest.c
diff --git a/docs/auth.html.in b/docs/auth.html.in index 2163959..ecff0fc 100644 --- a/docs/auth.html.in +++ b/docs/auth.html.in @@ -1,7 +1,7 @@ <?xml version="1.0"?> <html> <body> -<h1>Access control</h1> +<h1>Authentication& access control</h1> <p> When connecting to libvirt, some connections may require client authentication before allowing use of the APIs. The set of possible @@ -11,6 +11,122 @@
<ul id="toc"></ul>
+<h2><a name="Auth_client_config">Client configuration</a></h2> + +<p> + When connecting to a remote hypervisor which requires authentication, +most libvirt applications will prompt the user for the credentials. It is +also possible to provide a client configuration file containing all the +authentication credentials, avoiding any interaction. Libvirt will look +for the authentication file using the following sequence: +</p> +<ol> +<li>The file path specified by the $LIBVIRT_AUTH_FILE environment + variable.</li> +<li>The file path specified by the "authfile=/some/file" URI + query parameter</li> +<li>The file $HOME/.libvirt/auth.conf</li> +<li>The file /etc/libvirt/auth.conf</li> +</ol> + +<p> + The auth configuration file uses the traditional<code>".ini"</code> + style syntax. There are two types of groups that can be present in + the config. First there are one or more<strong>credential</strong> + sets, which provide the actual authentication credentials. The keys + within the group may be: +</p> + +<ul> +<li><code>username</code>: the user login name to act as. This + is relevant for ESX, Xen, HyperV and SSH, but probably not + the one you want to libvirtd with SASL.</li> +<li><code>authname</code>: the name to authorize as. This is + what is commonly required for libvirtd with SASL.</li> +<li><code>password</code>: the secret password</li> +<li><code>realm</code>: the domain realm for SASL, mostly + unused</li> +</ul> + +<p> + Each set of credentials has a name, which is part of the group + entry name. Overall the syntax is +</p> + +<pre> +[credentials-$NAME] +credname1=value1 +credname2=value2</pre> + +<p> + For example, to define two sets of credentials used for production + and test machines, using libvirtd, and a further ESX server for dev: +</p> +<pre> +[credentials-test] +authname=fred +password=123456 + +[credentials-prod] +authname=bar +password=letmein + +[credentials-dev] +username=joe +password=hello</pre> + +<p> + The second set of groups provide mappings of credentials to + specific machine services. The config file group names compromise + the service type and host: +</p> + +<pre> +[auth-$SERVICE-$HOSTNAME] +credentials=$CREDENTIALS</pre> + +<p> + For example, following the previous example, here is how to + list some machines +</p> + +<pre> +[auth-libvirt-test1.example.com] +credentials=test + +[auth-libvirt-test2.example.com] +credentials=test + +[auth-libvirt-demo3.example.com] +credentials=test + +[auth-libvirt-prod1.example.com] +credentials=prod + +[auth-esx-dev1.example.com] +credentials=dev</pre> + +<p> + The following service types are known to libvirt +</p> + +<ol> +<li><code>libvirt</code> - used for connections to a libvirtd + server, which is configured with SASL auth</li> +<li><code>ssh</code> - used for connections to a Phyp server + over SSH</li> +<li><code>esx</code> - used for connections to an ESX or + VirtualCenter server</li> +<li><code>xen</code> - used for connections to a Xen Enterprise + sever using XenAPI</li> +</ol> + +<p> + Applications using libvirt are free to use this same configuration + file for storing other credentials. For example, it can be used + to storage VNC or SPICE login credentials +</p> + <h2><a name="ACL_server_config">Server configuration</a></h2> <p> The libvirt daemon allows the administrator to choose the authentication diff --git a/po/POTFILES.in b/po/POTFILES.in index 8eaa8ad..6488c4c 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -124,6 +124,7 @@ src/util/storage_file.c src/util/sysinfo.c src/util/util.c src/util/virauth.c +src/util/virauthconfig.c src/util/viraudit.c src/util/virfile.c src/util/virhash.c diff --git a/src/Makefile.am b/src/Makefile.am index 3cbf9d7..a2aae9d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -81,6 +81,7 @@ UTIL_SOURCES = \ util/util.c util/util.h \ util/viraudit.c util/viraudit.h \ util/virauth.c util/virauth.h \ + util/virauthconfig.c util/virauthconfig.h \ util/virfile.c util/virfile.h \ util/virnodesuspend.c util/virnodesuspend.h \ util/virpidfile.c util/virpidfile.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6f53aa8..09e2c35 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1171,6 +1171,13 @@ virAuthGetPassword; virAuthGetConfigFilePath;
+# virauthconfig.h +virAuthConfigNew; +virAuthConfigNewData; +virAuthConfigLookup; +virAuthConfigFree;
Up virAuthConfigFree.
+ + # viraudit.h virAuditClose; virAuditEncode; diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c new file mode 100644 index 0000000..ad98959 --- /dev/null +++ b/src/util/virauthconfig.c @@ -0,0 +1,175 @@ +/* + * virauthconfig.c: authentication config handling + * + * Copyright (C) 2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange<berrange@redhat.com> + */ + +#include<config.h> + +#include "virauthconfig.h" + +#include "virkeyfile.h" +#include "memory.h" +#include "util.h" +#include "logging.h" +#include "virterror_internal.h" + + +struct _virAuthConfig { + virKeyFilePtr keyfile; + char *path; +}; + +#define VIR_FROM_THIS VIR_FROM_NONE + +#define virAuthReportError(code, ...) \ + virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) + + +virAuthConfigPtr virAuthConfigNew(const char *path) +{ + virAuthConfigPtr auth; + + if (VIR_ALLOC(auth)< 0) { + virReportOOMError(); + goto error; + } + + if (!(auth->path = strdup(path))) { + virReportOOMError(); + goto error; + } + + if (!(auth->keyfile = virKeyFileNew())) + goto error; + + if (virKeyFileLoadFile(auth->keyfile, path)< 0) + goto error; + + return auth; + +error: + virAuthConfigFree(auth); + return NULL; +} + + +virAuthConfigPtr virAuthConfigNewData(const char *path, + const char *data, + size_t len) +{ + virAuthConfigPtr auth; + + if (VIR_ALLOC(auth)< 0) { + virReportOOMError(); + goto error; + } + + if (!(auth->path = strdup(path))) { + virReportOOMError(); + goto error; + } + + if (!(auth->keyfile = virKeyFileNew())) + goto error; + + if (virKeyFileLoadData(auth->keyfile, path, data, len)< 0) + goto error; + + return auth; + +error: + virAuthConfigFree(auth); + return NULL; +} + + +void virAuthConfigFree(virAuthConfigPtr auth) +{ + if (!auth) + return; + + virKeyFileFree(auth->keyfile); + VIR_FREE(auth->path); + VIR_FREE(auth); +} + + +int virAuthConfigLookup(virAuthConfigPtr auth, + const char *service, + const char *hostname, + const char *credname, + const char **value) +{ + char *authgroup = NULL; + char *credgroup = NULL; + const char *authcred; + int ret = -1; + + *value = NULL; + + VIR_DEBUG("Lookup '%s' '%s' '%s'", service, NULLSTR(hostname), credname); + + if (!hostname) + hostname = "localhost"; + + if (virAsprintf(&authgroup, "auth-%s-%s", service, hostname)< 0) { + virReportOOMError(); + goto cleanup; + } + + if (!virKeyFileHasGroup(auth->keyfile, authgroup)) { + ret = 0; + goto cleanup; + } + + if (!(authcred = virKeyFileGetValueString(auth->keyfile, authgroup, "credentials"))) { + virAuthReportError(VIR_ERR_CONF_SYNTAX, + _("Missing item 'credentials' in group '%s' in '%s'"), + authgroup, auth->path); + goto cleanup; + } + + if (virAsprintf(&credgroup, "credentials-%s", authcred)< 0) { + virReportOOMError(); + goto cleanup; + } + + if (!virKeyFileHasGroup(auth->keyfile, credgroup)) { + virAuthReportError(VIR_ERR_CONF_SYNTAX, + _("Missing group 'credentials-%s' referenced from group '%s' in '%s'"), + authcred, authgroup, auth->path); + goto cleanup; + } + + if (!virKeyFileHasValue(auth->keyfile, credgroup, credname)) { + ret = 0; + goto cleanup; + } + + *value = virKeyFileGetValueString(auth->keyfile, credgroup, credname); + + ret = 0; + +cleanup: + VIR_FREE(authgroup); + VIR_FREE(credgroup); + return ret; +} diff --git a/src/util/virauthconfig.h b/src/util/virauthconfig.h new file mode 100644 index 0000000..cbeef85 --- /dev/null +++ b/src/util/virauthconfig.h @@ -0,0 +1,45 @@ +/* + * virauthconfig.h: authentication config handling + * + * Copyright (C) 2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange<berrange@redhat.com> + */ + +#ifndef __VIR_AUTHCONFIG_H__ +# define __VIR_AUTHCONFIG_H__ + +# include "internal.h" + +typedef struct _virAuthConfig virAuthConfig; +typedef virAuthConfig *virAuthConfigPtr; + + +virAuthConfigPtr virAuthConfigNew(const char *path); +virAuthConfigPtr virAuthConfigNewData(const char *path, + const char *data, + size_t len); + +void virAuthConfigFree(virAuthConfigPtr auth); + +int virAuthConfigLookup(virAuthConfigPtr auth, + const char *service, + const char *hostname, + const char *credname, + const char **value); + +#endif /* __VIR_AUTHCONFIG_H__ */ diff --git a/tests/Makefile.am b/tests/Makefile.am index 4dde3e9..8ba7e66 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -96,7 +96,8 @@ check_PROGRAMS = virshtest conftest sockettest \ commandtest commandhelper seclabeltest \ virhashtest virnetmessagetest virnetsockettest ssh \ utiltest virnettlscontexttest shunloadtest \ - virtimetest viruritest virkeyfiletest + virtimetest viruritest virkeyfiletest \ + virauthconfigtest
check_LTLIBRARIES = libshunload.la
@@ -221,6 +222,7 @@ TESTS = virshtest \ virtimetest \ viruritest \ virkeyfiletest \ + virauthconfigtest \ shunloadtest \ utiltest \ $(test_scripts) @@ -518,6 +520,11 @@ virkeyfiletest_SOURCES = \ virkeyfiletest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) virkeyfiletest_LDADD = ../src/libvirt-net-rpc.la $(LDADDS)
+virauthconfigtest_SOURCES = \ + virauthconfigtest.c testutils.h testutils.c +virauthconfigtest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) +virauthconfigtest_LDADD = ../src/libvirt-net-rpc.la $(LDADDS) + seclabeltest_SOURCES = \ seclabeltest.c seclabeltest_LDADD = ../src/libvirt_driver_security.la $(LDADDS) diff --git a/tests/virauthconfigtest.c b/tests/virauthconfigtest.c new file mode 100644 index 0000000..b4f08fd --- /dev/null +++ b/tests/virauthconfigtest.c @@ -0,0 +1,140 @@ +/* + * Copyright (C) 2011 Red Hat, Inc.
s/2011/2012/
+ * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange<berrange@redhat.com> + */ + +#include<config.h> + +#include<stdlib.h> +#include<signal.h> + +#include "testutils.h" +#include "util.h" +#include "virterror_internal.h" +#include "memory.h" +#include "logging.h" + +#include "virauthconfig.h" + +#define VIR_FROM_THIS VIR_FROM_RPC + +struct ConfigLookupData { + virAuthConfigPtr config; + const char *hostname; + const char *service; + const char *credname; + const char *expect; +}; + +static int testAuthLookup(const void *args) +{ + int ret = -1; + const struct ConfigLookupData *data = args; + const char *actual = NULL; + int rv; + + rv = virAuthConfigLookup(data->config, + data->service, + data->hostname, + data->credname, +&actual); + + if (rv< 0) + goto cleanup; + + if (data->expect) { + if (!actual || + !STREQ(actual, data->expect)) { + VIR_WARN("Expected value '%s' for '%s' '%s' '%s', but got '%s'", + data->expect, data->hostname, + data->service, data->credname, + NULLSTR(actual)); + goto cleanup; + } + } else { + if (actual) { + VIR_WARN("Did not expect a value for '%s' '%s' '%s', but got '%s'", + data->hostname, + data->service, data->credname, + actual); + goto cleanup; + } + } + + ret = 0; +cleanup: + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + virAuthConfigPtr config; + + signal(SIGPIPE, SIG_IGN); + +#define TEST_LOOKUP(config, hostname, service, credname, expect) \ + do { \ + const struct ConfigLookupData data = { \ + config, hostname, service, credname, expect \ + }; \ + if (virtTestRun("Test Lookup " hostname "-" service "-" credname, \ + 1, testAuthLookup,&data)< 0) \ + ret = -1; \ + } while (0) + + const char *confdata = + "[credentials-test]\n" + "username=fred\n" + "password=123456\n" + "\n" + "[credentials-prod]\n" + "username=bar\n" + "password=letmein\n" + "\n" + "[auth-libvirt-test1.example.com]\n" + "credentials=test\n" + "\n" + "[auth-libvirt-test2.example.com]\n" + "credentials=test\n" + "\n" + "[auth-libvirt-demo3.example.com]\n" + "credentials=test\n" + "\n" + "[auth-libvirt-prod1.example.com]\n" + "credentials=prod\n"; + + if (!(config = virAuthConfigNewData("auth.conf", confdata, strlen(confdata)))) + return EXIT_FAILURE; + + TEST_LOOKUP(config, "test1.example.com", "libvirt", "username", "fred"); + TEST_LOOKUP(config, "test1.example.com", "vnc", "username", NULL); + TEST_LOOKUP(config, "test1.example.com", "libvirt", "realm", NULL); + TEST_LOOKUP(config, "test66.example.com", "libvirt", "username", NULL); + TEST_LOOKUP(config, "prod1.example.com", "libvirt", "username", "bar"); + TEST_LOOKUP(config, "prod1.example.com", "libvirt", "password", "letmein"); + + virAuthConfigFree(config); + + return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); +} + +VIRT_TEST_MAIN(mymain)
ACK with the nits fixed.
On 03/20/2012 11:33 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
This defines the format for the auth credential config file and provides APIs to access the data. The config file contains one or more named 'credential' sets
[credentials-$NAME] credname1=value1 credname2=value2
eg
[credentials-test] authname=fred password=123456
I'm not always a fan of plain-text passwords; do you have plans to further enhance this to hook into our virSecret design, where the config file can list the name of a secret to reference, which in turn will trigger appropriate calls to the virSecret API to grab credentials on first use, securely caching them for later uses that need the same credentials but without the drawbacks of plain-text config files? But that's future enhancement, and doesn't stop this patch from going in once you address Osier's review comments. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Mar 22, 2012 at 07:02:31AM -0600, Eric Blake wrote:
On 03/20/2012 11:33 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
This defines the format for the auth credential config file and provides APIs to access the data. The config file contains one or more named 'credential' sets
[credentials-$NAME] credname1=value1 credname2=value2
eg
[credentials-test] authname=fred password=123456
I'm not always a fan of plain-text passwords; do you have plans to further enhance this to hook into our virSecret design, where the config file can list the name of a secret to reference, which in turn will trigger appropriate calls to the virSecret API to grab credentials on first use, securely caching them for later uses that need the same credentials but without the drawbacks of plain-text config files? But that's future enhancement, and doesn't stop this patch from going in once you address Osier's review comments.
These credentials are required in order to establish a connection to libvirt, so we don't have any virSecret APIs available yet. In addition this is client side, while the virSecret APIs are server side. Obviously this is not an ideal scenario from a security POV, but it is an optional feature. If people are using SASL Password auth and want to automate libvirt logins, there's not much choice in the matter. If they want something more secure they can setup SSH keys, or policy kit or Kerberos tickets. I envisage this as relevant for test/dev scenarios rather than production. 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 :|
From: "Daniel P. Berrange" <berrange@redhat.com> SASL may prompt for credentials after either a 'start' or 'step' invocation. In both cases the code to handle this is the same. Refactor this code into a separate method to reduce the duplication, since the complexity is about to grow * src/remote/remote_driver.c: Refactor interaction with SASL --- src/remote/remote_driver.c | 135 +++++++++++++++++++++++++------------------ 1 files changed, 78 insertions(+), 57 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index bc6fea2..1faaf9e 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2863,7 +2863,8 @@ static sasl_callback_t *remoteAuthMakeCallbacks(int *credtype, int ncredtype) * are basically a 1-to-1 copy of each other. */ static int remoteAuthMakeCredentials(sasl_interact_t *interact, - virConnectCredentialPtr *cred) + virConnectCredentialPtr *cred, + size_t *ncred) { int ninteract; if (!cred) @@ -2889,16 +2890,8 @@ static int remoteAuthMakeCredentials(sasl_interact_t *interact, (*cred)[ninteract].result = NULL; } - return ninteract; -} - -static void remoteAuthFreeCredentials(virConnectCredentialPtr cred, - int ncred) -{ - int i; - for (i = 0 ; i < ncred ; i++) - VIR_FREE(cred[i].result); - VIR_FREE(cred); + *ncred = ninteract; + return 0; } @@ -2919,6 +2912,69 @@ static void remoteAuthFillInteract(virConnectCredentialPtr cred, } } + +struct remoteAuthInteractState { + sasl_interact_t *interact; + virConnectCredentialPtr cred; + size_t ncred; +}; + + +static void remoteAuthInteractStateClear(struct remoteAuthInteractState *state) +{ + size_t i; + if (!state) + return; + + for (i = 0 ; i < state->ncred ; i++) + VIR_FREE(state->cred[i].result); + VIR_FREE(state->cred); + state->ncred = 0; +} + + +static int remoteAuthInteract(struct remoteAuthInteractState *state, + virConnectAuthPtr auth) +{ + int ret = -1; + + remoteAuthInteractStateClear(state); + + if (remoteAuthMakeCredentials(state->interact, &state->cred, &state->ncred) < 0) { + remoteError(VIR_ERR_AUTH_FAILED, "%s", + _("Failed to make auth credentials")); + goto cleanup; + } + + /* Run the authentication callback */ + if (!auth || !auth->cb) { + remoteError(VIR_ERR_AUTH_FAILED, "%s", + _("No authentication callback available")); + goto cleanup; + } + + if ((*(auth->cb))(state->cred, state->ncred, auth->cbdata) < 0) { + remoteError(VIR_ERR_AUTH_FAILED, "%s", + _("Failed to collect auth credentials")); + goto cleanup; + } + + remoteAuthFillInteract(state->cred, state->interact); + /* + * 'interact' now has pointers to strings in 'state->cred' + * so we must not free state->cred until the *next* + * sasl_start/step function is complete. Hence we + * call remoteAuthInteractStateClear() at the *start* + * of this method, rather than the end. + */ + + ret = 0; + +cleanup: + return ret; +} + + /* Perform the SASL authentication process */ static int @@ -2937,13 +2993,13 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int err, complete; int ssf; sasl_callback_t *saslcb = NULL; - sasl_interact_t *interact = NULL; - virConnectCredentialPtr cred = NULL; - int ncred = 0; int ret = -1; const char *mechlist; virNetSASLContextPtr saslCtxt; virNetSASLSessionPtr sasl = NULL; + struct remoteAuthInteractState state; + + memset(&state, 0, sizeof(state)); VIR_DEBUG("Client initialize SASL authentication"); @@ -3010,7 +3066,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, VIR_DEBUG("Client start negotiation mechlist '%s'", mechlist); if ((err = virNetSASLSessionClientStart(sasl, mechlist, - &interact, + &state.interact, &clientout, &clientoutlen, &mech)) < 0) @@ -3018,29 +3074,11 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, /* Need to gather some credentials from the client */ if (err == VIR_NET_SASL_INTERACT) { - const char *msg; - if (cred) { - remoteAuthFreeCredentials(cred, ncred); - cred = NULL; - } - if ((ncred = remoteAuthMakeCredentials(interact, &cred)) < 0) { - remoteError(VIR_ERR_AUTH_FAILED, "%s", - _("Failed to make auth credentials")); + if (remoteAuthInteract(&state, auth) < 0) { VIR_FREE(iret.mechlist); goto cleanup; } - /* Run the authentication callback */ - if (auth && auth->cb) { - if ((*(auth->cb))(cred, ncred, auth->cbdata) >= 0) { - remoteAuthFillInteract(cred, interact); - goto restart; - } - msg = "Failed to collect auth credentials"; - } else { - msg = "No authentication callback available"; - } - remoteError(VIR_ERR_AUTH_FAILED, "%s", msg); - goto cleanup; + goto restart; } VIR_FREE(iret.mechlist); @@ -3081,35 +3119,18 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, if ((err = virNetSASLSessionClientStep(sasl, serverin, serverinlen, - &interact, + &state.interact, &clientout, &clientoutlen)) < 0) goto cleanup; /* Need to gather some credentials from the client */ if (err == VIR_NET_SASL_INTERACT) { - const char *msg; - if (cred) { - remoteAuthFreeCredentials(cred, ncred); - cred = NULL; - } - if ((ncred = remoteAuthMakeCredentials(interact, &cred)) < 0) { - remoteError(VIR_ERR_AUTH_FAILED, "%s", - _("Failed to make auth credentials")); + if (remoteAuthInteract(&state, auth) < 0) { + VIR_FREE(iret.mechlist); goto cleanup; } - /* Run the authentication callback */ - if (auth && auth->cb) { - if ((*(auth->cb))(cred, ncred, auth->cbdata) >= 0) { - remoteAuthFillInteract(cred, interact); - goto restep; - } - msg = _("Failed to collect auth credentials"); - } else { - msg = _("No authentication callback available"); - } - remoteError(VIR_ERR_AUTH_FAILED, "%s", msg); - goto cleanup; + goto restep; } VIR_FREE(serverin); @@ -3171,8 +3192,8 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, cleanup: VIR_FREE(serverin); + remoteAuthInteractStateClear(&state); VIR_FREE(saslcb); - remoteAuthFreeCredentials(cred, ncred); virNetSASLSessionFree(sasl); virNetSASLContextFree(saslCtxt); -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
SASL may prompt for credentials after either a 'start' or 'step' invocation. In both cases the code to handle this is the same. Refactor this code into a separate method to reduce the duplication, since the complexity is about to grow
* src/remote/remote_driver.c: Refactor interaction with SASL --- src/remote/remote_driver.c | 135 +++++++++++++++++++++++++------------------ 1 files changed, 78 insertions(+), 57 deletions(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index bc6fea2..1faaf9e 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2863,7 +2863,8 @@ static sasl_callback_t *remoteAuthMakeCallbacks(int *credtype, int ncredtype) * are basically a 1-to-1 copy of each other. */ static int remoteAuthMakeCredentials(sasl_interact_t *interact, - virConnectCredentialPtr *cred) + virConnectCredentialPtr *cred, + size_t *ncred) { int ninteract; if (!cred) @@ -2889,16 +2890,8 @@ static int remoteAuthMakeCredentials(sasl_interact_t *interact, (*cred)[ninteract].result = NULL; }
- return ninteract; -} - -static void remoteAuthFreeCredentials(virConnectCredentialPtr cred, - int ncred) -{ - int i; - for (i = 0 ; i< ncred ; i++) - VIR_FREE(cred[i].result); - VIR_FREE(cred); + *ncred = ninteract; + return 0;
Better to change the comments for remoteAuthFreeCredentials. ACK.
From: "Daniel P. Berrange" <berrange@redhat.com> When SASL requests auth credentials, try to look them up in the config file first. If any are found, remove them from the list that the user is prompted for --- src/esx/esx_driver.c | 58 +++++++++--------- src/hyperv/hyperv_driver.c | 4 +- src/phyp/phyp_driver.c | 4 +- src/remote/remote_driver.c | 138 ++++++++++++++++++++++++++++++++++++-------- src/util/virauth.c | 69 +++++++++++++++++++++- src/util/virauth.h | 10 +++- src/xenapi/xenapi_driver.c | 4 +- 7 files changed, 224 insertions(+), 63 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 7689306..6962832 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -667,10 +667,8 @@ esxCapsInit(esxPrivate *priv) static int -esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, - const char *hostname, int port, - const char *predefinedUsername, - esxVI_ProductVersion expectedProductVersion, +esxConnectToHost(virConnectPtr conn, + virConnectAuthPtr auth, char **vCenterIpAddress) { int result = -1; @@ -682,25 +680,29 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, esxVI_String *propertyNameList = NULL; esxVI_ObjectContent *hostSystem = NULL; esxVI_Boolean inMaintenanceMode = esxVI_Boolean_Undefined; + esxPrivate *priv = conn->privateData; + esxVI_ProductVersion expectedProductVersion = STRCASEEQ(conn->uri->scheme, "esx") + ? esxVI_ProductVersion_ESX + : esxVI_ProductVersion_GSX; if (vCenterIpAddress == NULL || *vCenterIpAddress != NULL) { ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); return -1; } - if (esxUtil_ResolveHostname(hostname, ipAddress, NI_MAXHOST) < 0) { + if (esxUtil_ResolveHostname(conn->uri->server, ipAddress, NI_MAXHOST) < 0) { return -1; } - if (predefinedUsername != NULL) { - username = strdup(predefinedUsername); + if (conn->uri->user != NULL) { + username = strdup(conn->uri->user); if (username == NULL) { virReportOOMError(); goto cleanup; } } else { - username = virAuthGetUsername(auth, "root", hostname); + username = virAuthGetUsername(conn, auth, "esx", "root", conn->uri->server); if (username == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); @@ -708,7 +710,7 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, } } - unescapedPassword = virAuthGetPassword(auth, username, hostname); + unescapedPassword = virAuthGetPassword(conn, auth, "esx", username, conn->uri->server); if (unescapedPassword == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); @@ -722,7 +724,7 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, } if (virAsprintf(&url, "%s://%s:%d/sdk", priv->parsedUri->transport, - hostname, port) < 0) { + conn->uri->server, conn->uri->port) < 0) { virReportOOMError(); goto cleanup; } @@ -743,13 +745,13 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, priv->host->productVersion != esxVI_ProductVersion_ESX5x) { ESX_ERROR(VIR_ERR_INTERNAL_ERROR, _("%s is neither an ESX 3.5, 4.x nor 5.x host"), - hostname); + conn->uri->server); goto cleanup; } } else { /* GSX */ if (priv->host->productVersion != esxVI_ProductVersion_GSX20) { ESX_ERROR(VIR_ERR_INTERNAL_ERROR, - _("%s isn't a GSX 2.0 host"), hostname); + _("%s isn't a GSX 2.0 host"), conn->uri->server); goto cleanup; } } @@ -799,9 +801,9 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, static int -esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, - const char *hostname, int port, - const char *predefinedUsername, +esxConnectToVCenter(virConnectPtr conn, + virConnectAuthPtr auth, + const char *hostname, const char *hostSystemIpAddress) { int result = -1; @@ -810,6 +812,7 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, char *unescapedPassword = NULL; char *password = NULL; char *url = NULL; + esxPrivate *priv = conn->privateData; if (hostSystemIpAddress == NULL && (priv->parsedUri->path == NULL || STREQ(priv->parsedUri->path, "/"))) { @@ -822,15 +825,15 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, return -1; } - if (predefinedUsername != NULL) { - username = strdup(predefinedUsername); + if (conn->uri->user != NULL) { + username = strdup(conn->uri->user); if (username == NULL) { virReportOOMError(); goto cleanup; } } else { - username = virAuthGetUsername(auth, "administrator", hostname); + username = virAuthGetUsername(conn, auth, "esx", "administrator", hostname); if (username == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); @@ -838,7 +841,7 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, } } - unescapedPassword = virAuthGetPassword(auth, username, hostname); + unescapedPassword = virAuthGetPassword(conn, auth, "esx", username, hostname); if (unescapedPassword == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); @@ -852,7 +855,7 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, } if (virAsprintf(&url, "%s://%s:%d/sdk", priv->parsedUri->transport, - hostname, port) < 0) { + hostname, conn->uri->port) < 0) { virReportOOMError(); goto cleanup; } @@ -1046,11 +1049,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, if (STRCASEEQ(conn->uri->scheme, "esx") || STRCASEEQ(conn->uri->scheme, "gsx")) { /* Connect to host */ - if (esxConnectToHost(priv, auth, conn->uri->server, conn->uri->port, - conn->uri->user, - STRCASEEQ(conn->uri->scheme, "esx") - ? esxVI_ProductVersion_ESX - : esxVI_ProductVersion_GSX, + if (esxConnectToHost(conn, auth, &potentialVCenterIpAddress) < 0) { goto cleanup; } @@ -1089,8 +1088,8 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, } } - if (esxConnectToVCenter(priv, auth, vCenterIpAddress, - conn->uri->port, NULL, + if (esxConnectToVCenter(conn, auth, + vCenterIpAddress, priv->host->ipAddress) < 0) { goto cleanup; } @@ -1099,8 +1098,9 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, priv->primary = priv->host; } else { /* VPX */ /* Connect to vCenter */ - if (esxConnectToVCenter(priv, auth, conn->uri->server, conn->uri->port, - conn->uri->user, NULL) < 0) { + if (esxConnectToVCenter(conn, auth, + conn->uri->server, + NULL) < 0) { goto cleanup; } diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 0469e2e..05fce4f 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -147,7 +147,7 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) goto cleanup; } } else { - username = virAuthGetUsername(auth, "administrator", conn->uri->server); + username = virAuthGetUsername(conn, auth, "hyperv", "administrator", conn->uri->server); if (username == NULL) { HYPERV_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); @@ -155,7 +155,7 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) } } - password = virAuthGetPassword(auth, username, conn->uri->server); + password = virAuthGetPassword(conn, auth, "hyperv", username, conn->uri->server); if (password == NULL) { HYPERV_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 470706d..b883b56 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1006,7 +1006,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, goto err; } - username = virAuthGetUsername(auth, NULL, conn->uri->server); + username = virAuthGetUsername(conn, auth, "ssh", NULL, conn->uri->server); if (username == NULL) { PHYP_ERROR(VIR_ERR_AUTH_FAILED, "%s", @@ -1087,7 +1087,7 @@ keyboard_interactive: goto disconnect; } - password = virAuthGetPassword(auth, username, conn->uri->server); + password = virAuthGetPassword(conn, auth, "ssh", username, conn->uri->server); if (password == NULL) { PHYP_ERROR(VIR_ERR_AUTH_FAILED, "%s", diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1faaf9e..72b4b8f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -45,6 +45,8 @@ #include "intprops.h" #include "virtypedparam.h" #include "viruri.h" +#include "virauth.h" +#include "virauthconfig.h" #define VIR_FROM_THIS VIR_FROM_REMOTE @@ -461,6 +463,9 @@ doRemoteOpen (virConnectPtr conn, pkipath = strdup(var->value); if (!pkipath) goto out_of_memory; var->ignore = 1; + } else if (STRCASEEQ(var->name, "authfile")) { + /* Strip this param, used by virauth.c */ + var->ignore = 1; } else { VIR_DEBUG("passing through variable '%s' ('%s') to remote end", var->name, var->value); @@ -2870,27 +2875,35 @@ static int remoteAuthMakeCredentials(sasl_interact_t *interact, if (!cred) return -1; - for (ninteract = 0 ; interact[ninteract].id != 0 ; ninteract++) - ; /* empty */ + for (ninteract = 0, *ncred = 0 ; interact[ninteract].id != 0 ; ninteract++) { + if (interact[ninteract].result) + continue; + (*ncred)++; + } - if (VIR_ALLOC_N(*cred, ninteract) < 0) + if (VIR_ALLOC_N(*cred, *ncred) < 0) return -1; - for (ninteract = 0 ; interact[ninteract].id != 0 ; ninteract++) { - (*cred)[ninteract].type = remoteAuthCredSASL2Vir(interact[ninteract].id); - if (!(*cred)[ninteract].type) { + for (ninteract = 0, *ncred = 0 ; interact[ninteract].id != 0 ; ninteract++) { + if (interact[ninteract].result) + continue; + + (*cred)[*ncred].type = remoteAuthCredSASL2Vir(interact[ninteract].id); + if (!(*cred)[*ncred].type) { + *ncred = 0; VIR_FREE(*cred); return -1; } - if (interact[ninteract].challenge) - (*cred)[ninteract].challenge = interact[ninteract].challenge; - (*cred)[ninteract].prompt = interact[ninteract].prompt; - if (interact[ninteract].defresult) - (*cred)[ninteract].defresult = interact[ninteract].defresult; - (*cred)[ninteract].result = NULL; + if (interact[*ncred].challenge) + (*cred)[*ncred].challenge = interact[ninteract].challenge; + (*cred)[*ncred].prompt = interact[ninteract].prompt; + if (interact[*ncred].defresult) + (*cred)[*ncred].defresult = interact[ninteract].defresult; + (*cred)[*ncred].result = NULL; + + (*ncred)++; } - *ncred = ninteract; return 0; } @@ -2905,22 +2918,91 @@ static int remoteAuthMakeCredentials(sasl_interact_t *interact, static void remoteAuthFillInteract(virConnectCredentialPtr cred, sasl_interact_t *interact) { - int ninteract; - for (ninteract = 0 ; interact[ninteract].id != 0 ; ninteract++) { - interact[ninteract].result = cred[ninteract].result; - interact[ninteract].len = cred[ninteract].resultlen; + int ninteract, ncred; + for (ninteract = 0, ncred = 0 ; interact[ninteract].id != 0 ; ninteract++) { + if (interact[ninteract].result) + continue; + interact[ninteract].result = cred[ncred].result; + interact[ninteract].len = cred[ncred].resultlen; + ncred++; } } - struct remoteAuthInteractState { sasl_interact_t *interact; virConnectCredentialPtr cred; size_t ncred; + virAuthConfigPtr config; }; -static void remoteAuthInteractStateClear(struct remoteAuthInteractState *state) + +static int remoteAuthFillFromConfig(virConnectPtr conn, + struct remoteAuthInteractState *state) +{ + int ret = -1; + int ninteract; + const char *credname; + char *path = NULL; + + VIR_DEBUG("Trying to fill auth parameters from config file"); + + if (!state->config) { + if (virAuthGetConfigFilePath(conn, &path) < 0) + goto cleanup; + if (path == NULL) { + ret = 0; + goto cleanup; + } + + if (!(state->config = virAuthConfigNew(path))) + goto cleanup; + } + + for (ninteract = 0 ; state->interact[ninteract].id != 0 ; ninteract++) { + const char *value = NULL; + + switch (state->interact[ninteract].id) { + case SASL_CB_USER: + credname = "username"; + break; + case SASL_CB_AUTHNAME: + credname = "authname"; + break; + case SASL_CB_PASS: + credname = "password"; + break; + case SASL_CB_GETREALM: + credname = "realm"; + break; + default: + credname = NULL; + break; + } + + if (virAuthConfigLookup(state->config, + "libvirt", + conn->uri->server, + credname, + &value) < 0) + goto cleanup; + + if (value) { + state->interact[ninteract].result = value; + state->interact[ninteract].len = strlen(value); + } + } + + ret = 0; + +cleanup: + VIR_FREE(path); + return ret; +} + + +static void remoteAuthInteractStateClear(struct remoteAuthInteractState *state, + bool final) { size_t i; if (!state) @@ -2930,15 +3012,23 @@ static void remoteAuthInteractStateClear(struct remoteAuthInteractState *state) VIR_FREE(state->cred[i].result); VIR_FREE(state->cred); state->ncred = 0; + + if (final) + virAuthConfigFree(state->config); } -static int remoteAuthInteract(struct remoteAuthInteractState *state, +static int remoteAuthInteract(virConnectPtr conn, + struct remoteAuthInteractState *state, virConnectAuthPtr auth) { int ret = -1; - remoteAuthInteractStateClear(state); + VIR_DEBUG("Starting SASL interaction"); + remoteAuthInteractStateClear(state, false); + + if (remoteAuthFillFromConfig(conn, state) < 0) + goto cleanup; if (remoteAuthMakeCredentials(state->interact, &state->cred, &state->ncred) < 0) { remoteError(VIR_ERR_AUTH_FAILED, "%s", @@ -3074,7 +3164,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, /* Need to gather some credentials from the client */ if (err == VIR_NET_SASL_INTERACT) { - if (remoteAuthInteract(&state, auth) < 0) { + if (remoteAuthInteract(conn, &state, auth) < 0) { VIR_FREE(iret.mechlist); goto cleanup; } @@ -3126,7 +3216,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, /* Need to gather some credentials from the client */ if (err == VIR_NET_SASL_INTERACT) { - if (remoteAuthInteract(&state, auth) < 0) { + if (remoteAuthInteract(conn, &state, auth) < 0) { VIR_FREE(iret.mechlist); goto cleanup; } @@ -3192,7 +3282,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, cleanup: VIR_FREE(serverin); - remoteAuthInteractStateClear(&state); + remoteAuthInteractStateClear(&state, true); VIR_FREE(saslcb); virNetSASLSessionFree(sasl); virNetSASLContextFree(saslCtxt); diff --git a/src/util/virauth.c b/src/util/virauth.c index 150b8e7..940686f 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -30,6 +30,7 @@ #include "datatypes.h" #include "virterror_internal.h" #include "configmake.h" +#include "virauthconfig.h" #define VIR_FROM_THIS VIR_FROM_AUTH @@ -100,13 +101,68 @@ no_memory: } +static int +virAuthGetCredential(virConnectPtr conn, + const char *servicename, + const char *credname, + char **value) +{ + int ret = -1; + char *path = NULL; + virAuthConfigPtr config = NULL; + const char *tmp; + + *value = NULL; + + if (virAuthGetConfigFilePath(conn, &path) < 0) + goto cleanup; + + if (path == NULL) { + ret = 0; + goto cleanup; + } + + if (!(config = virAuthConfigNew(path))) + goto cleanup; + + if (virAuthConfigLookup(config, + servicename, + conn->uri->server, + credname, + &tmp) < 0) + goto cleanup; + + if (tmp && + !(*value = strdup(tmp))) { + virReportOOMError(); + goto cleanup; + } + + ret = 0; + +cleanup: + virAuthConfigFree(config); + VIR_FREE(path); + return ret; +} + + char * -virAuthGetUsername(virConnectAuthPtr auth, const char *defaultUsername, +virAuthGetUsername(virConnectPtr conn, + virConnectAuthPtr auth, + const char *servicename, + const char *defaultUsername, const char *hostname) { unsigned int ncred; virConnectCredential cred; char *prompt; + char *ret = NULL; + + if (virAuthGetCredential(conn, servicename, "username", &ret) < 0) + return NULL; + if (ret != NULL) + return ret; memset(&cred, 0, sizeof (virConnectCredential)); @@ -148,12 +204,21 @@ virAuthGetUsername(virConnectAuthPtr auth, const char *defaultUsername, char * -virAuthGetPassword(virConnectAuthPtr auth, const char *username, +virAuthGetPassword(virConnectPtr conn, + virConnectAuthPtr auth, + const char *servicename, + const char *username, const char *hostname) { unsigned int ncred; virConnectCredential cred; char *prompt; + char *ret = NULL; + + if (virAuthGetCredential(conn, servicename, "password", &ret) < 0) + return NULL; + if (ret != NULL) + return ret; memset(&cred, 0, sizeof (virConnectCredential)); diff --git a/src/util/virauth.h b/src/util/virauth.h index 7f43bee..1b315c7 100644 --- a/src/util/virauth.h +++ b/src/util/virauth.h @@ -27,9 +27,15 @@ int virAuthGetConfigFilePath(virConnectPtr conn, char **path); -char *virAuthGetUsername(virConnectAuthPtr auth, const char *defaultUsername, +char *virAuthGetUsername(virConnectPtr conn, + virConnectAuthPtr auth, + const char *servicename, + const char *defaultUsername, const char *hostname); -char *virAuthGetPassword(virConnectAuthPtr auth, const char *username, +char *virAuthGetPassword(virConnectPtr conn, + virConnectAuthPtr auth, + const char *servicename, + const char *username, const char *hostname); #endif /* __VIR_AUTH_H__ */ diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 3f88c91..2967f7c 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -138,7 +138,7 @@ xenapiOpen (virConnectPtr conn, virConnectAuthPtr auth, goto error; } } else { - username = virAuthGetUsername(auth, NULL, conn->uri->server); + username = virAuthGetUsername(conn, auth, "xen", NULL, conn->uri->server); if (username == NULL) { xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED, @@ -147,7 +147,7 @@ xenapiOpen (virConnectPtr conn, virConnectAuthPtr auth, } } - password = virAuthGetPassword(auth, username, conn->uri->server); + password = virAuthGetPassword(conn, auth, "xen", username, conn->uri->server); if (password == NULL) { xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED, -- 1.7.7.6
On 2012年03月21日 01:33, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
When SASL requests auth credentials, try to look them up in the config file first. If any are found, remove them from the list that the user is prompted for --- src/esx/esx_driver.c | 58 +++++++++--------- src/hyperv/hyperv_driver.c | 4 +- src/phyp/phyp_driver.c | 4 +- src/remote/remote_driver.c | 138 ++++++++++++++++++++++++++++++++++++-------- src/util/virauth.c | 69 +++++++++++++++++++++- src/util/virauth.h | 10 +++- src/xenapi/xenapi_driver.c | 4 +- 7 files changed, 224 insertions(+), 63 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 7689306..6962832 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -667,10 +667,8 @@ esxCapsInit(esxPrivate *priv)
static int -esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, - const char *hostname, int port, - const char *predefinedUsername, - esxVI_ProductVersion expectedProductVersion, +esxConnectToHost(virConnectPtr conn, + virConnectAuthPtr auth, char **vCenterIpAddress) { int result = -1; @@ -682,25 +680,29 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, esxVI_String *propertyNameList = NULL; esxVI_ObjectContent *hostSystem = NULL; esxVI_Boolean inMaintenanceMode = esxVI_Boolean_Undefined; + esxPrivate *priv = conn->privateData; + esxVI_ProductVersion expectedProductVersion = STRCASEEQ(conn->uri->scheme, "esx") + ? esxVI_ProductVersion_ESX + : esxVI_ProductVersion_GSX;
if (vCenterIpAddress == NULL || *vCenterIpAddress != NULL) { ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); return -1; }
- if (esxUtil_ResolveHostname(hostname, ipAddress, NI_MAXHOST)< 0) { + if (esxUtil_ResolveHostname(conn->uri->server, ipAddress, NI_MAXHOST)< 0) { return -1; }
- if (predefinedUsername != NULL) { - username = strdup(predefinedUsername); + if (conn->uri->user != NULL) { + username = strdup(conn->uri->user);
if (username == NULL) { virReportOOMError(); goto cleanup; } } else { - username = virAuthGetUsername(auth, "root", hostname); + username = virAuthGetUsername(conn, auth, "esx", "root", conn->uri->server);
if (username == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); @@ -708,7 +710,7 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, } }
- unescapedPassword = virAuthGetPassword(auth, username, hostname); + unescapedPassword = virAuthGetPassword(conn, auth, "esx", username, conn->uri->server);
if (unescapedPassword == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); @@ -722,7 +724,7 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, }
if (virAsprintf(&url, "%s://%s:%d/sdk", priv->parsedUri->transport, - hostname, port)< 0) { + conn->uri->server, conn->uri->port)< 0) { virReportOOMError(); goto cleanup; } @@ -743,13 +745,13 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth, priv->host->productVersion != esxVI_ProductVersion_ESX5x) { ESX_ERROR(VIR_ERR_INTERNAL_ERROR, _("%s is neither an ESX 3.5, 4.x nor 5.x host"), - hostname); + conn->uri->server); goto cleanup; } } else { /* GSX */ if (priv->host->productVersion != esxVI_ProductVersion_GSX20) { ESX_ERROR(VIR_ERR_INTERNAL_ERROR, - _("%s isn't a GSX 2.0 host"), hostname); + _("%s isn't a GSX 2.0 host"), conn->uri->server); goto cleanup; } } @@ -799,9 +801,9 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth,
static int -esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, - const char *hostname, int port, - const char *predefinedUsername, +esxConnectToVCenter(virConnectPtr conn, + virConnectAuthPtr auth, + const char *hostname, const char *hostSystemIpAddress) { int result = -1; @@ -810,6 +812,7 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, char *unescapedPassword = NULL; char *password = NULL; char *url = NULL; + esxPrivate *priv = conn->privateData;
if (hostSystemIpAddress == NULL&& (priv->parsedUri->path == NULL || STREQ(priv->parsedUri->path, "/"))) { @@ -822,15 +825,15 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, return -1; }
- if (predefinedUsername != NULL) { - username = strdup(predefinedUsername); + if (conn->uri->user != NULL) { + username = strdup(conn->uri->user);
if (username == NULL) { virReportOOMError(); goto cleanup; } } else { - username = virAuthGetUsername(auth, "administrator", hostname); + username = virAuthGetUsername(conn, auth, "esx", "administrator", hostname);
if (username == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); @@ -838,7 +841,7 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, } }
- unescapedPassword = virAuthGetPassword(auth, username, hostname); + unescapedPassword = virAuthGetPassword(conn, auth, "esx", username, hostname);
if (unescapedPassword == NULL) { ESX_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); @@ -852,7 +855,7 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, }
if (virAsprintf(&url, "%s://%s:%d/sdk", priv->parsedUri->transport, - hostname, port)< 0) { + hostname, conn->uri->port)< 0) { virReportOOMError(); goto cleanup; } @@ -1046,11 +1049,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, if (STRCASEEQ(conn->uri->scheme, "esx") || STRCASEEQ(conn->uri->scheme, "gsx")) { /* Connect to host */ - if (esxConnectToHost(priv, auth, conn->uri->server, conn->uri->port, - conn->uri->user, - STRCASEEQ(conn->uri->scheme, "esx") - ? esxVI_ProductVersion_ESX - : esxVI_ProductVersion_GSX, + if (esxConnectToHost(conn, auth, &potentialVCenterIpAddress)< 0) { goto cleanup; } @@ -1089,8 +1088,8 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, } }
- if (esxConnectToVCenter(priv, auth, vCenterIpAddress, - conn->uri->port, NULL, + if (esxConnectToVCenter(conn, auth, + vCenterIpAddress, priv->host->ipAddress)< 0) { goto cleanup; } @@ -1099,8 +1098,9 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, priv->primary = priv->host; } else { /* VPX */ /* Connect to vCenter */ - if (esxConnectToVCenter(priv, auth, conn->uri->server, conn->uri->port, - conn->uri->user, NULL)< 0) { + if (esxConnectToVCenter(conn, auth, + conn->uri->server, + NULL)< 0) { goto cleanup; }
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 0469e2e..05fce4f 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -147,7 +147,7 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) goto cleanup; } } else { - username = virAuthGetUsername(auth, "administrator", conn->uri->server); + username = virAuthGetUsername(conn, auth, "hyperv", "administrator", conn->uri->server);
if (username == NULL) { HYPERV_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); @@ -155,7 +155,7 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) } }
- password = virAuthGetPassword(auth, username, conn->uri->server); + password = virAuthGetPassword(conn, auth, "hyperv", username, conn->uri->server);
if (password == NULL) { HYPERV_ERROR(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 470706d..b883b56 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1006,7 +1006,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, goto err; }
- username = virAuthGetUsername(auth, NULL, conn->uri->server); + username = virAuthGetUsername(conn, auth, "ssh", NULL, conn->uri->server);
if (username == NULL) { PHYP_ERROR(VIR_ERR_AUTH_FAILED, "%s", @@ -1087,7 +1087,7 @@ keyboard_interactive: goto disconnect; }
- password = virAuthGetPassword(auth, username, conn->uri->server); + password = virAuthGetPassword(conn, auth, "ssh", username, conn->uri->server);
if (password == NULL) { PHYP_ERROR(VIR_ERR_AUTH_FAILED, "%s", diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1faaf9e..72b4b8f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -45,6 +45,8 @@ #include "intprops.h" #include "virtypedparam.h" #include "viruri.h" +#include "virauth.h" +#include "virauthconfig.h"
#define VIR_FROM_THIS VIR_FROM_REMOTE
@@ -461,6 +463,9 @@ doRemoteOpen (virConnectPtr conn, pkipath = strdup(var->value); if (!pkipath) goto out_of_memory; var->ignore = 1; + } else if (STRCASEEQ(var->name, "authfile")) { + /* Strip this param, used by virauth.c */ + var->ignore = 1; } else { VIR_DEBUG("passing through variable '%s' ('%s') to remote end", var->name, var->value); @@ -2870,27 +2875,35 @@ static int remoteAuthMakeCredentials(sasl_interact_t *interact, if (!cred) return -1;
- for (ninteract = 0 ; interact[ninteract].id != 0 ; ninteract++) - ; /* empty */ + for (ninteract = 0, *ncred = 0 ; interact[ninteract].id != 0 ; ninteract++) { + if (interact[ninteract].result) + continue; + (*ncred)++; + }
- if (VIR_ALLOC_N(*cred, ninteract)< 0) + if (VIR_ALLOC_N(*cred, *ncred)< 0) return -1;
- for (ninteract = 0 ; interact[ninteract].id != 0 ; ninteract++) { - (*cred)[ninteract].type = remoteAuthCredSASL2Vir(interact[ninteract].id); - if (!(*cred)[ninteract].type) { + for (ninteract = 0, *ncred = 0 ; interact[ninteract].id != 0 ; ninteract++) { + if (interact[ninteract].result) + continue; + + (*cred)[*ncred].type = remoteAuthCredSASL2Vir(interact[ninteract].id); + if (!(*cred)[*ncred].type) { + *ncred = 0; VIR_FREE(*cred); return -1; } - if (interact[ninteract].challenge) - (*cred)[ninteract].challenge = interact[ninteract].challenge; - (*cred)[ninteract].prompt = interact[ninteract].prompt; - if (interact[ninteract].defresult) - (*cred)[ninteract].defresult = interact[ninteract].defresult; - (*cred)[ninteract].result = NULL; + if (interact[*ncred].challenge) + (*cred)[*ncred].challenge = interact[ninteract].challenge; + (*cred)[*ncred].prompt = interact[ninteract].prompt; + if (interact[*ncred].defresult) + (*cred)[*ncred].defresult = interact[ninteract].defresult; + (*cred)[*ncred].result = NULL; + + (*ncred)++; }
- *ncred = ninteract; return 0; }
@@ -2905,22 +2918,91 @@ static int remoteAuthMakeCredentials(sasl_interact_t *interact, static void remoteAuthFillInteract(virConnectCredentialPtr cred, sasl_interact_t *interact) { - int ninteract; - for (ninteract = 0 ; interact[ninteract].id != 0 ; ninteract++) { - interact[ninteract].result = cred[ninteract].result; - interact[ninteract].len = cred[ninteract].resultlen; + int ninteract, ncred; + for (ninteract = 0, ncred = 0 ; interact[ninteract].id != 0 ; ninteract++) { + if (interact[ninteract].result) + continue; + interact[ninteract].result = cred[ncred].result; + interact[ninteract].len = cred[ncred].resultlen; + ncred++; } }
- struct remoteAuthInteractState { sasl_interact_t *interact; virConnectCredentialPtr cred; size_t ncred; + virAuthConfigPtr config; };
-static void remoteAuthInteractStateClear(struct remoteAuthInteractState *state) + +static int remoteAuthFillFromConfig(virConnectPtr conn, + struct remoteAuthInteractState *state) +{ + int ret = -1; + int ninteract; + const char *credname; + char *path = NULL; + + VIR_DEBUG("Trying to fill auth parameters from config file"); + + if (!state->config) { + if (virAuthGetConfigFilePath(conn,&path)< 0) + goto cleanup; + if (path == NULL) { + ret = 0; + goto cleanup; + } + + if (!(state->config = virAuthConfigNew(path))) + goto cleanup; + } + + for (ninteract = 0 ; state->interact[ninteract].id != 0 ; ninteract++) { + const char *value = NULL; + + switch (state->interact[ninteract].id) { + case SASL_CB_USER: + credname = "username"; + break; + case SASL_CB_AUTHNAME: + credname = "authname"; + break; + case SASL_CB_PASS: + credname = "password"; + break; + case SASL_CB_GETREALM: + credname = "realm"; + break; + default: + credname = NULL; + break; + } + + if (virAuthConfigLookup(state->config, + "libvirt", + conn->uri->server, + credname, +&value)< 0) + goto cleanup; + + if (value) { + state->interact[ninteract].result = value; + state->interact[ninteract].len = strlen(value); + } + } + + ret = 0; + +cleanup: + VIR_FREE(path); + return ret; +} + + +static void remoteAuthInteractStateClear(struct remoteAuthInteractState *state, + bool final) { size_t i; if (!state) @@ -2930,15 +3012,23 @@ static void remoteAuthInteractStateClear(struct remoteAuthInteractState *state) VIR_FREE(state->cred[i].result); VIR_FREE(state->cred); state->ncred = 0; + + if (final) + virAuthConfigFree(state->config); }
-static int remoteAuthInteract(struct remoteAuthInteractState *state, +static int remoteAuthInteract(virConnectPtr conn, + struct remoteAuthInteractState *state, virConnectAuthPtr auth) { int ret = -1;
- remoteAuthInteractStateClear(state); + VIR_DEBUG("Starting SASL interaction"); + remoteAuthInteractStateClear(state, false); + + if (remoteAuthFillFromConfig(conn, state)< 0) + goto cleanup;
if (remoteAuthMakeCredentials(state->interact,&state->cred,&state->ncred)< 0) { remoteError(VIR_ERR_AUTH_FAILED, "%s", @@ -3074,7 +3164,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv,
/* Need to gather some credentials from the client */ if (err == VIR_NET_SASL_INTERACT) { - if (remoteAuthInteract(&state, auth)< 0) { + if (remoteAuthInteract(conn,&state, auth)< 0) { VIR_FREE(iret.mechlist); goto cleanup; } @@ -3126,7 +3216,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv,
/* Need to gather some credentials from the client */ if (err == VIR_NET_SASL_INTERACT) { - if (remoteAuthInteract(&state, auth)< 0) { + if (remoteAuthInteract(conn,&state, auth)< 0) { VIR_FREE(iret.mechlist); goto cleanup; } @@ -3192,7 +3282,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, cleanup: VIR_FREE(serverin);
- remoteAuthInteractStateClear(&state); + remoteAuthInteractStateClear(&state, true); VIR_FREE(saslcb); virNetSASLSessionFree(sasl); virNetSASLContextFree(saslCtxt); diff --git a/src/util/virauth.c b/src/util/virauth.c index 150b8e7..940686f 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -30,6 +30,7 @@ #include "datatypes.h" #include "virterror_internal.h" #include "configmake.h" +#include "virauthconfig.h"
#define VIR_FROM_THIS VIR_FROM_AUTH
@@ -100,13 +101,68 @@ no_memory: }
+static int +virAuthGetCredential(virConnectPtr conn, + const char *servicename, + const char *credname, + char **value) +{ + int ret = -1; + char *path = NULL; + virAuthConfigPtr config = NULL; + const char *tmp; + + *value = NULL; + + if (virAuthGetConfigFilePath(conn,&path)< 0) + goto cleanup; + + if (path == NULL) { + ret = 0; + goto cleanup; + } + + if (!(config = virAuthConfigNew(path))) + goto cleanup; + + if (virAuthConfigLookup(config, + servicename, + conn->uri->server, + credname, +&tmp)< 0) + goto cleanup; + + if (tmp&& + !(*value = strdup(tmp))) { + virReportOOMError(); + goto cleanup; + } + + ret = 0; + +cleanup: + virAuthConfigFree(config); + VIR_FREE(path); + return ret; +} + + char * -virAuthGetUsername(virConnectAuthPtr auth, const char *defaultUsername, +virAuthGetUsername(virConnectPtr conn, + virConnectAuthPtr auth, + const char *servicename, + const char *defaultUsername, const char *hostname) { unsigned int ncred; virConnectCredential cred; char *prompt; + char *ret = NULL; + + if (virAuthGetCredential(conn, servicename, "username",&ret)< 0) + return NULL; + if (ret != NULL) + return ret;
memset(&cred, 0, sizeof (virConnectCredential));
@@ -148,12 +204,21 @@ virAuthGetUsername(virConnectAuthPtr auth, const char *defaultUsername,
char * -virAuthGetPassword(virConnectAuthPtr auth, const char *username, +virAuthGetPassword(virConnectPtr conn, + virConnectAuthPtr auth, + const char *servicename, + const char *username, const char *hostname) { unsigned int ncred; virConnectCredential cred; char *prompt; + char *ret = NULL; + + if (virAuthGetCredential(conn, servicename, "password",&ret)< 0) + return NULL; + if (ret != NULL) + return ret;
memset(&cred, 0, sizeof (virConnectCredential));
diff --git a/src/util/virauth.h b/src/util/virauth.h index 7f43bee..1b315c7 100644 --- a/src/util/virauth.h +++ b/src/util/virauth.h @@ -27,9 +27,15 @@ int virAuthGetConfigFilePath(virConnectPtr conn, char **path);
-char *virAuthGetUsername(virConnectAuthPtr auth, const char *defaultUsername, +char *virAuthGetUsername(virConnectPtr conn, + virConnectAuthPtr auth, + const char *servicename, + const char *defaultUsername, const char *hostname); -char *virAuthGetPassword(virConnectAuthPtr auth, const char *username, +char *virAuthGetPassword(virConnectPtr conn, + virConnectAuthPtr auth, + const char *servicename, + const char *username, const char *hostname);
#endif /* __VIR_AUTH_H__ */ diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 3f88c91..2967f7c 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -138,7 +138,7 @@ xenapiOpen (virConnectPtr conn, virConnectAuthPtr auth, goto error; } } else { - username = virAuthGetUsername(auth, NULL, conn->uri->server); + username = virAuthGetUsername(conn, auth, "xen", NULL, conn->uri->server);
if (username == NULL) { xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED, @@ -147,7 +147,7 @@ xenapiOpen (virConnectPtr conn, virConnectAuthPtr auth, } }
- password = virAuthGetPassword(auth, username, conn->uri->server); + password = virAuthGetPassword(conn, auth, "xen", username, conn->uri->server);
if (password == NULL) { xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED,
Looks fine to me, ACK.
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Osier Yang