Page MenuHomeFreeBSD

Fix two clang 3.6.0 warnings in usr.sbin/syslogd
ClosedPublic

Authored by dim on Jan 29 2015, 7:34 AM.

Details

Summary

Fix two clang 3.6.0 warnings in usr.sbin/syslogd:

usr.sbin/syslogd/syslogd.c:1023:10: error: address of array 'f->f_prevline' will always evaluate to 'true' [-Werror,-Wpointer-bool-conversion]
                    f->f_prevline && !strcmp(msg, f->f_prevline) &&
                    ~~~^~~~~~~~~~
usr.sbin/syslogd/syslogd.c:1178:16: error: address of array 'f->f_prevline' will always evaluate to 'true' [-Werror,-Wpointer-bool-conversion]
        } else if (f->f_prevline) {
               ~~  ~~~^~~~~~~~~~

In both cases, the f_prevline field of struct filed is a char array, so it can never be null.

However, I think the intent was to check whether the string was empty, so check f->f_prevline[0] instead. (See also line 1056, where f->f_prevline[0] is explicitly set to zero.)

Test Plan

Compile and run.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

dim retitled this revision from to Fix two clang 3.6.0 warnings in usr.sbin/syslogd.
dim updated this object.
dim edited the test plan for this revision. (Show Details)
dim added reviewers: jilles, ian, ae.

I disagree with your analysis. f_prevline has always been an array dating back to r1553. In r174533, obrien mistakenly assumed it was a pointer to an array and added null-pointer checks, his commit message even says so, see

https://svnweb.freebsd.org/base?view=revision&revision=174533

So the right fix is just to remove the mistaken NULL checks. That commit message also says something about trying to free f_prevline before exit, I guess we better make sure it's not doing that too.

In D1716#3, @ian wrote:

I disagree with your analysis. f_prevline has always been an array dating back to r1553. In r174533, obrien mistakenly assumed it was a pointer to an array and added null-pointer checks, his commit message even says so, see

https://svnweb.freebsd.org/base?view=revision&revision=174533

So the right fix is just to remove the mistaken NULL checks. That commit message also says something about trying to free f_prevline before exit, I guess we better make sure it's not doing that too.

Yes, I guess he just copy/pasted a commit message from Juniper there, and maybe Juniper had their own version with a dynamically allocated f_prevline.

I don't see it trying to free f_prevline anywhere, just a free(wmsg) at the end of fprintlog().

dim edited edge metadata.

Remove unnecessary f_prevline checks.

So, is this now good to commit?

jilles edited edge metadata.

Looks good to me.

This revision is now accepted and ready to land.Feb 5 2015, 10:12 PM
dim updated this revision to Diff 3651.

Closed by commit rS278297 (authored by @dim).