From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; To allow a virLockManagerPtr to be created directly from a driver table struct, replace the virLockManagerPluginPtr parameter with a virLockDriverPtr parameter. * src/locking/domain_lock.c, src/locking/lock_manager.c, src/locking/lock_manager.h: Replace plugin param with a driver in virLockManagerNew Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/locking/domain_lock.c | 2 +- src/locking/lock_manager.c | 31 +++++++++++++++++++------------ src/locking/lock_manager.h | 3 ++- 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c index 55f5640..ba49f9c 100644 --- a/src/locking/domain_lock.c +++ b/src/locking/domain_lock.c @@ -124,7 +124,7 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, memcpy(params[0].value.uuid, dom->def->uuid, VIR_UUID_BUFLEN); - if (!(lock = virLockManagerNew(plugin, + if (!(lock = virLockManagerNew(virLockManagerPluginGetDriver(plugin), VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN, ARRAY_CARDINALITY(params), params, diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index 18e739e..1d9c1bf 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -39,11 +39,11 @@ #define VIR_FROM_THIS VIR_FROM_LOCKING -#define CHECK_PLUGIN(field, errret) \ - if (!plugin->driver->field) { \ - virReportError(VIR_ERR_INTERNAL_ERROR, \ - _("Missing '%s' field in lock manager driver"), \ - #field); \ +#define CHECK_DRIVER(field, errret) \ + if (!driver->field) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("Missing '%s' field in lock manager driver"), \ + #field); \ return errret; \ } @@ -265,9 +265,16 @@ bool virLockManagerPluginUsesState(virLockManagerPluginPtr plugin) } +virLockDriverPtr virLockManagerPluginGetDriver(virLockManagerPluginPtr plugin) +{ + VIR_DEBUG("plugin=%p", plugin); + + return plugin->driver; +} + /** * virLockManagerNew: - * @plugin: the plugin implementation to use + * @driver: the lock manager implementation to use * @type: the type of process to be supervised * @flags: optional flags, currently unused * @@ -276,27 +283,27 @@ bool virLockManagerPluginUsesState(virLockManagerPluginPtr plugin) * * Returns a new lock manager context */ -virLockManagerPtr virLockManagerNew(virLockManagerPluginPtr plugin, +virLockManagerPtr virLockManagerNew(virLockDriverPtr driver, unsigned int type, size_t nparams, virLockManagerParamPtr params, unsigned int flags) { virLockManagerPtr lock; - VIR_DEBUG("plugin=%p type=%u nparams=%zu params=%p flags=%x", - plugin, type, nparams, params, flags); + VIR_DEBUG("driver=%p type=%u nparams=%zu params=%p flags=%x", + driver, type, nparams, params, flags); virLockManagerLogParams(nparams, params); - CHECK_PLUGIN(drvNew, NULL); + CHECK_DRIVER(drvNew, NULL); if (VIR_ALLOC(lock) < 0) { virReportOOMError(); return NULL; } - lock->driver = plugin->driver; + lock->driver = driver; - if (plugin->driver->drvNew(lock, type, nparams, params, flags) < 0) { + if (driver->drvNew(lock, type, nparams, params, flags) < 0) { VIR_FREE(lock); return NULL; } diff --git a/src/locking/lock_manager.h b/src/locking/lock_manager.h index b548d45..25c7f7f 100644 --- a/src/locking/lock_manager.h +++ b/src/locking/lock_manager.h @@ -37,8 +37,9 @@ void virLockManagerPluginUnref(virLockManagerPluginPtr plugin); const char *virLockManagerPluginGetName(virLockManagerPluginPtr plugin); bool virLockManagerPluginUsesState(virLockManagerPluginPtr plugin); +virLockDriverPtr virLockManagerPluginGetDriver(virLockManagerPluginPtr plugin); -virLockManagerPtr virLockManagerNew(virLockManagerPluginPtr plugin, +virLockManagerPtr virLockManagerNew(virLockDriverPtr driver, unsigned int type, size_t nparams, virLockManagerParamPtr params, -- 1.7.11.2

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; While the QEMU monitor/agent do not want JSON strings pretty printed, other parts of libvirt might. Instead of hardcoding QEMU's desired behaviour in virJSONValueToString(), add a boolean flag to control pretty printing Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/qemu/qemu_agent.c | 10 +++++----- src/qemu/qemu_monitor_json.c | 12 ++++++------ src/util/json.c | 9 +++++---- src/util/json.h | 3 ++- 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 15af758..513f1d5 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -993,7 +993,7 @@ qemuAgentCommand(qemuAgentPtr mon, memset(&msg, 0, sizeof(msg)); - if (!(cmdstr = virJSONValueToString(cmd))) { + if (!(cmdstr = virJSONValueToString(cmd, false))) { virReportOOMError(); goto cleanup; } @@ -1107,8 +1107,8 @@ qemuAgentCheckError(virJSONValuePtr cmd, { if (virJSONValueObjectHasKey(reply, "error")) { virJSONValuePtr error = virJSONValueObjectGet(reply, "error"); - char *cmdstr = virJSONValueToString(cmd); - char *replystr = virJSONValueToString(reply); + char *cmdstr = virJSONValueToString(cmd, false); + char *replystr = virJSONValueToString(reply, false); /* Log the full JSON formatted command & error */ VIR_DEBUG("unable to execute QEMU command %s: %s", @@ -1129,8 +1129,8 @@ qemuAgentCheckError(virJSONValuePtr cmd, VIR_FREE(replystr); return -1; } else if (!virJSONValueObjectHasKey(reply, "return")) { - char *cmdstr = virJSONValueToString(cmd); - char *replystr = virJSONValueToString(reply); + char *cmdstr = virJSONValueToString(cmd, false); + char *replystr = virJSONValueToString(reply, false); VIR_DEBUG("Neither 'return' nor 'error' is set in the JSON reply %s: %s", cmdstr, replystr); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3ede88d..14842e9 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -243,7 +243,7 @@ qemuMonitorJSONCommandWithFd(qemuMonitorPtr mon, } } - if (!(cmdstr = virJSONValueToString(cmd))) { + if (!(cmdstr = virJSONValueToString(cmd, false))) { virReportOOMError(); goto cleanup; } @@ -328,8 +328,8 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd, { if (virJSONValueObjectHasKey(reply, "error")) { virJSONValuePtr error = virJSONValueObjectGet(reply, "error"); - char *cmdstr = virJSONValueToString(cmd); - char *replystr = virJSONValueToString(reply); + char *cmdstr = virJSONValueToString(cmd, false); + char *replystr = virJSONValueToString(reply, false); /* Log the full JSON formatted command & error */ VIR_DEBUG("unable to execute QEMU command %s: %s", @@ -350,8 +350,8 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd, VIR_FREE(replystr); return -1; } else if (!virJSONValueObjectHasKey(reply, "return")) { - char *cmdstr = virJSONValueToString(cmd); - char *replystr = virJSONValueToString(reply); + char *cmdstr = virJSONValueToString(cmd, false); + char *replystr = virJSONValueToString(reply, false); VIR_DEBUG("Neither 'return' nor 'error' is set in the JSON reply %s: %s", cmdstr, replystr); @@ -3354,7 +3354,7 @@ int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (!(*reply_str = virJSONValueToString(reply))) + if (!(*reply_str = virJSONValueToString(reply, false))) goto cleanup; } diff --git a/src/util/json.c b/src/util/json.c index 5132989..8687a16 100644 --- a/src/util/json.c +++ b/src/util/json.c @@ -1054,14 +1054,15 @@ static int virJSONValueToStringOne(virJSONValuePtr object, return 0; } -char *virJSONValueToString(virJSONValuePtr object) +char *virJSONValueToString(virJSONValuePtr object, + bool pretty) { yajl_gen g; const unsigned char *str; char *ret = NULL; yajl_size_t len; # ifndef HAVE_YAJL2 - yajl_gen_config conf = { 0, " " }; /* Turns off pretty printing since QEMU can't cope */ + yajl_gen_config conf = { pretty ? 1 : 0, pretty ? " " : " "}; # endif VIR_DEBUG("object=%p", object); @@ -1069,8 +1070,8 @@ char *virJSONValueToString(virJSONValuePtr object) # ifdef HAVE_YAJL2 g = yajl_gen_alloc(NULL); if (g) { - yajl_gen_config(g, yajl_gen_beautify, 0); - yajl_gen_config(g, yajl_gen_indent_string, " "); + yajl_gen_config(g, yajl_gen_beautify, pretty ? 1 : 0); + yajl_gen_config(g, yajl_gen_indent_string, pretty ? " " : " "); yajl_gen_config(g, yajl_gen_validate_utf8, 1); } # else diff --git a/src/util/json.h b/src/util/json.h index c8bd504..bdba3dd 100644 --- a/src/util/json.h +++ b/src/util/json.h @@ -132,6 +132,7 @@ int virJSONValueObjectAppendBoolean(virJSONValuePtr object, const char *key, int int virJSONValueObjectAppendNull(virJSONValuePtr object, const char *key); virJSONValuePtr virJSONValueFromString(const char *jsonstring); -char *virJSONValueToString(virJSONValuePtr object); +char *virJSONValueToString(virJSONValuePtr object, + bool pretty); #endif /* __VIR_JSON_H_ */ -- 1.7.11.2

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; It is desirable to be able to query the config params of the thread pool, in order to save the server state. Add virThreadPoolGetMinWorkers, virThreadPoolGetMaxWorkers and virThreadPoolGetPriorityWorkers APIs. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/libvirt_private.syms | 3 +++ src/util/threadpool.c | 19 +++++++++++++++++++ src/util/threadpool.h | 4 ++++ 3 files changed, 26 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 79b4a18..3a23fe6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1085,6 +1085,9 @@ virSysinfoRead; virThreadPoolFree; virThreadPoolNew; virThreadPoolSendJob; +virThreadPoolGetMinWorkers; +virThreadPoolGetMaxWorkers; +virThreadPoolGetPriorityWorkers; # threads.h diff --git a/src/util/threadpool.c b/src/util/threadpool.c index 63c5ea0..2ae11b8 100644 --- a/src/util/threadpool.c +++ b/src/util/threadpool.c @@ -66,6 +66,7 @@ struct _virThreadPool { virCond quit_cond; size_t maxWorkers; + size_t minWorkers; size_t freeWorkers; size_t nWorkers; virThreadPtr workers; @@ -188,7 +189,9 @@ virThreadPoolPtr virThreadPoolNew(size_t minWorkers, if (VIR_ALLOC_N(pool->workers, minWorkers) < 0) goto error; + pool->minWorkers = minWorkers; pool->maxWorkers = maxWorkers; + for (i = 0; i < minWorkers; i++) { if (VIR_ALLOC(data) < 0) { virReportOOMError(); @@ -277,6 +280,22 @@ void virThreadPoolFree(virThreadPoolPtr pool) VIR_FREE(pool); } + +size_t virThreadPoolGetMinWorkers(virThreadPoolPtr pool) +{ + return pool->minWorkers; +} + +size_t virThreadPoolGetMaxWorkers(virThreadPoolPtr pool) +{ + return pool->maxWorkers; +} + +size_t virThreadPoolGetPriorityWorkers(virThreadPoolPtr pool) +{ + return pool->nPrioWorkers; +} + /* * @priority - job priority * Return: 0 on success, -1 otherwise diff --git a/src/util/threadpool.h b/src/util/threadpool.h index 894b278..798fd0b 100644 --- a/src/util/threadpool.h +++ b/src/util/threadpool.h @@ -39,6 +39,10 @@ virThreadPoolPtr virThreadPoolNew(size_t minWorkers, virThreadPoolJobFunc func, void *opaque) ATTRIBUTE_NONNULL(4); +size_t virThreadPoolGetMinWorkers(virThreadPoolPtr pool); +size_t virThreadPoolGetMaxWorkers(virThreadPoolPtr pool); +size_t virThreadPoolGetPriorityWorkers(virThreadPoolPtr pool); + void virThreadPoolFree(virThreadPoolPtr pool); int virThreadPoolSendJob(virThreadPoolPtr pool, -- 1.7.11.2

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Currently the virNetServerServicePtr is responsible for creating the virNetServerClientPtr instance when accepting a new connection. Change this so that the virNetServerServicePtr merely gives virNetServerPtr a virNetSocketPtr instance. The virNetServerPtr can then create the virNetServerClientPtr as it desires Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/libvirt_private.syms | 4 ++-- src/rpc/virnetserver.c | 14 ++++++++++++-- src/rpc/virnetserverclient.c | 4 +--- src/rpc/virnetserverservice.c | 38 +++++++++++++++----------------------- src/rpc/virnetserverservice.h | 4 +++- 5 files changed, 33 insertions(+), 31 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3a23fe6..e3d31dc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1487,8 +1487,6 @@ virNetServerKeepAliveRequired; virNetServerNew; virNetServerQuit; virNetServerRun; -virNetServerServiceNewTCP; -virNetServerServiceNewUNIX; virNetServerSetTLSContext; virNetServerUpdateServices; @@ -1553,7 +1551,9 @@ virNetServerProgramUnknownError; # virnetserverservice.h virNetServerServiceClose; virNetServerServiceGetAuth; +virNetServerServiceGetMaxRequests; virNetServerServiceGetPort; +virNetServerServiceGetTLSContext; virNetServerServiceIsReadonly; virNetServerServiceNewTCP; virNetServerServiceNewUNIX; diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 15abb56..c3eb2e5 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -261,11 +261,12 @@ cleanup: } -static int virNetServerDispatchNewClient(virNetServerServicePtr svc ATTRIBUTE_UNUSED, - virNetServerClientPtr client, +static int virNetServerDispatchNewClient(virNetServerServicePtr svc, + virNetSocketPtr clientsock, void *opaque) { virNetServerPtr srv = opaque; + virNetServerClientPtr client = NULL; virNetServerLock(srv); @@ -276,6 +277,13 @@ static int virNetServerDispatchNewClient(virNetServerServicePtr svc ATTRIBUTE_UN goto error; } + if (!(client = virNetServerClientNew(clientsock, + virNetServerServiceGetAuth(svc), + virNetServerServiceIsReadonly(svc), + virNetServerServiceGetMaxRequests(svc), + virNetServerServiceGetTLSContext(svc)))) + goto error; + if (virNetServerClientInit(client) < 0) goto error; @@ -301,6 +309,8 @@ static int virNetServerDispatchNewClient(virNetServerServicePtr svc ATTRIBUTE_UN return 0; error: + virNetServerClientClose(client); + virObjectUnref(client); virNetServerUnlock(srv); return -1; } diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index d135b0f..9f033c8 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -357,7 +357,7 @@ virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock, return NULL; } - client->sock = sock; + client->sock = virObjectRef(sock); client->auth = auth; client->readonly = readonly; client->tlsCtxt = virObjectRef(tls); @@ -385,8 +385,6 @@ virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock, return client; error: - /* XXX ref counting is better than this */ - client->sock = NULL; /* Caller owns 'sock' upon failure */ virObjectUnref(client); return NULL; } diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 000b5b6..eda5ef9 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -69,40 +69,21 @@ static void virNetServerServiceAccept(virNetSocketPtr sock, void *opaque) { virNetServerServicePtr svc = opaque; - virNetServerClientPtr client = NULL; virNetSocketPtr clientsock = NULL; if (virNetSocketAccept(sock, &clientsock) < 0) - goto error; + goto cleanup; if (!clientsock) /* Connection already went away */ goto cleanup; - if (!(client = virNetServerClientNew(clientsock, - svc->auth, - svc->readonly, - svc->nrequests_client_max, - svc->tls))) - goto error; - if (!svc->dispatchFunc) - goto error; - - if (svc->dispatchFunc(svc, client, svc->dispatchOpaque) < 0) - virNetServerClientClose(client); + goto cleanup; - virObjectUnref(client); + svc->dispatchFunc(svc, clientsock, svc->dispatchOpaque); cleanup: - return; - -error: - if (client) { - virNetServerClientClose(client); - virObjectUnref(client); - } else { - virObjectUnref(clientsock); - } + virObjectUnref(clientsock); } @@ -240,6 +221,17 @@ bool virNetServerServiceIsReadonly(virNetServerServicePtr svc) } +size_t virNetServerServiceGetMaxRequests(virNetServerServicePtr svc) +{ + return svc->nrequests_client_max; +} + +virNetTLSContextPtr virNetServerServiceGetTLSContext(virNetServerServicePtr svc) +{ + return svc->tls; +} + + void virNetServerServiceSetDispatcher(virNetServerServicePtr svc, virNetServerServiceDispatchFunc func, void *opaque) diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h index 98fd396..cb18e2d 100644 --- a/src/rpc/virnetserverservice.h +++ b/src/rpc/virnetserverservice.h @@ -34,7 +34,7 @@ enum { }; typedef int (*virNetServerServiceDispatchFunc)(virNetServerServicePtr svc, - virNetServerClientPtr client, + virNetSocketPtr sock, void *opaque); virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename, @@ -55,6 +55,8 @@ int virNetServerServiceGetPort(virNetServerServicePtr svc); int virNetServerServiceGetAuth(virNetServerServicePtr svc); bool virNetServerServiceIsReadonly(virNetServerServicePtr svc); +size_t virNetServerServiceGetMaxRequests(virNetServerServicePtr svc); +virNetTLSContextPtr virNetServerServiceGetTLSContext(virNetServerServicePtr svc); void virNetServerServiceSetDispatcher(virNetServerServicePtr svc, virNetServerServiceDispatchFunc func, -- 1.7.11.2

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Currently the virNetClientPtr constructor will always register the async IO event handler and the keepalive objects. In the case of the lock manager, there will be no event loop available not keepalive support required. Split this setup out of the constructor and into separate methods. The remote driver will enable async IO and keepalives, while the LXC driver will only even async IO Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/libvirt_private.syms | 2 ++ src/lxc/lxc_monitor.c | 2 ++ src/remote/remote_driver.c | 9 ++++++ src/rpc/virnetclient.c | 81 ++++++++++++++++++++++++++++++---------------- src/rpc/virnetclient.h | 3 ++ 5 files changed, 69 insertions(+), 28 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e3d31dc..78d944f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1320,6 +1320,8 @@ virNetClientNewExternal; virNetClientNewSSH; virNetClientNewTCP; virNetClientNewUNIX; +virNetClientRegisterASyncIO; +virNetClientRegisterKeepAlive; virNetClientRemoteAddrString; virNetClientRemoveStream; virNetClientSendNoReply; diff --git a/src/lxc/lxc_monitor.c b/src/lxc/lxc_monitor.c index a81d0f7..6378ca0 100644 --- a/src/lxc/lxc_monitor.c +++ b/src/lxc/lxc_monitor.c @@ -129,6 +129,8 @@ virLXCMonitorPtr virLXCMonitorNew(virDomainObjPtr vm, if (!(mon->client = virNetClientNewUNIX(sockpath, false, NULL))) goto error; + if (virNetClientRegisterASyncIO(mon->client) < 0) + goto error; if (!(mon->program = virNetClientProgramNew(VIR_LXC_PROTOCOL_PROGRAM, VIR_LXC_PROTOCOL_PROGRAM_VERSION, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8153d70..bc6d746 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -686,6 +686,15 @@ doRemoteOpen(virConnectPtr conn, } /* switch (transport) */ + if (virNetClientRegisterASyncIO(priv->client) < 0) { + VIR_DEBUG("Failed to add event watch, disabling events and support for" + " keepalive messages"); + virResetLastError(); + } else { + if (virNetClientRegisterKeepAlive(priv->client) < 0) + goto failed; + } + virNetClientSetCloseCallback(priv->client, remoteClientCloseFunc, conn, NULL); diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 42bdc54..f711751 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -68,6 +68,7 @@ struct _virNetClient { virMutex lock; virNetSocketPtr sock; + bool asyncIO; virNetTLSSessionPtr tls; char *hostname; @@ -309,7 +310,6 @@ static virNetClientPtr virNetClientNew(virNetSocketPtr sock, { virNetClientPtr client = NULL; int wakeupFD[2] = { -1, -1 }; - virKeepAlivePtr ka = NULL; if (virNetClientInitialize() < 0) return NULL; @@ -337,29 +337,6 @@ static virNetClientPtr virNetClientNew(virNetSocketPtr sock, !(client->hostname = strdup(hostname))) goto no_memory; - /* Set up a callback to listen on the socket data */ - virObjectRef(client); - if (virNetSocketAddIOCallback(client->sock, - VIR_EVENT_HANDLE_READABLE, - virNetClientIncomingEvent, - client, - virObjectFreeCallback) < 0) { - virObjectUnref(client); - VIR_DEBUG("Failed to add event watch, disabling events and support for" - " keepalive messages"); - } else { - /* Keepalive protocol consists of async messages so it can only be used - * if the client supports them */ - if (!(ka = virKeepAliveNew(-1, 0, client, - virNetClientKeepAliveSendCB, - virNetClientKeepAliveDeadCB, - virObjectFreeCallback))) - goto error; - /* keepalive object has a reference to client */ - virObjectRef(client); - } - - client->keepalive = ka; PROBE(RPC_CLIENT_NEW, "client=%p sock=%p", client, client->sock); @@ -370,10 +347,6 @@ no_memory: error: VIR_FORCE_CLOSE(wakeupFD[0]); VIR_FORCE_CLOSE(wakeupFD[1]); - if (ka) { - virKeepAliveStop(ka); - virObjectUnref(ka); - } virObjectUnref(client); return NULL; } @@ -433,6 +406,58 @@ virNetClientPtr virNetClientNewExternal(const char **cmdargv) } +int virNetClientRegisterASyncIO(virNetClientPtr client) +{ + if (client->asyncIO) + return 0; + + /* Set up a callback to listen on the socket data */ + virObjectRef(client); + if (virNetSocketAddIOCallback(client->sock, + VIR_EVENT_HANDLE_READABLE, + virNetClientIncomingEvent, + client, + virObjectFreeCallback) < 0) { + virObjectUnref(client); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to register async IO callback")); + return -1; + } + + client->asyncIO = true; + return 0; +} + + +int virNetClientRegisterKeepAlive(virNetClientPtr client) +{ + virKeepAlivePtr ka; + + if (client->keepalive) + return 0; + + if (!client->asyncIO) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Unable to enable keepalives without async IO support")); + return -1; + } + + /* Keepalive protocol consists of async messages so it can only be used + * if the client supports them */ + if (!(ka = virKeepAliveNew(-1, 0, client, + virNetClientKeepAliveSendCB, + virNetClientKeepAliveDeadCB, + virObjectFreeCallback))) + return -1; + + /* keepalive object has a reference to client */ + virObjectRef(client); + + client->keepalive = ka; + return 0; +} + + int virNetClientGetFD(virNetClientPtr client) { int fd; diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h index d6b9b3c..70da794 100644 --- a/src/rpc/virnetclient.h +++ b/src/rpc/virnetclient.h @@ -52,6 +52,9 @@ virNetClientPtr virNetClientNewSSH(const char *nodename, virNetClientPtr virNetClientNewExternal(const char **cmdargv); +int virNetClientRegisterASyncIO(virNetClientPtr client); +int virNetClientRegisterKeepAlive(virNetClientPtr client); + typedef void (*virNetClientCloseFunc)(virNetClientPtr client, int reason, void *opaque); -- 1.7.11.2

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/locking/domain_lock.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c index ba49f9c..4ac16f2 100644 --- a/src/locking/domain_lock.c +++ b/src/locking/domain_lock.c @@ -160,6 +160,9 @@ int virDomainLockProcessStart(virLockManagerPluginPtr plugin, int ret; int flags = VIR_LOCK_MANAGER_ACQUIRE_RESTRICT; + VIR_DEBUG("plugin=%p dom=%p paused=%d fd=%p", + plugin, dom, paused, fd); + if (!(lock = virDomainLockManagerNew(plugin, dom, true))) return -1; @@ -180,6 +183,9 @@ int virDomainLockProcessPause(virLockManagerPluginPtr plugin, virLockManagerPtr lock; int ret; + VIR_DEBUG("plugin=%p dom=%p state=%p", + plugin, dom, state); + if (!(lock = virDomainLockManagerNew(plugin, dom, true))) return -1; @@ -196,6 +202,9 @@ int virDomainLockProcessResume(virLockManagerPluginPtr plugin, virLockManagerPtr lock; int ret; + VIR_DEBUG("plugin=%p dom=%p state=%s", + plugin, dom, NULLSTR(state)); + if (!(lock = virDomainLockManagerNew(plugin, dom, true))) return -1; @@ -212,6 +221,9 @@ int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, virLockManagerPtr lock; int ret; + VIR_DEBUG("plugin=%p dom=%p state=%p", + plugin, dom, state); + if (!(lock = virDomainLockManagerNew(plugin, dom, true))) return -1; @@ -229,6 +241,9 @@ int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, virLockManagerPtr lock; int ret = -1; + VIR_DEBUG("plugin=%p dom=%p disk=%p", + plugin, dom, disk); + if (!(lock = virDomainLockManagerNew(plugin, dom, false))) return -1; @@ -253,6 +268,9 @@ int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, virLockManagerPtr lock; int ret = -1; + VIR_DEBUG("plugin=%p dom=%p disk=%p", + plugin, dom, disk); + if (!(lock = virDomainLockManagerNew(plugin, dom, false))) return -1; @@ -278,6 +296,9 @@ int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, virLockManagerPtr lock; int ret = -1; + VIR_DEBUG("plugin=%p dom=%p lease=%p", + plugin, dom, lease); + if (!(lock = virDomainLockManagerNew(plugin, dom, false))) return -1; @@ -302,6 +323,9 @@ int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, virLockManagerPtr lock; int ret = -1; + VIR_DEBUG("plugin=%p dom=%p lease=%p", + plugin, dom, lease); + if (!(lock = virDomainLockManagerNew(plugin, dom, false))) return -1; -- 1.7.11.2

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Currently there is a hook function that is invoked when a new client connection comes in, which allows an app to setup private data. This setup will make it difficult to serialize client state during process re-exec(). Change to a model where the app registers a callback when creating the virNetServerPtr instance, which is used to allocate the client private data immediately during virNetClientPtr construction. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- daemon/libvirtd.c | 1 + daemon/remote.c | 15 ++++++--------- daemon/remote.h | 6 +++--- src/lxc/lxc_controller.c | 23 +++++++++++++++++------ src/rpc/virnetserver.c | 24 +++++++++++++----------- src/rpc/virnetserver.h | 9 +++------ src/rpc/virnetserverclient.c | 31 +++++++++++++------------------ src/rpc/virnetserverclient.h | 13 +++++++------ 8 files changed, 63 insertions(+), 59 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index fa9e7e8..24e20f8 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1196,6 +1196,7 @@ int main(int argc, char **argv) { !!config->keepalive_required, config->mdns_adv ? config->mdns_name : NULL, remoteClientInitHook, + remoteClientFreeFunc, NULL))) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; diff --git a/daemon/remote.c b/daemon/remote.c index 832307e..851bcc1 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -631,7 +631,7 @@ verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST); * We keep the libvirt connection open until any async * jobs have finished, then clean it up elsewhere */ -static void remoteClientFreeFunc(void *data) +void remoteClientFreeFunc(void *data) { struct daemonClientPrivate *priv = data; @@ -663,31 +663,28 @@ static void remoteClientCloseFunc(virNetServerClientPtr client) } -int remoteClientInitHook(virNetServerPtr srv ATTRIBUTE_UNUSED, - virNetServerClientPtr client, - void *opaque ATTRIBUTE_UNUSED) +void *remoteClientInitHook(virNetServerClientPtr client, + void *opaque ATTRIBUTE_UNUSED) { struct daemonClientPrivate *priv; int i; if (VIR_ALLOC(priv) < 0) { virReportOOMError(); - return -1; + return NULL; } if (virMutexInit(&priv->lock) < 0) { VIR_FREE(priv); virReportOOMError(); - return -1; + return NULL; } for (i = 0 ; i < VIR_DOMAIN_EVENT_ID_LAST ; i++) priv->domainEventCallbackID[i] = -1; - virNetServerClientSetPrivateData(client, priv, - remoteClientFreeFunc); virNetServerClientSetCloseHook(client, remoteClientCloseFunc); - return 0; + return priv; } /*----- Functions. -----*/ diff --git a/daemon/remote.h b/daemon/remote.h index 9f662cb..2806494 100644 --- a/daemon/remote.h +++ b/daemon/remote.h @@ -35,8 +35,8 @@ extern size_t remoteNProcs; extern virNetServerProgramProc qemuProcs[]; extern size_t qemuNProcs; -int remoteClientInitHook(virNetServerPtr srv, - virNetServerClientPtr client, - void *opaque); +void remoteClientFreeFunc(void *data); +void *remoteClientInitHook(virNetServerClientPtr client, + void *opaque); #endif /* __LIBVIRTD_REMOTE_H__ */ diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index cb9fa41..4c3c17f 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -578,16 +578,26 @@ static void virLXCControllerClientCloseHook(virNetServerClientPtr client) } } -static int virLXCControllerClientHook(virNetServerPtr server ATTRIBUTE_UNUSED, - virNetServerClientPtr client, - void *opaque) +static void virLXCControllerClientPrivateFree(void *data) +{ + VIR_FREE(data); +} + +static void *virLXCControllerClientPrivateNew(virNetServerClientPtr client, + void *opaque) { virLXCControllerPtr ctrl = opaque; - virNetServerClientSetPrivateData(client, ctrl, NULL); + int *dummy; + + if (VIR_ALLOC(dummy) < 0) { + virReportOOMError(); + return NULL; + } + virNetServerClientSetCloseHook(client, virLXCControllerClientCloseHook); VIR_DEBUG("Got new client %p", client); ctrl->client = client; - return 0; + return dummy; } @@ -605,7 +615,8 @@ static int virLXCControllerSetupServer(virLXCControllerPtr ctrl) if (!(ctrl->server = virNetServerNew(0, 0, 0, 1, -1, 0, false, NULL, - virLXCControllerClientHook, + virLXCControllerClientPrivateNew, + virLXCControllerClientPrivateFree, ctrl))) goto error; diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index c3eb2e5..03cf0b7 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -104,8 +104,9 @@ struct _virNetServer { virNetServerAutoShutdownFunc autoShutdownFunc; void *autoShutdownOpaque; - virNetServerClientInitHook clientInitHook; - void *clientInitOpaque; + virNetServerClientPrivNew clientPrivNew; + virFreeCallback clientPrivFree; + void *clientPrivOpaque; }; @@ -281,16 +282,15 @@ static int virNetServerDispatchNewClient(virNetServerServicePtr svc, virNetServerServiceGetAuth(svc), virNetServerServiceIsReadonly(svc), virNetServerServiceGetMaxRequests(svc), - virNetServerServiceGetTLSContext(svc)))) + virNetServerServiceGetTLSContext(svc), + srv->clientPrivNew, + srv->clientPrivFree, + srv->clientPrivOpaque))) goto error; if (virNetServerClientInit(client) < 0) goto error; - if (srv->clientInitHook && - srv->clientInitHook(srv, client, srv->clientInitOpaque) < 0) - goto error; - if (VIR_EXPAND_N(srv->clients, srv->nclients, 1) < 0) { virReportOOMError(); goto error; @@ -352,8 +352,9 @@ virNetServerPtr virNetServerNew(size_t min_workers, unsigned int keepaliveCount, bool keepaliveRequired, const char *mdnsGroupName, - virNetServerClientInitHook clientInitHook, - void *opaque) + virNetServerClientPrivNew clientPrivNew, + virFreeCallback clientPrivFree, + void *clientPrivOpaque) { virNetServerPtr srv; struct sigaction sig_action; @@ -376,8 +377,9 @@ virNetServerPtr virNetServerNew(size_t min_workers, srv->keepaliveCount = keepaliveCount; srv->keepaliveRequired = keepaliveRequired; srv->sigwrite = srv->sigread = -1; - srv->clientInitHook = clientInitHook; - srv->clientInitOpaque = opaque; + srv->clientPrivNew = clientPrivNew; + srv->clientPrivFree = clientPrivFree; + srv->clientPrivOpaque = clientPrivOpaque; srv->privileged = geteuid() == 0 ? true : false; if (mdnsGroupName && diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h index 7dc52ca..9188072 100644 --- a/src/rpc/virnetserver.h +++ b/src/rpc/virnetserver.h @@ -32,10 +32,6 @@ # include "virnetserverservice.h" # include "virobject.h" -typedef int (*virNetServerClientInitHook)(virNetServerPtr srv, - virNetServerClientPtr client, - void *opaque); - virNetServerPtr virNetServerNew(size_t min_workers, size_t max_workers, size_t priority_workers, @@ -44,8 +40,9 @@ virNetServerPtr virNetServerNew(size_t min_workers, unsigned int keepaliveCount, bool keepaliveRequired, const char *mdnsGroupName, - virNetServerClientInitHook clientInitHook, - void *opaque); + virNetServerClientPrivNew clientPrivNew, + virFreeCallback clientPrivFree, + void *clientPrivOpaque); typedef int (*virNetServerAutoShutdownFunc)(virNetServerPtr srv, void *opaque); diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 9f033c8..c9703fd 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -97,7 +97,7 @@ struct _virNetServerClient void *dispatchOpaque; void *privateData; - virNetServerClientFreeFunc privateDataFreeFunc; + virFreeCallback privateDataFreeFunc; virNetServerClientCloseFunc privateDataCloseFunc; virKeepAlivePtr keepalive; @@ -340,7 +340,10 @@ virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock, int auth, bool readonly, size_t nrequests_max, - virNetTLSContextPtr tls) + virNetTLSContextPtr tls, + virNetServerClientPrivNew privNew, + virFreeCallback privFree, + void *privOpaque) { virNetServerClientPtr client; @@ -378,6 +381,14 @@ virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock, } client->nrequests = 1; + if (privNew) { + if (!(client->privateData = privNew(client, privOpaque))) { + virObjectUnref(client); + goto error; + } + client->privateDataFreeFunc = privFree; + } + PROBE(RPC_SERVER_CLIENT_NEW, "client=%p sock=%p", client, client->sock); @@ -507,22 +518,6 @@ const char *virNetServerClientGetIdentity(virNetServerClientPtr client) return identity; } -void virNetServerClientSetPrivateData(virNetServerClientPtr client, - void *opaque, - virNetServerClientFreeFunc ff) -{ - virNetServerClientLock(client); - - if (client->privateData && - client->privateDataFreeFunc) - client->privateDataFreeFunc(client->privateData); - - client->privateData = opaque; - client->privateDataFreeFunc = ff; - - virNetServerClientUnlock(client); -} - void *virNetServerClientGetPrivateData(virNetServerClientPtr client) { diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h index a1ff19b..f950c61 100644 --- a/src/rpc/virnetserverclient.h +++ b/src/rpc/virnetserverclient.h @@ -39,11 +39,17 @@ typedef int (*virNetServerClientFilterFunc)(virNetServerClientPtr client, virNetMessagePtr msg, void *opaque); +typedef void *(*virNetServerClientPrivNew)(virNetServerClientPtr client, + void *opaque); + virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock, int auth, bool readonly, size_t nrequests_max, - virNetTLSContextPtr tls); + virNetTLSContextPtr tls, + virNetServerClientPrivNew privNew, + virFreeCallback privFree, + void *privOpaque); int virNetServerClientAddFilter(virNetServerClientPtr client, virNetServerClientFilterFunc func, @@ -74,11 +80,6 @@ const char *virNetServerClientGetIdentity(virNetServerClientPtr client); int virNetServerClientGetUNIXIdentity(virNetServerClientPtr client, uid_t *uid, gid_t *gid, pid_t *pid); -typedef void (*virNetServerClientFreeFunc)(void *data); - -void virNetServerClientSetPrivateData(virNetServerClientPtr client, - void *opaque, - virNetServerClientFreeFunc ff); void *virNetServerClientGetPrivateData(virNetServerClientPtr client); typedef void (*virNetServerClientCloseFunc)(virNetServerClientPtr client); -- 1.7.11.2

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Currently the virNetServerDispatchNewClient both creates the virNetServerClientPtr instance and registers it with the virNetServerPtr internal state. Split the client registration code out into a separate virNetServerAddClient method to allow future reuse from other contexts Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/rpc/virnetserver.c | 46 ++++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 03cf0b7..0a6ecdc 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -262,13 +262,9 @@ cleanup: } -static int virNetServerDispatchNewClient(virNetServerServicePtr svc, - virNetSocketPtr clientsock, - void *opaque) +static int virNetServerAddClient(virNetServerPtr srv, + virNetServerClientPtr client) { - virNetServerPtr srv = opaque; - virNetServerClientPtr client = NULL; - virNetServerLock(srv); if (srv->nclients >= srv->nclients_max) { @@ -278,16 +274,6 @@ static int virNetServerDispatchNewClient(virNetServerServicePtr svc, goto error; } - if (!(client = virNetServerClientNew(clientsock, - virNetServerServiceGetAuth(svc), - virNetServerServiceIsReadonly(svc), - virNetServerServiceGetMaxRequests(svc), - virNetServerServiceGetTLSContext(svc), - srv->clientPrivNew, - srv->clientPrivFree, - srv->clientPrivOpaque))) - goto error; - if (virNetServerClientInit(client) < 0) goto error; @@ -309,12 +295,36 @@ static int virNetServerDispatchNewClient(virNetServerServicePtr svc, return 0; error: - virNetServerClientClose(client); - virObjectUnref(client); virNetServerUnlock(srv); return -1; } +static int virNetServerDispatchNewClient(virNetServerServicePtr svc, + virNetSocketPtr clientsock, + void *opaque) +{ + virNetServerPtr srv = opaque; + virNetServerClientPtr client; + + if (!(client = virNetServerClientNew(clientsock, + virNetServerServiceGetAuth(svc), + virNetServerServiceIsReadonly(svc), + virNetServerServiceGetMaxRequests(svc), + virNetServerServiceGetTLSContext(svc), + srv->clientPrivNew, + srv->clientPrivFree, + srv->clientPrivOpaque))) + return -1; + + if (virNetServerAddClient(srv, client) < 0) { + virNetServerClientClose(client); + virObjectUnref(client); + return -1; + } + virObjectUnref(client); + return 0; +} + static void virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED, -- 1.7.11.2

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; In preparation for adding further constructors, refactor the virNetServerClientNew method to move most of the code into a common virNetServerClientNewInternal helper API. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/rpc/virnetserverclient.c | 52 +++++++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index c9703fd..acd2b4d 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -336,19 +336,15 @@ static void virNetServerClientSockTimerFunc(int timer, } -virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock, - int auth, - bool readonly, - size_t nrequests_max, - virNetTLSContextPtr tls, - virNetServerClientPrivNew privNew, - virFreeCallback privFree, - void *privOpaque) +static virNetServerClientPtr +virNetServerClientNewInternal(virNetSocketPtr sock, + int auth, + bool readonly, + size_t nrequests_max, + virNetTLSContextPtr tls) { virNetServerClientPtr client; - VIR_DEBUG("sock=%p auth=%d tls=%p", sock, auth, tls); - if (virNetServerClientInitialize() < 0) return NULL; @@ -381,14 +377,6 @@ virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock, } client->nrequests = 1; - if (privNew) { - if (!(client->privateData = privNew(client, privOpaque))) { - virObjectUnref(client); - goto error; - } - client->privateDataFreeFunc = privFree; - } - PROBE(RPC_SERVER_CLIENT_NEW, "client=%p sock=%p", client, client->sock); @@ -401,6 +389,34 @@ error: } +virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock, + int auth, + bool readonly, + size_t nrequests_max, + virNetTLSContextPtr tls, + virNetServerClientPrivNew privNew, + virFreeCallback privFree, + void *privOpaque) +{ + virNetServerClientPtr client; + + VIR_DEBUG("sock=%p auth=%d tls=%p", sock, auth, tls); + + if (!(client = virNetServerClientNewInternal(sock, auth, readonly, nrequests_max, tls))) + return NULL; + + if (privNew) { + if (!(client->privateData = privNew(client, privOpaque))) { + virObjectUnref(client); + return NULL; + } + client->privateDataFreeFunc = privFree; + } + + return client; +} + + int virNetServerClientGetAuth(virNetServerClientPtr client) { int auth; -- 1.7.11.2

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; In order to support systemd socket based activation, it needs to be possible to create virNetSocketPtr and virNetServerServicePtr instance from a pre-opened file descriptor --- src/libvirt_private.syms | 2 ++ src/rpc/virnetserverservice.c | 49 +++++++++++++++++++++++++++++++++++++++++++ src/rpc/virnetserverservice.h | 5 +++++ src/rpc/virnetsocket.c | 20 ++++++++++++++++++ src/rpc/virnetsocket.h | 3 +++ 5 files changed, 79 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 78d944f..a0db0e5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1557,6 +1557,7 @@ virNetServerServiceGetMaxRequests; virNetServerServiceGetPort; virNetServerServiceGetTLSContext; virNetServerServiceIsReadonly; +virNetServerServiceNewFD; virNetServerServiceNewTCP; virNetServerServiceNewUNIX; virNetServerServiceSetDispatcher; @@ -1582,6 +1583,7 @@ virNetSocketNewConnectExternal; virNetSocketNewConnectSSH; virNetSocketNewConnectTCP; virNetSocketNewConnectUNIX; +virNetSocketNewListenFD; virNetSocketNewListenTCP; virNetSocketNewListenUNIX; virNetSocketRead; diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index eda5ef9..53ff503 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -200,6 +200,55 @@ error: return NULL; } +virNetServerServicePtr virNetServerServiceNewFD(int fd, + int auth, + bool readonly, + size_t nrequests_client_max, + virNetTLSContextPtr tls) +{ + virNetServerServicePtr svc; + int i; + + if (virNetServerServiceInitialize() < 0) + return NULL; + + if (!(svc = virObjectNew(virNetServerServiceClass))) + return NULL; + + svc->auth = auth; + svc->readonly = readonly; + svc->nrequests_client_max = nrequests_client_max; + svc->tls = virObjectRef(tls); + + svc->nsocks = 1; + if (VIR_ALLOC_N(svc->socks, svc->nsocks) < 0) + goto no_memory; + + if (virNetSocketNewListenFD(fd, + &svc->socks[0]) < 0) + goto error; + + for (i = 0 ; i < svc->nsocks ; i++) { + /* IO callback is initially disabled, until we're ready + * to deal with incoming clients */ + if (virNetSocketAddIOCallback(svc->socks[i], + 0, + virNetServerServiceAccept, + svc, + virObjectFreeCallback) < 0) + goto error; + } + + + return svc; + +no_memory: + virReportOOMError(); +error: + virObjectUnref(svc); + return NULL; +} + int virNetServerServiceGetPort(virNetServerServicePtr svc) { diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h index cb18e2d..48f49e7 100644 --- a/src/rpc/virnetserverservice.h +++ b/src/rpc/virnetserverservice.h @@ -50,6 +50,11 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path, bool readonly, size_t nrequests_client_max, virNetTLSContextPtr tls); +virNetServerServicePtr virNetServerServiceNewFD(int fd, + int auth, + bool readonly, + size_t nrequests_client_max, + virNetTLSContextPtr tls); int virNetServerServiceGetPort(virNetServerServicePtr svc); diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index b6f156b..69c25bd 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -399,6 +399,26 @@ int virNetSocketNewListenUNIX(const char *path ATTRIBUTE_UNUSED, } #endif +int virNetSocketNewListenFD(int fd, + virNetSocketPtr *retsock) +{ + virSocketAddr addr; + *retsock = NULL; + + memset(&addr, 0, sizeof(addr)); + + addr.len = sizeof(addr.data); + if (getsockname(fd, &addr.data.sa, &addr.len) < 0) { + virReportSystemError(errno, "%s", _("Unable to get local socket name")); + return -1; + } + + if (!(*retsock = virNetSocketNew(&addr, NULL, false, fd, -1, 0))) + return -1; + + return 0; +} + int virNetSocketNewConnectTCP(const char *nodename, const char *service, diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index cc3f912..3750955 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -52,6 +52,9 @@ int virNetSocketNewListenUNIX(const char *path, gid_t grp, virNetSocketPtr *addr); +int virNetSocketNewListenFD(int fd, + virNetSocketPtr *addr); + int virNetSocketNewConnectTCP(const char *nodename, const char *service, virNetSocketPtr *addr); -- 1.7.11.2

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; The previously introduced virFile{Lock,Unlock} APIs provide a way to acquire/release fcntl() locks on individual files. For unknown reason though, the POSIX spec says that fcntl() locks are released when *any* file handle refering to the same path is closed. In the following sequence threadA: fd1 = open("foo") threadB: fd2 = open("foo") threadA: virFileLock(fd1) threadB: virFileLock(fd2) threadB: close(fd2) you'd expect threadA to come out holding a lock on 'foo', and indeed it does hold a lock for a very short time. Unfortunately when threadB does close(fd2) this releases the lock associated with fd1. For the current libvirt use case for virFileLock - pidfiles - this doesn't matter since the lock is acquired at startup while single threaded an never released until exit. To provide a more generally useful API though, it is neccessary to introduce a slightly higher level abstraction, which is to be referred to as a "lockspace". This is to be provided by a virLockSpacePtr object in src/util/virlockspace.{c,h}. The core idea is that the lockspace keeps track of what files are already open+locked. This means that when a 2nd thread comes along and tries to acquire a lock, it doesn't end up opening and closing a new FD. The lockspace just checks the current list of held locks and immediately returns VIR_ERR_RESOURCE_BUSY. NB, the API as it stands is designed on the basis that the files being locked are not being otherwise opened and used by the application code. One approach to using this API is to acquire locks based on a hash of the filepath. eg to lock /var/lib/libvirt/images/foo.img the application might do virLockSpacePtr lockspace = virLockSpaceNew("/var/lib/libvirt/imagelocks"); lockname = md5sum("/var/lib/libvirt/images/foo.img") virLockSpaceAcquireLock(lockspace, lockname) It is also possible to do locks directly on resources by using a NULL lockspace directory and then using the file path as the lock name eg virLockSpacePtr lockspace = virLockSpaceNew(NULL) virLockSpaceAcquireLock(lockspace, "/var/lib/libvirt/images/foo.img") This is only safe todo though if no other part of the proces will be opening the files. This will be the case when this code is used inside the soon-to-be-reposted virlockd daemon Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- .gitignore | 1 + include/libvirt/virterror.h | 2 + po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 11 + src/util/virlockspace.c | 548 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virlockspace.h | 58 +++++ src/util/virterror.c | 9 +- tests/Makefile.am | 7 +- tests/virlockspacetest.c | 363 +++++++++++++++++++++++++++++ 10 files changed, 999 insertions(+), 2 deletions(-) create mode 100644 src/util/virlockspace.c create mode 100644 src/util/virlockspace.h create mode 100644 tests/virlockspacetest.c diff --git a/.gitignore b/.gitignore index cd8a85c..37767e0 100644 --- a/.gitignore +++ b/.gitignore @@ -158,6 +158,7 @@ /tests/virdrivermoduletest /tests/virhashtest /tests/virkeyfiletest +/tests/virlockspacetest /tests/virnet*test /tests/virshtest /tests/virtimetest diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index ad8e101..3e72cb7 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -110,6 +110,7 @@ typedef enum { VIR_FROM_AUTH = 46, /* Error from auth handling */ VIR_FROM_DBUS = 47, /* Error from DBus */ VIR_FROM_PARALLELS = 48, /* Error from Parallels */ + VIR_FROM_LOCKSPACE = 49, /* Error from lockspace */ # ifdef VIR_ENUM_SENTINELS VIR_ERR_DOMAIN_LAST @@ -277,6 +278,7 @@ typedef enum { VIR_ERR_MIGRATE_UNSAFE = 81, /* Migration is not safe */ VIR_ERR_OVERFLOW = 82, /* integer overflow */ VIR_ERR_BLOCK_COPY_ACTIVE = 83, /* action prevented by block copy job */ + VIR_ERR_RESOURCE_BUSY = 84, /* resource is already in use */ } virErrorNumber; /** diff --git a/po/POTFILES.in b/po/POTFILES.in index 37a00ee..183ab42 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -143,6 +143,7 @@ src/util/virdbus.c src/util/virfile.c src/util/virhash.c src/util/virkeyfile.c +src/util/virlockspace.c src/util/virnetdev.c src/util/virnetdevbridge.c src/util/virnetdevmacvlan.c diff --git a/src/Makefile.am b/src/Makefile.am index 6ed4a41..81ed279 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -96,6 +96,7 @@ UTIL_SOURCES = \ util/virkeycode.c util/virkeycode.h \ util/virkeyfile.c util/virkeyfile.h \ util/virkeymaps.h \ + util/virlockspace.c util/virlockspace.h \ util/virmacaddr.h util/virmacaddr.c \ util/virnetdev.h util/virnetdev.c \ util/virnetdevbandwidth.h util/virnetdevbandwidth.c \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a0db0e5..d05c246 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1288,6 +1288,17 @@ virKeyFileHasGroup; virKeyFileGetValueString; +# virlockspace.h +virLockSpaceAcquireResource; +virLockSpaceCreateResource; +virLockSpaceDeleteResource; +virLockSpaceFree; +virLockSpaceGetDirectory; +virLockSpaceNew; +virLockSpaceReleaseResource; +virLockSpaceReleaseResourcesForOwner; + + # virmacaddr.h virMacAddrCmp; virMacAddrCmpRaw; diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c new file mode 100644 index 0000000..47dde66 --- /dev/null +++ b/src/util/virlockspace.c @@ -0,0 +1,548 @@ +/* + * virlockspace.c: simple file based lockspaces + * + * Copyright (C) 2012 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 + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>;. + * + */ + +#include <config.h> + +#include "virlockspace.h" +#include "logging.h" +#include "memory.h" +#include "virterror_internal.h" +#include "util.h" +#include "virfile.h" +#include "virhash.h" +#include "threads.h" + +#include <fcntl.h> +#include <unistd.h> +#include <sys/stat.h> + +#define VIR_FROM_THIS VIR_FROM_LOCKSPACE + +typedef struct _virLockSpaceResource virLockSpaceResource; +typedef virLockSpaceResource *virLockSpaceResourcePtr; + +struct _virLockSpaceResource { + char *name; + char *path; + int fd; + bool lockHeld; + unsigned int flags; + size_t nOwners; + pid_t *owners; +}; + +struct _virLockSpace { + char *dir; + virMutex lock; + + virHashTablePtr resources; +}; + + +static char *virLockSpaceGetResourcePath(virLockSpacePtr lockspace, + const char *resname) +{ + char *ret; + if (lockspace->dir) { + if (virAsprintf(&ret, "%s/%s", lockspace->dir, resname) < 0) { + virReportOOMError(); + return NULL; + } + } else { + if (!(ret = strdup(resname))) { + virReportOOMError(); + return NULL; + } + } + + return ret; +} + + +static void virLockSpaceResourceFree(virLockSpaceResourcePtr res) +{ + if (!res) + return; + + if (res->lockHeld && + (res->flags & VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE)) { + if (res->flags & VIR_LOCK_SPACE_ACQUIRE_SHARED) { + /* We must upgrade to an exclusive lock to ensure + * no one else still has it before trying to delete */ + if (virFileLock(res->fd, false, 0, 1) < 0) { + VIR_DEBUG("Could not upgrade shared lease to exclusive, not deleting"); + } else { + unlink(res->path); + } + } else { + unlink(res->path); + } + } + + VIR_FORCE_CLOSE(res->fd); + VIR_FREE(res->path); + VIR_FREE(res->name); + VIR_FREE(res); +} + + +static virLockSpaceResourcePtr +virLockSpaceResourceNew(virLockSpacePtr lockspace, + const char *resname, + unsigned int flags, + pid_t owner) +{ + virLockSpaceResourcePtr res; + bool shared = !!(flags & VIR_LOCK_SPACE_ACQUIRE_SHARED); + + if (VIR_ALLOC(res) < 0) + return NULL; + + res->fd = -1; + res->flags = flags; + + if (!(res->name = strdup(resname))) + goto no_memory; + + if (!(res->path = virLockSpaceGetResourcePath(lockspace, resname))) + goto no_memory; + + if (flags & VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) { + while (1) { + struct stat a, b; + if ((res->fd = open(res->path, O_RDWR|O_CREAT, 0600)) < 0) { + virReportSystemError(errno, + _("Unable to open/create resource %s"), + res->path); + goto error; + } + + if (virSetCloseExec(res->fd) < 0) { + virReportSystemError(errno, + _("Failed to set close-on-exec flag '%s'"), + res->path); + goto error; + } + + if (fstat(res->fd, &b) < 0) { + virReportSystemError(errno, + _("Unable to check status of pid file '%s'"), + res->path); + goto error; + } + + if (virFileLock(res->fd, shared, 0, 1) < 0) { + if (errno == EACCES || errno == EAGAIN) { + virReportError(VIR_ERR_RESOURCE_BUSY, + _("Lockspace resource '%s' is locked"), + resname); + } else { + virReportSystemError(errno, + _("Unable to acquire lock on '%s'"), + res->path); + } + goto error; + } + + /* Now make sure the pidfile we locked is the same + * one that now exists on the filesystem + */ + if (stat(res->path, &a) < 0) { + char ebuf[1024] ATTRIBUTE_UNUSED; + VIR_DEBUG("Resource '%s' disappeared: %s", + res->path, virStrerror(errno, ebuf, sizeof(ebuf))); + VIR_FORCE_CLOSE(res->fd); + /* Someone else must be racing with us, so try agin */ + continue; + } + + if (a.st_ino == b.st_ino) + break; + + VIR_DEBUG("Resource '%s' was recreated", res->path); + VIR_FORCE_CLOSE(res->fd); + /* Someone else must be racing with us, so try agin */ + } + } else { + if ((res->fd = open(res->path, O_RDWR)) < 0) { + virReportSystemError(errno, + _("Unable to open resource %s"), + res->path); + goto error; + } + + if (virSetCloseExec(res->fd) < 0) { + virReportSystemError(errno, + _("Failed to set close-on-exec flag '%s'"), + res->path); + goto error; + } + + if (virFileLock(res->fd, !!(flags & VIR_LOCK_SPACE_ACQUIRE_SHARED), 0, 0) < 0) { + if (errno == EACCES || errno == EAGAIN) { + virReportError(VIR_ERR_RESOURCE_BUSY, + _("Lockspace resource '%s' is locked"), + resname); + } else { + virReportSystemError(errno, + _("Unable to acquire lock on '%s'"), + res->path); + } + goto error; + } + } + res->lockHeld = true; + + if (VIR_EXPAND_N(res->owners, res->nOwners, 1) < 0) + goto no_memory; + + res->owners[res->nOwners-1] = owner; + + return res; + +no_memory: + virReportOOMError(); +error: + virLockSpaceResourceFree(res); + return NULL; +} + + +static void virLockSpaceResourceDataFree(void *opaque, const void *name ATTRIBUTE_UNUSED) +{ + virLockSpaceResourcePtr res = opaque; + virLockSpaceResourceFree(res); +} + + +virLockSpacePtr virLockSpaceNew(const char *directory) +{ + virLockSpacePtr lockspace; + + VIR_DEBUG("directory=%s", NULLSTR(directory)); + + if (VIR_ALLOC(lockspace) < 0) + return NULL; + + if (virMutexInit(&lockspace->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to initialize lockspace mutex")); + VIR_FREE(lockspace); + return NULL; + } + + if (directory && + !(lockspace->dir = strdup(directory))) + goto no_memory; + + if (!(lockspace->resources = virHashCreate(10, + virLockSpaceResourceDataFree))) + goto error; + + if (directory) { + if (virFileExists(directory)) { + if (!virFileIsDir(directory)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Lockspace location %s exists, but is not a directory"), + directory); + goto error; + } + } else { + if (virFileMakePathWithMode(directory, 0700) < 0) { + virReportSystemError(errno, + _("Unable to create lockspace %s"), + directory); + goto error; + } + } + } + + return lockspace; + +no_memory: + virReportOOMError(); +error: + virLockSpaceFree(lockspace); + return NULL; +} + + +void virLockSpaceFree(virLockSpacePtr lockspace) +{ + if (!lockspace) + return; + + virHashFree(lockspace->resources); + VIR_FREE(lockspace->dir); + virMutexDestroy(&lockspace->lock); + VIR_FREE(lockspace); +} + + +const char *virLockSpaceGetDirectory(virLockSpacePtr lockspace) +{ + return lockspace->dir; +} + + +int virLockSpaceCreateResource(virLockSpacePtr lockspace, + const char *resname) +{ + int ret = -1; + char *respath = NULL; + virLockSpaceResourcePtr res; + + VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname); + + virMutexLock(&lockspace->lock); + + if ((res = virHashLookup(lockspace->resources, resname))) { + virReportError(VIR_ERR_RESOURCE_BUSY, + _("Lockspace resource '%s' is locked"), + resname); + goto cleanup; + } + + if (!(respath = virLockSpaceGetResourcePath(lockspace, resname))) + goto cleanup; + + if (virFileTouch(respath, 0600) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virMutexUnlock(&lockspace->lock); + VIR_FREE(respath); + return ret; +} + + +int virLockSpaceDeleteResource(virLockSpacePtr lockspace, + const char *resname) +{ + int ret = -1; + char *respath = NULL; + virLockSpaceResourcePtr res; + + VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname); + + virMutexLock(&lockspace->lock); + + if ((res = virHashLookup(lockspace->resources, resname))) { + virReportError(VIR_ERR_RESOURCE_BUSY, + _("Lockspace resource '%s' is locked"), + resname); + goto cleanup; + } + + if (!(respath = virLockSpaceGetResourcePath(lockspace, resname))) + goto cleanup; + + if (unlink(respath) < 0 && + errno != ENOENT) { + virReportSystemError(errno, + _("Unable to delete lockspace resource %s"), + respath); + goto cleanup; + } + + ret = 0; + +cleanup: + virMutexUnlock(&lockspace->lock); + VIR_FREE(respath); + return ret; +} + + +int virLockSpaceAcquireResource(virLockSpacePtr lockspace, + const char *resname, + pid_t owner, + unsigned int flags) +{ + int ret = -1; + virLockSpaceResourcePtr res; + + VIR_DEBUG("lockspace=%p resname=%s flags=%x owner=%lld", + lockspace, resname, flags, (unsigned long long)owner); + + virCheckFlags(VIR_LOCK_SPACE_ACQUIRE_SHARED | + VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE, -1); + + virMutexLock(&lockspace->lock); + + if ((res = virHashLookup(lockspace->resources, resname))) { + if ((res->flags & VIR_LOCK_SPACE_ACQUIRE_SHARED) && + (flags & VIR_LOCK_SPACE_ACQUIRE_SHARED)) { + + if (VIR_EXPAND_N(res->owners, res->nOwners, 1) < 0) { + virReportOOMError(); + goto cleanup; + } + res->owners[res->nOwners-1] = owner; + + goto done; + } + virReportError(VIR_ERR_RESOURCE_BUSY, + _("Lockspace resource '%s' is locked"), + resname); + goto cleanup; + } + + if (!(res = virLockSpaceResourceNew(lockspace, resname, flags, owner))) + goto cleanup; + + if (virHashAddEntry(lockspace->resources, resname, res) < 0) { + virLockSpaceResourceFree(res); + goto cleanup; + } + +done: + ret = 0; + +cleanup: + virMutexUnlock(&lockspace->lock); + return ret; +} + + +int virLockSpaceReleaseResource(virLockSpacePtr lockspace, + const char *resname, + pid_t owner) +{ + int ret = -1; + virLockSpaceResourcePtr res; + size_t i; + + VIR_DEBUG("lockspace=%p resname=%s owner=%lld", + lockspace, resname, (unsigned long long)owner); + + virMutexLock(&lockspace->lock); + + if (!(res = virHashLookup(lockspace->resources, resname))) { + virReportError(VIR_ERR_RESOURCE_BUSY, + _("Lockspace resource '%s' is not locked"), + resname); + goto cleanup; + } + + for (i = 0 ; i < res->nOwners ; i++) { + if (res->owners[i] == owner) { + break; + } + } + + if (i == res->nOwners) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("owner %lld does not hold the resource lock"), + (unsigned long long)owner); + goto cleanup; + } + + if (i < (res->nOwners - 1)) + memmove(res->owners + i, + res->owners + i + 1, + (res->nOwners - i - 1) * sizeof(res->owners[0])); + VIR_SHRINK_N(res->owners, res->nOwners, 1); + + if ((res->nOwners == 0) && + virHashRemoveEntry(lockspace->resources, resname) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virMutexUnlock(&lockspace->lock); + return ret; +} + + +struct virLockSpaceRemoveData { + pid_t owner; + size_t count; +}; + + +static int +virLockSpaceRemoveResourcesForOwner(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virLockSpaceResourcePtr res = (virLockSpaceResourcePtr)payload; + struct virLockSpaceRemoveData *data = (struct virLockSpaceRemoveData *)opaque; + size_t i; + + VIR_DEBUG("res %s owner %lld", res->name, (unsigned long long)data->owner); + + for (i = 0 ; i < res->nOwners ; i++) { + if (res->owners[i] == data->owner) { + break; + } + } + + if (i == res->nOwners) + return 0; + + data->count++; + + if (i < (res->nOwners - 1)) + memmove(res->owners + i, + res->owners + i + 1, + (res->nOwners - i - 1) * sizeof(res->owners[0])); + VIR_SHRINK_N(res->owners, res->nOwners, 1); + + if (res->nOwners) { + VIR_DEBUG("Other shared owners remain"); + return 0; + } + + VIR_DEBUG("No more owners, remove it"); + return 1; +} + + +int virLockSpaceReleaseResourcesForOwner(virLockSpacePtr lockspace, + pid_t owner) +{ + int ret = 0; + struct virLockSpaceRemoveData data = { + owner, 0 + }; + + VIR_DEBUG("lockspace=%p owner=%lld", lockspace, (unsigned long long)owner); + + virMutexLock(&lockspace->lock); + + if (virHashRemoveSet(lockspace->resources, + virLockSpaceRemoveResourcesForOwner, + &data) < 0) + goto error; + + ret = data.count; + + virMutexUnlock(&lockspace->lock); + return ret; + +error: + virMutexUnlock(&lockspace->lock); + return -1; +} diff --git a/src/util/virlockspace.h b/src/util/virlockspace.h new file mode 100644 index 0000000..bd8f91c --- /dev/null +++ b/src/util/virlockspace.h @@ -0,0 +1,58 @@ +/* + * virlockspace.h: simple file based lockspaces + * + * Copyright (C) 2012 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 + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>;. + * + */ + +#ifndef __VIR_LOCK_SPACE_H__ +# define __VIR_LOCK_SPACE_H__ + +# include "internal.h" + +typedef struct _virLockSpace virLockSpace; +typedef virLockSpace *virLockSpacePtr; + +virLockSpacePtr virLockSpaceNew(const char *directory); + +void virLockSpaceFree(virLockSpacePtr lockspace); + +const char *virLockSpaceGetDirectory(virLockSpacePtr lockspace); + +int virLockSpaceCreateResource(virLockSpacePtr lockspace, + const char *resname); +int virLockSpaceDeleteResource(virLockSpacePtr lockspace, + const char *resname); + +typedef enum { + VIR_LOCK_SPACE_ACQUIRE_SHARED = (1 << 0), + VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE = (1 << 1), +} virLockSpaceAcquireFlags; + +int virLockSpaceAcquireResource(virLockSpacePtr lockspace, + const char *resname, + pid_t owner, + unsigned int flags); + +int virLockSpaceReleaseResource(virLockSpacePtr lockspace, + const char *resname, + pid_t owner); + +int virLockSpaceReleaseResourcesForOwner(virLockSpacePtr lockspace, + pid_t owner); + +#endif /* __VIR_LOCK_SPACE_H__ */ diff --git a/src/util/virterror.c b/src/util/virterror.c index a40cfe0..74f511a 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -112,7 +112,8 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, "URI Utils", /* 45 */ "Authentication Utils", "DBus Utils", - "Parallels Cloud Server" + "Parallels Cloud Server", + "Lock Space", ) @@ -1185,6 +1186,12 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("block copy still active: %s"); break; + case VIR_ERR_RESOURCE_BUSY: + if (info == NULL) + errmsg = _("resource busy"); + else + errmsg = _("resource busy %s"); + break; } return errmsg; } diff --git a/tests/Makefile.am b/tests/Makefile.am index 60d322d..6ff51af 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -92,7 +92,7 @@ test_programs = virshtest sockettest \ viratomictest \ utiltest virnettlscontexttest shunloadtest \ virtimetest viruritest virkeyfiletest \ - virauthconfigtest + virauthconfigtest virlockspacetest if WITH_DRIVER_MODULES test_programs += virdrivermoduletest @@ -504,6 +504,11 @@ virtimetest_SOURCES = \ virtimetest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) virtimetest_LDADD = $(LDADDS) +virlockspacetest_SOURCES = \ + virlockspacetest.c testutils.h testutils.c +virlockspacetest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) +virlockspacetest_LDADD = $(LDADDS) + viruritest_SOURCES = \ viruritest.c testutils.h testutils.c viruritest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) diff --git a/tests/virlockspacetest.c b/tests/virlockspacetest.c new file mode 100644 index 0000000..ee58f95 --- /dev/null +++ b/tests/virlockspacetest.c @@ -0,0 +1,363 @@ +/* + * Copyright (C) 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 + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>;. + * + * Author: Daniel P. Berrange <berrange(a)redhat.com&gt; + */ + +#include <config.h> + +#include <stdlib.h> +#include <signal.h> +#include <sys/stat.h> + +#include "testutils.h" +#include "util.h" +#include "virterror_internal.h" +#include "memory.h" +#include "logging.h" + +#include "virlockspace.h" + +#define VIR_FROM_THIS VIR_FROM_RPC + +#define LOCKSPACE_DIR abs_builddir "/virlockspacedata" + +static int testLockSpaceCreate(const void *args ATTRIBUTE_UNUSED) +{ + virLockSpacePtr lockspace; + int ret = -1; + + rmdir(LOCKSPACE_DIR); + + lockspace = virLockSpaceNew(LOCKSPACE_DIR); + + if (!virFileIsDir(LOCKSPACE_DIR)) + goto cleanup; + + ret = 0; + +cleanup: + virLockSpaceFree(lockspace); + rmdir(LOCKSPACE_DIR); + return ret; +} + + +static int testLockSpaceResourceLifecycle(const void *args ATTRIBUTE_UNUSED) +{ + virLockSpacePtr lockspace; + int ret = -1; + + rmdir(LOCKSPACE_DIR); + + lockspace = virLockSpaceNew(LOCKSPACE_DIR); + + if (!virFileIsDir(LOCKSPACE_DIR)) + goto cleanup; + + if (virLockSpaceCreateResource(lockspace, "foo") < 0) + goto cleanup; + + if (!virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + if (virLockSpaceDeleteResource(lockspace, "foo") < 0) + goto cleanup; + + if (virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + ret = 0; + +cleanup: + virLockSpaceFree(lockspace); + rmdir(LOCKSPACE_DIR); + return ret; +} + + +static int testLockSpaceResourceLockExcl(const void *args ATTRIBUTE_UNUSED) +{ + virLockSpacePtr lockspace; + int ret = -1; + + rmdir(LOCKSPACE_DIR); + + lockspace = virLockSpaceNew(LOCKSPACE_DIR); + + if (!virFileIsDir(LOCKSPACE_DIR)) + goto cleanup; + + if (virLockSpaceCreateResource(lockspace, "foo") < 0) + goto cleanup; + + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) < 0) + goto cleanup; + + if (!virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) == 0) + goto cleanup; + + if (virLockSpaceDeleteResource(lockspace, "foo") == 0) + goto cleanup; + + if (virLockSpaceReleaseResource(lockspace, "foo", geteuid()) < 0) + goto cleanup; + + if (virLockSpaceDeleteResource(lockspace, "foo") < 0) + goto cleanup; + + if (virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + ret = 0; + +cleanup: + virLockSpaceFree(lockspace); + rmdir(LOCKSPACE_DIR); + return ret; +} + + +static int testLockSpaceResourceLockExclAuto(const void *args ATTRIBUTE_UNUSED) +{ + virLockSpacePtr lockspace; + int ret = -1; + + rmdir(LOCKSPACE_DIR); + + lockspace = virLockSpaceNew(LOCKSPACE_DIR); + + if (!virFileIsDir(LOCKSPACE_DIR)) + goto cleanup; + + if (virLockSpaceCreateResource(lockspace, "foo") < 0) + goto cleanup; + + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0) + goto cleanup; + + if (!virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + if (virLockSpaceReleaseResource(lockspace, "foo", geteuid()) < 0) + goto cleanup; + + if (virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + ret = 0; + +cleanup: + virLockSpaceFree(lockspace); + rmdir(LOCKSPACE_DIR); + return ret; +} + + +static int testLockSpaceResourceLockShr(const void *args ATTRIBUTE_UNUSED) +{ + virLockSpacePtr lockspace; + int ret = -1; + + rmdir(LOCKSPACE_DIR); + + lockspace = virLockSpaceNew(LOCKSPACE_DIR); + + if (!virFileIsDir(LOCKSPACE_DIR)) + goto cleanup; + + if (virLockSpaceCreateResource(lockspace, "foo") < 0) + goto cleanup; + + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + VIR_LOCK_SPACE_ACQUIRE_SHARED) < 0) + goto cleanup; + + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) == 0) + goto cleanup; + + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + VIR_LOCK_SPACE_ACQUIRE_SHARED) < 0) + goto cleanup; + + if (virLockSpaceDeleteResource(lockspace, "foo") == 0) + goto cleanup; + + if (virLockSpaceReleaseResource(lockspace, "foo", geteuid()) < 0) + goto cleanup; + + if (virLockSpaceDeleteResource(lockspace, "foo") == 0) + goto cleanup; + + if (virLockSpaceReleaseResource(lockspace, "foo", geteuid()) < 0) + goto cleanup; + + if (virLockSpaceDeleteResource(lockspace, "foo") < 0) + goto cleanup; + + if (virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + ret = 0; + +cleanup: + virLockSpaceFree(lockspace); + rmdir(LOCKSPACE_DIR); + return ret; +} + + +static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) +{ + virLockSpacePtr lockspace; + int ret = -1; + + rmdir(LOCKSPACE_DIR); + + lockspace = virLockSpaceNew(LOCKSPACE_DIR); + + if (!virFileIsDir(LOCKSPACE_DIR)) + goto cleanup; + + if (virLockSpaceCreateResource(lockspace, "foo") < 0) + goto cleanup; + + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + VIR_LOCK_SPACE_ACQUIRE_SHARED | + VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0) + goto cleanup; + + if (!virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) == 0) + goto cleanup; + + if (!virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + VIR_LOCK_SPACE_ACQUIRE_SHARED | + VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0) + goto cleanup; + + if (!virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + if (virLockSpaceReleaseResource(lockspace, "foo", geteuid()) < 0) + goto cleanup; + + if (!virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + if (virLockSpaceReleaseResource(lockspace, "foo", geteuid()) < 0) + goto cleanup; + + if (virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + ret = 0; + +cleanup: + virLockSpaceFree(lockspace); + rmdir(LOCKSPACE_DIR); + return ret; +} + + +static int testLockSpaceResourceLockPath(const void *args ATTRIBUTE_UNUSED) +{ + virLockSpacePtr lockspace; + int ret = -1; + + rmdir(LOCKSPACE_DIR); + + lockspace = virLockSpaceNew(NULL); + + mkdir(LOCKSPACE_DIR, 0700); + + if (virLockSpaceCreateResource(lockspace, LOCKSPACE_DIR "/foo") < 0) + goto cleanup; + + if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", geteuid(), 0) < 0) + goto cleanup; + + if (!virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", geteuid(), 0) == 0) + goto cleanup; + + if (virLockSpaceDeleteResource(lockspace, LOCKSPACE_DIR "/foo") == 0) + goto cleanup; + + if (virLockSpaceReleaseResource(lockspace, LOCKSPACE_DIR "/foo", geteuid()) < 0) + goto cleanup; + + if (virLockSpaceDeleteResource(lockspace, LOCKSPACE_DIR "/foo") < 0) + goto cleanup; + + if (virFileExists(LOCKSPACE_DIR "/foo")) + goto cleanup; + + ret = 0; + +cleanup: + virLockSpaceFree(lockspace); + rmdir(LOCKSPACE_DIR); + return ret; +} + + + +static int +mymain(void) +{ + int ret = 0; + + signal(SIGPIPE, SIG_IGN); + + if (virtTestRun("Lockspace creation", 1, testLockSpaceCreate, NULL) < 0) + ret = -1; + + if (virtTestRun("Lockspace res lifecycle", 1, testLockSpaceResourceLifecycle, NULL) < 0) + ret = -1; + + if (virtTestRun("Lockspace res lock excl", 1, testLockSpaceResourceLockExcl, NULL) < 0) + ret = -1; + + if (virtTestRun("Lockspace res lock shr", 1, testLockSpaceResourceLockShr, NULL) < 0) + ret = -1; + + if (virtTestRun("Lockspace res lock excl auto", 1, testLockSpaceResourceLockExclAuto, NULL) < 0) + ret = -1; + + if (virtTestRun("Lockspace res lock shr auto", 1, testLockSpaceResourceLockShrAuto, NULL) < 0) + ret = -1; + + if (virtTestRun("Lockspace res full path", 1, testLockSpaceResourceLockPath, NULL) < 0) + ret = -1; + + return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN(mymain) -- 1.7.11.2

On 08/09/2012 09:20 AM, Daniel P. Berrange wrote: I had a merge failure trying to apply this; you'll need a minor context rebase in cfg.mk to use this. You added a new branch for locking... ...so this branch won't be reached, so you should clean it up while here. Shouldn't we be using $(MKDIR_P) instead? and again. Is it worth using '= 1', '= 2', and so forth, to make future extensions aware that they must not be in the middle, since this is tied to RPC protocol? Oops - this can blindly re-close stdin or stdout or stderr if the parent process had those fds closed. It must be: if (stdinfd > STDERR_FILENO && VIR_CLOSE(stdinfd) < 0) goto cleanup; stdinfd = -1; if (stdoutfd > STDERR_FILENO && VIR_CLOSE(stdoutfd) < 0) goto cleanup; stdoutfd = -1; _exit(EXIT_SUCCESS) (I thought we had a syntax check for this - oh, I see - that rule checked for 'exit' but not '_exit' or '_Exit'. Gnulib patch coming up). Another case where EXIT_{SUCCESS,FAILURE} looks better. Copy and paste, but these are redundant (). Why are we copying so much code? Should some of this live in a common file under util, to be used by both this file and daemon/libvirtd.c? Do we need to worry about setting SIGPIPE to a known state? Shouldn't we also list '--help' as an option? 129 is still a valid character in single-byte locales, which someone could pass in from the command line via a short option and possibly mess up getopt_long. I think you want it to be 256 or greater. Why do these two use the non-NULL flag argument, instead of the more typical "NULL, 'd'" layout... ...especially since here you also handle short options for the same task, which do the same work? (I know, copy and paste from a bad example in libvirtd.c) That says that 'virtlockd --help' will fail with status 2, even when it successfully printed the usage message. I think you need to route --help slightly differently than the default handling of getopt_long error handling, if only to control the exit status differently (again, copying from a bad example). Looks mostly reasonable. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 08/09/2012 09:20 AM, Daniel P. Berrange wrote: Incomplete: squash this in so that we also use 'pdwtags' to ensure on-the-wire compatibility: diff --git c/src/Makefile.am i/src/Makefile.am index 30a5b42..ab228a5 100644 --- c/src/Makefile.am +++ i/src/Makefile.am @@ -278,7 +278,7 @@ EXTRA_DIST += $(REMOTE_DRIVER_PROTOCOL) \ # The alternation of the following regexps matches both cases. r1 = /\* \d+ \*/ r2 = /\* <[[:xdigit:]]+> \S+:\d+ \*/ -struct_prefix = (remote_|qemu_|virNet|keepalive_) +struct_prefix = (remote_|qemu_|virNet|keepalive_|virLockSpace) PDWTAGS = \ $(AM_V_GEN)if (pdwtags --help) > /dev/null 2>&1; then \ @@ -326,7 +326,9 @@ PROTOCOL_STRUCTS = \ $(srcdir)/remote_protocol-structs \ $(srcdir)/qemu_protocol-structs \ $(srcdir)/virnetprotocol-structs \ - $(srcdir)/virkeepaliveprotocol-structs + $(srcdir)/virkeepaliveprotocol-structs \ + $(srcdir)/lock_protocol-structs \ + $(NULL) if WITH_REMOTE check-protocol: $(PROTOCOL_STRUCTS) $(PROTOCOL_STRUCTS:structs=struct) @@ -338,6 +340,9 @@ $(srcdir)/remote_protocol-struct $(srcdir)/qemu_protocol-struct: \ $(srcdir)/virnetprotocol-struct $(srcdir)/virkeepaliveprotocol-struct: \ $(srcdir)/%-struct: libvirt_net_rpc_la-%.lo $(PDWTAGS) +# lock_protocol is built without libtool +$(srcdir)/lock_protocol-struct: $(srcdir)/%-struct: virtlockd-%.o + $(PDWTAGS) else !WITH_REMOTE # The $(PROTOCOL_STRUCTS) files must live in git, because they cannot be # re-generated when configured --without-remote. diff --git c/src/lock_protocol-structs i/src/lock_protocol-structs new file mode 100644 index 0000000..444bdb1 --- /dev/null +++ i/src/lock_protocol-structs @@ -0,0 +1,51 @@ +/* -*- c -*- */ +struct virLockSpaceProtocolOwner { + virLockSpaceProtocolUUID uuid; + virLockSpaceProtocolNonNullString name; + u_int id; + u_int pid; +}; +struct virLockSpaceProtocolRegisterArgs { + virLockSpaceProtocolOwner owner; + u_int flags; +}; +struct virLockSpaceProtocolRestrictArgs { + u_int flags; +}; +struct virLockSpaceProtocolNewArgs { + virLockSpaceProtocolNonNullString path; + u_int flags; +}; +struct virLockSpaceProtocolCreateResourceArgs { + virLockSpaceProtocolNonNullString path; + virLockSpaceProtocolNonNullString name; + u_int flags; +}; +struct virLockSpaceProtocolDeleteResourceArgs { + virLockSpaceProtocolNonNullString path; + virLockSpaceProtocolNonNullString name; + u_int flags; +}; +enum virLockSpaceProtocolAcquireResourceFlags { + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED = 1, + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE = 2, +}; +struct virLockSpaceProtocolAcquireResourceArgs { + virLockSpaceProtocolNonNullString path; + virLockSpaceProtocolNonNullString name; + u_int flags; +}; +struct virLockSpaceProtocolReleaseResourceArgs { + virLockSpaceProtocolNonNullString path; + virLockSpaceProtocolNonNullString name; + u_int flags; +}; +enum virLockSpaceProtocolProcedure { + VIR_LOCK_SPACE_PROTOCOL_PROC_REGISTER = 1, + VIR_LOCK_SPACE_PROTOCOL_PROC_RESTRICT = 2, + VIR_LOCK_SPACE_PROTOCOL_PROC_NEW = 3, + VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_RESOURCE = 4, + VIR_LOCK_SPACE_PROTOCOL_PROC_DELETE_RESOURCE = 5, + VIR_LOCK_SPACE_PROTOCOL_PROC_ACQUIRE_RESOURCE = 6, + VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE = 7, +}; [oh yuck - on re-reading that, we may have issues if someone configures where WITH_REMOTE and WITH_LIBVIRTD are not both empty or both set - I may have to do followup patches for those scenarios]. Not sorted. Are we sure that this will always be wide enough? That is, does the virtlockd binary make sense on mingw, where pid_t is 64 bits, or are we reasonably sure that we are safe with restricting the RPC call to 32 bits because it will only ever be used on UNIX-y systems? I guess you didn't bother hooking this up to remote_generator.pl :) For only 7 functions, that's probably okay. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 08/09/2012 09:20 AM, Daniel P. Berrange wrote: I hit a merge conflict applying this one, as well (this time in src/Makefile.am); shouldn't be too hard to rebase. Missing a \ Long lines; some \ would be nice. Stale comment - the goto is what lets you jump to an error label, and thus clean up any non-trivial code used before this macro. Rather than open-coding this loop, can't you use virPidAbort() from util/command.h? If you check flags here... ...or delay the assignment of priv... ...and check flags here, with virCheckFlags(..., -1)... ...then you wouldn't need virCheckFlagsGoto. Then again, I don't mind having the new macro, as it gives you the option to put flag checks at a location easier to read. Any reason you have two namespaces of flags, and have to translate between them, rather than guaranteeing that the flags have the same values and can be reused in both contexts? -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 08/09/2012 09:20 AM, Daniel P. Berrange wrote: s/enhancement/enhances/ I didn't closely read the systemd documentation to see if you integrated correctly, so I have to assume that you tested this and that it worked. That said, I'll review the C code for accuracy. $(MKDIR_P) s/INSTALL_SCRIPT/INSTALL_DATA/2 - systemd files should not be marked executable (cf. install-init-systemd in daemon/Makefile.am). Problem. Here, you look for '::sbindir::'... I was scratching my head on this one, until I realized you want to re-exec, and the re-exec uses our mechanisms rather than systemd mechanisms to reuse the fd. ...but here, you used @sbindir@. s/Loosing/Losing/ Losing locks is bad, but does virtlockd really cause fencing on failure, or are you over-exaggerating the effects due to copying from something stricter? -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 08/09/2012 09:20 AM, Daniel P. Berrange wrote: s/upgrads/upgrades/ s/systemd/systems/ ? That says 'if virtlockd.service failed to report status, then send it SIGUSR1'. Don't you really want to check '$? = 0'? Rather than re-creating with the original default of 3, should we recreate with the number of elements read from JSON? (Of course, that means you can't call virHashCreate until later in the function, which may be too complex to wait for) This hunk belongs in the previous patch. This cast to (pid_t) might result in truncation. Should we be doing further sanity checking, such as ensuring the cast doesn't truncate and that the resulting pid is non-negative? Oh - now I see something that I wasn't catching when I complained about the earlier patches dealing with a fork/CLOEXEC/exec timing - you _aren't_ forking, but directly calling exec to overlay the current process with the new binary! Furthermore, although it looks like virtlockd calls fork to daemonize at startup, it never spawns any child processes - so we _don't_ have to worry about any O_CLOEXEC races on creation, or restoring CLOEXEC on restart, precisely because we never spawn off a child in another thread where the leak would be problematic. Still, I would feel a lot better if we document this clearly in the code (maybe 'this function is only safe to call from virtlockd' for all PreExec functions that clear CLOEXEC). Shouldn't 0600 be sufficient? We don't need to execute the .json file. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 08/09/2012 09:20 AM, Daniel P. Berrange wrote: 2012 Rebase this out. Is this safe even in the presence of another app linking in libvirt.so and modifying its own environment? It seems like we might want to strdup() that into safe static storage to isolate ourselves from future putenv() nonsense. Is the empty space just so that you can trace that the function was reached? Is it worth any /* since 0.10.0 */ comments, in case we later add to this struct, to know when each callback supported which lock manager features? -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 08/09/2012 09:20 AM, Daniel P. Berrange wrote: Question - can virtlockd also be made to work in multi-host mode for shared file systems that support fcntl locking, possibly by adding some non-default configuration? Missing documentation in docs/locking.html.in on how to use virtlockd instead of sanlock, and any tips on setting up multi-box environments (if those are indeed possible). Is the config setting 'virtlockd'... ...or 'lockd'? This needs to be consistent. Maybe call out: This defaults to 'lockd' to use the virtlockd(8) daemon... Hmm, that means an earlier patch is also missing a virtlockd.8 man page. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org