framework for adding a filesystem to a pool / wrap internal members of dbus_context in Rc<RefCell<>> #102

trgill · 2016-10-20T20:37:36Z

added NotFound to ErrorEnum
added code to create track a filesystem associated with a pool in the SimEngine
added initial code for the filesystem interface (parameters)
added BTreeMap to track filesystems in the SimPool
changed internal memebers of dbus_context to be wrapped in a Rc<RefCell<>> to simplify borrowing issues
surpressed compiler warnings by adding _ to variable names in stubbed out prototypes
added stubs for pool interface

Signed-off-by: Todd Gill [email protected]

added NotFound to ErrorEnum added code to create track a filesystem associated with a pool in the SimEngine added initial code for the filesystem interface (parameters) added BTreeMap to track filesystems in the SimPool Signed-off-by: Todd Gill <[email protected]>

Having a Rc<RefCell<>> wrapper avoids having to borrow_mut() of the dbus_context - which simplifies acess to the mutable fields. Change dbus_context to implement Clone Signed-off-by: Todd Gill <[email protected]>

trgill · 2016-10-21T20:31:51Z

@agrover @mulkieran review please?

mulkieran

I can't really comment on the changes to the DbusContext. Other than the changes I've requested, it looks good to me.

mulkieran · 2016-10-21T20:44:48Z

src/sim_engine/pool.rs

+    fn create_filesystem(&mut self,
+                         _filesystem_name: &str,
+                         _mount_point: &str,
+                         _size: &str)


Change _size to a u64. This is certainly better than a string, although not ideal.

We do have a Sectors type, how about that?

We should take the opportunity to define exactly what Sectors represent in that case and why it is suitable or adequate for this method. Right now, our design document does not use the word "sector" at all.

It looks like we can get rid of SumSectors pretty soon, based on progress of rust-lang/rust#34529.

I am actually the world's leading expert in the representation of and computation with allocated address ranges for block storage, see: https://github.com/mulkieran/justbytes, and https://mojo.redhat.com/docs/DOC-1089158. (It's like being on the North Korean Olympic curling team). But, it would be a shame to waste my expertise in this area, if it would be useful.

@agrover ok if I change _size to u64 and we tackle this topic in another PR?

mulkieran · 2016-10-21T20:46:34Z

src/sim_engine/engine.rs

@@ -89,6 +89,15 @@ impl Engine for SimEngine {

        Ok(())
    }
