Re: [libvirt] [PATCH v3 5/5] secret: Move virStorageSecretType to secret_util and rename
On 06/06/2016 03:27 AM, Peter Krempa wrote:
On Fri, Jun 03, 2016 at 06:52:53 -0400, John Ferlan wrote:
> Move the enum into secret_util, rename it to be just virSecretLookupType.
> This includes quite a bit of collateral damage, but the goal is to remove
> the "virStorage*" and replace with the virSecretLookupType so that
it's
> easier to to add new lookups that aren't necessarily storage pool related.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> cfg.mk | 2 +-
> src/conf/secret_conf.h | 2 +-
> src/libxl/libxl_conf.c | 2 +-
> src/qemu/qemu_domain.c | 4 ++--
> src/secret/secret_util.c | 18 +++++++++---------
> src/secret/secret_util.h | 22 ++++++++++++++++++++--
> src/storage/storage_backend_iscsi.c | 7 ++++---
> src/storage/storage_backend_rbd.c | 3 ++-
> src/util/virstoragefile.c | 33 +++++++++++++++++----------------
> src/util/virstoragefile.h | 17 +++--------------
> tests/qemuargv2xmltest.c | 4 ++--
> 11 files changed, 62 insertions(+), 52 deletions(-)
>
> diff --git a/cfg.mk b/cfg.mk
> index a7b7266..0529a4e 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -780,7 +780,7 @@
mid_dirs=access|conf|cpu|locking|logging|network|node_device|rpc|security|storag
> sc_prohibit_cross_inclusion:
> @for dir in $(cross_dirs); do \
> case $$dir in \
> - util/) safe="util";; \
> + util/) safe="(util|secret)";; \
I don't think this is a good idea. utils are used in many places that
don't link to the secret driver. While this is now used just to pull in
one data type, the check is meant to prevent problems with linking the
file if certain modules are disabled.
Understood, but it was a far less invasive option than chasing the
virstoragefile.h includes to then modify many more places in order to
get the definition.
I would think that definition belongs with secret_util. It's how the
lookup of uuid/usage for virSecretGetSecretString works and has less to
do with something storage specific. If there's another way to make some
adjustment to that particular syntax-check, that's fine, but this is
what I found that worked. Those checks aren't necessarily very readable
to my eyes.
I'm not against keeping it in virstoragefile.h if that's the preference.
Is the name change OK or would you prefer to see
virStorageLookupTypeDef. It's always been a bit confusing when to see
"secretType" in the code when it really was the lookup type not the
"type" from the <secret...> object.
Taking things another step further - eventually secrets will be used to
support TLS objects - having a structure with virStorageLookupTypeDef
wouldn't seem to be appropriate.
Untying the existing relationship between the secret driver and the
storage driver (as well as qemu and libxl) without "falling back to"
libvirt API calls to virSecret* is going to require some amount of
cross-pollination. It's a matter of choice.
John