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@xxxxxxxxxxxxx>
>>>> ---
>>>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list