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