On 06/15/2015 05:44 AM, Michal Privoznik wrote:

On 09.06.2015 18:03, John Ferlan wrote:

...

https://bugzilla.redhat.com/show_bug.cgi?id=1200206

Commit id '1b4eaa61' added the ability to have a mode='direct' for an iscsi disk volume. It relied on virStorageTranslateDiskSourcePool in order to copy any disk source pool authentication information to the direct disk volume, but it neglected to also copy the 'secrettype' field which ends up being used in the domain volume formatting code. Adding a secrettype for this case will allow for proper formatting later and allow disk snapshotting to work properly

Additionally libvirtd restart processing would fail to find the domain since the translation processing code is run after domain xml processing, so handle the the case where the authdef could have an empty secrettype field when processing the auth and additionally ignore performing the actual and expected auth secret type checks for a DISK_VOLUME since that data will be reassembled later during translation processing of the running domain.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- Changes since v1:

- Found that the libvirtd restart path caused issues - my initial attempt to fix made a bad assumption - that I was running with a domain started after the initial patch which would have the secrettype filled in by the translate code.

So this patch changes the domain parse code to better account for the chance that secrettype isn't yet provided in the XML by having the authdef processing code fill in the value.

Secondly since the pooltype was only filled during the translation on libvirtd restart and that occurs after domain xml process, the pooltype field would be empty - thus it couldn't be used for comparison. Since, translation processing will destroy the authdef read in at parse time, modify the actual and expected check to ignore the DISK_VOLUME case

src/conf/domain_conf.c | 16 +++++++++++++++- src/storage/storage_driver.c | 10 ++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 36de844..d621c01 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6571,6 +6571,16 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlStrEqual(cur->name, BAD_CAST "auth")) { if (!(authdef = virStorageAuthDefParse(node->doc, cur))) goto error; + /* Shared processing code with storage pools can leave + * this empty, but disk formatting uses it as does command + * creation - so use the secretType to attempt to fill it in. + */ + if (!authdef->secrettype) { + const char *secrettype = + virSecretUsageTypeToString(authdef->secretType); + if (VIR_STRDUP(authdef->secrettype, secrettype) < 0) + goto error; + }

So _virStorageAuthDef has both @secrettype and @secretType? That's rather perplexing.

Yeah - I think it was a "convenience" perhaps when trying to combine the various authdef functions that had existed. I look to generate a followup that removes the need for it Tks - John

...

if ((auth_secret_usage = virSecretUsageTypeFromString(authdef->secrettype)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -6790,7 +6800,11 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, cur = cur->next; }

- if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage) { + /* Disk volume types will have authentication information handled in + * virStorageTranslateDiskSourcePool + */ + if (def->src->type != VIR_STORAGE_TYPE_VOLUME && + auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid secret type '%s'"), virSecretUsageTypeToString(auth_secret_usage)); diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ab8675d..57060ab 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3310,6 +3310,16 @@ virStorageTranslateDiskSourcePool(virConnectPtr conn, &pooldef->source) < 0) goto cleanup;

+ /* Source pool may not fill in the secrettype field, + * so we need to do so here + */ + if (def->src->auth && !def->src->auth->secrettype) { + const char *secrettype = + virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_ISCSI); + if (VIR_STRDUP(def->src->auth->secrettype, secrettype) < 0) + goto cleanup; + } + if (virStorageAddISCSIPoolSourceHost(def, pooldef) < 0) goto cleanup; break;

But the patch is right. ACK.

Michal