cmd/snap-update-ns: detect and report read-only filesystems #4166

Merged
merged 2 commits into from Nov 13, 2017

Conversation

Projects
None yet
4 participants
Contributor

zyga commented Nov 7, 2017

This is based on #4163

This small patch adds detection of read-only filesystems to
secure-mkdir-all. This will be soon picked up by the hole-poking code.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

zyga added some commits Oct 19, 2017

cmd/snap-update-ns: re-factor secureMkdirAll into secureMk{Prefix,Dir}
The act of "securely creating a directory" can be decomposed into
"securely create the prefix" and "securely create the leaf". Naturally
the leaf can now vary and we can easily create leaf directories, files
and symlinks.

This patch sets the stage for that to happen, by splitting the main
function into a pair of functions. On the outside nothing changes but
tests needed a slight tweak to accommodate how life-cycle of file
descriptors looks like.

There are also several new tests to improve code coverage in various
edge cases.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
cmd/snap-update-ns: detect and report read-only filesystems
This small patch adds detection of read-only filesystems to
secure-mkdir-all. This will be soon picked up by the hole-poking code.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

codecov-io commented Nov 7, 2017

Codecov Report

Merging #4166 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4166      +/-   ##
==========================================
+ Coverage   75.52%   75.57%   +0.05%     
==========================================
  Files         436      436              
  Lines       37766    37814      +48     
==========================================
+ Hits        28521    28578      +57     
+ Misses       7250     7244       -6     
+ Partials     1995     1992       -3
Impacted Files Coverage Δ
cmd/snap-update-ns/utils.go 100% <100%> (+14.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 533f5fa...c21a85a. Read the comment docs.

Looks good, with two tiny remarks for you to consider.

+ // Declare var and don't assign-declare below to ensure we don't swallow
+ // any errors by mistake.
+ var err error
+ var fd int
@stolowski

stolowski Nov 9, 2017

Contributor

I'd initialize it to a sane and safe value (which is -1 in this code), just to avoid nasty bugs in the future.

@zyga

zyga Nov 10, 2017

Contributor

This is a comment for the base PR. I'll address it there.

@zyga

zyga Nov 11, 2017

Contributor

I did and later undid this. It is an ineffectual assignment as all code paths later overwrite the value before it has a chance of being used. This is an error according to our validation tools.

+ segment := segments[i]
+ made := true
+ var err error
+ var newFd int
@stolowski

stolowski Nov 9, 2017

Contributor

Again, I'd initialize it to -1.

@zyga

zyga Nov 10, 2017

Contributor

This is a comment for the base PR. I'll address it there.

@zyga

zyga Nov 11, 2017

Contributor

Ditto.

@stolowski stolowski merged commit 497f9f1 into snapcore:master Nov 13, 2017

4 of 7 checks passed

artful-i386 autopkgtest finished (failure)
Details
xenial-amd64 autopkgtest finished (failure)
Details
zesty-amd64 autopkgtest running
Details
artful-amd64 autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-i386 autopkgtest finished (success)
Details
xenial-ppc64el autopkgtest finished (success)
Details

@zyga zyga deleted the zyga:tweak/detect-rofs branch Nov 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment