Warnings and Errors and Other Things by adamemerson · Pull Request #60986 · ceph/ceph

adamemerson · 2024-12-06T22:29:08Z

A cleanup

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
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 cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

yuvalif · 2024-12-07T13:27:45Z

src/common/ceph_argparse.cc

-  char tmp[strlen(first)+1];
-  dashes_to_underscores(first, tmp);
-  first = tmp;
+  std::vector<char> tmp(strlen(first) + 1);


if this PR isa about memory safety, maybe change to strnlen?

I'm not sure what the limit should be since they're null terminated strings on the command line. ARG_MAX?

(I guess _POSIX_ARG_MAX might be more portable, since ARG_MAX isn't defined everywhere. I could potentially call sysconf, too.)

src/common/config.cc

src/auth/Crypto.cc

src/os/bluestore/BlueStore.cc

idryomov · 2024-12-16T11:35:16Z

src/librbd/ImageCtx.h


  private:
+#if defined(__cpp_lib_atomic_shared_ptr)
+    std::atomic<std::shared_ptr<neorados::IOContext>> data_io_context;


The commit is titled "librbd: Fix atomic shared pointer situation", but there is no description. Can you explain what is being fixed? Given that atomic_*(&data_io_context) usage that was deprecated in C++20 remains, I'm not sure I see the point of this change.

My understanding is that the atomic_store specialization on shared_ptr per se was deprecated. (https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic), with the recommendation to replace it with atomic<shared_ptr>.

We can use std::atomic_store on any std::atomic<T>* (https://en.cppreference.com/w/cpp/atomic/atomic_store), essentially equivalent to the store member function.

GCC11 doesn't have the atomic<shared_ptr> specialization so we have to keep the regular shared_ptr for now until we can be rid of it, but we can use the new one in new compilers. (And based on the implementation note in CPPreference, the usual implementation of atomic_store<shared_ptr> sounds terrible and we probably want to not use it whenever possible.)

I'll put an explanation in the commit message.

src/librbd/librbd.cc

src/tools/rbd/Utils.cc

src/tools/rbd/action/Import.cc

src/test/librbd/test_librbd.cc

src/auth/Crypto.cc

src/blk/aio/aio.cc

github-actions · 2025-01-09T16:24:02Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

src/client/Client.cc

jmundack · 2025-01-10T14:08:38Z

@adamemerson - thanks for cleaning up all this!

chardan

Thanks for doing all of this work! I think the changes are really big improvements! Please double-check for situations with vector<T *> that things are getting destroyed in the way you expect, and consider std::string for cases where a character buffer is intended rather than some kind of binary-flavor data. (Alternatively, you can use a unique_ptr<> with a deleter, example included in notes.)

NOTE: please check to be sure they aren't just moving pointers around-- I may not have checked carefully enough (for which I apologize!), if they don't make copies please disregard.

src/auth/Crypto.cc

src/client/fuse_ll.cc

src/common/ceph_argparse.cc

src/test/librados_test_stub/LibradosTestStub.cc

src/test/librbd/test_librbd.cc

src/test/perf_local.cc

src/tools/rbd/Utils.cc

src/tools/rbd/action/Import.cc

src/common/ceph_argparse.cc

Since we have an obscenely wide supported version spread, we have to support the deprecated location, too. Signed-off-by: Adam Emerson <aemerson@redhat.com>

The `lruset` and `not_before_queue` tests require the error category from `buffer.cc`. Optimization removes any actual use of the category, so the link error only shows up when compiling `-O0`. Signed-off-by: Adam Emerson <aemerson@redhat.com>

To fix compile error on jammy Signed-off-by: Adam Emerson <aemerson@redhat.com>

Otherwise we hit a linker error when compiling with `-O0` due to the use of `buffer::error_category` not being optimized out. Signed-off-by: Adam Emerson <aemerson@redhat.com>

libstdc++ uses TBB to implement the execution library if it is available. If it's not present, we get a serial backend. Currently, we aren't getting link errors in most cases because the reference is optimized out, but when compiling with `-O0`, we hit a missing symbol. If we use more of the execution library, we'll reference TBB in ways that don't optimize out. As such, test if TBB is available. If so, link against it. Signed-off-by: Adam Emerson <aemerson@redhat.com>

Signed-off-by: Adam Emerson <aemerson@redhat.com>

In the first branch of the if statement `remainingBytes` may be more than the size of the `uint64_t`. Signed-off-by: Adam Emerson <aemerson@redhat.com>

chardan

+1! This is a big improvement, I think! I had a few comments about things you COULD do, but if this is passing Teuthology it's looking good to me, thank you for the hard work on it!

chardan · 2025-01-27T18:41:49Z

src/CMakeLists.txt

  list(APPEND EXTRALIBS gcov)
 endif(${ENABLE_COVERAGE})

+# libstdc++ uses TBB to implement <execution> if it is available,


chardan · 2025-01-27T18:49:36Z

src/auth/Crypto.cc

-  char buf[len];
-  hex2str(secret.c_str(), secret.length(), buf, len);
-  s = buf;
+  // Since we have to copy the result into the string anyway, we may


chardan · 2025-01-27T18:55:44Z

src/auth/Crypto.cc

+    s.resize(len);
+  }
+  len = hex2str(secret.c_str(), secret.length(), s.data(), len);
+  s.resize(len);


Consider making line 601 always happen? (Then, this and the if-statement can go away.)

chardan · 2025-01-27T18:58:39Z

src/client/fuse_ll.cc

 #define _CEPH_CLIENT_ID	"ceph.client_id"
 #endif

+static constexpr std::size_t sv_reserve = 1'024;


+1 for snazzy decimal literal

chardan · 2025-01-27T19:08:25Z

src/cls/otp/cls_otp.cc

-  char secret[seed.length()];
-  char *psecret = secret;
+  std::vector<char> secret(seed.length());
+  char *psecret = secret.data();


On line 295 there's a possibility of free() being called on this. Can the psecret pointer go away?

Oh... So... IF need_free gets set, psecret is re-assigned (line 282) and then free()ed.

chardan · 2025-01-27T19:35:02Z

src/rgw/rgw_asio_client.cc

+
+    std::string key{HTTP_};
+    key.reserve(name.size() + HTTP_.size());
+    std::transform(name.cbegin(), name.cend(),


Is this what that dash-to-underscore function we saw earlier does? Maybe replace the implementation with this..? Or make this a Named Function? It's also ok as-written-- +1 map... er... transform().

chardan · 2025-01-27T19:35:52Z

src/rgw/rgw_common.cc

+  std::string buf;
+  buf.reserve(orig.size());
+
+  std::transform(orig.begin(), orig.cend(),


Hm. Same thing, but tolower() instead of toupper(). I think this is still an improvement, but it does start my head-scratching!

chardan · 2025-01-27T19:37:43Z

src/rgw/rgw_cors.cc

-  return string(buf);
+  std::string buf;
+  buf.reserve(orig.size());
+  std::transform(orig.cbegin(), orig.cend(),


Consider also boost::string::to_lower().

chardan · 2025-01-27T19:42:10Z

src/rgw/rgw_rest.cc

+  mod.reserve(orig.size());
+  std::transform(orig.cbegin(), orig.cend(),
+		 std::back_inserter(mod),
+		 [](char c) { return char(toupper(underscise(c))); });


Nice! (Maybe apply to the other similar cases.)

chardan · 2025-01-27T19:43:31Z

src/rgw/rgw_rest_client.cc

+	  std::string_view src(tok, len);
+	  std::string key;
+	  key.reserve(len);
+	  std::transform(src.cbegin(), src.cend(),


Yeah, this pattern is definitely common enough that (I know you're probably tired of tinkering with this!) it would be good to see some kind of up/low-er_underscise() to have it in one place (strictly IMO though).

chardan · 2025-01-28T03:50:07Z

src/objclass/class_api.cc

-  char buf[size];
-  char tmp_dest[size + 4]; /* so that there's space for the extra '=' characters, and some */
+  std::vector<char> buf(size);
+  std::vector<char> tmp_dest(size + 4); /* so that there's space for the extra '=' characters, and some */


It seems odd that the dest-ptr would give us a size that we're going to ignore, though..?

chardan · 2025-01-28T03:50:52Z

src/objclass/class_api.cc

  }
  tmp_dest[ret] = '\0';
-  memcpy(dest, tmp_dest, size);
+  memcpy(dest, tmp_dest.data(), size);


It looks like it's ok if this truncates, then..?

github-actions · 2025-02-05T06:32:34Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

ronen-fr · 2025-03-16T11:14:19Z

Note: commit 7e3f243 should be backported to Squid, to handle https://tracker.ceph.com/issues/69659

adamemerson · 2025-04-03T23:22:24Z

Breaking these out into separate PRs so they can get approved/tested/merged individually

github-actions · 2025-06-03T00:06:55Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

github-actions · 2025-07-03T00:07:10Z

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

adamemerson requested review from a team as code owners December 6, 2024 22:29

github-actions bot added bluestore build/ops cephfs Ceph File System common core mon rbd rgw tests labels Dec 6, 2024

adamemerson force-pushed the wip-warnings-errors branch from 8655cb6 to eae028f Compare December 6, 2024 22:59

yuvalif reviewed Dec 7, 2024

View reviewed changes

src/common/config.cc Outdated Show resolved Hide resolved

cbodley reviewed Dec 10, 2024

View reviewed changes

src/auth/Crypto.cc Outdated Show resolved Hide resolved

ifed01 reviewed Dec 16, 2024

View reviewed changes

src/os/bluestore/BlueStore.cc Show resolved Hide resolved

idryomov reviewed Dec 16, 2024

View reviewed changes

github-actions bot added the needs-rebase label Jan 9, 2025

jmundack mentioned this pull request Jan 9, 2025

containerized build tools [V2] #59841

Merged

14 tasks

jmundack reviewed Jan 9, 2025

View reviewed changes

src/client/Client.cc Show resolved Hide resolved

chardan requested changes Jan 14, 2025

View reviewed changes

chardan reviewed Jan 14, 2025

View reviewed changes

src/common/ceph_argparse.cc Show resolved Hide resolved

adamemerson added 5 commits January 24, 2025 11:52

rgw: Fix deprecated amqp includes

6e0aca9

Since we have an obscenely wide supported version spread, we have to support the deprecated location, too. Signed-off-by: Adam Emerson <aemerson@redhat.com>

test: Add missing libraries

cad1e41

The `lruset` and `not_before_queue` tests require the error category from `buffer.cc`. Optimization removes any actual use of the category, so the link error only shows up when compiling `-O0`. Signed-off-by: Adam Emerson <aemerson@redhat.com>

test/test_ceph_timer: Timespan, not mono_time in comparison

c95235e

To fix compile error on jammy Signed-off-by: Adam Emerson <aemerson@redhat.com>

tests/mgr: Link unittest_mgr_ttlcache against ceph-common

14b24f1

Otherwise we hit a linker error when compiling with `-O0` due to the use of `buffer::error_category` not being optimized out. Signed-off-by: Adam Emerson <aemerson@redhat.com>

adamemerson added 17 commits January 24, 2025 16:55

test/crush: Remove VLAs

d02ad28

Signed-off-by: Adam Emerson <aemerson@redhat.com>

mds: Remove VLAs

72025b8

Signed-off-by: Adam Emerson <aemerson@redhat.com>

test/erasure-code: Remove VLAs

40ab31a

Signed-off-by: Adam Emerson <aemerson@redhat.com>

tools: Remove VLAs

227eb98

Signed-off-by: Adam Emerson <aemerson@redhat.com>

mon: Remove VLAs

77c5e00

Signed-off-by: Adam Emerson <aemerson@redhat.com>

objclass: Remove VLAs

4720166

Signed-off-by: Adam Emerson <aemerson@redhat.com>

test/libcephfs: Remove VLAs

890fc0f

Signed-off-by: Adam Emerson <aemerson@redhat.com>

test/librados: Remove VLAs

4c34dba

Signed-off-by: Adam Emerson <aemerson@redhat.com>

test: Remove VLAs

df06acb

Signed-off-by: Adam Emerson <aemerson@redhat.com>

erasure-code: Remove VLAs

dac0bf7

Signed-off-by: Adam Emerson <aemerson@redhat.com>

common/io_exerciser: Remove VLAs

b141ff1

Signed-off-by: Adam Emerson <aemerson@redhat.com>

test/radosstriper: Remove VLAs

aec6489

Signed-off-by: Adam Emerson <aemerson@redhat.com>

mgr: Remove VLAs

b361408

Signed-off-by: Adam Emerson <aemerson@redhat.com>

client: Remove VLAs

302691d

Signed-off-by: Adam Emerson <aemerson@redhat.com>

rbd: Remove VLAs

9f88811

Signed-off-by: Adam Emerson <aemerson@redhat.com>

build: Add VLA warning

1b895ce

Signed-off-by: Adam Emerson <aemerson@redhat.com>

common/io_exercise: Correct stringop_overflow warning

1d8aaf3

In the first branch of the if statement `remainingBytes` may be more than the size of the `uint64_t`. Signed-off-by: Adam Emerson <aemerson@redhat.com>

adamemerson force-pushed the wip-warnings-errors branch from 9c7a1af to 1d8aaf3 Compare January 24, 2025 21:55

chardan approved these changes Jan 27, 2025

View reviewed changes

chardan reviewed Jan 28, 2025

View reviewed changes

github-actions bot added the needs-rebase label Feb 5, 2025

adamemerson marked this pull request as draft April 3, 2025 23:21

github-actions bot added the stale label Jun 3, 2025

github-actions bot closed this Jul 3, 2025

Conversation

adamemerson commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 9, 2025

Uh oh!

Uh oh!

jmundack commented Jan 10, 2025

Uh oh!

chardan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chardan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 5, 2025

Uh oh!

ronen-fr commented Mar 16, 2025

adamemerson commented Dec 6, 2024 •

edited

Loading

chardan left a comment •

edited

Loading