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