Skip to content

Replace mnt-rs with libmount#1007

Merged
agrover merged 2 commits into
stratis-storage:masterfrom
tasleson:use_libmount
Jun 26, 2018
Merged

Replace mnt-rs with libmount#1007
agrover merged 2 commits into
stratis-storage:masterfrom
tasleson:use_libmount

Conversation

@tasleson
Copy link
Copy Markdown
Contributor

@tasleson tasleson commented Jun 21, 2018

The library mnt-rs utilzes /proc/mounts which contains inaccurate information and caused issue #695. After doing some research, it appears that /proc/self/mountinfo is the correct place to look for mount information, ref. https://bugzilla.redhat.com/show_bug.cgi?id=491924 . @agrover did a PR to utilize /proc/self/mountinfo to mnt-rs ref. stemjail/mnt-rs#4 , but that PR has stalled. Thus the decision was made to utilize libmount instead as it does utilize /proc/self/mountinfo.

@tasleson tasleson self-assigned this Jun 21, 2018
@tasleson tasleson added this to the Stratis 1.0 milestone Jun 21, 2018
@tasleson
Copy link
Copy Markdown
Contributor Author

Rebased

Ok(mount) => {
if mount.contains(&MountParam::Spec(search)) {
return Ok(Some(mount.file));
if mount.mount_source == *search {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we just want to use maj/min for comparison here, is all. Something like:

let major = self.thin_dev.device().major as u64;
let minor = self.thin_dev.device().minor as u64;
if mount.major == major && mount.minor == minor {

@tasleson tasleson force-pushed the use_libmount branch 2 times, most recently from 584b6d4 to 060326b Compare June 25, 2018 16:09
@agrover
Copy link
Copy Markdown
Contributor

agrover commented Jun 26, 2018

Why 060326b? Aren't mount.major and .minor already u64?

@tasleson
Copy link
Copy Markdown
Contributor Author

tasleson commented Jun 26, 2018

@agrover

Why 060326b? Aren't mount.major and .minor already u64?

pub struct Device {
    /// Device major number
    pub major: u32,
    /// Device minor number
    pub minor: u32,
}

Apparently devicemapper-0.21.0/src/device.rs has them as u32 and libmount has them as a c_ulong which can either be a u32 or a u64.

@tasleson
Copy link
Copy Markdown
Contributor Author

Fixes #820

@agrover
Copy link
Copy Markdown
Contributor

agrover commented Jun 26, 2018

ok thanks. One of THOSE situations.

👍 once Clippy is made happy.

Copy link
Copy Markdown
Member

@mulkieran mulkieran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'ld like an explanation in the PR message as to why this approach works and the previous didn't.

@agrover
Copy link
Copy Markdown
Contributor

agrover commented Jun 26, 2018

fwiw @tasleson you can find/copy the original problem description in #695.

@mulkieran
Copy link
Copy Markdown
Member

I already read #695. It didn't give me any idea of why this fixes the problem.

@tasleson
Copy link
Copy Markdown
Contributor Author

@mulkieran @agrover Please see updated PR message. Hopefully this clears things up.

@mulkieran
Copy link
Copy Markdown
Member

A 21 comment bz from 2009 in which the last entry is a promise to fix this once and for all in util-linux couldn't really be said to clear anything up :( But I guess it's a starting point for further research if necessary.

@mulkieran
Copy link
Copy Markdown
Member

I expect there will be some more minor clippy errors, but once those are fixed, I'm fine w/ it.

@mulkieran
Copy link
Copy Markdown
Member

Except that you might as well close this and start a new PR that squashes the commits that should be squashed, like the clippy fixes.

tasleson added 2 commits June 26, 2018 15:43
Signed-off-by: Tony Asleson <tasleson@redhat.com>
Addresses: stratis-storage#695

Signed-off-by: Tony Asleson <tasleson@redhat.com>
@tasleson tasleson dismissed mulkieran’s stale review June 26, 2018 20:45

Concerns have been addressed

@tasleson
Copy link
Copy Markdown
Contributor Author

Passing all checks, I squashed, should be able to merge when CI is done

@agrover agrover merged commit bf1d4b0 into stratis-storage:master Jun 26, 2018
@tasleson tasleson deleted the use_libmount branch September 7, 2018 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants