Conversation
9fa5b9a to
3365637
Compare
3365637 to
9df4967
Compare
fcb0815 to
87f514e
Compare
|
changelog:
|
8542569 to
da338a3
Compare
|
A lot of work was invested in this! |
da338a3 to
da7437d
Compare
d29aed8 to
34056c7
Compare
c6204ef to
cfc6598
Compare
cfc6598 to
ab1e089
Compare
ab1e089 to
f8c1528
Compare
… instead of pointers Problem: The current `generate_test_instance()` function returns `std::list<T*>`, which creates memory management issues: - Inconsistent lifecycle management of test instances - Callers don't always properly clean up allocated memory - Results in ASan memory leak reports in unit tests and ceph-dencoder Solution: Change `generate_test_instance()` to return `std::list<T>` instead of `std::list<T*>`: Core Changes: - Modified all classes with `generate_test_instance()` to return `std::list<T>` - Use `emplace_back()` without parameters** to avoid copy/move constructors for classes that don't define them - Updated ceph-dencoder to handle the new return type ceph-dencoder Adaptations: Since `m_list` now holds `T` objects instead of `T*`, and we can't assume `T` is copyable/moveable: - Keep `m_object` as a pointer for flexibility - Handle two scenarios: 1. `m_object` points to an element in `m_list` 2. `m_object` points to a decoded instance (requires manual cleanup) - Introduce `make_ptr()` as a factory function to create a smart pointer to conditionally free the managed pointer. Additional Cleanup: - Simplify DencoderBase constructor from template to plain function (extra parameters were never used in derived classes) With this change, object lifecycles are now managed by value semantics instead of raw pointers, eliminating memory leaks. Signed-off-by: Kefu Chai <tchaikov@gmail.com>
f8c1528 to
ed6b712
Compare
this shows that all of the encodings registered with ceph-dencoder are still working 👍 while this is a massive change set, i don't think it necessarily needs to go through all of the qa suites. but if anyone disagrees, we can ask Yuri to coordinate the testing |
| o.push_back(new cls_rbd_snap{}); | ||
| static std::list<cls_rbd_snap> generate_test_instances() { | ||
| std::list<cls_rbd_snap> o; | ||
| o.emplace_back(); |
There was a problem hiding this comment.
Is there a reason push_back() is used to invoke the default constructor in pretty much all other places (e.g. o.push_back(cls_rbd_parent{}); above)?
Separately, now that these "test instance" objects are no longer expected to be allocated with new, have you considered switching from push_back() to emplace_back() everywhere?
There was a problem hiding this comment.
Is there a reason
push_back()is used to invoke the default constructor in pretty much all other places (e.g.o.push_back(cls_rbd_parent{});above)?
hi @idryomov , No specific reason for the inconsistency. I made these changes in multiple passes - initially taking a conservative approach, then becoming more aggressive in later passes since this is test code where such changes carry lower risk.
Separately, now that these "test instance" objects are no longer expected to be allocated with
new, have you considered switching frompush_back()toemplace_back()everywhere?
Yes, I did consider this. Given my current bandwidth constraints, I'm prioritizing first completing the remaining changes in this commit's scope. The push_back() to emplace_back() conversion would make a good follow-up improvement.
| cls::rbd::UserSnapshotNamespace{}, 543, {}}); | ||
| o.push_back(cls_rbd_snap{1, "snap", 123456, | ||
| RBD_PROTECTION_STATUS_PROTECTED, cls_rbd_parent{}, 31, utime_t{}, | ||
| cls::rbd::UserSnapshotNamespace{}, 543, std::optional<uint64_t>{0}}); |
There was a problem hiding this comment.
Was there something that required spelling out utime_t{} and std::optional<uint64_t>{0} for this object but not for the previous or next object in this method?
There was a problem hiding this comment.
no specific reasons. it was a leftover. should be addressed by #65227
Problem:
The current
generate_test_instance()function returnsstd::list<T*>, which creates memory management issues:Solution:
Change
generate_test_instance()to returnstd::list<T>instead ofstd::list<T*>:Core Changes:
generate_test_instance()to returnstd::list<T>emplace_back()without parameters** to avoid copy/move constructors for classes that don't define themceph-dencoder Adaptations:
Since
m_listnow holdsTobjects instead ofT*, and we can't assumeTis copyable/moveable:m_objectas a pointer for flexibilitym_objectpoints to an element inm_listm_objectpoints to a decoded instance (requires manual cleanup)is_ownedflag to track when manual cleanup is neededAdditional Cleanup:
With this change, object lifecycles are now managed by value semantics instead of raw pointers, eliminating memory leaks.
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
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.Checklist
Show available Jenkins commands
jenkins test classic perfJenkins Job | Jenkins Job Definitionjenkins test crimson perfJenkins Job | Jenkins Job Definitionjenkins test signedJenkins Job | Jenkins Job Definitionjenkins test make checkJenkins Job | Jenkins Job Definitionjenkins test make check arm64Jenkins Job | Jenkins Job Definitionjenkins test submodulesJenkins Job | Jenkins Job Definitionjenkins test dashboardJenkins Job | Jenkins Job Definitionjenkins test dashboard cephadmJenkins Job | Jenkins Job Definitionjenkins test apiJenkins Job | Jenkins Job Definitionjenkins test docsReadTheDocs | Github Workflow Definitionjenkins test ceph-volume allJenkins Jobs | Jenkins Jobs Definitionjenkins test windowsJenkins Job | Jenkins Job Definitionjenkins test rook e2eJenkins Job | Jenkins Job Definition