+    fn get_pool(&mut self, name: &str) -> EngineResult<&mut Box<Pool>> {


Is it possible to drop the &mut?

no, because of the return type. I think we tried this and compiler caught it. It's returning a mutable reference into the Engine, so it needs to start with a mutable reference to the Engine.

I'm puzzled by the fact that it has to return a mutable reference to a Box.

But, the more I look, the more it looks like it has to be. OK.

trgill · 2016-10-21T21:04:25Z

@mulkieran am I good to merge after addressing those two issues? (I'll wait for agrover too)

agrover · 2016-10-21T22:20:04Z

👍

mulkieran · 2016-10-24T13:50:48Z

src/sim_engine/pool.rs

 use std::vec::Vec;

 use engine::EngineResult;
 use engine::Pool;

 use super::blockdev::SimDev;

+#[derive(Debug, Clone)]


Maybe you could break this currently tiny definition and implementation out to a separate file. It will get bigger.

ok if we do that on next PR?

mulkieran · 2016-10-24T13:52:41Z

src/dbus_api.rs

+    let object_path = m.path.get_name();
+    let return_message = message.method_return();
+
+    println!("Object Path {}", object_path);


Drop the println! for final commit.

mulkieran · 2016-10-24T13:54:30Z

src/dbus_api.rs

+
+    let pool_name = match dbus_context.pools.borrow_mut().get(&object_path.to_string()) {
+        Some(pool) => pool.clone(),
+        None => return Err(MethodErr::invalid_arg(&0)),


I think this should be a return message with an Err result on the dbus.

I changed to:

return Err(MethodErr::invalid_arg(&format!("object path not found {}", object_path.to_string())))

Ok?

mulkieran · 2016-10-24T13:56:57Z

src/dbus_api.rs

+        Err(x) => {
+            let (rc, rs) = engine_to_dbus_err(&x);
+            let (rc, rs) = code_to_message_items(rc, rs);
+            let entry = MessageItem::Struct(vec!["o".into(), "q".into(), "s".into()]);


I think it is necessary to use specific correct MessageItem constructors for the components of the message.

If the rc indicates an error, the client shouldn't look at the array contents, but I think you are right. I changed it to:

let entry = MessageItem::Struct(vec![MessageItem::ObjectPath(object_path.to_owned()), MessageItem::UInt16(0), MessageItem::Str("".to_string())]);

mulkieran · 2016-10-24T14:03:18Z

There are more changes I'm requesting. I realize now I could probably have done that as a second review.

changed filesystem size to u64 in the case of pool name lookup failure, format the return entry with MessageItem::x constructors Signed-off-by: Todd Gill <[email protected]>

when there is an error, return an empty Array of the proper type corrected stubs for rename, set_mountpoint and set_quota reformatted some code so rustfmt would complete Signed-off-by: Todd Gill <[email protected]>

Reformatted some code so that rustfmt would work Use vec![] for empty array Signed-off-by: Todd Gill <[email protected]>

trgill · 2016-10-25T21:12:24Z

PR review comments have been addressed.

Drop an unimplemented method

We have not yet stabilized the API, so make this clear in this document. Also, use 1/1/1.5/1.5 margins and update to LyX 2.3.0. fixes stratis-storage#102 Signed-off-by: Andy Grover <[email protected]>

trgill added the do not merge label Oct 20, 2016

trgill added 2 commits October 21, 2016 16:17

Make the internal memebers dbus_context of wrapped in Rc<RefCell<>>

8537796

Having a Rc<RefCell<>> wrapper avoids having to borrow_mut() of the dbus_context - which simplifies acess to the mutable fields. Change dbus_context to implement Clone Signed-off-by: Todd Gill <[email protected]>

trgill changed the title ~~Initial framework for adding a filesystem to a pool~~ framework for adding a filesystem to a pool / wrap internal members of dbus_context in Rc<RefCell<>> Oct 21, 2016

trgill removed the do not merge label Oct 21, 2016

mulkieran requested changes Oct 21, 2016

View reviewed changes

mulkieran reviewed Oct 24, 2016

View reviewed changes

trgill added 3 commits October 25, 2016 10:21

surpressed compiler warnings by adding _ to start of variable.

451ad21

changed filesystem size to u64 in the case of pool name lookup failure, format the return entry with MessageItem::x constructors Signed-off-by: Todd Gill <[email protected]>

add INTERNAL_ERROR to the ErrorEnum

f062eac

when there is an error, return an empty Array of the proper type corrected stubs for rename, set_mountpoint and set_quota reformatted some code so rustfmt would complete Signed-off-by: Todd Gill <[email protected]>

Corrected stub method names

9af1975

Reformatted some code so that rustfmt would work Use vec![] for empty array Signed-off-by: Todd Gill <[email protected]>

trgill merged commit cb0375f into stratis-storage:master Oct 25, 2016

trgill deleted the create_filesystem_in_pool branch January 19, 2017 19:51

mulkieran pushed a commit that referenced this pull request Oct 24, 2021

Merge pull request #102 from mulkieran/master-unimplemented

4a1a2ed

Drop an unimplemented method

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

framework for adding a filesystem to a pool / wrap internal members of dbus_context in Rc<RefCell<>> #102

framework for adding a filesystem to a pool / wrap internal members of dbus_context in Rc<RefCell<>> #102

trgill commented Oct 20, 2016 •

edited

Loading

trgill commented Oct 21, 2016

mulkieran left a comment

mulkieran Oct 21, 2016

agrover Oct 21, 2016

mulkieran Oct 24, 2016

trgill Oct 24, 2016

agrover Oct 24, 2016

mulkieran Oct 21, 2016

agrover Oct 21, 2016

mulkieran Oct 24, 2016

mulkieran Oct 24, 2016

trgill commented Oct 21, 2016

agrover commented Oct 21, 2016

mulkieran Oct 24, 2016

trgill Oct 25, 2016

mulkieran Oct 25, 2016

mulkieran Oct 24, 2016

trgill Oct 25, 2016

mulkieran Oct 24, 2016

trgill Oct 25, 2016

mulkieran Oct 24, 2016

trgill Oct 25, 2016

mulkieran commented Oct 24, 2016

trgill commented Oct 25, 2016

framework for adding a filesystem to a pool / wrap internal members of dbus_context in Rc<RefCell<>> #102

framework for adding a filesystem to a pool / wrap internal members of dbus_context in Rc<RefCell<>> #102

Conversation

trgill commented Oct 20, 2016 • edited Loading

trgill commented Oct 21, 2016

mulkieran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trgill commented Oct 21, 2016

agrover commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mulkieran commented Oct 24, 2016

trgill commented Oct 25, 2016

trgill commented Oct 20, 2016 •

edited

Loading