[libvirt PATCH 3/3] qemu: Extra check for NBD URI being specified

[Martin Kletzander]

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 at 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
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201217/6b25d2b8/attachment-0001.sig>