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@xxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
