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

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;