11:08 a.m.
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(a)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.
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