[libvirt] [PATCH 4/8] conf: Allow NULL for virDomainDeviceInfoCallback

Ján Tomko jtomko at redhat.com
Tue May 21 14:28:23 UTC 2019


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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190521/8625539b/attachment-0001.sig>


More information about the libvir-list mailing list