[libvirt] [PATCH 07/11] storage: Support to use secret object for iscsi chap "auth"

[John Ferlan]

On 05/28/2013 02:39 AM, Osier Yang wrote:
> Based on the plain password chap "auth" support, this gets
> the secret value (password) with the secret driver methods,
> and apply it for the "iscsiadm" update command.

Ugh.  Of course - it's the "next" patch (gets me every time).

In any case, since 6/11 cannot "assume" the passwd field is filled in,
you probably need to combine the two or just make the check in 6/11 for
type != PLAIN_PASSWORD - return 0;


> ---
>  src/storage/storage_backend_iscsi.c | 56 +++++++++++++++++++++++++++++++++----
>  1 file changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
> index 6a17b5a..516025a 100644
> --- a/src/storage/storage_backend_iscsi.c
> +++ b/src/storage/storage_backend_iscsi.c
> @@ -35,6 +35,8 @@
>  #include <unistd.h>
>  #include <sys/stat.h>
>  
> +#include "datatypes.h"
> +#include "driver.h"
>  #include "virerror.h"
>  #include "storage_backend_scsi.h"
>  #include "storage_backend_iscsi.h"
> @@ -42,6 +44,7 @@
>  #include "virlog.h"
>  #include "virfile.h"
>  #include "vircommand.h"
> +#include "virobject.h"
>  #include "virrandom.h"
>  #include "virstring.h"
>  
> @@ -746,10 +749,17 @@ cleanup:
>  }
>  
>  static int
> -virStorageBackendISCSISetAuth(virStoragePoolDefPtr def,
> +virStorageBackendISCSISetAuth(virConnectPtr conn,
> +                              virStoragePoolDefPtr def,
>                                const char *portal,
>                                const char *target)
>  {
> +    virSecretPtr secret = NULL;
> +    unsigned char *secret_value = NULL;
> +    const char *passwd = NULL;
> +    virStoragePoolAuthChap chap = def->source.auth.chap;
> +    int ret = -1;
> +
>      if (def->source.authType == VIR_STORAGE_POOL_AUTH_NONE)
>          return 0;
>  
> @@ -759,6 +769,35 @@ virStorageBackendISCSISetAuth(virStoragePoolDefPtr def,
>          return -1;
>      }
>  
> +    if (chap.type == VIR_STORAGE_POOL_AUTH_CHAP_SECRET) {
> +        if (chap.u.secret.uuidUsable)
> +            secret = virSecretLookupByUUID(conn, chap.u.secret.uuid);
> +        else
> +            secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_ISCSI,
> +                                            chap.u.secret.usage);
> +
> +        if (secret) {
> +            size_t secret_size;
> +            secret_value = conn->secretDriver->secretGetValue(secret, &secret_size, 0,
> +                                                              VIR_SECRET_GET_VALUE_INTERNAL_CALL);
> +            if (!secret_value) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("could not get the value of the secret "
> +                                 "for username %s"), chap.login);
> +                goto cleanup;
> +            }
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("username '%s' specified but secret not found"),
> +                           chap.login);
> +            goto cleanup;
> +        }
> +
> +        passwd = (const char *)secret_value;
> +    } else if (chap.type == VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD) {
> +        passwd = chap.u.passwd;
> +    }
> +
>      if (virStorageBackendISCSINodeUpdate(portal,
>                                           target,
>                                           "node.session.auth.authmethod",
> @@ -770,14 +809,18 @@ virStorageBackendISCSISetAuth(virStoragePoolDefPtr def,
>          virStorageBackendISCSINodeUpdate(portal,
>                                           target,
>                                           "node.session.auth.password",
> -                                         def->source.auth.chap.u.passwd) < 0)
> -        return -1;
> +                                         passwd) < 0)
> +        goto cleanup;
>  
> -    return 0;
> +    ret = 0;
> +cleanup:
> +    virObjectUnref(secret);
> +    VIR_FREE(secret_value);
> +    return ret;
>  }
>  
>  static int
> -virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
> +virStorageBackendISCSIStartPool(virConnectPtr conn,

Seeing this change to used now makes me wonder... Do we need to now
check that conn != NULL anywhere?

Since "virStorageBackendISCSIStartPool" is the "startPool" entry point -
I used cscope and looked for "startPool", I found one reference where
a NULL was passed:

src/storage/storage_driver.c
storageDriverAutostart()
...
            if (backend->startPool &&
                backend->startPool(NULL, pool) < 0) {
                virErrorPtr err = virGetLastError();
...

If I'm reading things right, I think that will cause issues in
virSecretLookupByUUID() and virSecretLookupByUsage() when called by
virStorageBackendISCSISetAuth().

John


>                                  virStoragePoolObjPtr pool)
>  {
>      char *portal = NULL;
> @@ -817,7 +860,8 @@ virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                                NULL, NULL) < 0)
>              goto cleanup;
>  
> -        if (virStorageBackendISCSISetAuth(pool->def,
> +        if (virStorageBackendISCSISetAuth(conn,
> +                                          pool->def,
>                                            portal,
>                                            pool->def->source.devices[0].path) < 0)
>              goto cleanup;
>