Re: [PATCH 1/5] virstorage: Introduce virStorageAuthDef

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

 




On 07/02/2014 08:13 AM, Michal Privoznik wrote:
> On 27.06.2014 17:11, John Ferlan wrote:
>> Introduce virStorageAuthDef and friends.  Future patches will merge/utilize
>> their view of storage source/pool auth/secret definitions.
>>
>> New API's include:
>>      virStorageAuthDefParse:  Parse the "<auth" XML data for either the
>>                               domain disk or storage pool returning a
> 
> If I were to be narrow-minded OCD libvirt developer, I'd point out:
> s,<auth,<auth/>, but since I'm not, I'll leave it :)
> 
>>                               virStorageAuthDefPtr
>>      virStorageAuthDefCopy:   Copy a virStorageAuthDefPtr - to be used by
>>                               the qemuTranslateDiskSourcePoolAuth when it
>>                               copies storage pool auth data into domain
>>                               disk auth data
>>      virStorageAuthDefFormat: Common output of the "<auth" in the domain
>>                               disk or storage pool XML
>>      virStorageAuthDefFree:   Free memory associated with virStorageAuthDef
>>
>> Subsequent patches will utilize the new functions for the domain disk and
>> storage pools.
>>
>> Future work in the hostdev pass through can then make use of common data
>> structures and code.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>>   src/libvirt_private.syms  |   4 +
>>   src/util/virstoragefile.c | 205 ++++++++++++++++++++++++++++++++++++++++++++++
>>   src/util/virstoragefile.h |  27 ++++++
>>   3 files changed, 236 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 1e1dd84..17dcd67 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1879,6 +1879,10 @@ virStorageGenerateQcowPassphrase;
>>
>>
>>   # util/virstoragefile.h
>> +virStorageAuthDefCopy;
>> +virStorageAuthDefFormat;
>> +virStorageAuthDefFree;
>> +virStorageAuthDefParse;
> 
> I have some doubts it the Format and Parse should be in src/util/. But 
> since we already have something similar in virstorageencryption.c I 
> guess we can live with this location too.
> 

Yes, that was my model - the main goal was to avoid duplicated code. I
think that's more ripe for forgetting something or doing something in
one place, but not the other and some day having someone wonder why
there's a difference.

>>   virStorageFileCanonicalizePath;
>>   virStorageFileChainGetBroken;
>>   virStorageFileChainLookup;
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 0c50de1..056e59e 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -29,6 +29,8 @@
>>   #include <fcntl.h>
>>   #include <stdlib.h>
>>   #include "viralloc.h"
>> +#include "virxml.h"
>> +#include "viruuid.h"
>>   #include "virerror.h"
>>   #include "virlog.h"
>>   #include "virfile.h"
>> @@ -97,6 +99,10 @@ VIR_ENUM_IMPL(virStorageSourcePoolMode,
>>                 "host",
>>                 "direct")
>>
>> +VIR_ENUM_IMPL(virStorageAuth,
>> +              VIR_STORAGE_AUTH_TYPE_LAST,
>> +              "none", "chap", "ceph")
>> +
>>   enum lv_endian {
>>       LV_LITTLE_ENDIAN = 1, /* 1234 */
>>       LV_BIG_ENDIAN         /* 4321 */
>> @@ -1500,6 +1506,205 @@ virStorageNetHostDefCopy(size_t nhosts,
>>   }
>>
>>
>> +void
>> +virStorageAuthDefFree(virStorageAuthDefPtr authdef)
>> +{
>> +    if (!authdef)
>> +        return;
>> +
>> +    VIR_FREE(authdef->username);
>> +    VIR_FREE(authdef->secrettype);
>> +    if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_USAGE)
>> +        VIR_FREE(authdef->secret.usage);
>> +    VIR_FREE(authdef);
>> +}
>> +
>> +
>> +virStorageAuthDefPtr
>> +virStorageAuthDefCopy(const virStorageAuthDef *src)
>> +{
>> +    virStorageAuthDefPtr ret;
>> +
>> +    if (VIR_ALLOC(ret) < 0)
>> +        return NULL;
>> +
>> +    if (VIR_STRDUP(ret->username, src->username) < 0)
>> +        goto error;
>> +    /* Not present for storage pool, but used for disk source */
>> +    if (src->secrettype)
>> +        if (VIR_STRDUP(ret->secrettype, src->secrettype) < 0)
>> +            goto error;
> 
> There's no need to check for src->secrettype as VIR_STRDUP accepts NULL.
> 
>> +    ret->authType = src->authType;
>> +    ret->secretType = src->secretType;
>> +    if (ret->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
>> +        memcpy(ret->secret.uuid, src->secret.uuid, sizeof(ret->secret.uuid));
>> +    } else if (ret->secretType == VIR_STORAGE_SECRET_TYPE_USAGE) {
>> +        if (VIR_STRDUP(ret->secret.usage, src->secret.usage) < 0)
>> +            goto error;
>> +    }
>> +    return ret;
>> +
>> + error:
>> +    virStorageAuthDefFree(ret);
>> +    return NULL;
>> +}
>> +
> 
> I like the explicit copying, but just a suggestion to consider:
> {
>     VIR_ALLOC(ret);
>     memcpy(ret, src, sizeof(*ret));

^^^
For me this is more ripe for someone forgetting to strdup the authname
and if necessary the ret->secret.usage.

>     VIR_STRDUP(ret->scerettype, src->secrettype);
>     if (ret->secretType == VIR_STORAGE_SECRET_TYPE_USAGE &&
>         VIR_STRDUP() < 0)
>         goto error;

^^^
See... no VIR_STRDUP(ret->username, src->username); :-)

