On 02/27/2014 01:15 AM, Nathan Scott wrote:
Hi Dave,
Here's that code review...
Thanks!
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.
OK. Fixed in commit 138d204590757960879499918315e725aa4fb920
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.
Looks like the mkdir_r function there does a bit more than the new
__pmMakePath. It also sets the group for each path component. I left
this alone.
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).
I was just following the lead of __pmGroupNameFromID() and
__pmUsernameFromID(), however, I can see that returning "unknown" here
could lead to successful placement of data in the directory "unknown"
where we really do want an error. Fixed in commit
d2e87db1c006c98067e05fac25c354dec79923c3.
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.
The purpose was to compute these only once. If we do this, then we need
two buffers, one to hold the <pid> version of the socket name and one to
hold the "primary" version. I see now that this is flawed, because pmlc
may want to contact pmloggers with different pids. Passing in the buffer
would then seem to be the right way to go. It will also remove the need
for the PM_LOCK.
Fixed in commit 9aabf24a17e71cff1c221a184d875b8f0ea0d76a.
+/*
+ * Common function for attemmpting connections to pmlogger.
+ */
+static int
+connectLogger (int fd, __pmSockAddr *myAddr)
whitespace oddity.
Fixed in commit fb5b05c1b1dabc4185263b87fd96f766621d2a62
+ /* Attept the connection. */
typo.
Fixed in commit fb5b05c1b1dabc4185263b87fd96f766621d2a62
+ 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;
+ }
+ }
Dead code removed in commit fb5b05c1b1dabc4185263b87fd96f766621d2a62
+ * Attemmpt connection to pmlogger via a local socket.
typo.
Fixed in commit fb5b05c1b1dabc4185263b87fd96f766621d2a62
+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.
Fixed in commit e5b3075f4617597e22f5ba88fb0078e51b4c8fa4
+ /* Attempt to connect */
+ sts = connectLogger(fd, myAddr);
+ __pmSockAddrFree(myAddr);
+
+ if (sts < 0)
+ fd = sts;
and there.
Fixed in commit e5b3075f4617597e22f5ba88fb0078e51b4c8fa4
+__pmConnectLogger(const char *connectionSpec, int *pid, int *port)
+ [...]
+ if (fd < 0) {
+ if (prefix_len == 5) /* "unix: */
double-quote oddity.
Fixed in commit fb5b05c1b1dabc4185263b87fd96f766621d2a62
+ 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.
Line was removed by my changes ...
#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.
... but reintroduced here. Fixed in commit
fb5b05c1b1dabc4185263b87fd96f766621d2a62 and
5046073d44836ff02af15630ba39e06cd4a3d0a5
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?
The problem would be how big to make it. The leak was very easy to fix,
so I left it as a heap object. Fixed in
b01572d581dea4adffd839d8e339656349e63b3d.
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?
Yep -- done in commits 138d204590757960879499918315e725aa4fb920 and
707b499d8fd9867be47bfae2c0d2f2e2b226aa33 (botched the new new the first
time).
+ 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).
Yeah. I don't know what I was thinking when I did that.
Fixed in commit fdbc97d3903c33f96580808fcef8a77376db4dc5
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? ;)
Say it 3 times fast ;)
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?)
Convention for other functions in the file for out of memory seems to be
to call __pmNoMem(). I've changed it to that. Fixed in commit
3b2eb2e0fb19e48b4dcca7782bf656afa12966bd
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...?
I've tracked this down and will address it in a separate email.
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.
Fixed in commit 9349d8641aa681b493c616f43258860965e48e90
+ return;
}
That final return is redundant.
and removed in commit fb5b05c1b1dabc4185263b87fd96f766621d2a62.
- free(hostName);
}
+ if (hostName != NULL)
+ free(hostName);
Good catch!
cheers.
--
Nathan
|