Skip to content

Commit

Permalink
ZOOKEEPER-3654: Incorrect *_CFLAGS handling in Automake
Browse files Browse the repository at this point in the history 
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 apache#1186 from ztzg/ZOOKEEPER-3654-incorrect-automake-flags
  • Loading branch information
@ztzg @RokLenarcic
ztzg authored and RokLenarcic committed Aug 31, 2022
1 parent 0affffa commit c08d547
Show file tree
Hide file tree
Showing 7 changed files with 53 additions and 50 deletions.
10 changes: 5 additions & 5 deletions zookeeper-client/zookeeper-client-c/Makefile.am
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down Expand Up @@ -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)
Expand Down
2 changes: 0 additions & 2 deletions zookeeper-client/zookeeper-client-c/src/load_gen.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@

static zhandle_t *zh;

static int shutdownThisThing=0;

// *****************************************************************************
//
static pthread_cond_t cond=PTHREAD_COND_INITIALIZER;
Expand Down
3 changes: 2 additions & 1 deletion zookeeper-client/zookeeper-client-c/src/mt_adaptor.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions zookeeper-client/zookeeper-client-c/src/zookeeper.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand All @@ -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)
{
Expand Down
25 changes: 15 additions & 10 deletions zookeeper-client/zookeeper-client-c/tests/TestClient.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -325,9 +325,10 @@ class Zookeeper_simpleSystem : public CPPUNIT_NS::TestFixture

/** 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 **/
Expand All @@ -353,24 +354,28 @@ class Zookeeper_simpleSystem : public CPPUNIT_NS::TestFixture

/** 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;
Expand Down Expand Up @@ -1039,8 +1044,8 @@ class Zookeeper_simpleSystem : public CPPUNIT_NS::TestFixture
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);
}
Expand All @@ -1057,12 +1062,12 @@ class Zookeeper_simpleSystem : public CPPUNIT_NS::TestFixture

// 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);
}
Expand Down
3 changes: 0 additions & 3 deletions zookeeper-client/zookeeper-client-c/tests/TestMulti.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -486,8 +486,6 @@ class Zookeeper_multi : public CPPUNIT_NS::TestFixture
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;
Expand Down Expand Up @@ -573,7 +571,6 @@ class Zookeeper_multi : public CPPUNIT_NS::TestFixture
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);
Expand Down
58 changes: 29 additions & 29 deletions zookeeper-client/zookeeper-client-c/tests/TestReconfigServer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -84,26 +84,26 @@ 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;
}
}
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);
}
Expand Down Expand Up @@ -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);
Expand All @@ -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()));
Expand All @@ -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;
}
Expand All @@ -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());
}
Expand All @@ -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);
Expand All @@ -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();
Expand All @@ -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;
}
Expand All @@ -269,28 +269,28 @@ 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,
&stat);
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());
}
zookeeper_close(zk);
}

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();
Expand Down Expand Up @@ -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));

Expand All @@ -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;
}
Expand All @@ -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.
Expand All @@ -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;
}
Expand All @@ -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.
Expand Down

0 comments on commit c08d547

Please sign in to comment.