[Openswan dev] [design] tweaks to recent Pluto logging change (fwd)
D. Hugh Redelmeier
hugh at mimosa.com
Sat Jul 9 12:14:18 CEST 2005
I'm cleaning up some OLD files. In the process, I noticed that this
patch never seemed to make it into FreeS/WAN or Openswan. At first
glance, it seems to still be applicable (with tweaking).
---------- Forwarded message ----------
Date: Tue, 19 Aug 2003 11:51:12 -0400 (EDT)
From: D. Hugh Redelmeier <hugh at mimosa.com>
To: FreeS/WAN Design <design at lists.freeswan.org>
Subject: [design] tweaks to recent Pluto logging change
Michael recently made changes to Pluto to add per-IP logging. I've
made some changes that I'd like to explain.
- I changed several variables and functions to be file-static rather
than global. Reducing the scope of an identifier makes the program
simpler to understand.
- I simplified the logic by making peerpeer_list initializing and
finalizing unconditional.
- I simplified the logic of close_peerlog
- I eliminated two memory leaks in peerlogopen. A chunk of memory
was not freed on an error path. This shows the kind of risks
incurred by using the return statement in the middle of a function.
- I fixed a bug in peerlogopen. The loop that copied the IP address
turning : and . into / neglected to terminate the result.
- I made a small simplification to peerlogopen
Each of these was separately checked into CVS a while ago.
I'm still not confident of the code to create directories as needed.
It uses the dirname function which might return "."; if it does, I
don't know where that string lives (the dirname specification in SUS2
doesn't say). I think that this would violate the assumptions of the
code.
If the access call fails, and the errno isn't ENOENT, then the error
is logged only if c->log_file_err is TRUE. That is fine. But the
error return is only performed if c->log_file_err is TRUE -- surely
it should always be done.
All-in-all, the code is contorted and not clearly correct.
I think that a better structure reflects the idea that the directory
requirement is recursive: if we need a writeable directory, and it
does not exist, then we need a writeable directory in which to create it.
Bonus: no malloc is needed to hold the path.
I've written BUT NOT TESTED code in this form.
Michael:
Could you look at this? Do you agree that this is a better structure?
If so, could you test it? I don't know what you used to test the
existing per-IP logging feature.
I've been sitting on this for a while. In the mean time, you've
advanced log.c. The only overlapping change that I noticed is the
renaming of "peerlogopen" to "open_peerlog"
Is 0750 the right permission to request in the mkdir? The convention
I follow is to give wide permissions so that UMASK can specify the
user's policy. I'd have used 0777. I have kept the version you used.
Hugh Redelmeier
hugh at mimosa.com voice: +1 416 482-8253
--- log.c.REAL 2003-07-20 15:38:11.000000000 -0400
+++ log.c 2003-08-19 01:40:58.000000000 -0400
@@ -12,7 +12,7 @@
* or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* for more details.
*
- * RCSID $Id: log.c,v 1.71 2003/07/20 19:38:11 dhr Exp $
+ * RCSID $Id: log.c,v 1.70 2003/07/20 19:33:33 dhr Exp $
*/
#include <stdio.h>
@@ -244,7 +244,72 @@
}
}
-/* open the per-peer log */
+/* attempt to arrange a writeable parent directory for <path>
+ * Result indicates success. Failure will be logged.
+ *
+ * NOTE: this routine must not call our own logging facilities to report
+ * an error since those routines are not re-entrant and such a call
+ * would be recursive.
+ */
+static bool
+ensure_writeable_parent_directory(char *path)
+{
+ /* NOTE: a / in the first char of a path is not like any other.
+ * That is why the strchr starts at path + 1.
+ */
+ char *e = strrchr(path + 1, '/'); /* end of directory prefix */
+ bool happy = TRUE;
+
+ if (e != NULL)
+ {
+ /* path has an explicit directory prefix: deal with it */
+
+ /* Treat a run of slashes as one.
+ * Remember that a / in the first char is different.
+ */
+ while (e > path+1 && e[-1] == '/')
+ e--;
+
+ *e = '\0'; /* carve off dirname part of path */
+
+ if (access(path, W_OK) == 0)
+ {
+ /* mission accomplished, with no work */
+ }
+ else if (errno != ENOENT)
+ {
+ /* cannot write to this directory for some reason
+ * other than a missing directory
+ */
+ syslog(LOG_CRIT, "can not write to %s: %s", path, strerror(errno));
+ happy = FALSE;
+ }
+ else
+ {
+ /* missing directory: try to create one */
+ happy = ensure_writeable_parent_directory(path);
+ if (happy)
+ {
+ if (mkdir(path, 0750) != 0)
+ {
+ syslog(LOG_CRIT, "can not create dir %s: %s"
+ , path, strerror(errno));
+ happy = FALSE;
+ }
+ }
+ }
+
+ *e = '/'; /* restore path to original form */
+ }
+ return happy;
+}
+
+/* open the per-peer log
+ *
+ * NOTE: this routine must not call our own logging facilities to report
+ * an error since those routines are not re-entrant and such a call
+ * would be recursive.
+ */
static void
peerlogopen(struct connection *c)
{
@@ -288,61 +353,9 @@
/* now open the file, creating directories if necessary */
- { /* create the directory */
- char *dname;
- int bpl_len = strlen(base_perpeer_logdir);
- char *slashloc;
-
- dname = clone_str(c->log_file_name, "temp copy of file name");
- dname = dirname(dname);
-
- if (access(dname, W_OK) != 0)
- {
- if (errno != ENOENT)
- {
- if (c->log_file_err)
- {
- syslog(LOG_CRIT, "can not write to %s: %s"
- , dname, strerror(errno));
- c->log_file_err = TRUE;
- pfree(dname);
- return;
- }
- }
-
- /* directory does not exist, walk path creating dirs */
- /* start at base_perpeer_logdir */
- slashloc = dname + bpl_len;
- slashloc++; /* since, by construction there is a slash
- right there */
-
- while (*slashloc != '\0')
- {
- char saveslash;
-
- /* look for next slash */
- while (*slashloc != '\0' && *slashloc != '/') slashloc++;
-
- saveslash = *slashloc;
-
- *slashloc = '\0';
-
- if (mkdir(dname, 0750) != 0 && errno != EEXIST)
- {
- syslog(LOG_CRIT, "can not create dir %s: %s"
- , dname, strerror(errno));
- c->log_file_err = TRUE;
- pfree(dname);
- return;
- }
- syslog(LOG_DEBUG, "created new directory %s", dname);
- *slashloc = saveslash;
- slashloc++;
- }
- }
-
- pfree(dname);
- }
+ c->log_file_err = !ensure_writeable_parent_directory(c->log_file_name);
+ if (c->log_file_err)
+ return;
c->log_file = fopen(c->log_file_name, "a");
if (c->log_file == NULL)
================ end ================
-
FreeS/WAN design list.
https://mj2.freeswan.org/cgi-bin/mj_wwwusr/domain=mj2.freeswan.org to unsubscribe
More information about the Dev
mailing list