[libvirt] [PATCH v3 0/5] Adjustments for secinfo command line build and secret changes
v2: http://www.redhat.com/archives/libvir-list/2016-May/msg02140.html Patch 1 is the same as v2 patch 1 Patch 2 is already ACK'd - it was the previous patch 5 Patch 3 & 4 replace patches 2-4 from v2 Patch 5 is the same as v2 patch 6. John Ferlan (5): qemu: Move and rename qemuBuildObjectCommandlineFromJSON storage: Use virSecretGetSecretString qemu: Use virJSONValueObjectCreate for master key qemu: Remove need for qemuBuildSecretInfoProps secret: Move virStorageSecretType to secret_util and rename cfg.mk | 2 +- po/POTFILES.in | 1 + src/Makefile.am | 2 + src/conf/secret_conf.h | 2 +- src/libvirt_private.syms | 4 + src/libxl/libxl_conf.c | 2 +- src/qemu/qemu_command.c | 202 +++++++----------------------------- src/qemu/qemu_command.h | 4 - src/qemu/qemu_domain.c | 4 +- src/secret/secret_util.c | 18 ++-- src/secret/secret_util.h | 22 +++- src/storage/storage_backend_iscsi.c | 55 ++-------- src/storage/storage_backend_rbd.c | 49 +-------- src/util/virqemu.c | 142 +++++++++++++++++++++++++ src/util/virqemu.h | 34 ++++++ src/util/virstoragefile.c | 33 +++--- src/util/virstoragefile.h | 17 +-- tests/qemuargv2xmltest.c | 4 +- tests/qemucommandutiltest.c | 9 +- 19 files changed, 294 insertions(+), 312 deletions(-) create mode 100644 src/util/virqemu.c create mode 100644 src/util/virqemu.h -- 2.5.5
Move the module from qemu_command.c to a new module virqemu.c and rename the API to virQEMUBuildObjectCommandline. This API will then be shareable with qemu-img and the need to build a security object for luks support. Signed-off-by: John Ferlan <jferlan@redhat.com> --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 4 ++ src/qemu/qemu_command.c | 126 +++------------------------------------ src/qemu/qemu_command.h | 4 -- src/util/virqemu.c | 142 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virqemu.h | 34 +++++++++++ tests/qemucommandutiltest.c | 9 ++- 8 files changed, 195 insertions(+), 126 deletions(-) create mode 100644 src/util/virqemu.c create mode 100644 src/util/virqemu.h diff --git a/po/POTFILES.in b/po/POTFILES.in index 0d92448..2a6fae4 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -225,6 +225,7 @@ src/util/virpidfile.c src/util/virpolkit.c src/util/virportallocator.c src/util/virprocess.c +src/util/virqemu.c src/util/virrandom.c src/util/virrotatingfile.c src/util/virscsi.c diff --git a/src/Makefile.am b/src/Makefile.am index 12b66c2..f3c9a14 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -153,6 +153,7 @@ UTIL_SOURCES = \ util/virportallocator.c util/virportallocator.h \ util/virprobe.h \ util/virprocess.c util/virprocess.h \ + util/virqemu.c util/virqemu.h \ util/virrandom.h util/virrandom.c \ util/virrotatingfile.h util/virrotatingfile.c \ util/virscsi.c util/virscsi.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 333bf7c..33b80e8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2141,6 +2141,10 @@ virProcessTranslateStatus; virProcessWait; +# util/virqemu.h +virQEMUBuildObjectCommandlineFromJSON; + + # util/virrandom.h virRandom; virRandomBits; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 368bd87..1455c0d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -37,6 +37,7 @@ #include "virfile.h" #include "virnetdev.h" #include "virnetdevbridge.h" +#include "virqemu.h" #include "virstring.h" #include "virtime.h" #include "viruuid.h" @@ -286,116 +287,6 @@ qemuVirCommandGetDevSet(virCommandPtr cmd, int fd) } -static int -qemuBuildObjectCommandLinePropsInternal(const char *key, - const virJSONValue *value, - virBufferPtr buf, - bool nested) -{ - virJSONValuePtr elem; - virBitmapPtr bitmap = NULL; - ssize_t pos = -1; - ssize_t end; - size_t i; - - switch ((virJSONType) value->type) { - case VIR_JSON_TYPE_STRING: - virBufferAsprintf(buf, ",%s=%s", key, value->data.string); - break; - - case VIR_JSON_TYPE_NUMBER: - virBufferAsprintf(buf, ",%s=%s", key, value->data.number); - break; - - case VIR_JSON_TYPE_BOOLEAN: - if (value->data.boolean) - virBufferAsprintf(buf, ",%s=yes", key); - else - virBufferAsprintf(buf, ",%s=no", key); - - break; - - case VIR_JSON_TYPE_ARRAY: - if (nested) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("nested -object property arrays are not supported")); - return -1; - } - - if (virJSONValueGetArrayAsBitmap(value, &bitmap) == 0) { - while ((pos = virBitmapNextSetBit(bitmap, pos)) > -1) { - if ((end = virBitmapNextClearBit(bitmap, pos)) < 0) - end = virBitmapLastSetBit(bitmap) + 1; - - if (end - 1 > pos) { - virBufferAsprintf(buf, ",%s=%zd-%zd", key, pos, end - 1); - pos = end; - } else { - virBufferAsprintf(buf, ",%s=%zd", key, pos); - } - } - } else { - /* fallback, treat the array as a non-bitmap, adding the key - * for each member */ - for (i = 0; i < virJSONValueArraySize(value); i++) { - elem = virJSONValueArrayGet((virJSONValuePtr)value, i); - - /* recurse to avoid duplicating code */ - if (qemuBuildObjectCommandLinePropsInternal(key, elem, buf, - true) < 0) - return -1; - } - } - break; - - case VIR_JSON_TYPE_OBJECT: - case VIR_JSON_TYPE_NULL: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("NULL and OBJECT JSON types can't be converted to " - "commandline string")); - return -1; - } - - virBitmapFree(bitmap); - return 0; -} - - -static int -qemuBuildObjectCommandLineProps(const char *key, - const virJSONValue *value, - void *opaque) -{ - return qemuBuildObjectCommandLinePropsInternal(key, value, opaque, false); -} - - -char * -qemuBuildObjectCommandlineFromJSON(const char *type, - const char *alias, - virJSONValuePtr props) -{ - virBuffer buf = VIR_BUFFER_INITIALIZER; - char *ret = NULL; - - virBufferAsprintf(&buf, "%s,id=%s", type, alias); - - if (virJSONValueObjectForeachKeyValue(props, - qemuBuildObjectCommandLineProps, - &buf) < 0) - goto cleanup; - - if (virBufferCheckError(&buf) < 0) - goto cleanup; - - ret = virBufferContentAndReset(&buf); - - cleanup: - virBufferFreeAndReset(&buf); - return ret; -} - - char *qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk) { char *ret; @@ -680,8 +571,9 @@ qemuBuildObjectSecretCommandLine(virCommandPtr cmd, if (qemuBuildSecretInfoProps(secinfo, &type, &props) < 0) return -1; - if (!(tmp = qemuBuildObjectCommandlineFromJSON(type, secinfo->s.aes.alias, - props))) + if (!(tmp = virQEMUBuildObjectCommandlineFromJSON(type, + secinfo->s.aes.alias, + props))) goto cleanup; virCommandAddArgList(cmd, "-object", tmp, NULL); @@ -3226,9 +3118,9 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, &props, false)) < 0) goto cleanup; - if (!(*backendStr = qemuBuildObjectCommandlineFromJSON(backendType, - alias, - props))) + if (!(*backendStr = virQEMUBuildObjectCommandlineFromJSON(backendType, + alias, + props))) goto cleanup; ret = rc; @@ -3268,7 +3160,7 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem, &backendType, &props, true) < 0) goto cleanup; - ret = qemuBuildObjectCommandlineFromJSON(backendType, alias, props); + ret = virQEMUBuildObjectCommandlineFromJSON(backendType, alias, props); cleanup: VIR_FREE(alias); @@ -5411,7 +5303,7 @@ qemuBuildRNGBackendStr(virDomainRNGDefPtr rng, if (qemuBuildRNGBackendProps(rng, qemuCaps, &type, &props) < 0) goto cleanup; - ret = qemuBuildObjectCommandlineFromJSON(type, alias, props); + ret = virQEMUBuildObjectCommandlineFromJSON(type, alias, props); cleanup: VIR_FREE(alias); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1c22705..d2b7fa7 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -44,10 +44,6 @@ VIR_ENUM_DECL(qemuVideo) -char *qemuBuildObjectCommandlineFromJSON(const char *type, - const char *alias, - virJSONValuePtr props); - virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver, virLogManagerPtr logManager, virDomainDefPtr def, diff --git a/src/util/virqemu.c b/src/util/virqemu.c new file mode 100644 index 0000000..f87e20b --- /dev/null +++ b/src/util/virqemu.c @@ -0,0 +1,142 @@ +/* + * virqemu.c: QEMU object parsing/formatting + * + * Copyright (C) 2016 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + + +#include <config.h> + +#include "virbuffer.h" +#include "virerror.h" +#include "virlog.h" +#include "virqemu.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.qemu"); + + +static int +virQEMUBuildObjectCommandLinePropsInternal(const char *key, + const virJSONValue *value, + virBufferPtr buf, + bool nested) +{ + virJSONValuePtr elem; + virBitmapPtr bitmap = NULL; + ssize_t pos = -1; + ssize_t end; + size_t i; + + switch ((virJSONType) value->type) { + case VIR_JSON_TYPE_STRING: + virBufferAsprintf(buf, ",%s=%s", key, value->data.string); + break; + + case VIR_JSON_TYPE_NUMBER: + virBufferAsprintf(buf, ",%s=%s", key, value->data.number); + break; + + case VIR_JSON_TYPE_BOOLEAN: + if (value->data.boolean) + virBufferAsprintf(buf, ",%s=yes", key); + else + virBufferAsprintf(buf, ",%s=no", key); + + break; + + case VIR_JSON_TYPE_ARRAY: + if (nested) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("nested -object property arrays are not supported")); + return -1; + } + + if (virJSONValueGetArrayAsBitmap(value, &bitmap) == 0) { + while ((pos = virBitmapNextSetBit(bitmap, pos)) > -1) { + if ((end = virBitmapNextClearBit(bitmap, pos)) < 0) + end = virBitmapLastSetBit(bitmap) + 1; + + if (end - 1 > pos) { + virBufferAsprintf(buf, ",%s=%zd-%zd", key, pos, end - 1); + pos = end; + } else { + virBufferAsprintf(buf, ",%s=%zd", key, pos); + } + } + } else { + /* fallback, treat the array as a non-bitmap, adding the key + * for each member */ + for (i = 0; i < virJSONValueArraySize(value); i++) { + elem = virJSONValueArrayGet((virJSONValuePtr)value, i); + + /* recurse to avoid duplicating code */ + if (virQEMUBuildObjectCommandLinePropsInternal(key, elem, buf, + true) < 0) + return -1; + } + } + break; + + case VIR_JSON_TYPE_OBJECT: + case VIR_JSON_TYPE_NULL: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("NULL and OBJECT JSON types can't be converted to " + "commandline string")); + return -1; + } + + virBitmapFree(bitmap); + return 0; +} + + +static int +virQEMUBuildObjectCommandLineProps(const char *key, + const virJSONValue *value, + void *opaque) +{ + return virQEMUBuildObjectCommandLinePropsInternal(key, value, opaque, false); +} + + +char * +virQEMUBuildObjectCommandlineFromJSON(const char *type, + const char *alias, + virJSONValuePtr props) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *ret = NULL; + + virBufferAsprintf(&buf, "%s,id=%s", type, alias); + + if (virJSONValueObjectForeachKeyValue(props, + virQEMUBuildObjectCommandLineProps, + &buf) < 0) + goto cleanup; + + if (virBufferCheckError(&buf) < 0) + goto cleanup; + + ret = virBufferContentAndReset(&buf); + + cleanup: + virBufferFreeAndReset(&buf); + return ret; +} diff --git a/src/util/virqemu.h b/src/util/virqemu.h new file mode 100644 index 0000000..0a72202 --- /dev/null +++ b/src/util/virqemu.h @@ -0,0 +1,34 @@ +/* + * virqemu.h: qemu specific object parsing/formatting + * + * Copyright (C) 2009, 2012-2016 Red Hat, Inc. + * Copyright (C) 2009 Daniel P. Berrange + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + + +#ifndef __VIR_QEMU_H_ +# define __VIR_QEMU_H_ + +# include "internal.h" +# include "virjson.h" + +char *virQEMUBuildObjectCommandlineFromJSON(const char *type, + const char *alias, + virJSONValuePtr props); + +#endif /* __VIR_QEMU_H_ */ diff --git a/tests/qemucommandutiltest.c b/tests/qemucommandutiltest.c index bd457f8..5072a62 100644 --- a/tests/qemucommandutiltest.c +++ b/tests/qemucommandutiltest.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2015 Red Hat, Inc. + * Copyright (C) 2015-2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -19,8 +19,8 @@ #include <config.h> -#include "qemu/qemu_command.h" #include "util/virjson.h" +#include "util/virqemu.h" #include "testutils.h" #include "testutilsqemu.h" @@ -51,9 +51,8 @@ testQemuCommandBuildObjectFromJSON(const void *opaque) data->expectprops ? data->expectprops : "") < 0) return -1; - result = qemuBuildObjectCommandlineFromJSON("testobject", - "testalias", - val); + result = virQEMUBuildObjectCommandlineFromJSON("testobject", + "testalias", val); if (STRNEQ_NULLABLE(expect, result)) { fprintf(stderr, "\nFailed to create object string. " -- 2.5.5
On Fri, Jun 03, 2016 at 06:52:49 -0400, John Ferlan wrote:
Move the module from qemu_command.c to a new module virqemu.c and rename the API to virQEMUBuildObjectCommandline.
This API will then be shareable with qemu-img and the need to build a security object for luks support.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 4 ++ src/qemu/qemu_command.c | 126 +++------------------------------------ src/qemu/qemu_command.h | 4 -- src/util/virqemu.c | 142 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virqemu.h | 34 +++++++++++ tests/qemucommandutiltest.c | 9 ++- 8 files changed, 195 insertions(+), 126 deletions(-) create mode 100644 src/util/virqemu.c create mode 100644 src/util/virqemu.h
[...]
diff --git a/src/util/virqemu.c b/src/util/virqemu.c new file mode 100644 index 0000000..f87e20b --- /dev/null +++ b/src/util/virqemu.c @@ -0,0 +1,142 @@ +/* + * virqemu.c: QEMU object parsing/formatting
More like "utilities for working with qemu and its tools". Same in the header. ACK. Peter
Rather than inline code secret lookup for rbd/iscsi, use the common function. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/Makefile.am | 1 + src/storage/storage_backend_iscsi.c | 50 +++++-------------------------------- src/storage/storage_backend_rbd.c | 48 +++-------------------------------- 3 files changed, 10 insertions(+), 89 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index f3c9a14..019242b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1615,6 +1615,7 @@ libvirt_driver_storage_impl_la_SOURCES = libvirt_driver_storage_impl_la_CFLAGS = \ -I$(srcdir)/access \ -I$(srcdir)/conf \ + -I$(srcdir)/secret \ $(AM_CFLAGS) libvirt_driver_storage_impl_la_LDFLAGS = $(AM_LDFLAGS) libvirt_driver_storage_impl_la_LIBADD = diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index bccfba3..a45c525 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -1,7 +1,7 @@ /* * storage_backend_iscsi.c: storage backend for iSCSI handling * - * Copyright (C) 2007-2014 Red Hat, Inc. + * Copyright (C) 2007-2016 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -43,7 +43,7 @@ #include "virobject.h" #include "virstring.h" #include "viruuid.h" - +#include "secret_util.h" #define VIR_FROM_THIS VIR_FROM_STORAGE VIR_LOG_INIT("storage.storage_backend_iscsi"); @@ -277,11 +277,10 @@ virStorageBackendISCSISetAuth(const char *portal, virConnectPtr conn, virStoragePoolSourcePtr source) { - virSecretPtr secret = NULL; unsigned char *secret_value = NULL; + size_t secret_size; virStorageAuthDefPtr authdef = source->auth; int ret = -1; - char uuidStr[VIR_UUID_STRING_BUFLEN]; if (!authdef || authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE) return 0; @@ -301,45 +300,9 @@ virStorageBackendISCSISetAuth(const char *portal, return -1; } - if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) - secret = virSecretLookupByUUID(conn, authdef->secret.uuid); - else - secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_ISCSI, - authdef->secret.usage); - - if (secret) { - size_t secret_size; - secret_value = - conn->secretDriver->secretGetValue(secret, &secret_size, 0, - VIR_SECRET_GET_VALUE_INTERNAL_CALL); - if (!secret_value) { - 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'"), - authdef->username, uuidStr); - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("could not get the value of the secret " - "for username %s using usage value '%s'"), - authdef->username, authdef->secret.usage); - } - goto cleanup; - } - } else { - 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'"), - authdef->secret.usage); - } + if (virSecretGetSecretString(conn, authdef, VIR_SECRET_USAGE_TYPE_ISCSI, + &secret_value, &secret_size) < 0) goto cleanup; - } if (virISCSINodeUpdate(portal, source->devices[0].path, @@ -358,8 +321,7 @@ virStorageBackendISCSISetAuth(const char *portal, ret = 0; cleanup: - virObjectUnref(secret); - VIR_FREE(secret_value); + VIR_DISPOSE_N(secret_value, secret_size); return ret; } diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index ac46b9d..64ec545 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -36,6 +36,7 @@ #include "virrandom.h" #include "rados/librados.h" #include "rbd/librbd.h" +#include "secret_util.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -62,8 +63,6 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, size_t secret_value_size = 0; char *rados_key = NULL; virBuffer mon_host = VIR_BUFFER_INITIALIZER; - virSecretPtr secret = NULL; - char secretUuid[VIR_UUID_STRING_BUFLEN]; size_t i; char *mon_buff = NULL; const char *client_mount_timeout = "30"; @@ -86,48 +85,9 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, return -1; } - 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 (authdef->secret.usage != NULL) { - VIR_DEBUG("Looking up secret by usage: %s", - authdef->secret.usage); - secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_CEPH, - authdef->secret.usage); - } - - if (secret == NULL) { - 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'"), - authdef->secret.usage); - } + if (virSecretGetSecretString(conn, authdef, VIR_SECRET_USAGE_TYPE_CEPH, + &secret_value, &secret_value_size) < 0) goto cleanup; - } - - secret_value = conn->secretDriver->secretGetValue(secret, - &secret_value_size, 0, - VIR_SECRET_GET_VALUE_INTERNAL_CALL); - - if (!secret_value) { - 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'"), - authdef->username, secretUuid); - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("could not get the value of the secret " - "for username '%s' using usage value '%s'"), - authdef->username, authdef->secret.usage); - } - goto cleanup; - } if (!(rados_key = virStringEncodeBase64(secret_value, secret_value_size))) goto cleanup; @@ -227,8 +187,6 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, VIR_DISPOSE_N(secret_value, secret_value_size); VIR_DISPOSE_STRING(rados_key); - virObjectUnref(secret); - virBufferFreeAndReset(&mon_host); VIR_FREE(mon_buff); return ret; -- 2.5.5
On Fri, Jun 03, 2016 at 06:52:50 -0400, John Ferlan wrote:
Rather than inline code secret lookup for rbd/iscsi, use the common function.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/Makefile.am | 1 + src/storage/storage_backend_iscsi.c | 50 +++++-------------------------------- src/storage/storage_backend_rbd.c | 48 +++-------------------------------- 3 files changed, 10 insertions(+), 89 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index f3c9a14..019242b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1615,6 +1615,7 @@ libvirt_driver_storage_impl_la_SOURCES = libvirt_driver_storage_impl_la_CFLAGS = \ -I$(srcdir)/access \ -I$(srcdir)/conf \ + -I$(srcdir)/secret \
Similarly to my complaint in 5/5 this is breaking the boundaries between the storage driver and the secret driver. This is not the first case of this though so we just need to thoroughly check that the appropriate bits are linked in to the storage driver. I'll need to check this. Otherwise looks good. Peter
On 06/06/2016 03:32 AM, Peter Krempa wrote:
On Fri, Jun 03, 2016 at 06:52:50 -0400, John Ferlan wrote:
Rather than inline code secret lookup for rbd/iscsi, use the common function.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/Makefile.am | 1 + src/storage/storage_backend_iscsi.c | 50 +++++-------------------------------- src/storage/storage_backend_rbd.c | 48 +++-------------------------------- 3 files changed, 10 insertions(+), 89 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index f3c9a14..019242b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1615,6 +1615,7 @@ libvirt_driver_storage_impl_la_SOURCES = libvirt_driver_storage_impl_la_CFLAGS = \ -I$(srcdir)/access \ -I$(srcdir)/conf \ + -I$(srcdir)/secret \
Similarly to my complaint in 5/5 this is breaking the boundaries between the storage driver and the secret driver. This is not the first case of this though so we just need to thoroughly check that the appropriate bits are linked in to the storage driver.
I'll need to check this.
Otherwise looks good.
Peter
Do you mean boundaries that weren't broken before? I would think 5/5 is a different case/issue. Ironcially, RBD/iSCSI would use virSecret* libvirt API's to get the lookup type and then conn->secretDriver knowledge to perform the actual lookup. This is the one that had already been ACK'd and essentially follows commit id 2844de6f4 done for libvirt_driver_qemu_impl_la and libvirt_driver_libxl_impl_la. It's adding access to one function and one enum. John
Rather than open coding, follow the secinfo code and use the common secret object build/generate sequence. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1455c0d..6920379 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -198,6 +198,9 @@ qemuBuildMasterKeyCommandLine(virCommandPtr cmd, char *alias = NULL; char *path = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; + virJSONValuePtr props = NULL; + char *tmp = NULL; + /* If the -object secret does not exist, then just return. This just * means the domain won't be able to use a secret master key and is @@ -219,10 +222,22 @@ qemuBuildMasterKeyCommandLine(virCommandPtr cmd, if (!(path = qemuDomainGetMasterKeyFilePath(domainLibDir))) goto cleanup; - virCommandAddArg(cmd, "-object"); - virBufferAsprintf(&buf, "secret,id=%s,format=raw,file=", alias); qemuBufferEscapeComma(&buf, path); - virCommandAddArgBuffer(cmd, &buf); + if (virBufferCheckError(&buf) < 0) + goto cleanup; + VIR_FREE(path); + path = virBufferContentAndReset(&buf); + + if (virJSONValueObjectCreate(&props, + "s:format", "raw", + "s:file", path, + NULL) < 0) + goto cleanup; + + if (!(tmp = virQEMUBuildObjectCommandlineFromJSON("secret", alias, props))) + goto cleanup; + + virCommandAddArgList(cmd, "-object", tmp, NULL); ret = 0; -- 2.5.5
On Fri, Jun 03, 2016 at 06:52:51 -0400, John Ferlan wrote:
Rather than open coding, follow the secinfo code and use the common secret object build/generate sequence.
The main reason to do this was to have a single code path that generates properties both for adding the object to qemu via commandline and via monitor. Is this going to be used on the monitor? If not there's no reason to do this. Peter
On 06/06/2016 03:23 AM, Peter Krempa wrote:
On Fri, Jun 03, 2016 at 06:52:51 -0400, John Ferlan wrote:
Rather than open coding, follow the secinfo code and use the common secret object build/generate sequence.
The main reason to do this was to have a single code path that generates properties both for adding the object to qemu via commandline and via monitor. Is this going to be used on the monitor? If not there's no reason to do this.
The main reason to do this was to have the commandline code generating the master secret perform the same virJSON* call that the code for generating the AES secret uses. Not sure what the reference for via monitor is all about. If using common code isn't desire, then I can drop it. John
On Mon, Jun 06, 2016 at 07:14:00 -0400, John Ferlan wrote:
On 06/06/2016 03:23 AM, Peter Krempa wrote:
On Fri, Jun 03, 2016 at 06:52:51 -0400, John Ferlan wrote:
Rather than open coding, follow the secinfo code and use the common secret object build/generate sequence.
The main reason to do this was to have a single code path that generates properties both for adding the object to qemu via commandline and via monitor. Is this going to be used on the monitor? If not there's no reason to do this.
The main reason to do this was to have the commandline code generating the master secret perform the same virJSON* call that the code for generating the AES secret uses.
Well that is useful only if you are going to use the same code generating the properties to be used via monitor too. Otherwise the code will waste quite a few cycles to parse the strings and generate the JSON structure just to iterate it and format it back to strings.
Not sure what the reference for via monitor is all about.
The idea behind qemuBuildObjectCommandlineFromJSON or virQEMUBuildObjectCommandlineFromJSON as of 1/5 was to have a common place that generates properties for objects (namely memory device backends at the time I wrote that fucntion) so that we don't need to implement it twice. Once for the command line and once for the monitor. As the monitor json property can be converted to the commandline it's desired to use the JSON format then.
If using common code isn't desire, then I can drop it.
It is desire if the code will be used both when generating the command line and when talking on the monitor. The master key is not the case since we will not be setting the master key using the monitor. Additionally since we consider that the monitor is secure from prying eyes we can pass the secrets directly that way so generating the JSON props is just a waste of compute time.
Just move the code into qemuBuildObjectSecretCommandLine. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 57 +++++++++++-------------------------------------- 1 file changed, 12 insertions(+), 45 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6920379..3fda234 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -523,52 +523,11 @@ qemuNetworkDriveGetPort(int protocol, /** - * qemuBuildSecretInfoProps: - * @secinfo: pointer to the secret info object - * @type: returns a pointer to a character string for object name - * @props: json properties to return - * - * Build the JSON properties for the secret info type. - * - * Returns 0 on success with the filled in JSON property; otherwise, - * returns -1 on failure error message set. - */ -static int -qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, - const char **type, - virJSONValuePtr *propsret) -{ - int ret = -1; - char *keyid = NULL; - - *type = "secret"; - - if (!(keyid = qemuDomainGetMasterKeyAlias())) - return -1; - - if (virJSONValueObjectCreate(propsret, - "s:data", secinfo->s.aes.ciphertext, - "s:keyid", keyid, - "s:iv", secinfo->s.aes.iv, - "s:format", "base64", NULL) < 0) - goto cleanup; - - ret = 0; - - cleanup: - VIR_FREE(keyid); - - return ret; -} - - -/** * qemuBuildObjectSecretCommandLine: * @cmd: the command to modify * @secinfo: pointer to the secret info object * - * If the secinfo is available and associated with an AES secret, - * then format the command line for the secret object. This object + * Format the command line for the AES secret object. This object * will be referenced by the device that needs/uses it, so it needs * to be in place first. * @@ -579,14 +538,21 @@ qemuBuildObjectSecretCommandLine(virCommandPtr cmd, qemuDomainSecretInfoPtr secinfo) { int ret = -1; + char *keyid = NULL; virJSONValuePtr props = NULL; - const char *type; char *tmp = NULL; - if (qemuBuildSecretInfoProps(secinfo, &type, &props) < 0) + if (!(keyid = qemuDomainGetMasterKeyAlias())) return -1; - if (!(tmp = virQEMUBuildObjectCommandlineFromJSON(type, + if (virJSONValueObjectCreate(&props, + "s:data", secinfo->s.aes.ciphertext, + "s:keyid", keyid, + "s:iv", secinfo->s.aes.iv, + "s:format", "base64", NULL) < 0) + goto cleanup; + + if (!(tmp = virQEMUBuildObjectCommandlineFromJSON("secret", secinfo->s.aes.alias, props))) goto cleanup; @@ -597,6 +563,7 @@ qemuBuildObjectSecretCommandLine(virCommandPtr cmd, cleanup: virJSONValueFree(props); VIR_FREE(tmp); + VIR_FREE(keyid); return ret; } -- 2.5.5
On Fri, Jun 03, 2016 at 06:52:52 -0400, John Ferlan wrote:
Just move the code into qemuBuildObjectSecretCommandLine.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 57 +++++++++++-------------------------------------- 1 file changed, 12 insertions(+), 45 deletions(-)
Again. If this is going to be reused in the monitor then it doesn't make sense to move it to the builder directly. If it's not going to be used with the monitor then it doesn't make sense to use the json parser and the object formatter to achieve the same at all since it's rather untrivial code that can be achieved with a single printf. Peter
On 06/06/2016 03:26 AM, Peter Krempa wrote:
On Fri, Jun 03, 2016 at 06:52:52 -0400, John Ferlan wrote:
Just move the code into qemuBuildObjectSecretCommandLine.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 57 +++++++++++-------------------------------------- 1 file changed, 12 insertions(+), 45 deletions(-)
Again. If this is going to be reused in the monitor then it doesn't make sense to move it to the builder directly. If it's not going to be used with the monitor then it doesn't make sense to use the json parser and the object formatter to achieve the same at all since it's rather untrivial code that can be achieved with a single printf.
Huh? This patch just moves code from qemuBuildSecretInfoProps into qemuBuildObjectSecretCommandLine as the only caller to the former was the latter. I can drop this though - it's not that important. John
On Mon, Jun 06, 2016 at 07:16:29 -0400, John Ferlan wrote:
On 06/06/2016 03:26 AM, Peter Krempa wrote:
On Fri, Jun 03, 2016 at 06:52:52 -0400, John Ferlan wrote:
Just move the code into qemuBuildObjectSecretCommandLine.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 57 +++++++++++-------------------------------------- 1 file changed, 12 insertions(+), 45 deletions(-)
Again. If this is going to be reused in the monitor then it doesn't make sense to move it to the builder directly. If it's not going to be used with the monitor then it doesn't make sense to use the json parser and the object formatter to achieve the same at all since it's rather untrivial code that can be achieved with a single printf.
Huh? This patch just moves code from qemuBuildSecretInfoProps into qemuBuildObjectSecretCommandLine as the only caller to the former was the latter.
I can drop this though - it's not that important.
I'm pointing out that if there's no need to use the code to pass secret objects via the monitor then we should not use the JSON to commandline generator at all. Moving of the code directly to the place where we format the command line is a very apparent reason that it's not going to be used anywhere besides the command line.
On 06/06/2016 07:31 AM, Peter Krempa wrote:
On Mon, Jun 06, 2016 at 07:16:29 -0400, John Ferlan wrote:
On 06/06/2016 03:26 AM, Peter Krempa wrote:
On Fri, Jun 03, 2016 at 06:52:52 -0400, John Ferlan wrote:
Just move the code into qemuBuildObjectSecretCommandLine.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 57 +++++++++++-------------------------------------- 1 file changed, 12 insertions(+), 45 deletions(-)
Again. If this is going to be reused in the monitor then it doesn't make sense to move it to the builder directly. If it's not going to be used with the monitor then it doesn't make sense to use the json parser and the object formatter to achieve the same at all since it's rather untrivial code that can be achieved with a single printf.
Huh? This patch just moves code from qemuBuildSecretInfoProps into qemuBuildObjectSecretCommandLine as the only caller to the former was the latter.
I can drop this though - it's not that important.
I'm pointing out that if there's no need to use the code to pass secret objects via the monitor then we should not use the JSON to commandline generator at all.
Moving of the code directly to the place where we format the command line is a very apparent reason that it's not going to be used anywhere besides the command line.
That's fine - I'll drop 3 & 4. The qemuBuildSecretInfoProps could have a future use from hotplug for the monitor. John
Move the enum into secret_util, rename it to be just virSecretLookupType. This includes quite a bit of collateral damage, but the goal is to remove the "virStorage*" and replace with the virSecretLookupType so that it's easier to to add new lookups that aren't necessarily storage pool related. Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 2 +- src/conf/secret_conf.h | 2 +- src/libxl/libxl_conf.c | 2 +- src/qemu/qemu_domain.c | 4 ++-- src/secret/secret_util.c | 18 +++++++++--------- src/secret/secret_util.h | 22 ++++++++++++++++++++-- src/storage/storage_backend_iscsi.c | 7 ++++--- src/storage/storage_backend_rbd.c | 3 ++- src/util/virstoragefile.c | 33 +++++++++++++++++---------------- src/util/virstoragefile.h | 17 +++-------------- tests/qemuargv2xmltest.c | 4 ++-- 11 files changed, 62 insertions(+), 52 deletions(-) diff --git a/cfg.mk b/cfg.mk index a7b7266..0529a4e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -780,7 +780,7 @@ mid_dirs=access|conf|cpu|locking|logging|network|node_device|rpc|security|storag sc_prohibit_cross_inclusion: @for dir in $(cross_dirs); do \ case $$dir in \ - util/) safe="util";; \ + util/) safe="(util|secret)";; \ access/ | conf/) safe="($$dir|conf|util)";; \ locking/) safe="($$dir|util|conf|rpc)";; \ cpu/| network/| node_device/| rpc/| security/| storage/) \ diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index ca1afec..4584403 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -35,7 +35,7 @@ struct _virSecretDef { bool isprivate; unsigned char uuid[VIR_UUID_BUFLEN]; char *description; /* May be NULL */ - int usage_type; + int usage_type; /* virSecretUsageType */ union { char *volume; /* May be NULL */ char *ceph; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index a64b4c1..b3f78f0 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1033,7 +1033,7 @@ libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) if (!(conn = virConnectOpen("xen:///system"))) goto cleanup; - if (virSecretGetSecretString(conn, src->auth, + if (virSecretGetSecretString(conn, &src->auth->secdef, VIR_SECRET_USAGE_TYPE_CEPH, &secret, &secretlen) < 0) goto cleanup; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c21465d..a871d3e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -829,7 +829,7 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD) secretType = VIR_SECRET_USAGE_TYPE_CEPH; - return virSecretGetSecretString(conn, authdef, secretType, + return virSecretGetSecretString(conn, &authdef->secdef, secretType, &secinfo->s.plain.secret, &secinfo->s.plain.secretlen); } @@ -902,7 +902,7 @@ qemuDomainSecretAESSetup(virConnectPtr conn, goto cleanup; /* Grab the unencoded secret */ - if (virSecretGetSecretString(conn, authdef, secretType, + if (virSecretGetSecretString(conn, &authdef->secdef, secretType, &secret, &secretlen) < 0) goto cleanup; diff --git a/src/secret/secret_util.c b/src/secret/secret_util.c index 5602401..7bfe635 100644 --- a/src/secret/secret_util.c +++ b/src/secret/secret_util.c @@ -36,12 +36,12 @@ VIR_LOG_INIT("secret.secret_util"); /* virSecretGetSecretString: * @conn: Pointer to the connection driver to make secret driver call - * @authdef: Pointer to the disk storage authentication - * @secretUsageType: Type of secret usage for authdef lookup + * @secdef: Pointer to a storage type def for uuid/usage lookup + * @secretUsageType: Type of secret usage for usage lookup * @secret: returned secret as a sized stream of unsigned chars * @secret_size: Return size of the secret - either raw text or base64 * - * Lookup the secret for the authdef usage type and return it as raw text. + * Lookup the secret for the usage type and return it as raw text. * It is up to the caller to encode the secret further. * * Returns 0 on success, -1 on failure. On success the memory in secret @@ -49,7 +49,7 @@ VIR_LOG_INIT("secret.secret_util"); */ int virSecretGetSecretString(virConnectPtr conn, - virStorageAuthDefPtr authdef, + virSecretLookupTypeDefPtr secdef, virSecretUsageType secretUsageType, uint8_t **secret, size_t *secret_size) @@ -57,14 +57,14 @@ virSecretGetSecretString(virConnectPtr conn, virSecretPtr sec = NULL; int ret = -1; - switch (authdef->secretType) { - case VIR_STORAGE_SECRET_TYPE_UUID: - sec = conn->secretDriver->secretLookupByUUID(conn, authdef->secret.uuid); + switch (secdef->type) { + case VIR_SECRET_LOOKUP_TYPE_UUID: + sec = conn->secretDriver->secretLookupByUUID(conn, secdef->u.uuid); break; - case VIR_STORAGE_SECRET_TYPE_USAGE: + case VIR_SECRET_LOOKUP_TYPE_USAGE: sec = conn->secretDriver->secretLookupByUsage(conn, secretUsageType, - authdef->secret.usage); + secdef->u.usage); break; } diff --git a/src/secret/secret_util.h b/src/secret/secret_util.h index a039662..18ac86a 100644 --- a/src/secret/secret_util.h +++ b/src/secret/secret_util.h @@ -23,10 +23,28 @@ # define __VIR_SECRET_H__ # include "internal.h" -# include "virstoragefile.h" + +typedef enum { + VIR_SECRET_LOOKUP_TYPE_NONE, + VIR_SECRET_LOOKUP_TYPE_UUID, + VIR_SECRET_LOOKUP_TYPE_USAGE, + + VIR_SECRET_LOOKUP_TYPE_LAST +} virSecretLookupType; + +typedef struct _virSecretLookupTypeDef virSecretLookupTypeDef; +typedef virSecretLookupTypeDef *virSecretLookupTypeDefPtr; +struct _virSecretLookupTypeDef { + int type; /* virSecretLookupType */ + union { + unsigned char uuid[VIR_UUID_BUFLEN]; + char *usage; + } u; + +}; int virSecretGetSecretString(virConnectPtr conn, - virStorageAuthDefPtr authdef, + virSecretLookupTypeDefPtr secdef, virSecretUsageType secretUsageType, uint8_t **ret_secret, size_t *ret_secret_size) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index a45c525..81872ea 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -285,8 +285,8 @@ virStorageBackendISCSISetAuth(const char *portal, if (!authdef || authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE) return 0; - VIR_DEBUG("username='%s' authType=%d secretType=%d", - authdef->username, authdef->authType, authdef->secretType); + VIR_DEBUG("username='%s' authType=%d secdef.type=%d", + authdef->username, authdef->authType, authdef->secdef.type); if (authdef->authType != VIR_STORAGE_AUTH_TYPE_CHAP) { virReportError(VIR_ERR_XML_ERROR, "%s", _("iscsi pool only supports 'chap' auth type")); @@ -300,7 +300,8 @@ virStorageBackendISCSISetAuth(const char *portal, return -1; } - if (virSecretGetSecretString(conn, authdef, VIR_SECRET_USAGE_TYPE_ISCSI, + if (virSecretGetSecretString(conn, &authdef->secdef, + VIR_SECRET_USAGE_TYPE_ISCSI, &secret_value, &secret_size) < 0) goto cleanup; diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 64ec545..bf77c54 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -85,7 +85,8 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, return -1; } - if (virSecretGetSecretString(conn, authdef, VIR_SECRET_USAGE_TYPE_CEPH, + if (virSecretGetSecretString(conn, &authdef->secdef, + VIR_SECRET_USAGE_TYPE_CEPH, &secret_value, &secret_value_size) < 0) goto cleanup; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d2da9e7..54940a0 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1506,8 +1506,8 @@ virStorageAuthDefFree(virStorageAuthDefPtr authdef) VIR_FREE(authdef->username); VIR_FREE(authdef->secrettype); - if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_USAGE) - VIR_FREE(authdef->secret.usage); + if (authdef->secdef.type == VIR_SECRET_LOOKUP_TYPE_USAGE) + VIR_FREE(authdef->secdef.u.usage); VIR_FREE(authdef); } @@ -1526,11 +1526,12 @@ virStorageAuthDefCopy(const virStorageAuthDef *src) 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) + ret->secdef.type = src->secdef.type; + if (ret->secdef.type == VIR_SECRET_LOOKUP_TYPE_UUID) { + memcpy(ret->secdef.u.uuid, src->secdef.u.uuid, + sizeof(ret->secdef.u.uuid)); + } else if (ret->secdef.type == VIR_SECRET_LOOKUP_TYPE_USAGE) { + if (VIR_STRDUP(ret->secdef.u.usage, src->secdef.u.usage) < 0) goto error; } return ret; @@ -1573,16 +1574,16 @@ virStorageAuthDefParseSecret(xmlXPathContextPtr ctxt, } if (uuid) { - if (virUUIDParse(uuid, authdef->secret.uuid) < 0) { + if (virUUIDParse(uuid, authdef->secdef.u.uuid) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid auth secret uuid")); goto cleanup; } - authdef->secretType = VIR_STORAGE_SECRET_TYPE_UUID; + authdef->secdef.type = VIR_SECRET_LOOKUP_TYPE_UUID; } else { - authdef->secret.usage = usage; + authdef->secdef.u.usage = usage; usage = NULL; - authdef->secretType = VIR_STORAGE_SECRET_TYPE_USAGE; + authdef->secdef.type = VIR_SECRET_LOOKUP_TYPE_USAGE; } ret = 0; @@ -1625,7 +1626,7 @@ virStorageAuthDefParseXML(xmlXPathContextPtr ctxt) VIR_FREE(authtype); } - authdef->secretType = VIR_STORAGE_SECRET_TYPE_NONE; + authdef->secdef.type = VIR_SECRET_LOOKUP_TYPE_NONE; if (virStorageAuthDefParseSecret(ctxt, authdef) < 0) goto error; @@ -1680,12 +1681,12 @@ virStorageAuthDefFormat(virBufferPtr buf, else virBufferAddLit(buf, "<secret"); - if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) { - virUUIDFormat(authdef->secret.uuid, uuidstr); + if (authdef->secdef.type == VIR_SECRET_LOOKUP_TYPE_UUID) { + virUUIDFormat(authdef->secdef.u.uuid, uuidstr); virBufferAsprintf(buf, " uuid='%s'/>\n", uuidstr); - } else if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_USAGE) { + } else if (authdef->secdef.type == VIR_SECRET_LOOKUP_TYPE_USAGE) { virBufferEscapeString(buf, " usage='%s'/>\n", - authdef->secret.usage); + authdef->secdef.u.usage); } else { virBufferAddLit(buf, "/>\n"); } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b88e715..b4ad42e 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -1,7 +1,7 @@ /* * virstoragefile.h: file utility functions for FS storage backend * - * Copyright (C) 2007-2009, 2012-2014 Red Hat, Inc. + * Copyright (C) 2007-2009, 2012-2016 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -28,6 +28,7 @@ # include "virseclabel.h" # include "virstorageencryption.h" # include "virutil.h" +# include "secret/secret_util.h" /* Minimum header size required to probe all known formats with * virStorageFileProbeFormat, or obtain metadata from a known format. @@ -201,25 +202,13 @@ typedef enum { } virStorageAuthType; VIR_ENUM_DECL(virStorageAuth) -typedef enum { - VIR_STORAGE_SECRET_TYPE_NONE, - VIR_STORAGE_SECRET_TYPE_UUID, - VIR_STORAGE_SECRET_TYPE_USAGE, - - 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; + virSecretLookupTypeDef secdef; }; typedef struct _virStorageDriverData virStorageDriverData; diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 0fe6b9b..212e6e8 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -36,8 +36,8 @@ static int testSanitizeDef(virDomainDefPtr vmdef) virDomainDiskDefPtr disk = vmdef->disks[i]; if (disk->src->auth) { - disk->src->auth->secretType = VIR_STORAGE_SECRET_TYPE_USAGE; - if (VIR_STRDUP(disk->src->auth->secret.usage, + disk->src->auth->secdef.type = VIR_SECRET_LOOKUP_TYPE_USAGE; + if (VIR_STRDUP(disk->src->auth->secdef.u.usage, "qemuargv2xml_usage") < 0) goto fail; } -- 2.5.5
On Fri, Jun 03, 2016 at 06:52:53 -0400, John Ferlan wrote:
Move the enum into secret_util, rename it to be just virSecretLookupType. This includes quite a bit of collateral damage, but the goal is to remove the "virStorage*" and replace with the virSecretLookupType so that it's easier to to add new lookups that aren't necessarily storage pool related.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 2 +- src/conf/secret_conf.h | 2 +- src/libxl/libxl_conf.c | 2 +- src/qemu/qemu_domain.c | 4 ++-- src/secret/secret_util.c | 18 +++++++++--------- src/secret/secret_util.h | 22 ++++++++++++++++++++-- src/storage/storage_backend_iscsi.c | 7 ++++--- src/storage/storage_backend_rbd.c | 3 ++- src/util/virstoragefile.c | 33 +++++++++++++++++---------------- src/util/virstoragefile.h | 17 +++-------------- tests/qemuargv2xmltest.c | 4 ++-- 11 files changed, 62 insertions(+), 52 deletions(-)
diff --git a/cfg.mk b/cfg.mk index a7b7266..0529a4e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -780,7 +780,7 @@ mid_dirs=access|conf|cpu|locking|logging|network|node_device|rpc|security|storag sc_prohibit_cross_inclusion: @for dir in $(cross_dirs); do \ case $$dir in \ - util/) safe="util";; \ + util/) safe="(util|secret)";; \
I don't think this is a good idea. utils are used in many places that don't link to the secret driver. While this is now used just to pull in one data type, the check is meant to prevent problems with linking the file if certain modules are disabled. Peter
On 06/06/2016 03:27 AM, Peter Krempa wrote:
On Fri, Jun 03, 2016 at 06:52:53 -0400, John Ferlan wrote:
Move the enum into secret_util, rename it to be just virSecretLookupType. This includes quite a bit of collateral damage, but the goal is to remove the "virStorage*" and replace with the virSecretLookupType so that it's easier to to add new lookups that aren't necessarily storage pool related.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 2 +- src/conf/secret_conf.h | 2 +- src/libxl/libxl_conf.c | 2 +- src/qemu/qemu_domain.c | 4 ++-- src/secret/secret_util.c | 18 +++++++++--------- src/secret/secret_util.h | 22 ++++++++++++++++++++-- src/storage/storage_backend_iscsi.c | 7 ++++--- src/storage/storage_backend_rbd.c | 3 ++- src/util/virstoragefile.c | 33 +++++++++++++++++---------------- src/util/virstoragefile.h | 17 +++-------------- tests/qemuargv2xmltest.c | 4 ++-- 11 files changed, 62 insertions(+), 52 deletions(-)
diff --git a/cfg.mk b/cfg.mk index a7b7266..0529a4e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -780,7 +780,7 @@ mid_dirs=access|conf|cpu|locking|logging|network|node_device|rpc|security|storag sc_prohibit_cross_inclusion: @for dir in $(cross_dirs); do \ case $$dir in \ - util/) safe="util";; \ + util/) safe="(util|secret)";; \
I don't think this is a good idea. utils are used in many places that don't link to the secret driver. While this is now used just to pull in one data type, the check is meant to prevent problems with linking the file if certain modules are disabled.
Understood, but it was a far less invasive option than chasing the virstoragefile.h includes to then modify many more places in order to get the definition. I would think that definition belongs with secret_util. It's how the lookup of uuid/usage for virSecretGetSecretString works and has less to do with something storage specific. If there's another way to make some adjustment to that particular syntax-check, that's fine, but this is what I found that worked. Those checks aren't necessarily very readable to my eyes. I'm not against keeping it in virstoragefile.h if that's the preference. Is the name change OK or would you prefer to see virStorageLookupTypeDef. It's always been a bit confusing when to see "secretType" in the code when it really was the lookup type not the "type" from the <secret...> object. Taking things another step further - eventually secrets will be used to support TLS objects - having a structure with virStorageLookupTypeDef wouldn't seem to be appropriate. Untying the existing relationship between the secret driver and the storage driver (as well as qemu and libxl) without "falling back to" libvirt API calls to virSecret* is going to require some amount of cross-pollination. It's a matter of choice. John
participants (2)
-
John Ferlan -
Peter Krempa