Prepare Bareos for an upgrade to the C++20 standard by alaaeddineelamri · Pull Request #1271 · bareos/bareos

alaaeddineelamri · 2022-10-05T11:10:44Z

Thank you for contributing to the Bareos Project!

Description

This PR will upgrade Bareos to use the C++20 standard.

Please check

Short description and the purpose of this PR is present above this paragraph
Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

PR name is meaningful
Purpose of the PR is understood
Commit descriptions are understandable and well formatted
Check backport line
Is the PR title usable as CHANGELOG entry?
Separate commit for CHANGELOG.md ("update CHANGELOG.md"). The PR number is correct.

Source code quality

Source code changes are understandable
Variable and function names are meaningful
Code comments are correct (logically and spelling)
Required documentation changes are present and part of the PR
bareos-check-sources --since-merge does not report any problems

core/src/include/jcr.h

arogge · 2022-10-12T13:56:24Z

core/src/tests/thread_list.cc

    status_ = Status::kWaiting;
-    bool success = cond_variable_.wait_for(ul, ms, [=]() { return notified; });
+    bool success
+        = cond_variable_.wait_for(ul, ms, [=, this]() { return notified; });


i guess you could just capture notified

creates this error:

bareos/core/src/tests/thread_list.cc:61: error: capture of non-variable ‘WaitCondition::notified’ bareos/core/src/tests/thread_list.cc: In member function ‘void WaitCondition::WaitFor(std::chrono::milliseconds)’: bareos/core/src/tests/thread_list.cc:61:19: error: capture of non-variable ‘WaitCondition::notified’

this must be captured.
capturing this->notified doesn't work either.

you're right, but you don't need to capture = then.

arogge · 2022-10-25T16:25:34Z

core/CMakeLists.txt

 # switch on CXX 17 Support
 #


Suggested change

# switch on CXX 17 Support

#

arogge · 2022-10-25T16:30:52Z

core/src/lib/bsock_tcp.cc

  timer_start = 0; /* clear timer */
  if (rc != pktsiz) {
-    errors++;
+    errors = errors + 1;


The atomic has working operator++()

Suggested change

errors = errors + 1;

++errors;

arogge · 2022-10-25T16:31:29Z

core/src/lib/bsock_tcp.cc

      b_errno = errno;
    }
-    errors++;
+    errors = errors + 1;


The atomic has working operator++()

Suggested change

errors = errors + 1;

++errors;

arogge · 2022-10-25T16:31:34Z

core/src/lib/bsock_tcp.cc

  timer_start = 0; /* clear timer */
  if (nbytes != header_length) {
-    errors++;
+    errors = errors + 1;


The atomic has working operator++()

Suggested change

errors = errors + 1;

++errors;

arogge · 2022-10-25T16:31:40Z

core/src/lib/bsock_tcp.cc

      b_errno = errno;
    }
-    errors++;
+    errors = errors + 1;


The atomic has working operator++()

Suggested change

errors = errors + 1;

++errors;

arogge · 2022-10-27T08:27:08Z

core/src/stored/vol_mgr.cc

  VolumeReservationItem* vol;
-  VolumeReservationItem emptyVol;

  vol = (VolumeReservationItem*)malloc(sizeof(VolumeReservationItem));
-  *vol = emptyVol;
+  vol = new (vol) VolumeReservationItem(); /* placement new instead of memset */
+


This is one of the places that we can actually improve a lot with C++20.
An obvious one is:

auto* vol = static_cast<VolumeReservationItem*>(malloc(sizeof(VolumeReservationItem))); std::construct_at(vol);

The two functions below FreeVolItem(), FreeReadVolItem() are identical, so we should probably remove the latter one.

Having said that, I think optimal code for this function would look like:

auto* vol = new VolumeReservationItem(VolumeName, dcr);

With all the setup and teardown moved into proper a ctor and dtor.

