-
Notifications
You must be signed in to change notification settings - Fork 229
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
Clarify effects of some commands in the man page #618
base: main
Are you sure you want to change the base?
Conversation
Improve readability by separating options' general description and subject-specific details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bwrap.xml: Place the --perms details in their own paragraph
This commit looks fine, I might cherry-pick it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a Signed-off-by
to the commits to indicate your acceptance of the Developer Certificate of Origin (basically declaring that we can use your contribution in the obvious ways).
<para>Note that if <arg choice="plain">DEST</arg> is visible outside | ||
of the created mount namespace, the effect is permanent.</para> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it scales very well to repeat this for every option that might alter the filesystem, because there are a lot:
- everything that mounts a directory will create an empty directory as a mount point if necessary
- everything that mounts a non-directory will create an empty regular file as a mount point if necessary
--symlink
will modify the real filesystem if its destination is a writeable file system- so will
--chmod
This is really a fact about mounting filesystems, not a fact about the command-line options that modify the filesystem. Would it perhaps be clearer what is going on if we say something similar in --bind
instead of this? Maybe like this:
<para>If a process inside the sandbox or a later command-line option
modifies the contents of <arg choice="plain">DEST</arg>, those
modifications will affect <arg choice="plain">SRC</arg>
outside the sandbox.</p>
If --bind
said this, would you have assumed that the same was true for --bind-try
, --dev-bind
and --dev-bind-try
without needing it to be said explicitly?
(We certainly don't need to add this to --ro-bind
and --ro-bind-try
because those are read-only mounts.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's valuable to use essentially the same language to talk about what happens if a process inside the sandbox does a filesystem write (whether that's symlink() or chmod() or a file overwrite or whatever), and what happens if a subsequent command-line option tells bwrap to do a filesystem write, because they're fundamentally the same operation.
I wouldn't want anyone to get the mistaken idea that a --symlink
or --chmod
or --file
is permanent, but the COMMAND doing an equivalent symlink()
or chmod()
or open()
somehow wouldn't be permanent.
<para>Note that, unlike <option>--bind-data</option>, if | ||
<arg choice="plain">DEST</arg> is visible outside of the created mount | ||
namespace, the effect is permanent.</para> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth calling out --file
specifically, because it's the only one that will silently overwrite an existing file, but I'd prefer it more like perhaps this:
<para>If <arg choice="plain">DEST</arg> already exists, it will be overwritten.
Unlike <option>--bind-data</option>, if <arg choice="plain">DEST</arg> or
a parent directory is a bind-mount from outside the sandbox,
this may overwrite an existing file outside the sandbox.</para>
Would that have given you the right expectations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that sounds great. Shall I apply it to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please!
Is it OK if I do this pseudonymously (using my GitHub username)? |
Hmm. I would personally be fine with that, but it isn't my project or my policy, and https://github.com/containers/common/blob/main/CONTRIBUTING.md does say no anonymous or pseudonymous contributions. @cgwalters, @alexlarsson, do you intend to be enforcing that policy for this project? |
Fixes #617.
RFC on wording, maybe "permanent" is not the best way to describe it, but it would address the confusion I had.