[libvirt PATCH 0/4] virnetserver: Make pool job name less generic
Jiri Denemark (4): virthreadpool: Copy job name virnetserver: Format functions consistently virnetserver: Use autoptr for virNetServer and virNetServerClient virnetserver: Make pool job name less generic src/rpc/virnetserver.c | 247 ++++++++++++++++++++--------------- src/rpc/virnetserverclient.h | 2 + src/util/virthreadpool.c | 5 +- 3 files changed, 145 insertions(+), 109 deletions(-) -- 2.34.0
Currently virThreadPoolNewFull relies on the caller to ensure the job name outlives the thread pool. Which basically enforces static strings. Let's drop this implicit requirement by making a copy of the job name. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/util/virthreadpool.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index 7bf4333885..426840e435 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -50,7 +50,7 @@ struct _virThreadPool { bool quit; virThreadPoolJobFunc jobFunc; - const char *jobName; + char *jobName; void *jobOpaque; virThreadPoolJobList jobList; size_t jobQueueDepth; @@ -237,7 +237,7 @@ virThreadPoolNewFull(size_t minWorkers, pool->jobList.tail = pool->jobList.head = NULL; pool->jobFunc = func; - pool->jobName = name; + pool->jobName = g_strdup(name); pool->jobOpaque = opaque; if (identity) @@ -312,6 +312,7 @@ void virThreadPoolFree(virThreadPool *pool) if (pool->identity) g_object_unref(pool->identity); + g_free(pool->jobName); g_free(pool->workers); virMutexUnlock(&pool->mutex); virMutexDestroy(&pool->mutex); -- 2.34.0
The file used a pretty inconsistent style for formatting function headers. Return types were both separate and on the same line as function names and functions were separated by one, two, and sometimes even three empty lines. Let's make it consistent by honoring our preferred coding style. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/rpc/virnetserver.c | 180 +++++++++++++++++++++++++---------------- 1 file changed, 112 insertions(+), 68 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index c7b4939398..f016f15f39 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -82,7 +82,8 @@ static void virNetServerUpdateServicesLocked(virNetServer *srv, static inline size_t virNetServerTrackPendingAuthLocked(virNetServer *srv); static inline size_t virNetServerTrackCompletedAuthLocked(virNetServer *srv); -static int virNetServerOnceInit(void) +static int +virNetServerOnceInit(void) { if (!VIR_CLASS_NEW(virNetServer, virClassForObjectLockable())) return -1; @@ -92,7 +93,9 @@ static int virNetServerOnceInit(void) VIR_ONCE_GLOBAL_INIT(virNetServer); -unsigned long long virNetServerNextClientID(virNetServer *srv) + +unsigned long long +virNetServerNextClientID(virNetServer *srv) { unsigned long long val; @@ -103,10 +106,12 @@ unsigned long long virNetServerNextClientID(virNetServer *srv) return val; } -static int virNetServerProcessMsg(virNetServer *srv, - virNetServerClient *client, - virNetServerProgram *prog, - virNetMessage *msg) + +static int +virNetServerProcessMsg(virNetServer *srv, + virNetServerClient *client, + virNetServerProgram *prog, + virNetMessage *msg) { if (!prog) { /* Only send back an error for type == CALL. Other @@ -141,7 +146,10 @@ static int virNetServerProcessMsg(virNetServer *srv, return 0; } -static void virNetServerHandleJob(void *jobOpaque, void *opaque) + +static void +virNetServerHandleJob(void *jobOpaque, + void *opaque) { virNetServer *srv = opaque; virNetServerJob *job = jobOpaque; @@ -165,6 +173,7 @@ static void virNetServerHandleJob(void *jobOpaque, void *opaque) VIR_FREE(job); } + /** * virNetServerGetProgramLocked: * @srv: server (must be locked by the caller) @@ -186,6 +195,7 @@ virNetServerGetProgramLocked(virNetServer *srv, return NULL; } + static void virNetServerDispatchNewMessage(virNetServerClient *client, virNetMessage *msg, @@ -289,8 +299,10 @@ virNetServerCheckLimits(virNetServer *srv) } } -int virNetServerAddClient(virNetServer *srv, - virNetServerClient *client) + +int +virNetServerAddClient(virNetServer *srv, + virNetServerClient *client) { virObjectLock(srv); @@ -323,9 +335,11 @@ int virNetServerAddClient(virNetServer *srv, return -1; } -static int virNetServerDispatchNewClient(virNetServerService *svc, - virNetSocket *clientsock, - void *opaque) + +static int +virNetServerDispatchNewClient(virNetServerService *svc, + virNetSocket *clientsock, + void *opaque) { virNetServer *srv = opaque; virNetServerClient *client; @@ -352,19 +366,20 @@ static int virNetServerDispatchNewClient(virNetServerService *svc, } -virNetServer *virNetServerNew(const char *name, - unsigned long long next_client_id, - size_t min_workers, - size_t max_workers, - size_t priority_workers, - size_t max_clients, - size_t max_anonymous_clients, - int keepaliveInterval, - unsigned int keepaliveCount, - virNetServerClientPrivNew clientPrivNew, - virNetServerClientPrivPreExecRestart clientPrivPreExecRestart, - virFreeCallback clientPrivFree, - void *clientPrivOpaque) +virNetServer * +virNetServerNew(const char *name, + unsigned long long next_client_id, + size_t min_workers, + size_t max_workers, + size_t priority_workers, + size_t max_clients, + size_t max_anonymous_clients, + int keepaliveInterval, + unsigned int keepaliveCount, + virNetServerClientPrivNew clientPrivNew, + virNetServerClientPrivPreExecRestart clientPrivPreExecRestart, + virFreeCallback clientPrivFree, + void *clientPrivOpaque) { virNetServer *srv; @@ -401,13 +416,14 @@ virNetServer *virNetServerNew(const char *name, } -virNetServer *virNetServerNewPostExecRestart(virJSONValue *object, - const char *name, - virNetServerClientPrivNew clientPrivNew, - virNetServerClientPrivNewPostExecRestart clientPrivNewPostExecRestart, - virNetServerClientPrivPreExecRestart clientPrivPreExecRestart, - virFreeCallback clientPrivFree, - void *clientPrivOpaque) +virNetServer * +virNetServerNewPostExecRestart(virJSONValue *object, + const char *name, + virNetServerClientPrivNew clientPrivNew, + virNetServerClientPrivNewPostExecRestart clientPrivNewPostExecRestart, + virNetServerClientPrivPreExecRestart clientPrivPreExecRestart, + virFreeCallback clientPrivFree, + void *clientPrivOpaque) { virNetServer *srv = NULL; virJSONValue *clients; @@ -553,7 +569,8 @@ virNetServer *virNetServerNewPostExecRestart(virJSONValue *object, } -virJSONValue *virNetServerPreExecRestart(virNetServer *srv) +virJSONValue * +virNetServerPreExecRestart(virNetServer *srv) { g_autoptr(virJSONValue) object = virJSONValueNewObject(); g_autoptr(virJSONValue) clients = virJSONValueNewArray(); @@ -621,9 +638,9 @@ virJSONValue *virNetServerPreExecRestart(virNetServer *srv) } - -int virNetServerAddService(virNetServer *srv, - virNetServerService *svc) +int +virNetServerAddService(virNetServer *srv, + virNetServerService *svc) { virObjectLock(srv); @@ -687,17 +704,18 @@ virNetServerAddServiceActivation(virNetServer *srv, } -int virNetServerAddServiceTCP(virNetServer *srv, - virSystemdActivation *act, - const char *actname, - const char *nodename, - const char *service, - int family, - int auth, - virNetTLSContext *tls, - bool readonly, - size_t max_queued_clients, - size_t nrequests_client_max) +int +virNetServerAddServiceTCP(virNetServer *srv, + virSystemdActivation *act, + const char *actname, + const char *nodename, + const char *service, + int family, + int auth, + virNetTLSContext *tls, + bool readonly, + size_t max_queued_clients, + size_t nrequests_client_max) { virNetServerService *svc = NULL; int ret; @@ -735,17 +753,18 @@ int virNetServerAddServiceTCP(virNetServer *srv, } -int virNetServerAddServiceUNIX(virNetServer *srv, - virSystemdActivation *act, - const char *actname, - const char *path, - mode_t mask, - gid_t grp, - int auth, - virNetTLSContext *tls, - bool readonly, - size_t max_queued_clients, - size_t nrequests_client_max) +int +virNetServerAddServiceUNIX(virNetServer *srv, + virSystemdActivation *act, + const char *actname, + const char *path, + mode_t mask, + gid_t grp, + int auth, + virNetTLSContext *tls, + bool readonly, + size_t max_queued_clients, + size_t nrequests_client_max) { virNetServerService *svc = NULL; int ret; @@ -783,8 +802,9 @@ int virNetServerAddServiceUNIX(virNetServer *srv, } -int virNetServerAddProgram(virNetServer *srv, - virNetServerProgram *prog) +int +virNetServerAddProgram(virNetServer *srv, + virNetServerProgram *prog) { virObjectLock(srv); @@ -795,8 +815,10 @@ int virNetServerAddProgram(virNetServer *srv, return 0; } -int virNetServerSetTLSContext(virNetServer *srv, - virNetTLSContext *tls) + +int +virNetServerSetTLSContext(virNetServer *srv, + virNetTLSContext *tls) { srv->tls = virObjectRef(tls); return 0; @@ -856,15 +878,18 @@ virNetServerUpdateServicesLocked(virNetServer *srv, } -void virNetServerUpdateServices(virNetServer *srv, - bool enabled) +void +virNetServerUpdateServices(virNetServer *srv, + bool enabled) { virObjectLock(srv); virNetServerUpdateServicesLocked(srv, enabled); virObjectUnlock(srv); } -void virNetServerDispose(void *obj) + +void +virNetServerDispose(void *obj) { virNetServer *srv = obj; size_t i; @@ -886,7 +911,9 @@ void virNetServerDispose(void *obj) g_free(srv->clients); } -void virNetServerClose(virNetServer *srv) + +void +virNetServerClose(virNetServer *srv) { size_t i; @@ -906,18 +933,21 @@ void virNetServerClose(virNetServer *srv) virObjectUnlock(srv); } + void virNetServerShutdownWait(virNetServer *srv) { virThreadPoolDrain(srv->workers); } + static inline size_t virNetServerTrackPendingAuthLocked(virNetServer *srv) { return ++srv->nclients_unauth; } + static inline size_t virNetServerTrackCompletedAuthLocked(virNetServer *srv) { @@ -937,6 +967,7 @@ virNetServerHasClients(virNetServer *srv) return ret; } + void virNetServerProcessClients(virNetServer *srv) { @@ -974,12 +1005,14 @@ virNetServerProcessClients(virNetServer *srv) virObjectUnlock(srv); } + const char * virNetServerGetName(virNetServer *srv) { return srv->name; } + int virNetServerGetThreadPoolParameters(virNetServer *srv, size_t *minWorkers, @@ -1002,6 +1035,7 @@ virNetServerGetThreadPoolParameters(virNetServer *srv, return 0; } + int virNetServerSetThreadPoolParameters(virNetServer *srv, long long int minWorkers, @@ -1018,6 +1052,7 @@ virNetServerSetThreadPoolParameters(virNetServer *srv, return ret; } + size_t virNetServerGetMaxClients(virNetServer *srv) { @@ -1030,6 +1065,7 @@ virNetServerGetMaxClients(virNetServer *srv) return ret; } + size_t virNetServerGetCurrentClients(virNetServer *srv) { @@ -1042,6 +1078,7 @@ virNetServerGetCurrentClients(virNetServer *srv) return ret; } + size_t virNetServerGetMaxUnauthClients(virNetServer *srv) { @@ -1054,6 +1091,7 @@ virNetServerGetMaxUnauthClients(virNetServer *srv) return ret; } + size_t virNetServerGetCurrentUnauthClients(virNetServer *srv) { @@ -1067,8 +1105,9 @@ virNetServerGetCurrentUnauthClients(virNetServer *srv) } -bool virNetServerNeedsAuth(virNetServer *srv, - int auth) +bool +virNetServerNeedsAuth(virNetServer *srv, + int auth) { bool ret = false; size_t i; @@ -1083,6 +1122,7 @@ bool virNetServerNeedsAuth(virNetServer *srv, return ret; } + int virNetServerGetClients(virNetServer *srv, virNetServerClient ***clts) @@ -1105,6 +1145,7 @@ virNetServerGetClients(virNetServer *srv, return nclients; } + virNetServerClient * virNetServerGetClient(virNetServer *srv, unsigned long long id) @@ -1128,6 +1169,7 @@ virNetServerGetClient(virNetServer *srv, return ret; } + int virNetServerSetClientLimits(virNetServer *srv, long long int maxClients, @@ -1164,6 +1206,7 @@ virNetServerSetClientLimits(virNetServer *srv, return ret; } + static virNetTLSContext * virNetServerGetTLSContext(virNetServer *srv) { @@ -1182,6 +1225,7 @@ virNetServerGetTLSContext(virNetServer *srv) return ctxt; } + int virNetServerUpdateTlsFiles(virNetServer *srv) { -- 2.34.0
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/rpc/virnetserver.c | 64 +++++++++++++++--------------------- src/rpc/virnetserverclient.h | 2 ++ 2 files changed, 28 insertions(+), 38 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index f016f15f39..d0f248e7f5 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -342,7 +342,7 @@ virNetServerDispatchNewClient(virNetServerService *svc, void *opaque) { virNetServer *srv = opaque; - virNetServerClient *client; + g_autoptr(virNetServerClient) client = NULL; if (!(client = virNetServerClientNew(virNetServerNextClientID(srv), clientsock, @@ -358,10 +358,8 @@ virNetServerDispatchNewClient(virNetServerService *svc, if (virNetServerAddClient(srv, client) < 0) { virNetServerClientClose(client); - virObjectUnref(client); return -1; } - virObjectUnref(client); return 0; } @@ -381,7 +379,7 @@ virNetServerNew(const char *name, virFreeCallback clientPrivFree, void *clientPrivOpaque) { - virNetServer *srv; + g_autoptr(virNetServer) srv = NULL; if (virNetServerInitialize() < 0) return NULL; @@ -395,7 +393,7 @@ virNetServerNew(const char *name, "rpc-worker", NULL, srv))) - goto error; + return NULL; srv->name = g_strdup(name); @@ -409,10 +407,7 @@ virNetServerNew(const char *name, srv->clientPrivFree = clientPrivFree; srv->clientPrivOpaque = clientPrivOpaque; - return srv; - error: - virObjectUnref(srv); - return NULL; + return g_steal_pointer(&srv); } @@ -425,7 +420,7 @@ virNetServerNewPostExecRestart(virJSONValue *object, virFreeCallback clientPrivFree, void *clientPrivOpaque) { - virNetServer *srv = NULL; + g_autoptr(virNetServer) srv = NULL; virJSONValue *clients; virJSONValue *services; size_t i; @@ -441,29 +436,29 @@ virNetServerNewPostExecRestart(virJSONValue *object, if (virJSONValueObjectGetNumberUint(object, "min_workers", &min_workers) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing min_workers data in JSON document")); - goto error; + return NULL; } if (virJSONValueObjectGetNumberUint(object, "max_workers", &max_workers) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing max_workers data in JSON document")); - goto error; + return NULL; } if (virJSONValueObjectGetNumberUint(object, "priority_workers", &priority_workers) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing priority_workers data in JSON document")); - goto error; + return NULL; } if (virJSONValueObjectGetNumberUint(object, "max_clients", &max_clients) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing max_clients data in JSON document")); - goto error; + return NULL; } if (virJSONValueObjectHasKey(object, "max_anonymous_clients")) { if (virJSONValueObjectGetNumberUint(object, "max_anonymous_clients", &max_anonymous_clients) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Malformed max_anonymous_clients data in JSON document")); - goto error; + return NULL; } } else { max_anonymous_clients = max_clients; @@ -471,12 +466,12 @@ virNetServerNewPostExecRestart(virJSONValue *object, if (virJSONValueObjectGetNumberUint(object, "keepaliveInterval", &keepaliveInterval) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing keepaliveInterval data in JSON document")); - goto error; + return NULL; } if (virJSONValueObjectGetNumberUint(object, "keepaliveCount", &keepaliveCount) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing keepaliveCount data in JSON document")); - goto error; + return NULL; } if (virJSONValueObjectGetNumberUlong(object, "next_client_id", @@ -492,18 +487,18 @@ virNetServerNewPostExecRestart(virJSONValue *object, keepaliveInterval, keepaliveCount, clientPrivNew, clientPrivPreExecRestart, clientPrivFree, clientPrivOpaque))) - goto error; + return NULL; if (!(services = virJSONValueObjectGet(object, "services"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing services data in JSON document")); - goto error; + return NULL; } if (!virJSONValueIsArray(services)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Malformed services array")); - goto error; + return NULL; } for (i = 0; i < virJSONValueArraySize(services); i++) { @@ -512,15 +507,15 @@ virNetServerNewPostExecRestart(virJSONValue *object, if (!child) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing service data in JSON document")); - goto error; + return NULL; } if (!(service = virNetServerServiceNewPostExecRestart(child))) - goto error; + return NULL; if (virNetServerAddService(srv, service) < 0) { virObjectUnref(service); - goto error; + return NULL; } } @@ -528,22 +523,22 @@ virNetServerNewPostExecRestart(virJSONValue *object, if (!(clients = virJSONValueObjectGet(object, "clients"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing clients data in JSON document")); - goto error; + return NULL; } if (!virJSONValueIsArray(clients)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Malformed clients array")); - goto error; + return NULL; } for (i = 0; i < virJSONValueArraySize(clients); i++) { - virNetServerClient *client; + g_autoptr(virNetServerClient) client = NULL; virJSONValue *child = virJSONValueArrayGet(clients, i); if (!child) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing client data in JSON document")); - goto error; + return NULL; } if (!(client = virNetServerClientNewPostExecRestart(srv, @@ -552,20 +547,13 @@ virNetServerNewPostExecRestart(virJSONValue *object, clientPrivPreExecRestart, clientPrivFree, clientPrivOpaque))) - goto error; + return NULL; - if (virNetServerAddClient(srv, client) < 0) { - virObjectUnref(client); - goto error; - } - virObjectUnref(client); + if (virNetServerAddClient(srv, client) < 0) + return NULL; } - return srv; - - error: - virObjectUnref(srv); - return NULL; + return g_steal_pointer(&srv); } diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h index a9d0afe95a..0d585eb2ce 100644 --- a/src/rpc/virnetserverclient.h +++ b/src/rpc/virnetserverclient.h @@ -28,8 +28,10 @@ #include "virjson.h" typedef struct _virNetServer virNetServer; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetServer, virObjectUnref); typedef struct _virNetServerClient virNetServerClient; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetServerClient, virObjectUnref); /* This function owns the "msg" pointer it is passed and * must arrange for virNetMessageFree to be called on it -- 2.34.0
The generic "rpc-worker" name becomes a name of the associated task, which may than appear in logs and bring some confusion. Let's add a server name to it so that one can easily see which daemon the task belongs to, which is especially useful for split daemons. And since the name would be too long, we can drop the "-worker" part and just keep it as "rpc-*" and "prio-rpc-*". Such confusing entries can, for example, be found in audit log when SELinux is complaining that "rpc-worker" was denied access to something. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/rpc/virnetserver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index d0f248e7f5..ad581a36dd 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -380,6 +380,7 @@ virNetServerNew(const char *name, void *clientPrivOpaque) { g_autoptr(virNetServer) srv = NULL; + g_autofree char *jobName = g_strdup_printf("rpc-%s", name); if (virNetServerInitialize() < 0) return NULL; @@ -390,7 +391,7 @@ virNetServerNew(const char *name, if (!(srv->workers = virThreadPoolNewFull(min_workers, max_workers, priority_workers, virNetServerHandleJob, - "rpc-worker", + jobName, NULL, srv))) return NULL; -- 2.34.0
On 11/26/21 20:52, Jiri Denemark wrote:
Jiri Denemark (4): virthreadpool: Copy job name virnetserver: Format functions consistently virnetserver: Use autoptr for virNetServer and virNetServerClient virnetserver: Make pool job name less generic
src/rpc/virnetserver.c | 247 ++++++++++++++++++++--------------- src/rpc/virnetserverclient.h | 2 + src/util/virthreadpool.c | 5 +- 3 files changed, 145 insertions(+), 109 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Jiri Denemark -
Michal Prívozník