[quagga-dev 3874] Re: Patch to quagga 0.99.2

Paul Jakma Paul.Jakma at Sun.COM
Wed Dec 7 08:35:00 GMT 2005


Hi Pedro,

On Mon, 5 Dec 2005, [ISO-8859-1] Pedro Andrés Aranda Gutiérrez wrote:

> Hi folks,
>
> after working for some time with quagga 0.99.1 as a route collector, I 
> was fed up with having to maintain scripts to create directories and 
> compress dumps. The attached patch integrates this functionality into 
> quagga.

Seems a good idea. Comments below:

Index: quagga/ChangeLog
===================================================================
RCS file: /var/cvsroot/quagga/ChangeLog,v
retrieving revision 1.139
diff -u -w -b -r1.139 ChangeLog
--- quagga/ChangeLog	26 Nov 2005 08:28:00 -0000	1.139
+++ quagga/ChangeLog	5 Dec 2005 12:01:30 -0000
@@ -1,3 +1,25 @@

A changelog entry as part of the patch, *excellent* :)

+2005-12-05 Pedro A. Aranda Gutierrez <paag at tid.es>
+
+	* (general) Nowadays, route collectors need external support
+	  scripts for many functions like creating the directory tree
+	  to hold the rib and update dumps and compressing said dumps
+	  to rationalise disk space. The proposed patch integrates
+	  these two functions.
+	* bgpd/bgp_main.c: added command line switch -z/--compress
+	  to pass the compressor which should be executed once the
+	  dump is finished

Should this perhaps be a run-time command instead? It seems a bit 
disjointed to have a command for turning dumps on and an argument for 
enabling compression.

+	* bgpd/bgp_main.c: added signal handler to catch the exit
+	  from the compression child process

Hmm, intereting. See some comments further below about handling 
children.

Index: quagga/bgpd/bgp_dump.c
===================================================================
RCS file: /var/cvsroot/quagga/bgpd/bgp_dump.c,v
retrieving revision 1.11
diff -u -w -b -r1.11 bgp_dump.c
--- quagga/bgpd/bgp_dump.c	9 Sep 2005 23:49:49 -0000	1.11
+++ quagga/bgpd/bgp_dump.c	5 Dec 2005 12:01:30 -0000
@@ -20,6 +20,29 @@

  #include <zebra.h>

+#ifndef ORIG

We shouldn't need an ORIG ifdef. Just remove them all, and any ORIG 
code.

+# include <string.h>

zebra.h already includes this.

+//
+// POSIX basename and dirname
+//
+# ifdef _GNU_SOURCE
+#  undef _GNU_SOURCE 
+#  include <libgen.h>
+#  define _GNU_SOURCE
+# else
+#  include <libgen.h>
+# endif

interesting, why must _GNU_SOURCE be undef'd?

+
+// this is for dir creation
+# include <sys/stat.h>
+# include <sys/types.h>

Already included in zebra.h

@@ -32,7 +55,6 @@
  #include "bgpd/bgp_route.h"
  #include "bgpd/bgp_attr.h"
  #include "bgpd/bgp_dump.h"
-

Leave that in, it's a pagination marker. ;)

@@ -71,6 +93,10 @@
    char *interval_str;

    struct thread *t_interval;
+
+#ifndef ORIG
+  char *compress;
+#endif
  };

ORIG define stuff should be removed. If this is useful, there's no 
reason for this to be a compile time thing.

  /* BGP packet dump output buffer. */
@@ -88,6 +114,118 @@
  /* Dump whole BGP table is very heavy process.  */
  struct thread *t_bgp_dump_routes;

+#ifndef ORIG
+/*
+ * Auxiliary  functions to automate the creation the
+ * directory structure  needed to store  the rib and
+ * update dumps and  to integrate the execution of a
+ * compression utility.
+ *
+ * These  functions  are always executed by external
+ * scripts in the route collector.  Unifying them in
+ * bgpd will reduce the route collector installation
+ * overhead and the possibility of install/config
+ * errors
+ */
+
+// define WITH_DIR_LEVEL mainly for debugging purposes
+#ifdef  WITH_DIR_LEVEL
+void zlog_debug_failure(int level,const char *fname)
+#else
+void zlog_debug_failure(const char *fname)
+#endif
+{
+  const char *cause;
+ 
+  switch (errno)
+    {
+    case EACCES: cause = "EACCES"; break;
+    case EEXIST: cause = "EEXIST"; break;
+    case ELOOP: cause = "ELOOP"; break;
+    case EMLINK: cause = "EMLINK"; break;
+    case ENAMETOOLONG: cause = "ENAMETOOLONG"; break;
+    case ENOENT: cause = "ENOENT"; break;
+    case ENOSPC: cause = "ENOSPC"; break;
+    case ENOTDIR: cause = "ENOTDIR"; break;
+    case EROFS : cause = "EROFS "; break;
+    default: cause = "UNKNOWN"; break; 
+    }
+#ifdef WITH_DIR_LEVEL
+  zlog_debug("(%2d) %s failure cause is %s",level,fname,cause);
+#else
+  zlog_debug("%s failure cause is %s",fname,cause);
+#endif
+}
+
+/*
+ * when level == 0, it assumes that the path
+ * includes the filename. It will create the
+ * full directory structure to insure that
+ * an open() will succeed
+ */

