- Notifications
You must be signed in to change notification settings - Fork18
Verify rsync protocol version match prior to proceeding - Rebased on current master.#71
Uh oh!
There was an error while loading.Please reload this page.
Conversation
`sync.Version()` proceeds with a warning that there may be a versionmismatch if it timesout.`syncCmd.version` assumes rsync is present in the path.`wsep.RemoteExecer` isn't available publicly on GitHub, so I took abest guess with it and `wsep.ExitError`.
Passing `s.ReadLine` to `strings.Split` was a mistake since it returnsthree 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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
cmd/coder/sync.go Outdated
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Probably leftover from a rebase, these were removed in master.
cmd/coder/sync.go Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The way this is laid out,version will always return an empty string. Reading fromr 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)}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Will do.
internal/sync/sync.go Outdated
| if err != nil { | ||
| return "", err | ||
| } | ||
| r := bufio.NewReader(process.Stdout()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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())Andbuf can be used down below to read the first line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Will do.
stephenwithavJul 2, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@scsmithr Do you want the remote check tolog.Fatal fail if we can't retrieve it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The remote check still has that.
223551b to35ee1deCompareinternal/sync/sync.go Outdated
| return "", err | ||
| } | ||
| buf := &bytes.Buffer{} | ||
| go io.Copy(buf, process.Stdout()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would remove thego 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Two very minor issues and we should be good to go.
internal/sync/sync.go Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| return"",fmt.Errorf("Version check exit status: %v",code) | |
| return"",fmt.Errorf("version check exit status: %v",code) |
internal/sync/sync.go Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd prefer returning the error here instead.
7ec1868 tod853066Compareinternal/sync/sync.go Outdated
| if _, ok := err.(wsep.ExitError); ok { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We may as well remove this branch.
cmd/coder/sync.go Outdated
| @@ -29,6 +33,23 @@ func (cmd *syncCmd) RegisterFlags(fl *pflag.FlagSet) { | |||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 otherVersion method)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice work, appreciate the contribution.
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.Potentiallycloses#65.