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

On Thu, Mar 24, 2011 at 10:24:26AM -0600, Jim Fehlig wrote: > 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. ACK, gets my vote with the changes you already mentioned. You might also want to add the PAUSED/RESUMED events to the Suspend and Resume driver methods.