[libvirt] [PATCH 0/6] qemu: command: Clean up formatting of some attributes (blockdev-add saga)
'file.password-secret' and 'file.debug' were improperly added to a random place when formatting -drive. This would not play well when the same attributes should be formatted via the JSON generator. Clean up the mess. Peter Krempa (6): qemu: command: Inject password-secret only when not using JSON props util: storage: Add fields for debug options for disk drivers qemu: block: Add support for formatting gluster debug level via JSON qemu: hotplug: Rename qemuDomainPrepareDisk to qemuHotplugPrepareDiskAccess qemu: domain: Unify disk source prepare steps qemu: command: Properly format disk 'debug' attribute src/qemu/qemu_block.c | 17 +++++++++++++++-- src/qemu/qemu_command.c | 34 ++++++++++------------------------ src/qemu/qemu_command.h | 1 - src/qemu/qemu_domain.c | 27 +++++++++++++++++++++++++-- src/qemu/qemu_domain.h | 16 ++++++---------- src/qemu/qemu_hotplug.c | 29 +++++++++++++---------------- src/qemu/qemu_process.c | 5 +---- src/util/virstoragefile.c | 2 ++ src/util/virstoragefile.h | 3 +++ 9 files changed, 75 insertions(+), 59 deletions(-) -- 2.14.3
The 'file.password-secret' injection should be used only if we are using the old formatter. When formatting the source string from the JSON properities, the property should be added there. Also drop the comment which refers to stuff that will not be used in libvirt sine -blockdev is the way to go. --- src/qemu/qemu_command.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 26d395d67c..667ac746e1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1578,6 +1578,9 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, } virQEMUBuildBufferEscapeComma(buf, source); + + if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) + virBufferAsprintf(buf, ",file.password-secret=%s", secinfo->s.aes.alias); } else { if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops))) goto cleanup; @@ -1592,16 +1595,6 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, virBufferAsprintf(buf, "file.debug=%d,", cfg->glusterDebugLevel); } - if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { - /* NB: If libvirt starts using the more modern option based - * syntax to build the command line (e.g., "-drive driver=rbd, - * filename=%s,...") instead of the legacy model (e.g."-drive - * file=%s,..."), then the "file." prefix can be removed - */ - virBufferAsprintf(buf, "file.password-secret=%s,", - secinfo->s.aes.alias); - } - if (encinfo) virQEMUBuildLuksOpts(buf, &disk->src->encryption->encinfo, encinfo->s.aes.alias); -- 2.14.3
On Thu, Nov 23, 2017 at 05:24:38PM +0100, Peter Krempa wrote:
The 'file.password-secret' injection should be used only if we are using the old formatter. When formatting the source string from the JSON properities, the property should be added there.
*properties
Also drop the comment which refers to stuff that will not be used in libvirt sine -blockdev is the way to go.
*since Jan
--- src/qemu/qemu_command.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
Some drive backends allow output of debugging information which can be configured using properities of the image. Add fields to virStorageSource which will allow carrying them. --- src/util/virstoragefile.c | 2 ++ src/util/virstoragefile.h | 3 +++ 2 files changed, 5 insertions(+) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 45a6dea8a0..6594715e5e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2061,6 +2061,8 @@ virStorageSourceCopy(const virStorageSource *src, ret->tlsFromConfig = src->tlsFromConfig; ret->tlsVerify = src->tlsVerify; ret->detected = src->detected; + ret->debugLevel = src->debugLevel; + ret->debug = src->debug; /* storage driver metadata are not copied */ ret->drv = NULL; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index af8f56c8a1..24382a0a6b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -303,6 +303,9 @@ struct _virStorageSource { bool tlsVerify; bool detected; /* true if this entry was not provided by the user */ + + unsigned int debugLevel; + bool debug; }; -- 2.14.3
On Thu, Nov 23, 2017 at 05:24:39PM +0100, Peter Krempa wrote:
Some drive backends allow output of debugging information which can be configured using properities of the image. Add fields to
*properties Jan
virStorageSource which will allow carrying them. --- src/util/virstoragefile.c | 2 ++ src/util/virstoragefile.h | 3 +++ 2 files changed, 5 insertions(+)
Improve the formatter so that we can use the 'debug' property straight away when using json. --- src/qemu/qemu_block.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 8b23df8227..3a48f60c00 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -639,6 +639,7 @@ static virJSONValuePtr qemuBlockStorageSourceGetGlusterProps(virStorageSourcePtr src) { virJSONValuePtr servers = NULL; + virJSONValuePtr props = NULL; virJSONValuePtr ret = NULL; if (!(servers = qemuBlockStorageSourceBuildHostsJSONSocketAddress(src, true))) @@ -650,12 +651,24 @@ qemuBlockStorageSourceGetGlusterProps(virStorageSourcePtr src) * server :[{type:"tcp", host:"1.2.3.4", port:24007}, * {type:"unix", socket:"/tmp/glusterd.socket"}, ...]} */ - if (virJSONValueObjectCreate(&ret, + if (virJSONValueObjectCreate(&props, "s:driver", "gluster", "s:volume", src->volume, "s:path", src->path, "a:server", servers, NULL) < 0) - virJSONValueFree(servers); + goto cleanup; + + servers = NULL; + + if (src->debug && + virJSONValueObjectAdd(props, "u:debug", src->debugLevel, NULL) < 0) + goto cleanup; + + VIR_STEAL_PTR(ret, props); + + cleanup: + virJSONValueFree(servers); + virJSONValueFree(props); return ret; } -- 2.14.3
Match the prefix of the file and choose a name which better describes what happens. --- src/qemu/qemu_hotplug.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 72a57d89ed..a49480ecb4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -70,7 +70,7 @@ unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5; /** - * qemuDomainPrepareDisk: + * qemuHotplugPrepareDiskAccess: * @driver: qemu driver struct * @vm: domain object * @disk: disk to prepare @@ -85,11 +85,11 @@ unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5; * Returns 0 on success and -1 on error. Reports libvirt error. */ static int -qemuDomainPrepareDisk(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk, - virStorageSourcePtr overridesrc, - bool teardown) +qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + virStorageSourcePtr overridesrc, + bool teardown) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; @@ -280,7 +280,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, goto cleanup; } - if (qemuDomainPrepareDisk(driver, vm, disk, newsrc, false) < 0) + if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0) goto cleanup; if (!(driveAlias = qemuAliasFromDisk(disk))) @@ -331,7 +331,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, /* remove the old source from shared device list */ ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); - ignore_value(qemuDomainPrepareDisk(driver, vm, disk, NULL, true)); + ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true)); virStorageSourceFree(disk->src); disk->src = newsrc; @@ -345,7 +345,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, error: virDomainAuditDisk(vm, disk->src, newsrc, "update", false); - ignore_value(qemuDomainPrepareDisk(driver, vm, disk, newsrc, true)); + ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, true)); goto cleanup; } @@ -378,7 +378,7 @@ qemuDomainAttachDiskGeneric(virConnectPtr conn, qemuDomainSecretInfoPtr secinfo = NULL; qemuDomainSecretInfoPtr encinfo = NULL; - if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) + if (qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, false) < 0) goto cleanup; if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) @@ -486,7 +486,7 @@ qemuDomainAttachDiskGeneric(virConnectPtr conn, error: qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src); - ignore_value(qemuDomainPrepareDisk(driver, vm, disk, NULL, true)); + ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true)); goto cleanup; } -- 2.14.3
Aggregate setup of various aspects of a disk source (secrets, TLS, ...) into one function so that we don't need to call multiple across the code base. --- src/qemu/qemu_domain.c | 20 ++++++++++++++++++-- src/qemu/qemu_domain.h | 16 ++++++---------- src/qemu/qemu_hotplug.c | 5 +---- src/qemu/qemu_process.c | 5 +---- 4 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f12450cc69..a5bfcfb48a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1449,7 +1449,7 @@ qemuDomainSecretStorageSourcePrepare(virConnectPtr conn, * Returns 0 on success, -1 on failure */ -int +static int qemuDomainSecretDiskPrepare(virConnectPtr conn, qemuDomainObjPrivatePtr priv, virDomainDiskDefPtr disk) @@ -7927,7 +7927,7 @@ qemuDomainPrepareChardevSource(virDomainDefPtr def, * * Returns 0 on success, -1 on bad config/failure */ -int +static int qemuDomainPrepareDiskSourceTLS(virStorageSourcePtr src, virQEMUDriverConfigPtr cfg) { @@ -10425,3 +10425,19 @@ qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver, virStringListFree(caps); return ret; } + + +int +qemuDomainPrepareDiskSource(virConnectPtr conn, + virDomainDiskDefPtr disk, + qemuDomainObjPrivatePtr priv, + virQEMUDriverConfigPtr cfg) +{ + if (qemuDomainPrepareDiskSourceTLS(disk->src, cfg) < 0) + return -1; + + if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) + return -1; + + return 0; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 9066f5d0f5..146b27dd57 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -839,11 +839,6 @@ qemuDomainSecretInfoTLSNew(virConnectPtr conn, const char *srcAlias, const char *secretUUID); -int qemuDomainSecretDiskPrepare(virConnectPtr conn, - qemuDomainObjPrivatePtr priv, - virDomainDiskDefPtr disk) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); - void qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr disk) ATTRIBUTE_NONNULL(1); @@ -886,11 +881,6 @@ void qemuDomainPrepareChardevSource(virDomainDefPtr def, virQEMUDriverConfigPtr cfg) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -int -qemuDomainPrepareDiskSourceTLS(virStorageSourcePtr src, - virQEMUDriverConfigPtr cfg) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); - int qemuDomainPrepareShmemChardev(virDomainShmemDefPtr shmem) ATTRIBUTE_NONNULL(1); @@ -1011,4 +1001,10 @@ qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob); +int +qemuDomainPrepareDiskSource(virConnectPtr conn, + virDomainDiskDefPtr disk, + qemuDomainObjPrivatePtr priv, + virQEMUDriverConfigPtr cfg); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a49480ecb4..44d48ca95a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -384,7 +384,7 @@ qemuDomainAttachDiskGeneric(virConnectPtr conn, if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; - if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) + if (qemuDomainPrepareDiskSource(conn, disk, priv, cfg) < 0) goto error; srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); @@ -401,9 +401,6 @@ qemuDomainAttachDiskGeneric(virConnectPtr conn, if (encinfo && qemuBuildSecretInfoProps(encinfo, &encobjProps) < 0) goto error; - if (qemuDomainPrepareDiskSourceTLS(disk->src, cfg) < 0) - goto error; - if (disk->src->haveTLS && qemuDomainAddDiskSrcTLSObject(driver, vm, disk->src, disk->info.alias) < 0) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6d242b1b51..d47c6d69be 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5362,10 +5362,7 @@ qemuProcessPrepareDomainStorage(virConnectPtr conn, continue; } - if (qemuDomainPrepareDiskSourceTLS(disk->src, cfg) < 0) - return -1; - - if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) + if (qemuDomainPrepareDiskSource(conn, disk, priv, cfg) < 0) return -1; } -- 2.14.3
Move the setup of the disk attribute to the disk source prepare function which will allow proper usage with JSON props and move the fallback (legacy) generating code into the block which is executed with legacy options. As a side-effect of this change we can clean up propagation of 'cfg' into the command generator. Also it's nice to see that the test output is the same even when the value is generated in a different place. --- src/qemu/qemu_command.c | 21 +++++++-------------- src/qemu/qemu_command.h | 1 - src/qemu/qemu_domain.c | 7 +++++++ src/qemu/qemu_hotplug.c | 2 +- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 667ac746e1..26f86c74b5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1509,9 +1509,7 @@ qemuDiskSourceGetProps(virStorageSourcePtr src) static int qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, - virQEMUDriverConfigPtr cfg, - virBufferPtr buf, - virQEMUCapsPtr qemuCaps) + virBufferPtr buf) { int actualType = virStorageSourceGetActualType(disk->src); qemuDomainStorageSourcePrivatePtr srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); @@ -1581,6 +1579,9 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) virBufferAsprintf(buf, ",file.password-secret=%s", secinfo->s.aes.alias); + + if (disk->src->debug) + virBufferAsprintf(buf, ",file.debug=%d", disk->src->debugLevel); } else { if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops))) goto cleanup; @@ -1589,12 +1590,6 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, } virBufferAddLit(buf, ","); - if (disk->src->type == VIR_STORAGE_TYPE_NETWORK && - disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_GLUSTER_DEBUG_LEVEL)) - virBufferAsprintf(buf, "file.debug=%d,", cfg->glusterDebugLevel); - } - if (encinfo) virQEMUBuildLuksOpts(buf, &disk->src->encryption->encinfo, encinfo->s.aes.alias); @@ -1722,13 +1717,12 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk, char * qemuBuildDriveStr(virDomainDiskDefPtr disk, - virQEMUDriverConfigPtr cfg, bool bootable, virQEMUCapsPtr qemuCaps) { virBuffer opt = VIR_BUFFER_INITIALIZER; - if (qemuBuildDriveSourceStr(disk, cfg, &opt, qemuCaps) < 0) + if (qemuBuildDriveSourceStr(disk, &opt) < 0) goto error; if (qemuDiskBusNeedsDeviceArg(disk->bus)) { @@ -2209,7 +2203,6 @@ qemuBuildDriveDevStr(const virDomainDef *def, static int qemuBuildDiskDriveCommandLine(virCommandPtr cmd, - virQEMUDriverConfigPtr cfg, const virDomainDef *def, virQEMUCapsPtr qemuCaps) { @@ -2289,7 +2282,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, virCommandAddArg(cmd, "-drive"); - if (!(optstr = qemuBuildDriveStr(disk, cfg, driveBoot, qemuCaps))) + if (!(optstr = qemuBuildDriveStr(disk, driveBoot, qemuCaps))) return -1; virCommandAddArg(cmd, optstr); @@ -10147,7 +10140,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildHubCommandLine(cmd, def, qemuCaps) < 0) goto error; - if (qemuBuildDiskDriveCommandLine(cmd, cfg, def, qemuCaps) < 0) + if (qemuBuildDiskDriveCommandLine(cmd, def, qemuCaps) < 0) goto error; if (qemuBuildFSDevCommandLine(cmd, def, qemuCaps) < 0) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 2bcfc6c707..18e0894581 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -101,7 +101,6 @@ char *qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk); /* Both legacy & current support */ char *qemuBuildDriveStr(virDomainDiskDefPtr disk, - virQEMUDriverConfigPtr cfg, bool bootable, virQEMUCapsPtr qemuCaps); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a5bfcfb48a..75482ac5e6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10439,5 +10439,12 @@ qemuDomainPrepareDiskSource(virConnectPtr conn, if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) return -1; + if (disk->src->type == VIR_STORAGE_TYPE_NETWORK && + disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_GLUSTER_DEBUG_LEVEL)) { + disk->src->debug = true; + disk->src->debugLevel = cfg->glusterDebugLevel; + } + return 0; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 44d48ca95a..a1a088af4b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -406,7 +406,7 @@ qemuDomainAttachDiskGeneric(virConnectPtr conn, disk->info.alias) < 0) goto error; - if (!(drivestr = qemuBuildDriveStr(disk, cfg, false, priv->qemuCaps))) + if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) goto error; if (!(drivealias = qemuAliasFromDisk(disk))) -- 2.14.3
On Thu, Nov 23, 2017 at 05:24:37PM +0100, Peter Krempa wrote:
'file.password-secret' and 'file.debug' were improperly added to a random place when formatting -drive. This would not play well when the same attributes should be formatted via the JSON generator.
Clean up the mess.
Peter Krempa (6): qemu: command: Inject password-secret only when not using JSON props util: storage: Add fields for debug options for disk drivers qemu: block: Add support for formatting gluster debug level via JSON qemu: hotplug: Rename qemuDomainPrepareDisk to qemuHotplugPrepareDiskAccess qemu: domain: Unify disk source prepare steps qemu: command: Properly format disk 'debug' attribute
ACK series Jan
participants (2)
-
Ján Tomko -
Peter Krempa