Re: [libvirt] [PATCH 3/6] conf: Move <disk> authdef validation
On 09/14/2017 11:58 PM, Peter Krempa wrote:
On Thu, Sep 14, 2017 at 14:03:07 -0400, John Ferlan wrote:
> Rather than checking during XML processing, move the checks for correct
> and valid auth into virDomainDiskDefParseValidate. This will introduce
> virDomainDiskSourceDefParseAuthValidate to validate that the authdef
> stored for the virStorageSource is valid. This can then be expanded
> to service backingStore sources as well.
>
> Alter the message text slightly as well to distinguish between an
> unknown name and an incorrectly used name. Since type is not a
> mandatory field, add the NULLSTR() around the output of the unknown
> error. NB, a config using unknown formatting would fail virschematest
> since it only accepts 'iscsi' and 'ceph' as "valid" types.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/domain_conf.c | 67 +++++++++++++++++++++++++-------------------------
> 1 file changed, 34 insertions(+), 33 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a43b25c31..07bda1a36 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8500,6 +8500,39 @@ virDomainDiskDefGeometryParse(virDomainDiskDefPtr def,
>
>
> static int
> +virDomainDiskSourceDefParseAuthValidate(const virStorageSource *src)
> +{
> + virStorageAuthDefPtr authdef = src->auth;
> + int actUsage;
> +
> + /* Disk volume types won't have the secrettype filled in until
> + * after virStorageTranslateDiskSourcePool is run
> + */
> + if (src->type == VIR_STORAGE_TYPE_VOLUME || !authdef)
> + return 0;
Should this also include || src->type != VIR_STORAGE_TYPE_NETWORK?
That check should work too... Not sure it'd work/look quite right as an
|| though
if (type == VOLUME || type != NETWORK || !authdef)
I'll just adjust to be if (src->type != VIR_STORAGE_TYPE_NETWORK ||
!authdef) and remove the comment.
> +
> + if ((actUsage = virSecretUsageTypeFromString(authdef->secrettype)) < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown secret type '%s'"),
> + NULLSTR(authdef->secrettype));
> + return -1;
> + }
> +
> + if ((src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI &&
> + actUsage != VIR_SECRET_USAGE_TYPE_ISCSI) ||
> + (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD &&
> + actUsage != VIR_SECRET_USAGE_TYPE_CEPH)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("invalid secret type '%s'"),
> + virSecretUsageTypeToString(actUsage));
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +
> +static int
> virDomainDiskDefParseValidate(const virDomainDiskDef *def)
> {
> if (def->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
> @@ -8572,7 +8605,7 @@ virDomainDiskDefParseValidate(const virDomainDiskDef *def)
> }
> }
>
> - return 0;
> + return virDomainDiskSourceDefParseAuthValidate(def->src);
As common in similar functions, add a if block and leave the "return 0"
intact. Saving two lines is not worth breaking the structure and making
the code less extensible.
True - I wrote it that way first, but changed it... Only because I had a
recollection of having it said I should just return directly, I'll
change it though.
if (!function)
return -1;
return 0;
> }
>
>
ACK with the above fixed.
Tks -
John