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

mimic: include/intarith: enforce the same type for p2*() arguments #27318

Merged
merged 1 commit into from Apr 12, 2019

Conversation

Projects
None yet
3 participants
@ifed01
Copy link
Contributor

commented Apr 2, 2019

x and align must be of the same width. Otherwise the lack of
sign-extension can bite, e.g.:

uint64_t x = 128000;
uint32_t align = 32768;
uint64_t res = p2roundup(x, align);
// res = 18446744069414715392

While the templates added in commit c06b97b ("include: Add
templates along side macros in intarith") are technically correct,
P2*() macros weren't meant to be used with different types. The
Solaris kernel (which is where they originated) has P2*_TYPED()
variants for that.

Signed-off-by: Ilya Dryomov idryomov@gmail.com
(cherry picked from commit d0293dd)

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

@ifed01 ifed01 requested a review from idryomov Apr 2, 2019

@idryomov
Copy link
Contributor

left a comment

Don't drop "Conflicts" from the commit message -- it's important to know which commits had conflicts (even if trivial or contextual). Instead mention that modifications to those files aren't relevant.

@@ -255,8 +255,7 @@ double StupidAllocator::get_fragmentation(uint64_t alloc_unit)
uint64_t max_intervals = 0;
uint64_t intervals = 0;
{
std::lock_guard<std::mutex> l(lock);

This comment has been minimized.

Copy link
@idryomov

idryomov Apr 2, 2019

Contributor

Why is this lock_guard removed?

include/intarith: enforce the same type for p2*() arguments
x and align must be of the same width.  Otherwise the lack of
sign-extension can bite, e.g.:

  uint64_t x = 128000;
  uint32_t align = 32768;
  uint64_t res = p2roundup(x, align);
  // res = 18446744069414715392

While the templates added in commit c06b97b ("include: Add
templates along side macros in intarith") are technically correct,
P2*() macros weren't meant to be used with different types.  The
Solaris kernel (which is where they originated) has P2*_TYPED()
variants for that.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
(cherry picked from commit d0293dd)

 Conflicts:
	src/librbd/io/ImageRequest.cc
	src/msg/async/frames_v2.h
	src/os/bluestore/StupidAllocator.cc

@ifed01 ifed01 force-pushed the ifed01:wip-ifed-fix-p2-mimic branch from 4e9fa99 to 3244c87 Apr 2, 2019

@ifed01

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@idryomov - thanks for you comments. Resolved.

@ifed01 ifed01 added this to the mimic milestone Apr 2, 2019

@yuriw

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

what suite do you want to run on this ?

@idryomov

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

@yuriw Should be covered by rados (bluestore) and rbd suites.

@yuriw

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

@yuriw

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

@yuriw yuriw merged commit 5540932 into ceph:mimic Apr 12, 2019

4 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

@ifed01 ifed01 deleted the ifed01:wip-ifed-fix-p2-mimic branch Apr 12, 2019

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.