On 05/26/2015 09:01 AM, Michal Privoznik wrote:
> From: Pavel Boldin <pboldin@xxxxxxxxxxxx>
>
> Implement a `migrate_disks' parameters for the QEMU driver. This multi-
> value parameter can be used to explicitly specify what block devices
> are to be migrated using the NBD server. Tunnelled migration using NBD
> is to be done.
>
> Signed-off-by: Pavel Boldin <pboldin@xxxxxxxxxxxx>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> include/libvirt/libvirt-domain.h | 9 ++
> src/qemu/qemu_driver.c | 72 +++++++++---
> src/qemu/qemu_migration.c | 245 ++++++++++++++++++++++++++++-----------
> src/qemu/qemu_migration.h | 24 ++--
> 4 files changed, 258 insertions(+), 92 deletions(-)
>
Didn't see much major - just a NIT or two... Also noted sometimes the
order of arguments is nmigrate_disks, migrate_disks and others it's
reversed (qemuMigrationBegin and virTypedParamsPickStrings). It's not a
big deal to me personally, but there are those that do ;-)
...
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 279b43f..5a8057e 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1579,12 +1579,34 @@ qemuMigrationPrecreateDisk(virConnectPtr conn,
> return ret;
> }
>
> +static bool
> +qemuMigrateDisk(virDomainDiskDef const *disk,
> + size_t nmigrate_disks, const char **migrate_disks)
NIT: 3rd arg on own line
> +{
> + size_t i;
> + /* Check if the disk alias is in the list */
> + if (nmigrate_disks) {
> + for (i = 0; i < nmigrate_disks; i++) {
> + if (STREQ(disk->dst, migrate_disks[i]))
> + return true;
> + }
> + return false;
> + }
> +
> + /* Default is to migrate only non-shared non-readonly disks
> + * with source */
> + return !disk->src->shared && !disk->src->readonly &&
> + virDomainDiskGetSource(disk);
> +}
> +
>
...
>
> @@ -2710,6 +2734,8 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
> const char *dname,
> char **cookieout,
> int *cookieoutlen,
> + size_t nmigrate_disks,
> + const char **migrate_disks,
> unsigned long flags)
> {
> char *rv = NULL;
> @@ -2721,9 +2747,11 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
> bool abort_on_error = !!(flags & VIR_MIGRATE_ABORT_ON_ERROR);
>
> VIR_DEBUG("driver=%p, vm=%p, xmlin=%s, dname=%s,"
> - " cookieout=%p, cookieoutlen=%p, flags=%lx",
> + " cookieout=%p, cookieoutlen=%p,"
> + " nmigrate_disks=%zu, migrate_disks=%p, flags=%lx",
> driver, vm, NULLSTR(xmlin), NULLSTR(dname),
> - cookieout, cookieoutlen, flags);
> + cookieout, cookieoutlen, nmigrate_disks,
> + migrate_disks, flags);
>
> if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> goto cleanup;
> @@ -2738,17 +2766,54 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
> if (!qemuMigrationIsAllowed(driver, vm, NULL, true, abort_on_error))
> goto cleanup;
>
> - if (!(flags & VIR_MIGRATE_UNSAFE) && !qemuMigrationIsSafe(vm->def))
> + if (!(flags & VIR_MIGRATE_UNSAFE) &&
> + !qemuMigrationIsSafe(vm->def, nmigrate_disks, migrate_disks))
> goto cleanup;
>
> - if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) &&
> - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_MIRROR)) {
> - /* TODO support NBD for TUNNELLED migration */
> - if (flags & VIR_MIGRATE_TUNNELLED) {
> - VIR_WARN("NBD in tunnelled migration is currently not supported");
> - } else {
> - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
> - priv->nbdPort = 0;
> + if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) {
> + bool has_drive_mirror = virQEMUCapsGet(priv->qemuCaps,
> + QEMU_CAPS_DRIVE_MIRROR);
> +
> + if (nmigrate_disks) {
> + if (has_drive_mirror) {
> + size_t i, j;
> +
> + /* Check user requested only known disk targets. */
> + for (i = 0; i < nmigrate_disks; i++) {
> + for (j = 0; j < vm->def->ndisks; j++) {
> + if (STREQ(vm->def->disks[j]->dst, migrate_disks[i]))
> + break;
> + }
> +
> + if (j == vm->def->ndisks) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("disk target %s not found"),
> + migrate_disks[i]);
> + goto cleanup;
> + }
> + }
> +
> + if (flags & VIR_MIGRATE_TUNNELLED) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("Selecting disks to migrate is not "
> + "implemented for tunnelled migration"));
> + goto cleanup;
> + }
NIT: this check could be done prior to loops...
> + } else {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("qemu does not support drive-mirror command"));
NIT: s/qemu/this qemu binary
> + goto cleanup;
A level of indention could be removed by
if (nmigrate_disks) {
size_t i, j;
if (!has_drive_mirror) {
virReportError()
goto cleanup;
}
if (flags & VIR_MIGRATE_TUNNELLED) {
virReportError()
goto cleanup;
}
for (...) {}
}
Rest seemed OK to me, too.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list