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(a)redhat.com&gt; --- 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

Rather than inline code secret lookup for rbd/iscsi, use the common function. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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 06/06/2016 03:32 AM, Peter Krempa wrote: 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

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(a)redhat.com&gt; --- 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 06/06/2016 03:27 AM, Peter Krempa wrote: 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