On Thu, Dec 17, 2020 at 11:15:11AM +0100, Peter Krempa wrote:
On Wed, Dec 16, 2020 at 12:19:28 +0100, Martin Kletzander wrote:
> It must be used when migration URI uses `unix:` transport because otherwise we
> cannot just guess where to connect for disk migration.
>
>
https://bugzilla.redhat.com/show_bug.cgi?id=1638889
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 5f0fb0a55fee..9caaa0723720 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11503,6 +11503,16 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn,
> QEMU_MIGRATION_DESTINATION)))
> return -1;
>
> + if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) ||
> + nmigrate_disks > 0) {
Is this to check that NBD is used? I think that's not enough as
VIR_MIGRATE_NON_SHARED_DISK/INC doesn't necessarily guarantee that NBD
is used. It also depends on whether TUNELLED and other possible flags
are in the mix too. We might need a helper function for that.
Thanks for the review. It would be nicer to have it extracted into a separate
function for that, I agree. Tunnelled migration would fortunately not work with
`--migrate-uri`, so we should be safe there (although the logic lends itself for
slightly more readable condition). I went through both QEMU_MIGRATION_FLAGS and
QEMU_MIGRATION_PARAMETERS and I believe these are the only cases that might
cause an issue. I'll move the condition to a separate function and I'll gladly
accept any suggestions for improvements.
Also 'nmigrate_disks' AFAIK requires that
VIR_MIGRATE_NON_SHARED_DISK
is used so it's redundant here.
I only did it based on what virsh does and `virsh migrate --migrate-disks vda
...` without any `--copy-storage-*` option worked for me and it looked like
virsh does not apply additional logic on top of this particular set of options.
> + if (uri_in && STRPREFIX(uri_in,
"unix:") && !nbdURI) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("NBD URI must be supplied when "
> + "migration URI uses UNIX transport
method"));
Please don't split error messages it's hard to grep for them if they are
split.
Are we finally enforcing this? I'd be glad to, I guess I'm stuck in the old ways
;)
> + return -1;
> + }
> + }
> +
> if (nbdURI && nbdPort) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> _("Both port and URI requested for disk migration
"
> @@ -11743,6 +11753,7 @@ qemuDomainMigratePerform3Params(virDomainPtr dom,
> &persist_xml) < 0)
> goto cleanup;
>
> +
Spurious newline addition.
> if (nbdURI && nbdPort) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> _("Both port and URI requested for disk migration
"
> @@ -11766,6 +11777,16 @@ qemuDomainMigratePerform3Params(virDomainPtr dom,
> if (nmigrate_disks < 0)
> goto cleanup;
>
> + if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) ||
> + nmigrate_disks > 0) {
> + if (uri && STRPREFIX(uri, "unix:") && !nbdURI) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("NBD URI must be supplied when "
> + "migration URI uses UNIX transport
method"));
> + return -1;
> + }
> + }
> +
> if (!(migParams = qemuMigrationParamsFromFlags(params, nparams, flags,
> QEMU_MIGRATION_SOURCE)))
> goto cleanup;
> --
> 2.29.2
>