#48805 Cleanup: Gcc 6.0 warnings
Closed: Fixed None Opened 2 years ago by firstyear.

When we swap to gcc 6 in fedora 24, this will trigger many more warnings in our code.

Additionally, with -Wextra we see lots of errors related to unused function parameters, mismatched sign checking in if statements.

I would like to sit down and go through all our gcc 6.0 compiler errors and either:

  • Fix the code.
  • Annonate the code to silence the warning.

No logic changes, mainly whitespace. I have run the test suite over this to ensure no accidental regressions were added.

Supplementary patch to fix many sign comparison warnings.
0001-Ticket-48805-Sign-comparison-checks.patch

Again, ran tests to ensure no regressions.

print_access_control_summary( char source, int ret_val, char clientDn,
{{{
662 662 struct codebook {
663 int code;
663 uint code;
664 664 char *text;
}}}
It's better to use the same type instead of the "default" type?
line 663 aclReasonCode_t code;


sha_pw_cmp (const char userpwd, const char dbpwd, unsigned int shaLen )
{{{
53 int hash_len; / must be a signed valued -- see below /
53 PRUint32 hash_len; / must be a signed valued -- see below /
}}}
But PRUint32 is unsigned... And it looks to me that pwdstorage_base64_decode_len does not return any negative value... so, we could remove the comment / must be a signed valued -- see below /?

Note: typedef unsigned int PRUint32;

static int printable_validate(
{{{
1330 1330 int rc = 0; / assume the value is valid /
1331 int i = 0;
1331 uint i = 0;
}}}
indentation???


do_add( Slapi_PBlock pb )
{{{
57 /
ber_len_t is a typedef uint. Can't be -1 /
57 58 ber_len_t len = -1;
}}}
Instead of -1, why don't you use LBER_ERROR?
{{{
519 /
Curiously, LBER_ERROR is uint, but set to -1 (0xffffffffU), even though ber_printf is int ... */
}}}
Casting to ber_tag_t makes -1 an unsigned int.
{{{

define LBER_ERROR ((ber_tag_t) -1)

}}}
So, (ber_tag_t) is doing the job here...

The same change could be applied to get_ldapmessage_controls_ext, get_filter_list( Connection conn, BerElement ber.

Please do not set 0 in get_filter_list. Set LBER_ERROR, instead.
{{{
387 ber_len_t len = -1;
387 / len, ber_len_t is uint. Can not be -1 /
388 ber_len_t len = 0;
}}}


{{{
int import_fifo_validate_capacity_or_expand(ImportJob job, int entrysize) {
}}}
Looking at the callers, size_t is being passed. Better change the function itself to this?
{{{
int import_fifo_validate_capacity_or_expand(ImportJob
job, size_t entrysize)
}}}


static int ldbm_config_db_cache_set(void arg, void value, char *errorbuf, int
{{{
1059 if ((int)val > li->li_dblayer_private->dblayer_cache_config) {
1060 delta = (int)val - li->li_dblayer_private->dblayer_cache_config;
}}}
Instead, we should declare val as "int"?
{{{
1045 size_t val = (size_t) ((uintptr_t)value);
}}}
Plus, this "val" is not used in this debug print?
{{{
1063 LDAPDebug1Arg(LDAP_DEBUG_ANY,"Error: db cachesize value is too large.\n", val);
}}}


perfctrs_as_entry( Slapi_Entry e, perfctrs_private priv, DB_ENV *db_env )
{{{
291 size_t i;
}}}
I'm curious.... Why this type is size_t and the similar variable in moddn_rename_children is "uint"? What's the difference? I see many changes to "size_t i;"...


chown_dir_files
{{{
205 * If both is set, this would set gid_t to -1 then the chown would fail
206 * because the check in slapd_chown_if_not_owner requires both uid && gid to pass.
}}}
??? Sorry, I still don't understand...

slapd_chown_if_not_owner(const char filename, uid_t uid, gid_t gid)
{{{
1159 /

1160 * The getpwnam man page shows that either the caller will get
1161 * a NULL pw struct, or it will be populated. So pw->pw_uid will
1162 * have a sane value. Given that uid_t is a ushort, checking
1163 * for -1 is pointless, we actually break fchown assumptions doing
1164 * it. Read the fchown man page.
1165 * Same for gid_t
1166 * if (((uid != -1) && (uid != statbuf.st_uid)) ||
1167 * ((gid != -1) && (gid != statbuf.st_gid)))
1168 */
1169 if ((uid != statbuf.st_uid) || (gid != statbuf.st_gid))
}}}
??? Isn't it a spec of slapd_chown_if_not_owner? I guess the author wanted to add an ability to change just uid or just gid without getting the current value of not changing id? With your change, the caller of slapd_chown_if_not_owner needs to get the current uid/gid to make it intact?


notes2str( unsigned int notes, char buf, size_t buflen )
{{{
1897 /
SLAPI_NOTEMAP_COUNT uses sizeof, size_t is unsigned. Was int */
1898 uint i;
}}}
indentation?

Applied all these recommendations, they are good points ....


perfctrs_as_entry( Slapi_Entry e, perfctrs_private priv, DB_ENV *db_env )
{{{
291 size_t i;
}}}
I'm curious.... Why this type is size_t and the similar variable in moddn_rename_children is "uint"? What's the difference? I see many changes to "size_t i;"...

Okay, the for statement here is:

{{{
for ( i = 0; i < SLAPI_LDBM_PERFCTR_AT_MAP_COUNT; ++i ) {
}}}

Here is the definition of SLAPI_LDBM_PERFCTR_AT_MAP_COUNT

{{{

define SLAPI_LDBM_PERFCTR_AT_MAP_COUNT \

(sizeof(perfctr_at_map) / sizeof(SlapiLDBMPerfctrATMap))

}}}

sizeof returns a size_t. So we make i a size_t so that we don't have a sign compare warning.

Same is true of many other cases for strlen, sizeof, etc. That's why there were lots of these changed.


??? Isn't it a spec of slapd_chown_if_not_owner? I guess the author wanted to add an ability to change just uid or just gid without getting the current value of not changing id? With your change, the caller of slapd_chown_if_not_owner needs to get the current uid/gid to make it intact?

I sat here thinking about this for a while, and rethinking I can see the original intent of the author now. I'm going to undo this "fix". I see what is happening here now. Thanks for the review.

The point of the original if was that if the -1 was there, it would trigger (uid != statbuf.st_uid), even if statbuf.st_uid was already correct. This statement is meant to avoid un-needed chown's, but I was thinking about it incorrectly. And the way it's structured, it will pass through the -1, to ignore correctly if the other meets the condition.

I'm rebuilding and testing now.

Supplementary patch to fix many sign comparison warnings, updated with Noriko's feedback.
0001-Ticket-48805-Sign-comparison-checks.2.patch

Thank you so much for cleaning up the code, William!!

There is one more place you'd better use LBER_ERROR for -1 and remove the comment (well, the comment is correct, tho. :). The rest looks good. You have my ack. Once control.c is fixed, please push to master which is for 1.3.6 and newer now (Thanks to, Mark)!
{{{
ldap/servers/slapd/control.c
diff --git a/ldap/servers/slapd/control.c b/ldap/servers/slapd/control.c
179 / ber_len_t is uint, cannot be -1 /
179 180 ber_len_t len = -1;
}}}

Updated with Mark's comment, commit 3e7d6d62f3b2889d0250cb0a3892982e42ada878 commit ea9d4bb162da15c39c61f410b214e4f5676bcbb5 Writing objects: 100% (112/112), 64.08 KiB | 0 bytes/s, done. Total 112 (delta 103), reused 0 (delta 0) To ssh://git.fedorahosted.org/git/389/ds.git 942c1af..3e7d6d6 master -> master

Fixing a compiler warning:
19e75b9..590799f master -> master
commit 590799f

Metadata Update from @firstyear:
- Issue assigned to firstyear
- Issue set to the milestone: 1.3.6 backlog

2 years ago

Login to comment on this ticket.

Metadata