Add implementation and test for SetupIPForwarding() #2
Conversation
| if bytes.Compare(procSetting, []byte("1\n")) == 0 { | ||
| WriteIPForwardingSetting(t, []byte{'0', '\n'}) | ||
| } else { | ||
| restore = true |
There was a problem hiding this comment.
Mmmm, I think this can be improved:
- We set
restore = trueonly when we don't write over the file, which feels backwards. - I'd rather use a
deferinstead of a boolean-based mechanism: here if the test fails for some reason, the restoring code is never called. - I think it's safer to just write back the original value in all cases without any other assumptions.
I think this whole block should be something like:
// Read current setting and ensure the original value gets restored.
procSetting := ReadCurrentIPForwardingSetting(t)
defer WriteIPForwardingSetting(t, procSetting)
// Disable IP forwarding if enabled.
if bytes.Compare(procSetting, []byte("1\n")) == 0 {
WriteIPForwardingSetting(t, []byte{"0\n"})
}
There was a problem hiding this comment.
This function tests the call setupIPForwarding() which does nothing but enabling ipv4 forwarding writing '1' in the file regardless of the existing value.
In order to verify the function does its job, we need to start the test from a state in which the file contains '0'.
We therefore need to "restore" only when the file contains '0' before executing the test and the call to SetupIPForwarding() was successful.
If the file contained '1' (then we reset) and the test is successful, then there is nothing to restore, the original value will be there.
I'd prefer avoid a redundant restore, which will happen using the defer.
But I do not have a strong opinion about it, at the end of the day this is test code.
I agree with you, it's more robust if we blindly always restore the original value.
I will make the change.
I will modify the comment as you suggested, it is more clear. Thanks.
|
Thanks for the PR 👍 All good besides a few nitpicks :-) |
|
Thanks Arnaud for the review, will take care of your comments. |
|
Why close? Updating the branch is easier, and it helps keeping track of the history! |
|
I did not mean to. I am still learning how to work on github. Sorry. Alessandro On Wed, Mar 4, 2015 at 10:27 AM, Arnaud Porterie [email protected]
|
|
No problem :-) We can still reopen it if required, and let me know if you need some coaching using GitHub! |
|
@icecrime Haha, actually you can't reopen after force-push :) |
TL;DR: check for IsExist(err) after a failed MkdirAll() is both
redundant and wrong -- so two reasons to remove it.
Quoting MkdirAll documentation:
> MkdirAll creates a directory named path, along with any necessary
> parents, and returns nil, or else returns an error. If path
> is already a directory, MkdirAll does nothing and returns nil.
This means two things:
1. If a directory to be created already exists, no error is
returned.
2. If the error returned is IsExist (EEXIST), it means there exists
a non-directory with the same name as MkdirAll need to use for
directory. Example: we want to MkdirAll("a/b"), but file "a"
(or "a/b") already exists, so MkdirAll fails.
The above is a theory, based on quoted documentation and my UNIX
knowledge.
3. In practice, though, current MkdirAll implementation [1] returns
ENOTDIR in most of cases described in moby#2, with the exception when
there is a race between MkdirAll and someone else creating the
last component of MkdirAll argument as a file. In this very case
MkdirAll() will indeed return EEXIST.
Because of moby#1, IsExist check after MkdirAll is not needed.
Because of moby#2 and moby#3, ignoring IsExist error is just plain wrong,
as directory we require is not created. It's cleaner to report
the error now.
[1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go
Signed-off-by: Kir Kolyshkin <[email protected]>
TL;DR: check for IsExist(err) after a failed MkdirAll() is both
redundant and wrong -- so two reasons to remove it.
Quoting MkdirAll documentation:
> MkdirAll creates a directory named path, along with any necessary
> parents, and returns nil, or else returns an error. If path
> is already a directory, MkdirAll does nothing and returns nil.
This means two things:
1. If a directory to be created already exists, no error is
returned.
2. If the error returned is IsExist (EEXIST), it means there exists
a non-directory with the same name as MkdirAll need to use for
directory. Example: we want to MkdirAll("a/b"), but file "a"
(or "a/b") already exists, so MkdirAll fails.
The above is a theory, based on quoted documentation and my UNIX
knowledge.
3. In practice, though, current MkdirAll implementation [1] returns
ENOTDIR in most of cases described in moby#2, with the exception when
there is a race between MkdirAll and someone else creating the
last component of MkdirAll argument as a file. In this very case
MkdirAll() will indeed return EEXIST.
Because of moby#1, IsExist check after MkdirAll is not needed.
Because of moby#2 and moby#3, ignoring IsExist error is just plain wrong,
as directory we require is not created. It's cleaner to report
the error now.
[1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go
Signed-off-by: Kir Kolyshkin <[email protected]>
Signed-off-by: Chris Telfer <[email protected]>
Signed-off-by: Alessandro Boch [email protected]