9:34 a.m.
This is an alternative approach to:
https://www.redhat.com/archives/libvir-list/2019-March/msg01111.html
which caused regression to which a proposed fix is here:
https://www.redhat.com/archives/libvir-list/2019-April/msg00756.html
One of the selling points was that having this check in
virDomainDefCompatibleDevice() will fix the problem across all drivers.
Well, turns out, some drivers don't care (e.g. lxc) and some have a
different workflow and thus don't need that check.
Michal Prívozník (4):
conf: Expose virDomainSCSIDriveAddressIsUsed
qemuhotplugtest: Don't plug a SCSI disk at unit 7
qemu_hotplug: Check for duplicate drive addresses
Revert "domain_conf: check device address before attach"
src/conf/domain_conf.c | 11 +----------
src/conf/domain_conf.h | 4 ++++
src/libvirt_private.syms | 1 +
src/qemu/qemu_hotplug.c | 6 ++++++
tests/qemuhotplugtest.c | 2 +-
.../qemuhotplug-disk-scsi-2.xml | 2 +-
...-base-without-scsi-controller-live+disk-scsi-2.xml | 4 ++--
7 files changed, 16 insertions(+), 14 deletions(-)
--
2.21.0
9:34 a.m.
New subject: [libvirt] [PATCH 1/4] conf: Expose virDomainSCSIDriveAddressIsUsed
This function checks if given drive address is already present in
passed domain definition. Expose the function as it will be used
shortly.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 2 +-
src/conf/domain_conf.h | 4 ++++
src/libvirt_private.syms | 1 +
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bbde3788a6..bce4e65e0d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4644,7 +4644,7 @@ virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def,
* Return true if the SCSI drive address is already in use, false
* otherwise.
*/
-static bool
+bool
virDomainSCSIDriveAddressIsUsed(const virDomainDef *def,
const virDomainDeviceDriveAddress *addr)
{
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 12eb71c197..fc3a8fb795 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2698,6 +2698,10 @@ virDomainXMLNamespacePtr
virDomainXMLOptionGetNamespace(virDomainXMLOptionPtr xmlopt)
ATTRIBUTE_NONNULL(1);
+bool
+virDomainSCSIDriveAddressIsUsed(const virDomainDef *def,
+ const virDomainDeviceDriveAddress *addr);
+
int virDomainDefPostParse(virDomainDefPtr def,
virCapsPtr caps,
unsigned int parseFlags,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6e64e77839..eb9727c299 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -535,6 +535,7 @@ virDomainRunningReasonTypeToString;
virDomainSaveConfig;
virDomainSaveStatus;
virDomainSaveXML;
+virDomainSCSIDriveAddressIsUsed;
virDomainSeclabelTypeFromString;
virDomainSeclabelTypeToString;
virDomainShmemDefEquals;
--
2.21.0
9:34 a.m.
New subject: [libvirt] [PATCH 2/4] qemuhotplugtest: Don't plug a SCSI disk at unit 7
Unit number 7 is kind of special. It's reserved for SCSI
controller. The comment in virDomainSCSIDriveAddressIsUsed()
summarizes that pretty nicely. Libvirt would never generate
such address.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/qemuhotplugtest.c | 2 +-
tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-2.xml | 2 +-
...uhotplug-base-without-scsi-controller-live+disk-scsi-2.xml | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 3eb97d886a..fbf488c54c 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -715,7 +715,7 @@ mymain(void)
"device_del", QMP_OK,
"human-monitor-command", HMP(""));
DO_TEST_DETACH("base-with-scsi-controller-live", "disk-scsi-2",
false, false,
- "device_del", QMP_DEVICE_DELETED("scsi3-0-5-7")
QMP_OK,
+ "device_del", QMP_DEVICE_DELETED("scsi3-0-5-6")
QMP_OK,
"human-monitor-command", HMP(""));
DO_TEST_ATTACH("base-live", "qemu-agent", false, true,
diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-2.xml
b/tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-2.xml
index 3a847fbda6..876afb182f 100644
--- a/tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-2.xml
+++ b/tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-2.xml
@@ -2,7 +2,7 @@
<driver name='qemu' type='raw' cache='none'/>
<source file='/dev/null'/>
<target dev='sdf' bus='scsi'/>
- <address type='drive' controller='3' bus='0'
target='5' unit='7'/>
+ <address type='drive' controller='3' bus='0'
target='5' unit='6'/>
<readonly/>
<shareable/>
</disk>
diff --git
a/tests/qemuhotplugtestdomains/qemuhotplug-base-without-scsi-controller-live+disk-scsi-2.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-without-scsi-controller-live+disk-scsi-2.xml
index d35fea6f5f..72b5174825 100644
---
a/tests/qemuhotplugtestdomains/qemuhotplug-base-without-scsi-controller-live+disk-scsi-2.xml
+++
b/tests/qemuhotplugtestdomains/qemuhotplug-base-without-scsi-controller-live+disk-scsi-2.xml
@@ -26,8 +26,8 @@
<target dev='sdf' bus='scsi'/>
<readonly/>
<shareable/>
- <alias name='scsi3-0-5-7'/>
- <address type='drive' controller='3' bus='0'
target='5' unit='7'/>
+ <alias name='scsi3-0-5-6'/>
+ <address type='drive' controller='3' bus='0'
target='5' unit='6'/>
</disk>
<controller type='usb' index='0'>
<alias name='usb'/>
--
2.21.0
9:34 a.m.
New subject: [libvirt] [PATCH 3/4] qemu_hotplug: Check for duplicate drive addresses
This tries to fix the same problem as f1d65853000 but it's doing
so in a less invasive way.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ef14b1977c..5b6afe5c2b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1183,6 +1183,12 @@ qemuDomainAttachSCSIDisk(virQEMUDriverPtr driver,
return -1;
}
+ if (virDomainSCSIDriveAddressIsUsed(vm->def, &disk->info.addr.drive)) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("Domain already contains a disk with that address"));
+ return -1;
+ }
+
/* Let's make sure the disk has a controller defined and loaded before
* trying to add it. The controller used by the disk must exist before a
* qemu command line string is generated.
--
2.21.0
9:35 a.m.
New subject: [libvirt] [PATCH 4/4] Revert "domain_conf: check device address before attach"
This reverts commit f1d6585300001c7b23b8796a0faa4411c3531996.
Turns out, this caused a regression. There is this (perhaps less
known) semantic of virDomainAttachDevice() where if the device
the API is trying to attach is a CDROM/floppy that is already in
the domain the attach request is handled as 'change the media in
the drive'.
We have a better fix anyways.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bce4e65e0d..14d3f4d67e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28586,15 +28586,6 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
if (oldDev)
data.oldInfo = virDomainDeviceGetInfo(oldDev);
- if (action == VIR_DOMAIN_DEVICE_ACTION_ATTACH &&
- data.newInfo &&
- data.newInfo->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
- virDomainDefHasDeviceAddress(def, data.newInfo)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("Domain already contains a device with the same
address"));
- return -1;
- }
-
if (action == VIR_DOMAIN_DEVICE_ACTION_UPDATE &&
live &&
(data.newInfo && data.oldInfo &&
--
2.21.0
1:36 p.m.
On 4/11/19 11:34 AM, Michal Privoznik wrote:
This is an alternative approach to:
https://www.redhat.com/archives/libvir-list/2019-March/msg01111.html
which caused regression to which a proposed fix is here:
https://www.redhat.com/archives/libvir-list/2019-April/msg00756.html
One of the selling points was that having this check in
virDomainDefCompatibleDevice() will fix the problem across all drivers.
Well, turns out, some drivers don't care (e.g. lxc) and some have a
different workflow and thus don't need that check.
Just tested the patch series and it fixes the problem for SCSI disks, as
expected.
The major drawback of reverting that patch is that we lose the address
check not just for other drivers (that might not care much for that), but
also for the remaining device types other than SCSI disks.
Now, taking a quick look in the code I see that there are similar checks
being made in VDA, PCI and DIMM type devices (didn't check the remaining
devices). If there are address redundancy checks for all other devices, then
the patch can be reverted without loss.
It is worth mentioning that the first 2 patches can be applied regardless of
what it is decided to do about the address checking, since they are a
straight
improvement in what already exists.
Thanks,
DHB
Michal Prívozník (4):
conf: Expose virDomainSCSIDriveAddressIsUsed
qemuhotplugtest: Don't plug a SCSI disk at unit 7
qemu_hotplug: Check for duplicate drive addresses
Revert "domain_conf: check device address before attach"
src/conf/domain_conf.c | 11 +----------
src/conf/domain_conf.h | 4 ++++
src/libvirt_private.syms | 1 +
src/qemu/qemu_hotplug.c | 6 ++++++
tests/qemuhotplugtest.c | 2 +-
.../qemuhotplug-disk-scsi-2.xml | 2 +-
...-base-without-scsi-controller-live+disk-scsi-2.xml | 4 ++--
7 files changed, 16 insertions(+), 14 deletions(-)
4:16 a.m.
On 4/11/19 8:36 PM, Daniel Henrique Barboza wrote:
On 4/11/19 11:34 AM, Michal Privoznik wrote:
> This is an alternative approach to:
>
> https://www.redhat.com/archives/libvir-list/2019-March/msg01111.html
>
> which caused regression to which a proposed fix is here:
>
> https://www.redhat.com/archives/libvir-list/2019-April/msg00756.html
>
> One of the selling points was that having this check in
> virDomainDefCompatibleDevice() will fix the problem across all drivers.
> Well, turns out, some drivers don't care (e.g. lxc) and some have a
> different workflow and thus don't need that check.
Just tested the patch series and it fixes the problem for SCSI disks, as
expected.
The major drawback of reverting that patch is that we lose the address
check not just for other drivers (that might not care much for that), but
also for the remaining device types other than SCSI disks.
Now, taking a quick look in the code I see that there are similar checks
being made in VDA, PCI and DIMM type devices (didn't check the remaining
devices). If there are address redundancy checks for all other devices,
then
the patch can be reverted without loss.
Yep, other types of addresees are covered. At least in drivers that do
care. LXC doesn't because it doesn't make much sense there. I mean, a
container shares HW with the host, so we can't really make it appear at
different addresses. For instance, when plugging a disk into an LXC
container it'll appear at the same adddess as in the host and no
<address/> in the XML is going to change that.
That's why I vouche for this approach. Sorry for not realizing this the
first time.
It is worth mentioning that the first 2 patches can be applied
regardless of
what it is decided to do about the address checking, since they are a
straight
improvement in what already exists.
Thanks,
Michal
12:21 p.m.
On 4/12/19 3:16 AM, Michal Privoznik wrote:
On 4/11/19 8:36 PM, Daniel Henrique Barboza wrote:
>
>
> On 4/11/19 11:34 AM, Michal Privoznik wrote:
>> This is an alternative approach to:
>>
>> https://www.redhat.com/archives/libvir-list/2019-March/msg01111.html
>>
>> which caused regression to which a proposed fix is here:
>>
>> https://www.redhat.com/archives/libvir-list/2019-April/msg00756.html
>>
>> One of the selling points was that having this check in
>> virDomainDefCompatibleDevice() will fix the problem across all drivers.
>> Well, turns out, some drivers don't care (e.g. lxc) and some have a
>> different workflow and thus don't need that check.
>
> Just tested the patch series and it fixes the problem for SCSI disks, as
> expected.
>
> The major drawback of reverting that patch is that we lose the address
> check not just for other drivers (that might not care much for that), but
> also for the remaining device types other than SCSI disks.
>
> Now, taking a quick look in the code I see that there are similar checks
> being made in VDA, PCI and DIMM type devices (didn't check the remaining
> devices). If there are address redundancy checks for all other devices, then
> the patch can be reverted without loss.
Yep, other types of addresees are covered. At least in drivers that do care. LXC
doesn't because it doesn't make much sense there. I mean, a container shares HW
with the host, so we can't really make it appear at different addresses. For
instance, when plugging a disk into an LXC container it'll appear at the same
adddess as in the host and no <address/> in the XML is going to change that.
That's why I vouche for this approach. Sorry for not realizing this the first time.
I agree with this approach as well. IMO the check should be done in the
hypervisor drivers, where there is more knowledge about what is permissible wrt
the various device types.
Regards,
Jim
>
> It is worth mentioning that the first 2 patches can be applied regardless of
> what it is decided to do about the address checking, since they are a straight
> improvement in what already exists.
Thanks,
Michal
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
12:27 p.m.
On 4/12/19 6:16 AM, Michal Privoznik wrote:
On 4/11/19 8:36 PM, Daniel Henrique Barboza wrote:
>
>
> On 4/11/19 11:34 AM, Michal Privoznik wrote:
>> This is an alternative approach to:
>>
>> https://www.redhat.com/archives/libvir-list/2019-March/msg01111.html
>>
>> which caused regression to which a proposed fix is here:
>>
>> https://www.redhat.com/archives/libvir-list/2019-April/msg00756.html
>>
>> One of the selling points was that having this check in
>> virDomainDefCompatibleDevice() will fix the problem across all drivers.
>> Well, turns out, some drivers don't care (e.g. lxc) and some have a
>> different workflow and thus don't need that check.
>
> Just tested the patch series and it fixes the problem for SCSI disks, as
> expected.
>
> The major drawback of reverting that patch is that we lose the address
> check not just for other drivers (that might not care much for that),
> but
> also for the remaining device types other than SCSI disks.
>
> Now, taking a quick look in the code I see that there are similar checks
> being made in VDA, PCI and DIMM type devices (didn't check the remaining
> devices). If there are address redundancy checks for all other
> devices, then
> the patch can be reverted without loss.
Yep, other types of addresees are covered. At least in drivers that do
care. LXC doesn't because it doesn't make much sense there. I mean, a
container shares HW with the host, so we can't really make it appear
at different addresses. For instance, when plugging a disk into an LXC
container it'll appear at the same adddess as in the host and no
<address/> in the XML is going to change that.
That's why I vouche for this approach. Sorry for not realizing this
the first time.
That makes sense.
For all patches:
Tested-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
>
> It is worth mentioning that the first 2 patches can be applied
> regardless of
> what it is decided to do about the address checking, since they are a
> straight
> improvement in what already exists.
Thanks,
Michal
12:25 p.m.
On 4/11/19 8:34 AM, Michal Privoznik wrote:
This is an alternative approach to:
https://www.redhat.com/archives/libvir-list/2019-March/msg01111.html
which caused regression to which a proposed fix is here:
https://www.redhat.com/archives/libvir-list/2019-April/msg00756.html
One of the selling points was that having this check in
virDomainDefCompatibleDevice() will fix the problem across all drivers.
Well, turns out, some drivers don't care (e.g. lxc) and some have a
different workflow and thus don't need that check.
Michal Prívozník (4):
conf: Expose virDomainSCSIDriveAddressIsUsed
qemuhotplugtest: Don't plug a SCSI disk at unit 7
qemu_hotplug: Check for duplicate drive addresses
Revert "domain_conf: check device address before attach"
src/conf/domain_conf.c | 11 +----------
src/conf/domain_conf.h | 4 ++++
src/libvirt_private.syms | 1 +
src/qemu/qemu_hotplug.c | 6 ++++++
tests/qemuhotplugtest.c | 2 +-
.../qemuhotplug-disk-scsi-2.xml | 2 +-
...-base-without-scsi-controller-live+disk-scsi-2.xml | 4 ++--
7 files changed, 16 insertions(+), 14 deletions(-)
Thanks. I reviewed and tested the series.
Reviewed-by: Jim Fehlig <jfehlig(a)suse.com>
Regards,
Jim
10:16 a.m.
On 4/12/19 7:25 PM, Jim Fehlig wrote:
On 4/11/19 8:34 AM, Michal Privoznik wrote:
> This is an alternative approach to:
>
> https://www.redhat.com/archives/libvir-list/2019-March/msg01111.html
>
> which caused regression to which a proposed fix is here:
>
> https://www.redhat.com/archives/libvir-list/2019-April/msg00756.html
>
> One of the selling points was that having this check in
> virDomainDefCompatibleDevice() will fix the problem across all drivers.
> Well, turns out, some drivers don't care (e.g. lxc) and some have a
> different workflow and thus don't need that check.
>
> Michal Prívozník (4):
> conf: Expose virDomainSCSIDriveAddressIsUsed
> qemuhotplugtest: Don't plug a SCSI disk at unit 7
> qemu_hotplug: Check for duplicate drive addresses
> Revert "domain_conf: check device address before attach"
>
> src/conf/domain_conf.c | 11 +----------
> src/conf/domain_conf.h | 4 ++++
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_hotplug.c | 6 ++++++
> tests/qemuhotplugtest.c | 2 +-
> .../qemuhotplug-disk-scsi-2.xml | 2 +-
> ...-base-without-scsi-controller-live+disk-scsi-2.xml | 4 ++--
> 7 files changed, 16 insertions(+), 14 deletions(-)
>
Thanks. I reviewed and tested the series.
Reviewed-by: Jim Fehlig <jfehlig(a)suse.com>
Oh forgot to push those. Sorry. Now pushed.
Thank you both.
Michal
2045
days inactive
2052
days old
10 comments
3 participants
participants (3)
-
Daniel Henrique Barboza -
Jim Fehlig -
Michal Privoznik