Re: [libvirt] [PATCH 0/3] Emit event on lease attach/detach
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