3:17 a.m.
I've just updated this patch so it can be applied to CVS.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/
3:57 a.m.
On Tue, Jun 03, 2008 at 09:17:01AM +0100, Richard W.M. Jones wrote:
I've just updated this patch so it can be applied to CVS.
[...]
Index: src/libvirt.c
===================================================================
RCS file: /data/cvs/libvirt/src/libvirt.c,v
retrieving revision 1.143
[...]
+ if (uri_out) uri = uri_out; /* Did domainMigratePrepare
change URI? */
+
+ assert (uri != NULL);
Hum, assert forces the process to exit. I don't think that's
acceptable for one of the library entry points.
And error and returning NULL looks far mor proper there.
[...]
- assert (uri != NULL);
+ assert (uri != NULL);
+ }
[...]
+qemudDomainMigratePerform (virDomainPtr dom,
+ const char *cookie ATTRIBUTE_UNUSED,
+ int cookielen ATTRIBUTE_UNUSED,
+ const char *uri,
+ unsigned long flags ATTRIBUTE_UNUSED,
+ const char *dname ATTRIBUTE_UNUSED,
+ unsigned long resource)
+{
[...]
+ free (info);
[...]
+ free (safe_uri);
[...]
+ free (info);
VIR_FREE should be used now i think.
Except for those points the patch looks fine to me,
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
8:21 a.m.
On Tue, Jun 03, 2008 at 09:17:01AM +0100, Richard W.M. Jones wrote:
Index: qemud/remote.c
===================================================================
RCS file: /data/cvs/libvirt/qemud/remote.c,v
retrieving revision 1.35
diff -u -p -r1.35 remote.c
--- qemud/remote.c 23 May 2008 08:24:44 -0000 1.35
+++ qemud/remote.c 3 Jun 2008 08:17:52 -0000
@@ -1346,6 +1346,66 @@ remoteDispatchDomainMigrateFinish (struc
}
static int
+remoteDispatchDomainMigratePrepare2 (struct qemud_server *server ATTRIBUTE_UNUSED,
+ struct qemud_client *client,
+ remote_message_header *req,
+ remote_domain_migrate_prepare2_args *args,
+ remote_domain_migrate_prepare2_ret *ret)
+{
+ int r;
+ char *cookie = NULL;
+ int cookielen = 0;
+ char *uri_in;
+ char **uri_out;
+ char *dname;
+ CHECK_CONN (client);
+
+ uri_in = args->uri_in == NULL ? NULL : *args->uri_in;
+ dname = args->dname == NULL ? NULL : *args->dname;
+
+ /* Wacky world of XDR ... */
+ uri_out = calloc (1, sizeof (*uri_out));
This ought to be updated to use
if (VIR_ALLOC(uri_out)) < 0) {
remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL);
return -2;
}
+
+ r = __virDomainMigratePrepare2 (client->conn, &cookie, &cookielen,
+ uri_in, uri_out,
+ args->flags, dname, args->resource,
+ args->dom_xml);
+ if (r == -1) return -1;
+
+ /* remoteDispatchClientRequest will free cookie, uri_out and
+ * the string if there is one.
+ */
+ ret->cookie.cookie_len = cookielen;
+ ret->cookie.cookie_val = cookie;
+ ret->uri_out = *uri_out == NULL ? NULL : uri_out;
This will leak uri_out in the 1st half of the conditional
+static int
+remoteDispatchDomainMigrateFinish2 (struct qemud_server *server ATTRIBUTE_UNUSED,
+ struct qemud_client *client,
+ remote_message_header *req,
+ remote_domain_migrate_finish2_args *args,
+ remote_domain_migrate_finish2_ret *ret)
+{
+ virDomainPtr ddom;
+ CHECK_CONN (client);
+
+ ddom = __virDomainMigrateFinish2 (client->conn, args->dname,
+ args->cookie.cookie_val,
+ args->cookie.cookie_len,
+ args->uri,
+ args->flags,
+ args->retcode);
+ if (ddom == NULL) return -1;
+
+ make_nonnull_domain (&ret->ddom, ddom);
Need to call virDomainFree(ddom) after this.
diff -u -p -r1.143 libvirt.c
--- src/libvirt.c 29 May 2008 19:23:17 -0000 1.143
+++ src/libvirt.c 3 Jun 2008 08:17:57 -0000
+ else /* if (version == 2) */ {
+ /* In version 2 of the protocol, the prepare step is slightly
+ * different. We fetch the domain XML of the source domain
+ * and pass it to Prepare2.
+ */
+ if (!conn->driver->domainDumpXML) {
+ virLibConnError (conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__);
+ return NULL;
+ }
+ dom_xml = conn->driver->domainDumpXML (domain,
+ VIR_DOMAIN_XML_SECURE);
+
+ if (!dom_xml)
+ return NULL;
+
+ ret = dconn->driver->domainMigratePrepare2
+ (dconn, &cookie, &cookielen, uri, &uri_out, flags, dname,
+ bandwidth, dom_xml);
+ free (dom_xml);
Can use VIR_FREE now
@@ -2190,18 +2230,28 @@ virDomainMigrate (virDomainPtr domain,
ret = conn->driver->domainMigratePerform
(domain, cookie, cookielen, uri, flags, dname, bandwidth);
- if (ret == -1) goto done;
-
- /* Get the destination domain and return it or error.
- * 'domain' no longer actually exists at this point (or so we hope), but
- * we still use the object in memory in order to get the name.
- */
- dname = dname ? dname : domain->name;
- if (dconn->driver->domainMigrateFinish)
- ddomain = dconn->driver->domainMigrateFinish
- (dconn, dname, cookie, cookielen, uri, flags);
- else
- ddomain = virDomainLookupByName (dconn, dname);
+ if (version == 1) {
+ if (ret == -1) goto done;
+ /* Get the destination domain and return it or error.
+ * 'domain' no longer actually exists at this point
+ * (or so we hope), but we still use the object in memory
+ * in order to get the name.
+ */
+ dname = dname ? dname : domain->name;
+ if (dconn->driver->domainMigrateFinish)
+ ddomain = dconn->driver->domainMigrateFinish
+ (dconn, dname, cookie, cookielen, uri, flags);
+ else
+ ddomain = virDomainLookupByName (dconn, dname);
So this code suggests that domainMigrateFinish is optional in v1
of the migration protocol...
+ } else /* if (version == 2) */ {
+ /* In version 2 of the migration protocol, we pass the
+ * status code from the sender to the destination host,
+ * so it can do any cleanup if the migration failed.
+ */
+ dname = dname ? dname : domain->name;
+ ddomain = dconn->driver->domainMigrateFinish2
+ (dconn, dname, cookie, cookielen, uri, flags, ret);
+ }
And compulsory in v2 ? Is that the correct understanding ? What if
we wanted to make Xen Driver use v2, but it doesn't require the finish
method ? I guess we could just no-op it.
+/* Migration support. */
+
+/* Prepare is the first step, and it runs on the destination host.
+ *
+ * This starts an empty VM listening on a TCP port.
+*/
+static int
+qemudDomainMigratePrepare2 (virConnectPtr dconn,
+ char **cookie ATTRIBUTE_UNUSED,
+ int *cookielen ATTRIBUTE_UNUSED,
+ const char *uri_in,
+ char **uri_out,
+ unsigned long flags ATTRIBUTE_UNUSED,
+ const char *dname,
+ unsigned long resource ATTRIBUTE_UNUSED,
+ const char *dom_xml)
+{
+ static int port = 0;
+ struct qemud_driver *driver = (struct qemud_driver *)dconn->privateData;
+ struct qemud_vm_def *def;
+ struct qemud_vm *vm;
+ int this_port;
+ char hostname [HOST_NAME_MAX+1];
+ const char *p;
+ const int uri_length_max =
+ 6 /* "tcp://" */ +
+ sizeof (hostname) /* hostname */ +
+ 1 + 5 + 1 /* :port\0 */;
+
+ if (!dom_xml) {
+ qemudReportError (dconn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ "%s", _("no domain XML passed"));
+ return -1;
+ }
+
+ /* The URI passed in may be NULL or a string "tcp://somehostname:port".
+ *
+ * If the URI passed in is NULL then we allocate a port number
+ * from our pool of port numbers and return a URI of
+ * "tcp://ourhostname:port".
+ *
+ * If the URI passed in is not NULL then we try to parse out the
+ * port number and use that (note that the hostname is assumed
+ * to be a correct hostname which refers to the target machine).
+ */
+ if (uri_in == NULL) {
+ this_port = QEMUD_MIGRATION_FIRST_PORT + port++;
+ if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0;
This is a little fragile - although the pool of 64 numbers is unlikely to
wrap around and collide in 'regular' use, if some client of libvirt were
mis-behaving its conceivable that they could issue a large number of migrate
requests and cause a collsion. I think we ought to explicitly track the
ports in use (and auto-expire if client crashes part way through).
+ /* Caller frees */
+ *uri_out = malloc (uri_length_max);
Can use VIR_ALLOC_N(*uri_out, uri_length_max) here
+ } else {
+ /* Check the URI starts with "tcp://". We will escape the
+ * URI when passing it to the qemu monitor, so bad
+ * characters in hostname part don't matter.
+ */
+ if (!STREQLEN (uri_in, "tcp://", 6)) {
+ qemudReportError (dconn, NULL, NULL, VIR_ERR_INVALID_ARG,
+ "%s", _("only tcp URIs are supported for KVM
migrations"));
+ return -1;
+ }
+
+ /* Get the port number. */
+ p = strrchr (uri_in, ':');
+ p++; /* definitely has a ':' in it, see above */
I think this should use the libxml2 URI parsing code instead.
+ /* Parse the domain XML. */
+ if (!(def = qemudParseVMDef (dconn, driver, dom_xml, NULL))) {
+ qemudReportError (dconn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
+ "%s", _("failed to parse XML"));
+ return -1;
+ }
+
+ /* Target domain name, maybe renamed. */
+ dname = dname ? dname : def->name;
+
+ /* Ensure the name and UUID don't already exist in an active VM */
+ vm = qemudFindVMByUUID (driver, def->uuid);
+ if (!vm) vm = qemudFindVMByName (driver, dname);
+ if (vm) {
+ qemudReportError (dconn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
+ _("domain is already active as '%s'"),
vm->def->name);
+ return -1;
+ }
+
+ if (!(vm = qemudAssignVMDef (dconn, driver, def))) {
+ qemudReportError (dconn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
+ "%s", _("failed to assign new VM"));
+ qemudFreeVMDef (def);
+ return -1;
+ }
+
+ /* Start the QEMU daemon, with the same command-line arguments plus
+ * -incoming tcp://0:port
+ */
+ snprintf (vm->migrateFrom, sizeof (vm->migrateFrom),
+ "tcp://0:%d", this_port);
+ if (qemudStartVMDaemon (dconn, driver, vm) < 0) {
+ qemudReportError (dconn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
+ "%s", _("failed to start listening VM"));
+ if (!vm->configFile[0])
+ qemudRemoveInactiveVM(driver, vm);
+ return -1;
+ }
I don't think we're handling persistent config files correctly in this
code. There's a couple of scenarios we need to deal with
Source: transient Target: none -> transient
Source: transient Target: transient, running -> error
Source: transient Target: persistent, inactive -> migrate, update active
config
Source: transient Target: persistent, running -> error
Source: persistent Target: none -> migrate & save persistent
config on success
Source: persistent Target: transient, running -> error
Source: persistent Target: persistent, inactive -> migrate, update active
config
Source: persistent Target: persistent, running -> error
5B5BIn particular, if the target already has a persistent config we need to be
careful not to remove than config upon migrate failure. We also need to
make sure that if we create a persistent config, that SaveVMDef is called
to actually flush it to disk.
+/* Perform is the second step, and it runs on the source host. */
+static int
+qemudDomainMigratePerform (virDomainPtr dom,
+ const char *cookie ATTRIBUTE_UNUSED,
+ int cookielen ATTRIBUTE_UNUSED,
+ const char *uri,
+ unsigned long flags ATTRIBUTE_UNUSED,
+ const char *dname ATTRIBUTE_UNUSED,
+ unsigned long resource)
+{
+ struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
+ struct qemud_vm *vm = qemudFindVMByID (driver, dom->id);
+ char *safe_uri;
+ char cmd[HOST_NAME_MAX+50];
+ char *info;
+
+ if (!vm) {
+ qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
+ _("no domain with matching id %d"), dom->id);
+ return -1;
+ }
+
+ if (!qemudIsActiveVM (vm)) {
+ qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+ "%s", _("domain is not running"));
+ return -1;
+ }
+
+ if (resource > 0) {
+ /* Issue migrate_set_speed command. Don't worry if it fails. */
+ snprintf (cmd, sizeof cmd, "migrate_set_speed %lum", resource);
+ qemudMonitorCommand (driver, vm, cmd, &info);
+
+ DEBUG ("migrate_set_speed reply: %s", info);
+ free (info);
+ }
+
+ /* Issue the migrate command. */
+ safe_uri = qemudEscapeMonitorArg (uri);
+ if (!safe_uri) {
+ qemudReportError (dom->conn, dom, NULL, VIR_ERR_SYSTEM_ERROR,
+ "%s", strerror (errno));
+ return -1;
+ }
+ snprintf (cmd, sizeof cmd, "migrate \"%s\"", safe_uri);
+ free (safe_uri);
+
+ if (qemudMonitorCommand (driver, vm, cmd, &info) < 0) {
+ qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+ "%s", _("migrate operation failed"));
+ return -1;
+ }
+
+ DEBUG ("migrate reply: %s", info);
+ free (info);
+
+ /* Clean up the source domain. */
+ qemudShutdownVMDaemon (dom->conn, driver, vm);
+ if (!vm->configFile[0])
+ qemudRemoveInactiveVM (driver, vm);
+
+ return 0;
+}
Need to use VIR_FREE() in various places here.
Dan,
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
6015
days inactive
6015
days old
2 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Richard W.M. Jones