Also no error checking for sercrettype strdup failure with us leaving
the API with nothing in secrettype which may have been important.


> }
> 
> I'm worried that if a new item is added into thepy virStorageAuthDef 
> struct, sooner or later we forget to update the Copy function. With 
> memcpy() we are safe for shallow copies at least.
> 

But when using memcpy() if we fail/error on a strdup(), then we have
pointers from one structure in the other structure - the failure path
gets messy. I think if you're not copying pointers to allocated things,
then sure memcpy() is fine, but once you have pointers to allocated
things - it's safer to be explicit on what you're copying.


>> +
>> +static int
>> +virStorageAuthDefParseSecret(xmlXPathContextPtr ctxt,
>> +                             virStorageAuthDefPtr authdef)
>> +{
>> +    char *uuid;
>> +    char *usage;
>> +    int ret = -1;
>> +
>> +    /* Used by the domain disk xml parsing in order to ensure the
>> +     * <secret type='%s' value matches the expected secret type for
>> +     * the style of disk (iscsi is chap, nbd is ceph). For some reason
>> +     * the virSecretUsageType{From|To}String() cannot be linked here
>> +     * and because only the domain parsing code cares - just keep
>> +     * it as a string.
>> +     */
>> +    authdef->secrettype = virXPathString("string(./secret/@type)", ctxt);
>> +
>> +    uuid = virXPathString("string(./secret/@uuid)", ctxt);
>> +    usage = virXPathString("string(./secret/@usage)", ctxt);
>> +    if (uuid == NULL && usage == NULL) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("missing auth secret uuid or usage attribute"));
>> +        goto cleanup;
>> +    }
>> +
>> +    if (uuid && usage) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("either auth secret uuid or usage expected"));
>> +        goto cleanup;
>> +    }
>> +
>> +    if (uuid) {
>> +        if (virUUIDParse(uuid, authdef->secret.uuid) < 0) {
>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                            _("invalid auth secret uuid"));
>> +            goto cleanup;
>> +        }
>> +        authdef->secretType = VIR_STORAGE_SECRET_TYPE_UUID;
>> +    } else {
>> +        authdef->secret.usage = usage;
>> +        usage = NULL;
>> +        authdef->secretType = VIR_STORAGE_SECRET_TYPE_USAGE;
>> +    }
>> +    ret = 0;
>> +
>> + cleanup:
>> +    VIR_FREE(uuid);
>> +    VIR_FREE(usage);
>> +    return ret;
>> +}
>> +
>> +
>> +static virStorageAuthDefPtr
>> +virStorageAuthDefParseXML(xmlXPathContextPtr ctxt)
>> +{
>> +    virStorageAuthDefPtr authdef = NULL;
>> +    char *username = NULL;
>> +    char *authtype = NULL;
>> +
>> +    if (VIR_ALLOC(authdef) < 0)
>> +        return NULL;
>> +
>> +    if (!(username = virXPathString("string(./@username)", ctxt))) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("missing username for auth"));
>> +        goto error;
>> +    }
>> +    authdef->username = username;
>> +    username = NULL;
>> +
>> +    authdef->authType = VIR_STORAGE_AUTH_TYPE_NONE;
>> +    authtype = virXPathString("string(./@type)", ctxt);
>> +    if (authtype) {
>> +        /* Used by the storage pool instead of the secret type field
>> +         * to define whether chap or ceph being used
>> +         */
>> +        if ((authdef->authType = virStorageAuthTypeFromString(authtype)) < 0) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                           _("unknown auth type '%s'"), authtype);
>> +            goto error;
>> +        }
>> +        VIR_FREE(authtype);
> 
> No need for this, as you'll free it just a few lines below.
> 

Only on the error path...

