mon: make paxos_size() unsigned

* 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<int>
  to std::set<unsigned>. 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<int>::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<int>::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 <tchaikov@gmail.com>

diff --git a/src/mon/ElectionLogic.h b/src/mon/ElectionLogic.h
index ed53e81b6187438e49aa5554ba79a4cedccc46d1..e2f2db82ac88fe54632e5977b26d65efa5e51e13 100644 (file)
--- 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 c9f9ed877eda418413d494d172f45fd27b3a45b3..5a23f1d877e8c9a2f86d2f10171a8fe4d35ef30b 100644 (file)
--- 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 309d5a3b43467fac093e3c61b173c7f98345cef3..2c3ec0c960c172a37640384213db737939215a30 100644 (file)
--- 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<int> disallowed_leaders;
   const std::set<int>& 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 ad189e29b060453e9eefc65818f889e754e476bf..7fedd2c1128b9506712a65eba0518b0b008fdf86 100644 (file)
--- 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<int> removed_ranks;
+  std::set<unsigned> 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 24a09f398a8ff0fa668f7e9120c0e89255287adf..35c71b68158899ad9ad709e68bcbc8f1a082ffa3 100644 (file)
--- 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 568cbcc3851ee217614845c06789cb71618e0a5f..188ec3323b9b254f01fdd96b92be611fd79023ef 100644 (file)
--- 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<int>& get_disallowed_leaders() const {
     return parent->get_disallowed_leaders();
   }