From 8c4507be212584491286612019984bd363e7f688 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Wed, 7 Dec 2022 17:53:20 +0100 Subject: [PATCH] runtime: Launch QEMU with cmd.Start() LaunchCustomQemu() currently starts QEMU with cmd.Run() which is supposed to block until the child process terminates. This assumes that QEMU daemonizes itself, otherwise LaunchCustomQemu() would block forever. The virtcontainers package indeed enables the Daemonize knob in the configuration but having such an implicit dependency on a supposedly configurable setting is ugly and fragile. cmd.Run() is : func (c *Cmd) Run() error { if err := c.Start(); err != nil { return err } return c.Wait() } Let's open-code this : govmm calls cmd.Start() and returns the cmd to virtcontainers which calls cmd.Wait(). If QEMU doesn't start, e.g. missing binary, there won't be any errors to collect from QEMU output. Just drop these lines in govmm. Similarily there won't be any log file to read from in virtcontainers. Drop that as well. Signed-off-by: Greg Kurz (cherry picked from commit bf4e3a618f3b2aa33ecabc2ca0f6f2d4a527ffe1) Signed-off-by: Greg Kurz --- src/runtime/pkg/govmm/qemu/examples_test.go | 11 +++++--- src/runtime/pkg/govmm/qemu/qemu.go | 30 +++++++-------------- src/runtime/virtcontainers/qemu.go | 19 +++++++------ 3 files changed, 26 insertions(+), 34 deletions(-) diff --git a/src/runtime/pkg/govmm/qemu/examples_test.go b/src/runtime/pkg/govmm/qemu/examples_test.go index 03e52b87ac..1a3f2e8ffb 100644 --- a/src/runtime/pkg/govmm/qemu/examples_test.go +++ b/src/runtime/pkg/govmm/qemu/examples_test.go @@ -27,13 +27,16 @@ func Example() { // resources params = append(params, "-m", "370", "-smp", "cpus=2") - // LaunchCustomQemu should return as soon as the instance has launched as we - // are using the --daemonize flag. It will set up a unix domain socket - // called /tmp/qmp-socket that we can use to manage the instance. - _, err := qemu.LaunchCustomQemu(context.Background(), "", params, nil, nil, nil) + // LaunchCustomQemu should return immediately. We must then wait + // the returned process to terminate as we are using the --daemonize + // flag. + // It will set up a unix domain socket called /tmp/qmp-socket that we + // can use to manage the instance. + proc, err := qemu.LaunchCustomQemu(context.Background(), "", params, nil, nil, nil) if err != nil { panic(err) } + proc.Wait() // This channel will be closed when the instance dies. disconnectedCh := make(chan struct{}) diff --git a/src/runtime/pkg/govmm/qemu/qemu.go b/src/runtime/pkg/govmm/qemu/qemu.go index b47a8fb175..10de342ed3 100644 --- a/src/runtime/pkg/govmm/qemu/qemu.go +++ b/src/runtime/pkg/govmm/qemu/qemu.go @@ -14,7 +14,6 @@ package qemu import ( - "bytes" "context" "fmt" "log" @@ -2967,12 +2966,8 @@ func (config *Config) appendFwCfg(logger QMPLog) { // // The Config parameter contains a set of qemu parameters and settings. // -// This function writes its log output via logger parameter. -// -// The function will block until the launched qemu process exits. "", nil -// will be returned if the launch succeeds. Otherwise a string containing -// the contents of stderr + a Go error object will be returned. -func LaunchQemu(config Config, logger QMPLog) (string, error) { +// See LaunchCustomQemu for more information. +func LaunchQemu(config Config, logger QMPLog) (*exec.Cmd, error) { config.appendName() config.appendUUID() config.appendMachine() @@ -2995,7 +2990,7 @@ func LaunchQemu(config Config, logger QMPLog) (string, error) { config.appendSeccompSandbox() if err := config.appendCPUs(); err != nil { - return "", err + return nil, err } ctx := config.Ctx @@ -3026,17 +3021,15 @@ func LaunchQemu(config Config, logger QMPLog) (string, error) { // // This function writes its log output via logger parameter. // -// The function will block until the launched qemu process exits. "", nil -// will be returned if the launch succeeds. Otherwise a string containing -// the contents of stderr + a Go error object will be returned. +// The function returns cmd, nil where cmd is a Go exec.Cmd object +// representing the QEMU process if launched successfully. Otherwise +// nil, err where err is a Go error object is returned. func LaunchCustomQemu(ctx context.Context, path string, params []string, fds []*os.File, - attr *syscall.SysProcAttr, logger QMPLog) (string, error) { + attr *syscall.SysProcAttr, logger QMPLog) (*exec.Cmd, error) { if logger == nil { logger = qmpNullLogger{} } - errStr := "" - if path == "" { path = "qemu-system-x86_64" } @@ -3050,15 +3043,12 @@ func LaunchCustomQemu(ctx context.Context, path string, params []string, fds []* cmd.SysProcAttr = attr - var stderr bytes.Buffer - cmd.Stderr = &stderr logger.Infof("launching %s with: %v", path, params) - err := cmd.Run() + err := cmd.Start() if err != nil { logger.Errorf("Unable to launch %s: %v", path, err) - errStr = stderr.String() - logger.Errorf("%s", errStr) + return nil, err } - return errStr, err + return cmd, nil } diff --git a/src/runtime/virtcontainers/qemu.go b/src/runtime/virtcontainers/qemu.go index 7ad589c641..761bdd2fc0 100644 --- a/src/runtime/virtcontainers/qemu.go +++ b/src/runtime/virtcontainers/qemu.go @@ -926,17 +926,16 @@ func (q *qemu) StartVM(ctx context.Context, timeout int) error { } - var strErr string - strErr, err = govmmQemu.LaunchQemu(q.qemuConfig, newQMPLogger()) + qemuCmd, err := govmmQemu.LaunchQemu(q.qemuConfig, newQMPLogger()) if err != nil { - if q.config.Debug && q.qemuConfig.LogFile != "" { - b, err := os.ReadFile(q.qemuConfig.LogFile) - if err == nil { - strErr += string(b) - } - } - q.Logger().WithError(err).Errorf("failed to launch qemu: %s", strErr) - return fmt.Errorf("failed to launch qemu: %s, error messages from qemu log: %s", err, strErr) + q.Logger().WithError(err).Error("failed to launch qemu") + return fmt.Errorf("failed to launch qemu: %s", err) + } + if q.qemuConfig.Knobs.Daemonize { + // LaunchQemu returns a handle on the upper QEMU process. + // Wait for it to exit to assume that the QEMU daemon was + // actually started. + qemuCmd.Wait() } err = q.waitVM(ctx, qmpConn, timeout)