>> +    }
>> +
>> +    authdef->secretType = VIR_STORAGE_SECRET_TYPE_NONE;
>> +    if (virStorageAuthDefParseSecret(ctxt, authdef) < 0)
>> +        goto error;
>> +
>> +    return authdef;
>> +
>> + error:
>> +    VIR_FREE(authtype);
>> +    VIR_FREE(username);
>> +    virStorageAuthDefFree(authdef);
>> +    return NULL;
>> +}
>> +
>> +
>> +virStorageAuthDefPtr
>> +virStorageAuthDefParse(xmlDocPtr xml, xmlNodePtr root)
>> +{
>> +    xmlXPathContextPtr ctxt = NULL;
>> +    virStorageAuthDefPtr authdef = NULL;
>> +
>> +    ctxt = xmlXPathNewContext(xml);
>> +    if (ctxt == NULL) {
>> +        virReportOOMError();
>> +        goto cleanup;
>> +    }
>> +
>> +    ctxt->node = root;
>> +    authdef = virStorageAuthDefParseXML(ctxt);
>> +
>> + cleanup:
>> +    xmlXPathFreeContext(ctxt);
>> +    return authdef;
>> +}
>> +
>> +
>> +int
>> +virStorageAuthDefFormat(virBufferPtr buf,
>> +                        virStorageAuthDefPtr authdef)
>> +{
>> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
>> +
>> +    if (authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE) {
>> +        virBufferEscapeString(buf, "<auth username='%s'>\n", authdef->username);
>> +    } else {
>> +        virBufferAsprintf(buf, "<auth type='%s' ",
>> +                          virStorageAuthTypeToString(authdef->authType));
>> +        virBufferEscapeString(buf, "username='%s'>\n", authdef->username);
>> +    }
>> +
>> +    virBufferAdjustIndent(buf, 2);
>> +    if (authdef->secrettype)
>> +        virBufferAsprintf(buf, "<secret type='%s'", authdef->secrettype);
>> +    else
>> +        virBufferAddLit(buf, "<secret");
>> +
>> +    if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
>> +        virUUIDFormat(authdef->secret.uuid, uuidstr);
>> +        virBufferAsprintf(buf, " uuid='%s'/>\n", uuidstr);
>> +    } else if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_USAGE) {
>> +        virBufferEscapeString(buf, " usage='%s'/>\n",
>> +                              authdef->secret.usage);
>> +    } else {
>> +        virBufferAddLit(buf, "/>\n");
>> +    }
>> +    virBufferAdjustIndent(buf, -2);
>> +    virBufferAddLit(buf, "</auth>\n");
>> +
>> +    return 0;
>> +}
>> +
>> +
>>   virSecurityDeviceLabelDefPtr
>>   virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
>>                                       const char *model)
>> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
>> index 48c7e02..383ba96 100644
>> --- a/src/util/virstoragefile.h
>> +++ b/src/util/virstoragefile.h
>> @@ -193,6 +193,15 @@ typedef virStorageSourcePoolDef *virStorageSourcePoolDefPtr;
>>
>>
>>   typedef enum {
>> +    VIR_STORAGE_AUTH_TYPE_NONE,
>> +    VIR_STORAGE_AUTH_TYPE_CHAP,
>> +    VIR_STORAGE_AUTH_TYPE_CEPHX,
>> +
>> +    VIR_STORAGE_AUTH_TYPE_LAST,
>> +} virStorageAuthType;
>> +VIR_ENUM_DECL(virStorageAuth)
>> +
>> +typedef enum {
>>       VIR_STORAGE_SECRET_TYPE_NONE,
>>       VIR_STORAGE_SECRET_TYPE_UUID,
>>       VIR_STORAGE_SECRET_TYPE_USAGE,
>> @@ -200,6 +209,19 @@ typedef enum {
>>       VIR_STORAGE_SECRET_TYPE_LAST
>>   } virStorageSecretType;
>>
>> +typedef struct _virStorageAuthDef virStorageAuthDef;
>> +typedef virStorageAuthDef *virStorageAuthDefPtr;
>> +struct _virStorageAuthDef {
>> +    char *username;
>> +    char *secrettype; /* <secret type='%s' for disk source */
>> +    int authType;     /* virStorageAuthType */
>> +    int secretType;   /* virStorageSecretType */
>> +    union {
>> +        unsigned char uuid[VIR_UUID_BUFLEN];
>> +        char *usage;
>> +    } secret;
>> +};
> 
> Is it necessary to expose the struct or can we hide it in the 
> virstoragefile.c?
> 

As you found out in 3/5... not easily... To be used by <disk> and
<pool>, but could also be used by <hostdev> for iSCSI.

John

>> +
>>   typedef struct _virStorageDriverData virStorageDriverData;
>>   typedef virStorageDriverData *virStorageDriverDataPtr;
>>
>> @@ -307,6 +329,11 @@ int virStorageFileGetLVMKey(const char *path,
>>   int virStorageFileGetSCSIKey(const char *path,
>>                                char **key);
>>
>> +void virStorageAuthDefFree(virStorageAuthDefPtr def);
>> +virStorageAuthDefPtr virStorageAuthDefCopy(const virStorageAuthDef *src);
>> +virStorageAuthDefPtr virStorageAuthDefParse(xmlDocPtr xml, xmlNodePtr root);
>> +int virStorageAuthDefFormat(virBufferPtr buf, virStorageAuthDefPtr authdef);
>> +
>>   virSecurityDeviceLabelDefPtr
>>   virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
>>                                       const char *model);
>>
> 
> 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]