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.