Re: [libvirt] [PATCH v2 1/2] qemu: Check for existing hostdev address for cold attach device

[John Ferlan]

On 06/13/2018 09:42 PM, Laine Stump wrote:
> On 06/13/2018 07:16 AM, John Ferlan wrote:
>>
>> On 06/13/2018 04:15 AM, Ján Tomko wrote:
>>> On Tue, Jun 12, 2018 at 10:32:06AM -0400, John Ferlan wrote:
 Add a check during qemuDomainAttachDeviceConfig whether the
 new/incoming  device would have an existing
  already and if so fail the attach. This can happen
 if two hostdev's are added supplying the same address or
 if the new hostdev address could possibly be a duplicate
 of an existing SCSI .

 Signed-off-by: John Ferlan 
 ---
 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 f0fb806fcd..ae8e0e898a 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -8015,6 +8015,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 check feels out of place here. We should be checking for that in
>>> postParse. Do we have the same problem on domain startup?
>>>
>> I've avoided new post parse checks due to the domain disappearing phenomena.
> 
> The difference with this is that any existing domain with this error
> would have failed to start qemu in previous versions, so it surely would
> have been fixed. And assignment of addresses to PCI devices that are
> "coldplugged" (ie those that are added individually with AttachDevice,
> but that are only added to the persistent config, *not* to the running
> guest, ie "CONFIG") is handled by a manually added call to
> virDomainDefPostParse() that is called directly from
> qemuDomainAttachDeviceConfig:
> 
>     qemuDomainAttachDeviceConfig
>   virDomainDefPostParse
>     qemuDomainDefAssignAddresses (aka assignAddressCallback)
>   qemuDomainAssignAddresses
> 
> Is there a reason why SCSI addresses aren't assigned by the same
> function. If we did that then at least everyone would have the same set
> of problems :-P)

Not 100% sure of any reason other than perhaps the "relationship"
between SCSI hostdev and SCSI disk. The generated address for a 
is based on the def->dst (see virDiskNameToIndex). A dst of "sda" would
equate to controller=0 and unit=0. A  will "find" a spot, but
defaults to c=0 & u=0 (virDomainHostdevAssignAddress). There is some
collision detection (virDomainDriveAddressIsUsedBy{Disk|Hostdev} and
virDomainDefCheckDuplicateDriveAddresses), but the timing of when those
checks can be called and when the above check is made is the issue.

During parse virDomainHostdevAssignAddress is called if an address is
not provided. That calls virDomainControllerSCSINextUnit to ensure no
duplicate address is generated by checking both existing SCSI disk and
hostdev's.

If an address is provided, the virDomainHostdevDefPostParse can only
check existing SCSI disks since virDomainHostdevInsert called during
qemuDomainAttachDeviceConfig would have put that address on the hostdev
list and calling virDomainSCSIDriveAddressIsUsed during post parse
instead of just virDomainDriveAddressIsUsedByDisk would result in a
conflict with itself.

So prior to inserting into the list calling virDomainDefHasDeviceAddress
(just like VIR_DOMAIN_DEVICE_RNG does in the same method) ensures the
about to be added hostdev doesn't conflict with any existing hostdev
because the parse code didn't make that check and checking for that
conflict in post parse would cause

And yes, if a duplicate was successfully inserted without the check,
then startup would have a failure since the validate path would call
virDomainDefCheckDuplicateDriveAddresses. If the "decision" is that we
can allow the start to fail when someone provides duplicate addresses,
then that's fine I can drop this patch. Although perhaps a comment in
qemuDomainAttachDeviceConfig would help the next person (or even myself,
because I'm sure to forget and see this again).

> 
> On the other hand, for hotplugged PCI devices ("LIVE"), both assigning
> addresses to devices with no supplied address, and checking for
> duplicate addresses is handled by qemuDomainEnsurePCIAddress (which
> calls virDomainPCIAddressEnsureAddr. No, I did not name either of them)
> - that is called directly from qemuDomainAttach${Blah}Device in
> qemu_hotplug.c.
> 
> So in both cases, nothing associated with *PCI* device addresses is done
> during the *Device* post parse callback, although in the case of CONFIG,
> addresses are assigned/checked 

Re: [libvirt] [PATCH v2 1/2] qemu: Check for existing hostdev address for cold attach device

[Laine Stump]

On 06/13/2018 07:16 AM, John Ferlan wrote:
>
> On 06/13/2018 04:15 AM, Ján Tomko wrote:
>> On Tue, Jun 12, 2018 at 10:32:06AM -0400, John Ferlan wrote:
>>> Add a check during qemuDomainAttachDeviceConfig whether the
>>> new/incoming  device would have an existing
>>>  already and if so fail the attach. This can happen
>>> if two hostdev's are added supplying the same address or
>>> if the new hostdev address could possibly be a duplicate
>>> of an existing SCSI .
>>>
>>> Signed-off-by: John Ferlan 
>>> ---
>>> 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 f0fb806fcd..ae8e0e898a 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -8015,6 +8015,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 check feels out of place here. We should be checking for that in
>> postParse. Do we have the same problem on domain startup?
>>
> I've avoided new post parse checks due to the domain disappearing phenomena.

The difference with this is that any existing domain with this error
would have failed to start qemu in previous versions, so it surely would
have been fixed. And assignment of addresses to PCI devices that are
"coldplugged" (ie those that are added individually with AttachDevice,
but that are only added to the persistent config, *not* to the running
guest, ie "CONFIG") is handled by a manually added call to
virDomainDefPostParse() that is called directly from
qemuDomainAttachDeviceConfig:

    qemuDomainAttachDeviceConfig
  virDomainDefPostParse
    qemuDomainDefAssignAddresses (aka assignAddressCallback)
  qemuDomainAssignAddresses

Is there a reason why SCSI addresses aren't assigned by the same
function. If we did that then at least everyone would have the same set
of problems :-P)

