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

client,common,mgr,rbd: clang related cleanups #33657

Merged
merged 8 commits into from Mar 5, 2020
Merged

Conversation

tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Mar 2, 2020

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard backend
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

as the `*` operator of xlist::iterator always returns a copy not a
reference, so there is no point to use `auto&` here. and clang complains
at seeing this. so, use `&&` instead, to silence this warning.

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov tchaikov requested a review from wjwithagen March 2, 2020 13:07
@tchaikov tchaikov mentioned this pull request Mar 2, 2020
3 tasks
@tchaikov tchaikov requested a review from neha-ojha March 2, 2020 13:08
@tchaikov tchaikov added this to the octopus milestone Mar 2, 2020
@tchaikov tchaikov added Clang cleanup FreeBSD Modifications specific for (only) FreeBSD labels Mar 2, 2020
@wjwithagen
Copy link
Contributor

@tchaikov
I guess this got hidden behind too many compiler error abort.

/home/jenkins/workspace/ceph-master/src/common/blkdev.cc:948:17: error: out-of-line definition of 'get_int_property' does not match any declaration in 'BlkDev'
int64_t BlkDev::get_int_property(char* prop) const
                ^~~~~~~~~~~~~~~~
/home/jenkins/workspace/ceph-master/src/common/blkdev.h:80:34: note: type of 1st parameter of member declaration does not match definition ('const char *' vs 'char *')
  int64_t get_int_property(const char* prop) const;
                                 ^
1 error generated.

to initialize an array with desginators is a C99 feature, strictly
speaking we cannot use this in C++. so clang complains at seeing this.
in this change we use plain C string instead of using an lookup table
for these constants.

Signed-off-by: Kefu Chai <kchai@redhat.com>
this addresses the FTBFS like
```
home/jenkins/workspace/ceph-master/src/mgr/MgrCap.h:12:1: error: declaration conflicts with target of using declaration already in scope
class CephContext;
^
/home/jenkins/workspace/ceph-master/src/include/common_fwd.h:10:9: note: target of using declaration
  class CephContext;
        ^
/home/jenkins/workspace/ceph-master/src/include/common_fwd.h:22:24: note: using declaration
using TOPNSPC::common::CephContext;
                       ^
In file included from /home/jenkins/workspace/ceph-master/src/mgr/MgrCap.cc:24:
```

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov
Copy link
Contributor Author

tchaikov commented Mar 2, 2020

@wjwithagen fixed and repushed.

@wjwithagen

This comment has been minimized.

@tchaikov tchaikov requested a review from dillaman March 3, 2020 04:24
@tchaikov tchaikov added the rbd label Mar 3, 2020
@tchaikov
Copy link
Contributor Author

tchaikov commented Mar 3, 2020

@wjwithagen thanks for testing. i fixed the issue and repushed.

@dillaman hi Jason, i piggy backed a couple of cleanups in this PR. could you help take a look at them?

this addresses the FTBFS caused by

```
/home/jenkins/workspace/ceph-master/src/tools/rbd_mirror/PoolMetaCache.h:12:1: error: declaration conflicts with target of using declaration already in scope
struct CephContext;
^
/home/jenkins/workspace/ceph-master/src/include/common_fwd.h:10:9: note: target of using declaration
  class CephContext;
        ^
/home/jenkins/workspace/ceph-master/src/include/common_fwd.h:22:24: note: using declaration
using TOPNSPC::common::CephContext;
                       ^
In file included from /home/jenkins/workspace/ceph-master/src/tools/rbd_mirror/PoolMetaCache.cc:6:
/home/jenkins/workspace/ceph-master/src/tools/rbd_mirror/PoolMetaCache.h:19:17: error: reference to 'CephContext' is ambiguous
  PoolMetaCache(CephContext* cct)
                ^
```

Signed-off-by: Kefu Chai <kchai@redhat.com>
Copy link
Contributor

@wjwithagen wjwithagen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes my issues I had with #31041

clang complains as `include/common_fwd.h` declares in a different way

Signed-off-by: Kefu Chai <kchai@redhat.com>
this silences warning from clang-11, like

```
../src/test/rbd_mirror/test_mock_ImageReplayer.cc:322:26: warning: lambda capture 'mock_replayer' is not used [-Wunused-lambda-capture]
      .WillOnce(Invoke([&mock_replayer, r](Context* ctx) {
                        ~^~~~~~~~~~~~~~
```

Signed-off-by: Kefu Chai <kchai@redhat.com>
`TYPED_TEST_CASE` is deprecated. let's use `TYPED_TEST_SUITE` instead.

this silences the warning from gtest.

