Verify rsync protocol version match prior to proceeding - Rebased on current master.#71
Conversation
`sync.Version()` proceeds with a warning that there may be a version mismatch if it timesout. `syncCmd.version` assumes rsync is present in the path. `wsep.RemoteExecer` isn't available publicly on GitHub, so I took a best guess with it and `wsep.ExitError`.
Passing `s.ReadLine` to `strings.Split` was a mistake since it returns three values. Having access to `wsep` helped the compiler find the bugs.
|
This builds locally fine. The CI bug is inaccessible to me. Any tips, @scsmithr? |
scsmithr
left a comment
scsmithr
left a comment
There was a problem hiding this comment.
I know it's kind of tough to be contributing changes that you don't have a good way of testing, but I think we're close.
And you can ignore the ci failure, it's just failing because there isn't a license for you on that service. Everything compiled fine on my end.
| // See https://lxadm.com/Rsync_exit_codes#List_of_standard_rsync_exit_codes. | ||
| var IncompatRsync = errors.New("rsync: exit status 2") | ||
| var StreamErrRsync = errors.New("rsync: exit status 12") |
There was a problem hiding this comment.
Probably leftover from a rebase, these were removed in master.
| func (s *syncCmd) version() string { | ||
| cmd := exec.Command("rsync", "--version") | ||
| stdout, err := cmd.StdoutPipe() | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
|
|
||
| if err := cmd.Start(); err != nil { | ||
| log.Fatal(err) | ||
| } | ||
|
|
||
| r := bufio.NewReader(stdout) | ||
| if err := cmd.Wait(); err != nil { | ||
| log.Fatal(err) | ||
| } | ||
|
|
||
| firstLine, err := r.ReadString('\n') | ||
| if err != nil { | ||
| return "" | ||
| } | ||
|
|
||
| versionString := strings.Split(firstLine, "protocol version ") | ||
|
|
||
| return versionString[1] |
There was a problem hiding this comment.
The way this is laid out, version will always return an empty string. Reading from r will always error, because the pipe will be closed before we start reading from the command.
I would suggest replacing reading the output of the command with:
out, err := cmd.CombinedOutput()
if err != nil {
log.Fatal(err)
}
And I would replace reading the first line with:
firstLine, err := bytes.NewBuffer(out).ReadString('\n')
if err != nil {
log.Fatal(err)
}
| if err != nil { | ||
| return "", err | ||
| } | ||
| r := bufio.NewReader(process.Stdout()) |
There was a problem hiding this comment.
We have the same issue here with the reader never reading anything.
I think the best solution here would be to replace this reader with:
buf := &bytes.Buffer{}
io.Copy(buf, process.Stdout())
And buf can be used down below to read the first line.
There was a problem hiding this comment.
@scsmithr Do you want the remote check to log.Fatal fail if we can't retrieve it?
There was a problem hiding this comment.
I think I prefer the behavior you had, where we just return the error and warn the user.
There was a problem hiding this comment.
The remote check still has that.
stephenwithav
force-pushed
the
add-version-check-locally
branch
from
223551b to
35ee1de
Compare
| return "", err | ||
| } | ||
| buf := &bytes.Buffer{} | ||
| go io.Copy(buf, process.Stdout()) |
There was a problem hiding this comment.
I would remove the go here. io.Copy will will terminate as expected once the command completes.
Having a goroutine here can be racy, because there's no guarantees that it runs or completes the copy before we start reading from the buffer.
scsmithr
left a comment
scsmithr
left a comment
There was a problem hiding this comment.
Two very minor issues and we should be good to go.
|
|
||
| err = process.Wait() | ||
| if code, ok := err.(wsep.ExitError); ok { | ||
| return "", fmt.Errorf("Version check exit status: %v", code) |
There was a problem hiding this comment.
| return "", fmt.Errorf("Version check exit status: %v", code) | |
| return "", fmt.Errorf("version check exit status: %v", code) |
| return "", fmt.Errorf("Version check exit status: %v", code) | ||
| } | ||
| if err != nil { | ||
| return "", fmt.Errorf("Server version mismatch") |
There was a problem hiding this comment.
I'd prefer returning the error here instead.
stephenwithav
force-pushed
the
add-version-check-locally
branch
from
7ec1868 to
d853066
Compare
| if _, ok := err.(wsep.ExitError); ok { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
We may as well remove this branch.
| fl.BoolVarP(&cmd.init, "init", "i", false, "do initial transfer and exit") | ||
| } | ||
|
|
||
| // Returns local rsync protocol version as a string. |
There was a problem hiding this comment.
Apologies for missing this earlier in the review. These comments should start with the function/method name. So this would be "version returns local rsync protocol version as a string."
(And same for the other Version method)
There was a problem hiding this comment.
No problem. I appreciate you teaching me the style you prefer prior to the interview next week so if I get the job, I'm already style trained.
scsmithr
left a comment
scsmithr
left a comment
There was a problem hiding this comment.
Nice work, appreciate the contribution.
2 participants
sync.Version()proceeds with a warning that there may be a versionmismatch if it timesout.
syncCmd.versionassumes rsync is present in the path.wsepintegration is currently unverified. Verification is next.Potentially closes #65.