-
Notifications
You must be signed in to change notification settings - Fork 55
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
Store StaticHeader::pool_uuid and StaticHeader::dev_uuid into a new StratisIdentifiers type #1826
Store StaticHeader::pool_uuid and StaticHeader::dev_uuid into a new StratisIdentifiers type #1826
Conversation
Test with Jenkins? |
2 similar comments
Test with Jenkins? |
Test with Jenkins? |
ok to test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just four spots to be fixed, otherwise looks just right.
@@ -554,8 +584,8 @@ pub mod tests { | |||
fn static_header(ref sh1 in static_header_strategy()) { | |||
let buf = sh1.sigblock_to_buf(); | |||
let sh2 = StaticHeader::sigblock_from_buf(&buf).unwrap().unwrap(); | |||
prop_assert_eq!(sh1.pool_uuid, sh2.pool_uuid); | |||
prop_assert_eq!(sh1.dev_uuid, sh2.dev_uuid); | |||
prop_assert_eq!(sh1.identifiers.pool_uuid, sh2.identifiers.pool_uuid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well just use one equality check on the identifiers, since they implement Eq
.
); | ||
let sh_newer = | ||
StaticHeader::new(sh.identifiers.clone(), sh.mda_size, sh.blkdev_size, ts + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can drop the clone here.
@@ -431,7 +462,7 @@ pub mod tests { | |||
|
|||
prop_assert!(StaticHeader::setup(&mut buf) | |||
.unwrap() | |||
.map(|new_sh| new_sh.pool_uuid == sh.pool_uuid && new_sh.dev_uuid == sh.dev_uuid) | |||
.map(|new_sh| new_sh.identifiers.pool_uuid == sh.identifiers.pool_uuid && new_sh.identifiers.device_uuid == sh.identifiers.device_uuid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just check equality of identifiers here.
where | ||
F: Read + Seek + SyncAll, | ||
{ | ||
StaticHeader::setup(f).map(|sh| sh.map(|sh| (sh.pool_uuid, sh.dev_uuid))) | ||
StaticHeader::setup(f).map(|sh| sh.map(|sh| sh.identifiers.clone())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be necessary to clone here.
3fa0418
to
6572f64
Compare
Updated and rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more places where you can take advantage of the fact that StratisIdentifiers is copy.
sh.pool_uuid, | ||
sh.dev_uuid, | ||
let sh_older = StaticHeader::new( | ||
StratisIdentifiers::new(sh.identifiers.pool_uuid, sh.identifiers.device_uuid), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, you can just use sh.identifiers
like you do with the definition of sh_newer
below.
); | ||
let sh_newer = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rustfmt 1.40 would like this assignment to be all on one line.
@@ -184,8 +180,7 @@ mod tests { | |||
let mut buf = Cursor::new(vec![0; *sh.blkdev_size.sectors().bytes() as usize]); | |||
let mut bda = BDA::initialize( | |||
&mut buf, | |||
sh.pool_uuid, | |||
sh.dev_uuid, | |||
StratisIdentifiers::new(sh.identifiers.pool_uuid, sh.identifiers.device_uuid), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use sh.identifiers
here, just like above.
@@ -229,8 +224,7 @@ mod tests { | |||
let mut buf = Cursor::new(vec![0; buf_size]); | |||
let mut bda = BDA::initialize( | |||
&mut buf, | |||
sh.pool_uuid, | |||
sh.dev_uuid, | |||
StratisIdentifiers::new(sh.identifiers.pool_uuid, sh.identifiers.device_uuid), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use sh.identiifers
here, too.
6572f64
to
a0ea898
Compare
I'm relying on the CI here, sorry. ^^' Updated otherwise! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good! Please fix the format error and we can put it up for final review.
…tratisIdentifiers type
a0ea898
to
5735826
Compare
Format, my arch enemy! Updated haha. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Fixes #1794.