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