Skip to content

Export mountinfo::MountInfoParser#7

Closed
agrover wants to merge 1 commit into
tailhook:masterfrom
agrover:export-mountinfoparser
Closed

Export mountinfo::MountInfoParser#7
agrover wants to merge 1 commit into
tailhook:masterfrom
agrover:export-mountinfoparser

Conversation

@agrover
Copy link
Copy Markdown

@agrover agrover commented Mar 6, 2018

Hi, I'd like to use MountInfoParser and was hoping libmount could export it.

@tailhook
Copy link
Copy Markdown
Owner

tailhook commented Mar 6, 2018

I'm fine with this. However, I should take a closer look to review the public API before a release because we are going to maintain it for long time.

Your feedback is valuable for this thing. Do you find current API good? Does it solves your task?

@agrover
Copy link
Copy Markdown
Author

agrover commented Mar 6, 2018

BTW I actually extended another crate that parsed /proc/mount to also handle mountinfo here: stemjail/mnt-rs#4 but which unfortunately hasn't been picked up yet. BTW libmount wouldn't want to take that code because it's under a different license.

Some nice things there that libmount might want to adopt are small helper constructors that handle reading from /proc/self/mountinfo (or /proc/(pid)/mountinfo) without making the caller do this. Going along with that would be the need for the MountInfoParser to own the buffer instead of just having a reference. I started to work on this but wasn't sure how to handle io:Errors that would now be possible. mnt-rs had a From conversion for io:Error to its ParseError type, but libmount's ParseError is a little more specialized so that didn't seem like an obviously good way to go.

@tailhook
Copy link
Copy Markdown
Owner

tailhook commented Mar 6, 2018

Some nice things there that libmount might want to adopt are small helper constructors

Well, what's your use case? I believe mountinfo is used just for few "system level" things and it's not hard to write two lines open, read_to_end.

Going along with that would be the need for the MountInfoParser to own the buffer

You can't return reference borrowed from the internal buffer when using Iterator trait. So either get rid of iterator or return fully owned entry.

I don't think it's justified. read_to_end is easy and mount table should be too large to have memory constraints. On other hand, quick scanning over list of mountpoints is a common use case I think.


For what it worth error type is not a problem, we can turn it into an enum. And it should be public API too. (your PR omits it)

So it can be used.

Signed-off-by: Andy Grover <agrover@redhat.com>
@agrover agrover force-pushed the export-mountinfoparser branch from ddff38c to a23fde3 Compare March 6, 2018 22:33
@agrover
Copy link
Copy Markdown
Author

agrover commented Mar 7, 2018

OK, I was just comparing to what the other crate did, I'm fine with current impl, tbh.

BTW for how I'd like to use MountInfoParser, I have a prelim commit with the changes for our project: agrover/stratisd@cc294f8. (We also probably could be using libmount for mounting and unmounting since we need that too, but this commit doesn't yet.)

@agrover
Copy link
Copy Markdown
Author

agrover commented Mar 7, 2018

Updated PR to export ParseError as well.

@tailhook
Copy link
Copy Markdown
Owner

tailhook commented Mar 7, 2018

Okay, further investigation shows:

  1. MountPoint type should be exported (it's a result of an iteration)
  2. What do you think of just publishing a mountinfo module? This way Parser, ParseError and MountPoint would be conveniently exported together. And also it's clear that MountPoint does nothing for other libmount operations and just an entry in mountinfo file.

@agrover
Copy link
Copy Markdown
Author

agrover commented Mar 7, 2018

Both points makes sense and sound good to me.

@tailhook
Copy link
Copy Markdown
Owner

tailhook commented Mar 7, 2018

Okay. Done in the current master.

@agrover
Copy link
Copy Markdown
Author

agrover commented Mar 7, 2018

Great, thanks so much! I look forward to making use of this! btw please also update the checkbox on the main README feature list since mountinfo parsing is now supported 😃 😃 😃

@agrover agrover closed this Mar 7, 2018
@agrover agrover deleted the export-mountinfoparser branch March 7, 2018 23:24
@tailhook
Copy link
Copy Markdown
Owner

Released as v0.1.9 with no further changes (apart from docstrings).

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.

2 participants