These wrapper functions were used to adapt the virObjectUnref() function
signature for different callbacks. But in commit 0d184072, the
virObjectUnref() function was changed to return a void instead of a
bool, so these adapters are no longer necessary.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
'cb' is always NULL when 'virObjectEventCallbackListAddID' is called.
Remove the call.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use virAppendElement instead of virInsertElementsN to implement
VIR_APPEND_ELEMENT which allows us to remove error handling as the
only relevant errors were removed when switching to aborting memory
allocation functions.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Historically, we declared pointer type to our types:
typedef struct _virXXX virXXX;
typedef virXXX *virXXXPtr;
But usefulness of such declaration is questionable, at best.
Unfortunately, we can't drop every such declaration - we have to
carry some over, because they are part of public API (e.g.
virDomainPtr). But for internal types - we can do drop them and
use what every other C project uses 'virXXX *'.
This change was generated by a very ugly shell script that
generated sed script which was then called over each file in the
repository. For the shell script refer to the cover letter:
https://listman.redhat.com/archives/libvir-list/2021-March/msg00537.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Generated by the following spatch:
@@
expression a, b;
@@
+ b = g_steal_pointer(&a);
- b = a;
... when != a
- a = NULL;
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
This patch takes on one set of examples of unnecessary use of
VIR_FREE() when g_free() is adequate - it modifies only vir*Free()
functions within the conf directory that take a single pointer and
free the object pointed to by that argument before returning. The
modification is to replace VIR_FREE() with g_free() for the object
itself *and* for all subordinate chunks of memory pointed to by that
object.
(NB: there are other functions that VIR_FREE subordinate memory of
objects that end up being freed before return (also sometimes with
VIR_FREE); I am purposefully ignoring those to reduce scope and focus
on a sub class where the pointlessness is obvious.)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Replace all occurrences of
if (VIR_STRDUP(a, b) < 0)
/* effectively dead code */
with:
a = g_strdup(b);
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use G_GNUC_UNUSED from GLib instead of ATTRIBUTE_UNUSED.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When registering new callback for an event, the event loop timer
must be created and registered. The timer has domain event state
object as an opaque argument which must be ref()-ed but only if
the timer was being created and registered successfully. We must
not ref it every time the virObjectEventStateRegisterID() runs.
Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Missing semicolon at the end of macros can confuse some analyzers
(like cppcheck <filename>). VIR_ONCE_GLOBAL_INIT is almost
exclusively called without an ending semicolon, but let's
standardize on using one like the other macros.
Add a dummy struct definition at the end of the macro, so
the compiler will require callers to add a semicolon.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
In many files there are header comments that contain an Author:
statement, supposedly reflecting who originally wrote the code.
In a large collaborative project like libvirt, any non-trivial
file will have been modified by a large number of different
contributors. IOW, the Author: comments are quickly out of date,
omitting people who have made significant contribitions.
In some places Author: lines have been added despite the person
merely being responsible for creating the file by moving existing
code out of another file. IOW, the Author: lines give an incorrect
record of authorship.
With this all in mind, the comments are useless as a means to identify
who to talk to about code in a particular file. Contributors will always
be better off using 'git log' and 'git blame' if they need to find the
author of a particular bit of code.
This commit thus deletes all Author: comments from the source and adds
a rule to prevent them reappearing.
The Copyright headers are similarly misleading and inaccurate, however,
we cannot delete these as they have legal meaning, despite being largely
inaccurate. In addition only the copyright holder is permitted to change
their respective copyright statement.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
So far we are repeating the following lines over and over:
if (!(virSomeObjectClass = virClassNew(virClassForObject(),
"virSomeObject",
sizeof(virSomeObject),
virSomeObjectDispose)))
return -1;
While this works, it is impossible to do some checking. Firstly,
the class name (the 2nd argument) doesn't match the name in the
code in all cases (the 3rd argument). Secondly, the current style
is needlessly verbose. This commit turns example into following:
if (!(VIR_CLASS_NEW(virSomeObject,
virClassForObject)))
return -1;
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
If a remote call fails during event registration (more than likely from
a network failure or remote libvirtd restart timed just right), then when
calling the virObjectEventStateDeregisterID we don't want to call the
registered @freecb function because that breaks our contract that we
would only call it after succesfully returning. If the @freecb routine
were called, it could result in a double free from properly coded
applications that free their opaque data on failure to register, as seen
in the following details:
Program terminated with signal 6, Aborted.
#0 0x00007fc45cba15d7 in raise
#1 0x00007fc45cba2cc8 in abort
#2 0x00007fc45cbe12f7 in __libc_message
#3 0x00007fc45cbe86d3 in _int_free
#4 0x00007fc45d8d292c in PyDict_Fini
#5 0x00007fc45d94f46a in Py_Finalize
#6 0x00007fc45d960735 in Py_Main
#7 0x00007fc45cb8daf5 in __libc_start_main
#8 0x0000000000400721 in _start
The double dereference of 'pyobj_cbData' is triggered in the following way:
(1) libvirt_virConnectDomainEventRegisterAny is invoked.
(2) the event is successfully added to the event callback list
(virDomainEventStateRegisterClient in
remoteConnectDomainEventRegisterAny returns 1 which means ok).
(3) when function remoteConnectDomainEventRegisterAny is hit,
network connection disconnected coincidently (or libvirtd is
restarted) in the context of function 'call' then the connection
is lost and the function 'call' failed, the branch
virObjectEventStateDeregisterID is therefore taken.
(4) 'pyobj_conn' is dereferenced the 1st time in
libvirt_virConnectDomainEventFreeFunc.
(5) 'pyobj_cbData' (refered to pyobj_conn) is dereferenced the
2nd time in libvirt_virConnectDomainEventRegisterAny.
(6) the double free error is triggered.
Resolve this by adding a @doFreeCb boolean in order to avoid calling the
freeCb in virObjectEventStateDeregisterID for any remote call failure in
a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* calls,
the passed value would be true indicating they should run the freecb if it
exists; whereas, it's false for the remote call failure path.
Patch based on the investigation and initial patch posted by
fangying <fangying1@huawei.com>.
If the last event callback is unregistered while the event loop is
dispatching, it is only marked as deleted, but not removed. The number
of callbacks is more than zero in that case, so the timer is not
removed. Because it can be removed in this function now (but also
accessed afterwards so that we set 'isDispatching = false' and have it
locked), we need to temporarily increase the reference counter of the
state for the duration of this function.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
There is a repeating pattern of code that removes the timer if it's not
needed. So let's move it to a new function. We'll also use it later.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
There should be one more reference because it is being kept in the list
of callbacks as an opaque. We also unref it properly using
virObjectFreeCallback.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Sometimes virObjectEventStateFlush can be called without timer (if the
last event was unregistered right when the timer fired). There is a
check for timer == -1, but that triggers warning and other log messages,
which is unnecessary.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
While no leak was observed yet, there might be one if
virObjectEventClass is ever derived from another class. Because
in that case plain VIR_FREE() will not call dispose() from parent
classes possibly leaking some memory.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Filtering monitor events by name requires tracking the name for
the duration of the filtering. In order to free the name, I
found it easiest to just piggyback on the user's freecb function,
which gets called when the event is deregistered.
For events without a name filter, we have the design of multiple
client registrations sharing a common server registration, because
the server side uses the same callback function and we reject
duplicate use of the same function. But with events in the mix,
we want to be able to allow the same function pointer to be used
with more than one event name. The solution is to tweak the
duplicate detection code to only act when there is no additional
filtering; if name filtering is in use, there is exactly one
client registration per server registration. Yes, this means
that there is no longer a bound on the number of server
registrations possible, so a malicious client could repeatedly
register for the same name event to exhaust server memory. On
the other hand, we already restricted monitor events to require
write access (compared to normal events only needing read access),
and separated it into the intentionally unsupported
libvirt-qemu.so, with documentation that using this function is
for debug purposes only; so it is not a security risk worth
worrying about a client trying to abuse multiple registrations.
* src/conf/domain_event.c (virDomainQemuMonitorEventData): New
struct.
(virDomainQemuMonitorEventFilter)
(virDomainQemuMonitorEventCleanup): New functions.
(virDomainQemuMonitorEventDispatchFunc)
(virDomainQemuMonitorEventStateRegisterID): Use new struct.
* src/conf/object_event.c (virObjectEventCallbackListCount)
(virObjectEventCallbackListAddID)
(virObjectEventCallbackListRemoveID)
(virObjectEventCallbackListMarkDeleteID): Drop duplicate detection
when filtering is in effect.
Signed-off-by: Eric Blake <eblake@redhat.com>
Any source file which calls the logging APIs now needs
to have a VIR_LOG_INIT("source.name") declaration at
the start of the file. This provides a static variable
of the virLogSource type.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
This fixes a possible double free. In virNetworkAssignDef() if
virBitmapNew() fails, then virNetworkObjFree(network) is called.
However, with network->def pointing to actual @def. So if caller
frees @def again, ...
Moreover, this fixes one possible memory leak too. In
virInterfaceAssignDef() if appending to the list of interfaces
fails, we ought to call virInterfaceObjFree() instead of bare
VIR_FREE().
Although, in order to do that some array size variables needs
to be turned into size_t rather than int.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
We want to convert over to server-side events, even for older
APIs. To do that, the client side of the remote driver wants
to distinguish between legacy virConnectDomainEventRegister and
normal virConnectDomainEventRegisterAny, while knowing the
client callbackID and the server's serverID for both types of
registration. The client also needs to probe whether the
server supports server-side filtering. However, for ease of
review, we don't actually use the new RPCs until a later patch.
* src/conf/object_event_private.h (virObjectEventStateCallbackID):
Add parameter.
* src/conf/object_event.c (virObjectEventCallbackListAddID)
(virObjectEventStateRegisterID): Separate legacy from callbackID.
(virObjectEventStateCallbackID): Pass through parameter.
(virObjectEventCallbackLookup): Let legacy and global domain
lifecycle events share a common remoteID.
* src/conf/network_event.c (virNetworkEventStateRegisterID):
Update caller.
* src/conf/domain_event.c (virDomainEventStateRegister)
(virDomainEventStateRegisterID, virDomainEventStateDeregister):
Likewise.
(virDomainEventStateRegisterClient)
(virDomainEventStateCallbackID): Implement new functions.
* src/conf/domain_event.h (virDomainEventStateRegisterClient)
(virDomainEventStateCallbackID): New prototypes.
* src/remote/remote_driver.c (private_data): Add field.
(doRemoteOpen): Probe server feature.
(remoteConnectDomainEventRegister)
(remoteConnectDomainEventRegisterAny): Use new function.
Signed-off-by: Eric Blake <eblake@redhat.com>
Ever since ACL filtering was added in commit 7639736 (v1.1.1), a
user could still use event registration to obtain access to a
domain that they could not normally access via virDomainLookup*
or virConnectListAllDomains and friends. We already have the
framework in the RPC generator for creating the filter, and
previous cleanup patches got us to the point that we can now
wire the filter through the entire object event stack.
Furthermore, whether or not domain:getattr is honored, use of
global events is a form of obtaining a list of networks, which
is covered by connect:search_domains added in a93cd08 (v1.1.0).
Ideally, we'd have a way to enforce connect:search_domains when
doing global registrations while omitting that check on a
per-domain registration. But this patch just unconditionally
requires connect:search_domains, even when no list could be
obtained, based on the following observations:
1. Administrators are unlikely to grant domain:getattr for one
or all domains while still denying connect:search_domains - a
user that is able to manage domains will want to be able to
manage them efficiently, but efficient management includes being
able to list the domains they can access. The idea of denying
connect:search_domains while still granting access to individual
domains is therefore not adding any real security, but just
serves as a layer of obscurity to annoy the end user.
2. In the current implementation, domain events are filtered
on the client; the server has no idea if a domain filter was
requested, and must therefore assume that all domain event
requests are global. Even if we fix the RPC protocol to
allow for server-side filtering for newer client/server combos,
making the connect:serach_domains ACL check conditional on
whether the domain argument was NULL won't benefit older clients.
Therefore, we choose to document that connect:search_domains
is a pre-requisite to any domain event management.
Network events need the same treatment, with the obvious
change of using connect:search_networks and network:getattr.
* src/access/viraccessperm.h
(VIR_ACCESS_PERM_CONNECT_SEARCH_DOMAINS)
(VIR_ACCESS_PERM_CONNECT_SEARCH_NETWORKS): Document additional
effect of the permission.
* src/conf/domain_event.h (virDomainEventStateRegister)
(virDomainEventStateRegisterID): Add new parameter.
* src/conf/network_event.h (virNetworkEventStateRegisterID):
Likewise.
* src/conf/object_event_private.h (virObjectEventStateRegisterID):
Likewise.
* src/conf/object_event.c (_virObjectEventCallback): Track a filter.
(virObjectEventDispatchMatchCallback): Use filter.
(virObjectEventCallbackListAddID): Register filter.
* src/conf/domain_event.c (virDomainEventFilter): New function.
(virDomainEventStateRegister, virDomainEventStateRegisterID):
Adjust callers.
* src/conf/network_event.c (virNetworkEventFilter): New function.
(virNetworkEventStateRegisterID): Adjust caller.
* src/remote/remote_protocol.x
(REMOTE_PROC_CONNECT_DOMAIN_EVENT_REGISTER)
(REMOTE_PROC_CONNECT_DOMAIN_EVENT_REGISTER_ANY)
(REMOTE_PROC_CONNECT_NETWORK_EVENT_REGISTER_ANY): Generate a
filter, and require connect:search_domains instead of weaker
connect:read.
* src/test/test_driver.c (testConnectDomainEventRegister)
(testConnectDomainEventRegisterAny)
(testConnectNetworkEventRegisterAny): Update callers.
* src/remote/remote_driver.c (remoteConnectDomainEventRegister)
(remoteConnectDomainEventRegisterAny): Likewise.
* src/xen/xen_driver.c (xenUnifiedConnectDomainEventRegister)
(xenUnifiedConnectDomainEventRegisterAny): Likewise.
* src/vbox/vbox_tmpl.c (vboxDomainGetXMLDesc): Likewise.
* src/libxl/libxl_driver.c (libxlConnectDomainEventRegister)
(libxlConnectDomainEventRegisterAny): Likewise.
* src/qemu/qemu_driver.c (qemuConnectDomainEventRegister)
(qemuConnectDomainEventRegisterAny): Likewise.
* src/uml/uml_driver.c (umlConnectDomainEventRegister)
(umlConnectDomainEventRegisterAny): Likewise.
* src/network/bridge_driver.c
(networkConnectNetworkEventRegisterAny): Likewise.
* src/lxc/lxc_driver.c (lxcConnectDomainEventRegister)
(lxcConnectDomainEventRegisterAny): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
In order to mirror a server with per-object filtering, the client
needs to track which server callbackID is servicing the client
callback. This patch introduces the notion of a serverID, as
well as the plumbing to use it for network events, although the
actual complexity of using per-object filtering in the remote
driver is deferred to a later patch.
* src/conf/object_event.h (virObjectEventStateEventID): Add parameter.
(virObjectEventStateQueueRemote, virObjectEventStateSetRemote):
New prototypes.
(virObjectEventStateRegisterID): Move...
* src/conf/object_event_private.h: ...here, and add parameter.
(_virObjectEvent): Add field.
* src/conf/network_event.h (virNetworkEventStateRegisterClient): New
prototype.
* src/conf/object_event.c (_virObjectEventCallback): Add field.
(virObjectEventStateSetRemote): New function.
(virObjectEventStateQueue): Make wrapper around...
(virObjectEventStateQueueRemote): New function.
(virObjectEventCallbackListCount): Tweak return count when remote
id matching is used.
(virObjectEventCallbackLookup, virObjectEventStateRegisterID):
Tweak registration when remote id matching will be used.
(virObjectEventNew): Default to no remote id.
(virObjectEventCallbackListAddID): Likewise, but set remote id
when one is available.
(virObjectEventCallbackListRemoveID)
(virObjectEventCallbackListMarkDeleteID): Adjust return value when
remote id was set.
(virObjectEventStateEventID): Query existing id.
(virObjectEventDispatchMatchCallback): Require matching event id.
(virObjectEventStateCallbackID): Adjust caller.
* src/conf/network_event.c (virNetworkEventStateRegisterClient): New
function.
(virNetworkEventStateRegisterID): Update caller.
* src/conf/domain_event.c (virDomainEventStateRegister)
(virDomainEventStateRegisterID): Update callers.
* src/remote/remote_driver.c
(remoteConnectNetworkEventRegisterAny)
(remoteConnectNetworkEventDeregisterAny)
(remoteConnectDomainEventDeregisterAny): Likewise.
(remoteEventQueue): Hoist earlier to avoid forward declaration,
and add parameter. Adjust all callers.
* src/libvirt_private.syms (conf/object_event.h): Drop function.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit cfd62c1 was incomplete; I found more cases where error
messages were being overwritten, and where the code between
the three registration/deregistration APIs was not consistent.
Since it is fairly easy to trigger an attempt to deregister an
unregistered object through public API, I also changed the error
message from VIR_ERR_INTERNAL_ERROR to VIR_ERR_INVALID_ARG.
* src/conf/object_event.c (virObjectEventCallbackListEventID):
Inline...
(virObjectEventStateEventID): ...into lone caller, and report
error on failure.
(virObjectEventCallbackListAddID, virObjectEventStateCallbackID)
(virObjectEventCallbackListRemoveID)
(virObjectEventCallbackListMarkDeleteID): Tweak error category.
* src/remote/remote_driver.c (remoteConnectDomainEventRegister):
Don't leak registration on failure.
(remoteConnectDomainEventDeregisterAny)
(remoteConnectNetworkEventDeregisterAny): Don't overwrite error.
Signed-off-by: Eric Blake <eblake@redhat.com>
If a user registers for a domain event filtered to a particular
domain, but the persistent domain is offline at the time, then
the code silently failed to set up the filter. As a result,
the event fires for all domains, rather than being filtered.
Network events were immune, since they always passed an id
0 argument.
The key to this patch is realizing that
virObjectEventDispatchMatchCallback() only cared about uuid;
so refusing to create a meta for a negative id is pointless,
and in fact, malloc'ing meta at all was overkill; instead,
just directly store a uuid and a flag of whether to filter.
Note that virObjectEventPtr still needs all fields of meta,
because this is how we reconstruct a virDomainPtr inside the
dispatch handler before calling the end user's callback
pointer with the correct object, even though only the uuid
portion of meta is used in deciding whether a callback
matches the given event. So while uuid is optional for
callbacks, it is mandatory for events.
The change to testDomainCreateXMLMixed is merely on the setup
scenario (as you can't register for a domain unless it is either
running or persistent). I actually first wrote that test for
this patch, then rebased it to also cover a prior patch (commit
4221d64), but had to adjust it for that patch to use Create
instead of Define for setting up the domain long enough to
register the event in order to work around this bug. But while
the setup is changed, the main body of the test is still about
whether creation events fire as expected.
* src/conf/object_event_private.h (_virObjectEventCallback):
Replace meta with uuid and flag.
(virObjectEventCallbackListAddID): Update signature.
* src/conf/object_event.h (virObjectEventStateRegisterID):
Likewise.
* src/conf/object_event_private.h (virObjectEventNew): Document
use of name and uuid in events.
* src/conf/object_event.c (virObjectEventCallbackListAddID): Drop
arguments that don't affect filtering.
(virObjectEventCallbackListRemoveID)
(virObjectEventDispatchMatchCallback)
(virObjectEventStateRegisterID): Update clients.
* src/conf/domain_event.c (virDomainEventCallbackListAdd)
(virDomainEventStateRegisterID): Likewise.
* src/conf/network_event.c (virNetworkEventStateRegisterID):
Likewise.
* tests/objecteventtest.c (testDomainCreateXMLMixed): Enhance test.
Signed-off-by: Eric Blake <eblake@redhat.com>
Consider these two calls, in either order:
id1 = virConnectDomainEventRegisterAny(conn, NULL,
VIR_DOMAIN_EVENT_ID_LIFECYCLE,
VIR_DOMAIN_EVENT_CALLBACK(callback), NULL, NULL);
virConnectDomainEventRegister(conn, callback, NULL, NULL);
Right now, the second call fails, because under the hood, the
old-style function registration is tightly coupled to the
new style lifecycle eventID, and the two calls both try
to register the same global eventID callback representation.
We've alreay documented that users should avoid old-style
registration and deregistration, so anyone heeding the advice
won't run into this situation. But it would be even nicer if
we pretend the two interfaces are completely separate, and
disallow any cross-linking. That is, a call to old-style
deregister should never remove a new-style callback even if it
is the same function pointer, and a call to new-style callback
using only callbackIDs obtained legitimately should never
remove an old-style callback (of course, since our callback
IDs are sequential, and there is still coupling under the
hood, you can easily guess the callbackID of an old style
registration and use new-style deregistration to nuke it - but
that starts to be blatantly bad coding on your part rather
than a surprising result on what looks like reasonable
stand-alone API).
With this patch, you can now register a global lifecycle event
handler twice, by using both old and new APIs; if such an event
occurs, your callback will be entered twice. But that is not a
problem in practice, since it is already possible to use the
new API to register both a global and per-domain event handler
using the same function, which will likewise fire your callback
twice for that domain. Duplicates are still prevented when
using the same API with same parameters twice (old-style twice,
new-style global twice, or new-style per-domain with same domain
twice), and things are still bounded (it is not possible to
register a single function pointer more than N+2 times per event
id, where N is the number of domains available on the connection).
Besides, it has always been possible to register as many
separate function pointers on the same event id as desired,
through either old or new style API, where the bound there is
the physical limitation of writing a program with enough
distinct function pointers.
Adding another event registration in the testsuite is sufficient
to cover this, where the test fails without the rest of the patch.
* src/conf/object_event.c (_virObjectEventCallback): Add field.
(virObjectEventCallbackLookup): Add argument.
(virObjectEventCallbackListAddID, virObjectEventStateCallbackID):
Adjust callers.
* tests/objecteventtest.c (testDomainCreateXMLMixed): Enhance test.
Signed-off-by: Eric Blake <eblake@redhat.com>
On the surface, this sequence of API calls should succeed:
id1 = virConnectDomainEventRegisterAny(..., VIR_DOMAIN_EVENT_ID_LIFECYCLE,...);
id2 = virConnectDomainEventRegisterAny(..., VIR_DOMAIN_EVENT_ID_RTC_CHANGE,...);
virConnectDomainEventDeregisterAny(id1);
id1 = virConnectDomainEventRegisterAny(..., VIR_DOMAIN_EVENT_ID_LIFECYCLE,...);
And for test:///default, it does. But for qemu:///system, it fails:
libvirt: XML-RPC error : internal error: domain event 0 already registered
Looking closer, the bug is caused by miscommunication between
the object event engine and the client side of the remote driver.
In our implementation, we set up a single server-side event per
eventID, then the client side replicates that one event to all
callbacks that have been registered client side. To know when
to turn the server side eventID on or off, the client side must
track how many events for the same eventID have been registered.
But while our code was filtering by eventID on event registration,
it did not filter on event deregistration. So the above API calls
resulted in the deregister returning 1 instead of 0, so no RPC
deregister was issued, and the final register detects on the
server side that the server is already handling eventID 0.
Unfortunately, since the problem is only observable on remote
connections, it's not possible to enhance objecteventtest to
expose the semantics using only public API entry points.
* src/conf/object_event.c (virObjectEventCallbackListCount): New
function.
(virObjectEventCallbackListAddID)
(virObjectEventCallbackListRemoveID)
(virObjectEventCallbackListMarkDeleteID): Use it.
Signed-off-by: Eric Blake <eblake@redhat.com>
Tighten up scope after the previous patch avoided using
internals. This will also make it easier to change
internal implementation without having to chase down quite
as many impacted callers or worrying about two files getting
implementations out of sync.
* src/conf/object_event_private.h
(virObjectEventCallbackListAddID, virObjectEventQueueClear)
(virObjectEventStateLock, virObjectEventStateUnlock)
(virObjectEventTimer): Drop prototype.
(_virObjectEventCallbackList, _virObjectEventState)
(_virObjectEventCallback): Move...
* src/conf/object_event.c: ...here.
(virObjectEventCallbackListAddID, virObjectEventQueueClear)
(virObjectEventStateLock, virObjectEventStateUnlock)
(virObjectEventTimer): Mark private.
Signed-off-by: Eric Blake <eblake@redhat.com>
Right now, the older virConnectDomainEventRegister (takes a
function pointer, returns 0 on success) and the newer
virConnectDomainEventRegisterID (takes an eventID, returns a
callbackID) share the underlying implementation (the older
API ends up consuming a callbackID for eventID 0 under the
hood). We implemented that by a lot of copy and pasted
code between object_event.c and domain_event.c, according to
whether we are dealing with a function pointer or an eventID.
However, our copy and paste is not symmetric. Consider this
sequence:
id1 = virConnectDomainEventRegisterAny(conn, dom,
VIR_DOMAIN_EVENT_ID_LIFECYCLE,
VIR_DOMAIN_EVENT_CALLBACK(callback), NULL, NULL);
virConnectDomainEventRegister(conn, callback, NULL, NULL);
virConnectDomainEventDeregister(conn, callback);
virConnectDomainEventDeregsiterAny(conn, id1);
the first three calls would succeed, but the third call ended
up nuking the id1 callbackID (the per-domain new-style handler),
then the fourth call failed with an error about an unknown
callbackID, leaving us with the global handler (old-style) still
live and receiving events. It required another old-style
deregister to clean up the mess. Root cause was that
virDomainEventCallbackList{Remove,MarkDelete} were only
checking for function pointer match, rather than also checking
for whether the registration was global.
Rather than playing with the guts of object_event ourselves
in domain_event, it is nicer to add a mapping function for the
internal callback id, then share common code for event removal.
For now, the function-to-id mapping is used only internally;
I thought about whether a new public API to let a user learn
the callback would be useful, but decided exposing this to the
user is probably a disservice, since we already publicly
document that they should avoid the old style, and since this
patch already demonstrates that older libvirt versions have
weird behavior when mixing old and new styles.
And like all good bug fix patches, I enhanced the testsuite,
validating that the changes in tests/ expose the failure
without the rest of the patch.
* src/conf/object_event.c (virObjectEventCallbackLookup)
(virObjectEventStateCallbackID): New functions.
(virObjectEventCallbackLookup): Use helper function.
* src/conf/object_event_private.h (virObjectEventStateCallbackID):
Declare new function.
* src/conf/domain_event.c (virDomainEventStateRegister)
(virDomainEventStateDeregister): Let common code handle the
complexity.
(virDomainEventCallbackListRemove)
(virDomainEventCallbackListMarkDelete)
(virDomainEventCallbackListAdd): Drop unused functions.
* tests/objecteventtest.c (testDomainCreateXMLMixed): New test.
Signed-off-by: Eric Blake <eblake@redhat.com>
We might as well take advantage of viralloc.h instead of open-coding
array management ourselves. While at it, I simplified several
places that were doing repetitive pointer chasing to use an
intermediate variable for legibility (some other places remain,
but they will disapper in later refactoring patches).
* src/conf/object_event_private.h (_virObjectEventCallbackList):
Use size_t for count.
* src/conf/object_event.c (_virObjectEventQueue): Likewise.
(virObjectEventCallbackListRemoveID): Use VIR_DELETE_ELEMENT.
(virObjectEventQueuePush, virObjectEventCallbackListAddID): Use
VIR_APPEND_ELEMENT.
(virObjectEventCallbackListEventID)
(virObjectEventStateDispatchCallbacks): Simplify code.
Signed-off-by: Eric Blake <eblake@redhat.com>
No need to use an int that only ever stores 0 and 1.
* src/conf/object_event_private.h (_virObjectEventCallback):
Change deleted to bool.
* src/conf/object_event.c (virObjectEventDispatchMatchCallback):
Switch return type to bool.
(virObjectEventCallbackListMarkDeleteID): Update client.
* src/conf/domain_event.c (virDomainEventCallbackListMarkDelete):
Likewise.
Any file with access to object_event_private.h also has access to
the internals of virObjectEvent, without needing an accessor
function. Not to mention the accessor function was doing type
checks that would always succeed.
* src/conf/object_event_private.h (virObjectEventGetEventID): Drop.
* src/conf/object_event.c (virObjectEventGetEventID): Drop.
(virObjectEventDispatchMatchCallback): Simplify caller.
* src/conf/domain_event.c (virDomainEventDispatchDefaultFunc):
Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
