ZOOKEEPER-3654: Incorrect *_CFLAGS handling in Automake

The `Makefile.am` distributed with the C client defines some per-target `*_CFLAGS` and `*_CXXFLAGS` variables.  These however, do not reference `AM_CFLAGS` (resp. AM_CXXFLAGS`, which means that some options (notably `-Wall`) are missing when building subsets of the code.

Dixit the [Automake docs](https://www.gnu.org/software/automake/manual/html_node/Program-and-Library-Variables.html):

> In compilations with per-target flags, the ordinary ‘AM_’ form of
> the flags variable is _not_ automatically included in the
> compilation (however, the user form of the variable _is_ included).
> So for instance, if you want the hypothetical ‘maude’ compilations
> to also use the value of ‘AM_CFLAGS’, you would need to write:
>
>      maude_CFLAGS = ... your flags ... $(AM_CFLAGS)

Restoring the flags, however, causes compilation failures (in the library) and a slew of new warnings (in the tests) which had not been noticed because of the missing options.

This series of patches (all "tagged" ZOOKEEPER-3654) fix these warnings and errors before re-enabling `-Wall` and friends for all targets.

Author: Damien Diederen <dd@crosstwine.com>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Andor Molnar <andor@apache.org>

Closes #1186 from ztzg/ZOOKEEPER-3654-incorrect-automake-flags

zookeeper-client/zookeeper-client-c/Makefile.am

@@ -60,7 +60,7 @@ libzookeeper_st_la_LDFLAGS = $(LIB_LDFLAGS) -export-symbols-regex $(EXPORT_SYMBO
 if WANT_SYNCAPI
 noinst_LTLIBRARIES += libzkmt.la
 libzkmt_la_SOURCES =$(COMMON_SRC) src/mt_adaptor.c
-libzkmt_la_CFLAGS = -DTHREADED
+libzkmt_la_CFLAGS = $(AM_CFLAGS) -DTHREADED
 libzkmt_la_LIBADD = -lm $(CLOCK_GETTIME_LIBS)
 
 lib_LTLIBRARIES += libzookeeper_mt.la
@@ -80,11 +80,11 @@ bin_PROGRAMS += cli_mt load_gen
 
 cli_mt_SOURCES = src/cli.c
 cli_mt_LDADD = libzookeeper_mt.la $(SASL_LIB_LDFLAGS)
-cli_mt_CFLAGS = -DTHREADED
+cli_mt_CFLAGS = $(AM_CFLAGS) -DTHREADED
 
 load_gen_SOURCES = src/load_gen.c
 load_gen_LDADD = libzookeeper_mt.la
-load_gen_CFLAGS = -DTHREADED
+load_gen_CFLAGS = $(AM_CFLAGS) -DTHREADED
 
 endif
 
@@ -134,14 +134,14 @@ TESTS_ENVIRONMENT = ZKROOT=${srcdir}/../.. \
                     CLASSPATH=$$CLASSPATH:$$CLOVER_HOME/lib/clover*.jar
 nodist_zktest_st_SOURCES = $(TEST_SOURCES)
 zktest_st_LDADD = libzkst.la libhashtable.la $(CPPUNIT_LIBS) $(OPENSSL_LIB_LDFLAGS) $(SASL_LIB_LDFLAGS) -ldl
-zktest_st_CXXFLAGS = -DUSE_STATIC_LIB $(CPPUNIT_CFLAGS) $(USEIPV6) $(SOLARIS_CPPFLAGS)
+zktest_st_CXXFLAGS = $(AM_CXXFLAGS) -DUSE_STATIC_LIB $(CPPUNIT_CFLAGS) $(SOLARIS_CPPFLAGS)
 zktest_st_LDFLAGS = -shared $(SYMBOL_WRAPPERS) $(SOLARIS_LIB_LDFLAGS)
 
 if WANT_SYNCAPI
   check_PROGRAMS += zktest-mt
   nodist_zktest_mt_SOURCES = $(TEST_SOURCES) tests/PthreadMocks.cc
   zktest_mt_LDADD = libzkmt.la libhashtable.la -lpthread $(CPPUNIT_LIBS) $(OPENSSL_LIB_LDFLAGS) $(SASL_LIB_LDFLAGS) -ldl
-  zktest_mt_CXXFLAGS = -DUSE_STATIC_LIB -DTHREADED $(CPPUNIT_CFLAGS) $(USEIPV6)
+  zktest_mt_CXXFLAGS = $(AM_CXXFLAGS) -DUSE_STATIC_LIB -DTHREADED $(CPPUNIT_CFLAGS) $(USEIPV6)
 if SOLARIS
   SHELL_SYMBOL_WRAPPERS_MT = cat ${srcdir}/tests/wrappers-mt.opt
   SYMBOL_WRAPPERS_MT=$(SYMBOL_WRAPPERS) $(SHELL_SYMBOL_WRAPPERS_MT:sh)

zookeeper-client/zookeeper-client-c/src/load_gen.c

@@ -27,8 +27,6 @@
 
 static zhandle_t *zh;
 
-static int shutdownThisThing=0;
-
 // *****************************************************************************
 //
 static pthread_cond_t cond=PTHREAD_COND_INITIALIZER;

zookeeper-client/zookeeper-client-c/src/mt_adaptor.c

@@ -369,13 +369,14 @@ void *do_io(void *v)
     fds[0].fd=adaptor_threads->self_pipe[0];
     fds[0].events=POLLIN;
     while(!zh->close_requested) {
-        zh->io_count++;
         struct timeval tv;
         int fd;
         int interest;
         int timeout;
         int maxfd=1;
 
+        zh->io_count++;
+
         zookeeper_interest(zh, &fd, &interest, &tv);
         if (fd != -1) {
             fds[1].fd=fd;

zookeeper-client/zookeeper-client-c/src/zookeeper.c

@@ -342,6 +342,7 @@ static int is_sasl_auth_in_progress(zhandle_t* zh)
 #endif /* HAVE_CYRUS_SASL_H */
 }
 
+#ifndef THREADED
 /*
  * abort due to the use of a sync api in a singlethreaded environment
  */
@@ -350,6 +351,7 @@ static void abort_singlethreaded(zhandle_t *zh)
     LOG_ERROR(LOGCALLBACK(zh), "Sync completion used without threads");
     abort();
 }
+#endif  /* THREADED */
 
 static ssize_t zookeeper_send(zsock_t *fd, const void* buf, size_t len)
 {

zookeeper-client/zookeeper-client-c/tests/TestClient.cc

@@ -325,9 +325,10 @@ public:
     
     /** have a callback in the default watcher **/
     static void default_zoo_watcher(zhandle_t *zzh, int type, int state, const char *path, void *context){
-        int zrc = 0;
+        int zrc;
         struct String_vector str_vec = {0, NULL};
         zrc = zoo_wget_children(zzh, "/mytest", default_zoo_watcher, NULL, &str_vec);
+        CPPUNIT_ASSERT(zrc == ZOK || zrc == ZCLOSING);
     }
 
     /** ZOOKEEPER-1057 This checks that the client connects to the second server when the first is not reachable **/
@@ -353,24 +354,28 @@ public:
 
     /** this checks for a deadlock in calling zookeeper_close and calls from a default watcher that might get triggered just when zookeeper_close() is in progress **/
     void testHangingClient() {
-        int zrc = 0;
+        int zrc;
         char buff[10] = "testall";
         char path[512];
-        watchctx_t *ctx;
+        watchctx_t *ctx = NULL;
         struct String_vector str_vec = {0, NULL};
         zhandle_t *zh = zookeeper_init(hostPorts, NULL, 10000, 0, ctx, 0);
         sleep(1);
         zrc = zoo_create(zh, "/mytest", buff, 10, &ZOO_OPEN_ACL_UNSAFE, 0, path, 512);
+        CPPUNIT_ASSERT_EQUAL((int)ZOK, zrc);
         zrc = zoo_wget_children(zh, "/mytest", default_zoo_watcher, NULL, &str_vec);
+        CPPUNIT_ASSERT_EQUAL((int)ZOK, zrc);
         zrc = zoo_create(zh, "/mytest/test1", buff, 10, &ZOO_OPEN_ACL_UNSAFE, 0, path, 512);
+        CPPUNIT_ASSERT_EQUAL((int)ZOK, zrc);
         zrc = zoo_wget_children(zh, "/mytest", default_zoo_watcher, NULL, &str_vec);
+        CPPUNIT_ASSERT_EQUAL((int)ZOK, zrc);
         zrc = zoo_delete(zh, "/mytest/test1", -1);
+        CPPUNIT_ASSERT_EQUAL((int)ZOK, zrc);
         zookeeper_close(zh);
     }
 
     void testBadDescriptor() {
-        int zrc = 0;
-        watchctx_t *ctx;
+        watchctx_t *ctx = NULL;
         zhandle_t *zh = zookeeper_init(hostPorts, NULL, 10000, 0, ctx, 0);
         sleep(1);
         zh->io_count = 0;
@@ -1039,8 +1044,8 @@ public:
         CPPUNIT_ASSERT_EQUAL(zoo_get_log_callback(zk), &logMessageHandler);
 
         // Log 10 messages and ensure all go to callback
-        int expected = 10;
-        for (int i = 0; i < expected; i++)
+        const std::size_t expected = 10;
+        for (std::size_t i = 0; i < expected; i++)
         {
             LOG_INFO(LOGCALLBACK(zk), "%s #%d", __FUNCTION__, i);
         }
@@ -1057,12 +1062,12 @@ public:
 
         // All the connection messages should have gone to the callback -- don't
         // want this to be a maintenance issue so we're not asserting exact count
-        int numBefore = logMessages.size();
+        std::size_t numBefore = logMessages.size();
         CPPUNIT_ASSERT(numBefore != 0);
 
         // Log 10 messages and ensure all go to callback
-        int expected = 10;
-        for (int i = 0; i < expected; i++)
+        const std::size_t expected = 10;
+        for (std::size_t i = 0; i < expected; i++)
         {
             LOG_INFO(LOGCALLBACK(zk), "%s #%d", __FUNCTION__, i);
         }

zookeeper-client/zookeeper-client-c/tests/TestMulti.cc

@@ -486,8 +486,6 @@ public:
         watchctx_t ctx;
         zhandle_t *zk = createClient(&ctx);
         int sz = 512;
-        char buf[sz];
-        int blen;
         char p1[sz];
         p1[0] = '\0';
         struct Stat stat;
@@ -573,7 +571,6 @@ public:
         int sz = 512;
         char p1[sz];
         p1[0] = '\0';
-        struct Stat s1;
 
         rc = zoo_create(zk, "/multi0", "", 0, &ZOO_OPEN_ACL_UNSAFE, 0, p1, sz);
         CPPUNIT_ASSERT_EQUAL((int)ZOK, rc);

zookeeper-client/zookeeper-client-c/tests/TestReconfigServer.cc

@@ -49,12 +49,12 @@ class TestReconfigServer : public CPPUNIT_NS::TestFixture {
     static const uint32_t NUM_SERVERS;
     FILE* logfile_;
     std::vector<ZooKeeperQuorumServer*> cluster_;
-    int32_t getLeader();
-    std::vector<int32_t> getFollowers();
+    std::size_t getLeader();
+    std::vector<std::size_t> getFollowers();
     void parseConfig(char* buf, int len, std::vector<std::string>& servers,
                      std::string& version);
     bool waitForConnected(zhandle_t* zh, uint32_t timeout_sec);
-    zhandle_t* connectFollowers(std::vector<int32_t> &followers);
+    zhandle_t* connectFollowers(std::vector<std::size_t> &followers);
 };
 
 const uint32_t TestReconfigServer::NUM_SERVERS = 3;
@@ -84,15 +84,15 @@ setUp() {
 
 void TestReconfigServer::
 tearDown() {
-    for (int i = 0; i < cluster_.size(); i++) {
+    for (std::size_t i = 0; i < cluster_.size(); i++) {
         delete cluster_[i];
     }
     cluster_.clear();
 }
 
-int32_t TestReconfigServer::
+std::size_t TestReconfigServer::
 getLeader() {
-    for (int32_t i = 0; i < cluster_.size(); i++) {
+    for (std::size_t i = 0; i < cluster_.size(); i++) {
         if (cluster_[i]->isLeader()) {
             return i;
         }
@@ -100,10 +100,10 @@ getLeader() {
     return -1;
 }
 
-std::vector<int32_t> TestReconfigServer::
+std::vector<std::size_t> TestReconfigServer::
 getFollowers() {
-    std::vector<int32_t> followers;
-    for (int32_t i = 0; i < cluster_.size(); i++) {
+    std::vector<std::size_t> followers;
+    for (std::size_t i = 0; i < cluster_.size(); i++) {
         if (cluster_[i]->isFollower()) {
             followers.push_back(i);
         }
@@ -154,7 +154,7 @@ testRemoveFollower() {
     char buf[len];
 
     // get config from leader.
-    int32_t leader = getLeader();
+    std::size_t leader = getLeader();
     CPPUNIT_ASSERT(leader >= 0);
     std::string host = cluster_[leader]->getHostPort();
     zhandle_t* zk = zookeeper_init(host.c_str(), NULL, 10000, NULL, NULL, 0);
@@ -167,13 +167,13 @@ testRemoveFollower() {
     // of the first NEWLEADER message, used as the initial version
     CPPUNIT_ASSERT_EQUAL(std::string("100000000"), version);
     CPPUNIT_ASSERT_EQUAL(NUM_SERVERS, (uint32_t)(servers.size()));
-    for (int i = 0; i < cluster_.size(); i++) {
+    for (std::size_t i = 0; i < cluster_.size(); i++) {
         CPPUNIT_ASSERT(std::find(servers.begin(), servers.end(),
                        cluster_[i]->getServerString()) != servers.end());
     }
 
     // remove a follower.
-    std::vector<int32_t> followers = getFollowers();
+    std::vector<std::size_t> followers = getFollowers();
     len = 1024;
     CPPUNIT_ASSERT_EQUAL(NUM_SERVERS - 1,
                          (uint32_t)(followers.size()));
@@ -185,7 +185,7 @@ testRemoveFollower() {
     parseConfig(buf, len, servers, version);
     CPPUNIT_ASSERT_EQUAL(std::string("100000002"), version);
     CPPUNIT_ASSERT_EQUAL(NUM_SERVERS - 1, (uint32_t)(servers.size()));
-    for (int i = 0; i < cluster_.size(); i++) {
+    for (std::size_t i = 0; i < cluster_.size(); i++) {
         if (i == followers[0]) {
             continue;
         }
@@ -201,7 +201,7 @@ testRemoveFollower() {
     CPPUNIT_ASSERT_EQUAL((int)ZOK, rc);
     parseConfig(buf, len, servers, version);
     CPPUNIT_ASSERT_EQUAL(NUM_SERVERS, (uint32_t)(servers.size()));
-    for (int i = 0; i < cluster_.size(); i++) {
+    for (std::size_t i = 0; i < cluster_.size(); i++) {
         CPPUNIT_ASSERT(std::find(servers.begin(), servers.end(),
                        cluster_[i]->getServerString()) != servers.end());
     }
@@ -222,7 +222,7 @@ testNonIncremental() {
     char buf[len];
 
     // get config from leader.
-    int32_t leader = getLeader();
+    std::size_t leader = getLeader();
     CPPUNIT_ASSERT(leader >= 0);
     std::string host = cluster_[leader]->getHostPort();
     zhandle_t* zk = zookeeper_init(host.c_str(), NULL, 10000, NULL, NULL, 0);
@@ -236,18 +236,18 @@ testNonIncremental() {
     // of the first NEWLEADER message, used as the initial version
     CPPUNIT_ASSERT_EQUAL(std::string("100000000"), version);
     CPPUNIT_ASSERT_EQUAL(NUM_SERVERS, (uint32_t)(servers.size()));
-    for (int i = 0; i < cluster_.size(); i++) {
+    for (std::size_t i = 0; i < cluster_.size(); i++) {
         CPPUNIT_ASSERT(std::find(servers.begin(), servers.end(),
                        cluster_[i]->getServerString()) != servers.end());
     }
 
     // remove a follower.
-    std::vector<int32_t> followers = getFollowers();
+    std::vector<std::size_t> followers = getFollowers();
     len = 1024;
     CPPUNIT_ASSERT_EQUAL(NUM_SERVERS - 1,
                          (uint32_t)(followers.size()));
     std::stringstream ss;
-    for (int i = 1; i < followers.size(); i++) {
+    for (std::size_t i = 1; i < followers.size(); i++) {
       ss << cluster_[followers[i]]->getServerString() << ",";
     }
     ss << cluster_[leader]->getServerString();
@@ -258,7 +258,7 @@ testNonIncremental() {
     parseConfig(buf, len, servers, version);
     CPPUNIT_ASSERT_EQUAL(std::string("100000002"), version);
     CPPUNIT_ASSERT_EQUAL(NUM_SERVERS - 1, (uint32_t)(servers.size()));
-    for (int i = 0; i < cluster_.size(); i++) {
+    for (std::size_t i = 0; i < cluster_.size(); i++) {
         if (i == followers[0]) {
             continue;
         }
@@ -269,7 +269,7 @@ testNonIncremental() {
     // add the follower back.
     len = 1024;
     ss.str("");
-    for (int i = 0; i < cluster_.size(); i++) {
+    for (std::size_t i = 0; i < cluster_.size(); i++) {
       ss << cluster_[i]->getServerString() << ",";
     }
     rc = zoo_reconfig(zk, NULL, NULL, ss.str().c_str(), -1, buf, &len,
@@ -277,7 +277,7 @@ testNonIncremental() {
     CPPUNIT_ASSERT_EQUAL((int)ZOK, rc);
     parseConfig(buf, len, servers, version);
     CPPUNIT_ASSERT_EQUAL(NUM_SERVERS, (uint32_t)(servers.size()));
-    for (int i = 0; i < cluster_.size(); i++) {
+    for (std::size_t i = 0; i < cluster_.size(); i++) {
         CPPUNIT_ASSERT(std::find(servers.begin(), servers.end(),
                        cluster_[i]->getServerString()) != servers.end());
     }
@@ -285,12 +285,12 @@ testNonIncremental() {
 }
 
 zhandle_t* TestReconfigServer::
-connectFollowers(std::vector<int32_t> &followers) {
+connectFollowers(std::vector<std::size_t> &followers) {
     std::stringstream ss;
-    int32_t leader = getLeader();
+    std::size_t leader = getLeader();
     CPPUNIT_ASSERT(leader >= 0);
     CPPUNIT_ASSERT_EQUAL(NUM_SERVERS - 1, (uint32_t)(followers.size()));
-    for (int i = 0; i < followers.size(); i++) {
+    for (std::size_t i = 0; i < followers.size(); i++) {
         ss << cluster_[followers[i]]->getHostPort() << ",";
     }
     ss << cluster_[leader]->getHostPort();
@@ -321,7 +321,7 @@ testRemoveConnectedFollower() {
 
     // connect to a follower.
     std::stringstream ss;
-    std::vector<int32_t> followers = getFollowers();
+    std::vector<std::size_t> followers = getFollowers();
     zhandle_t* zk = connectFollowers(followers);
     CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_add_auth(zk, "digest", "super:test", 10, NULL,(void*)ZOK));
 
@@ -333,7 +333,7 @@ testRemoveConnectedFollower() {
     CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_getconfig(zk, 0, buf, &len, &stat));
     parseConfig(buf, len, servers, version);
     CPPUNIT_ASSERT_EQUAL(NUM_SERVERS - 1, (uint32_t)(servers.size()));
-    for (int i = 0; i < cluster_.size(); i++) {
+    for (std::size_t i = 0; i < cluster_.size(); i++) {
         if (i == followers[0]) {
             continue;
         }
@@ -356,7 +356,7 @@ testReconfigFailureWithoutAuth() {
 
     // connect to a follower.
     std::stringstream ss;
-    std::vector<int32_t> followers = getFollowers();
+    std::vector<std::size_t> followers = getFollowers();
     zhandle_t* zk = connectFollowers(followers);
 
     // remove the follower.
@@ -374,7 +374,7 @@ testReconfigFailureWithoutAuth() {
     CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_getconfig(zk, 0, buf, &len, &stat));
     parseConfig(buf, len, servers, version);
     CPPUNIT_ASSERT_EQUAL(NUM_SERVERS - 1, (uint32_t)(servers.size()));
-    for (int i = 0; i < cluster_.size(); i++) {
+    for (std::size_t i = 0; i < cluster_.size(); i++) {
         if (i == followers[0]) {
             continue;
         }
@@ -400,7 +400,7 @@ testReconfigFailureWithoutServerSuperuserPasswordConfigured() {
 
     // connect to a follower.
     std::stringstream ss;
-    std::vector<int32_t> followers = getFollowers();
+    std::vector<std::size_t> followers = getFollowers();
     zhandle_t* zk = connectFollowers(followers);
 
     // remove the follower.