[libvirt] [PATCH 1/5] virstorage: Introduce virStorageAuthDef
Michal Privoznik
mprivozn at redhat.com
Wed Jul 2 12:13:40 UTC 2014
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 at redhat.com>
> ---
> 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.
> 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));
VIR_STRDUP(ret->scerettype, src->secrettype);
if (ret->secretType == VIR_STORAGE_SECRET_TYPE_USAGE &&
VIR_STRDUP() < 0)
goto error;
}
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.
> +
> +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.
> + }
> +
> + 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?
> +
> 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
More information about the libvir-list
mailing list