I'm curious, why do you want to create the directories? Do you have a 
hierarchy of directories? Does bgpd change this name? (I.e. is this 
creation of hierarchy of dirs a once-off, or something bgpd has to do 
regularly - if latter, i dont see where).

+#ifdef WITH_DIR_LEVEL
+int mkzebradirs(int level,char *path)
+#else
+int mkzebradirs(char *path)
+#endif
+{
+  char *path1 = strdup(path);

XSTRDUP (MTYPE_TMP, path); please, so it shows up in memory stats.

+  char *dir   = dirname(path1);
+
+  struct stat dirstat;
+ 
+  int  result = 0;
+
+#ifdef WITH_DIR_LEVEL
+  zlog_debug("(%2d) dir  = %s",level,dir);
+#else
+  zlog_debug("(mkzebradir) dir  = %s",dir);
+#endif

If the DIR_LEVEL stuff is useful, can we get rid of the ifdef? Given 
no-level is a special case of levels. Alternatively, get rid of the 
levels - the patch doesn't actually seem to make use of it anywhere.

+    dirumask=umask(0777 & (~DIR_PERMS));

Could LOGFILE_MASK possibly be reused here? E.g remove S_IRGRP from 
DIR_PERMS and:

      dirumask=umask(0777 & ~(DIR_PERMS | LOGFILE_MASK));

Ideally, make DIR_PERMS a configure option like LOGFILE_MASK, if we even 
need it (see comment above).

+int docommand(char *cmd)
+{
+  int childpid;
+  char buffer[2*MAXPATHLEN+2];
+
+  //
+  // copy to a static buffer before switching to child space
+  //
+  strcpy(buffer,cmd);

This doesn't seem necessary.

Trivial nitpick: Could we change the // comments to /* */ for 
consistency.

+  //
+  // fork() = 0 means child space
+  //
+  if ((childpid = fork()) != 0)
+    return childpid;
+  zlog_debug("execl(\"/bin/sh\",\"/bin/sh\",\"-c\",\"%s\",NULL);",buffer);
+  execl("/bin/sh","/bin/sh","-c",buffer,NULL);
+  exit (0);

buffer wasn't freed?

Also, I'm almost tempted to add a THREAD_CHILD thread type to 
lib/thread.c to handle children generically.

+}

+  //
+  // dump closed, if compression was requested, this is the moment
+  //
+  if (bgp_dump->compress != NULL)
+    {
+      int childpid = docommand(bgp_dump->compress);
+      if (childpid < 0) {
+	  zlog_warn ("execv: couldn't execute %s",bgp_dump->compress);
+	}
+      free(bgp_dump->compress);
+      bgp_dump->compress = NULL;
+    }
+  //
+  // prepare next compress process
+  // if dump file could be created
+  // and compression was requested
+  //
+  if (bgp_dump->fp != NULL)
+    { 
+      if ((compress != NULL)  && (compress[0] != '\x00'))

I have to ask, why would compress[0] be '\x00'? Also, why not use the 
bgp_dump->compress pointer to point to the commandline argument supplied 
compress, and leave that invariant?

The temporary pathname in this function can be left temporary.

+	{
+	  char compress_cmd[2*MAXPATHLEN+2];
+ 
+	  strcpy(compress_cmd,compress);
+	  strcat(compress_cmd," ");
+	  strcat(compress_cmd,realpath);

Use snprintf so that it is easy to verify compress_cmd[] isn't 
overflowed without having to hunt for realpath.

+  if (bgp_dump->compress)
+    free(bgp_dump->compress);
+  bgp_dump->compress = NULL;

I'm curious why compress is freed each time.

+#ifndef ORIG
+  { "compress",    required_argument, NULL, 'z'},
+#endif

Probably would be better off a runtime VTY command.

+#ifndef ORIG
+/* Manually specified compressor program */
+char *compress = NULL;
+#endif

Is this second pointer to the compressor programme required?

+#ifndef ORIG
+/* SIGCHLD handler cited and described in all books*/
+void sigchld()
+{
+  int stat;
+
+  zlog_debug("Entered sigchld()");
+  while(waitpid(-1, &stat, WNOHANG) > 0);

this should be (pid_t)-1 I think, to ensure the compiler generates -1 of 
appropriate width.

regards,

--paulj


More information about the Quagga-dev mailing list