The virDomainDeviceInfoIterate() function was initially
written with the expectation that all devices would embed a
virDomainDeviceInfo, and thus the user-provided callback
would never be passed NULL; however, that doesn't really
represent reality, as multiple devices don't have any
virDomainDeviceInfo associated with them.
Since we still want to be able to iterate over those devices,
clarify that callbacks are expected to be able to handle NULL
being passed to them, and update all existing callbacks so
that they actually do so.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/bhyve/bhyve_device.c | 4 ++++
src/conf/domain_addr.c | 9 +++++++++
src/conf/domain_conf.c | 34 +++++++++++++++++++++++++++++++-
src/libxl/libxl_driver.c | 3 +++
src/qemu/qemu_domain_address.c | 36 +++++++++++++++++++++++++++++++---
src/qemu/qemu_hotplug.c | 3 +++
6 files changed, 85 insertions(+), 4 deletions(-)
diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c
index 201044d9e6..8c897cbd8d 100644
--- a/src/bhyve/bhyve_device.c
+++ b/src/bhyve/bhyve_device.c
@@ -37,6 +37,10 @@ bhyveCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
void *opaque)
{
int ret = -1;
+
+ if (!info)
+ return 0;
+
if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
return 0;
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 04c4e6d7e1..548af89682 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1468,6 +1468,9 @@ virDomainCCWAddressAllocate(virDomainDefPtr def ATTRIBUTE_UNUSED,
virDomainDeviceInfoPtr info,
void *data)
{
+ if (!info)
+ return 0;
+
return virDomainCCWAddressAssign(info, data, true);
}
@@ -1477,6 +1480,9 @@ virDomainCCWAddressValidate(virDomainDefPtr def ATTRIBUTE_UNUSED,
virDomainDeviceInfoPtr info,
void *data)
{
+ if (!info)
+ return 0;
+
return virDomainCCWAddressAssign(info, data, false);
}
@@ -1694,6 +1700,9 @@ virDomainVirtioSerialAddrReserve(virDomainDefPtr def ATTRIBUTE_UNUSED,
bool b;
ssize_t i;
+ if (!info)
+ return 0;
+
if (!virDomainVirtioSerialAddrIsComplete(info))
return 0;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a3a514136b..f42d331341 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4049,6 +4049,9 @@ virDomainDefHasDeviceAddressIterator(virDomainDefPtr def ATTRIBUTE_UNUSED,
{
virDomainDeviceInfoPtr needle = opaque;
+ if (!info)
+ return 0;
+
/* break iteration if the info was found */
if (virDomainDeviceInfoAddressIsEqual(info, needle))
return -1;
@@ -4297,6 +4300,21 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
}
+/**
+ * virDomainDeviceInfoIterate:
+ * @def: domain definition
+ * @cb: callback
+ * @opaque: user data
+ *
+ * Call @cb for each device in @def.
+ *
+ * Note that some devices might not have a virDomainDeviceInfoPtr associated
+ * with them, in which case the corresponding argument passed to the callback
+ * will be NULL: @cb should be written to account for this possibility, which
+ * usually involves returning early.
+ *
+ * Return: 0 on success, <0 on failure
+ */
int
virDomainDeviceInfoIterate(virDomainDefPtr def,
virDomainDeviceInfoCallback cb,
@@ -5543,6 +5561,9 @@ virDomainDefCollectBootOrder(virDomainDefPtr def ATTRIBUTE_UNUSED,
virHashTablePtr bootHash = data;
VIR_AUTOFREE(char *) order = NULL;
+ if (!info)
+ return 0;
+
if (info->bootIndex == 0)
return 0;
@@ -6383,7 +6404,12 @@ virDomainDeviceDefValidateAliasesIterator(virDomainDefPtr def,
void *opaque)
{
struct virDomainDefValidateAliasesData *data = opaque;
- const char *alias = info->alias;
+ const char *alias;
+
+ if (!info)
+ return 0;
+
+ alias = info->alias;
if (!virDomainDeviceAliasIsUserAlias(alias))
return 0;
@@ -28767,6 +28793,9 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED,
{
virDomainCompatibleDeviceDataPtr data = opaque;
+ if (!info)
+ return 0;
+
/* Ignore the device we're about to update */
if (data->oldInfo == info)
return 0;
@@ -29856,6 +29885,9 @@ virDomainDefFindDeviceCallback(virDomainDefPtr def ATTRIBUTE_UNUSED,
{
virDomainDefFindDeviceCallbackData *data = opaque;
+ if (!info)
+ return 0;
+
if (STREQ_NULLABLE(info->alias, data->devAlias)) {
*data->dev = *dev;
return -1;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 2b9c6f1866..560f278761 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3593,6 +3593,9 @@ libxlComparePCIDevice(virDomainDefPtr def ATTRIBUTE_UNUSED,
{
virDomainDeviceInfoPtr info2 = opaque;
+ if (!info1)
+ return 0;
+
if (info1->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI ||
info2->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
return 0;
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 4b99e8ca93..5bef29b9df 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -169,6 +169,9 @@ qemuDomainSpaprVIOFindByReg(virDomainDefPtr def ATTRIBUTE_UNUSED,
{
virDomainDeviceInfoPtr target = opaque;
+ if (!info)
+ return 0;
+
if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO)
return 0;
@@ -427,6 +430,9 @@ qemuDomainHasVirtioMMIODevicesCallback(virDomainDefPtr def ATTRIBUTE_UNUSED,
virDomainDeviceInfoPtr info,
void *opaque)
{
+ if (!info)
+ return 0;
+
if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) {
/* We can stop iterating as soon as we find the first
* virtio-mmio device */
@@ -1083,6 +1089,9 @@ qemuDomainFillDevicePCIConnectFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
{
qemuDomainFillDevicePCIConnectFlagsIterData *data = opaque;
+ if (!info)
+ return 0;
+
info->pciConnectFlags
= qemuDomainDeviceCalculatePCIConnectFlags(dev, data->driver,
data->pcieFlags,
@@ -1139,6 +1148,9 @@ qemuDomainFillDevicePCIExtensionFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
{
virQEMUCapsPtr qemuCaps = opaque;
+ if (!info)
+ return 0;
+
info->pciAddrExtFlags =
qemuDomainDeviceCalculatePCIAddressExtensionFlags(qemuCaps, dev);
@@ -1188,6 +1200,9 @@ qemuDomainFindUnusedIsolationGroupIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
{
unsigned int *isolationGroup = opaque;
+ if (!info)
+ return 0;
+
if (info->isolationGroup == *isolationGroup)
return -1;
@@ -1479,7 +1494,12 @@ qemuDomainAssignPCIAddressExtension(virDomainDefPtr def ATTRIBUTE_UNUSED,
void *opaque)
{
virDomainPCIAddressSetPtr addrs = opaque;
- virPCIDeviceAddressPtr addr = &info->addr.pci;
+ virPCIDeviceAddressPtr addr;
+
+ if (!info)
+ return 0;
+
+ addr = &info->addr.pci;
if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
addr->extFlags = info->pciAddrExtFlags;
@@ -1498,7 +1518,12 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
{
virDomainPCIAddressSetPtr addrs = opaque;
int ret = -1;
- virPCIDeviceAddressPtr addr = &info->addr.pci;
+ virPCIDeviceAddressPtr addr;
+
+ if (!info)
+ return 0;
+
+ addr = &info->addr.pci;
if (!virDeviceInfoPCIAddressIsPresent(info) ||
((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) &&
@@ -1590,7 +1615,12 @@ qemuDomainCollectPCIAddressExtension(virDomainDefPtr def ATTRIBUTE_UNUSED,
void *opaque)
{
virDomainPCIAddressSetPtr addrs = opaque;
- virPCIDeviceAddressPtr addr = &info->addr.pci;
+ virPCIDeviceAddressPtr addr;
+
+ if (!info)
+ return 0;
+
+ addr = &info->addr.pci;
if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
addr->extFlags = info->pciAddrExtFlags;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 59ff88565d..e275685a60 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4415,6 +4415,9 @@ static int qemuComparePCIDevice(virDomainDefPtr def ATTRIBUTE_UNUSED,
{
virDomainDeviceInfoPtr info2 = opaque;
+ if (!info1->type)
+ return 0;
+
if (info1->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI ||
info2->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
return 0;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, May 20, 2019 at 01:37:49PM +0200, Andrea Bolognani wrote:
>The virDomainDeviceInfoIterate() function was initially
>written with the expectation that all devices would embed a
>virDomainDeviceInfo, and thus the user-provided callback
>would never be passed NULL; however, that doesn't really
>represent reality, as multiple devices don't have any
>virDomainDeviceInfo associated with them.
virDomainDeviceInfoIterate is specifically meant for iterating
over device infos.
Commit 88d24aaccc1e31ff1ce682f9496cf08cc7f7c216 :
conf: domain: Introduce virDomainDeviceIterateFlags
documented this function as iterating over devices (but did not
implement this for every device) and then
commit dd45c2710f6fd2d4f8a47f97960532d0e0091e7d
conf: domain: gfx: Iterate over graphics devices when doing validation
added one of them.
Of course, there is a huge overlap between iterating over all devices
and just those with an info, but since the callers do request *Info*
I don't think they should expect it to be NULL.
Jano
>
>Since we still want to be able to iterate over those devices,
>clarify that callbacks are expected to be able to handle NULL
>being passed to them, and update all existing callbacks so
>that they actually do so.
>
>Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>---
> src/bhyve/bhyve_device.c | 4 ++++
> src/conf/domain_addr.c | 9 +++++++++
> src/conf/domain_conf.c | 34 +++++++++++++++++++++++++++++++-
> src/libxl/libxl_driver.c | 3 +++
> src/qemu/qemu_domain_address.c | 36 +++++++++++++++++++++++++++++++---
> src/qemu/qemu_hotplug.c | 3 +++
> 6 files changed, 85 insertions(+), 4 deletions(-)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 2019-05-20 at 14:59 +0200, Ján Tomko wrote: > On Mon, May 20, 2019 at 01:37:49PM +0200, Andrea Bolognani wrote: > > The virDomainDeviceInfoIterate() function was initially > > written with the expectation that all devices would embed a > > virDomainDeviceInfo, and thus the user-provided callback > > would never be passed NULL; however, that doesn't really > > represent reality, as multiple devices don't have any > > virDomainDeviceInfo associated with them. > > virDomainDeviceInfoIterate is specifically meant for iterating > over device infos. > > Commit 88d24aaccc1e31ff1ce682f9496cf08cc7f7c216 : > conf: domain: Introduce virDomainDeviceIterateFlags > > documented this function as iterating over devices (but did not > implement this for every device) and then > > commit dd45c2710f6fd2d4f8a47f97960532d0e0091e7d > conf: domain: gfx: Iterate over graphics devices when doing validation > > added one of them. Yeah, I'm aware of the history here, and the next patch partially reverts the second commit. > Of course, there is a huge overlap between iterating over all devices > and just those with an info, but since the callers do request *Info* > I don't think they should expect it to be NULL. I realize that. I even toyed with the idea of renaming virDomainDeviceInfoIterate() to virDomainDeviceIterate() to get rid of the inconsistency, but I didn't feel like it would be worth the churn and though that documenting the expectations would be enough. I'd be fine renaming it, though. Personally I don't think adding a new virDomainDeviceIterateFlags for each virDomainDeviceInfo-less device class is a good solution, as it leaves the door open to wiring up a validate callback that looks like it would be called but actually isn't: this is what caused dd45c27, and what happened to me while I was moving the validation code for Intel IOMMU from qemu_command to domain_conf. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, May 20, 2019 at 03:24:06PM +0200, Andrea Bolognani wrote: >On Mon, 2019-05-20 at 14:59 +0200, Ján Tomko wrote: >> On Mon, May 20, 2019 at 01:37:49PM +0200, Andrea Bolognani wrote: >> > The virDomainDeviceInfoIterate() function was initially >> > written with the expectation that all devices would embed a >> > virDomainDeviceInfo, and thus the user-provided callback >> > would never be passed NULL; however, that doesn't really >> > represent reality, as multiple devices don't have any >> > virDomainDeviceInfo associated with them. >> >> virDomainDeviceInfoIterate is specifically meant for iterating >> over device infos. >> >> Commit 88d24aaccc1e31ff1ce682f9496cf08cc7f7c216 : >> conf: domain: Introduce virDomainDeviceIterateFlags >> >> documented this function as iterating over devices (but did not >> implement this for every device) and then >> >> commit dd45c2710f6fd2d4f8a47f97960532d0e0091e7d >> conf: domain: gfx: Iterate over graphics devices when doing validation >> >> added one of them. > >Yeah, I'm aware of the history here, and the next patch partially >reverts the second commit. > >> Of course, there is a huge overlap between iterating over all devices >> and just those with an info, but since the callers do request *Info* >> I don't think they should expect it to be NULL. > >I realize that. I even toyed with the idea of renaming >virDomainDeviceInfoIterate() to virDomainDeviceIterate() to get rid >of the inconsistency, but I didn't feel like it would be worth the >churn and though that documenting the expectations would be enough. I think there would be less churn that way, see my proposal: Message-Id: <cover.1558448715.git.jtomko@redhat.com> https://www.redhat.com/archives/libvir-list/2019-May/msg00607.html >I'd be fine renaming it, though. > >Personally I don't think adding a new virDomainDeviceIterateFlags >for each virDomainDeviceInfo-less device class is a good solution, Agreed. >as it leaves the door open to wiring up a validate callback that >looks like it would be called but actually isn't: this is what >caused dd45c27, and what happened to me while I was moving the >validation code for Intel IOMMU from qemu_command to domain_conf. > Jano -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2019-05-21 at 16:28 +0200, Ján Tomko wrote: > On Mon, May 20, 2019 at 03:24:06PM +0200, Andrea Bolognani wrote: > > On Mon, 2019-05-20 at 14:59 +0200, Ján Tomko wrote: > > > Of course, there is a huge overlap between iterating over all devices > > > and just those with an info, but since the callers do request *Info* > > > I don't think they should expect it to be NULL. > > > > I realize that. I even toyed with the idea of renaming > > virDomainDeviceInfoIterate() to virDomainDeviceIterate() to get rid > > of the inconsistency, but I didn't feel like it would be worth the > > churn and though that documenting the expectations would be enough. > > I think there would be less churn that way, see my proposal: > Message-Id: <cover.1558448715.git.jtomko@redhat.com> > https://www.redhat.com/archives/libvir-list/2019-May/msg00607.html Well of course you're avoiding most of the churn I was mentioning: you're adding a new function instead of renaming the existing one! That's cheating ;) I don't much like the idea of adding a separate function that does almost the same thing but not quite, because that will ultimately result in the same situation we have now: someone will add a new callback and (reasonably) expect it to be called for all devices, but that won't happen because the original code uses the DeviceInfo variant instead of the Device one. That's unnecessary friction. Not to mention that your new function also happens to iterate over all consoles, which the existing variant doesn't. That's an extra little inconsistency that we really don't need to introduce. So I still prefer my approach. As said earlier, I'm also not entirely convinced keeping the existing function name is a good idea, so I'll happily rename it at the same time as I'm updating and documenting its contract. Especially once that's done, I don't see any problem with passing the callback a pointer that might be NULL and expecting the user to check before using it, as that's par for the course when using the C language. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, May 22, 2019 at 03:36:59PM +0200, Andrea Bolognani wrote: >On Tue, 2019-05-21 at 16:28 +0200, Ján Tomko wrote: >> On Mon, May 20, 2019 at 03:24:06PM +0200, Andrea Bolognani wrote: >> > On Mon, 2019-05-20 at 14:59 +0200, Ján Tomko wrote: >> > > Of course, there is a huge overlap between iterating over all devices >> > > and just those with an info, but since the callers do request *Info* >> > > I don't think they should expect it to be NULL. >> > >> > I realize that. I even toyed with the idea of renaming >> > virDomainDeviceInfoIterate() to virDomainDeviceIterate() to get rid >> > of the inconsistency, but I didn't feel like it would be worth the >> > churn and though that documenting the expectations would be enough. >> >> I think there would be less churn that way, see my proposal: >> Message-Id: <cover.1558448715.git.jtomko@redhat.com> >> https://www.redhat.com/archives/libvir-list/2019-May/msg00607.html > >Well of course you're avoiding most of the churn I was mentioning: >you're adding a new function instead of renaming the existing one! >That's cheating ;) > >I don't much like the idea of adding a separate function that does >almost the same thing but not quite, because that will ultimately >result in the same situation we have now: someone will add a new >callback and (reasonably) expect it to be called for all devices, >but that won't happen because the original code uses the DeviceInfo >variant instead of the Device one. That's unnecessary friction. > So is having to argue about not putting if (!info) into every single internal function that should not have been called with a NULL info in the first place. Jano -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2019-05-22 at 16:12 +0200, Ján Tomko wrote: > On Wed, May 22, 2019 at 03:36:59PM +0200, Andrea Bolognani wrote: > > I don't much like the idea of adding a separate function that does > > almost the same thing but not quite, because that will ultimately > > result in the same situation we have now: someone will add a new > > callback and (reasonably) expect it to be called for all devices, > > but that won't happen because the original code uses the DeviceInfo > > variant instead of the Device one. That's unnecessary friction. > > So is having to argue about not putting if (!info) into every single > internal function that should not have been called with a NULL info in > the first place. Well, it's pretty clear at this point that we're not likely to ever reach an agreement. Let's just go with (some variation of) your version and then move on to something more productive. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.