On 04/16/2016 10:17 AM, John Ferlan wrote:
> Rather than needing to pass the conn parameter to various command
> line building API's, add qemuDomainSecretPrepare just prior to the
> qemuProcessLaunch which calls qemuBuilCommandLine. The function
> must be called after qemuProcessPrepareHost since it's expected
> to eventually need the domain masterKey generated during the prepare
> host call. Additionally, future patches may require device aliases
> (assigned during the prepare domain call) in order to associate
> the secret objects.
>
> The qemuDomainSecretDestroy is called after the qemuProcessLaunch
> finishes in order to clear and free memory used by the secrets
> that were recently prepared, so they are not kept around in memory
> too long.
>
> Placing the setup here is beneficial for future patches which will
> need the domain masterKey in order to generate an encrypted secret
> along with an initialization vector to be saved and passed (since
> the masterKey shouldn't be passed around).
>
> Finally, since the secret is not added during command line build,
> the hotplug code will need to get the secret into the private disk data.
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
> src/qemu/qemu_command.c | 45 ++++-----------
> src/qemu/qemu_command.h | 5 +-
> src/qemu/qemu_domain.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++--
> src/qemu/qemu_domain.h | 15 ++++-
> src/qemu/qemu_driver.c | 10 ++--
> src/qemu/qemu_hotplug.c | 26 +++++----
> src/qemu/qemu_hotplug.h | 1 -
> src/qemu/qemu_process.c | 8 +++
> 8 files changed, 202 insertions(+), 58 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 26c19ff..24ed8ed 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -832,7 +832,7 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,
>
> int
> qemuGetDriveSourceString(virStorageSourcePtr src,
> - virConnectPtr conn,
> + qemuDomainSecretInfoPtr secinfo,
> char **source)
> {
> int actualType = virStorageSourceGetActualType(src);
> @@ -846,31 +846,6 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
> if (virStorageSourceIsEmpty(src))
> return 1;
>
> - if (conn) {
> - if (actualType == VIR_STORAGE_TYPE_NETWORK &&
> - 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;
> -
> - if (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
> - /* qemu requires the secret to be encoded for RBD */
> - encode = true;
> - secretType = VIR_SECRET_USAGE_TYPE_CEPH;
> - }
> -
> - if (!(secret = virSecretGetSecretString(conn,
> - protocol,
> - encode,
> - src->auth,
> - secretType)))
> - goto cleanup;
> - }
> - }
> -
> switch ((virStorageType) actualType) {
> case VIR_STORAGE_TYPE_BLOCK:
> case VIR_STORAGE_TYPE_FILE:
> @@ -881,6 +856,11 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
> break;
>
> case VIR_STORAGE_TYPE_NETWORK:
> + if (secinfo) {
> + username = secinfo->s.plain.username;
> + secret = secinfo->s.plain.secret;
> + }
> +
> if (!(*source = qemuBuildNetworkDriveURI(src, username, secret)))
> goto cleanup;
> break;
> @@ -894,7 +874,6 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
> ret = 0;
>
> cleanup:
> - VIR_FREE(secret);
> return ret;
> }
>
> @@ -1033,8 +1012,7 @@ qemuCheckFips(void)
>
>
> char *
> -qemuBuildDriveStr(virConnectPtr conn,
> - virDomainDiskDefPtr disk,
> +qemuBuildDriveStr(virDomainDiskDefPtr disk,
> bool bootable,
> virQEMUCapsPtr qemuCaps)
> {
> @@ -1046,6 +1024,7 @@ qemuBuildDriveStr(virConnectPtr conn,
> int busid = -1, unitid = -1;
> char *source = NULL;
> int actualType = virStorageSourceGetActualType(disk->src);
> + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>
> if (idx < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -1127,7 +1106,7 @@ qemuBuildDriveStr(virConnectPtr conn,
> break;
> }
>
> - if (qemuGetDriveSourceString(disk->src, conn, &source) < 0)
> + if (qemuGetDriveSourceString(disk->src, diskPriv->secinfo, &source) < 0)
> goto error;
>
> if (source &&
> @@ -1816,7 +1795,6 @@ qemuBuildDriveDevStr(const virDomainDef *def,
>
> static int
> qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
> - virConnectPtr conn,
> const virDomainDef *def,
> virQEMUCapsPtr qemuCaps,
> bool emitBootindex)
> @@ -1910,7 +1888,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
> deviceFlagMasked = true;
> }
> }
> - optstr = qemuBuildDriveStr(conn, disk,
> + optstr = qemuBuildDriveStr(disk,
> emitBootindex ? false : !!bootindex,
> qemuCaps);
> if (deviceFlagMasked)
> @@ -9363,8 +9341,7 @@ qemuBuildCommandLine(virConnectPtr conn,
> if (qemuBuildHubCommandLine(cmd, def, qemuCaps) < 0)
> goto error;
>
> - if (qemuBuildDiskDriveCommandLine(cmd, conn, def, qemuCaps,
> - emitBootindex) < 0)
> + if (qemuBuildDiskDriveCommandLine(cmd, def, qemuCaps, emitBootindex) < 0)
> goto error;
>
> if (qemuBuildFSDevCommandLine(cmd, def, qemuCaps) < 0)
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 2e2f133..2662d9b 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -99,8 +99,7 @@ char *qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk,
> virQEMUCapsPtr qemuCaps);
>
> /* Both legacy & current support */
> -char *qemuBuildDriveStr(virConnectPtr conn,
> - virDomainDiskDefPtr disk,
> +char *qemuBuildDriveStr(virDomainDiskDefPtr disk,
> bool bootable,
> virQEMUCapsPtr qemuCaps);
>
> @@ -177,7 +176,7 @@ char *qemuBuildRedirdevDevStr(const virDomainDef *def,
> int qemuNetworkPrepareDevices(virDomainDefPtr def);
>
> int qemuGetDriveSourceString(virStorageSourcePtr src,
> - virConnectPtr conn,
> + qemuDomainSecretInfoPtr secinfo,
> char **source);
>
> int qemuCheckDiskConfig(virDomainDiskDefPtr disk);
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 55cb30f..93033d9 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -46,6 +46,7 @@
> #include "viratomic.h"
> #include "virprocess.h"
> #include "virrandom.h"
> +#include "secret_util.h"
> #include "base64.h"
> #ifdef WITH_GNUTLS
> # include <gnutls/gnutls.h>
> @@ -791,6 +792,146 @@ qemuDomainDiskPrivateDispose(void *obj)
> }
>
>
> +/* qemuDomainSecretPlainSetup:
> + * @conn: Pointer to connection
> + * @secinfo: Pointer to secret info
> + * @protocol: Protocol for secret
> + * @authdef: Pointer to auth data
> + *
> + * Taking a secinfo, fill in the plaintext information
> + *
> + * Returns 0 on success, -1 on failure with error message
> + */
> +static int
> +qemuDomainSecretPlainSetup(virConnectPtr conn,
> + qemuDomainSecretInfoPtr secinfo,
> + virStorageNetProtocol protocol,
> + virStorageAuthDefPtr authdef)
> +{
> + bool encode = false;
> + int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
> + const char *protocolstr = virStorageNetProtocolTypeToString(protocol);
> +
> + secinfo->type = VIR_DOMAIN_SECRET_INFO_PLAIN;
> + if (VIR_STRDUP(secinfo->s.plain.username, authdef->username) < 0)
> + return -1;
> +
> + if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
> + /* qemu requires the secret to be encoded for RBD */
> + encode = true;
> + secretType = VIR_SECRET_USAGE_TYPE_CEPH;
> + }
> +
> + if (!(secinfo->s.plain.secret =
> + virSecretGetSecretString(conn, protocolstr, encode,
> + authdef, secretType)))
> + return -1;
> +
> + return 0;
> +}
> +
> +
> +/* qemuDomainSecretDiskDestroy:
> + * @disk: Pointer to a disk definition
> + *
> + * Clear and destroy memory associated with the secret
> + */
> +void
> +qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk)
> +{
> + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> +
> + if (!diskPriv->secinfo)
> + return;
> +
> + qemuDomainSecretInfoFree(&diskPriv->secinfo);
> +}
> +
> +
> +/* qemuDomainSecretDiskPrepare:
> + * @conn: Pointer to connection
> + * @disk: Pointer to a disk definition
> + *
> + * For the right disk, generate the qemuDomainSecretInfo structure.
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +int
> +qemuDomainSecretDiskPrepare(virConnectPtr conn,
> + virDomainDiskDefPtr disk)
> +{
> + virStorageSourcePtr src = disk->src;
> + qemuDomainSecretInfoPtr secinfo = NULL;
> +
> + if (conn && !virStorageSourceIsEmpty(src) &&
> + virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK &&
> + src->auth &&
> + (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI ||
> + src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
> +
Is conn ever going to be NULL here?
> + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> +
> + if (VIR_ALLOC(secinfo) < 0)
> + return -1;
> +
> + if (qemuDomainSecretPlainSetup(conn, secinfo, src->protocol,
> + src->auth) < 0)
> + goto error;
> +
> + diskPriv->secinfo = secinfo;
> + }
> +
> + return 0;
> +
> + error:
> + qemuDomainSecretInfoFree(&secinfo);
> + return -1;
> +}
> +
> +
> +/* qemuDomainSecretDestroy:
> + * @vm: Domain object
> + *
> + * Once completed with the generation of the command line it is
> + * expect to remove the secrets
> + */
> +void
> +qemuDomainSecretDestroy(virDomainObjPtr vm)
> +{
> + size_t i;
> +
> + for (i = 0; i < vm->def->ndisks; i++)
> + qemuDomainSecretDiskDestroy(vm->def->disks[i]);
> +}
> +
> +
> +/* qemuDomainSecretPrepare:
> + * @conn: Pointer to connection
> + * @vm: Domain object
> + *
> + * For any objects that may require an auth/secret setup, create a
> + * qemuDomainSecretInfo and save it in the approriate place within
> + * the private structures. This will be used by command line build
> + * code in order to pass the secret along to qemu in order to provide
> + * the necessary authentication data.
> + *
> + * Returns 0 on success, -1 on failure with error message set
> + */
> +int
> +qemuDomainSecretPrepare(virConnectPtr conn,
> + virDomainObjPtr vm)
> +{
> + size_t i;
> +
> + for (i = 0; i < vm->def->ndisks; i++) {
> + if (qemuDomainSecretDiskPrepare(conn, vm->def->disks[i]) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +
> /* This is the old way of setting up per-domain directories */
> static int
> qemuDomainSetPrivatePathsOld(virQEMUDriverPtr driver,
> @@ -3782,8 +3923,7 @@ qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver,
>
>
> bool
> -qemuDomainDiskSourceDiffers(virConnectPtr conn,
> - virDomainDiskDefPtr disk,
> +qemuDomainDiskSourceDiffers(virDomainDiskDefPtr disk,
> virDomainDiskDefPtr origDisk)
> {
> char *diskSrc = NULL, *origDiskSrc = NULL;
> @@ -3799,8 +3939,10 @@ qemuDomainDiskSourceDiffers(virConnectPtr conn,
> if (diskEmpty ^ origDiskEmpty)
> return true;
>
> - if (qemuGetDriveSourceString(disk->src, conn, &diskSrc) < 0 ||
> - qemuGetDriveSourceString(origDisk->src, conn, &origDiskSrc) < 0)
> + /* This won't be a network storage, so no need to get the diskPriv
> + * in order to fetch the secret, thus NULL for param2 */
> + if (qemuGetDriveSourceString(disk->src, NULL, &diskSrc) < 0 ||
> + qemuGetDriveSourceString(origDisk->src, NULL, &origDiskSrc) < 0)
> goto cleanup;
>
> /* So far in qemu disk sources are considered different
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 9cfe3e4..bde71a4 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -497,8 +497,7 @@ int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
> bool force_probe,
> bool report_broken);
>
> -bool qemuDomainDiskSourceDiffers(virConnectPtr conn,
> - virDomainDiskDefPtr disk,
> +bool qemuDomainDiskSourceDiffers(virDomainDiskDefPtr disk,
> virDomainDiskDefPtr origDisk);
>
> bool qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
> @@ -616,4 +615,16 @@ int qemuDomainMasterKeyCreate(virQEMUDriverPtr driver,
>
> void qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv);
>
> +void qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk)
> + ATTRIBUTE_NONNULL(1);
> +
> +int qemuDomainSecretDiskPrepare(virConnectPtr conn, virDomainDiskDefPtr disk)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +
> +void qemuDomainSecretDestroy(virDomainObjPtr vm)
> + ATTRIBUTE_NONNULL(1);
> +
> +int qemuDomainSecretPrepare(virConnectPtr conn, virDomainObjPtr vm)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +
> #endif /* __QEMU_DOMAIN_H__ */
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1ba8ab9..bb55b7d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7710,14 +7710,16 @@ qemuDomainChangeDiskLive(virConnectPtr conn,
>
> orig_disk->startupPolicy = dev->data.disk->startupPolicy;
>
> - if (qemuDomainDiskSourceDiffers(conn, disk, orig_disk)) {
> + if (qemuDomainDiskSourceDiffers(disk, orig_disk)) {
> /* Add the new disk src into shared disk hash table */
> if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0)
> goto cleanup;
>
> - if (qemuDomainChangeEjectableMedia(driver, conn, vm,
> - orig_disk, dev->data.disk->src, force) < 0) {
> - ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk, vm->def->name));
> + if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk,
> + dev->data.disk->src,
> + force) < 0) {
> + ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk,
> + vm->def->name));
> goto rollback;
> }
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index ef8696b..692e3e7 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -148,7 +148,6 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver,
> /**
> * qemuDomainChangeEjectableMedia:
> * @driver: qemu driver structure
> - * @conn: connection structure
> * @vm: domain definition
> * @disk: disk definition to change the source of
> * @newsrc: new disk source to change to
> @@ -163,7 +162,6 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver,
> */
> int
> qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> - virConnectPtr conn,
> virDomainObjPtr vm,
> virDomainDiskDefPtr disk,
> virStorageSourcePtr newsrc,
> @@ -223,7 +221,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> } while (rc < 0);
>
> if (!virStorageSourceIsEmpty(newsrc)) {
> - if (qemuGetDriveSourceString(newsrc, conn, &sourcestr) < 0)
> + /* cdrom/floppy won't have secret */
> + if (qemuGetDriveSourceString(newsrc, NULL, &sourcestr) < 0)
> goto error;
>
Why not? Can't you have an rbd backed cdrom device?
ACK otherwise
- Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list