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@xxxxxxxxxx> >> --- >> 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 > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list