[PATCH 00/15] fix more issues from clang(++) and cppchecker

[PATCH 00/15] fix more issues from clang(++) and cppchecker

[Danny Al-Gaaf]

Here another set of patches to fix issues found by clang/clang++ 
and cppchecker.

Danny Al-Gaaf (15):
  kv_flat_btree_async.cc: use vector instead of VLA's
  common/config.h: declaration of config_option as struct
  src/msg/msg_types.h: pass function parameter by reference
  fuse_ll.cc: fix -Wgnu-designator
  common/AsyncReserver.h: use empty() instead of size()
  common/WorkQueue.h: use empty() instead of size()
  common/config.cc: fix -Wgnu-designator
  src/log/Entry.h: pass function parameter by reference
  src/mon/PGMonitor.cc: remove unused variable
  src/msg/Messenger.h: pass function parameter by reference
  src/osd/OSD.h: use empty() instead of size()
  src/osd/PG.h: use empty() instead of size()
  src/osd/osd_types.h: pass function parameter by reference
  test_mon_workloadgen.cc: fix -Wgnu
  librados/librados.cc: fix implicitly-defined namespace 'std'

 src/client/fuse_ll.cc                      | 68 +++++++++++++++---------------
 src/common/AsyncReserver.h                 |  2 +-
 src/common/WorkQueue.h                     |  2 +-
 src/common/config.cc                       | 14 +++---
 src/common/config.h                        |  2 +-
 src/key_value_store/kv_flat_btree_async.cc | 14 +++---
 src/librados/librados.cc                   | 16 ++++++-
 src/log/Entry.h                            |  2 +-
 src/mon/PGMonitor.cc                       |  1 -
 src/msg/Messenger.h                        |  2 +-
 src/msg/msg_types.h                        |  2 +-
 src/osd/OSD.h                              |  4 +-
 src/osd/PG.h                               |  2 +-
 src/osd/osd_types.h                        |  2 +-
 src/test/mon/test_mon_workloadgen.cc       |  3 +-
 15 files changed, 74 insertions(+), 62 deletions(-)

-- 
1.8.1.2

[PATCH 01/15] kv_flat_btree_async.cc: use vector instead of VLA's

[Danny Al-Gaaf]

Fix "variable length array of non-POD element type" errors caused by
using librados::ObjectWriteOperation VLAs. (-Wvla)

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

diff --git a/src/key_value_store/kv_flat_btree_async.cc b/src/key_value_store/kv_flat_btree_async.cc
index 96c6cb0..4342e70 100644
--- a/src/key_value_store/kv_flat_btree_async.cc
+++ b/src/key_value_store/kv_flat_btree_async.cc
@@ -1119,9 +1119,9 @@ int KvFlatBtreeAsync::cleanup(const index_data &idata, const int &errno) {
     //all changes were created except for updating the index and possibly
     //deleting the objects. roll forward.
     vector<pair<pair<int, string>, librados::ObjectWriteOperation*> > ops;
-    librados::ObjectWriteOperation owos[idata.to_delete.size() + 1];
+    vector<librados::ObjectWriteOperation*> owos(idata.to_delete.size() + 1);
     for (int i = 0; i <= (int)idata.to_delete.size(); ++i) {
-      ops.push_back(make_pair(pair<int, string>(0, ""), &owos[i]));
+      ops.push_back(make_pair(pair<int, string>(0, ""), owos[i]));
     }
     set_up_ops(vector<object_data>(),
 	vector<object_data>(), &ops, idata, &err);
@@ -1883,23 +1883,23 @@ int KvFlatBtreeAsync::set_many(const map<string, bufferlist> &in_map) {
   to_create[to_create.size() - 1].max_kdata =
       to_delete[to_delete.size() - 1].max_kdata;
 
-  librados::ObjectWriteOperation owos[2 + 2 * to_delete.size()
-                                      + to_create.size()];
+  vector<librados::ObjectWriteOperation*> owos(2 + 2 * to_delete.size()
+                                      + to_create.size());
   vector<pair<pair<int, string>, librados::ObjectWriteOperation*> > ops;
 
 
   index_data idata;
-  set_up_prefix_index(to_create, to_delete, &owos[0], &idata, &err);
+  set_up_prefix_index(to_create, to_delete, owos[0], &idata, &err);
 
   if (verbose) cout << "finished making to_create and to_delete. "
       << std::endl;
 
   ops.push_back(make_pair(
       pair<int, string>(ADD_PREFIX, index_name),
-      &owos[0]));
+      owos[0]));
   for (int i = 1; i < 2 + 2 * (int)to_delete.size() + (int)to_create.size();
       i++) {
-    ops.push_back(make_pair(make_pair(0,""), &owos[i]));
+    ops.push_back(make_pair(make_pair(0,""), owos[i]));
   }
 
   set_up_ops(to_create, to_delete, &ops, idata, &err);
