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

[Nikolay Shirokovskiy]

On 01.10.2015 12:30, Jiri Denemark wrote:
> On Thu, Oct 01, 2015 at 10:34:49 +0300, Nikolay Shirokovskiy wrote:
>>
>>
>> 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;
> 
> I don't know, I was thinking about something similar:
> 
>     const char *compatParams[] = {VIR_MIGRATE_PARAM_DEST_NAME,
>                                   VIR_MIGRATE_PARAM_BANDWIDTH,
>                                   VIR_MIGRATE_PARAM_URI};
>     size_t ncompatParams = ARRAY_CARDINALITY(compatParams);
> 
>     if (flags & VIR_MIGRATE_PEER2PEER)
>         ncompatParams--;
> 
>     if (!virTypedParamsCheck(params, nparams, compatParams,
>                              ncompatParams) ...
> 
> but it looked similarly fragile. Maybe defining two arrays, one for p2p
> and one for direct migration would be the right clean way of doing
> this...

then it became bulky, like

    const char *compatParamsP2P[] = { VIR_MIGRATE_PARAM_DEST_NAME,              
                                      VIR_MIGRATE_PARAM_BANDWIDTH };            
    const char *compatParamsDirect[] = { 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;                                                   
    unsigned long long bandwidth = 0;                                           
                                                                                
    if (flags & VIR_MIGRATE_PEER2PEER) {                                        
        if (!virTypedParamsCheck(params, nparams, compatParamsP2P,              
                                 ARRAY_CARDINALITY(compatParamsP2P))) {         
            virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",                  
                           _("Some parameters are not supported by migration "  
                             "protocol 2."));                                   
            return -1;                                                          
        }                                                                       
    } else {                                                                    
        if (!virTypedParamsCheck(params, nparams, compatParamsDirect,           
                                 ARRAY_CARDINALITY(compatParamsDirect))) {      
            virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",                  
                           _("Some parameters are not supported by migration "  
                             "protocol 2."));                                   
            return -1;                                                          
        }                                                                       
    } 

May be return to the first variant. Anyway uri parameter is special case.

> 
> Jirka
>