Hi Dave,
Here's that code review...
> diff --git a/src/libpcp/src/exports b/src/libpcp/src/exports
> index 9b17688..36675b8 100644
> --- a/src/libpcp/src/exports
> +++ b/src/libpcp/src/exports
> @@ -228,6 +228,8 @@ PCP_3.0 {
> __pmLogChkLabel;
> __pmLogClose;
> __pmLogCreate;
> + __pmLogLocalSocketDefault;
> + __pmLogLocalSocketUser;
> __pmLogFetch;
> __pmLogFetchInterp;
> __pmLogFindLocalPorts;
> @@ -259,6 +261,7 @@ PCP_3.0 {
> __pmMapErrno;
> __pmMemoryMap;
> __pmMemoryUnmap;
> + __pmMkdir;
> __pmMktime;
> __pmMultiThreaded;
> __pmNativeConfig;
Hmm, we can't just stick these slap bang in the middle here -
needs a PCP_3.2 exports section, which inherits from PCP_3.1
now, doesn't it? Stan's time API work will piggyback on this
new section, as will the common getopts goo I hope.
> diff --git a/src/include/pcp/impl.h b/src/include/pcp/impl.h
> index df78f1d..988de0f 100644
> --- a/src/include/pcp/impl.h
> +++ b/src/include/pcp/impl.h
> @@ -1262,6 +1265,7 @@ extern void __pmConfig(__pmConfigCallback);
> extern char *__pmNativePath(char *);
> extern int __pmAbsolutePath(char *);
> extern int __pmPathSeparator(void);
> +extern int __pmMkdir(const char *, mode_t);
BTW, looks like pmpost.c could make use of this, removing the
mkdir_r (equivalent code) there. Also, see below re naming.
> diff --git a/src/libpcp/src/accounts.c b/src/libpcp/src/accounts.c
> index 89b967d..8c2eb7b 100644
> --- a/src/libpcp/src/accounts.c
> +++ b/src/libpcp/src/accounts.c
>
> + result = getpwuid(uid);
> + snprintf(buf, size, "%s", result ? result->pw_dir : "unknown");
Putting "unknown" as the homedir path seems dodgey - looks like
we need some error handling here (return NULL, deal with it in
the callers).
> diff --git a/src/libpcp/src/logconnect.c b/src/libpcp/src/logconnect.c
> index 547eb9a..4e282ca 100644
> --- a/src/libpcp/src/logconnect.c
> +++ b/src/libpcp/src/logconnect.c
> @@ -62,11 +62,185 @@ __pmLoggerTimeout(void)
> }
>
> /*
> - * expect one of pid or port to be 0 ... if port is 0, use
> - * hostname+pid to find port, assuming pmcd is running there
> + * Return the path to the default PMLOGGER local unix domain socket.
> + * Returns a pointer to a static buffer which can be used directly.
> + * Return the path regardless of whether unix domain sockets are
> + * supported by our build. Other functions can then print reasonable
> + * messages if an attempt is made to use one.
> + */
> +const char *
> +__pmLogLocalSocketDefault(int pid)
> +{
> + static char pmlogger_socket[MAXPATHLEN];
> + static char pmlogger_socket_primary[MAXPATHLEN];
Do these need to be here, static? Seems a buffer (and size) could
be passed in. Also, not really following why we need both buffers
separately. A comment would help, but can they be pushed out to
the caller(s) instead.
> +/*
> + * Common function for attemmpting connections to pmlogger.
> + */
> +static int
> +connectLogger (int fd, __pmSockAddr *myAddr)
whitespace oddity.
> + /* Attept the connection. */
typo.
> + sts = 0;
dead code? (above line, cos...)
> + if ((rc = __pmSelectRead(fd+1, &rfds, pstv)) == 1) {
> + sts = __pmConnectCheckError(fd);
> + }
> + else if (rc == 0) {
> + sts = ETIMEDOUT;
> + }
> + else {
> + sts = (rc < 0) ? neterror() : EINVAL;
> + }
> + }
> + * Attemmpt connection to pmlogger via a local socket.
typo.
> +static int
> +connectLoggerLocal(const char *local_socket)
> +{
> +#if defined(HAVE_STRUCT_SOCKADDR_UN)
> + char socket_path[MAXPATHLEN];
> + int fd;
> + int sts;
> + __pmSockAddr *myAddr;
> +
> + /* Create a socket */
> + fd = __pmCreateUnixSocket();
> + if (fd < 0)
> + return -ECONNREFUSED;
> +
> + /* Set up the socket address. */
> + myAddr = __pmSockAddrAlloc();
> + if (myAddr == NULL) {
> + __pmNotifyErr(LOG_ERR, "__pmConnectLogger: out of memory\n");
> + return -ENOMEM;
fd is leaked here.
> + /* Attempt to connect */
> + sts = connectLogger(fd, myAddr);
> + __pmSockAddrFree(myAddr);
> +
> + if (sts < 0)
> + fd = sts;
and there.
> +__pmConnectLogger(const char *connectionSpec, int *pid, int *port)
> + [...]
> + if (fd < 0) {
> + if (prefix_len == 5) /* "unix: */
double-quote oddity.
> + if (*pid == PM_LOG_ALL_PIDS) {
> #ifdef PCP_DEBUG
> if (pmDebug & DBG_TRACE_CONTEXT)
> - fprintf(stderr, "__pmConnectLogger: __pmLogFindPort -> 1,
> cannot contact pmcd\n");
pmcd? pmlogger.
> #ifdef PCP_DEBUG
> - if (pmDebug & DBG_TRACE_CONTEXT)
> - fprintf(stderr, "__pmConnectLogger: gethostbyname: %s\n",
> - hoststrerror());
> + if (pmDebug & DBG_TRACE_CONTEXT)
> + fprintf(stderr, "__pmConnectLogger: __pmLogFindPort -> 1,
> cannot contact pmcd\n");
ditto.
> diff --git a/src/libpcp/src/logportmap.c b/src/libpcp/src/logportmap.c
> index 22b1798..1cf463b 100644
> --- a/src/libpcp/src/logportmap.c
> +++ b/src/libpcp/src/logportmap.c
> @@ -365,6 +358,7 @@ int
> __pmLogFindPort(const char *host, int pid, __pmLogPort **lpp)
> {
> int ctx, oldctx;
> + char *ctxhost;
> int sts, numval;
> int i, j;
> int findone = pid != PM_LOG_ALL_PIDS;
> @@ -386,9 +380,21 @@ __pmLogFindPort(const char *host, int pid, __pmLogPort
> **lpp)
> return localcon;
>
> /* note: there may not be a current context */
> + ctx = 0;
> oldctx = pmWhichContext();
>
> - if ((ctx = pmNewContext(PM_CONTEXT_HOST, host)) < 0)
> + /*
> + * Enclose ctxhost in [] in case it is an ipv6 address. This prevents
> + * the first colon from being taken as a port separator by pmNewContext
> + * and does no harm otherwise.
> + */
> + ctxhost = malloc(strlen(host) + 2 + 1);
> + if (ctxhost == NULL) {
> + sts = -ENOMEM;
> + goto ctxErr;
> + }
> + sprintf(ctxhost, "[%s]", host);
> + if ((ctx = pmNewContext(PM_CONTEXT_HOST, ctxhost)) < 0)
> return ctx;
Leaks ctxhost here.
> if ((sts = pmLookupName(1, namelist, &pmid)) < 0)
> goto ctxErr;
... there ...
> @@ -434,6 +440,7 @@ resErr:
> ctxErr:
> if (oldctx >= 0)
> pmUseContext(oldctx);
> - pmDestroyContext(ctx);
> + if (ctx >= 0)
> + pmDestroyContext(ctx);
> return sts;
> }
... and everywhere. Should ctxhost be an on-stack buffer instead?
> diff --git a/src/libpcp/src/secureconnect.c b/src/libpcp/src/secureconnect.c
> index 396727e..b44851c 100644
> --- a/src/libpcp/src/secureconnect.c
> +++ b/src/libpcp/src/secureconnect.c
> @@ -383,7 +358,7 @@ __pmInitCertificates(void)
> * not using secure connections (initially everyone) don't
> * have to diagnose / put up with spurious errors.
> */
> - if (mkpath(dbpath(nssdb, sizeof(nssdb), "sql:"), 0700) < 0)
> + if (__pmMkdir(dbpath(nssdb, sizeof(nssdb), "sql:"), 0700) < 0)
Hmm, actually, mkpath was quite a descriptive name ... could
we go with __pmMakePath for this new interface instead?
> + if (address->sockaddr.raw.family == PR_AF_INET ||
> + address->sockaddr.raw.family == PR_AF_INET6) {
> + prStatus = PR_GetHostByAddr(&address->sockaddr, &buffer[0],
> sizeof(buffer), &he);
> #ifdef PCP_DEBUG
> - if (pmDebug & DBG_TRACE_DESPERATE) {
> - if (prStatus != PR_SUCCESS) {
> - fprintf(stderr, "%s:PR_GetHostByAddr(%s) returns %d (%s)\n",
> __FILE__, __pmSockAddrToString(address), PR_GetError(),
> PR_ErrorToString(PR_GetError(), PR_LANGUAGE_I_DEFAULT));
> - }
> - }
> + if (pmDebug & DBG_TRACE_DESPERATE) {
> + if (prStatus != PR_SUCCESS) {
> + fprintf(stderr, "%s:PR_GetHostByAddr(%s) returns %d (%s)\n",
> __FILE__, __pmSockAddrToString(address), PR_GetError(),
> PR_ErrorToString(PR_GetError(), PR_LANGUAGE_I_DEFAULT));
> + }
> + }
> #endif
> + }
> + else if (address->sockaddr.raw.family == PR_AF_LOCAL)
> + return strdup(address->sockaddr.local.path);
> + else {
> + __pmNotifyErr(LOG_ERR,
> + "%s:__pmGetNameInfo: Invalid address family: %d\n",
> __FILE__,
> + address->sockaddr.raw.family);
> + return NULL;
> + }
> +
> name = (prStatus == PR_SUCCESS ? strdup(he.h_name) : NULL);
> return name;
This code looks slightly wierd to me now, because we conditionally
setup "he" early on, then we have a chunk of code that operates in
a completely different way (PR_AF_LOCAL), and then we come back to
"he" at the end. It would read/flow a little better if we did the
af_unix family first, and then the inet/ipv6 - or, perhaps moving
the strdup(he.h_name) & prStatus test inside the inet/ipv6 branch
would do the trick (could do the Notify without an "else" then).
> diff --git a/src/libpcp/src/util.c b/src/libpcp/src/util.c
> index e92d22b..5d575b1 100644
> --- a/src/libpcp/src/util.c
> +++ b/src/libpcp/src/util.c
> @@ -1475,6 +1475,34 @@ dirname(char *name)
> }
> #endif /* HAVE_DIRNAME */
>
> +/*
> + * Create a directory, including all of its path components.
> + */
> +int
> +__pmMkdir(const char *dir, mode_t mode)
__pmMakePath ... just rolls off the tongue, doesn't it? ;)
> diff --git a/src/pmlc/actions.c b/src/pmlc/actions.c
> index 45c60d2..77088eb 100644
> --- a/src/pmlc/actions.c
> +++ b/src/pmlc/actions.c
> @@ -91,12 +92,17 @@ ConnectPMCD(void)
> * if pmcd host is "localhost"-alike then use hostname that
> * was used to contact pmlogger, as from here (where pmlc is
> * running) "localhost" is likely to connect us to the wrong
> - * pmcd or no pmcd at all
> + * pmcd or no pmcd at all.
What's the "point"? Ahahahah, I crack myself up sometimes.
> */
> srchost = strdup(lasthost);
> }
> else
> srchost = strdup(lsp->ls_fqdn);
> +
> + if (srchost == NULL) {
> + sts = -ENOMEM;
> + goto done;
> + }
Looks like error reporting is being done within the above routine?
(i.e. do we need a fprintf here, like the other error cases?)
> diff --git a/src/pmlogconf/pmlogconf.sh b/src/pmlogconf/pmlogconf.sh
> index db98da8..7e10e5d 100755
> --- a/src/pmlogconf/pmlogconf.sh
> +++ b/src/pmlogconf/pmlogconf.sh
> @@ -11,6 +11,7 @@
> # when the group was added to the configuration file
> # delta delta argument for pmlogger "logging ... on delta" clause
> #
> +# Copyright (c) 2014 Red Hat.
> # Copyright (c) 1998,2003 Silicon Graphics, Inc. All Rights Reserved.
> #
> # This program is free software; you can redistribute it and/or modify it
> @@ -485,8 +486,9 @@ End-of-File
> #
>
> [access]
> -disallow * : all;
> -allow localhost : enquire;
> +disallow .* : all;
> +disallow :* : all;
> +allow local:* : enquire;
> End-of-File
Hmmm. When I run QA on this code, after an upgrade (so, without
the above change), test 023 hangs. I wonder if the above change
is being assumed to be in place...?
> diff --git a/src/pmlogger/src/ports.c b/src/pmlogger/src/ports.c
> index 1f867cd..b3320ee 100644
> --- a/src/pmlogger/src/ports.c
> +++ b/src/pmlogger/src/ports.c
> [...]
> +GetPorts(char *file)
> [...]
> + /* Now listen on the new socket. */
> + if (sts >= 0) {
> + sts = __pmListen(fd, 5); /* Max. of 5 pending connection
> requests */
> + if (sts == -1) {
> + fprintf(stderr, "__pmListen: %s\n", netstrerror());
Do we leak fd here?
> + }
> + else {
> + ctlfds[ctlix] = fd;
> + ++socketsCreated;
> + }
> }
(back to top of loop) ... why yes, yes we do.
> + return;
> }
That final return is redundant.
> - free(hostName);
> }
> + if (hostName != NULL)
> + free(hostName);
Good catch!
cheers.
--
Nathan
|