environs/sshstorage: stream stdin

environs/sshstorage: stream stdin

Change sshstorage's Put to stream to the
ssh command's stdin, rather than reading
into memory and then writing.

Also, drive-by: use `mktemp --tmpdir` rather
than exporting $TMPDIR.

https://code.launchpad.net/~axwalk/juju-core/sshstorage-stream-input/+merge/188508

(do not edit description out of merge proposal)

[axw, 2013-10-01 04:43:45]

Please take a look.

[axw, 2013-10-01 09:59:13]

Please take a look.

[axw, 2013-10-01 10:26:40]

Please take a look.

[rog, 2013-10-01 10:27:39]

Thanks for doing this. Looks good, with a few suggestions below.

https://codereview.appspot.com/14197043/diff/1003/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/14197043/diff/1003/environs/sshstorage/storage...
environs/sshstorage/storage.go:153: command = fmt.Sprintf("(%s) << EOF",
command)
s/EOF/'@EOF'/

using quotes means that the shell doesn't scan through
for potential variable expansion.

The @ character is just so it can't appear in a normal
base64 output.

https://codereview.appspot.com/14197043/diff/1003/environs/sshstorage/storage...
environs/sshstorage/storage.go:159: wrapper, err := utils.NewWrapWriter(s.stdin,
base64LineLength)
You should consider wrapping s.stdin in a bufio.Writer here.
(perhaps even wrap it inside the SSHStorage and flushing
when appropriate)

https://codereview.appspot.com/14197043/diff/1003/utils/wrapwriter.go
File utils/wrapwriter.go (right):

https://codereview.appspot.com/14197043/diff/1003/utils/wrapwriter.go#newcode44
utils/wrapwriter.go:44: return n, err
s/n/total/

https://codereview.appspot.com/14197043/diff/1003/utils/wrapwriter.go#newcode44
utils/wrapwriter.go:44: return n, err
s/n/total/

https://codereview.appspot.com/14197043/diff/1003/utils/wrapwriter_test.go
File utils/wrapwriter_test.go (right):

https://codereview.appspot.com/14197043/diff/1003/utils/wrapwriter_test.go#ne...
utils/wrapwriter_test.go:29: type test struct {
I'd like to see better tests here:

- what happens when writes return errors or part
- does it cope ok with several writes in a row?

For the latter, perhaps take a piece of input, split it up
many different ways (use some simple algorithm or
table to do that), and check that the output is
always the same.

[jameinel, 2013-10-01 11:09:25]

https://codereview.appspot.com/14197043/diff/1003/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/14197043/diff/1003/environs/sshstorage/storage...
environs/sshstorage/storage.go:153: command = fmt.Sprintf("(%s) << EOF",
command)
On 2013/10/01 10:27:39, rog wrote:
> s/EOF/'@EOF'/
> 
> using quotes means that the shell doesn't scan through
> for potential variable expansion.
> 
> The @ character is just so it can't appear in a normal
> base64 output.

Sounds interesting, though we should test that it works properly with /bin/dash.

[rog, 2013-10-01 11:46:18]

On 1 October 2013 12:09,  <john.meinel@canonical.com> wrote:
>
>
https://codereview.appspot.com/14197043/diff/1003/environs/sshstorage/storage.go
> File environs/sshstorage/storage.go (right):
>
>
https://codereview.appspot.com/14197043/diff/1003/environs/sshstorage/storage...
> environs/sshstorage/storage.go:153: command = fmt.Sprintf("(%s) << EOF",
> command)
> On 2013/10/01 10:27:39, rog wrote:
>>
>> s/EOF/'@EOF'/
>
>
>> using quotes means that the shell doesn't scan through
>> for potential variable expansion.
>
>
>> The @ character is just so it can't appear in a normal
>> base64 output.
>
>
> Sounds interesting, though we should test that it works properly with
> /bin/dash.

It does (it's original Bourne shell behaviour).

[axw, 2013-10-01 14:56:50]

Please take a look.

[rog, 2013-10-01 15:28:47]

LGTM modulo the below suggestions. Thanks!

https://codereview.appspot.com/14197043/diff/14001/environs/sshstorage/linewr...
File environs/sshstorage/linewrapwriter.go (right):

https://codereview.appspot.com/14197043/diff/14001/environs/sshstorage/linewr...
environs/sshstorage/linewrapwriter.go:22: // after every "lineLength" bytes.
// Moreover it gives no consideration to multibyte
// utf-8 characters, which it can split arbitrarily.

https://codereview.appspot.com/14197043/diff/14001/environs/sshstorage/storag...
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/14197043/diff/14001/environs/sshstorage/storag...
environs/sshstorage/storage.go:149: stdin = bufio.NewWriter(s.stdin)
I'm not sure this half-way house is great.
I'd either make s.stdin a bufio.Writer, or
call NewWriter inside the later "if input != nil" block,
avoiding the ugly dynamic type conversion either way,
and allowing the use of the more natural WriteString
methods.

https://codereview.appspot.com/14197043/diff/14001/environs/sshstorage/storag...
environs/sshstorage/storage.go:154: // here-document must start on the
outer-most command.
That's not actually the case, I think. Did you try it?

[axw, 2013-10-02 04:07:53]

Please take a look.

[axw1, 2013-10-02 05:20:27]

https://codereview.appspot.com/14197043/diff/14001/environs/sshstorage/linewr...
File environs/sshstorage/linewrapwriter.go (right):

https://codereview.appspot.com/14197043/diff/14001/environs/sshstorage/linewr...
environs/sshstorage/linewrapwriter.go:22: // after every "lineLength" bytes.
On 2013/10/01 15:28:47, rog wrote:
> // Moreover it gives no consideration to multibyte
> // utf-8 characters, which it can split arbitrarily.

Done.

https://codereview.appspot.com/14197043/diff/14001/environs/sshstorage/storag...
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/14197043/diff/14001/environs/sshstorage/storag...
environs/sshstorage/storage.go:149: stdin = bufio.NewWriter(s.stdin)
On 2013/10/01 15:28:47, rog wrote:
> I'm not sure this half-way house is great.
> I'd either make s.stdin a bufio.Writer, or
> call NewWriter inside the later "if input != nil" block,
> avoiding the ugly dynamic type conversion either way,
> and allowing the use of the more natural WriteString
> methods.

It's not in the later block because it's used before then. If it were in there,
then we'd have two writes to s.stdin.

I agree about the ugliness, though. I'll wrap s.stdin, but only in this function
so as not to lose the WriteCloser interface.

https://codereview.appspot.com/14197043/diff/14001/environs/sshstorage/storag...
environs/sshstorage/storage.go:154: // here-document must start on the
outer-most command.
On 2013/10/01 15:28:47, rog wrote:
> That's not actually the case, I think. Did you try it?

I did, but I'm not sure what I was doing before to cause it to fail. I *think* I
may have had a bug where something was being wrapped twice, and ended up as
((command << EOF)).

Anyway, I've put it after 'base64 -d' and it does indeed work fine. Thanks.