Skip to content

opts/mount: add tmpfs-specific options#28150

Merged
vieux merged 1 commit intomoby:masterfrom
AkihiroSuda:mounttmpcli
Nov 10, 2016
Merged

opts/mount: add tmpfs-specific options#28150
vieux merged 1 commit intomoby:masterfrom
AkihiroSuda:mounttmpcli

Conversation

@AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Nov 8, 2016

- What I did

added following options for --mount (addendum to #26837):

  • tmpfs-size
  • tmpfs-mode

- How I did it

Updated opts/mount

- How to verify it

$ docker run -it --rm --mount type=tmpfs,target=/foo,tmpfs-size=1m busybox df /foo
Filesystem           1K-blocks      Used Available Use% Mounted on
tmpfs                     1024         0      1024   0% /foo
$ docker run -it --rm --mount type=tmpfs,target=/foo,tmpfs-mode=0700 busybox stat /foo
  File: /foo
  Size: 40              Blocks: 0          IO Block: 4096   directory
Device: 31h/49d Inode: 2           Links: 2
Access: (0700/drwx------)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2016-11-08 05:59:58.000000000
Modify: 2016-11-08 05:59:58.000000000
Change: 2016-11-08 05:59:58.000000000

- Description for the changelog

opts/mount: add tmpfs-specific options

- A picture of a cute animal (not mandatory but encouraged)

penguin

Signed-off-by: Akihiro Suda [email protected]

@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented Nov 8, 2016

cc @stevvooe @cpuguy83

@AkihiroSuda AkihiroSuda added the area/cli Client label Nov 8, 2016
stevvooe
stevvooe reviewed Nov 8, 2016
View reviewed changes
opts/mount.go Outdated
Copy link
Contributor

@stevvooe stevvooe Nov 8, 2016

Choose a reason for hiding this comment

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

Are there any cases where one would not pass these as octal?

Copy link
Member Author

@AkihiroSuda AkihiroSuda Nov 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

No.
Updated PR to use ParseUint(value, 8, ..) instead.
Linux tmpfs uses strtoul(..,8,.. ) as well: http://lxr.free-electrons.com/source/mm/shmem.c?v=4.8#L3440

stevvooe
stevvooe reviewed Nov 8, 2016
View reviewed changes
opts/mount_test.go Outdated
Copy link
Contributor

@stevvooe stevvooe Nov 8, 2016

Choose a reason for hiding this comment

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

Make sure to test a few variations of octal mode bits. For example, 0700 and 700 will be different.

Copy link
Member Author

@AkihiroSuda AkihiroSuda Nov 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

In Linux tmpfs 0700 is identical to 700.
http://lxr.free-electrons.com/source/mm/shmem.c?v=4.8#L3440

Updated the PR to test that.

@AkihiroSuda
opts/mount: add tmpfs-specific options
45ed6a7 
added following options:

 * tmpfs-size
 * tmpfs-mode

Signed-off-by: Akihiro Suda <[email protected]>
@vdemeester
Copy link
Member

vdemeester commented Nov 9, 2016

Design SGTM 🐸

cpuguy83
cpuguy83 approved these changes Nov 9, 2016
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@stevvooe
Copy link
Contributor

stevvooe commented Nov 9, 2016

LGTM

@cpuguy83
Copy link
Member

cpuguy83 commented Nov 9, 2016

ping @thaJeztah

thaJeztah
thaJeztah approved these changes Nov 10, 2016
View reviewed changes
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.

docs LGTM!

@thaJeztah
Copy link
Member

thaJeztah commented Nov 10, 2016

Please wait with merging until #27967 is merged!

@vieux vieux merged commit 55543c4 into moby:master Nov 10, 2016
@GordonTheTurtle GordonTheTurtle added this to the 1.13.0 milestone Nov 10, 2016
@AkihiroSuda AkihiroSuda mentioned this pull request Nov 12, 2016
Merged
4 tasks
@thaJeztah thaJeztah mentioned this pull request Dec 2, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevvooe stevvooe stevvooe left review comments

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

No one assigned

Labels

area/cli Client impact/changelog impact/cli impact/documentation status/3-docs-review

Projects

None yet

Milestone

1.13.0

Development

Successfully merging this pull request may close these issues.

7 participants

@AkihiroSuda @vdemeester @stevvooe @cpuguy83 @thaJeztah @vieux @GordonTheTurtle