-- 
1.8.1.2

[PATCH 02/15] common/config.h: declaration of config_option as struct

[Danny Al-Gaaf]

Change declaration of config_option from 'class' to 'struct' since
it's defined as struct and used this way (access members). The declaration
as class doesn't change the behaviour.

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

diff --git a/src/common/config.h b/src/common/config.h
index 9bf04fe..cf397bb 100644
--- a/src/common/config.h
+++ b/src/common/config.h
@@ -33,7 +33,7 @@ extern struct ceph_file_layout g_default_file_layout;
 #define OSD_REP_SPLAY   1
 #define OSD_REP_CHAIN   2
 
-class config_option;
+struct config_option;
 class CephContext;
 
 extern const char *CEPH_CONF_FILE_DEFAULT;
-- 
1.8.1.2

[PATCH 03/15] src/msg/msg_types.h: pass function parameter by reference

[Danny Al-Gaaf]

Fix "Function parameter 'm' should be passed by reference." from cppchecker.

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

diff --git a/src/msg/msg_types.h b/src/msg/msg_types.h
index 8f3d74b..e80639e 100644
--- a/src/msg/msg_types.h
+++ b/src/msg/msg_types.h
@@ -142,7 +142,7 @@ inline std::ostream& operator<<(std::ostream& out, const ceph_entity_name& addr)
 namespace __gnu_cxx {
   template<> struct hash< entity_name_t >
   {
-    size_t operator()( const entity_name_t m ) const
+    size_t operator()( const entity_name_t &m ) const
     {
       return rjhash32(m.type() ^ m.num());
     }
-- 
1.8.1.2

[PATCH 04/15] fuse_ll.cc: fix -Wgnu-designator

[Danny Al-Gaaf]

Fix 'const static struct fuse_lowlevel_ops fuse_ll_oper' related
-Wgnu-designator warning from clang(++):

client/fuse_ll.cc:537:2: warning: use of GNU old-style field
  designator extension [-Wgnu-designator]

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/client/fuse_ll.cc | 68 +++++++++++++++++++++++++--------------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/src/client/fuse_ll.cc b/src/client/fuse_ll.cc
index 3e14bc0..a73442a 100644
--- a/src/client/fuse_ll.cc
+++ b/src/client/fuse_ll.cc
@@ -534,40 +534,40 @@ static void do_init(void *data, fuse_conn_info *bar)
 }
 
 const static struct fuse_lowlevel_ops fuse_ll_oper = {
- init: do_init,
- destroy: 0,
- lookup: fuse_ll_lookup,
- forget: fuse_ll_forget,
- getattr: fuse_ll_getattr,
- setattr: fuse_ll_setattr,
- readlink: fuse_ll_readlink,
- mknod: fuse_ll_mknod,
- mkdir: fuse_ll_mkdir,
- unlink: fuse_ll_unlink,
- rmdir: fuse_ll_rmdir,
- symlink: fuse_ll_symlink,
- rename: fuse_ll_rename,
- link: fuse_ll_link,
- open: fuse_ll_open,
- read: fuse_ll_read,
- write: fuse_ll_write,
- flush: fuse_ll_flush,
- release: fuse_ll_release,
- fsync: fuse_ll_fsync,
- opendir: fuse_ll_opendir,
- readdir: fuse_ll_readdir,
- releasedir: fuse_ll_releasedir,
- fsyncdir: 0,
- statfs: fuse_ll_statfs,
- setxattr: fuse_ll_setxattr,
- getxattr: fuse_ll_getxattr,
- listxattr: fuse_ll_listxattr,
- removexattr: fuse_ll_removexattr,
- access: 0,
- create: fuse_ll_create,
- getlk: 0,
- setlk: 0,
- bmap: 0
+ .init = do_init,
+ .destroy = 0,
+ .lookup = fuse_ll_lookup,
+ .forget = fuse_ll_forget,
+ .getattr = fuse_ll_getattr,
+ .setattr = fuse_ll_setattr,
+ .readlink = fuse_ll_readlink,
+ .mknod = fuse_ll_mknod,
+ .mkdir = fuse_ll_mkdir,
+ .unlink = fuse_ll_unlink,
+ .rmdir = fuse_ll_rmdir,
+ .symlink = fuse_ll_symlink,
+ .rename = fuse_ll_rename,
+ .link = fuse_ll_link,
+ .open = fuse_ll_open,
+ .read = fuse_ll_read,
+ .write = fuse_ll_write,
+ .flush = fuse_ll_flush,
+ .release = fuse_ll_release,
+ .fsync = fuse_ll_fsync,
+ .opendir = fuse_ll_opendir,
+ .readdir = fuse_ll_readdir,
+ .releasedir = fuse_ll_releasedir,
+ .fsyncdir = 0,
+ .statfs = fuse_ll_statfs,
+ .setxattr = fuse_ll_setxattr,
+ .getxattr = fuse_ll_getxattr,
+ .listxattr = fuse_ll_listxattr,
+ .removexattr = fuse_ll_removexattr,
+ .access = 0,
+ .create = fuse_ll_create,
+ .getlk = 0,
+ .setlk = 0,
+ .bmap = 0
 };
 
 
-- 
1.8.1.2

[PATCH 05/15] common/AsyncReserver.h: use empty() instead of size()

[Danny Al-Gaaf]

Fix warning for usage of queue.size() in do_queues(). Use empty()
since it should be prefered as it has, following the standard, a
constant time complexity regardless of the containter type. The
same is not guaranteed for size().

warning from cppchecker was:
[common/AsyncReserver.h:40]: (performance) Possible inefficient
  checking for 'queue' emptiness.

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

diff --git a/src/common/AsyncReserver.h b/src/common/AsyncReserver.h
index 8cc2258..638bfb3 100644
--- a/src/common/AsyncReserver.h
+++ b/src/common/AsyncReserver.h
@@ -37,7 +37,7 @@ class AsyncReserver {
 
   void do_queues() {
     while (in_progress.size() < max_allowed &&
-           queue.size()) {
+           !queue.empty()) {
       pair<T, Context*> p = queue.front();
       queue_pointers.erase(p.first);
       queue.pop_front();
-- 
1.8.1.2

[PATCH 06/15] common/WorkQueue.h: use empty() instead of size()

[Danny Al-Gaaf]

Fix warning for usage of out->size() in _void_dequeue(). Use empty()
since it should be prefered as it has, following the standard, a
constant time complexity regardless of the containter type. The
same is not guaranteed for size().

warning from cppchecker was:
[common/WorkQueue.h:97]: (performance) Possible inefficient
  checking for 'queue' emptiness.

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

diff --git a/src/common/WorkQueue.h b/src/common/WorkQueue.h
index b19a6a2..ced952c 100644
--- a/src/common/WorkQueue.h
+++ b/src/common/WorkQueue.h
@@ -94,7 +94,7 @@ public:
     void *_void_dequeue() {
       list<T*> *out(new list<T*>);
       _dequeue(out);
-      if (out->size()) {
+      if (!out->empty()) {
 	return (void *)out;
       } else {
 	delete out;
-- 
1.8.1.2

[PATCH 07/15] common/config.cc: fix -Wgnu-designator

[Danny Al-Gaaf]

Fix 'struct ceph_file_layout g_default_file_layout' related
-Wgnu-designator warning from clang(++):

common/config.cc:61:2: warning: use of GNU old-style field
  designator extension [-Wgnu-designator]

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

diff --git a/src/common/config.cc b/src/common/config.cc
index 87b74ed..384c4b3 100644
--- a/src/common/config.cc
+++ b/src/common/config.cc
@@ -58,13 +58,13 @@ const char *CEPH_CONF_FILE_DEFAULT = "/etc/ceph/$cluster.conf, ~/.ceph/$cluster.
 
 // file layouts
 struct ceph_file_layout g_default_file_layout = {
- fl_stripe_unit: init_le32(1<<22),
- fl_stripe_count: init_le32(1),
- fl_object_size: init_le32(1<<22),
- fl_cas_hash: init_le32(0),
- fl_object_stripe_unit: init_le32(0),
- fl_unused: init_le32(-1),
- fl_pg_pool : init_le32(-1),
+ .fl_stripe_unit = init_le32(1<<22),
+ .fl_stripe_count = init_le32(1),
+ .fl_object_size = init_le32(1<<22),
+ .fl_cas_hash = init_le32(0),
+ .fl_object_stripe_unit = init_le32(0),
+ .fl_unused = init_le32(-1),
+ .fl_pg_pool = init_le32(-1),
 };
 
 #define _STR(x) #x
-- 
1.8.1.2

[PATCH 08/15] src/log/Entry.h: pass function parameter by reference

[Danny Al-Gaaf]

Fix "(performance) Function parameter 's' should be passed by reference."
from cppchecker.

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

diff --git a/src/log/Entry.h b/src/log/Entry.h
index 7f6b149..7cdf116 100644
--- a/src/log/Entry.h
+++ b/src/log/Entry.h
@@ -40,7 +40,7 @@ struct Entry {
     }
   }
 
-  void set_str(const std::string s) {
+  void set_str(const std::string &s) {
     ostream os(&m_streambuf);
     os << s;
   }
-- 
1.8.1.2

[PATCH 09/15] src/mon/PGMonitor.cc: remove unused variable

[Danny Al-Gaaf]

Remove unused variable to fix:

mon/PGMonitor.cc:170:11: warning: unused variable 'now'
  [-Wunused-variable]

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

diff --git a/src/mon/PGMonitor.cc b/src/mon/PGMonitor.cc
index 7e9b83b..f34ffb8 100644
--- a/src/mon/PGMonitor.cc
+++ b/src/mon/PGMonitor.cc
@@ -167,7 +167,6 @@ void PGMonitor::update_from_paxos()
   } 
 
   // walk through incrementals
-  utime_t now(ceph_clock_now(g_ceph_context));
   while (paxosv > pg_map.version) {
     bufferlist bl;
     bool success = paxos->read(pg_map.version+1, bl);
-- 
1.8.1.2

[PATCH 10/15] src/msg/Messenger.h: pass function parameter by reference

[Danny Al-Gaaf]

Fix "(performance) Function parameter 'm' should be passed by reference."
from cppchecker.

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

diff --git a/src/msg/Messenger.h b/src/msg/Messenger.h
index b75e442..f47c2cf 100644
--- a/src/msg/Messenger.h
+++ b/src/msg/Messenger.h
@@ -184,7 +184,7 @@ public:
    *
    * @param m The name to set.
    */
-  void set_myname(const entity_name_t m) { my_inst.name = m; }
+  void set_myname(const entity_name_t& m) { my_inst.name = m; }
   /**
    * Set the unknown address components for this Messenger.
    * This is useful if the Messenger doesn't know its full address just by
-- 
1.8.1.2

[PATCH 11/15] src/osd/OSD.h: use empty() instead of size()

[Danny Al-Gaaf]

Fix warning for usage of *.size(). Use empty() since it should be
prefered as it has, following the standard, a constant time
complexity regardless of the containter type. The same is not
guaranteed for size().

warning from cppchecker was:
[osd/OSD.h:265]: (performance) Possible inefficient checking for
   'last_scrub_pg' emptiness.
[osd/OSD.h:274]: (performance) Possible inefficient checking for
   'last_scrub_pg' emptiness.

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

diff --git a/src/osd/OSD.h b/src/osd/OSD.h
index b411c17..03d78cc 100644
--- a/src/osd/OSD.h
+++ b/src/osd/OSD.h
@@ -262,7 +262,7 @@ public:
   }
   bool first_scrub_stamp(pair<utime_t, pg_t> *out) {
     Mutex::Locker l(sched_scrub_lock);
-    if (last_scrub_pg.size() == 0)
+    if (last_scrub_pg.empty())
       return false;
     set< pair<utime_t, pg_t> >::iterator iter = last_scrub_pg.begin();
     *out = *iter;
@@ -271,7 +271,7 @@ public:
   bool next_scrub_stamp(pair<utime_t, pg_t> next,
 			pair<utime_t, pg_t> *out) {
     Mutex::Locker l(sched_scrub_lock);
-    if (last_scrub_pg.size() == 0)
+    if (last_scrub_pg.empty())
       return false;
     set< pair<utime_t, pg_t> >::iterator iter = last_scrub_pg.lower_bound(next);
     if (iter == last_scrub_pg.end())
-- 
1.8.1.2

[PATCH 12/15] src/osd/PG.h: use empty() instead of size()

[Danny Al-Gaaf]

Fix warning for usage of objects.size(). Use empty() since it
should be prefered as it has, following the standard, a constant
time complexity regardless of the containter type. The same is not
guaranteed for size().

warning from cppchecker was:
[osd/PG.h:599]: (performance) Possible inefficient checking for
  'objects' emptiness.

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

diff --git a/src/osd/PG.h b/src/osd/PG.h
index ba80f81..3c95058 100644
--- a/src/osd/PG.h
+++ b/src/osd/PG.h
@@ -596,7 +596,7 @@ protected:
 
     /// Adjusts begin to the first object
     void trim() {
-      if (objects.size())
+      if (!objects.empty())
 	begin = objects.begin()->first;
       else
 	begin = end;
-- 
1.8.1.2

[PATCH 13/15] src/osd/osd_types.h: pass function parameter by reference

[Danny Al-Gaaf]

Fix "(performance) Function parameter 'e' should be passed by reference."
from cppchecker.

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

diff --git a/src/osd/osd_types.h b/src/osd/osd_types.h
index e068057..b193c2b 100644
--- a/src/osd/osd_types.h
+++ b/src/osd/osd_types.h
@@ -496,7 +496,7 @@ inline bool operator>(const eversion_t& l, const eversion_t& r) {
 inline bool operator>=(const eversion_t& l, const eversion_t& r) {
   return (l.epoch == r.epoch) ? (l.version >= r.version):(l.epoch >= r.epoch);
 }
-inline ostream& operator<<(ostream& out, const eversion_t e) {
+inline ostream& operator<<(ostream& out, const eversion_t& e) {
   return out << e.epoch << "'" << e.version;
 }
 
-- 
1.8.1.2

[PATCH 14/15] test_mon_workloadgen.cc: fix -Wgnu

[Danny Al-Gaaf]

Fix warning from clang(++):

 test/mon/test_mon_workloadgen.cc:311:23: warning: in-class
 initializer for static data member of type 'const double' is
 a GNU extension [-Wgnu]

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/test/mon/test_mon_workloadgen.cc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/test/mon/test_mon_workloadgen.cc b/src/test/mon/test_mon_workloadgen.cc
index 6c9d2bb..e86d285 100644
--- a/src/test/mon/test_mon_workloadgen.cc
+++ b/src/test/mon/test_mon_workloadgen.cc
@@ -308,7 +308,7 @@ class OSDStub : public TestStub
   boost::uniform_int<> mon_osd_rng;
 
   utime_t last_boot_attempt;
-  static const double STUB_BOOT_INTERVAL = 10.0;
+  static const double STUB_BOOT_INTERVAL;
 
 
  public:
@@ -902,6 +902,7 @@ class OSDStub : public TestStub
   }
 };
 
+double const OSDStub::STUB_BOOT_INTERVAL = 10.0;
 
 #undef dout_prefix
 #define dout_prefix *_dout << "main "
-- 
1.8.1.2

[PATCH 15/15] librados/librados.cc: fix implicitly-defined namespace 'std'

[Danny Al-Gaaf]

Fix warning from clang(++):

librados/librados.cc:15:17: warning: using directive refers to
 implicitly-defined namespace 'std'
using namespace std;
                ^
Include what we need and use the related classes.

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/librados/librados.cc | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/librados/librados.cc b/src/librados/librados.cc
index 0ac6eb4..5a81a26 100644
--- a/src/librados/librados.cc
+++ b/src/librados/librados.cc
@@ -12,8 +12,6 @@
  *
  */
 
-using namespace std;
-
 #include "common/config.h"
 #include "common/errno.h"
 #include "common/ceph_argparse.h"
@@ -27,6 +25,20 @@ using namespace std;
 #include "librados/PoolAsyncCompletionImpl.h"
 #include "librados/RadosClient.h"
 
+#include <string>
+#include <map>
+#include <set>
+#include <vector>
+#include <list>
+#include <stdexcept>
+
+using std::string;
+using std::map;
+using std::set;
+using std::vector;
+using std::list;
+using std::runtime_error;
+
 #define dout_subsys ceph_subsys_rados
 #undef dout_prefix
 #define dout_prefix *_dout << "librados: "
-- 
1.8.1.2

Re: [PATCH 01/15] kv_flat_btree_async.cc: use vector instead of VLA's

[Sage Weil]

On Thu, 7 Feb 2013, Danny Al-Gaaf wrote:
> Fix "variable length array of non-POD element type" errors caused by
> using librados::ObjectWriteOperation VLAs. (-Wvla)
> 
> Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
> ---
>  src/key_value_store/kv_flat_btree_async.cc | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/src/key_value_store/kv_flat_btree_async.cc b/src/key_value_store/kv_flat_btree_async.cc
> index 96c6cb0..4342e70 100644
> --- a/src/key_value_store/kv_flat_btree_async.cc
> +++ b/src/key_value_store/kv_flat_btree_async.cc
> @@ -1119,9 +1119,9 @@ int KvFlatBtreeAsync::cleanup(const index_data &idata, const int &errno) {
>      //all changes were created except for updating the index and possibly
>      //deleting the objects. roll forward.
>      vector<pair<pair<int, string>, librados::ObjectWriteOperation*> > ops;
> -    librados::ObjectWriteOperation owos[idata.to_delete.size() + 1];
> +    vector<librados::ObjectWriteOperation*> owos(idata.to_delete.size() + 1);

I haven't read much of the surrounding code, but from what is included 
here I don't think this is equivalent... these are just null pointers 
initially, and so

>      for (int i = 0; i <= (int)idata.to_delete.size(); ++i) {
> -      ops.push_back(make_pair(pair<int, string>(0, ""), &owos[i]));
> +      ops.push_back(make_pair(pair<int, string>(0, ""), owos[i]));

this doesn't do anything useful... owos[i] may as well be NULL.  Why not 
make it

vector<librados::ObjectWriteOperation> owos(...)

?

>      }
>      set_up_ops(vector<object_data>(),
>  	vector<object_data>(), &ops, idata, &err);
> @@ -1883,23 +1883,23 @@ int KvFlatBtreeAsync::set_many(const map<string, bufferlist> &in_map) {
>    to_create[to_create.size() - 1].max_kdata =
>        to_delete[to_delete.size() - 1].max_kdata;
>  
> -  librados::ObjectWriteOperation owos[2 + 2 * to_delete.size()
> -                                      + to_create.size()];
> +  vector<librados::ObjectWriteOperation*> owos(2 + 2 * to_delete.size()
> +                                      + to_create.size());

Same thing here...

>    vector<pair<pair<int, string>, librados::ObjectWriteOperation*> > ops;
>  
>  
>    index_data idata;
> -  set_up_prefix_index(to_create, to_delete, &owos[0], &idata, &err);
> +  set_up_prefix_index(to_create, to_delete, owos[0], &idata, &err);
>  
>    if (verbose) cout << "finished making to_create and to_delete. "
>        << std::endl;
>  
>    ops.push_back(make_pair(
>        pair<int, string>(ADD_PREFIX, index_name),
> -      &owos[0]));
> +      owos[0]));
>    for (int i = 1; i < 2 + 2 * (int)to_delete.size() + (int)to_create.size();
>        i++) {
> -    ops.push_back(make_pair(make_pair(0,""), &owos[i]));
> +    ops.push_back(make_pair(make_pair(0,""), owos[i]));
>    }
>  
>    set_up_ops(to_create, to_delete, &ops, idata, &err);
> -- 
> 1.8.1.2
> 
> 

Re: [PATCH 13/15] src/osd/osd_types.h: pass function parameter by reference

[Sage Weil]

On Thu, 7 Feb 2013, Danny Al-Gaaf wrote:
> Fix "(performance) Function parameter 'e' should be passed by reference."
> from cppchecker.

eversion_t is only 12-16 bytes (depending on alignment), so I'm not sure a 
pointer indirection (or whatever the compiler turns the & parameter into) 
is going to buy us anything.

> 
> Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
> ---
>  src/osd/osd_types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/osd/osd_types.h b/src/osd/osd_types.h
> index e068057..b193c2b 100644
> --- a/src/osd/osd_types.h
> +++ b/src/osd/osd_types.h
> @@ -496,7 +496,7 @@ inline bool operator>(const eversion_t& l, const eversion_t& r) {
>  inline bool operator>=(const eversion_t& l, const eversion_t& r) {
>    return (l.epoch == r.epoch) ? (l.version >= r.version):(l.epoch >= r.epoch);
>  }
> -inline ostream& operator<<(ostream& out, const eversion_t e) {
> +inline ostream& operator<<(ostream& out, const eversion_t& e) {
>    return out << e.epoch << "'" << e.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 00/15] fix more issues from clang(++) and cppchecker

[Sage Weil]

Hey Danny-

These look good, modulo those 2 comments.  Do you have a public git tree 
with these patches I can cherry-pick or pull from?  It's a bit faster than 
yanking them off the list for merge (although posting to the list for 
review is still good, of course).

Thanks!
sage


On Thu, 7 Feb 2013, Danny Al-Gaaf wrote:

> Here another set of patches to fix issues found by clang/clang++ 
> and cppchecker.
> 
> Danny Al-Gaaf (15):
>   kv_flat_btree_async.cc: use vector instead of VLA's
>   common/config.h: declaration of config_option as struct
>   src/msg/msg_types.h: pass function parameter by reference
>   fuse_ll.cc: fix -Wgnu-designator
>   common/AsyncReserver.h: use empty() instead of size()
>   common/WorkQueue.h: use empty() instead of size()
>   common/config.cc: fix -Wgnu-designator
>   src/log/Entry.h: pass function parameter by reference
>   src/mon/PGMonitor.cc: remove unused variable
>   src/msg/Messenger.h: pass function parameter by reference
>   src/osd/OSD.h: use empty() instead of size()
>   src/osd/PG.h: use empty() instead of size()
>   src/osd/osd_types.h: pass function parameter by reference
>   test_mon_workloadgen.cc: fix -Wgnu
>   librados/librados.cc: fix implicitly-defined namespace 'std'
> 
>  src/client/fuse_ll.cc                      | 68 +++++++++++++++---------------
>  src/common/AsyncReserver.h                 |  2 +-
>  src/common/WorkQueue.h                     |  2 +-
>  src/common/config.cc                       | 14 +++---
>  src/common/config.h                        |  2 +-
>  src/key_value_store/kv_flat_btree_async.cc | 14 +++---
>  src/librados/librados.cc                   | 16 ++++++-
>  src/log/Entry.h                            |  2 +-
>  src/mon/PGMonitor.cc                       |  1 -
>  src/msg/Messenger.h                        |  2 +-
>  src/msg/msg_types.h                        |  2 +-
>  src/osd/OSD.h                              |  4 +-
>  src/osd/PG.h                               |  2 +-
>  src/osd/osd_types.h                        |  2 +-
>  src/test/mon/test_mon_workloadgen.cc       |  3 +-
>  15 files changed, 74 insertions(+), 62 deletions(-)
> 
> -- 
> 1.8.1.2
> 
> 

Re: [PATCH 00/15] fix more issues from clang(++) and cppchecker

[Danny Al-Gaaf]

Am 10.02.2013 07:02, schrieb Sage Weil:
> Hey Danny-
> 
> These look good, modulo those 2 comments.  Do you have a public git tree 
> with these patches I can cherry-pick or pull from?  It's a bit faster than 
> yanking them off the list for merge (although posting to the list for 
> review is still good, of course).

Check:

https://github.com/dalgaaf/ceph.git

I've move the patches to these branches and filled a pull request:

wip-da-sca-memleaks (6 patches, incl. fixed version of patch 5)
-> https://github.com/ceph/ceph/pull/42

wip-da-sca-cppcheck-clang (13 patches, without patch 1 and 13)
-> https://github.com/ceph/ceph/pull/43

Danny

Re: [PATCH 01/15] kv_flat_btree_async.cc: use vector instead of VLA's

[Danny Al-Gaaf]

Am 10.02.2013 06:57, schrieb Sage Weil:
> On Thu, 7 Feb 2013, Danny Al-Gaaf wrote:
>> Fix "variable length array of non-POD element type" errors caused by
>> using librados::ObjectWriteOperation VLAs. (-Wvla)
>>
>> Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
>> ---
>>  src/key_value_store/kv_flat_btree_async.cc | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/key_value_store/kv_flat_btree_async.cc b/src/key_value_store/kv_flat_btree_async.cc
>> index 96c6cb0..4342e70 100644
>> --- a/src/key_value_store/kv_flat_btree_async.cc
>> +++ b/src/key_value_store/kv_flat_btree_async.cc
>> @@ -1119,9 +1119,9 @@ int KvFlatBtreeAsync::cleanup(const index_data &idata, const int &errno) {
>>      //all changes were created except for updating the index and possibly
>>      //deleting the objects. roll forward.
>>      vector<pair<pair<int, string>, librados::ObjectWriteOperation*> > ops;
>> -    librados::ObjectWriteOperation owos[idata.to_delete.size() + 1];
>> +    vector<librados::ObjectWriteOperation*> owos(idata.to_delete.size() + 1);
> 
> I haven't read much of the surrounding code, but from what is included 
> here I don't think this is equivalent... these are just null pointers 
> initially, and so
> 
>>      for (int i = 0; i <= (int)idata.to_delete.size(); ++i) {
>> -      ops.push_back(make_pair(pair<int, string>(0, ""), &owos[i]));
>> +      ops.push_back(make_pair(pair<int, string>(0, ""), owos[i]));
> 
> this doesn't do anything useful... owos[i] may as well be NULL.  Why not 
> make it
> 
> vector<librados::ObjectWriteOperation> owos(...)
> 
> ?

Because this would lead to a linker error:

kv_flat_btree_async.o: In function `void
std::__uninitialized_fill_n<false>::__uninit_fill_n<librados::ObjectWriteOperation*,
unsigned long,
librados::ObjectWriteOperation>(librados::ObjectWriteOperation*,
unsigned long, librados::ObjectWriteOperation const&)':
/usr/bin/../lib64/gcc/x86_64-suse-linux/4.7/../../../../include/c++/4.7/bits/stl_uninitialized.h:188:
undefined reference to
`librados::ObjectOperation::ObjectOperation(librados::ObjectOperation
const&)'
/usr/bin/../lib64/gcc/x86_64-suse-linux/4.7/../../../../include/c++/4.7/bits/stl_uninitialized.h:188:
undefined reference to
`librados::ObjectOperation::ObjectOperation(librados::ObjectOperation
const&)'


Because in src/include/rados/librados.hpp
librados::ObjectOperation::ObjectOperation(librados::ObjectOperation
const&) was is defined, but not implemented in the librados.cc.

Not sure if removing ObjectOperation(librados::ObjectOperation const&)
is the way to go here.

Danny

Re: [PATCH 01/15] kv_flat_btree_async.cc: use vector instead of VLA's

[Sage Weil]

On Mon, 11 Feb 2013, Danny Al-Gaaf wrote:
> Am 10.02.2013 06:57, schrieb Sage Weil:
> > On Thu, 7 Feb 2013, Danny Al-Gaaf wrote:
> >> Fix "variable length array of non-POD element type" errors caused by
> >> using librados::ObjectWriteOperation VLAs. (-Wvla)
> >>
> >> Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
> >> ---
> >>  src/key_value_store/kv_flat_btree_async.cc | 14 +++++++-------
> >>  1 file changed, 7 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/src/key_value_store/kv_flat_btree_async.cc b/src/key_value_store/kv_flat_btree_async.cc
> >> index 96c6cb0..4342e70 100644
> >> --- a/src/key_value_store/kv_flat_btree_async.cc
> >> +++ b/src/key_value_store/kv_flat_btree_async.cc
> >> @@ -1119,9 +1119,9 @@ int KvFlatBtreeAsync::cleanup(const index_data &idata, const int &errno) {
> >>      //all changes were created except for updating the index and possibly
> >>      //deleting the objects. roll forward.
> >>      vector<pair<pair<int, string>, librados::ObjectWriteOperation*> > ops;
> >> -    librados::ObjectWriteOperation owos[idata.to_delete.size() + 1];
> >> +    vector<librados::ObjectWriteOperation*> owos(idata.to_delete.size() + 1);
> > 
> > I haven't read much of the surrounding code, but from what is included 
> > here I don't think this is equivalent... these are just null pointers 
> > initially, and so
> > 
> >>      for (int i = 0; i <= (int)idata.to_delete.size(); ++i) {
> >> -      ops.push_back(make_pair(pair<int, string>(0, ""), &owos[i]));
> >> +      ops.push_back(make_pair(pair<int, string>(0, ""), owos[i]));
> > 
> > this doesn't do anything useful... owos[i] may as well be NULL.  Why not 
> > make it
> > 
> > vector<librados::ObjectWriteOperation> owos(...)
> > 
> > ?
> 
> Because this would lead to a linker error:
> 
> kv_flat_btree_async.o: In function `void
> std::__uninitialized_fill_n<false>::__uninit_fill_n<librados::ObjectWriteOperation*,
> unsigned long,
> librados::ObjectWriteOperation>(librados::ObjectWriteOperation*,
> unsigned long, librados::ObjectWriteOperation const&)':
> /usr/bin/../lib64/gcc/x86_64-suse-linux/4.7/../../../../include/c++/4.7/bits/stl_uninitialized.h:188:
> undefined reference to
> `librados::ObjectOperation::ObjectOperation(librados::ObjectOperation
> const&)'
> /usr/bin/../lib64/gcc/x86_64-suse-linux/4.7/../../../../include/c++/4.7/bits/stl_uninitialized.h:188:
> undefined reference to
> `librados::ObjectOperation::ObjectOperation(librados::ObjectOperation
> const&)'
> 
> 
> Because in src/include/rados/librados.hpp
> librados::ObjectOperation::ObjectOperation(librados::ObjectOperation
> const&) was is defined, but not implemented in the librados.cc.
> 
> Not sure if removing ObjectOperation(librados::ObjectOperation const&)
> is the way to go here.

Oh, I see... yeah, we shouldn't remove that.  Probably we should 
restructure the code to use a list<>, which doesn't require a copy 
constructor or assignment operator.

Note that this particular code shouldn't hold up the rest of the patches, 
since it's not being used by anything (yet!).

sage