Re: [libvirt] [PATCH V2 13/13] libxl: add migration support
On 19.03.2014 23:52, Jim Fehlig wrote:
Michal Privoznik wrote:
> On 13.03.2014 23:11, Jim Fehlig wrote:
>> This patch adds initial migration support to the libxl driver,
>> using the VIR_DRV_FEATURE_MIGRATION_PARAMS family of migration
>> functions.
>>
>> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
>> ---
>>
>> V2:
>> - Now that the libxl driver supports hostdev passthrough, ensure
>> domain has no hostdevs before migration
>> - Fail early in libxlDomainMigrateBegin3Params if domain is inactive
>> - Change name of local variable in libxlDoMigrateSend from 'live'
>> to 'xl_flags'.
>>
>> po/POTFILES.in | 1 +
>> src/Makefile.am | 3 +-
>> src/libxl/libxl_conf.h | 6 +
>> src/libxl/libxl_domain.h | 1 +
>> src/libxl/libxl_driver.c | 225 +++++++++++++++++
>> src/libxl/libxl_migration.c | 598
>> ++++++++++++++++++++++++++++++++++++++++++++
>> src/libxl/libxl_migration.h | 78 ++++++
>> 7 files changed, 911 insertions(+), 1 deletion(-)
>>
>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 3306dc1..6fc5266 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>
>> @@ -4313,6 +4323,216 @@ cleanup:
>> return ret;
>> }
>>
>> +static char *
>> +libxlDomainMigrateBegin3Params(virDomainPtr domain,
>> + virTypedParameterPtr params,
>> + int nparams,
>> + char **cookieout ATTRIBUTE_UNUSED,
>> + int *cookieoutlen ATTRIBUTE_UNUSED,
>> + unsigned int flags)
>> +{
>> + const char *xmlin = NULL;
>> + const char *dname = NULL;
>> + virDomainObjPtr vm = NULL;
>> +
>> + virCheckFlags(LIBXL_MIGRATION_FLAGS, NULL);
>> + if (virTypedParamsValidate(params, nparams,
>> LIBXL_MIGRATION_PARAMETERS) < 0)
>> + return NULL;
>> +
>> + if (virTypedParamsGetString(params, nparams,
>> + VIR_MIGRATE_PARAM_DEST_XML,
>> + &xmlin) < 0 ||
>> + virTypedParamsGetString(params, nparams,
>> + VIR_MIGRATE_PARAM_DEST_NAME,
>> + &dname) < 0)
>
> I'd expect @dname to be used somewhere...
Seems I followed the qemu driver too closely, where AFAICT dname isn't
used in the begin phase either. Is there even anything to do with dname
in the begin phase on the migration source?
Yes, it's not used in qemu driver either. It's not a bug though. I've
just spotted an unused variable so I've pointed it out during review.
I'd go with removing it both here and there (in two separate patches)
and call it the day.
>> +libxlDomainMigrationPerform(libxlDriverPrivatePtr driver,
>> + virDomainObjPtr vm,
>> + const char *dom_xml ATTRIBUTE_UNUSED,
>> + const char *dconnuri ATTRIBUTE_UNUSED,
>> + const char *uri_str,
>> + const char *dname ATTRIBUTE_UNUSED,
>> + unsigned int flags)
>> +{
>> + char *hostname = NULL;
>> + unsigned short port = 0;
>> + char portstr[100];
>> + virURIPtr uri = NULL;
>> + virNetSocketPtr sock;
>> + int sockfd = -1;
>> + int saved_errno = EINVAL;
>> + int ret = -1;
>> +
>
> Missing virCheckFlags(VIR_MIGRATE_LIVE, -1);
Flags are checked in the calling function, libxlDomainMigratePerform3Params.
Oh, right.
I've fixed up the other issues you mentioned, but will wait to send a V3
until clarifying the use of dname in the begin phase.
Regards,
Jim
Michal