On 12/01/14 15:57, 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/qemu/qemu_migration.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 150 insertions(+)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 26df9c7..9ccfee2 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
>
> @@ -1454,6 +1455,152 @@ 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, '/'))) {
For finding the base path you'd normally want to use mdir_name(), but
then you wouldn't be able to extract the volume name too.
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("malformed disk path: %s"),
> + disk->src->path);
> + goto cleanup;
> + }
> +
> + *volName = '\0';
> + volName++;
I guess this is good enough then.
> +
> + if (!(pool = storagePoolLookupByPath(conn, basePath))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unable to find pool to %s"),
"unable to find pool for path '%s'" ?
> + 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,
> + _("unsupported disk type '%s'"),
Possibly a more helpful message: "cannot precreate storage for disk type
'%s' ?
> + virStorageTypeToString(disk->src->type));
> + goto cleanup;
> + break;
> + }
> +
> + virBufferAddLit(&buf, "<volume>\n");
> + virBufferAdjustIndent(&buf, 2);
> + virBufferAsprintf(&buf, "<name>%s</name>", volName);
You need to use virBufferEscape for the name
> + virBufferAsprintf(&buf, "<capacity>%llu</capacity>\n", capacity);
> + virBufferAddLit(&buf, "<target>\n");
> + virBufferAdjustIndent(&buf, 2);
> + virBufferAsprintf(&buf, "<format type='%s'/>\n", format);
Format is okay as we control the strings.
> + 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);
> + 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);
> +
> + 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);
> +
> + VIR_DEBUG("Proceeding with disk source %s", NULLSTR(diskSrcPath));
The storage pre-creation should be skipped in case the target file
already exists as previously the user had to pre-create the file
manually. In case we'd do this unconditionally we might break the case
somebody is already precreating it.
> +
> + if (qemuMigrationPrecreateDisk(conn, disk, nbd->disks[i].capacity) < 0)
> + goto cleanup;
> + }
> +
> + ret = 0;
> + cleanup:
> + return ret;
> +}
> +
> +
> /**
> * qemuMigrationStartNBDServer:
> * @driver: qemu driver
> @@ -2838,6 +2985,9 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
> goto cleanup;
> }
>
> + if (qemuMigrationPrecreateStorage(dconn, driver, vm, mig->nbd) < 0)
> + goto cleanup;
> +
> if (qemuMigrationJobStart(driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN) < 0)
> goto cleanup;
> qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PREPARE);
>
Otherwise looks good code-wise, but I don't see any mention of this new
feature in the documentation.
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list