-
Notifications
You must be signed in to change notification settings - Fork 41
Fix DebugGCS time.Duration parameter to work across all platforms #243
Conversation
@jiria - This is not necessary. |
@asherkariv, can you please comment on why this was needed? |
@jterry75 Synced offline with Asher. time.Duration requires int64, so we cannot pass explicitly casted int value as it was done in the original version. This worked on 64 bit platform as int would be matching int64, but not on 32 bit platform. As such, we can either change it to int64 cast (current change), or completely remove the cast (after having the offline discussion, we believe that would be cleaner). What are your thoughts? |
Oh I am sorry I didn't properly look at the review. I think the right fix is to just remove the cast altogether: proc.WaitTimeout(time.Second * 30) Can you remove this in the other areas we do this silly cast as well? |
Only found more casts in client/createext4vhdx.go, but removing that seems to fail the build. I have not dug further, as I am not sure I can validate those code changes. |
@jiria I just tried this on my own and it compiles fine. Can you make this change please? diff --git a/client/createext4vhdx.go b/client/createext4vhdx.go
index 48daaeb..870b4ee 100644
--- a/client/createext4vhdx.go
+++ b/client/createext4vhdx.go
@@ -97,7 +97,7 @@ func (config *Config) CreateExt4Vhdx(destFile string, sizeGB uint32, cacheFile s
return fmt.Errorf("failed to `%s` following hot-add %s to utility VM: %s", testdCommand, destFile, err)
}
defer testdProc.Close()
- testdProc.WaitTimeout(time.Duration(int(time.Second) * config.UvmTimeoutSeconds))
+ testdProc.WaitTimeout(time.Second * config.UvmTimeoutSeconds)
testdExitCode, err := testdProc.ExitCode()
if err != nil {
config.dismount(destFile)
@@ -117,7 +117,7 @@ func (config *Config) CreateExt4Vhdx(destFile string, sizeGB uint32, cacheFile s
return fmt.Errorf("failed to `%s` following hot-add %s to utility VM: %s", lsCommand, destFile, err)
}
defer lsProc.Close()
- lsProc.WaitTimeout(time.Duration(int(time.Second) * config.UvmTimeoutSeconds))
+ lsProc.WaitTimeout(time.Second * config.UvmTimeoutSeconds)
lsExitCode, err := lsProc.ExitCode()
if err != nil {
config.dismount(destFile)
@@ -139,7 +139,7 @@ func (config *Config) CreateExt4Vhdx(destFile string, sizeGB uint32, cacheFile s
return fmt.Errorf("failed to RunProcess %q following hot-add %s to utility VM: %s", destFile, mkfsCommand, err)
}
defer mkfsProc.Close()
- mkfsProc.WaitTimeout(time.Duration(int(time.Second) * config.UvmTimeoutSeconds))
+ mkfsProc.WaitTimeout(time.Second * config.UvmTimeoutSeconds)
mkfsExitCode, err := mkfsProc.ExitCode()
if err != nil {
config.dismount(destFile)
diff --git a/client/process.go b/client/process.go
index 822a90b..a8ffdf9 100644
--- a/client/process.go
+++ b/client/process.go
@@ -158,7 +158,7 @@ func (config *Config) DebugGCS() {
logrus.Debugln("benign failure getting gcs logs: ", err)
}
if proc != nil {
- proc.WaitTimeout(time.Duration(int(time.Second) * 30))
+ proc.WaitTimeout(time.Second * 30)
}
logrus.Debugf("GCS Debugging:\n%s\n\nEnd GCS Debugging", strings.TrimSpace(out.String()))
} |
Interesting, duplicating your change into the moby repo and building that, results in:
|
Fix time.Duration parameter to have the right size across all platforms. Work done by askariv.