SCA fixes #18495

dalgaaf · 2017-10-23T21:42:20Z

No description provided.

tchaikov · 2017-10-24T04:38:53Z

src/rgw/rgw_op.cc

@@ -3831,10 +3831,6 @@ void RGWPostObj::execute()

    s->obj_size = ofs;

-    if (supplied_md5_b64 && strcmp(calc_md5, supplied_md5)) {


@dalgaaf seems the master has already fixed this issue. see https://github.com/ceph/ceph/blob/master/src/rgw/rgw_op.cc#L3586

No, it seem not to be fixed. you picked the wrong line: https://github.com/ceph/ceph/blob/master/src/rgw/rgw_op.cc#L3834

ahh, right. i was wrong.

tchaikov · 2017-10-24T04:43:17Z

src/compressor/lz4/LZ4Compressor.h

@@ -46,7 +46,7 @@ class LZ4Compressor : public Compressor {
      compressed_len = LZ4_compress_fast_continue(
        &lz4_stream, data, outptr.c_str()+pos, origin_len,
        outptr.length()-pos, 1);
-      if (compressed_len <= 0)
+      if (compressed_len = 0)


i think instead of using uint32_t, we should use int to hold the retval of LZ4_compress_fast_continue(), see https://github.com/lz4/lz4/blob/3d260f352293d5cddc8e7f7464b93308d47ed176/lib/lz4.h#L270

although the doc of this func reads

or 0 if there is an error (typically, compressed data cannot fit into 'dst')

if (compressed_len <= 0)

is more consistent with its sample and looks more correct. =)

Not sure why the LZ4 function returns int if the result can only be 0 or positive. I assume compressed_len is currently uint32_t because in compressed it's size_t

If you prefer " if (compressed_len <= 0)" then we should change the compressed_len var to int

@dalgaaf Do you mean "==" ?

Sorry, yes! It should be "=="

@dalgaaf yes, i prefer if (compressed_len <= 0). so it's more "complete" and so we can avoid implicit castings from signed to unsigned. but i have no strong feeling on this.

I assume compressed_len is currently uint32_t because in compressed it's size_t

yes, we can cast it to uint32_t when encoding it.

tchaikov · 2017-10-24T06:43:32Z

src/osd/OSDMap.cc

@@ -2068,7 +2068,8 @@ void OSDMap::_get_temp_osds(const pg_pool_t& pool, pg_t pg,

 void OSDMap::pg_to_raw_osds(pg_t pg, vector<int> *raw, int *primary) const
 {
-  *primary = -1;


the condition 'if(primary)' is redundant

@dalgaaf i checked all caller sites of OSDMap::pg_to_raw_osds(), and it turns out it is redundant.

I will then remove the redundant check

actually I change the check to do the same as in the next function

well, it's still redundant. and if primary is nullptr, it crashes just like the next function. so i'd say, it's not wrong, neither is it quite right.

Fix for: [src/rgw/rgw_op.cc:3829]: (error) Uninitialized variable: calc_md5 Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/compressor/lz4/LZ4Compressor.h:49]: (style) Checking if unsigned variable 'compressed_len' is less than zero. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/librbd/librbd.cc:3578]: (style) Checking if unsigned variable 'data_len' is less than zero. [src/librbd/librbd.cc:3799]: (style) Checking if unsigned variable 'data_len' is less than zero. [src/librbd/librbd.cc:3576]: (style) Checking if unsigned variable 'data_len' is less than zero. [src/librbd/librbd.cc:3796]: (style) Checking if unsigned variable 'data_len' is less than zero. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/kv/MemDB.h:59]: (warning) Member variable 'MemDB::m_total_bytes' is not initialized in the constructor. [src/kv/MemDB.h:59]: (warning) Member variable 'MemDB::m_allocated_bytes' is not initialized in the constructor. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/mon/MDSMonitor.cc:1717]: (performance) Inefficient usage of string::find() in condition; string::compare() would be faster. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/msg/async/AsyncConnection.cc:122]: (warning) Member variable 'AsyncConnection::msg_left' is not initialized in the constructor. [src/msg/async/AsyncConnection.cc:122]: (warning) Member variable 'AsyncConnection::cur_msg_size' is not initialized in the constructor. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/msg/async/Stack.cc:42]: (warning) %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/msg/async/dpdk/PacketUtil.h:162]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/rgw/rgw_admin.cc:5862]: (warning) Comparison of a boolean expression with an integer. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/rgw/rgw_admin.cc:4228]: (style) Redundant condition: If 'ret < 0', the comparison 'ret' is always true. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/include/CompatSet.h:112]: (warning) %lld in format string (no. 1) requires 'long long' but the argument type is 'unsigned long long'. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/librbd/Watcher.cc:131]: (style) Condition 'r>=0' is always true Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Use the same logic as in OSDMap::pg_to_raw_up(). Fix for: [src/osd/OSDMap.cc:2071] -> [src/osd/OSDMap.cc:2077]: (warning) Either the condition 'if(primary)' is redundant or there is possible null pointer dereference: primary. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/osd/osd_types.cc:439]: (warning) %d in format string (no. 1) requires 'int *' but the argument type is 'unsigned int *'. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/rgw/rgw_rados.cc:3752]: (performance) Inefficient usage of string::find() in condition; string::compare() would be faster. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/test/librbd/test_internal.cc:1054]: (warning) %ld in format string (no. 1) requires 'long' but the argument type is 'unsigned long'. [src/test/librbd/test_internal.cc:1054]: (warning) %ld in format string (no. 2) requires 'long' but the argument type is 'unsigned long'. [src/test/librbd/test_internal.cc:1063]: (warning) %ld in format string (no. 1) requires 'long' but the argument type is 'unsigned long'. [src/test/librbd/test_internal.cc:1073]: (warning) %ld in format string (no. 1) requires 'long' but the argument type is 'unsigned long'. [src/test/librbd/test_internal.cc:1114]: (warning) %ld in format string (no. 1) requires 'long' but the argument type is 'unsigned long'. [src/test/librbd/test_internal.cc:1121]: (warning) %ld in format string (no. 1) requires 'long' but the argument type is 'unsigned long'. [src/test/librbd/test_internal.cc:1129]: (warning) %ld in format string (no. 1) requires 'long' but the argument type is 'unsigned long'. [src/test/librbd/test_internal.cc:1196]: (warning) %ld in format string (no. 1) requires 'long' but the argument type is 'unsigned long'. [src/test/librbd/test_internal.cc:1196]: (warning) %ld in format string (no. 2) requires 'long' but the argument type is 'unsigned long'. [src/test/librbd/test_internal.cc:1205]: (warning) %ld in format string (no. 1) requires 'long' but the argument type is 'unsigned long'. [src/test/librbd/test_internal.cc:1215]: (warning) %ld in format string (no. 1) requires 'long' but the argument type is 'unsigned long'. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/test/librbd/test_librbd.cc:2610]: (warning) %lld in format string (no. 1) requires 'long long' but the argument type is 'unsigned long long'. [src/test/librbd/test_librbd.cc:2610]: (warning) %lld in format string (no. 2) requires 'long long' but the argument type is 'unsigned long long'. [src/test/librbd/test_librbd.cc:2610]: (warning) %lld in format string (no. 3) requires 'long long' but the argument type is 'unsigned long long'. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

