Conversation
Signed-off-by: John Howard <jhoward@microsoft.com>
|
LGTM |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
t.b.h., the abstraction is a bit awkward here (i.e. RemotefsCmd being just "remotefs", OpenFileCmd being (surprise) "openfile"); also (and I know we don't have CI setup yet for LCOW), but perhaps we should have tests for this added (as a follow up)
| @@ -33,7 +33,7 @@ func (l *lcowfs) OpenFile(path string, flag int, perm os.FileMode) (_ driver.Fil | |||
| flagStr := strconv.FormatInt(int64(flag), 10) | |||
| permStr := strconv.FormatUint(uint64(perm), 8) | |||
There was a problem hiding this comment.
Not related to this PR, but wondering why all the conversion is done here, instead of just using the correct verbs in the fmt.Sprintf()? (%d for int, %o for octal (or %#o for octal with leading zero); https://play.golang.org/p/9H-rAAA5jaT
There was a problem hiding this comment.
Yeah I didn't think about that. All of the strconv stuff can be changed to use %d or %o in the fmt.Sprintf statement.
| permStr := strconv.FormatUint(uint64(perm), 8) | ||
|
|
||
| commandLine := fmt.Sprintf("%s %s %s %s", remotefs.RemotefsCmd, remotefs.OpenFileCmd, flagStr, permStr) | ||
| commandLine := fmt.Sprintf("%s %s %s %s %s", remotefs.RemotefsCmd, remotefs.OpenFileCmd, path, flagStr, permStr) |
There was a problem hiding this comment.
Should we have validation of the arguments that were passed? (path not empty, flag not zero? (not sure what flag is without description), permStr being a valid value?
Also; perhaps we should change the fmt.Errorf("failed to open file pipes %s: %s", path, err) to use %q for the path, then it becomes more aparent if the value is empty
There was a problem hiding this comment.
Defers largely to @gupta-ak, but the remote command will fail when executed with invalid parameters
There was a problem hiding this comment.
Yeah, the remote command on the GCS side validates the parameters, so it will fail there.
thaJeztah
added
platform/windows
status/4-merge
kind/experimental
and removed
status/0-triage
labels
thaJeztah
added
the
area/lcow
6 participants
Signed-off-by: John Howard jhoward@microsoft.com
While debugging multiple LCOW issues, found that the
OpenFilecall in the remote fs was calling gcstools with the wrong parameters -pathwas omitted.@darrenstahlmsft @gupta-ak @johnstep PTAL