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

mountinfo: deprecate PidMountInfo #47

Merged
merged 1 commit into from
Oct 6, 2020
Merged

Conversation

kolyshkin
Copy link
Collaborator

@kolyshkin kolyshkin commented Oct 2, 2020

Function PidMountInfo is not named correctly (it should be PID not Pid,
and we already have mountinfo in the package name so it's tautological),
and can not accept FilterFunc.

Besides, there are not too many users of this function (I was only able
to found one, see [1]) and users can switch to GetMountsFromReader.

Depreate this function. The plan is to remove it before v1.0.

[1] DataDog/datadog-agent@406d988

@thaJeztah
Copy link
Member

wrt naming; its also worth considering to take the package name itself into account, eg

mountinfo.ForPID(pid)
mountinfo.FromPID(pid)
mountinfo.Get(filter)

(typing on my phone, so just a quick blurb)

@kolyshkin
Copy link
Collaborator Author

I didn't want to rename everything :) as we'll have to convert all users and/or deprecate stuff.
This is more about ability to use filters and PID.

But it's a good idea to make names shorter. Let me think about it.

@thaJeztah
Copy link
Member

Containerd uses mount.PID() https://github.com/containerd/containerd/blob/master/mount/mountinfo_linux.go#L137

That said; have we checked if this function is used? ISTR in containerd it was not in use (need to double check), not sure if it's used in runc?

@kolyshkin
Copy link
Collaborator Author

One other way to proceed is to deprecate PidMountinfo, suggesting to use GetMountsFromReader with the first argument being an opened /proc/self/mountinfo. Maybe this would be the best way to go.

@kolyshkin
Copy link
Collaborator Author

have we checked if this function is used?

I think I've seen it used once but don't remember where it was :-\

For now I guess what I said above is the best way to go

@kolyshkin
Copy link
Collaborator Author

I think I've seen it used once but don't remember where it was :-\

Ah, here's the (only) user I found: DataDog/datadog-agent@406d988

@kolyshkin kolyshkin changed the title mountinfo: add GetMountsForPID, deprecate PidMountInfo mountinfo: deprecate PidMountInfo Oct 5, 2020
Function PidMountInfo is not named correctly (it should be PID not Pid,
and we already have mountinfo in the package name so it's tautological),
and can not accept FilterFunc.

Besides, there are not too many users of this function (I was only able
to found one, see [1]) and users can switch to GetMountsFromReader.

Depreate this function. The plan is to remove it before v1.0.

[1] DataDog/datadog-agent@406d98801434dedc87

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Collaborator Author

@thaJeztah I've change this PR to only deprecate PidMountInfo, no longer introducing a replacement for it. PTAL

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 95f2efb into moby:master Oct 6, 2020
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