liewegas · 2017-10-28T19:38:40Z

src/osd/OSDMap.cc

+  if (!pool) {
+    if (primary)
+      *primary = -1;
+    if (raw)


yeah, i think the if (Primary) and if (raw) lines should go away

tchaikov · 2017-10-31T04:04:05Z

tchaikov requested a review from dillaman October 24, 2017 04:33

tchaikov reviewed Oct 24, 2017

View reviewed changes

dalgaaf added 19 commits October 25, 2017 18:14

rgw/rgw_op.cc: fix wrong function order

4d2f838

Fix for: [src/rgw/rgw_op.cc:3829]: (error) Uninitialized variable: calc_md5 Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

LZ4Compressor.h: fix usage of unsigned var

f24a028

Fix for: [src/compressor/lz4/LZ4Compressor.h:49]: (style) Checking if unsigned variable 'compressed_len' is less than zero. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

MDSMonitor.cc: use string::compare()

3f1b2ad

Fix for: [src/mon/MDSMonitor.cc:1717]: (performance) Inefficient usage of string::find() in condition; string::compare() would be faster. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

async/Stack.cc: fix conversion specifier

c6e4ea9

Fix for: [src/msg/async/Stack.cc:42]: (warning) %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

dpdk/PacketUtil.h: remove consecutive break after abort()

14697d1

Fix for: [src/msg/async/dpdk/PacketUtil.h:162]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

rgw_admin.cc: do not compare int with boolean

d541914

Fix for: [src/rgw/rgw_admin.cc:5862]: (warning) Comparison of a boolean expression with an integer. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

rgw_admin.cc: remove redundant check

b2e9c8d

Fix for: [src/rgw/rgw_admin.cc:4228]: (style) Redundant condition: If 'ret < 0', the comparison 'ret' is always true. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Client.cc: fix format qualifiers

82c815a

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

CompatSet.h: fix format qualifier

f8cc3e2

Fix for: [src/include/CompatSet.h:112]: (warning) %lld in format string (no. 1) requires 'long long' but the argument type is 'unsigned long long'. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

librbd/Watcher.cc: remove redundant if()

f764f25

Fix for: [src/librbd/Watcher.cc:131]: (style) Condition 'r>=0' is always true Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

osd/osd_types.cc: fix sscanf format qualifiers to %u

ce0fa35

Fix for: [src/osd/osd_types.cc:439]: (warning) %d in format string (no. 1) requires 'int *' but the argument type is 'unsigned int *'. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

rgw_rados.cc: fix format qualifiers

e3f9fa9

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

rgw_rados.cc: use compare() instead of find()

e045962

Fix for: [src/rgw/rgw_rados.cc:3752]: (performance) Inefficient usage of string::find() in condition; string::compare() would be faster. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

dalgaaf force-pushed the wip-da-SCA-20171013 branch from db9ff38 to fc2f9bd Compare October 25, 2017 16:15

tchaikov added cleanup needs-qa labels Oct 25, 2017

liewegas reviewed Oct 28, 2017

View reviewed changes

tchaikov added the wip-kefu-testing label Oct 30, 2017

tchaikov approved these changes Oct 31, 2017

View reviewed changes

tchaikov merged commit c41da89 into master Oct 31, 2017

tchaikov deleted the wip-da-SCA-20171013 branch October 31, 2017 04:13

tchaikov mentioned this pull request Oct 31, 2017

osd/OSDMap: remove the unnecessary checks for null #18636

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCA fixes #18495

SCA fixes #18495

dalgaaf commented Oct 23, 2017

tchaikov Oct 24, 2017

dalgaaf Oct 24, 2017

tchaikov Oct 25, 2017

tchaikov Oct 24, 2017

tchaikov Oct 24, 2017

dalgaaf Oct 24, 2017

dalgaaf Oct 24, 2017

dzafman Oct 24, 2017

dalgaaf Oct 24, 2017

tchaikov Oct 25, 2017 •

edited

Loading

tchaikov Oct 24, 2017 •

edited

Loading

dalgaaf Oct 24, 2017

dalgaaf Oct 26, 2017

tchaikov Oct 26, 2017 •

edited

Loading

liewegas Oct 28, 2017

tchaikov commented Oct 31, 2017

		@@ -3831,10 +3831,6 @@ void RGWPostObj::execute()

		s->obj_size = ofs;

		if (supplied_md5_b64 && strcmp(calc_md5, supplied_md5)) {

SCA fixes #18495

SCA fixes #18495

Conversation

dalgaaf commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

tchaikov Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Oct 31, 2017

tchaikov Oct 25, 2017 •

edited

Loading

tchaikov Oct 24, 2017 •

edited

Loading

tchaikov Oct 26, 2017 •

edited

Loading