2:55 p.m.
On 04/04/2016 10:33 AM, Ján Tomko wrote:
On Thu, Mar 31, 2016 at 11:05:07AM -0400, John Ferlan wrote:
> Commit id 'fb2bd208' essentially copied the qemuGetSecretString
> creating an libxlGetSecretString. Rather than have multiple copies
> of the same code, create virsecret.{c,h} files and place the common
> function in there.
>
> Usage is from both qemu_command.c and libxl_conf.c
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> Not for 1.3.3, but I may as well get it "out there" now...
>
> po/POTFILES.in | 1 +
> src/Makefile.am | 1 +
> src/libvirt_private.syms | 3 ++
> src/libxl/libxl_conf.c | 82 +++-----------------------------
> src/qemu/qemu_command.c | 87 ++++------------------------------
> src/util/virsecret.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virsecret.h | 35 ++++++++++++++
> 7 files changed, 174 insertions(+), 155 deletions(-)
> create mode 100644 src/util/virsecret.c
> create mode 100644 src/util/virsecret.h
>
> diff --git a/src/util/virsecret.c b/src/util/virsecret.c
> new file mode 100644
> index 0000000..07c052a
> --- /dev/null
> +++ b/src/util/virsecret.c
> @@ -0,0 +1,120 @@
> +/*
> + * virsecret.c: secret related utility functions
> + *
> + * Copyright (C) 2016 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>;.
> + *
> + */
> +
> +#include <config.h>
> +
> +#include "virsecret.h"
> +#include "viralloc.h"
> +#include "virerror.h"
> +#include "virlog.h"
> +#include "virobject.h"
> +#include "viruuid.h"
vir{error,object,uuid}.h are pulled in by datatypes.h
> +#include "base64.h"
> +#include "datatypes.h"
> +
datatypes.h contains internal definitions of public structs and should
not be included in src/util/
Hm. OK, I see. Not the first abuser though... (virauth, virdnsmasq,
virerror, virnodesuspend, virstats)
Simple enough to remove from virsecret, virdnsmasq, virnodesuspend, and
virstats, but more involved from virauth and virerror. Then of course
how to create a rule to enforce no further inclusion?
> +#define VIR_FROM_THIS VIR_FROM_SECRET
> +
> +VIR_LOG_INIT("util.secret");
> +
> +
> +/* virSecretGetSecretString:
> + * @conn: Pointer to the connection driver to make secret driver call
> + * @scheme: Unique enough string for error message to help determine cause
> + * @encoded: Whether the returned secret needs to be base64 encoded
> + * @authdef: Pointer to the disk storage authentication
> + * @secretUsageType: Type of secret usage for authdef lookup
> + *
> + * Lookup the secret for the authdef usage type and return it either as
> + * raw text or encoded based on the caller's need.
> + *
> + * Returns a pointer to memory that needs to be cleared and free'd after
> + * usage or NULL on error.
> + */
> +char *
> +virSecretGetSecretString(virConnectPtr conn,
> + const char *scheme,
> + bool encoded,
> + virStorageAuthDefPtr authdef,
> + virSecretUsageType secretUsageType)
> +{
> + size_t secret_size;
> + virSecretPtr sec = NULL;
> + char *secret = NULL;
> + char uuidStr[VIR_UUID_STRING_BUFLEN];
> +
> + /* look up secret */
> + switch (authdef->secretType) {
> + case VIR_STORAGE_SECRET_TYPE_UUID:
> + sec = virSecretLookupByUUID(conn, authdef->secret.uuid);
> + virUUIDFormat(authdef->secret.uuid, uuidStr);
> + break;
> + case VIR_STORAGE_SECRET_TYPE_USAGE:
> + sec = virSecretLookupByUsage(conn, secretUsageType,
> + authdef->secret.usage);
> + break;
> + }
> +
> + if (!sec) {
> + if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
> + virReportError(VIR_ERR_NO_SECRET,
> + _("%s no secret matches uuid '%s'"),
> + scheme, uuidStr);
> + } else {
> + virReportError(VIR_ERR_NO_SECRET,
> + _("%s no secret matches usage value
'%s'"),
> + scheme, authdef->secret.usage);
> + }
> + goto cleanup;
> + }
> +
> + secret = (char *)conn->secretDriver->secretGetValue(sec, &secret_size,
0,
> +
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
The secret driver should be calling functions from src/util, not the
other way around.
Could this function be moved into src/secret?
Won't that place dependencies on secret_driver for libxl_driver and
qemu_driver? IOW: Some sort of Makefile magic to get a symbol from the
src/secret/secret_util.c (assuming that's the name). That is Makefile
magic for which I'm not sure how to generate...
Alternatively, if I call:
secret = (char *)virSecretGetValue(sec, &secret_size, flags);
where 'flags' is a passed VIR_SECRET_GET_VALUE_INTERNAL_CALL value, does
that suffice? The 3 callers would then pass the flag since they can
include datatypes.h.
The virSecretGetValue doesn't check that flags == 0, but also doesn't
document this internal value.
> + if (!secret) {
> + if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("could not get value of the secret for "
> + "username '%s' using uuid
'%s'"),
> + authdef->username, uuidStr);
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("could not get value of the secret for "
> + "username '%s' using usage value
'%s'"),
> + authdef->username, authdef->secret.usage);
> + }
> + goto cleanup;
> + }
> +
> + if (encoded) {
> + char *base64 = NULL;
> +
> + base64_encode_alloc(secret, secret_size, &base64);
> + VIR_FREE(secret);
> + if (!base64) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + secret = base64;
> + }
> +
> + cleanup:
> + virObjectUnref(sec);
> + return secret;
> +}
> diff --git a/src/util/virsecret.h b/src/util/virsecret.h
> new file mode 100644
> index 0000000..8ef0629
> --- /dev/null
> +++ b/src/util/virsecret.h
> @@ -0,0 +1,35 @@
> +/*
> + * virsecret.h: secret related utility functions
> + *
> + * Copyright (C) 2016 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>;.
> + *
> + */
> +
> +#ifndef __VIR_SECRET_H__
> +# define __VIR_SECRET_H__
> +
> +# include "internal.h"
> +# include "virstoragefile.h"
> +
> +char *virSecretGetSecretString(virConnectPtr conn,
> + const char *scheme,
> + bool encoded,
> + virStorageAuthDefPtr authdef,
> + virSecretUsageType secretUsageType)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
s/3/4/
oh right - thanks. Bad fingers, they'll be punished ;-)
John