Move StaticHeader into its own file #1792

GuillaumeGomez · 2020-01-22T19:53:48Z

Part of #1573.

cc @mulkieran

stratis-bot · 2020-01-22T19:55:04Z

Test with Jenkins?

stratis-bot · 2020-01-22T19:57:02Z

Test with Jenkins?

stratis-bot · 2020-01-22T19:57:08Z

Test with Jenkins?

mulkieran · 2020-01-22T19:59:24Z

ok to test

mulkieran

Thanks for coming back! Generally it looks good; I found a few things I noted. Please also move the test method test_ownership into the static header test methods as it has become a pure StaticHeader test.

mulkieran · 2020-01-27T18:57:17Z

src/engine/strat_engine/backstore/metadata/mod.rs

    sizes::{BDAExtendedSize, BlockdevSize, MDADataSize},
+    static_header::{device_identifiers, disown_device, StaticHeader},


Please don't export StaticHeader...I think we may have to export eventually, but for now we get along great with just exporting device_identifiers and disown_device.

mulkieran · 2020-01-27T19:07:01Z

src/engine/strat_engine/backstore/metadata/bda.rs

 #[derive(Debug)]
 pub struct BDA {
    header: StaticHeader,
    regions: mda::MDARegions,
 }

 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
-enum MetadataLocation {
+pub enum MetadataLocation {


Please move the definition of this type into the static_header module and import it into the bda module. It defines the API of some static_header methods which some bda methods call, so that is the best place to put it.

GuillaumeGomez · 2020-01-28T21:20:27Z

Updated!

mulkieran · 2020-01-29T16:08:13Z

The Jenkins test failure duplicates the ones in Travis, just an unused import:

  Compiling libstratis v2.0.0 (/home/fedora/workspace/stratisd)
error: unused import: `StaticHeader`
   --> src/engine/strat_engine/backstore/metadata/bda.rs:151:69
    |
151 |         tests::random_static_header, tests::static_header_strategy, StaticHeader,
    |                                                                     ^^^^^^^^^^^^
    |
    = note: `-D unused-imports` implied by `-D unused`

GuillaumeGomez · 2020-01-29T16:12:21Z

Updated.

mulkieran

Other than the requested changes to the use directives, this all looks good to me! Please make the changes and squash into one commit and I'll put it up for final review.

mulkieran · 2020-01-29T17:14:43Z

src/engine/strat_engine/backstore/metadata/static_header.rs

+
+    use devicemapper::{Bytes, Sectors, IEC};
+
+    use super::super::sizes::{static_header_size, MDADataSize};


Please use absolute name, should be crate::engine::strat_engine::metadata::sizes::{static_header_size, MDADataSize};

mulkieran · 2020-01-29T17:18:31Z

src/engine/strat_engine/backstore/metadata/bda.rs


+    use super::super::sizes::static_header_size;


Please use crate name and merge the imports, as:

use crate::engine::strat_engine::metadata::{sizes::static_header_size, static_header::{tests::random_static_header, tests::static_header_strategy}}

But always leave the use super::*; separate in tests, which is the only place we use it.

GuillaumeGomez · 2020-01-29T22:09:25Z

Updated.

mulkieran · 2020-01-30T13:57:35Z

Whoops! It's not strat_engine::metadata, it's strat_engine::backstore::metadata.

GuillaumeGomez · 2020-01-30T14:02:54Z

Arf, I didn't double-check, my bad!

mulkieran

Whoops, correct import strat_engine::backstory::metadata still missing in one file. Also, please make sure to rebase to pull in latest Travis updates.

mulkieran

Just one more thing about the imports...otherwise it's ready to go.

src/engine/strat_engine/backstore/metadata/bda.rs

mulkieran · 2020-02-04T22:28:26Z

src/engine/strat_engine/backstore/metadata/static_header.rs

+
+    use devicemapper::{Bytes, Sectors, IEC};
+
+    use super::*;


Please reverse the order here as well.

GuillaumeGomez · 2020-02-05T09:24:59Z

Done as well!

mulkieran

@GuillaumeGomez Could you grab this commit 390eb43 and squash it onto your work? Then it should be entirely done, and we can move it into final review. Thanks!

GuillaumeGomez · 2020-02-05T19:03:52Z

Done!

mulkieran

Ok, looks good!

mulkieran

Whoops! Please add license header to newly created file.

GuillaumeGomez · 2020-02-06T14:41:02Z

Updated. :)

mulkieran

Ok!

mulkieran assigned GuillaumeGomez Jan 22, 2020

mulkieran requested changes Jan 27, 2020

View reviewed changes

GuillaumeGomez force-pushed the move-stratis-header branch from c01491a to 4ab9836 Compare January 28, 2020 21:20

GuillaumeGomez force-pushed the move-stratis-header branch from 4ab9836 to bbfa466 Compare January 28, 2020 21:27

GuillaumeGomez force-pushed the move-stratis-header branch from bbfa466 to 40dee0c Compare January 29, 2020 16:12

mulkieran requested changes Jan 29, 2020

View reviewed changes

GuillaumeGomez force-pushed the move-stratis-header branch from 40dee0c to ef01735 Compare January 29, 2020 22:09

GuillaumeGomez force-pushed the move-stratis-header branch from ef01735 to ed56589 Compare January 30, 2020 14:02

mulkieran requested changes Feb 4, 2020

View reviewed changes

GuillaumeGomez force-pushed the move-stratis-header branch 4 times, most recently from 88b658f to 398a285 Compare February 4, 2020 16:11

GuillaumeGomez force-pushed the move-stratis-header branch 3 times, most recently from 2161a98 to ae18dae Compare February 4, 2020 20:55

mulkieran requested changes Feb 4, 2020

View reviewed changes

GuillaumeGomez force-pushed the move-stratis-header branch from ae18dae to cccccdd Compare February 5, 2020 09:24

mulkieran requested changes Feb 5, 2020

View reviewed changes

GuillaumeGomez force-pushed the move-stratis-header branch from cccccdd to 545d94b Compare February 5, 2020 19:03

mulkieran requested review from bgurney-rh, drckeefe, jbaublitz and lleshchi February 5, 2020 20:26

mulkieran approved these changes Feb 5, 2020

View reviewed changes

lleshchi approved these changes Feb 5, 2020

View reviewed changes

bgurney-rh approved these changes Feb 5, 2020

View reviewed changes

drckeefe approved these changes Feb 5, 2020

View reviewed changes

mulkieran removed the request for review from jbaublitz February 6, 2020 14:34

mulkieran requested changes Feb 6, 2020

View reviewed changes

Move StaticHeader into its own file

faaa502

GuillaumeGomez force-pushed the move-stratis-header branch from 545d94b to faaa502 Compare February 6, 2020 14:40

mulkieran approved these changes Feb 6, 2020

View reviewed changes

mulkieran merged commit 78fbf7a into stratis-storage:develop Feb 6, 2020

GuillaumeGomez deleted the move-stratis-header branch February 6, 2020 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move StaticHeader into its own file #1792

Move StaticHeader into its own file #1792

GuillaumeGomez commented Jan 22, 2020

stratis-bot commented Jan 22, 2020

stratis-bot commented Jan 22, 2020

stratis-bot commented Jan 22, 2020

mulkieran commented Jan 22, 2020

mulkieran left a comment

mulkieran Jan 27, 2020

mulkieran Jan 27, 2020

GuillaumeGomez commented Jan 28, 2020

mulkieran commented Jan 29, 2020

GuillaumeGomez commented Jan 29, 2020

mulkieran left a comment

mulkieran Jan 29, 2020

mulkieran Jan 29, 2020

GuillaumeGomez commented Jan 29, 2020

mulkieran commented Jan 30, 2020

GuillaumeGomez commented Jan 30, 2020

mulkieran left a comment

mulkieran left a comment

mulkieran Feb 4, 2020

GuillaumeGomez commented Feb 5, 2020

mulkieran left a comment

GuillaumeGomez commented Feb 5, 2020

mulkieran left a comment

mulkieran left a comment

GuillaumeGomez commented Feb 6, 2020

mulkieran left a comment

		sizes::{BDAExtendedSize, BlockdevSize, MDADataSize},
		static_header::{device_identifiers, disown_device, StaticHeader},


		use devicemapper::{Bytes, Sectors, IEC};

		use super::super::sizes::{static_header_size, MDADataSize};

Move StaticHeader into its own file #1792

Move StaticHeader into its own file #1792

Conversation

GuillaumeGomez commented Jan 22, 2020

stratis-bot commented Jan 22, 2020

stratis-bot commented Jan 22, 2020

stratis-bot commented Jan 22, 2020

mulkieran commented Jan 22, 2020

mulkieran left a comment

Choose a reason for hiding this comment

mulkieran Jan 27, 2020

Choose a reason for hiding this comment

mulkieran Jan 27, 2020

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 28, 2020

mulkieran commented Jan 29, 2020

GuillaumeGomez commented Jan 29, 2020

mulkieran left a comment

Choose a reason for hiding this comment

mulkieran Jan 29, 2020

Choose a reason for hiding this comment

mulkieran Jan 29, 2020

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 29, 2020

mulkieran commented Jan 30, 2020

GuillaumeGomez commented Jan 30, 2020

mulkieran left a comment

Choose a reason for hiding this comment

mulkieran left a comment

Choose a reason for hiding this comment

mulkieran Feb 4, 2020

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 5, 2020

mulkieran left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 5, 2020

mulkieran left a comment

Choose a reason for hiding this comment

mulkieran left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 6, 2020

mulkieran left a comment

Choose a reason for hiding this comment