10:11 a.m.
While understandably not really a 1.2.6 candidate, I figured I'd post this
now in hopes that it gets the ball rolling. The changes will help with
a related bz to support libiscsi for SCSI passthrough devices where a
<hostdev> entry for an iscsi adapter would be able to have <auth> data.
The bulk of changes are code motion/merge. The details for each are:
The first patch will create new API's which will then be used by the
parse domain disk and the parse storage pool code if the "<auth" element
is found. The logic merges the parsing found in the domain disk and
storage pool code with one slight caveat - the domain disk code expects
to find "<secret type='xxxx'", where xxxx is 'iscsi' or
'ceph'. Trying
use the Type{To/From}String API's in virstoragefile.c resulted in a
link time failure (even though secret_conf.h was included). Never quite
figured it out - I will take suggestions though. Although since the
domain disk code is all that really cares about it and the domain_conf
code can use the Type{To/From}String API's - I just left it as is. The
new copy API is used when the pool code needs to copy it's auth data into
each domain disk's auth entry.
The second patch will fix a not-run test that tests the auth. It seems
it was not added to the active list of tests because the output generated
did not include the "usage" information (eg "
usage='mycluster_myname") that
is part of the <secret> element. Additionally because it did not include
it the output was "<secret type='iscsi' </auth>". So rather
than ignore
the test - add to the filters to avoid looking for "<secret" - through
to the closing ">". The -auth.xml and -auth.args files needed adjustments
to follow the non "-auth" version of the files that have changed over time,
but not also changed int the "-auth" files.
The third patch will cause the domain disk algorithms to utilize the
new generic parse and format APIs as well as using the 'authdef' instead
of the inline structure fields. This patch also restores the checking for
</auth> in the recently restored auth test. Still avoiding the whole
<secret... /> line as I saw no way to filter just the usage that's in
the stored XML file.
The fourth patch is a mostly innocuous html change - it could go early,
but it just felt better in this place.
The fifth patch will cause the storage pool algorithms to utilize the
new generic parse/format API's as well as new authdef structure for
accessing the data. The removal of the duplicated cephx/chap structures
seems (in my mind at least) to simplify things. They were essentially
copies of each other with no seemingly real value in keeping separate
other than the visual in the code of ".chap." or ".cephx.".
John Ferlan (5):
virstorage: Introduce virStorageAuthDef
qemuargv2xmltest: Resurrect RBD and iSCSI auth
Utilize virDomainDiskAuth for domain disk
formatdomain: Fix issues found describing auth
Utilize virDomainDiskAuth for storage pools
docs/formatdomain.html.in | 24 +--
src/conf/domain_conf.c | 106 ++--------
src/conf/storage_conf.c | 152 ++------------
src/conf/storage_conf.h | 38 +---
src/libvirt_private.syms | 5 +-
src/qemu/qemu_command.c | 72 ++++---
src/qemu/qemu_conf.c | 46 +----
src/storage/storage_backend_iscsi.c | 41 ++--
src/storage/storage_backend_rbd.c | 65 +++---
src/util/virstoragefile.c | 219 +++++++++++++++++++--
src/util/virstoragefile.h | 37 +++-
tests/qemuargv2xmltest.c | 3 +
...qemuxml2argv-disk-drive-network-iscsi-auth.args | 4 +-
.../qemuxml2argv-disk-drive-network-iscsi-auth.xml | 12 +-
.../qemuxml2argv-disk-drive-network-rbd-auth.xml | 4 +-
15 files changed, 415 insertions(+), 413 deletions(-)
--
1.9.3
10:11 a.m.
New subject: [libvirt] [PATCH 1/5] virstorage: Introduce virStorageAuthDef
Introduce virStorageAuthDef and friends. Future patches will merge/utilize
their view of storage source/pool auth/secret definitions.
New API's include:
virStorageAuthDefParse: Parse the "<auth" XML data for either the
domain disk or storage pool returning a
virStorageAuthDefPtr
virStorageAuthDefCopy: Copy a virStorageAuthDefPtr - to be used by
the qemuTranslateDiskSourcePoolAuth when it
copies storage pool auth data into domain
disk auth data
virStorageAuthDefFormat: Common output of the "<auth" in the domain
disk or storage pool XML
virStorageAuthDefFree: Free memory associated with virStorageAuthDef
Subsequent patches will utilize the new functions for the domain disk and
storage pools.
Future work in the hostdev pass through can then make use of common data
structures and code.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libvirt_private.syms | 4 +
src/util/virstoragefile.c | 205 ++++++++++++++++++++++++++++++++++++++++++++++
src/util/virstoragefile.h | 27 ++++++
3 files changed, 236 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1e1dd84..17dcd67 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1879,6 +1879,10 @@ virStorageGenerateQcowPassphrase;
# util/virstoragefile.h
+virStorageAuthDefCopy;
+virStorageAuthDefFormat;
+virStorageAuthDefFree;
+virStorageAuthDefParse;
virStorageFileCanonicalizePath;
virStorageFileChainGetBroken;
virStorageFileChainLookup;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 0c50de1..056e59e 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -29,6 +29,8 @@
#include <fcntl.h>
#include <stdlib.h>
#include "viralloc.h"
+#include "virxml.h"
+#include "viruuid.h"
#include "virerror.h"
#include "virlog.h"
#include "virfile.h"
@@ -97,6 +99,10 @@ VIR_ENUM_IMPL(virStorageSourcePoolMode,
"host",
"direct")
+VIR_ENUM_IMPL(virStorageAuth,
+ VIR_STORAGE_AUTH_TYPE_LAST,
+ "none", "chap", "ceph")
+
enum lv_endian {
LV_LITTLE_ENDIAN = 1, /* 1234 */
LV_BIG_ENDIAN /* 4321 */
@@ -1500,6 +1506,205 @@ virStorageNetHostDefCopy(size_t nhosts,
}
+void
+virStorageAuthDefFree(virStorageAuthDefPtr authdef)
+{
+ if (!authdef)
+ return;
+
+ VIR_FREE(authdef->username);
+ VIR_FREE(authdef->secrettype);
+ if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_USAGE)
+ VIR_FREE(authdef->secret.usage);
+ VIR_FREE(authdef);
+}
+
+
+virStorageAuthDefPtr
+virStorageAuthDefCopy(const virStorageAuthDef *src)
+{
+ virStorageAuthDefPtr ret;
+
+ if (VIR_ALLOC(ret) < 0)
+ return NULL;
+
+ if (VIR_STRDUP(ret->username, src->username) < 0)
+ goto error;
+ /* Not present for storage pool, but used for disk source */
+ if (src->secrettype)
+ if (VIR_STRDUP(ret->secrettype, src->secrettype) < 0)
+ goto error;
+ ret->authType = src->authType;
+ ret->secretType = src->secretType;
+ if (ret->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
+ memcpy(ret->secret.uuid, src->secret.uuid, sizeof(ret->secret.uuid));
+ } else if (ret->secretType == VIR_STORAGE_SECRET_TYPE_USAGE) {
+ if (VIR_STRDUP(ret->secret.usage, src->secret.usage) < 0)
+ goto error;
+ }
+ return ret;
+
+ error:
+ virStorageAuthDefFree(ret);
+ return NULL;
+}
+
+
+static int
+virStorageAuthDefParseSecret(xmlXPathContextPtr ctxt,
+ virStorageAuthDefPtr authdef)
+{
+ char *uuid;
+ char *usage;
+ int ret = -1;
+
+ /* Used by the domain disk xml parsing in order to ensure the
+ * <secret type='%s' value matches the expected secret type for
+ * the style of disk (iscsi is chap, nbd is ceph). For some reason
+ * the virSecretUsageType{From|To}String() cannot be linked here
+ * and because only the domain parsing code cares - just keep
+ * it as a string.
+ */
+ authdef->secrettype = virXPathString("string(./secret/@type)", ctxt);
+
+ uuid = virXPathString("string(./secret/@uuid)", ctxt);
+ usage = virXPathString("string(./secret/@usage)", ctxt);
+ if (uuid == NULL && usage == NULL) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing auth secret uuid or usage attribute"));
+ goto cleanup;
+ }
+
+ if (uuid && usage) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("either auth secret uuid or usage expected"));
+ goto cleanup;
+ }
+
+ if (uuid) {
+ if (virUUIDParse(uuid, authdef->secret.uuid) < 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("invalid auth secret uuid"));
+ goto cleanup;
+ }
+ authdef->secretType = VIR_STORAGE_SECRET_TYPE_UUID;
+ } else {
+ authdef->secret.usage = usage;
+ usage = NULL;
+ authdef->secretType = VIR_STORAGE_SECRET_TYPE_USAGE;
+ }
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(uuid);
+ VIR_FREE(usage);
+ return ret;
+}
+
+
+static virStorageAuthDefPtr
+virStorageAuthDefParseXML(xmlXPathContextPtr ctxt)
+{
+ virStorageAuthDefPtr authdef = NULL;
+ char *username = NULL;
+ char *authtype = NULL;
+
+ if (VIR_ALLOC(authdef) < 0)
+ return NULL;
+
+ if (!(username = virXPathString("string(./@username)", ctxt))) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing username for auth"));
+ goto error;
+ }
+ authdef->username = username;
+ username = NULL;
+
+ authdef->authType = VIR_STORAGE_AUTH_TYPE_NONE;
+ authtype = virXPathString("string(./@type)", ctxt);
+ if (authtype) {
+ /* Used by the storage pool instead of the secret type field
+ * to define whether chap or ceph being used
+ */
+ if ((authdef->authType = virStorageAuthTypeFromString(authtype)) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown auth type '%s'"), authtype);
+ goto error;
+ }
+ VIR_FREE(authtype);
+ }
+
+ authdef->secretType = VIR_STORAGE_SECRET_TYPE_NONE;
+ if (virStorageAuthDefParseSecret(ctxt, authdef) < 0)
+ goto error;
+
+ return authdef;
+
+ error:
+ VIR_FREE(authtype);
+ VIR_FREE(username);
+ virStorageAuthDefFree(authdef);
+ return NULL;
+}
+
+
+virStorageAuthDefPtr
+virStorageAuthDefParse(xmlDocPtr xml, xmlNodePtr root)
+{
+ xmlXPathContextPtr ctxt = NULL;
+ virStorageAuthDefPtr authdef = NULL;
+
+ ctxt = xmlXPathNewContext(xml);
+ if (ctxt == NULL) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ ctxt->node = root;
+ authdef = virStorageAuthDefParseXML(ctxt);
+
+ cleanup:
+ xmlXPathFreeContext(ctxt);
+ return authdef;
+}
+
+
+int
+virStorageAuthDefFormat(virBufferPtr buf,
+ virStorageAuthDefPtr authdef)
+{
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+ if (authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE) {
+ virBufferEscapeString(buf, "<auth username='%s'>\n",
authdef->username);
+ } else {
+ virBufferAsprintf(buf, "<auth type='%s' ",
+ virStorageAuthTypeToString(authdef->authType));
+ virBufferEscapeString(buf, "username='%s'>\n",
authdef->username);
+ }
+
+ virBufferAdjustIndent(buf, 2);
+ if (authdef->secrettype)
+ virBufferAsprintf(buf, "<secret type='%s'",
authdef->secrettype);
+ else
+ virBufferAddLit(buf, "<secret");
+
+ if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
+ virUUIDFormat(authdef->secret.uuid, uuidstr);
+ virBufferAsprintf(buf, " uuid='%s'/>\n", uuidstr);
+ } else if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_USAGE) {
+ virBufferEscapeString(buf, " usage='%s'/>\n",
+ authdef->secret.usage);
+ } else {
+ virBufferAddLit(buf, "/>\n");
+ }
+ virBufferAdjustIndent(buf, -2);
+ virBufferAddLit(buf, "</auth>\n");
+
+ return 0;
+}
+
+
virSecurityDeviceLabelDefPtr
virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
const char *model)
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 48c7e02..383ba96 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -193,6 +193,15 @@ typedef virStorageSourcePoolDef *virStorageSourcePoolDefPtr;
typedef enum {
+ VIR_STORAGE_AUTH_TYPE_NONE,
+ VIR_STORAGE_AUTH_TYPE_CHAP,
+ VIR_STORAGE_AUTH_TYPE_CEPHX,
+
+ VIR_STORAGE_AUTH_TYPE_LAST,
+} virStorageAuthType;
+VIR_ENUM_DECL(virStorageAuth)
+
+typedef enum {
VIR_STORAGE_SECRET_TYPE_NONE,
VIR_STORAGE_SECRET_TYPE_UUID,
VIR_STORAGE_SECRET_TYPE_USAGE,
@@ -200,6 +209,19 @@ typedef enum {
VIR_STORAGE_SECRET_TYPE_LAST
} virStorageSecretType;
+typedef struct _virStorageAuthDef virStorageAuthDef;
+typedef virStorageAuthDef *virStorageAuthDefPtr;
+struct _virStorageAuthDef {
+ char *username;
+ char *secrettype; /* <secret type='%s' for disk source */
+ int authType; /* virStorageAuthType */
+ int secretType; /* virStorageSecretType */
+ union {
+ unsigned char uuid[VIR_UUID_BUFLEN];
+ char *usage;
+ } secret;
+};
+
typedef struct _virStorageDriverData virStorageDriverData;
typedef virStorageDriverData *virStorageDriverDataPtr;
@@ -307,6 +329,11 @@ int virStorageFileGetLVMKey(const char *path,
int virStorageFileGetSCSIKey(const char *path,
char **key);
+void virStorageAuthDefFree(virStorageAuthDefPtr def);
+virStorageAuthDefPtr virStorageAuthDefCopy(const virStorageAuthDef *src);
+virStorageAuthDefPtr virStorageAuthDefParse(xmlDocPtr xml, xmlNodePtr root);
+int virStorageAuthDefFormat(virBufferPtr buf, virStorageAuthDefPtr authdef);
+
virSecurityDeviceLabelDefPtr
virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
const char *model);
--
1.9.3
7:13 a.m.
New subject: [libvirt] [PATCH 1/5] virstorage: Introduce virStorageAuthDef
On 27.06.2014 17:11, John Ferlan wrote:
Introduce virStorageAuthDef and friends. Future patches will
merge/utilize
their view of storage source/pool auth/secret definitions.
New API's include:
virStorageAuthDefParse: Parse the "<auth" XML data for either the
domain disk or storage pool returning a
If I were to be narrow-minded OCD libvirt developer, I'd point out:
s,<auth,<auth/>, but since I'm not, I'll leave it :)
virStorageAuthDefPtr
virStorageAuthDefCopy: Copy a virStorageAuthDefPtr - to be used by
the qemuTranslateDiskSourcePoolAuth when it
copies storage pool auth data into domain
disk auth data
virStorageAuthDefFormat: Common output of the "<auth" in the domain
disk or storage pool XML
virStorageAuthDefFree: Free memory associated with virStorageAuthDef
Subsequent patches will utilize the new functions for the domain disk and
storage pools.
Future work in the hostdev pass through can then make use of common data
structures and code.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libvirt_private.syms | 4 +
src/util/virstoragefile.c | 205 ++++++++++++++++++++++++++++++++++++++++++++++
src/util/virstoragefile.h | 27 ++++++
3 files changed, 236 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1e1dd84..17dcd67 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1879,6 +1879,10 @@ virStorageGenerateQcowPassphrase;
# util/virstoragefile.h
+virStorageAuthDefCopy;
+virStorageAuthDefFormat;
+virStorageAuthDefFree;
+virStorageAuthDefParse;
I have some doubts it the Format and Parse should be in src/util/. But
since we already have something similar in virstorageencryption.c I
guess we can live with this location too.
virStorageFileCanonicalizePath;
virStorageFileChainGetBroken;
virStorageFileChainLookup;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 0c50de1..056e59e 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -29,6 +29,8 @@
#include <fcntl.h>
#include <stdlib.h>
#include "viralloc.h"
+#include "virxml.h"
+#include "viruuid.h"
#include "virerror.h"
#include "virlog.h"
#include "virfile.h"
@@ -97,6 +99,10 @@ VIR_ENUM_IMPL(virStorageSourcePoolMode,
"host",
"direct")
+VIR_ENUM_IMPL(virStorageAuth,
+ VIR_STORAGE_AUTH_TYPE_LAST,
+ "none", "chap", "ceph")
+
enum lv_endian {
LV_LITTLE_ENDIAN = 1, /* 1234 */
LV_BIG_ENDIAN /* 4321 */
@@ -1500,6 +1506,205 @@ virStorageNetHostDefCopy(size_t nhosts,
}
+void
+virStorageAuthDefFree(virStorageAuthDefPtr authdef)
+{
+ if (!authdef)
+ return;
+
+ VIR_FREE(authdef->username);
+ VIR_FREE(authdef->secrettype);
+ if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_USAGE)
+ VIR_FREE(authdef->secret.usage);
+ VIR_FREE(authdef);
+}
+
+
+virStorageAuthDefPtr
+virStorageAuthDefCopy(const virStorageAuthDef *src)
+{
+ virStorageAuthDefPtr ret;
+
+ if (VIR_ALLOC(ret) < 0)
+ return NULL;
+
+ if (VIR_STRDUP(ret->username, src->username) < 0)
+ goto error;
+ /* Not present for storage pool, but used for disk source */
+ if (src->secrettype)
+ if (VIR_STRDUP(ret->secrettype, src->secrettype) < 0)
+ goto error;
There's no need to check for src->secrettype as VIR_STRDUP accepts NULL.
+ ret->authType = src->authType;
+ ret->secretType = src->secretType;
+ if (ret->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
+ memcpy(ret->secret.uuid, src->secret.uuid, sizeof(ret->secret.uuid));
+ } else if (ret->secretType == VIR_STORAGE_SECRET_TYPE_USAGE) {
+ if (VIR_STRDUP(ret->secret.usage, src->secret.usage) < 0)
+ goto error;
+ }
+ return ret;
+
+ error:
+ virStorageAuthDefFree(ret);
+ return NULL;
+}
+
I like the explicit copying, but just a suggestion to consider:
{
VIR_ALLOC(ret);
memcpy(ret, src, sizeof(*ret));
VIR_STRDUP(ret->scerettype, src->secrettype);
if (ret->secretType == VIR_STORAGE_SECRET_TYPE_USAGE &&
VIR_STRDUP() < 0)
goto error;
}
I'm worried that if a new item is added into thepy virStorageAuthDef
struct, sooner or later we forget to update the Copy function. With
memcpy() we are safe for shallow copies at least.
+
+static int
+virStorageAuthDefParseSecret(xmlXPathContextPtr ctxt,
+ virStorageAuthDefPtr authdef)
+{
+ char *uuid;
+ char *usage;
+ int ret = -1;
+
+ /* Used by the domain disk xml parsing in order to ensure the
+ * <secret type='%s' value matches the expected secret type for
+ * the style of disk (iscsi is chap, nbd is ceph). For some reason
+ * the virSecretUsageType{From|To}String() cannot be linked here
+ * and because only the domain parsing code cares - just keep
+ * it as a string.
+ */
+ authdef->secrettype = virXPathString("string(./secret/@type)", ctxt);
+
+ uuid = virXPathString("string(./secret/@uuid)", ctxt);
+ usage = virXPathString("string(./secret/@usage)", ctxt);
+ if (uuid == NULL && usage == NULL) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing auth secret uuid or usage attribute"));
+ goto cleanup;
+ }
+
+ if (uuid && usage) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("either auth secret uuid or usage expected"));
+ goto cleanup;
+ }
+
+ if (uuid) {
+ if (virUUIDParse(uuid, authdef->secret.uuid) < 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("invalid auth secret uuid"));
+ goto cleanup;
+ }
+ authdef->secretType = VIR_STORAGE_SECRET_TYPE_UUID;
+ } else {
+ authdef->secret.usage = usage;
+ usage = NULL;
+ authdef->secretType = VIR_STORAGE_SECRET_TYPE_USAGE;
+ }
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(uuid);
+ VIR_FREE(usage);
+ return ret;
+}
+
+
+static virStorageAuthDefPtr
+virStorageAuthDefParseXML(xmlXPathContextPtr ctxt)
+{
+ virStorageAuthDefPtr authdef = NULL;
+ char *username = NULL;
+ char *authtype = NULL;
+
+ if (VIR_ALLOC(authdef) < 0)
+ return NULL;
+
+ if (!(username = virXPathString("string(./@username)", ctxt))) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing username for auth"));
+ goto error;
+ }
+ authdef->username = username;
+ username = NULL;
+
+ authdef->authType = VIR_STORAGE_AUTH_TYPE_NONE;
+ authtype = virXPathString("string(./@type)", ctxt);
+ if (authtype) {
+ /* Used by the storage pool instead of the secret type field
+ * to define whether chap or ceph being used
+ */
+ if ((authdef->authType = virStorageAuthTypeFromString(authtype)) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown auth type '%s'"), authtype);
+ goto error;
+ }
+ VIR_FREE(authtype);
No need for this, as you'll free it just a few lines below.
+ }
+
+ authdef->secretType = VIR_STORAGE_SECRET_TYPE_NONE;
+ if (virStorageAuthDefParseSecret(ctxt, authdef) < 0)
+ goto error;
+
+ return authdef;
+
+ error:
+ VIR_FREE(authtype);
+ VIR_FREE(username);
+ virStorageAuthDefFree(authdef);
+ return NULL;
+}
+
+
+virStorageAuthDefPtr
+virStorageAuthDefParse(xmlDocPtr xml, xmlNodePtr root)
+{
+ xmlXPathContextPtr ctxt = NULL;
+ virStorageAuthDefPtr authdef = NULL;
+
+ ctxt = xmlXPathNewContext(xml);
+ if (ctxt == NULL) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ ctxt->node = root;
+ authdef = virStorageAuthDefParseXML(ctxt);
+
+ cleanup:
+ xmlXPathFreeContext(ctxt);
+ return authdef;
+}
+
+
+int
+virStorageAuthDefFormat(virBufferPtr buf,
+ virStorageAuthDefPtr authdef)
+{
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+ if (authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE) {
+ virBufferEscapeString(buf, "<auth username='%s'>\n",
authdef->username);
+ } else {
+ virBufferAsprintf(buf, "<auth type='%s' ",
+ virStorageAuthTypeToString(authdef->authType));
+ virBufferEscapeString(buf, "username='%s'>\n",
authdef->username);
+ }
+
+ virBufferAdjustIndent(buf, 2);
+ if (authdef->secrettype)
+ virBufferAsprintf(buf, "<secret type='%s'",
authdef->secrettype);
+ else
+ virBufferAddLit(buf, "<secret");
+
+ if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
+ virUUIDFormat(authdef->secret.uuid, uuidstr);
+ virBufferAsprintf(buf, " uuid='%s'/>\n", uuidstr);
+ } else if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_USAGE) {
+ virBufferEscapeString(buf, " usage='%s'/>\n",
+ authdef->secret.usage);
+ } else {
+ virBufferAddLit(buf, "/>\n");
+ }
+ virBufferAdjustIndent(buf, -2);
+ virBufferAddLit(buf, "</auth>\n");
+
+ return 0;
+}
+
+
virSecurityDeviceLabelDefPtr
virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
const char *model)
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 48c7e02..383ba96 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -193,6 +193,15 @@ typedef virStorageSourcePoolDef *virStorageSourcePoolDefPtr;
typedef enum {
+ VIR_STORAGE_AUTH_TYPE_NONE,
+ VIR_STORAGE_AUTH_TYPE_CHAP,
+ VIR_STORAGE_AUTH_TYPE_CEPHX,
+
+ VIR_STORAGE_AUTH_TYPE_LAST,
+} virStorageAuthType;
+VIR_ENUM_DECL(virStorageAuth)
+
+typedef enum {
VIR_STORAGE_SECRET_TYPE_NONE,
VIR_STORAGE_SECRET_TYPE_UUID,
VIR_STORAGE_SECRET_TYPE_USAGE,
@@ -200,6 +209,19 @@ typedef enum {
VIR_STORAGE_SECRET_TYPE_LAST
} virStorageSecretType;
+typedef struct _virStorageAuthDef virStorageAuthDef;
+typedef virStorageAuthDef *virStorageAuthDefPtr;
+struct _virStorageAuthDef {
+ char *username;
+ char *secrettype; /* <secret type='%s' for disk source */
+ int authType; /* virStorageAuthType */
+ int secretType; /* virStorageSecretType */
+ union {
+ unsigned char uuid[VIR_UUID_BUFLEN];
+ char *usage;
+ } secret;
+};
Is it necessary to expose the struct or can we hide it in the
virstoragefile.c?
+
typedef struct _virStorageDriverData virStorageDriverData;
typedef virStorageDriverData *virStorageDriverDataPtr;
@@ -307,6 +329,11 @@ int virStorageFileGetLVMKey(const char *path,
int virStorageFileGetSCSIKey(const char *path,
char **key);
+void virStorageAuthDefFree(virStorageAuthDefPtr def);
+virStorageAuthDefPtr virStorageAuthDefCopy(const virStorageAuthDef *src);
+virStorageAuthDefPtr virStorageAuthDefParse(xmlDocPtr xml, xmlNodePtr root);
+int virStorageAuthDefFormat(virBufferPtr buf, virStorageAuthDefPtr authdef);
+
virSecurityDeviceLabelDefPtr
virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
const char *model);
Michal
11:08 a.m.
New subject: [libvirt] [PATCH 1/5] virstorage: Introduce virStorageAuthDef
On 07/02/2014 08:13 AM, Michal Privoznik wrote:
On 27.06.2014 17:11, John Ferlan wrote:
> Introduce virStorageAuthDef and friends. Future patches will merge/utilize
> their view of storage source/pool auth/secret definitions.
>
> New API's include:
> virStorageAuthDefParse: Parse the "<auth" XML data for either the
> domain disk or storage pool returning a
If I were to be narrow-minded OCD libvirt developer, I'd point out:
s,<auth,<auth/>, but since I'm not, I'll leave it :)
> virStorageAuthDefPtr
> virStorageAuthDefCopy: Copy a virStorageAuthDefPtr - to be used by
> the qemuTranslateDiskSourcePoolAuth when it
> copies storage pool auth data into domain
> disk auth data
> virStorageAuthDefFormat: Common output of the "<auth" in the
domain
> disk or storage pool XML
> virStorageAuthDefFree: Free memory associated with virStorageAuthDef
>
> Subsequent patches will utilize the new functions for the domain disk and
> storage pools.
>
> Future work in the hostdev pass through can then make use of common data
> structures and code.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/libvirt_private.syms | 4 +
> src/util/virstoragefile.c | 205 ++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virstoragefile.h | 27 ++++++
> 3 files changed, 236 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 1e1dd84..17dcd67 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1879,6 +1879,10 @@ virStorageGenerateQcowPassphrase;
>
>
> # util/virstoragefile.h
> +virStorageAuthDefCopy;
> +virStorageAuthDefFormat;
> +virStorageAuthDefFree;
> +virStorageAuthDefParse;
I have some doubts it the Format and Parse should be in src/util/. But
since we already have something similar in virstorageencryption.c I
guess we can live with this location too.
Yes, that was my model - the main goal was to avoid duplicated code. I
think that's more ripe for forgetting something or doing something in
one place, but not the other and some day having someone wonder why
there's a difference.
> virStorageFileCanonicalizePath;
> virStorageFileChainGetBroken;
> virStorageFileChainLookup;
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 0c50de1..056e59e 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -29,6 +29,8 @@
> #include <fcntl.h>
> #include <stdlib.h>
> #include "viralloc.h"
> +#include "virxml.h"
> +#include "viruuid.h"
> #include "virerror.h"
> #include "virlog.h"
> #include "virfile.h"
> @@ -97,6 +99,10 @@ VIR_ENUM_IMPL(virStorageSourcePoolMode,
> "host",
> "direct")
>
> +VIR_ENUM_IMPL(virStorageAuth,
> + VIR_STORAGE_AUTH_TYPE_LAST,
> + "none", "chap", "ceph")
> +
> enum lv_endian {
> LV_LITTLE_ENDIAN = 1, /* 1234 */
> LV_BIG_ENDIAN /* 4321 */
> @@ -1500,6 +1506,205 @@ virStorageNetHostDefCopy(size_t nhosts,
> }
>
>
> +void
> +virStorageAuthDefFree(virStorageAuthDefPtr authdef)
> +{
> + if (!authdef)
> + return;
> +
> + VIR_FREE(authdef->username);
> + VIR_FREE(authdef->secrettype);
> + if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_USAGE)
> + VIR_FREE(authdef->secret.usage);
> + VIR_FREE(authdef);
> +}
> +
> +
> +virStorageAuthDefPtr
> +virStorageAuthDefCopy(const virStorageAuthDef *src)
> +{
> + virStorageAuthDefPtr ret;
> +
> + if (VIR_ALLOC(ret) < 0)
> + return NULL;
> +
> + if (VIR_STRDUP(ret->username, src->username) < 0)
> + goto error;
> + /* Not present for storage pool, but used for disk source */
> + if (src->secrettype)
> + if (VIR_STRDUP(ret->secrettype, src->secrettype) < 0)
> + goto error;
There's no need to check for src->secrettype as VIR_STRDUP accepts NULL.
> + ret->authType = src->authType;
> + ret->secretType = src->secretType;
> + if (ret->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
> + memcpy(ret->secret.uuid, src->secret.uuid,
sizeof(ret->secret.uuid));
> + } else if (ret->secretType == VIR_STORAGE_SECRET_TYPE_USAGE) {
> + if (VIR_STRDUP(ret->secret.usage, src->secret.usage) < 0)
> + goto error;
> + }
> + return ret;
> +
> + error:
> + virStorageAuthDefFree(ret);
> + return NULL;
> +}
> +
I like the explicit copying, but just a suggestion to consider:
{
VIR_ALLOC(ret);
memcpy(ret, src, sizeof(*ret));
^^^
For me this is more ripe for someone forgetting to strdup the authname
and if necessary the ret->secret.usage.
VIR_STRDUP(ret->scerettype, src->secrettype);
if (ret->secretType == VIR_STORAGE_SECRET_TYPE_USAGE &&
VIR_STRDUP() < 0)
goto error;
^^^
See... no VIR_STRDUP(ret->username, src->username); :-)
Also no error checking for sercrettype strdup failure with us leaving
the API with nothing in secrettype which may have been important.
}
I'm worried that if a new item is added into thepy virStorageAuthDef
struct, sooner or later we forget to update the Copy function. With
memcpy() we are safe for shallow copies at least.
But when using memcpy() if we fail/error on a strdup(), then we have
pointers from one structure in the other structure - the failure path
gets messy. I think if you're not copying pointers to allocated things,
then sure memcpy() is fine, but once you have pointers to allocated
things - it's safer to be explicit on what you're copying.
> +
> +static int
> +virStorageAuthDefParseSecret(xmlXPathContextPtr ctxt,
> + virStorageAuthDefPtr authdef)
> +{
> + char *uuid;
> + char *usage;
> + int ret = -1;
> +
> + /* Used by the domain disk xml parsing in order to ensure the
> + * <secret type='%s' value matches the expected secret type for
> + * the style of disk (iscsi is chap, nbd is ceph). For some reason
> + * the virSecretUsageType{From|To}String() cannot be linked here
> + * and because only the domain parsing code cares - just keep
> + * it as a string.
> + */
> + authdef->secrettype = virXPathString("string(./secret/@type)",
ctxt);
> +
> + uuid = virXPathString("string(./secret/@uuid)", ctxt);
> + usage = virXPathString("string(./secret/@usage)", ctxt);
> + if (uuid == NULL && usage == NULL) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("missing auth secret uuid or usage attribute"));
> + goto cleanup;
> + }
> +
> + if (uuid && usage) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("either auth secret uuid or usage expected"));
> + goto cleanup;
> + }
> +
> + if (uuid) {
> + if (virUUIDParse(uuid, authdef->secret.uuid) < 0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("invalid auth secret uuid"));
> + goto cleanup;
> + }
> + authdef->secretType = VIR_STORAGE_SECRET_TYPE_UUID;
> + } else {
> + authdef->secret.usage = usage;
> + usage = NULL;
> + authdef->secretType = VIR_STORAGE_SECRET_TYPE_USAGE;
> + }
> + ret = 0;
> +
> + cleanup:
> + VIR_FREE(uuid);
> + VIR_FREE(usage);
> + return ret;
> +}
> +
> +
> +static virStorageAuthDefPtr
> +virStorageAuthDefParseXML(xmlXPathContextPtr ctxt)
> +{
> + virStorageAuthDefPtr authdef = NULL;
> + char *username = NULL;
> + char *authtype = NULL;
> +
> + if (VIR_ALLOC(authdef) < 0)
> + return NULL;
> +
> + if (!(username = virXPathString("string(./@username)", ctxt))) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("missing username for auth"));
> + goto error;
> + }
> + authdef->username = username;
> + username = NULL;
> +
> + authdef->authType = VIR_STORAGE_AUTH_TYPE_NONE;
> + authtype = virXPathString("string(./@type)", ctxt);
> + if (authtype) {
> + /* Used by the storage pool instead of the secret type field
> + * to define whether chap or ceph being used
> + */
> + if ((authdef->authType = virStorageAuthTypeFromString(authtype)) < 0)
{
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown auth type '%s'"),
authtype);
> + goto error;
> + }
> + VIR_FREE(authtype);
No need for this, as you'll free it just a few lines below.
Only on the error path...
> + }
> +
> + authdef->secretType = VIR_STORAGE_SECRET_TYPE_NONE;
> + if (virStorageAuthDefParseSecret(ctxt, authdef) < 0)
> + goto error;
> +
> + return authdef;
> +
> + error:
> + VIR_FREE(authtype);
> + VIR_FREE(username);
> + virStorageAuthDefFree(authdef);
> + return NULL;
> +}
> +
> +
> +virStorageAuthDefPtr
> +virStorageAuthDefParse(xmlDocPtr xml, xmlNodePtr root)
> +{
> + xmlXPathContextPtr ctxt = NULL;
> + virStorageAuthDefPtr authdef = NULL;
> +
> + ctxt = xmlXPathNewContext(xml);
> + if (ctxt == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + ctxt->node = root;
> + authdef = virStorageAuthDefParseXML(ctxt);
> +
> + cleanup:
> + xmlXPathFreeContext(ctxt);
> + return authdef;
> +}
> +
> +
> +int
> +virStorageAuthDefFormat(virBufferPtr buf,
> + virStorageAuthDefPtr authdef)
> +{
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> + if (authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE) {
> + virBufferEscapeString(buf, "<auth username='%s'>\n",
authdef->username);
> + } else {
> + virBufferAsprintf(buf, "<auth type='%s' ",
> + virStorageAuthTypeToString(authdef->authType));
> + virBufferEscapeString(buf, "username='%s'>\n",
authdef->username);
> + }
> +
> + virBufferAdjustIndent(buf, 2);
> + if (authdef->secrettype)
> + virBufferAsprintf(buf, "<secret type='%s'",
authdef->secrettype);
> + else
> + virBufferAddLit(buf, "<secret");
> +
> + if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
> + virUUIDFormat(authdef->secret.uuid, uuidstr);
> + virBufferAsprintf(buf, " uuid='%s'/>\n", uuidstr);
> + } else if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_USAGE) {
> + virBufferEscapeString(buf, " usage='%s'/>\n",
> + authdef->secret.usage);
> + } else {
> + virBufferAddLit(buf, "/>\n");
> + }
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</auth>\n");
> +
> + return 0;
> +}
> +
> +
> virSecurityDeviceLabelDefPtr
> virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
> const char *model)
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 48c7e02..383ba96 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -193,6 +193,15 @@ typedef virStorageSourcePoolDef *virStorageSourcePoolDefPtr;
>
>
> typedef enum {
> + VIR_STORAGE_AUTH_TYPE_NONE,
> + VIR_STORAGE_AUTH_TYPE_CHAP,
> + VIR_STORAGE_AUTH_TYPE_CEPHX,
> +
> + VIR_STORAGE_AUTH_TYPE_LAST,
> +} virStorageAuthType;
> +VIR_ENUM_DECL(virStorageAuth)
> +
> +typedef enum {
> VIR_STORAGE_SECRET_TYPE_NONE,
> VIR_STORAGE_SECRET_TYPE_UUID,
> VIR_STORAGE_SECRET_TYPE_USAGE,
> @@ -200,6 +209,19 @@ typedef enum {
> VIR_STORAGE_SECRET_TYPE_LAST
> } virStorageSecretType;
>
> +typedef struct _virStorageAuthDef virStorageAuthDef;
> +typedef virStorageAuthDef *virStorageAuthDefPtr;
> +struct _virStorageAuthDef {
> + char *username;
> + char *secrettype; /* <secret type='%s' for disk source */
> + int authType; /* virStorageAuthType */
> + int secretType; /* virStorageSecretType */
> + union {
> + unsigned char uuid[VIR_UUID_BUFLEN];
> + char *usage;
> + } secret;
> +};
Is it necessary to expose the struct or can we hide it in the
virstoragefile.c?
As you found out in 3/5... not easily... To be used by <disk> and
<pool>, but could also be used by <hostdev> for iSCSI.
John
> +
> typedef struct _virStorageDriverData virStorageDriverData;
> typedef virStorageDriverData *virStorageDriverDataPtr;
>
> @@ -307,6 +329,11 @@ int virStorageFileGetLVMKey(const char *path,
> int virStorageFileGetSCSIKey(const char *path,
> char **key);
>
> +void virStorageAuthDefFree(virStorageAuthDefPtr def);
> +virStorageAuthDefPtr virStorageAuthDefCopy(const virStorageAuthDef *src);
> +virStorageAuthDefPtr virStorageAuthDefParse(xmlDocPtr xml, xmlNodePtr root);
> +int virStorageAuthDefFormat(virBufferPtr buf, virStorageAuthDefPtr authdef);
> +
> virSecurityDeviceLabelDefPtr
> virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
> const char *model);
>
Michal
10:11 a.m.
New subject: [libvirt] [PATCH 2/5] qemuargv2xmltest: Resurrect RBD and iSCSI auth
Ressurect the disk-drive-network-iscsi-auth and disk-drive-network-rbd-auth
tests. Make adjustments to the args and xml file to be compatible with
other changes made to the non "-auth" so that the only difference is the
authentication information.
Adjust the qemuargv2xmltest.c to filter out "<secret" and
"</auth>" since
the args -> xml has no concept of usage it doesn't get printed. This results
in the </auth> being printed on the same line as "<secret" and the
secret
XML is not closed - a bit of an issue, but soon to be fixed.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tests/qemuargv2xmltest.c | 4 ++++
.../qemuxml2argv-disk-drive-network-iscsi-auth.args | 4 +++-
.../qemuxml2argv-disk-drive-network-iscsi-auth.xml | 12 +++++++++---
.../qemuxml2argv-disk-drive-network-rbd-auth.xml | 4 +++-
4 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
index 2cbbe3d..04d5a65 100644
--- a/tests/qemuargv2xmltest.c
+++ b/tests/qemuargv2xmltest.c
@@ -26,6 +26,8 @@ static int blankProblemElements(char *data)
if (virtTestClearLineRegex("<name>[[:alnum:]]+</name>", data)
< 0 ||
virtTestClearLineRegex("<uuid>([[:alnum:]]|-)+</uuid>",
data) < 0 ||
virtTestClearLineRegex("<memory.*>[[:digit:]]+</memory>",
data) < 0 ||
+ virtTestClearLineRegex("<secret.*>", data) < 0 ||
+ virtTestClearLineRegex("</auth.*>", data) < 0 ||
virtTestClearLineRegex("<currentMemory.*>[[:digit:]]+</currentMemory>",
data) < 0 ||
virtTestClearLineRegex("<readonly/>", data) < 0 ||
@@ -226,8 +228,10 @@ mymain(void)
DO_TEST("disk-drive-network-nbd-ipv6-export");
DO_TEST("disk-drive-network-nbd-unix");
DO_TEST("disk-drive-network-iscsi");
+ DO_TEST("disk-drive-network-iscsi-auth");
DO_TEST("disk-drive-network-gluster");
DO_TEST("disk-drive-network-rbd");
+ DO_TEST("disk-drive-network-rbd-auth");
DO_TEST("disk-drive-network-rbd-ipv6");
/* older format using CEPH_ARGS env var */
DO_TEST("disk-drive-network-rbd-ceph-env");
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.args
index dd8fee4..4c5e1be 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.args
@@ -3,5 +3,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
QEMU_AUDIO_DRV=none \
-M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \
-no-acpi -boot c -usb \
-drive file=iscsi://myname:AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A@example.org\
-/iqn.1992-01.com.example,if=virtio,format=raw \
+:6000/iqn.1992-01.com.example,if=virtio,format=raw \
+-drive file=iscsi://example.org:6000/iqn.1992-01.com.example/1,if=virtio,\
+format=raw \
-net none -serial none -parallel none
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.xml
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.xml
index ee87bdf..45df270 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.xml
@@ -20,13 +20,19 @@
<secret type='iscsi' usage='mycluster_myname'/>
</auth>
<source protocol='iscsi' name='iqn.1992-01.com.example'>
- <host name='example.org'/>
+ <host name='example.org' port='6000'/>
</source>
<target dev='vda' bus='virtio'/>
</disk>
+ <disk type='network' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source protocol='iscsi' name='iqn.1992-01.com.example/1'>
+ <host name='example.org' port='6000'/>
+ </source>
+ <target dev='vdb' bus='virtio'/>
+ </disk>
<controller type='usb' index='0'/>
- <controller type='ide' index='0'/>
<controller type='pci' index='0' model='pci-root'/>
- <memballoon model='virtio'/>
+ <memballoon model='none'/>
</devices>
</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
index 189ce6b..72923ea 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
@@ -15,6 +15,7 @@
<devices>
<emulator>/usr/bin/qemu</emulator>
<disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
<source dev='/dev/HostVG/QEMUGuest1'/>
<target dev='hda' bus='ide'/>
<address type='drive' controller='0' bus='0'
target='0' unit='0'/>
@@ -33,6 +34,7 @@
</disk>
<controller type='usb' index='0'/>
<controller type='ide' index='0'/>
- <memballoon model='virtio'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <memballoon model='none'/>
</devices>
</domain>
--
1.9.3
10:11 a.m.
New subject: [libvirt] [PATCH 3/5] Utilize virDomainDiskAuth for domain disk
Replace the inline "auth" struct in virStorageSource with a pointer
to a virStorageAuthDefPtr and utilize between the domain_conf, qemu_conf,
and qemu_command sources for finding the auth data for a domain disk
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 106 +++++++---------------------------------------
src/libvirt_private.syms | 1 -
src/qemu/qemu_command.c | 72 +++++++++++++++++++++----------
src/qemu/qemu_conf.c | 26 +++++++-----
src/util/virstoragefile.c | 14 +-----
src/util/virstoragefile.h | 10 +----
tests/qemuargv2xmltest.c | 1 -
7 files changed, 81 insertions(+), 149 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b7aa4f5..93f09a7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5203,7 +5203,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
{
virDomainDiskDefPtr def;
xmlNodePtr sourceNode = NULL;
- xmlNodePtr cur, child;
+ xmlNodePtr cur;
xmlNodePtr save_ctxt = ctxt->node;
char *type = NULL;
char *device = NULL;
@@ -5227,10 +5227,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
virStorageEncryptionPtr encryption = NULL;
char *serial = NULL;
char *startupPolicy = NULL;
- char *authUsername = NULL;
- char *authUsage = NULL;
- char *authUUID = NULL;
- char *usageType = NULL;
+ virStorageAuthDefPtr authdef = NULL;
char *tray = NULL;
char *removable = NULL;
char *logical_block_size = NULL;
@@ -5432,65 +5429,14 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
VIR_FREE(ready);
}
} else if (xmlStrEqual(cur->name, BAD_CAST "auth")) {
- authUsername = virXMLPropString(cur, "username");
- if (authUsername == NULL) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("missing username for auth"));
+ if (!(authdef = virStorageAuthDefParse(node->doc, cur)))
+ goto error;
+ if ((auth_secret_usage =
+ virSecretUsageTypeFromString(authdef->secrettype)) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("invalid secret type %s"),
+ authdef->secrettype);
goto error;
- }
-
- def->src->auth.secretType = VIR_STORAGE_SECRET_TYPE_NONE;
- child = cur->children;
- while (child != NULL) {
- if (child->type == XML_ELEMENT_NODE &&
- xmlStrEqual(child->name, BAD_CAST "secret")) {
- usageType = virXMLPropString(child, "type");
- if (usageType == NULL) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("missing type for secret"));
- goto error;
- }
- auth_secret_usage =
- virSecretUsageTypeFromString(usageType);
- if (auth_secret_usage < 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("invalid secret type %s"),
- usageType);
- goto error;
- }
-
- authUUID = virXMLPropString(child, "uuid");
- authUsage = virXMLPropString(child, "usage");
-
- if (authUUID != NULL && authUsage != NULL) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("only one of uuid and usage can be
specified"));
- goto error;
- }
-
- if (!authUUID && !authUsage) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("either uuid or usage should be "
- "specified for a secret"));
- goto error;
- }
-
- if (authUUID != NULL) {
- def->src->auth.secretType =
VIR_STORAGE_SECRET_TYPE_UUID;
- if (virUUIDParse(authUUID,
- def->src->auth.secret.uuid) < 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("malformed uuid %s"),
- authUUID);
- goto error;
- }
- } else if (authUsage != NULL) {
- def->src->auth.secretType =
VIR_STORAGE_SECRET_TYPE_USAGE;
- def->src->auth.secret.usage = authUsage;
- authUsage = NULL;
- }
- }
- child = child->next;
}
} else if (xmlStrEqual(cur->name, BAD_CAST "iotune")) {
if (virXPathULongLong("string(./iotune/total_bytes_sec)",
@@ -5944,8 +5890,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
def->dst = target;
target = NULL;
- def->src->auth.username = authUsername;
- authUsername = NULL;
+ def->src->auth = authdef;
+ authdef = NULL;
def->src->driverName = driverName;
driverName = NULL;
def->src->encryption = encryption;
@@ -5987,10 +5933,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
VIR_FREE(removable);
VIR_FREE(trans);
VIR_FREE(device);
- VIR_FREE(authUsername);
- VIR_FREE(usageType);
- VIR_FREE(authUUID);
- VIR_FREE(authUsage);
+ virStorageAuthDefFree(authdef);
VIR_FREE(driverType);
VIR_FREE(driverName);
VIR_FREE(cachetag);
@@ -15066,8 +15009,6 @@ virDomainDiskDefFormat(virBufferPtr buf,
const char *sgio = virDomainDeviceSGIOTypeToString(def->sgio);
const char *discard = virDomainDiskDiscardTypeToString(def->discard);
- char uuidstr[VIR_UUID_STRING_BUFLEN];
-
if (!type || !def->src->type) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected disk type %d"), def->src->type);
@@ -15149,26 +15090,9 @@ virDomainDiskDefFormat(virBufferPtr buf,
virBufferAddLit(buf, "/>\n");
}
- if (def->src->auth.username) {
- virBufferEscapeString(buf, "<auth username='%s'>\n",
- def->src->auth.username);
- virBufferAdjustIndent(buf, 2);
- if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) {
- virBufferAddLit(buf, "<secret type='iscsi'");
- } else if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
- virBufferAddLit(buf, "<secret type='ceph'");
- }
-
- if (def->src->auth.secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
- virUUIDFormat(def->src->auth.secret.uuid, uuidstr);
- virBufferAsprintf(buf, " uuid='%s'/>\n", uuidstr);
- }
- if (def->src->auth.secretType == VIR_STORAGE_SECRET_TYPE_USAGE) {
- virBufferEscapeString(buf, " usage='%s'/>\n",
- def->src->auth.secret.usage);
- }
- virBufferAdjustIndent(buf, -2);
- virBufferAddLit(buf, "</auth>\n");
+ if (def->src->auth) {
+ if (virStorageAuthDefFormat(buf, def->src->auth) < 0)
+ return -1;
}
if (virDomainDiskSourceFormat(buf, def->src, def->startupPolicy,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 17dcd67..119175b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1907,7 +1907,6 @@ virStorageNetHostDefFree;
virStorageNetHostTransportTypeFromString;
virStorageNetHostTransportTypeToString;
virStorageNetProtocolTypeToString;
-virStorageSourceAuthClear;
virStorageSourceBackingStoreClear;
virStorageSourceClear;
virStorageSourceFree;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 63f322a..6664547 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -45,6 +45,7 @@
#include "domain_conf.h"
#include "snapshot_conf.h"
#include "storage_conf.h"
+#include "secret_conf.h"
#include "network/bridge_driver.h"
#include "virnetdevtap.h"
#include "base64.h"
@@ -2469,9 +2470,7 @@ static char *
qemuGetSecretString(virConnectPtr conn,
const char *scheme,
bool encoded,
- int diskSecretType,
- char *username,
- unsigned char *uuid, char *usage,
+ virStorageAuthDefPtr authdef,
virSecretUsageType secretUsageType)
{
size_t secret_size;
@@ -2480,25 +2479,26 @@ qemuGetSecretString(virConnectPtr conn,
char uuidStr[VIR_UUID_STRING_BUFLEN];
/* look up secret */
- switch (diskSecretType) {
+ switch (authdef->secretType) {
case VIR_STORAGE_SECRET_TYPE_UUID:
- sec = virSecretLookupByUUID(conn, uuid);
- virUUIDFormat(uuid, uuidStr);
+ sec = virSecretLookupByUUID(conn, authdef->secret.uuid);
+ virUUIDFormat(authdef->secret.uuid, uuidStr);
break;
case VIR_STORAGE_SECRET_TYPE_USAGE:
- sec = virSecretLookupByUsage(conn, secretUsageType, usage);
+ sec = virSecretLookupByUsage(conn, secretUsageType,
+ authdef->secret.usage);
break;
}
if (!sec) {
- if (diskSecretType == VIR_STORAGE_SECRET_TYPE_UUID) {
+ if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
virReportError(VIR_ERR_NO_SECRET,
_("%s no secret matches uuid '%s'"),
scheme, uuidStr);
} else {
virReportError(VIR_ERR_NO_SECRET,
_("%s no secret matches usage value '%s'"),
- scheme, usage);
+ scheme, authdef->secret.usage);
}
goto cleanup;
}
@@ -2506,16 +2506,16 @@ qemuGetSecretString(virConnectPtr conn,
secret = (char *)conn->secretDriver->secretGetValue(sec, &secret_size, 0,
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
if (!secret) {
- if (diskSecretType == VIR_STORAGE_SECRET_TYPE_UUID) {
+ if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("could not get value of the secret for "
"username '%s' using uuid '%s'"),
- username, uuidStr);
+ authdef->username, uuidStr);
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("could not get value of the secret for "
"username '%s' using usage value
'%s'"),
- username, usage);
+ authdef->username, authdef->secret.usage);
}
goto cleanup;
}
@@ -2619,9 +2619,22 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)
*e = '\0';
}
- if (STRPREFIX(p, "id=") &&
- VIR_STRDUP(disk->src->auth.username, p + strlen("id=")) <
0)
- goto error;
+ if (STRPREFIX(p, "id=")) {
+ const char *secrettype;
+ /* formulate a disk->src->auth */
+ if (VIR_ALLOC(disk->src->auth) < 0)
+ goto error;
+
+ if (VIR_STRDUP(disk->src->auth->username, p +
strlen("id=")) < 0)
+ goto error;
+ secrettype = virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH);
+ if (VIR_STRDUP(disk->src->auth->secrettype, secrettype) < 0)
+ goto error;
+
+ /* Cannot formulate a secretType (eg, usage or uuid) given
+ * what is provided.
+ */
+ }
if (STRPREFIX(p, "mon_host=")) {
char *h, *sep;
@@ -2650,6 +2663,7 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)
error:
VIR_FREE(options);
+ virStorageAuthDefFree(disk->src->auth);
return -1;
}
@@ -2719,12 +2733,27 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri,
}
if (uri->user) {
+ const char *secrettype;
+ /* formulate a def->src->auth */
+ if (VIR_ALLOC(def->src->auth) < 0)
+ goto error;
+
secret = strchr(uri->user, ':');
if (secret)
*secret = '\0';
- if (VIR_STRDUP(def->src->auth.username, uri->user) < 0)
+ if (VIR_STRDUP(def->src->auth->username, uri->user) < 0)
goto error;
+ if (STREQ(scheme, "iscsi")) {
+ secrettype =
+ virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_ISCSI);
+ if (VIR_STRDUP(def->src->auth->secrettype, secrettype) < 0)
+ goto error;
+ }
+
+ /* Cannot formulate a secretType (eg, usage or uuid) given
+ * what is provided.
+ */
}
def->src->nhosts = 1;
@@ -2738,6 +2767,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri,
error:
virStorageNetHostDefClear(def->src->hosts);
VIR_FREE(def->src->hosts);
+ VIR_FREE(def->src->auth);
goto cleanup;
}
@@ -3134,14 +3164,13 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
if (conn) {
if (actualType == VIR_STORAGE_TYPE_NETWORK &&
- src->auth.username &&
+ src->auth &&
(src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI ||
src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
bool encode = false;
int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
const char *protocol = virStorageNetProtocolTypeToString(src->protocol);
-
- username = src->auth.username;
+ username = src->auth->username;
if (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
/* qemu requires the secret to be encoded for RBD */
@@ -3152,10 +3181,7 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
if (!(secret = qemuGetSecretString(conn,
protocol,
encode,
- src->auth.secretType,
- username,
- src->auth.secret.uuid,
- src->auth.secret.usage,
+ src->auth,
secretType)))
goto cleanup;
}
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 8a3bdef..43af60e 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1214,45 +1214,49 @@ qemuTranslateDiskSourcePoolAuth(virDomainDiskDefPtr def,
virStoragePoolDefPtr pooldef)
{
int ret = -1;
+ virStorageAuthDefPtr authdef;
/* Only necessary when authentication set */
if (pooldef->source.authType == VIR_STORAGE_POOL_AUTH_NONE) {
ret = 0;
goto cleanup;
}
+ if (VIR_ALLOC(def->src->auth) < 0)
+ goto cleanup;
+ authdef = def->src->auth;
/* Copy the authentication information from the storage pool
* into the virDomainDiskDef
*/
if (pooldef->source.authType == VIR_STORAGE_POOL_AUTH_CHAP) {
- if (VIR_STRDUP(def->src->auth.username,
+ if (VIR_STRDUP(authdef->username,
pooldef->source.auth.chap.username) < 0)
goto cleanup;
if (pooldef->source.auth.chap.secret.uuidUsable) {
- def->src->auth.secretType = VIR_STORAGE_SECRET_TYPE_UUID;
- memcpy(def->src->auth.secret.uuid,
+ authdef->secretType = VIR_STORAGE_SECRET_TYPE_UUID;
+ memcpy(authdef->secret.uuid,
pooldef->source.auth.chap.secret.uuid,
VIR_UUID_BUFLEN);
} else {
- if (VIR_STRDUP(def->src->auth.secret.usage,
+ if (VIR_STRDUP(authdef->secret.usage,
pooldef->source.auth.chap.secret.usage) < 0)
goto cleanup;
- def->src->auth.secretType = VIR_STORAGE_SECRET_TYPE_USAGE;
+ authdef->secretType = VIR_STORAGE_SECRET_TYPE_USAGE;
}
} else if (pooldef->source.authType == VIR_STORAGE_POOL_AUTH_CEPHX) {
- if (VIR_STRDUP(def->src->auth.username,
+ if (VIR_STRDUP(authdef->username,
pooldef->source.auth.cephx.username) < 0)
goto cleanup;
if (pooldef->source.auth.cephx.secret.uuidUsable) {
- def->src->auth.secretType = VIR_STORAGE_SECRET_TYPE_UUID;
- memcpy(def->src->auth.secret.uuid,
+ authdef->secretType = VIR_STORAGE_SECRET_TYPE_UUID;
+ memcpy(authdef->secret.uuid,
pooldef->source.auth.cephx.secret.uuid,
VIR_UUID_BUFLEN);
} else {
- if (VIR_STRDUP(def->src->auth.secret.usage,
+ if (VIR_STRDUP(authdef->secret.usage,
pooldef->source.auth.cephx.secret.usage) < 0)
goto cleanup;
- def->src->auth.secretType = VIR_STORAGE_SECRET_TYPE_USAGE;
+ authdef->secretType = VIR_STORAGE_SECRET_TYPE_USAGE;
}
}
ret = 0;
@@ -1315,7 +1319,7 @@ qemuTranslateDiskSourcePool(virConnectPtr conn,
VIR_FREE(def->src->path);
virStorageNetHostDefFree(def->src->nhosts, def->src->hosts);
- virStorageSourceAuthClear(def->src);
+ virStorageAuthDefFree(def->src->auth);
switch ((virStoragePoolType) pooldef->type) {
case VIR_STORAGE_POOL_DIR:
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 056e59e..93de343 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1733,18 +1733,6 @@ virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def)
}
-void
-virStorageSourceAuthClear(virStorageSourcePtr def)
-{
- VIR_FREE(def->auth.username);
-
- if (def->auth.secretType == VIR_STORAGE_SECRET_TYPE_USAGE)
- VIR_FREE(def->auth.secret.usage);
-
- def->auth.secretType = VIR_STORAGE_SECRET_TYPE_NONE;
-}
-
-
int
virStorageSourceGetActualType(virStorageSourcePtr def)
{
@@ -1802,7 +1790,7 @@ virStorageSourceClear(virStorageSourcePtr def)
VIR_FREE(def->timestamps);
virStorageNetHostDefFree(def->nhosts, def->hosts);
- virStorageSourceAuthClear(def);
+ virStorageAuthDefFree(def->auth);
virStorageSourceBackingStoreClear(def);
}
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 383ba96..833fbe1 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -239,14 +239,7 @@ struct _virStorageSource {
size_t nhosts;
virStorageNetHostDefPtr hosts;
virStorageSourcePoolDefPtr srcpool;
- struct {
- char *username;
- int secretType; /* virStorageSecretType */
- union {
- unsigned char uuid[VIR_UUID_BUFLEN];
- char *usage;
- } secret;
- } auth;
+ virStorageAuthDefPtr auth;
virStorageEncryptionPtr encryption;
char *driverName;
@@ -343,7 +336,6 @@ void virStorageNetHostDefFree(size_t nhosts, virStorageNetHostDefPtr
hosts);
virStorageNetHostDefPtr virStorageNetHostDefCopy(size_t nhosts,
virStorageNetHostDefPtr hosts);
-void virStorageSourceAuthClear(virStorageSourcePtr def);
void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def);
void virStorageSourceClear(virStorageSourcePtr def);
int virStorageSourceGetActualType(virStorageSourcePtr def);
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
index 04d5a65..6bad7d6 100644
--- a/tests/qemuargv2xmltest.c
+++ b/tests/qemuargv2xmltest.c
@@ -27,7 +27,6 @@ static int blankProblemElements(char *data)
virtTestClearLineRegex("<uuid>([[:alnum:]]|-)+</uuid>",
data) < 0 ||
virtTestClearLineRegex("<memory.*>[[:digit:]]+</memory>",
data) < 0 ||
virtTestClearLineRegex("<secret.*>", data) < 0 ||
- virtTestClearLineRegex("</auth.*>", data) < 0 ||
virtTestClearLineRegex("<currentMemory.*>[[:digit:]]+</currentMemory>",
data) < 0 ||
virtTestClearLineRegex("<readonly/>", data) < 0 ||
--
1.9.3
7:13 a.m.
New subject: [libvirt] [PATCH 3/5] Utilize virDomainDiskAuth for domain disk
On 27.06.2014 17:11, John Ferlan wrote:
Replace the inline "auth" struct in virStorageSource with a
pointer
to a virStorageAuthDefPtr and utilize between the domain_conf, qemu_conf,
and qemu_command sources for finding the auth data for a domain disk
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 106 +++++++---------------------------------------
src/libvirt_private.syms | 1 -
src/qemu/qemu_command.c | 72 +++++++++++++++++++++----------
src/qemu/qemu_conf.c | 26 +++++++-----
src/util/virstoragefile.c | 14 +-----
src/util/virstoragefile.h | 10 +----
tests/qemuargv2xmltest.c | 1 -
7 files changed, 81 insertions(+), 149 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 63f322a..6664547 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -45,6 +45,7 @@
#include "domain_conf.h"
#include "snapshot_conf.h"
#include "storage_conf.h"
+#include "secret_conf.h"
#include "network/bridge_driver.h"
#include "virnetdevtap.h"
#include "base64.h"
@@ -2469,9 +2470,7 @@ static char *
qemuGetSecretString(virConnectPtr conn,
const char *scheme,
bool encoded,
- int diskSecretType,
- char *username,
- unsigned char *uuid, char *usage,
+ virStorageAuthDefPtr authdef,
virSecretUsageType secretUsageType)
{
size_t secret_size;
@@ -2480,25 +2479,26 @@ qemuGetSecretString(virConnectPtr conn,
char uuidStr[VIR_UUID_STRING_BUFLEN];
/* look up secret */
- switch (diskSecretType) {
+ switch (authdef->secretType) {
Ahh, this answers my question in 1/5. Okay, leave the structure public.
Michal
8:10 a.m.
New subject: [libvirt] [PATCH 3/5] Utilize virDomainDiskAuth for domain disk
On 06/27/2014 05:11 PM, John Ferlan wrote:
Replace the inline "auth" struct in virStorageSource with a
pointer
to a virStorageAuthDefPtr and utilize between the domain_conf, qemu_conf,
and qemu_command sources for finding the auth data for a domain disk
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 106 +++++++---------------------------------------
src/libvirt_private.syms | 1 -
src/qemu/qemu_command.c | 72 +++++++++++++++++++++----------
src/qemu/qemu_conf.c | 26 +++++++-----
src/util/virstoragefile.c | 14 +-----
src/util/virstoragefile.h | 10 +----
tests/qemuargv2xmltest.c | 1 -
7 files changed, 81 insertions(+), 149 deletions(-)
@@ -2650,6 +2663,7 @@ static int
qemuParseRBDString(virDomainDiskDefPtr disk)
error:
VIR_FREE(options);
+ virStorageAuthDefFree(disk->src->auth);
This causes a double free - both callers free disk on failure.
return -1;
}
@@ -2738,6 +2767,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr
def, virURIPtr uri,
error:
virStorageNetHostDefClear(def->src->hosts);
VIR_FREE(def->src->hosts);
+ VIR_FREE(def->src->auth);
This should be freed by the callers too. (by StorageAuthDefFree)
goto cleanup;
}
@@ -1802,7 +1790,7 @@ virStorageSourceClear(virStorageSourcePtr def)
VIR_FREE(def->timestamps);
virStorageNetHostDefFree(def->nhosts, def->hosts);
- virStorageSourceAuthClear(def);
+ virStorageAuthDefFree(def->auth);
I don't like *Clear functions leaving pointers to freed memory behind, but
this one is only called right before freeing the StorageSource and it already
leaves def->hosts.
virStorageSourceBackingStoreClear(def);
}
Jan
10:44 a.m.
New subject: [libvirt] [PATCH 3/5] Utilize virDomainDiskAuth for domain disk
On 07/02/2014 09:10 AM, Ján Tomko wrote:
On 06/27/2014 05:11 PM, John Ferlan wrote:
> Replace the inline "auth" struct in virStorageSource with a pointer
> to a virStorageAuthDefPtr and utilize between the domain_conf, qemu_conf,
> and qemu_command sources for finding the auth data for a domain disk
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/domain_conf.c | 106 +++++++---------------------------------------
> src/libvirt_private.syms | 1 -
> src/qemu/qemu_command.c | 72 +++++++++++++++++++++----------
> src/qemu/qemu_conf.c | 26 +++++++-----
> src/util/virstoragefile.c | 14 +-----
> src/util/virstoragefile.h | 10 +----
> tests/qemuargv2xmltest.c | 1 -
> 7 files changed, 81 insertions(+), 149 deletions(-)
>
> @@ -2650,6 +2663,7 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)
>
> error:
> VIR_FREE(options);
> + virStorageAuthDefFree(disk->src->auth);
This causes a double free - both callers free disk on failure.
Oh right - the need to either sent disk->src->auth = NULL or
follow what was done in domain_conf - that is:
@@ -2590,6 +2590,7 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)
{
char *options = NULL;
char *p, *e, *next;
+ virStorageAuthDefPtr authdef = NULL;
p = strchr(disk->src->path, ':');
if (p) {
@@ -2621,15 +2622,17 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)
if (STRPREFIX(p, "id=")) {
const char *secrettype;
- /* formulate a disk->src->auth */
- if (VIR_ALLOC(disk->src->auth) < 0)
+ /* formulate authdef for disk->src->auth */
+ if (VIR_ALLOC(authdef) < 0)
goto error;
- if (VIR_STRDUP(disk->src->auth->username, p +
strlen("id=")) < 0)
+ if (VIR_STRDUP(authdef->username, p + strlen("id=")) < 0)
goto error;
secrettype = virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH)
- if (VIR_STRDUP(disk->src->auth->secrettype, secrettype) < 0)
+ if (VIR_STRDUP(authdef->secrettype, secrettype) < 0)
goto error;
+ disk->src->auth = authdef;
+ authdef = NULL;
/* Cannot formulate a secretType (eg, usage or uuid) given
* what is provided.
@@ -2663,7 +2666,7 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)
error:
VIR_FREE(options);
- virStorageAuthDefFree(disk->src->auth);
+ virStorageAuthDefFree(authdef);
return -1;
}
> return -1;
> }
>
> @@ -2738,6 +2767,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr
uri,
> error:
> virStorageNetHostDefClear(def->src->hosts);
> VIR_FREE(def->src->hosts);
> + VIR_FREE(def->src->auth);
This should be freed by the callers too. (by StorageAuthDefFree)
Hmm.. what was I thinking - even Coverity didn't catch this one...
Similar to RBD:
@@ -2676,6 +2679,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr
char *sock = NULL;
char *volimg = NULL;
char *secret = NULL;
+ virStorageAuthDefPtr authdef = NULL;
if (VIR_ALLOC(def->src->hosts) < 0)
goto error;
@@ -2734,22 +2738,24 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIP
if (uri->user) {
const char *secrettype;
- /* formulate a def->src->auth */
- if (VIR_ALLOC(def->src->auth) < 0)
+ /* formulate authdef for disk->src->auth */
+ if (VIR_ALLOC(authdef) < 0)
goto error;
secret = strchr(uri->user, ':');
if (secret)
*secret = '\0';
- if (VIR_STRDUP(def->src->auth->username, uri->user) < 0)
+ if (VIR_STRDUP(authdef->username, uri->user) < 0)
goto error;
if (STREQ(scheme, "iscsi")) {
secrettype =
virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_ISCSI);
- if (VIR_STRDUP(def->src->auth->secrettype, secrettype) < 0)
+ if (VIR_STRDUP(authdef->secrettype, secrettype) < 0)
goto error;
}
+ def->src->auth = authdef;
+ authdef = NULL;
/* Cannot formulate a secretType (eg, usage or uuid) given
* what is provided.
@@ -2767,7 +2773,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr
error:
virStorageNetHostDefClear(def->src->hosts);
VIR_FREE(def->src->hosts);
- VIR_FREE(def->src->auth);
+ virStorageAuthDefFree(authdef);
goto cleanup;
}
> goto cleanup;
> }
>
> @@ -1802,7 +1790,7 @@ virStorageSourceClear(virStorageSourcePtr def)
> VIR_FREE(def->timestamps);
>
> virStorageNetHostDefFree(def->nhosts, def->hosts);
> - virStorageSourceAuthClear(def);
> + virStorageAuthDefFree(def->auth);
I don't like *Clear functions leaving pointers to freed memory behind, but
this one is only called right before freeing the StorageSource and it already
leaves def->hosts.
I think you are asking for a 'def->auth = NULL;' right?
Similarly a 'def->hosts = NULL;' Of course it doesn't matter
since we're freeing def anyway. If you want it - I can put
it there.
FWIW: I was looking at the ->encryption code as a model...
At least in the domain_conf code...
John
>
> virStorageSourceBackingStoreClear(def);
> }
Jan
3:39 a.m.
New subject: [libvirt] [PATCH 3/5] Utilize virDomainDiskAuth for domain disk
On 07/02/2014 05:44 PM, John Ferlan wrote:
On 07/02/2014 09:10 AM, Ján Tomko wrote:
> On 06/27/2014 05:11 PM, John Ferlan wrote:
>> @@ -1802,7 +1790,7 @@ virStorageSourceClear(virStorageSourcePtr def)
>> VIR_FREE(def->timestamps);
>>
>> virStorageNetHostDefFree(def->nhosts, def->hosts);
>> - virStorageSourceAuthClear(def);
>> + virStorageAuthDefFree(def->auth);
>
> I don't like *Clear functions leaving pointers to freed memory behind, but
> this one is only called right before freeing the StorageSource and it already
> leaves def->hosts.
>
I think you are asking for a 'def->auth = NULL;' right?
Yes.
Similarly a 'def->hosts = NULL;' Of course it doesn't
matter
since we're freeing def anyway. If you want it - I can put
it there.
I think that's better left for a separate cleanup. I'll make a note on my TODO
list :)
Jan
10:11 a.m.
New subject: [libvirt] [PATCH 4/5] formatdomain: Fix issues found describing auth
Fix a couple of typos ('chap' should have been 'iscsi' and there was
a stray 'iqn.2013-07.com.example:iscsi-pool' entry. Clean up the
description of the <auth> element for the disk
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
docs/formatdomain.html.in | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 3075e16..d3e776b 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1594,18 +1594,17 @@
<host name='example.com' port='3260'/>
</source>
<auth username='myuser'>
- <secret type='chap' usage='libvirtiscsi'/>
+ <secret type='iscsi' usage='libvirtiscsi'/>
</auth>
<target dev='vda' bus='virtio'/>
</disk>
<disk type='network' device='lun'>
<driver name='qemu' type='raw'/>
<source protocol='iscsi'
name='iqn.2013-07.com.example:iscsi-nopool/1'>
- iqn.2013-07.com.example:iscsi-pool
<host name='example.com' port='3260'/>
</source>
<auth username='myuser'>
- <secret type='chap' usage='libvirtiscsi'/>
+ <secret type='iscsi' usage='libvirtiscsi'/>
</auth>
<target dev='sda' bus='scsi'/>
</disk>
@@ -1613,7 +1612,7 @@
<driver name='qemu' type='raw'/>
<source pool='iscsi-pool' volume='unit:0:0:1'
mode='host'/>
<auth username='myuser'>
- <secret type='chap' usage='libvirtiscsi'/>
+ <secret type='iscsi' usage='libvirtiscsi'/>
</auth>
<target dev='vda' bus='virtio'/>
</disk>
@@ -1621,7 +1620,7 @@
<driver name='qemu' type='raw'/>
<source pool='iscsi-pool' volume='unit:0:0:2'
mode='direct'/>
<auth username='myuser'>
- <secret type='chap' usage='libvirtiscsi'/>
+ <secret type='iscsi' usage='libvirtiscsi'/>
</auth>
<target dev='vda' bus='virtio'/>
</disk>
@@ -2180,7 +2179,10 @@
are available, each defaulting to 0.
</dd>
<dt><code>auth</code></dt>
- <dd>If present, the <code>auth</code> element provides the
+ <dd>The <code>auth</code> element is supported for a disk
+ <code>type</code> "network" that is using a
<code>source</code>
+ element with the <code>protocol</code> attributes "rbd" or
"iscsi".
+ If present, the <code>auth</code> element provides the
authentication credentials needed to access the source. It
includes a mandatory attribute <code>username</code>, which
identifies the username to use during authentication, as well
@@ -2189,11 +2191,11 @@
a <a href="formatsecret.html">libvirt secret object</a>
that
holds the actual password or other credentials (the domain XML
intentionally does not expose the password, only the reference
- to the object that does manage the password). For now, the
- known secret <code>type</code>s are "ceph", for Ceph RBD
- network sources, and "iscsi", for CHAP authentication of iSCSI
- targets. Both require either a <code>uuid</code> attribute
- with the UUID of the secret object, or a <code>usage</code>
+ to the object that does manage the password).
+ Known secret types are "ceph" for Ceph RBD network sources and
+ "iscsi" for CHAP authentication of iSCSI targets.
+ Both will require either a <code>uuid</code> attribute
+ with the UUID of the secret object or a <code>usage</code>
attribute matching the key that was specified in the
secret object. <span class="since">libvirt 0.9.7</span>
</dd>
--
1.9.3
10:11 a.m.
New subject: [libvirt] [PATCH 5/5] Utilize virDomainDiskAuth for storage pools
Replace the authType, chap, and cephx unions in virStoragePoolSource
with a single pointer to a virStorageAuthDefPtr. Adjust all users of
the previous chap/cephx and secret unions with the source->auth data.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/storage_conf.c | 152 +++++-------------------------------
src/conf/storage_conf.h | 38 +--------
src/qemu/qemu_conf.c | 46 ++---------
src/storage/storage_backend_iscsi.c | 41 +++++-----
src/storage/storage_backend_rbd.c | 65 ++++++++-------
5 files changed, 80 insertions(+), 262 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 8b6fd79..5b152f1 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -44,9 +44,12 @@
#include "viralloc.h"
#include "virfile.h"
#include "virstring.h"
+#include "virlog.h"
#define VIR_FROM_THIS VIR_FROM_STORAGE
+VIR_LOG_INIT("conf.storage_conf");
+
#define DEFAULT_POOL_PERM_MODE 0755
#define DEFAULT_VOL_PERM_MODE 0600
@@ -98,10 +101,6 @@ VIR_ENUM_IMPL(virStoragePoolSourceAdapter,
VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST,
"default", "scsi_host", "fc_host")
-VIR_ENUM_IMPL(virStoragePoolAuth,
- VIR_STORAGE_POOL_AUTH_LAST,
- "none", "chap", "ceph")
-
typedef const char *(*virStorageVolFormatToString)(int format);
typedef int (*virStorageVolFormatFromString)(const char *format);
typedef const char *(*virStorageVolFeatureToString)(int feature);
@@ -374,18 +373,9 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source)
VIR_FREE(source->name);
virStoragePoolSourceAdapterClear(source->adapter);
VIR_FREE(source->initiator.iqn);
+ virStorageAuthDefFree(source->auth);
VIR_FREE(source->vendor);
VIR_FREE(source->product);
-
- if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) {
- VIR_FREE(source->auth.chap.username);
- VIR_FREE(source->auth.chap.secret.usage);
- }
-
- if (source->authType == VIR_STORAGE_POOL_AUTH_CEPHX) {
- VIR_FREE(source->auth.cephx.username);
- VIR_FREE(source->auth.cephx.secret.usage);
- }
}
void
@@ -462,106 +452,17 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
}
static int
-virStoragePoolDefParseAuthSecret(xmlXPathContextPtr ctxt,
- virStoragePoolAuthSecretPtr secret)
-{
- char *uuid = NULL;
- int ret = -1;
-
- uuid = virXPathString("string(./auth/secret/@uuid)", ctxt);
- secret->usage = virXPathString("string(./auth/secret/@usage)", ctxt);
- if (uuid == NULL && secret->usage == NULL) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("missing auth secret uuid or usage attribute"));
- return -1;
- }
-
- if (uuid != NULL) {
- if (secret->usage != NULL) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("either auth secret uuid or usage expected"));
- goto cleanup;
- }
- if (virUUIDParse(uuid, secret->uuid) < 0) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("invalid auth secret uuid"));
- goto cleanup;
- }
- secret->uuidUsable = true;
- } else {
- secret->uuidUsable = false;
- }
-
- ret = 0;
- cleanup:
- VIR_FREE(uuid);
- return ret;
-}
-
-static int
-virStoragePoolDefParseAuth(xmlXPathContextPtr ctxt,
- virStoragePoolSourcePtr source)
-{
- int ret = -1;
- char *authType = NULL;
- char *username = NULL;
-
- authType = virXPathString("string(./auth/@type)", ctxt);
- if (authType == NULL) {
- source->authType = VIR_STORAGE_POOL_AUTH_NONE;
- ret = 0;
- goto cleanup;
- }
-
- if ((source->authType =
- virStoragePoolAuthTypeFromString(authType)) < 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown auth type '%s'"),
- authType);
- goto cleanup;
- }
-
- username = virXPathString("string(./auth/@username)", ctxt);
- if (username == NULL) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("missing auth username attribute"));
- goto cleanup;
- }
-
- if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) {
- source->auth.chap.username = username;
- username = NULL;
- if (virStoragePoolDefParseAuthSecret(ctxt,
- &source->auth.chap.secret) < 0)
- goto cleanup;
- }
- else if (source->authType == VIR_STORAGE_POOL_AUTH_CEPHX) {
- source->auth.cephx.username = username;
- username = NULL;
- if (virStoragePoolDefParseAuthSecret(ctxt,
- &source->auth.cephx.secret) < 0)
- goto cleanup;
- }
-
- ret = 0;
-
- cleanup:
- VIR_FREE(authType);
- VIR_FREE(username);
- return ret;
-}
-
-static int
virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
virStoragePoolSourcePtr source,
int pool_type,
xmlNodePtr node)
{
int ret = -1;
- xmlNodePtr relnode, *nodeset = NULL;
+ xmlNodePtr relnode, authnode, *nodeset = NULL;
int nsource;
size_t i;
virStoragePoolOptionsPtr options;
+ virStorageAuthDefPtr authdef = NULL;
char *name = NULL;
char *port = NULL;
char *adapter_type = NULL;
@@ -705,8 +606,18 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST;
}
- if (virStoragePoolDefParseAuth(ctxt, source) < 0)
- goto cleanup;
+ if ((authnode = virXPathNode("./auth", ctxt))) {
+ if (!(authdef = virStorageAuthDefParse(node->doc, authnode)))
+ goto cleanup;
+
+ if (authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("storage pool missing auth type"));
+ goto cleanup;
+ }
+
+ source->auth = authdef;
+ }
source->vendor = virXPathString("string(./vendor/@name)", ctxt);
source->product = virXPathString("string(./product/@name)", ctxt);
@@ -1057,7 +968,6 @@ virStoragePoolSourceFormat(virBufferPtr buf,
virStoragePoolSourcePtr src)
{
size_t i, j;
- char uuid[VIR_UUID_STRING_BUFLEN];
virBufferAddLit(buf, "<source>\n");
virBufferAdjustIndent(buf, 2);
@@ -1138,29 +1048,9 @@ virStoragePoolSourceFormat(virBufferPtr buf,
virBufferAsprintf(buf, "<format type='%s'/>\n", format);
}
- if (src->authType == VIR_STORAGE_POOL_AUTH_CHAP ||
- src->authType == VIR_STORAGE_POOL_AUTH_CEPHX) {
- virBufferAsprintf(buf, "<auth type='%s' ",
- virStoragePoolAuthTypeToString(src->authType));
- virBufferEscapeString(buf, "username='%s'>\n",
- (src->authType == VIR_STORAGE_POOL_AUTH_CHAP ?
- src->auth.chap.username :
- src->auth.cephx.username));
- virBufferAdjustIndent(buf, 2);
-
- virBufferAddLit(buf, "<secret");
- if (src->auth.cephx.secret.uuidUsable) {
- virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
- virBufferAsprintf(buf, " uuid='%s'", uuid);
- }
-
- if (src->auth.cephx.secret.usage != NULL) {
- virBufferAsprintf(buf, " usage='%s'",
src->auth.cephx.secret.usage);
- }
- virBufferAddLit(buf, "/>\n");
-
- virBufferAdjustIndent(buf, -2);
- virBufferAddLit(buf, "</auth>\n");
+ if (src->auth) {
+ if (virStorageAuthDefFormat(buf, src->auth) < 0)
+ return -1;
}
virBufferEscapeString(buf, "<vendor name='%s'/>\n",
src->vendor);
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index 04d99eb..47f769b 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -106,37 +106,6 @@ typedef enum {
} virStoragePoolDeviceType;
-typedef enum {
- VIR_STORAGE_POOL_AUTH_NONE,
- VIR_STORAGE_POOL_AUTH_CHAP,
- VIR_STORAGE_POOL_AUTH_CEPHX,
-
- VIR_STORAGE_POOL_AUTH_LAST,
-} virStoragePoolAuthType;
-VIR_ENUM_DECL(virStoragePoolAuth)
-
-typedef struct _virStoragePoolAuthSecret virStoragePoolAuthSecret;
-typedef virStoragePoolAuthSecret *virStoragePoolAuthSecretPtr;
-struct _virStoragePoolAuthSecret {
- unsigned char uuid[VIR_UUID_BUFLEN];
- char *usage;
- bool uuidUsable;
-};
-
-typedef struct _virStoragePoolAuthChap virStoragePoolAuthChap;
-typedef virStoragePoolAuthChap *virStoragePoolAuthChapPtr;
-struct _virStoragePoolAuthChap {
- char *username;
- virStoragePoolAuthSecret secret;
-};
-
-typedef struct _virStoragePoolAuthCephx virStoragePoolAuthCephx;
-typedef virStoragePoolAuthCephx *virStoragePoolAuthCephxPtr;
-struct _virStoragePoolAuthCephx {
- char *username;
- virStoragePoolAuthSecret secret;
-};
-
/*
* For remote pools, info on how to reach the host
*/
@@ -243,11 +212,8 @@ struct _virStoragePoolSource {
/* Initiator IQN */
virStoragePoolSourceInitiatorAttr initiator;
- int authType; /* virStoragePoolAuthType */
- union {
- virStoragePoolAuthChap chap;
- virStoragePoolAuthCephx cephx;
- } auth;
+ /* Authentication information */
+ virStorageAuthDefPtr auth;
/* Vendor of the source */
char *vendor;
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 43af60e..f92f831 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1211,54 +1211,18 @@ qemuAddISCSIPoolSourceHost(virDomainDiskDefPtr def,
static int
qemuTranslateDiskSourcePoolAuth(virDomainDiskDefPtr def,
- virStoragePoolDefPtr pooldef)
+ virStoragePoolSourcePtr source)
{
int ret = -1;
- virStorageAuthDefPtr authdef;
/* Only necessary when authentication set */
- if (pooldef->source.authType == VIR_STORAGE_POOL_AUTH_NONE) {
+ if (!source->auth) {
ret = 0;
goto cleanup;
}
- if (VIR_ALLOC(def->src->auth) < 0)
+ def->src->auth = virStorageAuthDefCopy(source->auth);
+ if (!def->src->auth)
goto cleanup;
- authdef = def->src->auth;
-
- /* Copy the authentication information from the storage pool
- * into the virDomainDiskDef
- */
- if (pooldef->source.authType == VIR_STORAGE_POOL_AUTH_CHAP) {
- if (VIR_STRDUP(authdef->username,
- pooldef->source.auth.chap.username) < 0)
- goto cleanup;
- if (pooldef->source.auth.chap.secret.uuidUsable) {
- authdef->secretType = VIR_STORAGE_SECRET_TYPE_UUID;
- memcpy(authdef->secret.uuid,
- pooldef->source.auth.chap.secret.uuid,
- VIR_UUID_BUFLEN);
- } else {
- if (VIR_STRDUP(authdef->secret.usage,
- pooldef->source.auth.chap.secret.usage) < 0)
- goto cleanup;
- authdef->secretType = VIR_STORAGE_SECRET_TYPE_USAGE;
- }
- } else if (pooldef->source.authType == VIR_STORAGE_POOL_AUTH_CEPHX) {
- if (VIR_STRDUP(authdef->username,
- pooldef->source.auth.cephx.username) < 0)
- goto cleanup;
- if (pooldef->source.auth.cephx.secret.uuidUsable) {
- authdef->secretType = VIR_STORAGE_SECRET_TYPE_UUID;
- memcpy(authdef->secret.uuid,
- pooldef->source.auth.cephx.secret.uuid,
- VIR_UUID_BUFLEN);
- } else {
- if (VIR_STRDUP(authdef->secret.usage,
- pooldef->source.auth.cephx.secret.usage) < 0)
- goto cleanup;
- authdef->secretType = VIR_STORAGE_SECRET_TYPE_USAGE;
- }
- }
ret = 0;
cleanup:
@@ -1387,7 +1351,7 @@ qemuTranslateDiskSourcePool(virConnectPtr conn,
def->src->srcpool->actualtype = VIR_STORAGE_TYPE_NETWORK;
def->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI;
- if (qemuTranslateDiskSourcePoolAuth(def, pooldef) < 0)
+ if (qemuTranslateDiskSourcePoolAuth(def, &pooldef->source) < 0)
goto cleanup;
if (qemuAddISCSIPoolSourceHost(def, pooldef) < 0)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index aa6980c..3aac9d3 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -278,18 +278,20 @@ virStorageBackendISCSICheckPool(virConnectPtr conn
ATTRIBUTE_UNUSED,
static int
virStorageBackendISCSISetAuth(const char *portal,
virConnectPtr conn,
- virStoragePoolDefPtr def)
+ virStoragePoolSourcePtr source)
{
virSecretPtr secret = NULL;
unsigned char *secret_value = NULL;
- virStoragePoolAuthChap chap;
+ virStorageAuthDefPtr authdef = source->auth;
int ret = -1;
char uuidStr[VIR_UUID_STRING_BUFLEN];
- if (def->source.authType == VIR_STORAGE_POOL_AUTH_NONE)
+ if (!authdef || authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE)
return 0;
- if (def->source.authType != VIR_STORAGE_POOL_AUTH_CHAP) {
+ VIR_DEBUG("username='%s' authType=%d secretType=%d",
+ authdef->username, authdef->authType, authdef->secretType);
+ if (authdef->authType != VIR_STORAGE_AUTH_TYPE_CHAP) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("iscsi pool only supports 'chap' auth
type"));
return -1;
@@ -302,12 +304,11 @@ virStorageBackendISCSISetAuth(const char *portal,
return -1;
}
- chap = def->source.auth.chap;
- if (chap.secret.uuidUsable)
- secret = virSecretLookupByUUID(conn, chap.secret.uuid);
+ if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID)
+ secret = virSecretLookupByUUID(conn, authdef->secret.uuid);
else
secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_ISCSI,
- chap.secret.usage);
+ authdef->secret.usage);
if (secret) {
size_t secret_size;
@@ -315,44 +316,44 @@ virStorageBackendISCSISetAuth(const char *portal,
conn->secretDriver->secretGetValue(secret, &secret_size, 0,
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
if (!secret_value) {
- if (chap.secret.uuidUsable) {
- virUUIDFormat(chap.secret.uuid, uuidStr);
+ if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
+ virUUIDFormat(authdef->secret.uuid, uuidStr);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("could not get the value of the secret "
"for username %s using uuid '%s'"),
- chap.username, uuidStr);
+ authdef->username, uuidStr);
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("could not get the value of the secret "
"for username %s using usage value
'%s'"),
- chap.username, chap.secret.usage);
+ authdef->username, authdef->secret.usage);
}
goto cleanup;
}
} else {
- if (chap.secret.uuidUsable) {
- virUUIDFormat(chap.secret.uuid, uuidStr);
+ if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
+ virUUIDFormat(authdef->secret.uuid, uuidStr);
virReportError(VIR_ERR_NO_SECRET,
_("no secret matches uuid '%s'"),
uuidStr);
} else {
virReportError(VIR_ERR_NO_SECRET,
_("no secret matches usage value '%s'"),
- chap.secret.usage);
+ authdef->secret.usage);
}
goto cleanup;
}
if (virISCSINodeUpdate(portal,
- def->source.devices[0].path,
+ source->devices[0].path,
"node.session.auth.authmethod",
"CHAP") < 0 ||
virISCSINodeUpdate(portal,
- def->source.devices[0].path,
+ source->devices[0].path,
"node.session.auth.username",
- chap.username) < 0 ||
+ authdef->username) < 0 ||
virISCSINodeUpdate(portal,
- def->source.devices[0].path,
+ source->devices[0].path,
"node.session.auth.password",
(const char *)secret_value) < 0)
goto cleanup;
@@ -404,7 +405,7 @@ virStorageBackendISCSIStartPool(virConnectPtr conn,
NULL, NULL) < 0)
goto cleanup;
- if (virStorageBackendISCSISetAuth(portal, conn, pool->def) < 0)
+ if (virStorageBackendISCSISetAuth(portal, conn, &pool->def->source)
< 0)
goto cleanup;
if (virISCSIConnectionLogin(portal,
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
index 5d4ef79..a582743 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -50,10 +50,11 @@ typedef virStorageBackendRBDState *virStorageBackendRBDStatePtr;
static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
virConnectPtr conn,
- virStoragePoolObjPtr pool)
+ virStoragePoolSourcePtr source)
{
int ret = -1;
int r = 0;
+ virStorageAuthDefPtr authdef = source->auth;
unsigned char *secret_value = NULL;
size_t secret_value_size;
char *rados_key = NULL;
@@ -66,12 +67,9 @@ static int
virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
const char *mon_op_timeout = "30";
const char *osd_op_timeout = "30";
- VIR_DEBUG("Found Cephx username: %s",
- pool->def->source.auth.cephx.username);
-
- if (pool->def->source.auth.cephx.username != NULL) {
- VIR_DEBUG("Using cephx authorization");
- r = rados_create(&ptr->cluster,
pool->def->source.auth.cephx.username);
+ if (authdef) {
+ VIR_DEBUG("Using cephx authorization, username: %s",
authdef->username);
+ r = rados_create(&ptr->cluster, authdef->username);
if (r < 0) {
virReportSystemError(-r, "%s", _("failed to initialize
RADOS"));
goto cleanup;
@@ -84,46 +82,45 @@ static int
virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
return -1;
}
- if (pool->def->source.auth.cephx.secret.uuidUsable) {
- virUUIDFormat(pool->def->source.auth.cephx.secret.uuid, secretUuid);
+ if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
+ virUUIDFormat(authdef->secret.uuid, secretUuid);
VIR_DEBUG("Looking up secret by UUID: %s", secretUuid);
secret = virSecretLookupByUUIDString(conn, secretUuid);
- } else if (pool->def->source.auth.cephx.secret.usage != NULL) {
+ } else if (authdef->secret.usage != NULL) {
VIR_DEBUG("Looking up secret by usage: %s",
- pool->def->source.auth.cephx.secret.usage);
+ authdef->secret.usage);
secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_CEPH,
-
pool->def->source.auth.cephx.secret.usage);
+ authdef->secret.usage);
}
if (secret == NULL) {
- if (pool->def->source.auth.cephx.secret.uuidUsable) {
+ if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
virReportError(VIR_ERR_NO_SECRET,
_("no secret matches uuid '%s'"),
secretUuid);
} else {
virReportError(VIR_ERR_NO_SECRET,
_("no secret matches usage value
'%s'"),
- pool->def->source.auth.cephx.secret.usage);
+ authdef->secret.usage);
}
goto cleanup;
}
- secret_value = conn->secretDriver->secretGetValue(secret,
&secret_value_size, 0,
+ secret_value = conn->secretDriver->secretGetValue(secret,
+ &secret_value_size, 0,
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
if (!secret_value) {
- if (pool->def->source.auth.cephx.secret.uuidUsable) {
+ if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("could not get the value of the secret "
"for username '%s' using uuid
'%s'"),
- pool->def->source.auth.cephx.username,
- secretUuid);
+ authdef->username, secretUuid);
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("could not get the value of the secret "
"for username '%s' using usage value
'%s'"),
- pool->def->source.auth.cephx.username,
- pool->def->source.auth.cephx.secret.usage);
+ authdef->username, authdef->secret.usage);
}
goto cleanup;
}
@@ -170,18 +167,18 @@ static int
virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
}
VIR_DEBUG("Found %zu RADOS cluster monitors in the pool configuration",
- pool->def->source.nhost);
+ source->nhost);
- for (i = 0; i < pool->def->source.nhost; i++) {
- if (pool->def->source.hosts[i].name != NULL &&
- !pool->def->source.hosts[i].port) {
+ for (i = 0; i < source->nhost; i++) {
+ if (source->hosts[i].name != NULL &&
+ !source->hosts[i].port) {
virBufferAsprintf(&mon_host, "%s:6789,",
- pool->def->source.hosts[i].name);
- } else if (pool->def->source.hosts[i].name != NULL &&
- pool->def->source.hosts[i].port) {
+ source->hosts[i].name);
+ } else if (source->hosts[i].name != NULL &&
+ source->hosts[i].port) {
virBufferAsprintf(&mon_host, "%s:%d,",
- pool->def->source.hosts[i].name,
- pool->def->source.hosts[i].port);
+ source->hosts[i].name,
+ source->hosts[i].port);
} else {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("received malformed monitor, check the XML
definition"));
@@ -335,7 +332,7 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn,
ptr.cluster = NULL;
ptr.ioctx = NULL;
- if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, pool) < 0) {
+ if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source)
< 0) {
goto cleanup;
}
@@ -437,7 +434,7 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
VIR_WARN("%s", _("This storage backend does not supported zeroed
removal of volumes"));
}
- if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, pool) < 0) {
+ if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source)
< 0) {
goto cleanup;
}
@@ -520,7 +517,7 @@ virStorageBackendRBDBuildVol(virConnectPtr conn,
virCheckFlags(0, -1);
- if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, pool) < 0)
+ if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source)
< 0)
goto cleanup;
if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0)
@@ -560,7 +557,7 @@ static int virStorageBackendRBDRefreshVol(virConnectPtr conn,
ptr.ioctx = NULL;
int ret = -1;
- if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, pool) < 0) {
+ if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source)
< 0) {
goto cleanup;
}
@@ -594,7 +591,7 @@ static int virStorageBackendRBDResizeVol(virConnectPtr conn
ATTRIBUTE_UNUSED,
virCheckFlags(0, -1);
- if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, pool) < 0) {
+ if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source)
< 0) {
goto cleanup;
}
--
1.9.3
7:13 a.m.
On 27.06.2014 17:11, John Ferlan wrote:
While understandably not really a 1.2.6 candidate, I figured I'd
post this
now in hopes that it gets the ball rolling. The changes will help with
a related bz to support libiscsi for SCSI passthrough devices where a
<hostdev> entry for an iscsi adapter would be able to have <auth> data.
The bulk of changes are code motion/merge. The details for each are:
The first patch will create new API's which will then be used by the
parse domain disk and the parse storage pool code if the "<auth" element
is found. The logic merges the parsing found in the domain disk and
storage pool code with one slight caveat - the domain disk code expects
to find "<secret type='xxxx'", where xxxx is 'iscsi' or
'ceph'. Trying
use the Type{To/From}String API's in virstoragefile.c resulted in a
link time failure (even though secret_conf.h was included). Never quite
figured it out - I will take suggestions though. Although since the
domain disk code is all that really cares about it and the domain_conf
code can use the Type{To/From}String API's - I just left it as is. The
new copy API is used when the pool code needs to copy it's auth data into
each domain disk's auth entry.
The second patch will fix a not-run test that tests the auth. It seems
it was not added to the active list of tests because the output generated
did not include the "usage" information (eg "
usage='mycluster_myname") that
is part of the <secret> element. Additionally because it did not include
it the output was "<secret type='iscsi' </auth>". So rather
than ignore
the test - add to the filters to avoid looking for "<secret" - through
to the closing ">". The -auth.xml and -auth.args files needed adjustments
to follow the non "-auth" version of the files that have changed over time,
but not also changed int the "-auth" files.
The third patch will cause the domain disk algorithms to utilize the
new generic parse and format APIs as well as using the 'authdef' instead
of the inline structure fields. This patch also restores the checking for
</auth> in the recently restored auth test. Still avoiding the whole
<secret... /> line as I saw no way to filter just the usage that's in
the stored XML file.
The fourth patch is a mostly innocuous html change - it could go early,
but it just felt better in this place.
The fifth patch will cause the storage pool algorithms to utilize the
new generic parse/format API's as well as new authdef structure for
accessing the data. The removal of the duplicated cephx/chap structures
seems (in my mind at least) to simplify things. They were essentially
copies of each other with no seemingly real value in keeping separate
other than the visual in the code of ".chap." or ".cephx.".
John Ferlan (5):
virstorage: Introduce virStorageAuthDef
qemuargv2xmltest: Resurrect RBD and iSCSI auth
Utilize virDomainDiskAuth for domain disk
formatdomain: Fix issues found describing auth
Utilize virDomainDiskAuth for storage pools
docs/formatdomain.html.in | 24 +--
src/conf/domain_conf.c | 106 ++--------
src/conf/storage_conf.c | 152 ++------------
src/conf/storage_conf.h | 38 +---
src/libvirt_private.syms | 5 +-
src/qemu/qemu_command.c | 72 ++++---
src/qemu/qemu_conf.c | 46 +----
src/storage/storage_backend_iscsi.c | 41 ++--
src/storage/storage_backend_rbd.c | 65 +++---
src/util/virstoragefile.c | 219 +++++++++++++++++++--
src/util/virstoragefile.h | 37 +++-
tests/qemuargv2xmltest.c | 3 +
...qemuxml2argv-disk-drive-network-iscsi-auth.args | 4 +-
.../qemuxml2argv-disk-drive-network-iscsi-auth.xml | 12 +-
.../qemuxml2argv-disk-drive-network-rbd-auth.xml | 4 +-
15 files changed, 415 insertions(+), 413 deletions(-)
ACK series.
Michal
4:44 p.m.
On 06/27/2014 11:11 AM, John Ferlan wrote:
While understandably not really a 1.2.6 candidate, I figured I'd
post this
now in hopes that it gets the ball rolling. The changes will help with
a related bz to support libiscsi for SCSI passthrough devices where a
<hostdev> entry for an iscsi adapter would be able to have <auth> data.
The bulk of changes are code motion/merge. The details for each are:
The first patch will create new API's which will then be used by the
parse domain disk and the parse storage pool code if the "<auth" element
is found. The logic merges the parsing found in the domain disk and
storage pool code with one slight caveat - the domain disk code expects
to find "<secret type='xxxx'", where xxxx is 'iscsi' or
'ceph'. Trying
use the Type{To/From}String API's in virstoragefile.c resulted in a
link time failure (even though secret_conf.h was included). Never quite
figured it out - I will take suggestions though. Although since the
domain disk code is all that really cares about it and the domain_conf
code can use the Type{To/From}String API's - I just left it as is. The
new copy API is used when the pool code needs to copy it's auth data into
each domain disk's auth entry.
The second patch will fix a not-run test that tests the auth. It seems
it was not added to the active list of tests because the output generated
did not include the "usage" information (eg "
usage='mycluster_myname") that
is part of the <secret> element. Additionally because it did not include
it the output was "<secret type='iscsi' </auth>". So rather
than ignore
the test - add to the filters to avoid looking for "<secret" - through
to the closing ">". The -auth.xml and -auth.args files needed adjustments
to follow the non "-auth" version of the files that have changed over time,
but not also changed int the "-auth" files.
The third patch will cause the domain disk algorithms to utilize the
new generic parse and format APIs as well as using the 'authdef' instead
of the inline structure fields. This patch also restores the checking for
</auth> in the recently restored auth test. Still avoiding the whole
<secret... /> line as I saw no way to filter just the usage that's in
the stored XML file.
The fourth patch is a mostly innocuous html change - it could go early,
but it just felt better in this place.
The fifth patch will cause the storage pool algorithms to utilize the
new generic parse/format API's as well as new authdef structure for
accessing the data. The removal of the duplicated cephx/chap structures
seems (in my mind at least) to simplify things. They were essentially
copies of each other with no seemingly real value in keeping separate
other than the visual in the code of ".chap." or ".cephx.".
John Ferlan (5):
virstorage: Introduce virStorageAuthDef
qemuargv2xmltest: Resurrect RBD and iSCSI auth
Utilize virDomainDiskAuth for domain disk
formatdomain: Fix issues found describing auth
Utilize virDomainDiskAuth for storage pools
docs/formatdomain.html.in | 24 +--
src/conf/domain_conf.c | 106 ++--------
src/conf/storage_conf.c | 152 ++------------
src/conf/storage_conf.h | 38 +---
src/libvirt_private.syms | 5 +-
src/qemu/qemu_command.c | 72 ++++---
src/qemu/qemu_conf.c | 46 +----
src/storage/storage_backend_iscsi.c | 41 ++--
src/storage/storage_backend_rbd.c | 65 +++---
src/util/virstoragefile.c | 219 +++++++++++++++++++--
src/util/virstoragefile.h | 37 +++-
tests/qemuargv2xmltest.c | 3 +
...qemuxml2argv-disk-drive-network-iscsi-auth.args | 4 +-
.../qemuxml2argv-disk-drive-network-iscsi-auth.xml | 12 +-
.../qemuxml2argv-disk-drive-network-rbd-auth.xml | 4 +-
15 files changed, 415 insertions(+), 413 deletions(-)
Made adjustments from reviews, made sure my build worked, retested, and
pushed.
Thanks for the reviews -
John
3830
days inactive
3836
days old
13 comments
3 participants
participants (3)
-
John Ferlan -
Ján Tomko -
Michal Privoznik