Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various fixes for SCA issues #21708

Merged
merged 10 commits into from Apr 30, 2018

Conversation

Projects
None yet
6 participants
@dalgaaf
Copy link
Member

commented Apr 27, 2018

  • use empty() over length() for emptiness check
  • use c++ over c-style casts
  • fixes types
  • prefer ++operators
  • don't compare unsigned to less than 0
  • reduce scope of variables

dalgaaf added some commits Apr 25, 2018

test/test_cors.cc: use empty() over length() for emptiness check
Fix for:
[src/test/test_cors.cc:117]: (style) Checking if unsigned
 variable 'host.length()' is less than zero.
[src/test/test_cors.cc:118]: (style) Checking if unsigned
 variable 'creds.length()' is less than zero.

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
crush/CrushWrapper: use cpp *_cast instead of c-style
Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
test/test_rgw_admin*: don't compare unsigend to less than 0
Make checks simpler, use empty() instead of 'length() <= 0'.

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
librbd/Journal.cc: use static_cast instead of c-style
Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
hypertable/CephBroker.cc: use int64_t instead of uint64_t
The usage of uint64_t to get the return value of ceph_lseek() will
lead to loss of the error code returned from the function. This will
break error case detection. Use a signed variable and cast before
offset returned instead.

Fix for:

[src/client/hypertable/CephBroker.cc:209]: (style) Checking if
 unsigned variable 'offset=ceph_lseek(cmount,fdata->fd,0,SEEK_CUR)' is less
 than zero.
[src/client/hypertable/CephBroker.cc:244]: (style) Checking if
 unsigned variable 'offset=(uint64_t)ceph_lseek(cmount,fdata->fd,0,SEEK_CUR)'
 is less than zero.

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
src/rgw/rgw_file.cc: fix lock_guard usage
Fix for:

[src/rgw/rgw_file.cc:255]: (style) Instance of 'lock_guard' object
 is destroyed immediately.

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
common/buffer.cc: prefer ++operators for non-primitive types
Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
common/TrackedOp: fix 'warned' handling
Fix for this cppcheck issue:

[src/common/TrackedOp.cc:385]: (style) The scope of the variable
 'warned' can be reduced.

Actually it seems that warned was not set correctly anymore
after rewrite of the code. Set 'warned' in with_slow_ops_in_flight()
as already done with number of slow iops.

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
client/Client.cc: reduce scope of variable 'r'
Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
test/perf_local.cc: reduce scope of some variables
Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
@@ -396,7 +398,7 @@ bool OpTracker::check_ops_in_flight(std::string* summary,
op.warn_interval_multiplier *= 2;
};
int slow = 0;
if (with_slow_ops_in_flight(&oldest_secs, &slow, warn_on_slow_op)) {
if (with_slow_ops_in_flight(&oldest_secs, &slow, &warned, warn_on_slow_op)) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Apr 28, 2018

Contributor

i'd say it's a bug fix.

This comment has been minimized.

Copy link
@dalgaaf

dalgaaf Apr 28, 2018

Author Member

Yes this one is, found it only because SCA told the scope of warned can be reduced.

This comment has been minimized.

Copy link
@smithfarm

smithfarm Apr 29, 2018

Contributor

bug was introduced post-luminous

@@ -252,7 +252,7 @@ namespace rgw {
int rc = rgwlib.get_fe()->execute_req(&req);
if ((rc == 0) &&
(req.get_ret() == 0)) {
lock_guard(rgw_fh->mtx);
lock_guard guard(rgw_fh->mtx);

This comment has been minimized.

Copy link
@cbodley

cbodley Apr 28, 2018

Contributor

also a bug fix 👍

@dillaman
Copy link
Contributor

left a comment

RBD lgtm

@tchaikov tchaikov merged commit e62bc6b into ceph:master Apr 30, 2018

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
@kashirin-alex

This comment has been minimized.

Copy link

commented on 7a17f59 Aug 21, 2018

Nice to know some are here with Hypertable FSBroker,

You are welcome to check-on a merged version to current version of Hypertable https://github.com/kashirin-alex/hypertable/tree/master/src/cc/FsBroker/ceph

I think, It is slightly not correct for the FsBroker to be a source of it's parent. (so as, main.cc is not included to build against Hypertable shared-libs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.