External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-12-22 05:35:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

event: don't let old-style events clobber per-domain events

Browse Source 
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>
This commit is contained in:
Eric Blake 2014-01-03 16:50:14 -07:00
parent 53827c125e
commit 4221d64fcb
4 changed files with 173 additions and 169 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

171
src/conf/domain_event.c
View File

@ -360,111 +360,6 @@ virDomainEventDeviceRemovedDispose(void *obj)
}
/**
* virDomainEventCallbackListRemove:
* @conn: pointer to the connection
* @cbList: the list
* @callback: the callback to remove
*
* Internal function to remove a callback from a virObjectEventCallbackListPtr,
* when registered via the older virConnectDomainEventRegister with no
* callbackID
*/
static int
virDomainEventCallbackListRemove(virConnectPtr conn,
virObjectEventCallbackListPtr cbList,
virConnectDomainEventCallback callback)
{
int ret = 0;
size_t i;
for (i = 0; i < cbList->count; i++) {
if (cbList->callbacks[i]->cb == VIR_OBJECT_EVENT_CALLBACK(callback) &&
cbList->callbacks[i]->eventID == VIR_DOMAIN_EVENT_ID_LIFECYCLE &&
cbList->callbacks[i]->conn == conn) {
virFreeCallback freecb = cbList->callbacks[i]->freecb;
if (freecb)
(*freecb)(cbList->callbacks[i]->opaque);
virObjectUnref(cbList->callbacks[i]->conn);
VIR_FREE(cbList->callbacks[i]);
if (i < (cbList->count - 1))
memmove(cbList->callbacks + i,
cbList->callbacks + i + 1,
sizeof(*(cbList->callbacks)) *
(cbList->count - (i + 1)));
if (VIR_REALLOC_N(cbList->callbacks,
cbList->count - 1) < 0) {
; /* Failure to reduce memory allocation isn't fatal */
}
cbList->count--;
for (i = 0; i < cbList->count; i++) {
if (!cbList->callbacks[i]->deleted)
ret++;
}
return ret;
}
}
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("could not find event callback for removal"));
return -1;
}
static int
virDomainEventCallbackListMarkDelete(virConnectPtr conn,
virObjectEventCallbackListPtr cbList,
virConnectDomainEventCallback callback)
{
int ret = 0;
size_t i;
for (i = 0; i < cbList->count; i++) {
if (cbList->callbacks[i]->cb == VIR_OBJECT_EVENT_CALLBACK(callback) &&
cbList->callbacks[i]->eventID == VIR_DOMAIN_EVENT_ID_LIFECYCLE &&
cbList->callbacks[i]->conn == conn) {
cbList->callbacks[i]->deleted = true;
for (i = 0; i < cbList->count; i++) {
if (!cbList->callbacks[i]->deleted)
ret++;
}
return ret;
}
}
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("could not find event callback for deletion"));
return -1;
}
/**
* virDomainEventCallbackListAdd:
* @conn: pointer to the connection
* @cbList: the list
* @callback: the callback to add
* @opaque: opaque data to pass to @callback
* @freecb: callback to free @opaque
*
* Internal function to add a callback from a virObjectEventCallbackListPtr,
* when registered via the older virConnectDomainEventRegister.
*/
static int
virDomainEventCallbackListAdd(virConnectPtr conn,
virObjectEventCallbackListPtr cbList,
virConnectDomainEventCallback callback,
void *opaque,
virFreeCallback freecb)
{
return virObjectEventCallbackListAddID(conn, cbList, NULL, NULL, 0,
virDomainEventClass,
VIR_DOMAIN_EVENT_ID_LIFECYCLE,
VIR_OBJECT_EVENT_CALLBACK(callback),
opaque, freecb, NULL);
}
static void *
virDomainEventNew(virClassPtr klass,
int eventID,
@ -1386,37 +1281,14 @@ virDomainEventStateRegister(virConnectPtr conn,
void *opaque,
virFreeCallback freecb)
{
int ret = -1;
if (virDomainEventsInitialize() < 0)
return -1;
virObjectEventStateLock(state);
if ((state->callbacks->count == 0) &&
(state->timer == -1) &&
(state->timer = virEventAddTimeout(-1,
virObjectEventTimer,
state,
NULL)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("could not initialize domain event timer"));
goto cleanup;
}
ret = virDomainEventCallbackListAdd(conn, state->callbacks,
callback, opaque, freecb);
if (ret == -1 &&
state->callbacks->count == 0 &&
state->timer != -1) {
virEventRemoveTimeout(state->timer);
state->timer = -1;
}
cleanup:
virObjectEventStateUnlock(state);
return ret;
return virObjectEventStateRegisterID(conn, state, NULL, NULL, 0,
virDomainEventClass,
VIR_DOMAIN_EVENT_ID_LIFECYCLE,
VIR_OBJECT_EVENT_CALLBACK(callback),
opaque, freecb, NULL);
}
@ -1467,34 +1339,25 @@ virDomainEventStateRegisterID(virConnectPtr conn,
* virDomainEventStateDeregister:
* @conn: connection to associate with callback
* @state: object event state
* @callback: function to remove from event
* @cb: function to remove from event
*
* Unregister the function @callback with connection @conn,
* from @state, for lifecycle events.
* Unregister the function @cb with connection @conn, from @state, for
* lifecycle events.
*
* Returns: the number of lifecycle callbacks still registered, or -1 on error
*/
int
virDomainEventStateDeregister(virConnectPtr conn,
virObjectEventStatePtr state,
virConnectDomainEventCallback callback)
virConnectDomainEventCallback cb)
{
int ret;
int callbackID;
virObjectEventStateLock(state);
if (state->isDispatching)
ret = virDomainEventCallbackListMarkDelete(conn,
state->callbacks, callback);
else
ret = virDomainEventCallbackListRemove(conn, state->callbacks, callback);
if (state->callbacks->count == 0 &&
state->timer != -1) {
virEventRemoveTimeout(state->timer);
state->timer = -1;
virObjectEventQueueClear(state->queue);
}
virObjectEventStateUnlock(state);
return ret;
callbackID = virObjectEventStateCallbackID(conn, state,
virDomainEventClass,
VIR_DOMAIN_EVENT_ID_LIFECYCLE,
VIR_OBJECT_EVENT_CALLBACK(cb));
if (callbackID < 0)
return -1;
return virObjectEventStateDeregisterID(conn, state, callbackID);
}

94
src/conf/object_event.c
View File

@ -207,6 +207,50 @@ virObjectEventCallbackListPurgeMarked(virObjectEventCallbackListPtr cbList)
}
/**
* virObjectEventCallbackLookup:
* @conn: pointer to the connection
* @cbList: the list
* @uuid: the uuid of the object to filter on
* @klass: the base event class
* @eventID: the event ID
* @callback: the callback to locate
*
* Internal function to determine if @callback already has a
* callbackID in @cbList for the given @conn and other filters.
* Return the id if found, or -1 with no error issued if not present.
*/
static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
virObjectEventCallbackLookup(virConnectPtr conn,
virObjectEventCallbackListPtr cbList,
unsigned char uuid[VIR_UUID_BUFLEN],
virClassPtr klass,
int eventID,
virConnectObjectEventGenericCallback callback)
{
int ret = -1;
size_t i;
for (i = 0; i < cbList->count; i++) {
virObjectEventCallbackPtr cb = cbList->callbacks[i];
if (cb->deleted)
continue;
if (cb->cb == callback &&
cb->klass == klass &&
cb->eventID == eventID &&
cb->conn == conn &&
((uuid && cb->meta &&
memcmp(cb->meta->uuid, uuid, VIR_UUID_BUFLEN) == 0) ||
(!uuid && !cb->meta))) {
ret = cb->callbackID;
break;
}
}
return ret;
}
/**
* virObjectEventCallbackListAddID:
* @conn: pointer to the connection
@ -251,19 +295,11 @@ virObjectEventCallbackListAddID(virConnectPtr conn,
}
/* check if we already have this callback on our list */
for (i = 0; i < cbList->count; i++) {
if (cbList->callbacks[i]->cb == VIR_OBJECT_EVENT_CALLBACK(callback) &&
cbList->callbacks[i]->klass == klass &&
cbList->callbacks[i]->eventID == eventID &&
cbList->callbacks[i]->conn == conn &&
((uuid && cbList->callbacks[i]->meta &&
memcmp(cbList->callbacks[i]->meta->uuid,
uuid, VIR_UUID_BUFLEN) == 0) ||
(!uuid && !cbList->callbacks[i]->meta))) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("event callback already tracked"));
return -1;
}
if (virObjectEventCallbackLookup(conn, cbList, uuid,
klass, eventID, callback) != -1) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("event callback already tracked"));
return -1;
}
/* Allocate new event */
if (VIR_ALLOC(event) < 0)
@ -786,6 +822,38 @@ virObjectEventStateDeregisterID(virConnectPtr conn,
return ret;
}
/**
* virObjectEventStateCallbackID:
* @conn: connection associated with callback
* @state: object event state
* @klass: the base event class
* @eventID: the event ID
* @callback: function registered as a callback
*
* Returns the callbackID of @callback, or -1 with an error issued if the
* function is not currently registered.
*/
int
virObjectEventStateCallbackID(virConnectPtr conn,
virObjectEventStatePtr state,
virClassPtr klass,
int eventID,
virConnectObjectEventGenericCallback callback)
{
int ret = -1;
virObjectEventStateLock(state);
ret = virObjectEventCallbackLookup(conn, state->callbacks, NULL,
klass, eventID, callback);
virObjectEventStateUnlock(state);
if (ret < 0)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("event callback function %p not registered"),
callback);
return ret;
}
/**
* virObjectEventStateEventID:

9
src/conf/object_event_private.h
View File

@ -86,6 +86,15 @@ struct _virObjectEvent {
virClassPtr
virClassForObjectEvent(void);
int
virObjectEventStateCallbackID(virConnectPtr conn,
virObjectEventStatePtr state,
virClassPtr klass,
int eventID,
virConnectObjectEventGenericCallback callback)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
ATTRIBUTE_NONNULL(5);
int
virObjectEventCallbackListAddID(virConnectPtr conn,
virObjectEventCallbackListPtr cbList,

68
tests/objecteventtest.c
View File

@ -167,7 +167,7 @@ cleanup:
}
static int
testDomainCreateXML(const void *data)
testDomainCreateXMLNew(const void *data)
{
const objecteventTest *test = data;
lifecycleEventCounter counter;
@ -207,6 +207,67 @@ cleanup:
return ret;
}
static int
testDomainCreateXMLMixed(const void *data)
{
const objecteventTest *test = data;
lifecycleEventCounter counter;
virDomainPtr dom;
int ret = -1;
int id = -1;
bool registered = false;
lifecycleEventCounter_reset(&counter);
/* Fun with mixing old and new API. Handler should be fired twice,
* once for each registration. */
if (!(dom = virDomainCreateXML(test->conn, domainDef, 0))
goto cleanup;
id = virConnectDomainEventRegisterAny(test->conn, dom,
VIR_DOMAIN_EVENT_ID_LIFECYCLE,
VIR_DOMAIN_EVENT_CALLBACK(&domainLifecycleCb),
&counter, NULL);
if (id < 0)
goto cleanup;
if (virDomainDestroy(dom) < 0)
goto cleanup;
if (virConnectDomainEventRegister(test->conn,
domainLifecycleCb,
&counter, NULL) != 0)
goto cleanup;
registered = true;
dom = virDomainCreateXML(test->conn, domainDef, 0);
if (dom == NULL || virEventRunDefaultImpl() < 0)
goto cleanup;
if (counter.startEvents != 2 || counter.unexpectedEvents > 0)
goto cleanup;
if (virConnectDomainEventDeregister(test->conn, domainLifecycleCb) != 0)
goto cleanup;
registered = false;
if (virConnectDomainEventDeregisterAny(test->conn, id) != 0)
goto cleanup;
id = -1;
ret = 0;
cleanup:
if (id >= 0)
virConnectDomainEventDeregisterAny(test->conn, id);
if (registered)
virConnectDomainEventDeregister(test->conn, domainLifecycleCb);
if (dom != NULL) {
virDomainUndefine(dom);
virDomainDestroy(dom);
virDomainFree(dom);
}
return ret;
}
static int
testDomainDefine(const void *data)
{
@ -471,7 +532,10 @@ mymain(void)
testDomainCreateXMLOld, &test) < 0)
ret = EXIT_FAILURE;
if (virtTestRun("Domain createXML start event (new API)",
testDomainCreateXML, &test) < 0)
testDomainCreateXMLNew, &test) < 0)
ret = EXIT_FAILURE;
if (virtTestRun("Domain createXML start event (both API)",
testDomainCreateXMLMixed, &test) < 0)
ret = EXIT_FAILURE;
if (virtTestRun("Domain (un)define events", testDomainDefine, &test) < 0)
ret = EXIT_FAILURE;