Re: [Libvirt-cim] [PATCH 1 of 3] Define guest before migrating

KR> +static CMPIStatus define_guest_on_target(virConnectPtr dconn, KR> + virDomainPtr dom, KR> + struct migration_job *job, KR> + bool is_offline_migrate) KR> +{ KR> + CMPIStatus s = {CMPI_RC_OK, NULL}; KR> + virDomainPtr ddom = NULL; KR> + char *xml = NULL; KR> + KR> + if (!is_offline_migrate) { KR> + ddom = virDomainLookupByName(dconn, job->domain); KR> + if (ddom == NULL) { KR> + CU_DEBUG("Failed to lookup `%s'", job->domain); KR> + cu_statusf(_BROKER, &s, KR> + CMPI_RC_ERR_FAILED, KR> + "Failed to lookup domain `%s'", job->domain); KR> + goto out; KR> + } KR> + KR> + xml = virDomainGetXMLDesc(ddom, 0); KR> + } KR> + else That 'else' is broken. KR> + xml = virDomainGetXMLDesc(dom, 0); KR> + KR> + if (xml == NULL) { KR> + cu_statusf(_BROKER, &s, KR> + CMPI_RC_ERR_FAILED, KR> + "Unable to retrieve domain XML."); KR> + goto out; KR> + } KR> + KR> + ddom = virDomainDefineXML(dconn, xml); KR> + if (ddom == NULL) { KR> + CU_DEBUG("Failed to define domain from XML"); KR> + cu_statusf(_BROKER, &s, KR> + CMPI_RC_ERR_FAILED, KR> + "Failed to create domain"); KR> + } KR> + KR> + out: KR> + free(xml); KR> + virDomainFree(ddom); KR> + return s; KR> +} KR> + I really don't like the way this function re-defines an existing guest on the remote or copies one from local to remote depending on the flag. To me it looks like it should be able to just fail if it can't virDomainGetXMLDesc(dom). If you pass in a remote dom then it will do the redefine. If you pass in a local dom, it will effectively copy. KR> switch(job->type) { KR> case CIM_MIGRATE_OTHER: KR> CU_DEBUG("Preparing for offline migration"); KR> - s = handle_offline_migrate(job->conn, dom, uri, xml, job); KR> + s = handle_offline_migrate(job->conn, dom, uri, job); KR> break; KR> case CIM_MIGRATE_LIVE: KR> CU_DEBUG("Preparing for live migration"); KR> @@ -717,6 +729,15 @@ static CMPIStatus migrate_vs(struct migr KR> goto out; KR> CU_DEBUG("Migration succeeded"); KR> + KR> + s = define_guest_on_target(job->conn, NULL, job, false); KR> + if (s.rc != CMPI_RC_OK) { KR> + CU_DEBUG("Define failed"); KR> + cu_statusf(_BROKER, &s, KR> + CMPI_RC_ERR_FAILED, KR> + "Unable to define guest on target system"); KR> + } KR> + KR> cu_statusf(_BROKER, &s, KR> CMPI_RC_OK, KR> ""); It seems to me that a migration of any sort has now become like an offline migration, but with a little extra goo in the middle. So, I wonder if we can't change the switch above to something like this: xml = virDomainGetXMLDesc(local_dom); switch (type) { case CIM_MIGRATE_OTHER: /* Offline */ break; case CIM_MIGRATE_LIVE: s = handle_migration(dconn, dom) break; default: /* ERROR */ } if (s.rc == CMPI_RC_OK) define_guest_on_target(dconn, xml); Thus, offline migration is mostly just a fall-through, but the other migration types have actual work in the middle. Thoughts? Since Kaitlin is out, I'll pick this up and re-swizzle. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms(a)us.ibm.com