What is currently stopping us from doing that is that the volume list is a dlist<VolumeReservationItem> and that dlist<>::destroy() always uses free(n) to simply free the memory.
So if we extended dlist<T> to dlist<T, use_delete = false> and changed dlist<>::destroy() to honor that flag and use delete instead of free() we could then add a ctor/dtor to VolumeReservationItem, simplifying new_vol_item(), FreeVolItem(), FreeReadVolItem(), dup_vol_list() and FreeVolumeList() a lot.

However, after reading what I just wrote, this may be better suited for another cleanup PR.

I had somewhat of the same idea here at least in the sense of just using new and trying to replace the free with a delete somewhere or a destructor, but I ended up with the same conclusion that those changes would probably require more work than simply complying with c++20 standards

Certain compilers we use in our containers seem to not support std::construct_at

That's bad. std::construct_at() is part of C++20. If a compiler or library doesn't support it, we don't have real C++20 support there.
Maybe we can move to even newer compilers/libraries on the affected platforms?

I cannot find construct_at() as a feature on cppreference's compiler support page. However, there are a lot of features that require at least GCC 10 or even GCC 11.
At least for RHEL 7 and RHEL 8 we're on GCC 8 right now, which seems to be way too old for C++20.

arogge · 2022-10-27T08:30:26Z

core/src/stored/vol_mgr.h

+  void IncUseCount(void) { use_count_++; }
+  void DecUseCount(void) { use_count_--; }


Suggested change

void IncUseCount(void) { use_count_++; }

void DecUseCount(void) { use_count_--; }

void IncUseCount(void) { ++use_count_; }

void DecUseCount(void) { --use_count_; }

arogge · 2022-10-27T08:32:20Z

core/src/tests/thread_list.cc

    status_ = Status::kWaiting;
-    bool success = cond_variable_.wait_for(ul, ms, [=]() { return notified; });
+    bool success
+        = cond_variable_.wait_for(ul, ms, [=, this]() { return notified; });


you're right, but you don't need to capture = then.

arogge · 2022-10-27T08:51:05Z

core/src/stored/dev.h

  Device() = default;
  virtual ~Device();
-  Device* volatile swap_dev{}; /**< Swap vol from this device */
+  Device* swap_dev{}; /**< Swap vol from this device */


Hmm... that's a tough one. Basically, if the code ran on C++11 and later with volatile it should also run fine without volatile.
We could probably make this a std::atomic<Device*>, but I don't think that is required.

arogge · 2022-10-27T13:41:32Z

core/src/stored/device_control_record.h

  JobControlRecord* jcr{};         /**< Pointer to JobControlRecord */
  pthread_mutex_t mutex_ = PTHREAD_MUTEX_INITIALIZER;  /**< Access control */
  pthread_mutex_t r_mutex = PTHREAD_MUTEX_INITIALIZER; /**< rLock pre-mutex */
-  Device* volatile dev{};          /**< Pointer to device */


This does indeed produce different assembler code in some translation units. However, I don't see how not using volatile here should lead to an issue.

volmgr vol_mgr: use std::construct_at instead of placement new

replace problematic accesses to JobStatus with getter

Certain compilers we use in our containers seem to not support std::construct_at

C++20 supports designated initializers. However, it doesn't allow mixing of named and unnamed initialization. As we need to use the PyVarObject_HEAD_INIT() macro, which doesn't name the attributes it sets, we have to downgrade the compiler to C++17, so designated initializers are handled as compiler extension, which will allow the mixing that occurs here. See also: python/cpython#99202

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5285-upgrade-to-cpp20-standard branch 2 times, most recently from f234b52 to 4cbd561 Compare October 7, 2022 12:24

arogge reviewed Oct 12, 2022

View reviewed changes

core/src/include/jcr.h Show resolved Hide resolved

arogge reviewed Oct 12, 2022

View reviewed changes

arogge self-assigned this Oct 13, 2022

alaaeddineelamri marked this pull request as ready for review October 13, 2022 11:45

