[libvirt] [PATCH 3/6] conf: Move <disk> authdef validation

[John Ferlan]

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