From 6836da56fe54c00e2b8f8a1b4a619b3b3a790d7d Mon Sep 17 00:00:00 2001 From: Christophe Fergeau Date: Wed, 28 Jun 2023 15:51:47 +0200 Subject: [PATCH] serial: Add --device virtio-serial,pty support This new virtio-serial option allocates a pseudo-tty for the VM console. It can then be accessed using `screen` for example. This is a bit similar to the `--device virtio-serial,stdio` option, except that the console is not tied to the terminal running vfkit, it's possible to connect/disconnect from the pseudo-tty from any terminal. This fixes https://github.com/crc-org/vfkit/issues/48 Signed-off-by: Christophe Fergeau --- doc/usage.md | 15 ++++++++++++- go.mod | 1 + go.sum | 3 +++ pkg/config/json_test.go | 2 +- pkg/config/virtio.go | 33 +++++++++++++++++++++++----- pkg/config/virtio_test.go | 7 ++++++ pkg/vf/virtio.go | 45 ++++++++++++++++++++++++++++++++------- test/vm_test.go | 42 +++++++++++++++++++++++++++++++++--- 8 files changed, 130 insertions(+), 18 deletions(-) diff --git a/doc/usage.md b/doc/usage.md index 38887f96..f8d06c81 100644 --- a/doc/usage.md +++ b/doc/usage.md @@ -225,11 +225,12 @@ This is useful in combination with usermode networking stacks such as [gvisor-ta #### Description The `--device virtio-serial` option adds a serial device to the virtual machine. This is useful to redirect text output from the virtual machine to a log file. -The `logFilePath` and `stdio` arguments are mutually exclusive. +The `logFilePath`, `stdio`, `pty` arguments are mutually exclusive. #### Arguments - `logFilePath`: path where the serial port output should be written. - `stdio`: uses stdin/stdout for the serial console input/output. +- `pty`: allocates a pseudo-terminal for the serial console input/output. #### Example @@ -244,6 +245,18 @@ launched from will be used as an interactive serial console for that device: --device virtio-serial,stdio ``` +This adds a virtio-serial device to the VM, and creates a pseudo-terminal for +the console for that device: +``` +--device virtio-serial,pty +``` +Once the VM is running, you can connect to its console with: +``` +screen /dev/ttys002 +``` +`/dev/ttys002` will vary between `vfkit` runs. +The `/dev/ttys???` path to the pty is printed during vfkit startup. +It's also available through the `/vm/inspect` endpoint of [REST API](#restful-service) in the `ptyName` field of the `virtio-serial` device. ### Random Number Generator diff --git a/go.mod b/go.mod index 49b4b235..09f4d7c6 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/crc-org/crc/v2 v2.33.0 github.com/gin-gonic/gin v1.9.1 github.com/onsi/gocleanup v0.0.0-20140331211545-c1a5478700b5 + github.com/pkg/term v1.1.0 github.com/prashantgupta24/mac-sleep-notifier v1.0.1 github.com/sirupsen/logrus v1.9.3 github.com/spf13/cobra v1.8.0 diff --git a/go.sum b/go.sum index 24c3e60a..599179ca 100644 --- a/go.sum +++ b/go.sum @@ -116,6 +116,8 @@ github.com/pelletier/go-toml/v2 v2.1.1 h1:LWAJwfNvjQZCFIDKWYQaM62NcYeYViCmWIwmOS github.com/pelletier/go-toml/v2 v2.1.1/go.mod h1:tJU2Z3ZkXwnxa4DPO899bsyIoywizdUvyaeZurnPPDc= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= +github.com/pkg/term v1.1.0 h1:xIAAdCMh3QIAy+5FrE8Ad8XoDhEU4ufwbaSozViP9kk= +github.com/pkg/term v1.1.0/go.mod h1:E25nymQcrSllhX42Ok8MRm1+hyBdHY0dCeiKZ9jpNGw= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= @@ -195,6 +197,7 @@ golang.org/x/sys v0.0.0-20190916202348-b4ddaad3f8a3/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20191005200804-aed5e4c7ecf9/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20191120155948-bd437916bb0e/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20200909081042-eff7692f9009/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20201204225414-ed752295db88/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210112080510-489259a85091/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= diff --git a/pkg/config/json_test.go b/pkg/config/json_test.go index 72763f1f..88b3480f 100644 --- a/pkg/config/json_test.go +++ b/pkg/config/json_test.go @@ -232,7 +232,7 @@ var jsonStabilityTests = map[string]jsonStabilityTest{ }, "VirtioSerial": { obj: &VirtioSerial{}, - expectedJSON: `{"kind":"virtioserial","logFile":"LogFile","usesStdio":true}`, + expectedJSON: `{"kind":"virtioserial","logFile":"LogFile","ptyName":"PtyName","usesPty":true,"usesStdio":true}`, }, "VirtioVsock": { obj: &VirtioVsock{}, diff --git a/pkg/config/virtio.go b/pkg/config/virtio.go index 5523cf9a..10ddc92e 100644 --- a/pkg/config/virtio.go +++ b/pkg/config/virtio.go @@ -104,6 +104,10 @@ type VirtioNet struct { type VirtioSerial struct { LogFile string `json:"logFile,omitempty"` UsesStdio bool `json:"usesStdio,omitempty"` + UsesPty bool `json:"usesPty,omitempty"` + // PtyName must not be set when creating the VM, from a user perspective, it's read-only, + // vfkit will set it during VM startup. + PtyName string `json:"ptyName,omitempty"` } // TODO: Add VirtioBalloon @@ -195,12 +199,24 @@ func VirtioSerialNewStdio() (VirtioDevice, error) { }, nil } +func VirtioSerialNewPty() (VirtioDevice, error) { + return &VirtioSerial{ + UsesPty: true, + }, nil +} + func (dev *VirtioSerial) validate() error { if dev.LogFile != "" && dev.UsesStdio { return fmt.Errorf("'logFilePath' and 'stdio' cannot be set at the same time") } - if dev.LogFile == "" && !dev.UsesStdio { - return fmt.Errorf("one of 'logFilePath' or 'stdio' must be set") + if dev.LogFile != "" && dev.UsesPty { + return fmt.Errorf("'logFilePath' and 'pty' cannot be set at the same time") + } + if dev.UsesStdio && dev.UsesPty { + return fmt.Errorf("'stdio' and 'pty' cannot be set at the same time") + } + if dev.LogFile == "" && !dev.UsesStdio && !dev.UsesPty { + return fmt.Errorf("one of 'logFilePath', 'stdio' or 'pty' must be set") } return nil @@ -210,11 +226,16 @@ func (dev *VirtioSerial) ToCmdLine() ([]string, error) { if err := dev.validate(); err != nil { return nil, err } - if dev.UsesStdio { + switch { + case dev.UsesStdio: return []string{"--device", "virtio-serial,stdio"}, nil + case dev.UsesPty: + return []string{"--device", "virtio-serial,pty"}, nil + case dev.LogFile != "": + fallthrough + default: + return []string{"--device", fmt.Sprintf("virtio-serial,logFilePath=%s", dev.LogFile)}, nil } - - return []string{"--device", fmt.Sprintf("virtio-serial,logFilePath=%s", dev.LogFile)}, nil } func (dev *VirtioSerial) FromOptions(options []option) error { @@ -227,6 +248,8 @@ func (dev *VirtioSerial) FromOptions(options []option) error { return fmt.Errorf("unexpected value for virtio-serial 'stdio' option: %s", option.value) } dev.UsesStdio = true + case "pty": + dev.UsesPty = true default: return fmt.Errorf("unknown option for virtio-serial devices: %s", option.key) } diff --git a/pkg/config/virtio_test.go b/pkg/config/virtio_test.go index 371c1875..149ffd97 100644 --- a/pkg/config/virtio_test.go +++ b/pkg/config/virtio_test.go @@ -120,6 +120,13 @@ var virtioDevTests = map[string]virtioDevTest{ }, expectedCmdLine: []string{"--device", "virtio-serial,stdio"}, }, + "NewVirtioSerialPty": { + newDev: VirtioSerialNewPty, + expectedDev: &VirtioSerial{ + UsesPty: true, + }, + expectedCmdLine: []string{"--device", "virtio-serial,pty"}, + }, "NewVirtioNet": { newDev: func() (VirtioDevice, error) { return VirtioNetNew("") }, expectedDev: &VirtioNet{ diff --git a/pkg/vf/virtio.go b/pkg/vf/virtio.go index 38cec7a9..ec52689f 100644 --- a/pkg/vf/virtio.go +++ b/pkg/vf/virtio.go @@ -7,10 +7,12 @@ import ( "syscall" "github.com/crc-org/vfkit/pkg/config" + "github.com/onsi/gocleanup" + "golang.org/x/sys/unix" "github.com/Code-Hex/vz/v3" + "github.com/pkg/term/termios" log "github.com/sirupsen/logrus" - "golang.org/x/sys/unix" ) type RosettaShare config.RosettaShare @@ -221,17 +223,38 @@ func setRawMode(f *os.File) error { func (dev *VirtioSerial) toVz() (*vz.VirtioConsoleDeviceSerialPortConfiguration, error) { var serialPortAttachment vz.SerialPortAttachment - var err error - if dev.UsesStdio { + var retErr error + switch { + case dev.UsesStdio: if err := setRawMode(os.Stdin); err != nil { return nil, err } - serialPortAttachment, err = vz.NewFileHandleSerialPortAttachment(os.Stdin, os.Stdout) - } else { - serialPortAttachment, err = vz.NewFileSerialPortAttachment(dev.LogFile, false) + serialPortAttachment, retErr = vz.NewFileHandleSerialPortAttachment(os.Stdin, os.Stdout) + case dev.UsesPty: + master, slave, err := termios.Pty() + if err != nil { + return nil, err + } + // as far as I can tell, we have no use for the slave fd in the + // vfkit process, the user will open minicom/screen/... /dev/ttys00? + // when needed + defer slave.Close() + + // the master fd must stay open for vfkit's lifetime + gocleanup.Register(func() { _ = master.Close() }) + + dev.PtyName = slave.Name() + + if err := setRawMode(master); err != nil { + return nil, err + } + serialPortAttachment, retErr = vz.NewFileHandleSerialPortAttachment(master, master) + + default: + serialPortAttachment, retErr = vz.NewFileSerialPortAttachment(dev.LogFile, false) } - if err != nil { - return nil, err + if retErr != nil { + return nil, retErr } return vz.NewVirtioConsoleDeviceSerialPortConfiguration(serialPortAttachment) @@ -244,11 +267,17 @@ func (dev *VirtioSerial) AddToVirtualMachineConfig(vmConfig *VirtualMachineConfi if dev.UsesStdio { log.Infof("Adding stdio console") } + if dev.PtyName != "" { + return fmt.Errorf("VirtioSerial.PtyName must be empty (current value: %s)", dev.PtyName) + } consoleConfig, err := dev.toVz() if err != nil { return err } + if dev.UsesPty { + log.Infof("Using PTY (pty path: %s)", dev.PtyName) + } vmConfig.serialPortsConfiguration = append(vmConfig.serialPortsConfiguration, consoleConfig) return nil diff --git a/test/vm_test.go b/test/vm_test.go index 336ecea0..d63a3632 100644 --- a/test/vm_test.go +++ b/test/vm_test.go @@ -15,6 +15,7 @@ import ( "github.com/crc-org/vfkit/pkg/config" log "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -272,7 +273,7 @@ var pciidVersionedTests = map[int]map[string]pciidTest{ 14: pciidMacOS14Tests, } -func checkRestDevices(t *testing.T, vm *testVM) { +func restInspect(t *testing.T, vm *testVM) *config.VirtualMachine { tr := &http.Transport{ Dial: func(_, _ string) (conn net.Conn, err error) { return net.Dial("unix", vm.restSocketPath) @@ -287,7 +288,7 @@ func checkRestDevices(t *testing.T, vm *testVM) { var unmarshalledVM config.VirtualMachine err = json.Unmarshal(body, &unmarshalledVM) require.NoError(t, err) - require.Equal(t, vm.config, &unmarshalledVM) + return &unmarshalledVM } func testPCIId(t *testing.T, test pciidTest, provider OsProvider) { @@ -303,7 +304,9 @@ func testPCIId(t *testing.T, test pciidTest, provider OsProvider) { vm.Start(t) vm.WaitForSSH(t) checkPCIDevice(t, vm, test.vendorID, test.deviceID) - checkRestDevices(t, vm) + + unmarshalledVM := restInspect(t, vm) + require.Equal(t, vm.config, unmarshalledVM) vm.Stop(t) } @@ -334,6 +337,39 @@ func TestPCIIds(t *testing.T) { } } +func TestVirtioSerialPTY(t *testing.T) { + puipuiProvider := NewPuipuiProvider() + log.Info("fetching os image") + tempDir := t.TempDir() + err := puipuiProvider.Fetch(tempDir) + require.NoError(t, err) + + vm := NewTestVM(t, puipuiProvider) + defer vm.Close(t) + require.NotNil(t, vm) + + vm.AddSSH(t, "tcp") + dev, err := config.VirtioSerialNewPty() + require.NoError(t, err) + vm.AddDevice(t, dev) + + vm.Start(t) + vm.WaitForSSH(t) + runtimeVM := restInspect(t, vm) + var foundVirtioSerial bool + for _, dev := range runtimeVM.Devices { + runtimeDev, ok := dev.(*config.VirtioSerial) + if ok { + assert.NotEmpty(t, runtimeDev.PtyName) + foundVirtioSerial = true + break + } + } + require.True(t, foundVirtioSerial) + + vm.Stop(t) +} + func checkPCIDevice(t *testing.T, vm *testVM, vendorID, deviceID int) { re := regexp.MustCompile(fmt.Sprintf("(?m)[[:blank:]]%04x:%04x\n", vendorID, deviceID)) lspci, err := vm.SSHCombinedOutput(t, "lspci")