Re: [libvirt] [PATCH 4/8] conf: Allow NULL for virDomainDeviceInfoCallback
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