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

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(a)virtuozzo.com&gt; >>>> --- >>>> 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.