code review 77080043: ogle: framework for communicating with proxy

ogle: framework for communicating with proxy
Interface definition for interacting with program:
        program/program.go
Beginnings of RPC server to provide access to program:
        program/server/server.go
Beginnings of client to talk to RPC server, plus SSH connection:
        program/client/client.go
Beginnings of ogleproxy, which implements the RPC service:
        cmd/ogleproxy/main.go
Transport definitions for RPC:
        program/proxyrpc/proxyrpc.go

No tests yet - they will come when there's a little more in place.
But it works, as tested by an external toy.

[r, 2014-03-17 21:10:59]

Hello nigeltao,

I'd like you to review this change to
https://code.google.com/p/ogle

[nigeltao, 2014-03-18 04:06:07]

LGTM.

https://codereview.appspot.com/77080043/diff/1/cmd/ogleproxy/main.go
File cmd/ogleproxy/main.go (right):

https://codereview.appspot.com/77080043/diff/1/cmd/ogleproxy/main.go#newcode7
cmd/ogleproxy/main.go:7: 
Delete the blank line so that the comment above becomes a doc comment?

https://codereview.appspot.com/77080043/diff/1/cmd/ogleproxy/main.go#newcode39
cmd/ogleproxy/main.go:39: s := &server.Server{}
Personally, I'd roll this into the next line.
err = rpc.Register(&server.Server{})

Feel free to ignore this and other similar stylistic suggestions.

https://codereview.appspot.com/77080043/diff/1/cmd/ogleproxy/main.go#newcode52
cmd/ogleproxy/main.go:52: rpc.ServeConn(conn)
Similarly,
rpc.ServeConn(&rwc{
  os.Stdin,
  os.Stdout,
})

https://codereview.appspot.com/77080043/diff/1/program/client/client.go
File program/client/client.go (right):

https://codereview.appspot.com/77080043/diff/1/program/client/client.go#newco...
program/client/client.go:20: var _ = fmt.Printf // DELETE
Delete.

https://codereview.appspot.com/77080043/diff/1/program/client/client.go#newco...
program/client/client.go:39: cmd := exec.Command("/usr/bin/ssh", host,
"ogleproxy", "-text", textFile)
Add a "// TODO: add args."??

https://codereview.appspot.com/77080043/diff/1/program/client/client.go#newco...
program/client/client.go:70: }
default:
  return an error??

