[PATCH 0/8] fix some issues from cppchecker/clang++

[PATCH 0/8] fix some issues from cppchecker/clang++

[Danny Al-Gaaf]

These patches fix some issues found by using cppchecker/clang++.

Danny Al-Gaaf (8):
  WorkQueue.h: fix cast
  ceph_crypto.cc: remove unused shutdown() outside crypto ifdef's
  obj_bencher.cc: use vector instead of VLA's
  include/buffer.h: fix operator=
  include/types.h: change operator<< function parameter
  include/xlist.h: fix C-style pointer casting
  messages/MOSDRepScrub.h: initialize member variable in constructor
  msg/Message.h: fix C-style pointer casting

 src/common/WorkQueue.h      |  4 ++--
 src/common/ceph_crypto.cc   |  1 -
 src/common/obj_bencher.cc   | 13 +++++++------
 src/include/buffer.h        | 10 +++++++---
 src/include/types.h         |  2 +-
 src/include/xlist.h         |  6 +++---
 src/messages/MOSDRepScrub.h |  2 +-
 src/msg/Message.h           |  4 ++--
 8 files changed, 23 insertions(+), 19 deletions(-)

-- 
1.8.1.2

[PATCH 1/8] WorkQueue.h: fix cast

[Danny Al-Gaaf]

Replace C-style pointer casting with correct static_cast<>().

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/common/WorkQueue.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/common/WorkQueue.h b/src/common/WorkQueue.h
index 9fb215b..b19a6a2 100644
--- a/src/common/WorkQueue.h
+++ b/src/common/WorkQueue.h
@@ -251,10 +251,10 @@ public:
       return (void *)_dequeue();
     }
     void _void_process(void *p, TPHandle &handle) {
-      _process((T *)p, handle);
+      _process(static_cast<T *>(p), handle);
     }
     void _void_process_finish(void *p) {
-      _process_finish((T *)p);
+      _process_finish(static_cast<T *>(p));
     }
 
   public:
-- 
1.8.1.2

[PATCH 2/8] ceph_crypto.cc: remove unused shutdown() outside crypto ifdef's

[Danny Al-Gaaf]

Fix "out-of-line declaration of a member must be a definition
[-Wout-of-line-declaration]". Remove ceph::crypto::shutdown() outside
the crypto related ifdef's. Without nss or cryptopp configure will
fail anyways.

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/common/ceph_crypto.cc | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/common/ceph_crypto.cc b/src/common/ceph_crypto.cc
index 3f04349..96fa157 100644
--- a/src/common/ceph_crypto.cc
+++ b/src/common/ceph_crypto.cc
@@ -20,7 +20,6 @@
 #include <pthread.h>
 #include <stdlib.h>
 
-void ceph::crypto::shutdown();
 
 #ifdef USE_CRYPTOPP
 void ceph::crypto::init(CephContext *cct)
-- 
1.8.1.2

[PATCH 3/8] obj_bencher.cc: use vector instead of VLA's

[Danny Al-Gaaf]

Fix "variable length array of non-POD element type" error. (-Wvla)

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/common/obj_bencher.cc | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/common/obj_bencher.cc b/src/common/obj_bencher.cc
index 74d54e1..d756123 100644
--- a/src/common/obj_bencher.cc
+++ b/src/common/obj_bencher.cc
@@ -25,6 +25,7 @@
 #include <stdlib.h>
 #include <time.h>
 #include <sstream>
+#include <vector>
 
 
 const std::string BENCH_LASTRUN_METADATA = "benchmark_last_metadata";
