Skip to content
Permalink
Browse files

vm/gce: close old consolew in Run

Run can be executed several times on a VM.
  • Loading branch information...
dvyukov committed Dec 2, 2018
1 parent 7a0edfb commit 7dcaeaf3220109910515ec208b7ed6db4e8435a2
Showing with 6 additions and 0 deletions.
  1. +3 −0 vm/gce/gce.go
  2. +3 −0 vm/vmimpl/openbsd.go
@@ -228,6 +228,9 @@ func (inst *instance) Run(timeout time.Duration, stop <-chan bool, command strin
conWpipe.Close()
return nil, nil, err
}
if inst.consolew != nil {
inst.consolew.Close()
}
inst.consolew = conw
if err := con.Start(); err != nil {
conRpipe.Close()
@@ -1,3 +1,6 @@
// Copyright 2018 syzkaller project authors. All rights reserved.
// Use of this source code is governed by Apache 2 LICENSE that can be found in the LICENSE file.

package vmimpl

import (

4 comments on commit 7dcaeaf

@dvyukov

This comment has been minimized.

Copy link
Collaborator Author

replied Dec 2, 2018

@blackgnezdo

This comment has been minimized.

Copy link
Member

replied Dec 2, 2018

A couple of observations:

  1. Go's interface duck typing makes it harder to figure out what protocols are expected from implementations. In fact, the reader has to work to learn which interface are in fact implemented.
  2. I'd probably introduce more life-cycle methods for instances so that they go through a more structured sequence instead of knowing Run needs to support reentrance. Something like explicit Cleanup() between Run()s would've made it more obvious which hooks should be touched as data gets added to instance.

My apologies about missing the license in the file. Should I add a validation step to presubmit to check all files contain the header?

@dvyukov

This comment has been minimized.

Copy link
Collaborator Author

replied Dec 3, 2018

Should I add a validation step to presubmit to check all files contain the header?

This would be useful. We may then as well support the official convention for auto-generated files so that they are exempted from license check:
https://golang.org/pkg/cmd/go/internal/generate/

@dvyukov

This comment has been minimized.

Copy link
Collaborator Author

replied Dec 3, 2018

I'd probably introduce more life-cycle methods for instances so that they go through a more structured sequence instead of knowing Run needs to support reentrance. Something like explicit Cleanup() between Run()s would've made it more obvious which hooks should be touched as data gets added to instance.

Complicating API does not look like an obvious win for me. E.g. one can call read on a file multiple times, but there is no cleanup_after_read to prepare for the next call. What would be more unconditionally useful:

  1. Actual testing of that code. It's tricky and it breaks periodically. We don't test it other than actual prod.
  2. More code unification between VM types. Besides simplifying implementations, it can also be used to capture properties like repeated Run, e.g. by adding Cleanup method as implementation hook. I think we should stick to a model where an impl returns console pipe from ctor, that would prevent the bug too.
  3. Better docs.
Please sign in to comment.
You can’t perform that action at this time.