https://codereview.appspot.com/77080043/diff/1/program/client/client.go#newco...
program/client/client.go:71: conn := &rwc{
I'd roll it up.

return &Program{
  client: rpc.NewClient(&rwc{
    ssh: cmd,
    r:   fromStdout,
    w:   toStdin,
  }),
}, nil

https://codereview.appspot.com/77080043/diff/1/program/client/client.go#newco...
program/client/client.go:149: f := &File{
return &File{
  etc,
}, nil

https://codereview.appspot.com/77080043/diff/1/program/program.go
File program/program.go (right):

https://codereview.appspot.com/77080043/diff/1/program/program.go#newcode66
program/program.go:66: // It is an error if two breakpoints evaluate to the same
PC.
This line is news to me, as different strings can map to the same PC, but we can
discuss that when we get to implementing it.

https://codereview.appspot.com/77080043/diff/1/program/program.go#newcode78
program/program.go:78: // routine like io.Copy dangerous.
s/routine/routines/

The line break before instead of after "routine" also looks a little weird.

https://codereview.appspot.com/77080043/diff/1/program/server/server.go
File program/server/server.go (right):

https://codereview.appspot.com/77080043/diff/1/program/server/server.go#newco...
program/server/server.go:48: for index = 0; index < len(s.files) &&
s.files[index] != nil; index++ {
You could drop the "index = 0".

https://codereview.appspot.com/77080043/diff/1/program/server/server.go#newco...
program/server/server.go:55: f = file{
I guess this works, since you've stored &f, but it looks odd to assign to f (a
local variable) at the end of a function. How about moving the "var f file" down
to be a ":=" assignment/declaration:

// Find a file descriptor (index) slot.
index := 0
for ; index < len(s.files) && s.files[index] != nil; index++ {
}
f := &file{
  mode: req.Mode,
  index: index,
  f: osFile,
}
if index == len(s.files) {
  s.files = append(s.files, f)
} else {
  s.files[index] = f
}
return nil

https://codereview.appspot.com/77080043/diff/1/program/server/server.go#newco...
program/server/server.go:65: if fd < 0 || fd >= len(s.files) || s.files[fd] ==
nil {
Keep the inequalities pointing the same way??

if fd < 0 || len(s.files) <= fd || etc {

Similarly below.

[r, 2014-03-18 12:17:22]

https://codereview.appspot.com/77080043/diff/1/cmd/ogleproxy/main.go
File cmd/ogleproxy/main.go (right):

https://codereview.appspot.com/77080043/diff/1/cmd/ogleproxy/main.go#newcode7
cmd/ogleproxy/main.go:7: 
On 2014/03/18 04:06:07, nigeltao wrote:
> Delete the blank line so that the comment above becomes a doc comment?

Done.

https://codereview.appspot.com/77080043/diff/1/cmd/ogleproxy/main.go#newcode39
cmd/ogleproxy/main.go:39: s := &server.Server{}
On 2014/03/18 04:06:07, nigeltao wrote:
> Personally, I'd roll this into the next line.
> err = rpc.Register(&server.Server{})
> 
> Feel free to ignore this and other similar stylistic suggestions.

Done.

https://codereview.appspot.com/77080043/diff/1/cmd/ogleproxy/main.go#newcode52
cmd/ogleproxy/main.go:52: rpc.ServeConn(conn)
On 2014/03/18 04:06:07, nigeltao wrote:
> Similarly,
> rpc.ServeConn(&rwc{
>   os.Stdin,
>   os.Stdout,
> })

Done.

https://codereview.appspot.com/77080043/diff/1/program/client/client.go
File program/client/client.go (right):

https://codereview.appspot.com/77080043/diff/1/program/client/client.go#newco...
program/client/client.go:20: var _ = fmt.Printf // DELETE
On 2014/03/18 04:06:07, nigeltao wrote:
> Delete.

Done.

https://codereview.appspot.com/77080043/diff/1/program/client/client.go#newco...
program/client/client.go:39: cmd := exec.Command("/usr/bin/ssh", host,
"ogleproxy", "-text", textFile)
On 2014/03/18 04:06:07, nigeltao wrote:
> Add a "// TODO: add args."??

Done.

https://codereview.appspot.com/77080043/diff/1/program/client/client.go#newco...
program/client/client.go:70: }
On 2014/03/18 04:06:07, nigeltao wrote:
> default:
>   return an error??

Done.

https://codereview.appspot.com/77080043/diff/1/program/client/client.go#newco...
program/client/client.go:71: conn := &rwc{
not the return.

https://codereview.appspot.com/77080043/diff/1/program/client/client.go#newco...
program/client/client.go:149: f := &File{
On 2014/03/18 04:06:07, nigeltao wrote:
> return &File{
>   etc,
> }, nil

not this one. i like the return to be clean and simple.

https://codereview.appspot.com/77080043/diff/1/program/program.go
File program/program.go (right):

https://codereview.appspot.com/77080043/diff/1/program/program.go#newcode66
program/program.go:66: // It is an error if two breakpoints evaluate to the same
PC.
On 2014/03/18 04:06:07, nigeltao wrote:
> This line is news to me, as different strings can map to the same PC, but we
can
> discuss that when we get to implementing it.

Done.

https://codereview.appspot.com/77080043/diff/1/program/program.go#newcode78
program/program.go:78: // routine like io.Copy dangerous.
On 2014/03/18 04:06:07, nigeltao wrote:
> s/routine/routines/
> 
> The line break before instead of after "routine" also looks a little weird.

Done.

https://codereview.appspot.com/77080043/diff/1/program/server/server.go
File program/server/server.go (right):

https://codereview.appspot.com/77080043/diff/1/program/server/server.go#newco...
program/server/server.go:48: for index = 0; index < len(s.files) &&
s.files[index] != nil; index++ {
On 2014/03/18 04:06:07, nigeltao wrote:
> You could drop the "index = 0".

Done.

https://codereview.appspot.com/77080043/diff/1/program/server/server.go#newco...
program/server/server.go:55: f = file{
On 2014/03/18 04:06:07, nigeltao wrote:
> I guess this works, since you've stored &f, but it looks odd to assign to f (a
> local variable) at the end of a function. How about moving the "var f file"
down
> to be a ":=" assignment/declaration:
> 
> // Find a file descriptor (index) slot.
> index := 0
> for ; index < len(s.files) && s.files[index] != nil; index++ {
> }
> f := &file{
>   mode: req.Mode,
>   index: index,
>   f: osFile,
> }
> if index == len(s.files) {
>   s.files = append(s.files, f)
> } else {
>   s.files[index] = f
> }
> return nil

Done.

https://codereview.appspot.com/77080043/diff/1/program/server/server.go#newco...
program/server/server.go:65: if fd < 0 || fd >= len(s.files) || s.files[fd] ==
nil {
On 2014/03/18 04:06:07, nigeltao wrote:
> Keep the inequalities pointing the same way??
> 
> if fd < 0 || len(s.files) <= fd || etc {
> 
> Similarly below.

Done.

[r, 2014-03-18 12:18:05]

*** Submitted as https://code.google.com/p/ogle/source/detail?r=d44ba9529c0f ***

ogle: framework for communicating with proxy
Interface definition for interacting with program:
        program/program.go
Beginnings of RPC server to provide access to program:
        program/server/server.go
Beginnings of client to talk to RPC server, plus SSH connection:
        program/client/client.go
Beginnings of ogleproxy, which implements the RPC service:
        cmd/ogleproxy/main.go
Transport definitions for RPC:
        program/proxyrpc/proxyrpc.go

No tests yet - they will come when there's a little more in place.
But it works, as tested by an external toy.

LGTM=nigeltao
R=nigeltao
https://codereview.appspot.com/77080043