Re: [libvirt] [PATCH] util: Create virsecret module adding virSecretGetSecretString

2016-04-04 Thread John Ferlan 

[...]


>>
>> 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...
> 

I think I figured out the magic incantation... Patches sent shortly

[...]

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Create virsecret module adding virSecretGetSecretString

2016-04-04 Thread John Ferlan 


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 
>> ---
>> 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 000..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
>> + * .
>> + *
>> + */
>> +
>> +#include 
>> +
>> +#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

Re: [libvirt] [PATCH] util: Create virsecret module adding virSecretGetSecretString

2016-04-04 Thread Ján Tomko 
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 
> ---
> 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 000..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
> + * .
> + *
> + */
> +
> +#include 
> +
> +#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/

> +#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, _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?

> +if (!secret) {
> +if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("could not get value of the secret for "
> +

[libvirt] [PATCH] util: Create virsecret module adding virSecretGetSecretString

2016-03-31 Thread John Ferlan 
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 
---
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/po/POTFILES.in b/po/POTFILES.in
index 0d7f9f9..e3b8468 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -229,6 +229,7 @@ src/util/virportallocator.c
 src/util/virprocess.c
 src/util/virrandom.c
 src/util/virrotatingfile.c
+src/util/virsecret.c
 src/util/virsexpr.c
 src/util/virscsi.c
 src/util/virsocketaddr.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 1726d06..4783f40 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -156,6 +156,7 @@ UTIL_SOURCES =  
\
util/virrotatingfile.h util/virrotatingfile.c   \
util/virscsi.c util/virscsi.h   \
util/virseclabel.c util/virseclabel.h   \
+   util/virsecret.c util/virsecret.h   \
util/virsexpr.c util/virsexpr.h \
util/virsocketaddr.h util/virsocketaddr.c   \
util/virstats.c util/virstats.h \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 684f06c..fe3d132 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2142,6 +2142,9 @@ virSecurityLabelDefFree;
 virSecurityLabelDefNew;
 
 
+# util/virsecret.h
+virSecretGetSecretString;
+
 # util/virsexpr.h
 sexpr2string;
 sexpr_append;
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 82ba417..db26511 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -46,7 +46,7 @@
 #include "libxl_conf.h"
 #include "libxl_utils.h"
 #include "virstoragefile.h"
-#include "base64.h"
+#include "virsecret.h"
 
 
 #define VIR_FROM_THIS VIR_FROM_LIBXL
@@ -935,76 +935,6 @@ libxlDomainGetEmulatorType(const virDomainDef *def)
 return ret;
 }
 
-static char *
-libxlGetSecretString(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, _size, 0,
-
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
-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, );
-VIR_FREE(secret);
-if (!base64) {
-virReportOOMError();
-goto cleanup;
-}
-secret = base64;
-}