[libvirt] [PATCH 0/3] Emit event on lease attach/detach

[Cole Robinson]

On 4/23/19 9:32 AM, Daniel P. Berrangé wrote:
> On Tue, Apr 23, 2019 at 03:06:22PM +0200, Michal Privoznik wrote:
>> On 4/16/19 1:39 AM, Cole Robinson wrote:
>>> On 4/5/19 3:57 AM, Michal Privoznik wrote:
>>>> Unfortunately, we can't emit VIR_DOMAIN_EVENT_ID_DEVICE_ADDED or
>>>> VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED event because that carries device
>>>> alias within itself and leases don't have one.
>>>>
>>>
>>> Hmm. I understand that <leases> aren't really devices so it doesn't have
>>> any direct reason to give it an info structure and assign it an alias.
>>> But that's really an argument for why they shouldn't have been  a device
>>> to begin with. I presume we did it that way to take advantage of the
>>> existing hotplug APIs. But then adding a special purpose event seems
>>> like going in the opposite direction.
>>>
>>> How invasive is it to add an 'info' bit to lease devices, allocate
>>> aliases with the rest of qemu devices, maybe add an ADDRESS_TYPE_LEASE
>>> or something that is just an explicit no op for the ADDRESS consumers.
>>> My guess is it's not too bad but there could be dragons lurking
>>>
>>> There's already places internally where this would simplify things: it's
>>> annoying that device iterator code already needs to play games like
>>> 'check the address of all devices, oh except leases and graphics which
>>> don't have info, oh and hostdev info is a pointer for some reason'
>>
>> Yeah, it's annoying, but I remember discussion from when I was introducing
>> user aliases. I wanted to just have some dummy element that would not mean
>> anything to libvirt but users could set it and then match devices using it
>> as a mark. It was rejected because it doesn't map onto anything in qemu.
>> Well, nor do leases, nor would their address.
>>
>> I don't have a preference here really. But if we wanted to invent addresees
>> for leases then I guess we would have to do it for already running domains
>> too (when daemon restarts).
> 
> My preference is for the new event as this series does. We shouldn't try
> to force something to be more like a device when it clearly isn't.

IMO that ship has sailed. From the API perspective it's largely already
a device: it appears in the <devices> block of the domain, it's
added/removed/updated with the *Device* APIs.

Yes it shouldn't output or accept an <address>, but that's basically its
current state. If we add an .info struct internally I suspect it won't
take much extra work to preserve that behavior.

For info alias I think we should be adding one anyways. From my reading
the primary motivation of the user aliases support is that it gives API
users a unique way to identify the device, which seems just as valid for
leases as anything else listed in <devices>.

BTW I think all that logic applies to <graphics> devices as well. They
don't have .info internally, so don't get aliases. If there's a future
when <graphics> is hotpluggable we are going to have to add a custom
event for that too. The alias uniqueness bit applies as well, though not
too much an issue in practice for qemu because we enforce that only a
single <graphics> for each @type is allowed (but that's an artificial
distinction AFAICT, qemu looks like it can support multiple vnc server
instances for example)

Thanks,
Cole