On Mon, Sep 03, 2012 at 02:23:24PM +0800, liguang wrote:
> allow migration even domain isn't active by
> inserting some stubs to tunnel migration path.
>
> Signed-off-by: liguang <lig.fnst@xxxxxxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 2 +-
> src/qemu/qemu_migration.c | 181
> +++++++++++++++++++++++++++++++++++++++++++--
> src/qemu/qemu_migration.h | 3 +-
> 3 files changed, 178 insertions(+), 8 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d74bf52..00ca211 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -9779,7 +9779,7 @@ qemuDomainMigratePrepareTunnel3(virConnectPtr
> dconn,
>
> virCheckFlags(QEMU_MIGRATION_FLAGS, -1);
>
> - if (!dom_xml) {
> + if (!dom_xml && !(flags & VIR_MIGRATE_OFFLINE)) {
This is bogus, XML should be required no matter what.
> @@ -721,6 +747,12 @@ qemuMigrationBakeCookie(qemuMigrationCookiePtr mig,
> qemuMigrationCookieAddPersistent(mig, dom) < 0)
> return -1;
>
> + if (flags & QEMU_MIGRATION_COOKIE_OFFLINE) {
> + mig->flags |= QEMU_MIGRATION_COOKIE_OFFLINE;
> + mig->offline = 1;
'mig->offline' is just duplicating what's already tracked in flags.
> @@ -1307,6 +1339,27 @@ qemuMigrationPrepareAny(struct qemud_driver
> *driver,
> /* Domain starts inactive, even if the domain XML had an id field.
> */
> vm->def->id = -1;
>
> + if (tunnel) {
> + if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein,
> cookieinlen,
> +
> QEMU_MIGRATION_COOKIE_OFFLINE)))
> + return ret;
> + else if (mig->offline) {
> + char *file, *str, *tmp = NULL;
> + ret = 0;
> + for (str = mig->mig_file; ; str = NULL) {
> + file = strtok_r(str, " ", &tmp);
Encoding multiple filenames in a single string like this is pure evil and
a security flaw waiting to happen. eg, consider how well this works if the
guest filename contains a space character.
> + if (file == NULL)
> + break;
> + if (virFDStreamCreateFile(st, file, 0, 0, O_WRONLY, 0)
> < 0) {
Re-using the same virStreamPtr object for multiple files is an abuse
of the stream API. If this actually works it is by pure luck and is
certainly not intended to work.
> +/*
> + * do offline migration
> + */
> +static int doMigrateOffline(struct qemud_driver *driver,
> + virConnectPtr dconn,
> + virDomainObjPtr vm,
> + const char *cookiein,
> + int cookieinlen,
> + char **cookieout,
> + int *cookieoutlen,
> + unsigned long flags,
> + const char *dom_xml,
> + const char *dname,
> + virStreamPtr st,
> + unsigned long resource)
> +{
> + xmlDocPtr xml = NULL;
> + xmlXPathContextPtr ctxt = NULL;
> + xmlNodePtr *disks = NULL;
> + qemuMigrationCookiePtr mig = NULL;
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> + int i = 0, cn = 0, fd = -1, ret = -1;
> + char *src[] = {NULL}, *files = NULL;
> +
> + VIR_DEBUG("driver=%p, vm=%p, st=%p, cookiein=%s, cookieinlen=%d, "
> + "cookieout=%p, cookieoutlen=%p, dom_xml=%s,"
> + "dname=%s, flags=%lx, resource=%lu",
> + driver, vm, st, NULLSTR(cookiein), cookieinlen,
> + cookieout, cookieoutlen, dom_xml, dname, flags,
> resource);
> +
> + xml = virXMLParseStringCtxt(dom_xml, _("(domain_definition)"),
> &ctxt);
> + if (!xml) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("can't parse dom_xml for offline migration
> \n"));
> + goto cleanup;
> + }
> + cn = virXPathNodeSet("./devices/disk", ctxt, &disks);
> + if (cn < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Fail to get disk node\n"));
> + goto cleanup;
> + }
> + cn = 1;
> +
> + for (i = 0 ; i < cn ; i++) {
> + ctxt->node = disks[i];
> + src[i] = virXPathString("string(./source/@file"
> + "|./source/@dir"
> + "|./source/@name)", ctxt);
> + virBufferAsprintf(&buf, "%s ", src[i]);
> + }
Writing custom XML parsing code is forbidden - we have APIs for doing
this.
I don't really know how you're expecting this work at all when
'source/@dir' is in the XML, since this refers to a directory tree,
not a single file & you're not recursively copying the directory.
Likewise this can't possibly work with ./source/@name since that
refers to a network block device which you can't simply open as
if it were a local file.
> + if (qemuMigrationBakeCookie(mig, driver, vm,
> + cookieout, cookieoutlen,
> + QEMU_MIGRATION_COOKIE_OFFLINE) < 0)
> + goto cleanup;
> +
> + cookiein = *cookieout;
> + cookieinlen = *cookieoutlen;
> + cookieout = NULL;
> + cookieoutlen = 0;
> +
> + qemuDomainObjEnterRemoteWithDriver(driver, vm);
> + ret = dconn->driver->domainMigratePrepareTunnel3
> + (dconn, st, cookiein, cookieinlen,
> + cookieout, cookieoutlen,
> + flags, dname, resource, dom_xml);
> + qemuDomainObjExitRemoteWithDriver(driver, vm);
> + if (ret == -1)
> + goto cleanup;
> +
> + for (i = 0; i < cn; i++) {
> + if ((fd = open(src[i], O_RDONLY)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s %s",
> + _("Fail to open file for offline migration
> \n"), src[i]);
> + goto cleanup;
> + }
> + if (virStreamSendAll(st, doReadFile, &fd) < 0)
> + goto cleanup;
> + if (VIR_CLOSE(fd) < 0)
> + goto cleanup;
> + }
Again re-using the same stream instance for multiple files is not
allowed.
This doesn't give any way to control whether all disks images should
be copied or not.
It completely ignores the virDomainMigrate APIs flags which let the
app specify whether they want storage copied or not.
> @@ -2557,7 +2727,7 @@ static int doPeer2PeerMigrate(struct qemud_driver
> *driver,
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("guest unexpectedly quit"));
> - goto cleanup;
> + flags |= VIR_MIGRATE_OFFLINE;
> }
So if the app issues virDomainMigrate and the guest they were trying
to migrate has just quit, they will now suddenly find themselves copying
all disks images. Not only is this is a change in semantic behaviour of
the API, but it is not even desirable IMHO. The common case is that the
deployments do have shared storage, so will not want to copy disk images.
In summary I don't think this patch or any like it is suitable for
libvirt.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list