"Todd C. Miller" <todd.mil...@courtesan.com> writes:
> On Fri, 14 Jul 2017 22:59:58 +0200, Jeremie Courreges-Anglas wrote:
>
>> "Todd C. Miller" <todd.mil...@courtesan.com> writes:
>>
>> > On Fri, 14 Jul 2017 22:15:49 +0200, Jeremie Courreges-Anglas wrote:
>> >
>> >> the proposal in
>> >>
>> >> https://marc.info/?l=openbsd-misc&m=150001966325010&w=2
>> >>
>> >> made me look at newsyslog and its manually linked lists. Here's a stab
>> >> at using TAILQs instead.
>> >
>> > Oh good, now I don't have to do it ;-)
>>
>> ;)
>>
>> > I have a diff to make change errx() into warnx() + continue.
>>
>> I have a similar diff which is a bit intrusive, as it tries to properly
>> free resources allocated in parse_file(). Maybe show us yours first?
>
> This doesn't attempt to free any resources.
Wouldn't it be better if we at least tried to properly free what was
allocated in parse_file()?
> One thing I wanted to
> do was to make parse_file() return an error value when there was a
> parse error so the exit code could be non-zero.
That would be better, a valid use case is checking the exit status of
newsyslog -n to detect errors.
Also, should we really ignore errors for everything, or only for
getpwnam/getgrnam(3) errors? The latter appears safer to me.
> - todd
>
> Index: usr.bin/newsyslog/newsyslog.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/newsyslog/newsyslog.c,v
> retrieving revision 1.103
> diff -u -p -u -r1.103 newsyslog.c
> --- usr.bin/newsyslog/newsyslog.c 14 Jul 2017 20:51:17 -0000 1.103
> +++ usr.bin/newsyslog/newsyslog.c 14 Jul 2017 21:11:29 -0000
> @@ -510,9 +510,11 @@ parse_file(struct entrylist *list, int *
> *group++ = '\0';
> if (*q) {
> if (!(isnumberstr(q))) {
> - if ((pwd = getpwnam(q)) == NULL)
> - errx(1, "%s:%d: unknown user:
> %s",
> + if ((pwd = getpwnam(q)) == NULL) {
> + warnx("%s:%d: unknown user: %s",
> conf, lineno, q);
> + continue;
> + }
> working->uid = pwd->pw_uid;
> } else
> working->uid = atoi(q);
> @@ -522,9 +524,11 @@ parse_file(struct entrylist *list, int *
> q = group;
> if (*q) {
> if (!(isnumberstr(q))) {
> - if ((grp = getgrnam(q)) == NULL)
> - errx(1, "%s:%d: unknown group:
> %s",
> + if ((grp = getgrnam(q)) == NULL) {
> + warnx("%s:%d: unknown group:
> %s",
> conf, lineno, q);
> + continue;
> + }
> working->gid = grp->gr_gid;
> } else
> working->gid = atoi(q);
> @@ -539,15 +543,19 @@ parse_file(struct entrylist *list, int *
> }
>
> l = strtol(q, &ep, 8);
> - if (*ep != '\0' || l < 0 || l > ALLPERMS)
> - errx(1, "%s:%d: bad permissions: %s", conf, lineno, q);
> + if (*ep != '\0' || l < 0 || l > ALLPERMS) {
> + warnx("%s:%d: bad permissions: %s", conf, lineno, q);
> + continue;
> + }
> working->permissions = (mode_t)l;
>
> q = parse = missing_field(sob(++parse), errline, lineno);
> *(parse = son(parse)) = '\0';
> l = strtol(q, &ep, 10);
> - if (*ep != '\0' || l < 0 || l >= INT_MAX)
> - errx(1, "%s:%d: bad number: %s", conf, lineno, q);
> + if (*ep != '\0' || l < 0 || l >= INT_MAX) {
> + warnx("%s:%d: bad number: %s", conf, lineno, q);
> + continue;
> + }
> working->numlogs = (int)l;
>
> q = parse = missing_field(sob(++parse), errline, lineno);
> @@ -561,23 +569,29 @@ parse_file(struct entrylist *list, int *
> q = parse = missing_field(sob(++parse), errline, lineno);
> *(parse = son(parse)) = '\0';
> l = strtol(q, &ep, 10);
> - if (l < 0 || l >= INT_MAX)
> - errx(1, "%s:%d: interval out of range: %s", conf,
> + if (l < 0 || l >= INT_MAX) {
> + warnx("%s:%d: interval out of range: %s", conf,
> lineno, q);
> + continue;
> + }
> working->hours = (int)l;
> switch (*ep) {
> case '\0':
> break;
> case '@':
> working->trim_at = parse8601(ep + 1);
> - if (working->trim_at == (time_t) - 1)
> - errx(1, "%s:%d: bad time: %s", conf, lineno, q);
> + if (working->trim_at == (time_t) - 1) {
> + warnx("%s:%d: bad time: %s", conf, lineno, q);
> + continue;
> + }
> working->flags |= CE_TRIMAT;
> break;
> case '$':
> working->trim_at = parseDWM(ep + 1);
> - if (working->trim_at == (time_t) - 1)
> - errx(1, "%s:%d: bad time: %s", conf, lineno, q);
> + if (working->trim_at == (time_t) - 1) {
> + warnx("%s:%d: bad time: %s", conf, lineno, q);
> + continue;
> + }
> working->flags |= CE_TRIMAT;
> break;
> case '*':
> @@ -585,8 +599,8 @@ parse_file(struct entrylist *list, int *
> break;
> /* FALLTHROUGH */
> default:
> - errx(1, "%s:%d: bad interval/at: %s", conf, lineno, q);
> - break;
> + warnx("%s:%d: bad interval/at: %s", conf, lineno, q);
> + continue;
> }
>
> q = sob(++parse); /* Optional field */
> @@ -612,9 +626,9 @@ parse_file(struct entrylist *list, int *
> working->flags |= CE_FOLLOW;
> break;
> default:
> - errx(1, "%s:%d: illegal flag: `%c'",
> + warnx("%s:%d: illegal flag: `%c'",
> conf, lineno, *q);
> - break;
> + continue;
> }
> q++;
> }
> @@ -631,9 +645,11 @@ parse_file(struct entrylist *list, int *
> break;
> if (*q == '/') {
> *(parse = son(parse)) = '\0';
> - if (strlen(q) >= PATH_MAX)
> - errx(1, "%s:%d: pathname too long: %s",
> + if (strlen(q) >= PATH_MAX) {
> + warnx("%s:%d: pathname too long: %s",
> conf, lineno, q);
> + continue;
> + }
> working->pidfile = strdup(q);
> if (working->pidfile == NULL)
> err(1, "strdup");
> @@ -656,23 +672,29 @@ parse_file(struct entrylist *list, int *
> break;
> }
> }
> - if (i == NSIG)
> - errx(1, "%s:%d: unknown signal: %s",
> + if (i == NSIG) {
> + warnx("%s:%d: unknown signal: %s",
> conf, lineno, q);
> + continue;
> + }
> } else if (working->flags & CE_MONITOR) {
> *(parse = son(parse)) = '\0';
> working->whom = strdup(q);
> if (working->whom == NULL)
> err(1, "strdup");
> - } else
> - errx(1, "%s:%d: unrecognized field: %s",
> + } else {
> + warnx("%s:%d: unrecognized field: %s",
> conf, lineno, q);
> + continue;
> + }
> }
> free(errline);
>
> - if ((working->flags & CE_MONITOR) && working->whom == NULL)
> - errx(1, "%s:%d: missing monitor notification field",
> + if ((working->flags & CE_MONITOR) && working->whom == NULL) {
> + warnx("%s:%d: missing monitor notification field",
> conf, lineno);
> + continue;
> + }
>
> /* If there is an arcdir, set working->backdir. */
> if (arcdir != NULL && working->logbase != NULL) {
> @@ -701,15 +723,19 @@ parse_file(struct entrylist *list, int *
> if (working->backdir != NULL) {
> if (snprintf(line, sizeof(line), "%s/%s.%d%s",
> working->backdir, working->logbase,
> - working->numlogs, COMPRESS_POSTFIX) >= PATH_MAX)
> - errx(1, "%s:%d: pathname too long: %s",
> + working->numlogs, COMPRESS_POSTFIX) >= PATH_MAX) {
> + warnx("%s:%d: pathname too long: %s",
> conf, lineno, q);
> + continue;
> + }
> } else {
> if (snprintf(line, sizeof(line), "%s.%d%s",
> working->log, working->numlogs, COMPRESS_POSTFIX)
> - >= PATH_MAX)
> - errx(1, "%s:%d: pathname too long: %s",
> + >= PATH_MAX) {
> + warnx("%s:%d: pathname too long: %s",
> conf, lineno, working->log);
> + continue;
> + }
> }
> TAILQ_INSERT_TAIL(list, working, next);
> (*nentries)++;
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
