Code review comment for lp:~thumper/juju-core/juju-ssh-dir-as-user

Revision history for this message
Andrew Wilkins (axwalk) wrote :

https://codereview.appspot.com/52950043/diff/1/utils/file.go
File utils/file.go (right):

https://codereview.appspot.com/52950043/diff/1/utils/file.go#newcode104
utils/file.go:104: func IsDirectory(path string) bool {
In light of my comments on MkDirAllForUser, I think this should just be
ditched. Some callers will care about symlinks, some won't.

https://codereview.appspot.com/52950043/diff/1/utils/file.go#newcode106
utils/file.go:106: if os.IsNotExist(err) {
This is redundant, given the below check.

https://codereview.appspot.com/52950043/diff/1/utils/sudo.go
File utils/sudo.go (right):

https://codereview.appspot.com/52950043/diff/1/utils/sudo.go#newcode63
utils/sudo.go:63: if IsDirectory(dir) {
How about symlinks?

I think, rather than IsDirectory, use os.Lstat and os.IsNotExist. If
there's a file in the path, then the function can just fail when we
attempt to mkdir.

IOW, something like:

_, err := os.Lstat(dir)
if os.IsNotExist(err) {
     if err := MkdirAllForUser(filepath.Dir(dir), perm); err != nil {
         return err
     }
     err = MkdirForUser(dir, perm)
}
if err != nil {
     return err
}
return ChownToUser(dir)

https://codereview.appspot.com/52950043/diff/1/utils/sudo.go#newcode79
utils/sudo.go:79: os.RemoveAll(topMostDir)
This scares me. Can we please not remove things on failure? What if
chown $HOME is a symlink, and chmod fails? Then we try to RemoveAll
$HOME.

https://codereview.appspot.com/52950043/

« Back to merge proposal