Skip to content
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

Make GetMountsFromReader linux-only #38

Closed
wants to merge 1 commit into from

Conversation

thaJeztah
Copy link
Member

closes #35

GetMountsFromReader was a wrapper for parseInfoFile on Linux.
All other implementations do not parse mountinfo, so had to
stub out the implemetation and omit the actual reader.

(worse, a FreeBSD implementation was missing, causing compile on
that to fail).

This patch makes GetMountsFromReader linux-only, as it was added
to allow benchmarking the mountinfo parsing, which only makes
sense on Linux.

GetMountsFromReader was a wrapper for parseInfoFile on Linux.
All other implementations do not parse `mountinfo`, so had to
stub out the implemetation and omit the actual reader.

(worse, a FreeBSD implementation was missing, causing compile on
that to fail).

This patch makes GetMountsFromReader linux-only, as it was added
to allow benchmarking the `mountinfo` parsing, which only makes
sense on Linux.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
// reader provided, with an optional filter applied (use nil
// for no filter). This can be useful in tests or benchmarks
// that provide a fake mountinfo data.
func GetMountsFromReader(reader io.Reader, f FilterFunc) ([]*Info, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that GetMountsFromReader is effectively just an alias for parseInfoFile, should we just export parseInfoFile on Linux instead?

Looks like it's not used anywhere currently https://grep.app/search?q=mountinfo.GetMountsFromReader

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is used in a few places, so I'd rather not change the name. Surely we can just rename parseInfoFile to GetMountsFromReader, if this is what you mean

One such place is opencontainers/runc@999ff3f, and another is

Copy link
Collaborator

Choose a reason for hiding this comment

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

... and another is moby/moby#40694

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, it's not it. I don't remember why I added it but I definitely had a good reason to :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

But surely it makes sense to make it Linux-only

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please see #39

@thaJeztah
Copy link
Member Author

@kolyshkin ptal <3

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.

None yet

2 participants