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