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

[Jiri Denemark]

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

Jirka