From 1529e4978670d80d71d951838f1a9e564cbe53ee Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Sun, 10 Jul 2022 00:50:14 -0400 Subject: [PATCH] mon: make paxos_size() unsigned MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit * make paxos_size() unsigned, as paxos_size() returns the size of MonMap::mon_info, so it should be always a non-negative value, and more importantly, it represents a size. * change the type of MonMap::removed_ranks from std::set to std::set. for two reasons: - removed_ranks only tracks the rank which is greater or equal to 0 - helps to silence the warnings listed below. MonMap::removed_ranks is persisted using encode()/decode(), but this change is backward compatible, as we use the raw encoder to encode signed and unsigned integers, the difference between the encoding schema between them only matters when MSB in the number is used, but this is not likely happen, as we neither have a negative rank in removed_ranks, no have a rank greater than `(unsigned)-1`, i.e., 0xffffffff. this change partially reverts f75dfbc055ccf4e43b817ed5aa52898ff680e19e to address the compiling warnings like: /home/kefu/dev/ceph/src/mon/ElectionLogic.cc: In member function ‘void ElectionLogic::end_election_period()’: /home/kefu/dev/ceph/src/mon/ElectionLogic.cc:173:23: error: comparison of integer expressions of different signedness: ‘std::set::size_type’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare] 173 | acked_me.size() > (elector->paxos_size() / 2)) { | ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/kefu/dev/ceph/src/mon/ElectionLogic.cc: In member function ‘void ElectionLogic::propose_connectivity_handler(int, epoch_t, const ConnectionTracker*)’: /home/kefu/dev/ceph/src/mon/ElectionLogic.cc:338:28: error: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Werror=sign-compare] 338 | for (unsigned i = 0; i < elector->paxos_size(); ++i) { | ~~^~~~~~~~~~~~~~~~~~~~~~~ /home/kefu/dev/ceph/src/mon/ElectionLogic.cc: In member function ‘void ElectionLogic::receive_ack(int, epoch_t)’: /home/kefu/dev/ceph/src/mon/ElectionLogic.cc:469:25: error: comparison of integer expressions of different signedness: ‘std::set::size_type’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare] 469 | if (acked_me.size() == elector->paxos_size()) { | ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ cc1plus: all warnings being treated as errors make[3]: *** [src/mon/CMakeFiles/mon.dir/build.make:328: src/mon/CMakeFiles/mon.dir/ElectionLogic.cc.o] Error 1 make[3]: *** Waiting for unfinished jobs.... make[3]: Leaving directory '/home/kefu/dev/ceph/build' [ 48%] Built target libglobal_objs /home/kefu/dev/ceph/src/mon/Elector.cc: In member function ‘void Elector::notify_rank_removed(int)’: /home/kefu/dev/ceph/src/mon/Elector.cc:734:43: error: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Werror=sign-compare] 734 | for (unsigned i = rank_removed + 1; i <= paxos_size() ; ++i) { | ~~^~~~~~~~~~~~~~~ Fixes: https://tracker.ceph.com/issues/56581 Signed-off-by: Kefu Chai --- src/mon/ElectionLogic.h | 2 +- src/mon/Elector.cc | 4 ++-- src/mon/Elector.h | 4 ++-- src/mon/MonMap.h | 2 +- src/mon/Monitor.cc | 2 +- src/test/mon/test_election.cc | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/mon/ElectionLogic.h b/src/mon/ElectionLogic.h index ed53e81b61874..e2f2db82ac88f 100644 --- a/src/mon/ElectionLogic.h +++ b/src/mon/ElectionLogic.h @@ -90,7 +90,7 @@ public: * by making a paxos commit, but not by injecting values while * an election is ongoing.) */ - virtual int paxos_size() const = 0; + virtual unsigned paxos_size() const = 0; /** * Retrieve a set of ranks which are not allowed to become the leader. * Like paxos_size(), This set can change between elections, but not diff --git a/src/mon/Elector.cc b/src/mon/Elector.cc index c9f9ed877eda4..5a23f1d877e8c 100644 --- a/src/mon/Elector.cc +++ b/src/mon/Elector.cc @@ -130,7 +130,7 @@ bool Elector::ever_participated() const return mon->has_ever_joined; } -int Elector::paxos_size() const +unsigned Elector::paxos_size() const { return mon->monmap->size(); } @@ -708,7 +708,7 @@ void Elector::notify_rank_changed(int new_rank) dead_pinging.erase(new_rank); } -void Elector::notify_rank_removed(int rank_removed) +void Elector::notify_rank_removed(unsigned rank_removed) { peer_tracker.notify_rank_removed(rank_removed); /* we have to clean up the pinging state, which is annoying diff --git a/src/mon/Elector.h b/src/mon/Elector.h index 309d5a3b43467..2c3ec0c960c17 100644 --- a/src/mon/Elector.h +++ b/src/mon/Elector.h @@ -240,7 +240,7 @@ class Elector : public ElectionOwner, RankProvider { /* Retrieve the Monitor::has_ever_joined member */ bool ever_participated() const; /* Retrieve monmap->size() */ - int paxos_size() const; + unsigned paxos_size() const; /* Right now we don't disallow anybody */ std::set disallowed_leaders; const std::set& get_disallowed_leaders() const { return disallowed_leaders; } @@ -371,7 +371,7 @@ class Elector : public ElectionOwner, RankProvider { * This is safe to call even if we haven't joined or are currently * in a quorum. */ - void notify_rank_removed(int rank_removed); + void notify_rank_removed(unsigned rank_removed); void notify_strategy_maybe_changed(int strategy); /** * Set the disallowed leaders. diff --git a/src/mon/MonMap.h b/src/mon/MonMap.h index ad189e29b0604..7fedd2c1128b9 100644 --- a/src/mon/MonMap.h +++ b/src/mon/MonMap.h @@ -107,7 +107,7 @@ class MonMap { /* ranks which were removed when this map took effect. There should only be one at a time, but leave support for arbitrary numbers just to be safe. */ - std::set removed_ranks; + std::set removed_ranks; /** * Persistent Features are all those features that once set on a diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc index 24a09f398a8ff..35c71b6815889 100644 --- a/src/mon/Monitor.cc +++ b/src/mon/Monitor.cc @@ -6589,7 +6589,7 @@ void Monitor::notify_new_monmap(bool can_change_external_state) dout(30) << __func__ << "we have " << monmap->removed_ranks.size() << " removed ranks" << dendl; for (auto i = monmap->removed_ranks.rbegin(); i != monmap->removed_ranks.rend(); ++i) { - int rank = *i; + unsigned rank = *i; dout(10) << __func__ << "removing rank " << rank << dendl; elector.notify_rank_removed(rank); } diff --git a/src/test/mon/test_election.cc b/src/test/mon/test_election.cc index 568cbcc3851ee..188ec3323b9b2 100644 --- a/src/test/mon/test_election.cc +++ b/src/test/mon/test_election.cc @@ -147,7 +147,7 @@ struct Owner : public ElectionOwner, RankProvider { logic.start(); } bool ever_participated() const { return ever_joined; } - int paxos_size() const { return parent->get_paxos_size(); } + unsigned paxos_size() const { return parent->get_paxos_size(); } const set& get_disallowed_leaders() const { return parent->get_disallowed_leaders(); } -- 2.39.5