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: docs nits #52

Merged
merged 2 commits into from
Nov 6, 2020
Merged

mountinfo: docs nits #52

merged 2 commits into from
Nov 6, 2020

Conversation

kolyshkin
Copy link
Collaborator

mountinfo: docs nits

  1. Add a "custom file" use case to GetMountsFromReader.

  2. Remove obsoleted old comment from parseMountTable.

mountinfo.Info: refresh doc

Refresh documentation about mountinfo.Info fields, based on the latest
proc(5) from man-pages v5.08.

Comment on lines +36 to +39
// Major and Minor are the major and the minor components of the Dev
// field of unix.Stat_t structure returned by unix.*Stat calls for
// files on this filesystem.
Major, Minor int
Copy link
Member

Choose a reason for hiding this comment

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

Curious; how does the documentation show these combined comments? Does it repeat the same doc for each of them, or does it also should them grouped?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. go doc shows it as is:
	// Major and Minor are the major and the minor components of the Dev
	// field of unix.Stat_t structure returned by unix.*Stat calls for
	// files on this filesystem.
	Major, Minor int
  1. go doc (a webserver) does exactly the same since it's a struct members:
type Info
Info reveals information about a particular mounted filesystem. This struct is populated from the content in the /proc/<pid>/mountinfo file.

type Info struct {
    // ID is a unique identifier of the mount (may be reused after umount).
    ID int

    // Parent is the ID of the parent mount (or of self for the root
    // of this mount namespace's mount tree).
    Parent int

    // Major and Minor are the major and the minor components of the Dev
    // field of unix.Stat_t structure returned by unix.*Stat calls for
    // files on this filesystem.
    Major, Minor int
...

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, had wires crossed and thought we were in a var() block 🤦

makes sense

Source string

// VFSOptions represents per super block options.
// VFSOptions is per-superblock options, comma-separated.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// VFSOptions is per-superblock options, comma-separated.
// VFSOptions are per-superblock options, comma-separated.

or

Suggested change
// VFSOptions is per-superblock options, comma-separated.
// VFSOptions represents per-superblock options, comma-separated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a native speaker, but VFSOptions is a (single) variable, so AFAICS using plural is wrong here, as we don't describe the "options", but a variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, if it would be plural, we'd say "VFSOptions represent". See, this feels wrong -- as we don't talk about options (plural), but a variable (singular).

Copy link
Member

Choose a reason for hiding this comment

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

we don't describe the "options", but a variable.
Also, if it would be plural, we'd say "VFSOptions represent". See, this feels wrong -- as we don't talk about options (plural), but a variable (singular).

Yes, you're right; it should be describing the variable, not the options in the variable.

Something feels "off" in using is here, and I think I just found the problem/solution; usually (in case of a "slice"); how about this:

Suggested change
// VFSOptions is per-superblock options, comma-separated.
// VFSOptions is a comma-separated list of per-superblock options.

Perhaps the per- could be dropped (?)

Suggested change
// VFSOptions is per-superblock options, comma-separated.
// VFSOptions is a comma-separated list of superblock options.

Mountpoint string

// Options represents mount-specific options.
// Options is per mount options, comma-separated.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Options is per mount options, comma-separated.
// Options are per mount options, comma-separated.

or perhaps

Suggested change
// Options is per mount options, comma-separated.
// Options represents per mount options, comma-separated.

or s/represents/stores/ or s/represents/holds/ ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like the term "represents" here (as it implies some kind of conversion).

I don't like "stores" or "holds", as this is a string, and yes, it stores some value but it's kinda obvious.

Brevity is good here.

Copy link
Member

Choose a reason for hiding this comment

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

see my other comment, I think we can use the same approach here;

Suggested change
// Options is per mount options, comma-separated.
// Options is a comma-separated list of per mount options.

Could perhaps omit the per here (?);

Suggested change
// Options is per mount options, comma-separated.
// Options is a comma-separated list of mount options.

@@ -12,7 +12,8 @@ import (
// GetMountsFromReader retrieves a list of mounts from the
// 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.
// that provide a fake mountinfo data, or when a source other
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// that provide a fake mountinfo data, or when a source other
// that provide fake mountinfo data, or when a source other

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, fixed!

1. Add a "custom file" use case to GetMountsFromReader.

2. Remove obsoleted old comment from parseMountTable.

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

@thaJeztah addressed your comments, PTAL

Source string

// VFSOptions represents per super block options.
// VFSOptions is per-superblock options, comma-separated.
Copy link
Member

Choose a reason for hiding this comment

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

we don't describe the "options", but a variable.
Also, if it would be plural, we'd say "VFSOptions represent". See, this feels wrong -- as we don't talk about options (plural), but a variable (singular).

Yes, you're right; it should be describing the variable, not the options in the variable.

Something feels "off" in using is here, and I think I just found the problem/solution; usually (in case of a "slice"); how about this:

Suggested change
// VFSOptions is per-superblock options, comma-separated.
// VFSOptions is a comma-separated list of per-superblock options.

Perhaps the per- could be dropped (?)

Suggested change
// VFSOptions is per-superblock options, comma-separated.
// VFSOptions is a comma-separated list of superblock options.

Mountpoint string

// Options represents mount-specific options.
// Options is per mount options, comma-separated.
Copy link
Member

Choose a reason for hiding this comment

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

see my other comment, I think we can use the same approach here;

Suggested change
// Options is per mount options, comma-separated.
// Options is a comma-separated list of per mount options.

Could perhaps omit the per here (?);

Suggested change
// Options is per mount options, comma-separated.
// Options is a comma-separated list of mount options.

Refresh documentation about mountinfo.Info fields, based on the latest
proc(5) from man-pages v5.08.

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

The wording ("per-superblock" and "per-mount") was taken from proc(5) man page, but I have changed it according to last review from @thaJeztah. PTAL

@thaJeztah
Copy link
Member

The wording ("per-superblock" and "per-mount") was taken from proc(5) man page

Ah! Didn't know. I guess removing it is ok (if we hear it's not clear we could always add back the per-)

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, thanks!

@thaJeztah thaJeztah merged commit 97292d9 into moby:master Nov 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.

2 participants