[libvirt] [PATCH v3 09/14] migration: refactor: refactor parameter compatibility checks

[Nikolay Shirokovskiy]

On 30.09.2015 16:38, Jiri Denemark wrote:
> On Fri, Sep 18, 2015 at 18:05:47 +0300, Nikolay Shirokovskiy wrote:
>> Move virDomainMigrateUnmanagedProto* expected params list check into
>> function itself and use common virTypedParamsCheck for this purpose.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>> ---
>>  src/libvirt-domain.c |   56 ++++++++++++++++++++-----------------------------
>>  1 files changed, 23 insertions(+), 33 deletions(-)
>>
>> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
>> index f87a22d..abed9d6 100644
>> --- a/src/libvirt-domain.c
>> +++ b/src/libvirt-domain.c
>> @@ -3322,30 +3322,31 @@ virDomainMigrateUnmanagedProto2(virDomainPtr domain,
>>                                  int nparams,
>>                                  unsigned int flags)
>>  {
>> +    /* uri parameter is added for direct case */
>> +    const char *compatParams[] = { VIR_MIGRATE_PARAM_DEST_NAME,
>> +                                   VIR_MIGRATE_PARAM_BANDWIDTH,
>> +                                   VIR_MIGRATE_PARAM_URI };
>>      const char *uri = NULL;
>>      const char *miguri = NULL;
>>      const char *dname = NULL;
>> -    const char *xmlin = NULL;
>>      unsigned long long bandwidth = 0;
>>  
>> +    if (!virTypedParamsCheck(params, nparams, compatParams,
>> +                             ARRAY_CARDINALITY(compatParams))) {
>> +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>> +                       _("Migration does not support some of parameters."));
> 
> How about "Some parameters are not supported by migration protocol 2"?
> 
>> +        return -1;
>> +    }
>> +
>>      if (virTypedParamsGetString(params, nparams,
>>                                  VIR_MIGRATE_PARAM_URI, &miguri) < 0 ||
>>          virTypedParamsGetString(params, nparams,
>>                                  VIR_MIGRATE_PARAM_DEST_NAME, &dname) < 0 ||
>> -        virTypedParamsGetString(params, nparams,
>> -                                VIR_MIGRATE_PARAM_DEST_XML, &xmlin) < 0 ||
>>          virTypedParamsGetULLong(params, nparams,
>>                                  VIR_MIGRATE_PARAM_BANDWIDTH, &bandwidth) < 0) {
>>          return -1;
>>      }
>>  
>> -    if (xmlin) {
>> -        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>> -                       _("Unable to change target guest XML during "
>> -                         "migration"));
>> -        return -1;
>> -    }
>> -
>>      if (flags & VIR_MIGRATE_PEER2PEER) {
>>          if (miguri) {
>>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                               _("Unable to override peer2peer migration URI"));
>                return -1;
>            }
> 
> Can we merge this check with with the others by adding
> VIR_MIGRATE_PARAM_URI to compatParams only if VIR_MIGRATE_PEER2PEER was
> passed? I think it would be a bit cleaner, although it depends on how
> dirty way of doing that you choose :-)
I'm not sure is this considered dirty?

1. char** cast (could be fixed by changing function signature to const char * const *
2. unchecked array accessing - safe until somebody will edit this code


    const char *compatParams[3] = { VIR_MIGRATE_PARAM_DEST_NAME,                
                                    VIR_MIGRATE_PARAM_BANDWIDTH,                
                                    NULL};                                      
    size_t ncompatParams = 0;                                                       
                                                                                
    ncompatParams = virStringListLength((char**)compatParams);                  
    if (!(flags & VIR_MIGRATE_PEER2PEER))                                       
        compatParams[ncompatParams++] = VIR_MIGRATE_PARAM_URI; 

> 
>> @@ -3369,11 +3370,22 @@ virDomainMigrateUnmanagedProto3(virDomainPtr domain,
>>                                  int nparams,
>>                                  unsigned int flags)
>>  {
>> +    const char *compatParams[] = { VIR_MIGRATE_PARAM_URI,
>> +                                   VIR_MIGRATE_PARAM_DEST_NAME,
>> +                                   VIR_MIGRATE_PARAM_DEST_XML,
>> +                                   VIR_MIGRATE_PARAM_BANDWIDTH };
>>      const char *miguri = NULL;
>>      const char *dname = NULL;
>>      const char *xmlin = NULL;
>>      unsigned long long bandwidth = 0;
>>  
>> +    if (!virTypedParamsCheck(params, nparams, compatParams,
>> +                             ARRAY_CARDINALITY(compatParams))) {
>> +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>> +                       _("Migration does not support some of parameters."));
> 
> How about "Some parameters are not supported by migration protocol 3"?
> 
>> +        return -1;
>> +    }
>> +
>>      if (virTypedParamsGetString(params, nparams,
>>                                  VIR_MIGRATE_PARAM_URI, &miguri) < 0 ||
>>          virTypedParamsGetString(params, nparams,
> 
> Jirka
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>