qemu: Check for existing hostdev address for cold attach device

[libvirt] [PATCH] qemu: Check for existing hostdev address for cold attach device

Posted by John Ferlan 5 years, 10 months ago

https://bugzilla.redhat.com/show_bug.cgi?id=1559867

Add a check if the incoming <hostdev ...> with specified <address>
already exists and if so fail the attach.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/qemu/qemu_driver.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 38ea865ce3..7b4cdbcdcf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8014,6 +8014,12 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
                            _("device is already in the domain configuration"));
             return -1;
         }
+        if (dev->data.hostdev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+            virDomainDefHasDeviceAddress(vmdef, dev->data.hostdev->info)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("a device with the same address already exists "));
+            return -1;
+        }
         if (virDomainHostdevInsert(vmdef, hostdev))
             return -1;
         dev->data.hostdev = NULL;
-- 
2.14.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Check for existing hostdev address for cold attach device

Posted by Ján Tomko 5 years, 10 months ago

On Wed, Jun 06, 2018 at 10:44:10AM -0400, John Ferlan wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1559867
>
>Add a check if the incoming <hostdev ...> with specified <address>
>already exists and if so fail the attach.
>

Looking at the reproducer steps, the bug talks about libvirt
auto-generating a duplicate address, not user-specified input.

>Signed-off-by: John Ferlan <jferlan@redhat.com>
>---
> src/qemu/qemu_driver.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index 38ea865ce3..7b4cdbcdcf 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -8014,6 +8014,12 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
>                            _("device is already in the domain configuration"));
>             return -1;
>         }
>+        if (dev->data.hostdev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
>+            virDomainDefHasDeviceAddress(vmdef, dev->data.hostdev->info)) {
>+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+                           _("a device with the same address already exists "));
>+            return -1;
>+        }

This does not fix the problem of autogenerating the address in any way
(which seems to work when I call attach --config while the domain is not
running).

Also, if we needded additional checks for guest addresses on attach,
we should either do it for all devices (and drop the calling of
per-domain postParse after adding a single device), or take care of the
conflicts in postParse.

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Check for existing hostdev address for cold attach device

Posted by John Ferlan 5 years, 10 months ago

On 06/11/2018 10:42 AM, Ján Tomko wrote:
> On Wed, Jun 06, 2018 at 10:44:10AM -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1559867
>>
>> Add a check if the incoming <hostdev ...> with specified <address>
>> already exists and if so fail the attach.
>>
> 
> Looking at the reproducer steps, the bug talks about libvirt
> auto-generating a duplicate address, not user-specified input.
> 

Oh, right - perhaps should have been clearer what's happening with the
<hostdev> addition. While/when not provided, the post parse processing
for a hostdev will automagically generate the address... of course in
the code I was looking at that <address> was there and essentially
copied the check from VIR_DOMAIN_DEVICE_RNG and that's what was on my
mind when I wrote the terse commit message.

>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>> src/qemu/qemu_driver.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 38ea865ce3..7b4cdbcdcf 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -8014,6 +8014,12 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr
>> vmdef,
>>                            _("device is already in the domain
>> configuration"));
>>             return -1;
>>         }
>> +        if (dev->data.hostdev->info->type !=
>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
>> +            virDomainDefHasDeviceAddress(vmdef,
>> dev->data.hostdev->info)) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                           _("a device with the same address already
>> exists "));
>> +            return -1;
>> +        }
> 
> This does not fix the problem of autogenerating the address in any way
> (which seems to work when I call attach --config while the domain is not
> running).

True, but it does avoid the attempt to attach a duplicated address -
still I see now it's the autogeneration of a <hostdev> address that's
broken when a domain is running and the attachment is for the config.

When just looking at the virDomainHostdevAssignAddress code one could
believe sure this should provide the right answer, but unlike the SCSI
option for virDomainDiskDefAssignAddress which makes use of the def->dst
in order to "seed" the @idx for the subsequent equations the hostdev
code doesn't have a name so it linearly looks for an available address
during post parse processing.

In this instance however, it's using the running domain's vm->def
instead of the persistent vm->newDef, thus obtaining the same answer
twice with the second one being incorrect.

Updated patches will be posted soon.

> 
> Also, if we needded additional checks for guest addresses on attach,
> we should either do it for all devices (and drop the calling of
> per-domain postParse after adding a single device), or take care of the
> conflicts in postParse.
> 

Not 100% sure what you mean by this. Still the processing of a SCSI
hostdev relies not only on existing SCSI hostdev's, but also any
existing SCSI disk - so making the correct calculation is challenging.
Whether that same "need" exists for other cold attach devices (net,
lease, controller, chrdev, fs, rng, memory, redirdev, shmem, watchdog,
and vsock) would seem to extend beyond the bounds of this bz.

Thanks for the review -

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list