8:48 a.m.
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>
>>> ---
>>> 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.
Jirka