```
../src/test/rbd_mirror/test_ImageReplayer.cc:584:1: warning: 'TypedTestCaseIsDeprecated' is deprecated: TYPED_TEST_CASE is deprecated, please use TYPED_TEST_SUITE [-Wdeprecated-declarations]
TYPED_TEST_CASE(TestImageReplayer, TestImageReplayerTypes);
^
../src/googletest/googletest/include/gtest/gtest-typed-test.h:229:38: note: expanded from macro 'TYPED_TEST_CASE'
  static_assert(::testing::internal::TypedTestCaseIsDeprecated(), ""); \
                                     ^
../src/googletest/googletest/include/gtest/internal/gtest-internal.h:1246:1: note: 'TypedTestCaseIsDeprecated' has been explicitly marked deprecated here
GTEST_INTERNAL_DEPRECATED(
^
../src/googletest/googletest/include/gtest/internal/gtest-port.h:2216:59: note: expanded from macro 'GTEST_INTERNAL_DEPRECATED'
                                                          ^
```

Signed-off-by: Kefu Chai <kchai@redhat.com>
this silences the warning from clang-11, like

```
../src/librbd/image/RefreshRequest.cc:34:16: warning: unused variable 'MAX_METADATA_ITEMS' [-Wunused-const-variable]
const uint64_t MAX_METADATA_ITEMS = 128;
               ^
```

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov
Copy link
Contributor Author

tchaikov commented Mar 3, 2020

@dillaman i reverted the reordering changes and updated src/test/librbd/test_librbd.cc as well. PTAL.

Copy link

@dillaman dillaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm -- thx!

@tchaikov tchaikov requested a review from batrick March 3, 2020 13:17
@tchaikov tchaikov changed the title client,common,mgr: clang related cleanups client,common,mgr,rbd: clang related cleanups Mar 3, 2020
@dillaman
Copy link

dillaman commented Mar 3, 2020

jenkins test make check

@dillaman
Copy link

dillaman commented Mar 3, 2020

(failed in TestImageReplayer/1.Resync and a fix was recently merged to master)

@tchaikov
Copy link
Contributor Author

tchaikov commented Mar 3, 2020

jenkins test make check

@tchaikov
Copy link
Contributor Author

tchaikov commented Mar 4, 2020

@tchaikov tchaikov self-assigned this Mar 4, 2020
@wjwithagen
Copy link
Contributor

Since its "parent" PR is backported to Octopus, this one also needs to be backported.

@dillaman
Copy link

dillaman commented Mar 4, 2020

Since its "parent" PR is backported to Octopus, this one also needs to be backported.

The octopus branch will be reset to master — they haven’t truly been split yet.

@batrick
Copy link
Member

batrick commented Mar 5, 2020

Edit: after checking this further, I don't see how it could be related. https://tracker.ceph.com/issues/44430

Seeing this failure:

Valgrind: mds (UninitCondition), client (UninitCondition), mon (UninitCondition), osd (UninitCondition)
5 jobs: ['4824922', '4824877', '4824831', '4825012', '4824967']
suites intersection: ['centos_latest.yaml', 'clusters/fixed-2-ucephfs.yaml', 'conf/{client.yaml', 'fs/verify/{begin.yaml', 'mds.yaml', 'mon-debug.yaml', 'mon.yaml', 'mount/fuse.yaml', 'osd.yaml}', 'overrides/{frag_enable.yaml', 'tasks/cfuse_workunit_suites_fsstress.yaml', 'validater/valgrind.yaml}', 'whitelist_health.yaml', 'whitelist_wrongly_marked_down.yaml}']
suites union: ['centos_latest.yaml', 'clusters/fixed-2-ucephfs.yaml', 'conf/{client.yaml', 'fs/verify/{begin.yaml', 'mds.yaml', 'mon-debug.yaml', 'mon.yaml', 'mount/fuse.yaml', 'objectstore-ec/bluestore-bitmap.yaml', 'objectstore-ec/bluestore-comp-ec-root.yaml', 'objectstore-ec/bluestore-comp.yaml', 'objectstore-ec/bluestore-ec-root.yaml', 'objectstore-ec/filestore-xfs.yaml', 'osd.yaml}', 'overrides/{frag_enable.yaml', 'tasks/cfuse_workunit_suites_fsstress.yaml', 'validater/valgrind.yaml}', 'whitelist_health.yaml', 'whitelist_wrongly_marked_down.yaml}']

http://pulpito.ceph.com/pdonnell-2020-03-04_14:24:35-fs-wip-pdonnell-testing-20200303.175301-distro-basic-smithi/

This PR is the likely cause.

@tchaikov tchaikov merged commit 539c078 into ceph:master Mar 5, 2020
@tchaikov tchaikov deleted the wip-clang branch March 5, 2020 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clang cleanup FreeBSD Modifications specific for (only) FreeBSD needs-qa rbd
Projects
None yet
5 participants