Re: [libvirt] [PATCH 1/8] Add event callbacks to libxl driver

Markus Groß wrote: > --- > src/libxl/libxl_conf.h | 8 ++- > src/libxl/libxl_driver.c | 199 +++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 202 insertions(+), 5 deletions(-) > > diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h > index bb49d35..a86df9a 100644 > --- a/src/libxl/libxl_conf.h > +++ b/src/libxl/libxl_conf.h > @@ -26,6 +26,7 @@ > > # include "internal.h" > # include "domain_conf.h" > +# include "domain_event.h" > # include "capabilities.h" > # include "configmake.h" > # include "bitmap.h" > @@ -57,6 +58,12 @@ struct _libxlDriverPrivate { > virBitmapPtr reservedVNCPorts; > virDomainObjList domains; > > + /* A list of callbacks */ > + virDomainEventCallbackListPtr domainEventCallbacks; > + virDomainEventQueuePtr domainEventQueue; > + int domainEventTimer; > + int domainEventDispatching; > + > char *configDir; > char *autostartDir; > char *logDir; > @@ -87,5 +94,4 @@ int > libxlBuildDomainConfig(libxlDriverPrivatePtr driver, > virDomainDefPtr def, libxl_domain_config *d_config); > > - > #endif /* LIBXL_CONF_H */ > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index 615cb47..ad95e2b 100644 > --- a/src/libxl/libxl_driver.c > +++ b/src/libxl/libxl_driver.c > @@ -1,6 +1,7 @@ > /*---------------------------------------------------------------------------*/ > /* Copyright (c) 2011 SUSE LINUX Products GmbH, Nuernberg, Germany. > * Copyright (C) 2011 Univention GmbH. > + * Copyright (C) 2006-2011 Red Hat, Inc. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public > @@ -15,6 +16,11 @@ > * You should have received a copy of the GNU Lesser General Public > * License along with this library; if not, write to the Free Software > * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + * > + * Authors: > + * Jim Fehlig <jfehlig(a)novell.com&gt; > + * Markus Groß <gross(a)univention.de&gt; > + * Daniel P. Berrange <berrange(a)redhat.com&gt; > I have no objections to adding authors list, but we should probably add it to the other libxl files to be consistent. But that can be done with a subsequent patch. > */ > /*---------------------------------------------------------------------------*/ > > @@ -99,6 +105,58 @@ libxlDomainObjPrivateFree(void *data) > VIR_FREE(priv); > } > > +static void > +libxlDomainEventDispatchFunc(virConnectPtr conn, virDomainEventPtr event, > + virConnectDomainEventGenericCallback cb, > + void *cbopaque, void *opaque) > +{ > + libxlDriverPrivatePtr driver = opaque; > + > + /* Drop the lock whle dispatching, for sake of re-entrancy */ > + libxlDriverUnlock(driver); > + virDomainEventDispatchDefaultFunc(conn, event, cb, cbopaque, NULL); > + libxlDriverLock(driver); > +} > + > +static void > +libxlDomainEventFlush(int timer ATTRIBUTE_UNUSED, void *opaque) > +{ > + libxlDriverPrivatePtr driver = opaque; > + virDomainEventQueue tempQueue; > + > + libxlDriverLock(driver); > + > + driver->domainEventDispatching = 1; > + > + /* Copy the queue, so we're reentrant safe */ > + tempQueue.count = driver->domainEventQueue->count; > + tempQueue.events = driver->domainEventQueue->events; > + driver->domainEventQueue->count = 0; > + driver->domainEventQueue->events = NULL; > + > + virEventUpdateTimeout(driver->domainEventTimer, -1); > + virDomainEventQueueDispatch(&tempQueue, > + driver->domainEventCallbacks, > + libxlDomainEventDispatchFunc, > + driver); > + > + /* Purge any deleted callbacks */ > + virDomainEventCallbackListPurgeMarked(driver->domainEventCallbacks); > + > + driver->domainEventDispatching = 0; > + libxlDriverUnlock(driver); > +} > + > +/* driver must be locked before calling */ > +static void > +libxlDomainEventQueue(libxlDriverPrivatePtr driver, virDomainEventPtr event) > +{ > + if (virDomainEventQueuePush(driver->domainEventQueue, event) < 0) > + virDomainEventFree(event); > + if (driver->domainEventQueue->count == 1) > + virEventUpdateTimeout(driver->domainEventTimer, 0); > +} > + > /* > * Remove reference to domain object. > */ > @@ -187,6 +245,7 @@ static void libxlEventHandler(int watch, > libxlDriverPrivatePtr driver = libxl_driver; > virDomainObjPtr vm = data; > libxlDomainObjPrivatePtr priv; > + virDomainEventPtr dom_event = NULL; > libxl_event event; > libxl_dominfo info; > > @@ -225,6 +284,9 @@ static void libxlEventHandler(int watch, > switch (info.shutdown_reason) { > case SHUTDOWN_poweroff: > case SHUTDOWN_crash: > + dom_event = virDomainEventNewFromObj(vm, > + VIR_DOMAIN_EVENT_STOPPED, > + VIR_DOMAIN_EVENT_STOPPED_CRASHED); > I don't think we want the CRASHED detail in case of a normal poweroff :-). > libxlVmReap(driver, vm, 0); > if (!vm->persistent) { > virDomainRemoveInactive(&driver->domains, vm); > @@ -244,6 +306,11 @@ static void libxlEventHandler(int watch, > cleanup: > if (vm) > virDomainObjUnlock(vm); > + if (dom_event) { > + libxlDriverLock(driver); > + libxlDomainEventQueue(driver, dom_event); > + libxlDriverUnlock(driver); > + } > libxl_free_event(&event); > } > > @@ -303,6 +370,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, > { > libxl_domain_config d_config; > virDomainDefPtr def = vm->def; > + virDomainEventPtr event = NULL; > int ret; > uint32_t domid = 0; > char *dom_xml = NULL; > @@ -347,9 +415,14 @@ libxlVmStart(libxlDriverPrivatePtr driver, > vm->state = VIR_DOMAIN_PAUSED; > } > > + > if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) > goto error; > > + event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, > + VIR_DOMAIN_EVENT_STARTED_BOOTED); > + libxlDomainEventQueue(driver, event); > + > libxl_domain_config_destroy(&d_config); > VIR_FREE(dom_xml); > return 0; > @@ -447,6 +520,13 @@ libxlShutdown(void) > VIR_FREE(libxl_driver->libDir); > VIR_FREE(libxl_driver->saveDir); > > + /* Free domain callback list */ > + virDomainEventCallbackListFree(libxl_driver->domainEventCallbacks); > + virDomainEventQueueFree(libxl_driver->domainEventQueue); > + > + if (libxl_driver->domainEventTimer != -1) > + virEventRemoveTimeout(libxl_driver->domainEventTimer); > + > libxlDriverUnlock(libxl_driver); > virMutexDestroy(&libxl_driver->lock); > VIR_FREE(libxl_driver); > @@ -556,6 +636,16 @@ libxlStartup(int privileged) { > } > VIR_FREE(log_file); > > + /* Init callback list */ > + if (VIR_ALLOC(libxl_driver->domainEventCallbacks) < 0) > + goto out_of_memory; > + if (!(libxl_driver->domainEventQueue = virDomainEventQueueNew())) > + goto out_of_memory; > + if ((libxl_driver->domainEventTimer = > + virEventAddTimeout(-1, libxlDomainEventFlush, libxl_driver, NULL)) < 0) > + goto error; > + > + > libxl_driver->logger = > (xentoollog_logger *)xtl_createlogger_stdiostream(libxl_driver->logger_file, XTL_DEBUG, 0); > if (!libxl_driver->logger) { > @@ -698,6 +788,11 @@ libxlOpen(virConnectPtr conn, > static int > libxlClose(virConnectPtr conn ATTRIBUTE_UNUSED) > { > + libxlDriverPrivatePtr driver = conn->privateData; > + > + libxlDriverLock(driver); > + virDomainEventCallbackListRemoveConn(conn, driver->domainEventCallbacks); > + libxlDriverUnlock(driver); > conn->privateData = NULL; > return 0; > } > @@ -1026,6 +1121,7 @@ libxlDomainDestroy(virDomainPtr dom) > virDomainObjPtr vm; > int ret = -1; > libxlDomainObjPrivatePtr priv; > + virDomainEventPtr event = NULL; > > libxlDriverLock(driver); > vm = virDomainFindByUUID(&driver->domains, dom->uuid); > @@ -1043,6 +1139,9 @@ libxlDomainDestroy(virDomainPtr dom) > goto cleanup; > } > > + event = virDomainEventNewFromObj(vm,VIR_DOMAIN_EVENT_STOPPED, > + VIR_DOMAIN_EVENT_STOPPED_DESTROYED); > + > priv = vm->privateData; > if (libxlVmReap(driver, vm, 1) != 0) { > libxlError(VIR_ERR_INTERNAL_ERROR, > @@ -1060,6 +1159,8 @@ libxlDomainDestroy(virDomainPtr dom) > cleanup: > if (vm) > virDomainObjUnlock(vm); > + if (event) > + libxlDomainEventQueue(driver, event); > libxlDriverUnlock(driver); > return ret; > } > @@ -1193,6 +1294,7 @@ libxlDomainDefineXML(virConnectPtr conn, const char *xml) > virDomainDefPtr def = NULL; > virDomainObjPtr vm = NULL; > virDomainPtr dom = NULL; > + virDomainEventPtr event = NULL; > int dupVM; > > libxlDriverLock(driver); > @@ -1220,10 +1322,17 @@ libxlDomainDefineXML(virConnectPtr conn, const char *xml) > if (dom) > dom->id = vm->def->id; > > + event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_DEFINED, > + !dupVM ? > + VIR_DOMAIN_EVENT_DEFINED_ADDED : > + VIR_DOMAIN_EVENT_DEFINED_UPDATED); > + > cleanup: > virDomainDefFree(def); > if (vm) > virDomainObjUnlock(vm); > + if (event) > + libxlDomainEventQueue(driver, event); > libxlDriverUnlock(driver); > return dom; > } > @@ -1233,6 +1342,7 @@ libxlDomainUndefine(virDomainPtr dom) > { > libxlDriverPrivatePtr driver = dom->conn->privateData; > virDomainObjPtr vm; > + virDomainEventPtr event = NULL; > int ret = -1; > > libxlDriverLock(driver); > @@ -1264,6 +1374,9 @@ libxlDomainUndefine(virDomainPtr dom) > vm) < 0) > goto cleanup; > > + event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_UNDEFINED, > + VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); > + > virDomainRemoveInactive(&driver->domains, vm); > vm = NULL; > ret = 0; > @@ -1271,11 +1384,51 @@ libxlDomainUndefine(virDomainPtr dom) > cleanup: > if (vm) > virDomainObjUnlock(vm); > + if (event) > + libxlDomainEventQueue(driver, event); > libxlDriverUnlock(driver); > return ret; > } > > static int > +libxlDomainEventRegister(virConnectPtr conn, > + virConnectDomainEventCallback callback, void *opaque, > + virFreeCallback freecb) > +{ > + libxlDriverPrivatePtr driver = conn->privateData; > + int ret; > + > + libxlDriverLock(driver); > + ret = virDomainEventCallbackListAdd(conn, driver->domainEventCallbacks, > + callback, opaque, freecb); > + libxlDriverUnlock(driver); > + > + return ret; > +} > + > + > +static int > +libxlDomainEventDeregister(virConnectPtr conn, > + virConnectDomainEventCallback callback) > +{ > + libxlDriverPrivatePtr driver = conn->privateData; > + int ret; > + > + libxlDriverLock(driver); > + if (driver->domainEventDispatching) > + ret = virDomainEventCallbackListMarkDelete(conn, > + driver->domainEventCallbacks, > + callback); > + else > + ret = virDomainEventCallbackListRemove(conn, > + driver->domainEventCallbacks, > + callback); > + libxlDriverUnlock(driver); > + > + return ret; > +} > + > +static int > libxlDomainIsActive(virDomainPtr dom) > { > libxlDriverPrivatePtr driver = dom->conn->privateData; > @@ -1319,6 +1472,44 @@ libxlDomainIsPersistent(virDomainPtr dom) > return ret; > } > > +static int > +libxlDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int eventID, > + virConnectDomainEventGenericCallback callback, > + void *opaque, virFreeCallback freecb) > +{ > + libxlDriverPrivatePtr driver = conn->privateData; > + int ret; > + > + libxlDriverLock(driver); > + ret = virDomainEventCallbackListAddID(conn, driver->domainEventCallbacks, > + dom, eventID, callback, opaque, > + freecb); > + libxlDriverUnlock(driver); > + > + return ret; > +} > + > + > +static int > +libxlDomainEventDeregisterAny(virConnectPtr conn, int callbackID) > +{ > + libxlDriverPrivatePtr driver = conn->privateData; > + int ret; > + > + libxlDriverLock(driver); > + if (driver->domainEventDispatching) > + ret = virDomainEventCallbackListMarkDeleteID(conn, > + driver->domainEventCallbacks, > + callbackID); > + else > + ret = virDomainEventCallbackListRemoveID(conn, > + driver->domainEventCallbacks, > + callbackID); > + libxlDriverUnlock(driver); > + > + return ret; > +} > + > This all looks good and quite similar to event callback code in qemu driver, but I'd prefer a quick review by someone (danpb?) familiar with its subtleties.