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