[libvirt] [PATCH v2] storage: Need to set secrettype for direct iscsi disk volume

[John Ferlan]

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 at 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
>