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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

>                  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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]