Re: [libvirt] [PATCH 3/6] conf: Move authdef validation

2017-09-15 Thread 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 
>> ---
>>  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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/6] conf: Move authdef validation

2017-09-14 Thread Peter Krempa 
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 
> ---
>  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?

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

>  }
>  
>  

ACK with the above fixed.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2017-09-14 Thread John Ferlan 
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 
---
 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;
+
+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);
 }
 
 
@@ -8731,8 +8764,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 char *vendor = NULL;
 char *product = NULL;
 char *domain_name = NULL;
-int expected_secret_usage = -1;
-int auth_secret_usage = -1;
 
 if (!(def = virDomainDiskDefNew(xmlopt)))
 return NULL;
@@ -8776,13 +8807,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 
 source = true;
 
-if (def->src->type == VIR_STORAGE_TYPE_NETWORK) {
-if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI)
-expected_secret_usage = VIR_SECRET_USAGE_TYPE_ISCSI;
-else if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)
-expected_secret_usage = VIR_SECRET_USAGE_TYPE_CEPH;
-}
-
 startupPolicy = virXMLPropString(cur, "startupPolicy");
 
 } else if (!target &&
@@ -8840,17 +8864,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
virXMLNodeNameEqual(cur, "auth")) {
 if (!(authdef = virStorageAuthDefParse(node->doc, cur)))
 goto error;
-/* Disk volume types won't have the secrettype filled in until
- * after virStorageTranslateDiskSourcePool is run
- */
-if (def->src->type != VIR_STORAGE_TYPE_VOLUME &&
-(auth_secret_usage =
- virSecretUsageTypeFromString(authdef->secrettype)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("invalid secret type %s"),
-   authdef->secrettype);
-goto error;
-}
 } else if (virXMLNodeNameEqual(cur, "iotune")) {
 if (virDomainDiskDefIotuneParse(def, ctxt) < 0)
 goto error;
@@ -8914,18 +8927,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 }
 }
 
-/* Disk volume types will have authentication information handled in
- * virStorageTranslateDiskSourcePool
- */
-if (def->src->type != VIR_STORAGE_TYPE_VOLUME &&
-auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage) 
{
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("invalid secret type '%s'"),
-   virSecretUsageTypeToString(auth_secret_usage));
-goto error;
-}
-
-
 /* Only CDROM and Floppy devices are allowed missing source path
  * to indicate no media present. LUN is for raw access CD-ROMs