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

[Osier Yang <jyang@xxxxxxxxxx>]

On 07/06/13 00:51, John Ferlan wrote:
> 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;

Prefer the later.

> > ---
> >   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().

Reasonable, will update.

Osier

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list