On the other hand, for hotplugged PCI devices ("LIVE"), both assigning
addresses to devices with no supplied address, and checking for
duplicate addresses is handled by qemuDomainEnsurePCIAddress (which
calls virDomainPCIAddressEnsureAddr. No, I did not name either of them)
- that is called directly from qemuDomainAttach${Blah}Device in
qemu_hotplug.c.

So in both cases, nothing associated with *PCI* device addresses is done
during the *Device* post parse callback, although in the case of CONFIG,
addresses are assigned/checked by a "POST-postparse" manual call to the
*Domain* PostParse callback.

Sigh. I was hoping to arrive at some nugget of good information that
would point to the best solution. Instead I've just pointed out how
convoluted everything is, and given myself more ideas of other possible
bugs. Maybe if I look at it again in the morning with a fresh brain...



>
> But no, I don't believe the same problem exists there. How would you
> suggest reproducing that?
>
> If one virsh edit's their domain adding two hostdev's with the same
> , then virDomainDefValidateInternal catches that with the call
> to virDomainDefCheckDuplicateDriveAddresses.  With this patch if one
> calls attach-device --config with a duplicated , then this
> check will catch that.
>
> This is similar to VIR_DOMAIN_DEVICE_RNG processing in the same code.
>
> John
>
>> Jano
>>
>>>     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
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>

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

Re: [libvirt] [PATCH v2 1/2] qemu: Check for existing hostdev address for cold attach device

[John Ferlan]

On 06/13/2018 04:15 AM, Ján Tomko wrote:
> On Tue, Jun 12, 2018 at 10:32:06AM -0400, John Ferlan wrote:
>> Add a check during qemuDomainAttachDeviceConfig whether the
>> new/incoming  device would have an existing
>>  already and if so fail the attach. This can happen
>> if two hostdev's are added supplying the same address or
>> if the new hostdev address could possibly be a duplicate
>> of an existing SCSI .
>>
>> Signed-off-by: John Ferlan 
>> ---
>> 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 f0fb806fcd..ae8e0e898a 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -8015,6 +8015,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 check feels out of place here. We should be checking for that in
> postParse. Do we have the same problem on domain startup?
> 

I've avoided new post parse checks due to the domain disappearing phenomena.

But no, I don't believe the same problem exists there. How would you
suggest reproducing that?

If one virsh edit's their domain adding two hostdev's with the same
, then virDomainDefValidateInternal catches that with the call
to virDomainDefCheckDuplicateDriveAddresses.  With this patch if one
calls attach-device --config with a duplicated , then this
check will catch that.

This is similar to VIR_DOMAIN_DEVICE_RNG processing in the same code.

John

> Jano
> 
>>     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

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

Re: [libvirt] [PATCH v2 1/2] qemu: Check for existing hostdev address for cold attach device

[Ján Tomko]

On Tue, Jun 12, 2018 at 10:32:06AM -0400, John Ferlan wrote:

Add a check during qemuDomainAttachDeviceConfig whether the
new/incoming  device would have an existing
 already and if so fail the attach. This can happen
if two hostdev's are added supplying the same address or
if the new hostdev address could possibly be a duplicate
of an existing SCSI .

Signed-off-by: John Ferlan 
---
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 f0fb806fcd..ae8e0e898a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8015,6 +8015,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 check feels out of place here. We should be checking for that in
postParse. Do we have the same problem on domain startup?

Jano


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


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 1/2] qemu: Check for existing hostdev address for cold attach device

[John Ferlan]

Add a check during qemuDomainAttachDeviceConfig whether the
new/incoming  device would have an existing
 already and if so fail the attach. This can happen
if two hostdev's are added supplying the same address or
if the new hostdev address could possibly be a duplicate
of an existing SCSI .

Signed-off-by: John Ferlan 
---
 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 f0fb806fcd..ae8e0e898a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8015,6 +8015,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