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

[Jiri Denemark]

On Thu, Oct 01, 2015 at 16:48:44 +0300, Nikolay Shirokovskiy wrote:
> 
> 
> 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;
>         }
>     }

Well, you could make it a bit shorter:

    const char **compat;
    size_t ncompat;

    if (flags & VIR_MIGRATE_PEER2PEER) {
        compat = compatParamsP2P;
        ncompat = ARRAY_CARDINALITY(compatParamsP2P);
    } else {
        compat = compatParamsDirect;
        ncompat = ARRAY_CARDINALITY(compatParamsDirect);
    }

    if (!virTypedParamsCheck(params, nparams, compat, ncompat) ...

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

Yeah, looking at all other options I think your original code was the
best :-) So just keep it unchanged.

Jirka