On 12/02/14 17:13, Michal Privoznik wrote:
> Based on previous commit, we can now precreate missing volumes. While
> digging out the functionality from storage driver would be nicer, if
> you've seen the code it's nearly impossible. So I'm going from the
> other end:
>
> 1) For given disk target, disk path is looked up.
> 2) For the disk path, storage pool is looked up, a volume XML is
> constructed and then passed to virStorageVolCreateXML() which has all
> the knowledge how to create raw images, (encrypted) qcow(2) images,
> etc.
>
> One of the advantages of this approach is, we don't have to care about
> image conversion - qemu does that for us. So for instance, users can
> transform qcow2 into raw on migration (if the correct XML is passed to
> the migration API).
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/libvirt-domain.c | 3 +
> src/qemu/qemu_migration.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 162 insertions(+)
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 01bd9e4..8333183 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -4312,6 +4312,9 @@ virDomainMigrateToURI(virDomainPtr domain,
> * If you want to copy non-shared storage within migration you
> * can use either VIR_MIGRATE_NON_SHARED_DISK or
> * VIR_MIGRATE_NON_SHARED_INC as they are mutually exclusive.
> + * As of 1.2.11 disks of some types ('file' and 'volume') are
> + * precreated automatically, if there's a pool defined on the
> + * destination for the disk path.
> *
> * If a hypervisor supports changing the configuration of the guest
> * during migration, the @dxml parameter specifies the new config
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 36b7e43..11e2a2c 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -58,6 +58,7 @@
> #include "virtypedparam.h"
> #include "virprocess.h"
> #include "nwfilter_conf.h"
> +#include "storage/storage_driver.h"
>
> #define VIR_FROM_THIS VIR_FROM_QEMU
>
> @@ -1451,6 +1452,161 @@ qemuMigrationRestoreDomainState(virConnectPtr conn, virDomainObjPtr vm)
> return ret;
> }
>
> +
> +static int
> +qemuMigrationPrecreateDisk(virConnectPtr conn,
> + virDomainDiskDefPtr disk,
> + unsigned long long capacity)
> +{
> + int ret = -1;
> + virStoragePoolPtr pool = NULL;
> + virStorageVolPtr vol = NULL;
> + char *volName = NULL, *basePath = NULL;
> + char *volStr = NULL;
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> + const char *format = NULL;
> + unsigned int flags = 0;
> +
> + VIR_DEBUG("Precreate disk type=%s", virStorageTypeToString(disk->src->type));
> +
> + switch ((virStorageType) disk->src->type) {
> + case VIR_STORAGE_TYPE_FILE:
> + if (!virDomainDiskGetSource(disk)) {
> + VIR_DEBUG("Dropping sourceless disk '%s'",
> + disk->dst);
> + return 0;
> + }
> +
> + if (VIR_STRDUP(basePath, disk->src->path) < 0)
> + goto cleanup;
> +
> + if (!(volName = strrchr(basePath, '/'))) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("malformed disk path: %s"),
> + disk->src->path);
> + goto cleanup;
> + }
> +
> + *volName = '\0';
> + volName++;
> +
> + if (!(pool = storagePoolLookupByTargetPath(conn, basePath)))
> + goto cleanup;
> + format = virStorageFileFormatTypeToString(disk->src->format);
> + if (disk->src->format == VIR_STORAGE_FILE_QCOW2)
> + flags |= VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA;
> + break;
> +
> + case VIR_STORAGE_TYPE_VOLUME:
> + if (!(pool = virStoragePoolLookupByName(conn, disk->src->srcpool->pool)))
> + goto cleanup;
> + format = virStorageFileFormatTypeToString(disk->src->format);
> + volName = disk->src->srcpool->volume;
> + if (disk->src->format == VIR_STORAGE_FILE_QCOW2)
> + flags |= VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA;
> + break;
> +
> + case VIR_STORAGE_TYPE_BLOCK:
> + case VIR_STORAGE_TYPE_DIR:
> + case VIR_STORAGE_TYPE_NETWORK:
> + case VIR_STORAGE_TYPE_NONE:
> + case VIR_STORAGE_TYPE_LAST:
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot precreate storage for disk type '%s'"),
> + virStorageTypeToString(disk->src->type));
> + goto cleanup;
> + break;
> + }
> +
> + if ((vol = virStorageVolLookupByName(pool, volName))) {
> + VIR_DEBUG("Skipping creation of already existing volume of name '%s'",
> + volName);
> + ret = 0;
> + goto cleanup;
> + }
> +
> + virBufferAddLit(&buf, "<volume>\n");
> + virBufferAdjustIndent(&buf, 2);
> + virBufferEscapeString(&buf, "<name>%s</name>\n", volName);
> + virBufferAsprintf(&buf, "<capacity>%llu</capacity>\n", capacity);
> + virBufferAddLit(&buf, "<target>\n");
> + virBufferAdjustIndent(&buf, 2);
> + virBufferAsprintf(&buf, "<format type='%s'/>\n", format);
> + virBufferAdjustIndent(&buf, -2);
> + virBufferAddLit(&buf, "</target>\n");
> + virBufferAdjustIndent(&buf, -2);
> + virBufferAddLit(&buf, "</volume>\n");
> +
> + if (!(volStr = virBufferContentAndReset(&buf))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("unable to create volume XML"));
> + goto cleanup;
> + }
> +
> + if (!(vol = virStorageVolCreateXML(pool, volStr, flags)))
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(basePath);
> + VIR_FREE(volStr);
> + if (vol)
> + virStorageVolFree(vol);
> + if (pool)
> + virStoragePoolFree(pool);
I just ACKed John's series that forbids the virStorageVolFree and
virStoragePoolFree functions in our codebase. You'll need to change them
to virObjectUnref().
> + return ret;
> +}
> +
> +
> +static int
> +qemuMigrationPrecreateStorage(virConnectPtr conn,
> + virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> + virDomainObjPtr vm,
> + qemuMigrationCookieNBDPtr nbd)
> +{
> + int ret = -1;
> + size_t i = 0;
> +
> + if (!nbd || !nbd->ndisks)
> + return 0;
> +
> + for (i = 0; i < nbd->ndisks; i++) {
> + virDomainDiskDefPtr disk;
> + int indx;
> + const char *diskSrcPath;
> +
> + VIR_DEBUG("Looking up disk target '%s' (capacity=%lluu)",
> + nbd->disks[i].target, nbd->disks[i].capacity);
Following up on 3/4: here and below you use the "target" field without
verifying that it was provided.
Sounds like bikeshedding, but if we can be robust here, we should be. I
think the check should be in 3/4
> +
> + if ((indx = virDomainDiskIndexByName(vm->def,
> + nbd->disks[i].target, false)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unable to find disk by target: %s"),
> + nbd->disks[i].target);
> + goto cleanup;
> + }
> +
> + disk = vm->def->disks[indx];
> + diskSrcPath = virDomainDiskGetSource(disk);
> +
> + if (disk->src->shared || disk->src->readonly ||
> + (diskSrcPath && virFileExists(diskSrcPath))) {
> + /* Skip shared, read-only and already existing disks. */
> + continue;
> + }
> +
> + VIR_DEBUG("Proceeding with disk source %s", NULLSTR(diskSrcPath));
> +
> + if (qemuMigrationPrecreateDisk(conn, disk, nbd->disks[i].capacity) < 0)
> + goto cleanup;
> + }
> +
> + ret = 0;
> + cleanup:
> + return ret;
> +}
> +
> +
> /**
> * qemuMigrationStartNBDServer:
> * @driver: qemu driver
ACK if you change the virStorage*Free funcs.
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list