alaaeddineelamri marked this pull request as draft October 21, 2022 17:09

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5285-upgrade-to-cpp20-standard branch 2 times, most recently from 240ca7f to 642ecc3 Compare October 24, 2022 11:05

alaaeddineelamri marked this pull request as ready for review October 24, 2022 11:38

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5285-upgrade-to-cpp20-standard branch from 642ecc3 to 3c20a73 Compare October 25, 2022 12:24

alaaeddineelamri requested a review from arogge October 25, 2022 13:48

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5285-upgrade-to-cpp20-standard branch from 3c20a73 to 42b43e1 Compare October 25, 2022 15:59

arogge reviewed Oct 27, 2022

View reviewed changes

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5285-upgrade-to-cpp20-standard branch 2 times, most recently from 0861754 to b4cbcbf Compare October 28, 2022 17:41

alaaeddineelamri changed the title ~~Upgrade Bareos to use the C++20 standard~~ Prepare Bareos for an upgrade to the C++20 standard Nov 7, 2022

arogge force-pushed the dev/alaaeddineelamri/master/s5285-upgrade-to-cpp20-standard branch 3 times, most recently from dbef0a9 to bebe040 Compare November 7, 2022 14:36

arogge approved these changes Nov 7, 2022

View reviewed changes

arogge force-pushed the dev/alaaeddineelamri/master/s5285-upgrade-to-cpp20-standard branch 3 times, most recently from 442b2a4 to 7c74b46 Compare November 7, 2022 15:03

Alaa Eddine Elamri added 6 commits November 7, 2022 17:16

complying with the deprecation of implicit capture of this via [=]

d988217

windows: fix windows complaints

74ff88e

bsock: switch volatile to std::atomic

dedc7f8

jcr: use count volatile to atomic

5287fd9

jcr_private: SDJobStatus from volatile to atomic

26fe7f4

jcr_private: FDJobStatus from volatile to atomic

7f903c5

Alaa Eddine Elamri and others added 15 commits November 7, 2022 17:16

jcr_private: sd_msg_thread_done from volatile to atomic

dee4a25

device_control_record.h: remove volatile keyword from Device*

3aba6b1

replace volatile device pointers with actual variable

6c265ec

remove volatile from swap_device

098a464

vol_mgr: replace volatile with atomic for use_count_

ee9346d

volmgr vol_mgr: use std::construct_at instead of placement new

vol_mgr: remove duplicate FreeReadVolItem

f5a0286

remove reset jobstatus and replace with forcejobstatus

ed06265

rename setJobStatus to setJobStatusWithPriorityCheck

65d3ad1

rename forceJobStatus to setJobStatus

61b44e4

replace volatile jobstatus with atomic

d4c36b6

replace problematic accesses to JobStatus with getter

make JobStatus private and replace accesses with setter and getter

c94a9a9

use compare exchange before changing status

608f067

vol_mgr: replace construct_at with placement new

b1c6f2c

Certain compilers we use in our containers seem to not support std::construct_at

Updated CHANGELOG.md

3922775

arogge force-pushed the dev/alaaeddineelamri/master/s5285-upgrade-to-cpp20-standard branch from 7c74b46 to 3922775 Compare November 7, 2022 16:17

arogge merged commit 43dea81 into bareos:master Nov 7, 2022

arogge mentioned this pull request Jan 17, 2023

storage daemon: fix crash on volume swap #1356

Merged

17 tasks

alaaeddineelamri mentioned this pull request Jul 6, 2023

status storage: fix wrong data displayed about waiting jobs #1476

Merged

9 tasks

		void IncUseCount(void) { use_count_++; }
		void DecUseCount(void) { use_count_--; }

Conversation

alaaeddineelamri commented Oct 5, 2022 • edited by arogge Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thank you for contributing to the Bareos Project!

Description

Please check

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

Source code quality

Uh oh!

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!

alaaeddineelamri Oct 28, 2022 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alaaeddineelamri commented Oct 5, 2022 •

edited by arogge

Loading

alaaeddineelamri Oct 28, 2022 •

edited

Loading