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