@@ -305,11 +306,11 @@ int ObjBencher::write_bench(int secondsToRun, int concurrentios) {
   std::string prefix = generate_object_prefix();
   out(cout) << "Object prefix: " << prefix << std::endl;
 
-  std::string name[concurrentios];
+  std::vector<string> name(concurrentios);
   std::string newName;
   bufferlist* contents[concurrentios];
   double total_latency = 0;
-  utime_t start_times[concurrentios];
+  std::vector<utime_t> start_times(concurrentios);
   utime_t stopTime;
   int r = 0;
   bufferlist b_write;
@@ -493,13 +494,13 @@ int ObjBencher::write_bench(int secondsToRun, int concurrentios) {
 
 int ObjBencher::seq_read_bench(int seconds_to_run, int num_objects, int concurrentios, int pid) {
   lock_cond lc(&lock);
-  std::string name[concurrentios];
+  std::vector<string> name(concurrentios);
   std::string newName;
   bufferlist* contents[concurrentios];
   int index[concurrentios];
   int errors = 0;
   utime_t start_time;
-  utime_t start_times[concurrentios];
+  std::vector<utime_t> start_times(concurrentios);
   utime_t time_to_run;
   time_to_run.set_from_double(seconds_to_run);
   double total_latency = 0;
@@ -705,7 +706,7 @@ int ObjBencher::clean_up(const std::string& prefix, int concurrentios) {
 
 int ObjBencher::clean_up(int num_objects, int prevPid, int concurrentios) {
   lock_cond lc(&lock);
-  std::string name[concurrentios];
+  std::vector<string> name(concurrentios);
   std::string newName;
   int r = 0;
   utime_t runtime;
@@ -865,7 +866,7 @@ bool ObjBencher::more_objects_matching_prefix(const std::string& prefix, std::li
 
 int ObjBencher::clean_up_slow(const std::string& prefix, int concurrentios) {
   lock_cond lc(&lock);
-  std::string name[concurrentios];
+  std::vector<string> name(concurrentios);
   std::string newName;
   int r = 0;
   utime_t runtime;
-- 
1.8.1.2

[PATCH 4/8] include/buffer.h: fix operator=

[Danny Al-Gaaf]

Fix operator=: return "iterator&" instead of 'iterator'. Also set last_p and
other.append_buffer and check if 'this' equals 'other' before set anything.

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/include/buffer.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/include/buffer.h b/src/include/buffer.h
index 9a635bd..448d947 100644
--- a/src/include/buffer.h
+++ b/src/include/buffer.h
@@ -248,7 +248,7 @@ public:
 					p(other.p),
 					p_off(other.p_off) {}
 
-      iterator operator=(const iterator& other) {
+      iterator& operator=(const iterator& other) {
 	if (this != &other) {
 	  bl = other.bl;
 	  ls = other.ls;
@@ -305,8 +305,12 @@ public:
     
     list(const list& other) : _buffers(other._buffers), _len(other._len), last_p(this) { }
     list& operator= (const list& other) {
-      _buffers = other._buffers;
-      _len = other._len;
+      if (this != &other) {
+        _buffers = other._buffers;
+        _len = other._len;
+        last_p = other.last_p;
+	append_buffer = other.append_buffer;
+      }
       return *this;
     }
 
-- 
1.8.1.2

[PATCH 5/8] include/types.h: change operator<< function parameter

[Danny Al-Gaaf]

Fix "Function parameter 'v' should be passed by reference." from cppchecker.
Use 'const pair<A,B>& v' similar to the other operator<< in this file.

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/include/types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/include/types.h b/src/include/types.h
index c783b6e..dff47ac 100644
--- a/src/include/types.h
+++ b/src/include/types.h
@@ -120,7 +120,7 @@ namespace __gnu_cxx {
 // -- io helpers --
 
 template<class A, class B>
-inline ostream& operator<<(ostream& out, const pair<A,B> v) {
+inline ostream& operator<<(ostream& out, const pair<A,B>& v) {
   return out << v.first << "," << v.second;
 }
 
-- 
1.8.1.2

[PATCH 6/8] include/xlist.h: fix C-style pointer casting

[Danny Al-Gaaf]

Replace C-style pointer casting with correct static_cast<>().

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/include/xlist.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/include/xlist.h b/src/include/xlist.h
index 5c2bf03..5384561 100644
--- a/src/include/xlist.h
+++ b/src/include/xlist.h
@@ -132,8 +132,8 @@ public:
     assert((bool)_front == (bool)_size);
   }
 
-  T front() { return (T)_front->_item; }
-  T back() { return (T)_back->_item; }
+  T front() { return static_cast<T>(_front->_item); }
+  T back() { return static_cast<T>(_back->_item); }
 
   void pop_front() {
     assert(!empty());
@@ -149,7 +149,7 @@ public:
     item *cur;
   public:
     iterator(item *i = 0) : cur(i) {}
-    T operator*() { return (T)cur->_item; }
+    T operator*() { return static_cast<T>(cur->_item); }
     iterator& operator++() {
       assert(cur);
       assert(cur->_list);
-- 
1.8.1.2

[PATCH 7/8] messages/MOSDRepScrub.h: initialize member variable in constructor

[Danny Al-Gaaf]

Initialize chunky and deep bool member variables in the constructor
with false.

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/messages/MOSDRepScrub.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/messages/MOSDRepScrub.h b/src/messages/MOSDRepScrub.h
index 2d3a66d..cd1de06 100644
--- a/src/messages/MOSDRepScrub.h
+++ b/src/messages/MOSDRepScrub.h
@@ -36,7 +36,7 @@ struct MOSDRepScrub : public Message {
   hobject_t end;         // upper bound of scrub, exclusive
   bool deep;             // true if scrub should be deep
 
-  MOSDRepScrub() : Message(MSG_OSD_REP_SCRUB, HEAD_VERSION, COMPAT_VERSION) { }
+  MOSDRepScrub() : Message(MSG_OSD_REP_SCRUB, HEAD_VERSION, COMPAT_VERSION) { chunky = false; deep = false; }
   MOSDRepScrub(pg_t pgid, eversion_t scrub_from, eversion_t scrub_to,
 	       epoch_t map_epoch)
     : Message(MSG_OSD_REP_SCRUB, HEAD_VERSION, COMPAT_VERSION),
-- 
1.8.1.2

[PATCH 8/8] msg/Message.h: fix C-style pointer casting

[Danny Al-Gaaf]

Replace C-style pointer casting with correct static_cast<>().

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/msg/Message.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/msg/Message.h b/src/msg/Message.h
index 5bdd4d4..5e2b4f5 100644
--- a/src/msg/Message.h
+++ b/src/msg/Message.h
@@ -188,7 +188,7 @@ public:
   }
 
   Connection *get() {
-    return (Connection *)RefCountedObject::get();
+    return static_cast<Connection *>(RefCountedObject::get());
   }
 
   void set_priv(RefCountedObject *o) {
@@ -329,7 +329,7 @@ public:
   }
 
   Message *get() {
-    return (Message *)RefCountedObject::get();
+    return static_cast<Message *>(RefCountedObject::get());
   }
 
 protected:
-- 
1.8.1.2

Re: [PATCH 0/8] fix some issues from cppchecker/clang++

[Gary Lowell]

Hi Danny -

These patches are now in the wip.cppchecker branch.  Build so far looks good.

Cheers,
Gary

On Feb 5, 2013, at 2:52 PM, Danny Al-Gaaf wrote:

> These patches fix some issues found by using cppchecker/clang++.
> 
> Danny Al-Gaaf (8):
>  WorkQueue.h: fix cast
>  ceph_crypto.cc: remove unused shutdown() outside crypto ifdef's
>  obj_bencher.cc: use vector instead of VLA's
>  include/buffer.h: fix operator=
>  include/types.h: change operator<< function parameter
>  include/xlist.h: fix C-style pointer casting
>  messages/MOSDRepScrub.h: initialize member variable in constructor
>  msg/Message.h: fix C-style pointer casting
> 
> src/common/WorkQueue.h      |  4 ++--
> src/common/ceph_crypto.cc   |  1 -
> src/common/obj_bencher.cc   | 13 +++++++------
> src/include/buffer.h        | 10 +++++++---
> src/include/types.h         |  2 +-
> src/include/xlist.h         |  6 +++---
> src/messages/MOSDRepScrub.h |  2 +-
> src/msg/Message.h           |  4 ++--
> 8 files changed, 23 insertions(+), 19 deletions(-)
> 
> -- 
> 1.8.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/8] include/buffer.h: fix operator=

[Sage Weil]

On Tue, 5 Feb 2013, Danny Al-Gaaf wrote:
> Fix operator=: return "iterator&" instead of 'iterator'. Also set last_p and
> other.append_buffer and check if 'this' equals 'other' before set anything.
> 
> Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
> ---
>  src/include/buffer.h | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/include/buffer.h b/src/include/buffer.h
> index 9a635bd..448d947 100644
> --- a/src/include/buffer.h
> +++ b/src/include/buffer.h
> @@ -248,7 +248,7 @@ public:
>  					p(other.p),
>  					p_off(other.p_off) {}
>  
> -      iterator operator=(const iterator& other) {
> +      iterator& operator=(const iterator& other) {
>  	if (this != &other) {
>  	  bl = other.bl;
>  	  ls = other.ls;
> @@ -305,8 +305,12 @@ public:
>      
>      list(const list& other) : _buffers(other._buffers), _len(other._len), last_p(this) { }
>      list& operator= (const list& other) {
> -      _buffers = other._buffers;
> -      _len = other._len;
> +      if (this != &other) {
> +        _buffers = other._buffers;
> +        _len = other._len;
> +        last_p = other.last_p;
> +	append_buffer = other.append_buffer;

These two fields shouldn't be copied.  append_buffer is a private buffer 
we are writing into when we call the encode() helpers, and having two 
lists sharing it would allow them to clobber each other in hard-to-debug 
ways.  last_p is in the same category.

> +      }
>        return *this;
>      }
>  
> -- 
> 1.8.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 7/8] messages/MOSDRepScrub.h: initialize member variable in constructor

[Sage Weil]

On Tue, 5 Feb 2013, Danny Al-Gaaf wrote:
> Initialize chunky and deep bool member variables in the constructor
> with false.
> 
> Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
> ---
>  src/messages/MOSDRepScrub.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/messages/MOSDRepScrub.h b/src/messages/MOSDRepScrub.h
> index 2d3a66d..cd1de06 100644
> --- a/src/messages/MOSDRepScrub.h
> +++ b/src/messages/MOSDRepScrub.h
> @@ -36,7 +36,7 @@ struct MOSDRepScrub : public Message {
>    hobject_t end;         // upper bound of scrub, exclusive
>    bool deep;             // true if scrub should be deep
>  
> -  MOSDRepScrub() : Message(MSG_OSD_REP_SCRUB, HEAD_VERSION, COMPAT_VERSION) { }
> +  MOSDRepScrub() : Message(MSG_OSD_REP_SCRUB, HEAD_VERSION, COMPAT_VERSION) { chunky = false; deep = false; }

Let's use the C++ initalize syntax:

  MOSDRepScrub() : Message(MSG_OSD_REP_SCRUB, HEAD_VERSION, COMPAT_VERSION)
    : chunky(false), deep(false) {}

Just for consistency's sake!

sage

>    MOSDRepScrub(pg_t pgid, eversion_t scrub_from, eversion_t scrub_to,
>  	       epoch_t map_epoch)
>      : Message(MSG_OSD_REP_SCRUB, HEAD_VERSION, COMPAT_VERSION),
> -- 
> 1.8.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 0/8] fix some issues from cppchecker/clang++

[Sage Weil]

On Tue, 5 Feb 2013, Danny Al-Gaaf wrote:
> These patches fix some issues found by using cppchecker/clang++.

A couple comments, but otherwise these look good. 

Thanks, Danny!
sage


> 
> Danny Al-Gaaf (8):
>   WorkQueue.h: fix cast
>   ceph_crypto.cc: remove unused shutdown() outside crypto ifdef's
>   obj_bencher.cc: use vector instead of VLA's
>   include/buffer.h: fix operator=
>   include/types.h: change operator<< function parameter
>   include/xlist.h: fix C-style pointer casting
>   messages/MOSDRepScrub.h: initialize member variable in constructor
>   msg/Message.h: fix C-style pointer casting
> 
>  src/common/WorkQueue.h      |  4 ++--
>  src/common/ceph_crypto.cc   |  1 -
>  src/common/obj_bencher.cc   | 13 +++++++------
>  src/include/buffer.h        | 10 +++++++---
>  src/include/types.h         |  2 +-
>  src/include/xlist.h         |  6 +++---
>  src/messages/MOSDRepScrub.h |  2 +-
>  src/msg/Message.h           |  4 ++--
>  8 files changed, 23 insertions(+), 19 deletions(-)
> 
> -- 
> 1.8.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

[PATCH 1/8] WorkQueue.h: fix cast

[Danny Al-Gaaf]

Replace C-style pointer casting with correct static_cast<>().

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/common/WorkQueue.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/common/WorkQueue.h b/src/common/WorkQueue.h
index 9fb215b..b19a6a2 100644
--- a/src/common/WorkQueue.h
+++ b/src/common/WorkQueue.h
@@ -251,10 +251,10 @@ public:
       return (void *)_dequeue();
     }
     void _void_process(void *p, TPHandle &handle) {
-      _process((T *)p, handle);
+      _process(static_cast<T *>(p), handle);
     }
     void _void_process_finish(void *p) {
-      _process_finish((T *)p);
+      _process_finish(static_cast<T *>(p));
     }
 
   public:
-- 
1.8.1.2