[PATCH 1/3] dix: Add facilities for client ID tracking.
Vignatti Tiago (Nokia-MS/Helsinki)
tiago.vignatti at nokia.com
Mon Aug 30 10:52:25 PDT 2010
Hi,
On Mon, Aug 30, 2010 at 03:29:31PM +0200, ext Rami Ylimäki wrote:
> Make some existing functionality from SELinux and IA extensions available
> for general use.
probably you need to preserve the original copyright through the files then? I
don't know how it works (for instance, if depends the amount of code you
copied)
> Signed-off-by: Rami Ylimäki <rami.ylimaki at vincit.fi>
> ---
> dix/Makefile.am | 1 +
> dix/client.c | 273 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> include/client.h | 51 ++++++++++
> 3 files changed, 325 insertions(+), 0 deletions(-)
> create mode 100644 dix/client.c
> create mode 100644 include/client.h
>
> diff --git a/dix/Makefile.am b/dix/Makefile.am
> index 5e2dad7..bc87da5 100644
> --- a/dix/Makefile.am
> +++ b/dix/Makefile.am
> @@ -7,6 +7,7 @@ libmain_la_SOURCES = \
>
> libdix_la_SOURCES = \
> atom.c \
> + client.c \
> colormap.c \
> cursor.c \
> deprecated.c \
> diff --git a/dix/client.c b/dix/client.c
> new file mode 100644
> index 0000000..de55b3e
> --- /dev/null
> +++ b/dix/client.c
> @@ -0,0 +1,273 @@
> +/*
> + * Copyright (c) 2010, Nokia Corporation.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
it's always cool and noble to amend your authorship here :)
Also, client.c is not so suggestive name for a client tracking information.
Maybe you could also put some note on the top of this file mentioning what's
the intention of this file. It helps a lot.
> + */
> +
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include "client.h"
> +#include "dixstruct.h"
> +
> +/* Key for identifying ID information for a client. */
> +static DevPrivateKeyRec ClientIdsPrivKeyRec;
> +static DevPrivateKey ClientIdsPrivKey = &ClientIdsPrivKeyRec;
> +
> +/* Return client private holding PID and command line string. Error
> + * (NULL) is returned if PID is not available for the client. */
> +ClientIdsPrivatePtr GetClientIds(ClientPtr client)
> +{
> + PrivatePtr *privates = &(client)->devPrivates;
> + pointer priv = dixLookupPrivate(privates, ClientIdsPrivKey);
> + return (ClientIdsPrivatePtr) priv;
> +}
> +
> +/* Called after PID and command line string have been determined for a
> + * client (all clients, including remote clients, except server
> + * client). You may call GetClientPid and GetClientCmd after this
> + * notification. */
> +static CallbackListPtr ClientIdsReservedCbs = NULL;
> +CallbackListPtr *GetClientIdsReservedCbs(void)
> +{
> + return &ClientIdsReservedCbs;
> +}
> +
> +/* Called before PID and command line string will be invalidated for a
> + * client (all clients, including remote clients, except server
> + * client). GetClientPid and GetClientCmd will return errors when
> + * called after this notification. */
> +static CallbackListPtr ClientIdsReleasedCbs = NULL;
> +CallbackListPtr *GetClientIdsReleasedCbs(void)
> +{
> + return &ClientIdsReleasedCbs;
> +}
> +
> +/* Try to determine a PID for a client from its connection
> + * information. This should be called only once when new client has
> + * connected, use GetClientPid to determine the PID at other
> + * times. Error (0) is returned if PID can't be determined for the
> + * client. */
> +static pid_t DetermineClientPid(ClientPtr client)
> +{
> + LocalClientCredRec *lcc = NULL;
> + pid_t pid = 0;
> +
> + if (client == NullClient)
> + return 0;
> +
> + if (client == serverClient)
> + return getpid();
> +
> + if (GetLocalClientCreds(client, &lcc) != -1)
> + {
> + if (lcc->fieldsSet & LCC_PID_SET)
> + pid = lcc->pid;
> + FreeLocalClientCreds(lcc);
> + }
> +
> + return pid;
> +}
> +
> +/* Try to determine a command line string for a client based on its
> + * PID. This should be called only once when a new client has
> + * connected, use GetClientCmd to determine the string at other
> + * times. Error (NULL) is returned if command line string can't be
> + * determined for the client. You must release the returned string by
> + * calling free when it's not used anymore. */
> +static const char *DetermineClientCmd(ClientPtr client, pid_t pid)
> +{
> + char path[PATH_MAX + 1];
> + char *cmd = NULL;
> + int fd = 0;
> + int bytes = 0;
> +
> + if (client == NullClient)
> + return NULL;
> +
> + if (snprintf(path, sizeof(path), "/proc/%d/cmdline", pid) < 0)
> + return NULL;
> +
> + fd = open(path, O_RDONLY);
> + if (fd < 0)
> + return NULL;
> + bytes = read(fd, path, sizeof(path));
> + if (bytes <= 0)
> + return NULL;
> + if (close(fd) < 0)
> + return NULL;
> +
> + /* We are only interested in the process name. We don't care about
> + * its arguments. Allocate space only for the process name. */
> + path[bytes - 1] = '\0';
> + bytes = strlen(path) + 1;
> + cmd = malloc(bytes);
> + if (cmd == NULL)
> + return NULL;
> + strncpy(cmd, path, bytes);
> + cmd[bytes - 1] = '\0';
> +
> + return cmd;
> +}
> +
> +/* Called when new client connects. Allocates client private for the
> + * client and fills it with ID information. */
> +static void ReserveClientIds(ClientPtr client)
> +{
> + ClientIdsPrivatePtr priv = NULL;
> + pid_t pid = 0;
> +
> + if (client == NullClient)
> + return;
> +
> + /* Allocate private structure only if PID is available. */
> + pid = DetermineClientPid(client);
> + if (pid)
> + {
> + priv = malloc(sizeof(ClientIdsPrivateRec));
> + if (priv)
> + {
> + priv->pid = pid;
> + priv->cmd = DetermineClientCmd(client, pid);
> + }
> + }
> +
> + DebugF("client(%lx): Reserved pid(%d).\n",
> + client->clientAsMask, priv ? priv->pid : 0);
> + DebugF("client(%lx): Reserved cmd(%s).\n",
> + client->clientAsMask, (priv && priv->cmd) ? priv->cmd : "NULL");
> +
> + dixSetPrivate(&(client)->devPrivates, ClientIdsPrivKey, priv);
> +}
> +
> +/* Called when existing client disconnects. Frees client ID
> + * information as well as the client private. */
> +static void ReleaseClientIds(ClientPtr client)
> +{
> + ClientIdsPrivatePtr priv = NULL;
> +
> + if (client == NullClient)
> + return;
> +
> + priv = GetClientIds(client);
> + if (!priv)
> + return;
> +
> + DebugF("client(%lx): Released pid(%d).\n",
> + client->clientAsMask, priv ? priv->pid : 0);
> + DebugF("client(%lx): Released cmd(%s).\n",
> + client->clientAsMask, (priv && priv->cmd) ? priv->cmd : "NULL");
> +
> + free((void *) priv->cmd); /* const char * */
> + free(priv);
> + dixSetPrivate(&(client)->devPrivates, ClientIdsPrivKey, NULL);
> +}
> +
> +/* Called when new client connects or existing client disconnects. */
> +static void ClientIdsChanged(CallbackListPtr *pcbl, pointer nulldata, pointer calldata)
> +{
> + NewClientInfoRec *pci = (NewClientInfoRec *)calldata;
> + ClientPtr client = pci->client;
> +
> + switch (client->clientState)
> + {
> + case ClientStateGone:
> + case ClientStateRetained:
> + /* Notify subscribers that client IDs will be released before
> + * they are actually released. */
> + CallCallbacks(&ClientIdsReleasedCbs, client);
> + ReleaseClientIds(client);
> + break;
> + case ClientStateInitial:
> + /* Reserve client IDs and notify subscribers about new IDs
> + * afterwards. */
> + ReserveClientIds(client);
> + CallCallbacks(&ClientIdsReservedCbs, client);
> + break;
> + default:
> + break;
> + }
> +}
> +
> +/* Starts tracking of client connections and disconnections. After
> + * initialization, PID and command line strings are determined and
> + * cached for each connected client. Call this before any clients have
> + * connected. */
> +void InitClientIds(ClientPtr server)
> +{
> + if (!dixRegisterPrivateKey(ClientIdsPrivKey, PRIVATE_CLIENT, 0))
> + FatalError("Can't register client IDs private.\n");
> +
> + if (!AddCallback(&ClientStateCallback, ClientIdsChanged, NULL))
> + FatalError("Can't track client IDs.\n");
> +
> + ReserveClientIds(server);
> +}
> +
> +/* Stops tracking clients. Call this after all clients have
> + * disconnected. */
> +void CloseClientIds(ClientPtr server)
> +{
> + if (!DeleteCallback(&ClientStateCallback, ClientIdsChanged, NULL))
> + LogMessage(X_ERROR, "Can't stop tracking client IDs.\n");
> +
> + DeleteCallbackList(&ClientIdsReservedCbs);
> + DeleteCallbackList(&ClientIdsReleasedCbs);
> +
> + ReleaseClientIds(server);
> +}
> +
> +/* Get cached PID of a client. Returns error (0) if called:
> + * - before ClientIdsReservedCbs notification
> + * - after ClientIdsReleasedCbs notification
> + * - for remote clients */
> +pid_t GetClientPid(ClientPtr client)
> +{
> + ClientIdsPrivatePtr priv = NULL;
> +
> + if (client == NullClient)
> + return 0;
> +
> + priv = GetClientIds(client);
> + if (!priv)
> + return 0;
> +
> + return priv->pid;
> +}
> +
> +/* Get cached command line string of a client. Returns error (NULL) if
> + * called:
> + * - before ClientIdsReservedCbs notification
> + * - after ClientIdsReleasedCbs notification
> + * - for remote clients */
> +const char *GetClientCmd(ClientPtr client)
> +{
> + ClientIdsPrivatePtr priv = NULL;
> +
> + if (client == NullClient)
> + return NULL;
> +
> + priv = GetClientIds(client);
> + if (!priv)
> + return NULL;
> +
> + return priv->cmd;
> +}
I liked a lot the description you put all over this file. If you want to be
even cooler, you could use doxygen style (take a look on dix/events.c for
reference).
> diff --git a/include/client.h b/include/client.h
> new file mode 100644
> index 0000000..40811c0
> --- /dev/null
> +++ b/include/client.h
> @@ -0,0 +1,51 @@
> +/*
> + * Copyright (c) 2010, Nokia Corporation.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef CLIENT_IDS_H
> +#define CLIENT_IDS_H
does anyone knows if these macros on the top of header files are useful for
some reason? I don't think so.
> +#include <sys/types.h>
> +
> +#include "dix.h"
> +
> +/* Client specific ID information. The corresponding client private
> + * will be NULL if PID is not available for the client. */
> +typedef struct
> +{
> + pid_t pid; /* always non-zero */
> + const char *cmd; /* NULL if not available */
nitpick: cmdline would be more suggestive.
> +} ClientIdsPrivateRec, *ClientIdsPrivatePtr;
> +
> +/* Initialize and clean up. */
> +void InitClientIds(ClientPtr server);
> +void CloseClientIds(ClientPtr server);
> +
> +/* Register to notifications. */
> +extern _X_EXPORT CallbackListPtr *GetClientIdsReservedCbs(void);
> +extern _X_EXPORT CallbackListPtr *GetClientIdsReleasedCbs(void);
> +
> +/* Query client IDs. */
> +extern _X_EXPORT ClientIdsPrivatePtr GetClientIds(ClientPtr client);
> +extern _X_EXPORT pid_t GetClientPid(ClientPtr client);
> +extern _X_EXPORT const char *GetClientCmd(ClientPtr client);
> +
> +#endif /* CLIENT_IDS_H */
In general the code looks pretty clean and nice. I'm afraid only that this is
one of the typical components that will bloat the server when someone doesn't
want actually use it, for instance not using/compiling with XRes or XACE.
Therefore we'll want want to conditionalize depending RES && XACE.
Anyway, you can consider for the whole series my review already:
Reviewed-by: Tiago Vignatti <tiago.vignatti at nokia.com>
Thanks,
Tiago
More information about the xorg-devel
mailing list