syslogd config patch

Denys Vlasenko vda.linux at googlemail.com
Wed Dec 29 05:24:36 UTC 2010


On Thursday 25 November 2010 17:13, Sergey Naumov wrote:
> New version of syslog.conf support is attached.
> 
> 1. Config entries now organized as linked list so we don't need to go
> through config twice.
> 2. Functions from parse_config now handle about 1/2 of parsing
> (comments, blanks, tokenizing by blanks).
> 3. Some major bugs removed.

+#define CFG_MAX_ENTRIES 32

This constant is unused.



+static const CODE* find_fac_prio(int val, char *name, const CODE* c_set)
+{
+       if (!c_set) return NULL;

c_set is never NULL.



+#if ENABLE_FEATURE_SYSLOGD_CFG
+#define cfgbuf bb_common_bufsiz1
+#define CFGDELIM " \t"

One-use #define. What's the point?




+static void syslogdcfg_parse(const char * file)
+{
+       char *t;
+       const char * f = file;
+<----->
+       // use double pointer to avoid checking whether head is initialized
+       logFile_t **ppLogF = &(G.pLogF);
+       logFile_t *pLogF;
+       const CODE *code;
+<----->
+       // the first token is facility.priority
+       // the second one is file name
+       // the third one have to be NULL
+       char *tok[3];
+       parser_t *parser;
+<----->
+       if (f == NULL) f = "/etc/syslog.conf";
+<----->
+       parser = config_open(f);
+       if (parser) {
+               while (config_read(parser, tok, 3, 2,
+                                      "#"CFGDELIM, PARSE_NORMAL | PARSE_MIN_DIE)) {
+                       if (tok[2]) { t = tok[2]; goto cfgerr; }
+                       if (!(t = strchr(tok[0], '.'))) { t = tok[0]; goto cfgerr; }
+                       *t++ = '\0'; // separate facility from priority
+                       *ppLogF = xmalloc(sizeof(logFile_t));
+                       pLogF = *ppLogF;
+                       memset(pLogF, 0, sizeof(logFile_t));
+                       pLogF->logFD = -1;
+                       if (*t == '!' && *(t+1) == '=') {
+                               pLogF->flags |= FL_PNEQ, t += 2;
+                       } else if (*t == '=') {
+                               pLogF->flags |= FL_PEQ, t++;
+                       }
+                       // priority
+                       if (*t == '*') pLogF->flags |= FL_PANY;
+                       else {
+                               if ((code = find_fac_prio(-1, t, prioritynames))) {
+                                       pLogF->pri = code->c_val;
+                               } else goto cfgerr;
+                       }
+                       //facility
+                       t = tok[0];
+                       if (*t == '*') pLogF->flags |= FL_FANY;
+                       else {
+                               if ((code = find_fac_prio(-1, t, facilitynames))) {
+                                       pLogF->pri |= code->c_val;
+                               } else goto cfgerr;
+                       }
+                       pLogF->logFilePath = xstrdup(tok[1]);
+                       ppLogF = &(pLogF->next);
+                       continue;
+               cfgerr:
+                       bb_error_msg_and_die("bad line %d: wrong token \"%s\".",
+                                            parser->lineno, t);
+               }
+               config_close(parser);
+       } else if (file) {
+       // -f /some/config/file was explicitly used
+               bb_error_msg_and_die("%s: not found.", file);
+       }
+       // else -f was not specified and no default config found => NOP
+}
+#endif

This code may be correct, but it is not very readable:

* assignments inside if()

* multi-statement lines

* not very carefully placed comments with sligntly broken English

* some parts can be expressed clearer:
  Example 1:
  +                               if ((code = find_fac_prio(-1, t, prioritynames))) {
  +                                       pLogF->pri = code->c_val;
  +                               } else goto cfgerr;
  can be
  +                               code = find_fac_prio(-1, t, prioritynames);
  +                               if (!code)
  +                                       goto cfgerr;
  +                               pLogF->pri = code->c_val;
  Example 2:
  +       const char * f = file;
  +       if (f == NULL) f = "/etc/syslog.conf";
  +       parser = config_open(f);
  +       if (parser) {
  +               huge block
  +               huge block
  +               huge block
  +               huge block
  +               config_close(parser);
  +       } else if (file) {
  +       // -f /some/config/file was explicitly used
  +               bb_error_msg_and_die("%s: not found.", file);
  +       }
  +       // else -f was not specified and no default config found => NOP
  can be
  +       parser = config_open2(file ? file : "/etc/syslog.conf", file ? xfopen_for_read : fopen_or_warn_stdin);
  +       if (!parser)
  +               return; /* didn't find default /etc/syslog.conf */
  +       huge block
  +       huge block
  +       huge block
  +       huge block
  +       config_close(parser);

* stray trailing tabs



+                       if (! (pLogF->flags & FL_FANY))
+                       {
+                               lfac = (LOG_FAC(pLogF->pri) << 3);
+                               if (lfac != mfac) continue; //facility does not match
+                       }

{} style doesn't match the rest of the source



+<-----><------><------>
+                       if (pLogF->flags & FL_PANY) goto match;
+                       else {

else after goto isn't needed, makes code harder to read.


-- 
vda


More information about the busybox mailing list