On Thu, Oct 01, 2015 at 16:48:44 +0300, Nikolay Shirokovskiy wrote:
>
>
> 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;
> }
> }
Well, you could make it a bit shorter:
const char **compat;
size_t ncompat;
if (flags & VIR_MIGRATE_PEER2PEER) {
compat = compatParamsP2P;
ncompat = ARRAY_CARDINALITY(compatParamsP2P);
} else {
compat = compatParamsDirect;
ncompat = ARRAY_CARDINALITY(compatParamsDirect);
}
if (!virTypedParamsCheck(params, nparams, compat, ncompat) ...
> May be return to the first variant. Anyway uri parameter is special case.
Yeah, looking at all other options I think your original code was the
best :-) So just keep it unchanged.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list