Skip to content
Closed
2 changes: 1 addition & 1 deletion CODINGSTYLE.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ coding style should follow the

You may use tools like
[uncrustify](http://uncrustify.sourceforge.net/) with this
[config file](https://github.com/freebsd/pkg/blob/master/freebsd.cfg)
[config file](https://raw.githubusercontent.com/freebsd/pkg/master/freebsd.cfg)
for *new* code, though the result may not be perfect.

Keep in mind that, especially for most of the `bhyve` derived code, it
Expand Down
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ HYPERKIT_LIB_SRC := \
src/lib/fwctl.c \
src/lib/inout.c \
src/lib/ioapic.c \
src/lib/log.c \
src/lib/md5c.c \
src/lib/mem.c \
src/lib/mevent.c \
Expand Down
68 changes: 44 additions & 24 deletions go/hyperkit.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ const (
ConsoleStdio = iota
// ConsoleFile configures console to a tty and output to a file
ConsoleFile
// ConsoleLog configures console to a tty and sends its contents to the logs
ConsoleLog

defaultVPNKitSock = "Library/Containers/com.docker.docker/Data/s50"

Expand Down Expand Up @@ -117,8 +119,7 @@ type HyperKit struct {
// Memory is the amount of megabytes of memory for the VM.
Memory int `json:"memory"`

// Console defines where the console of the VM should be
// connected to. ConsoleStdio and ConsoleFile are supported.
// Console defines where the console of the VM should be connected to.
Console int `json:"console"`

// Below here are internal members, but they are exported so
Expand Down Expand Up @@ -199,11 +200,15 @@ func (h *HyperKit) check() error {
log.Debugf("hyperkit: check %#v", h)
var err error
// Sanity checks on configuration
if h.Console == ConsoleFile && h.StateDir == "" {
return fmt.Errorf("If ConsoleFile is set, StateDir must be specified")
}
if h.Console == ConsoleStdio && !isTerminal(os.Stdout) && h.StateDir == "" {
return fmt.Errorf("If ConsoleStdio is set but stdio is not a terminal, StateDir must be specified")
switch h.Console {
case ConsoleFile, ConsoleLog:
if h.StateDir == "" {
return fmt.Errorf("If ConsoleFile or ConsoleLog is set, StateDir must be specified")
}
case ConsoleStdio:
if !isTerminal(os.Stdout) && h.StateDir == "" {
return fmt.Errorf("If ConsoleStdio is set but stdio is not a terminal, StateDir must be specified")
}
}
for _, image := range h.ISOImages {
if _, err = os.Stat(image); os.IsNotExist(err) {
Expand Down Expand Up @@ -420,10 +425,20 @@ func (h *HyperKit) buildArgs(cmdline string) {
nextSlot++
}

if h.Console == ConsoleStdio && isTerminal(os.Stdout) {
a = append(a, "-l", "com1,stdio")
} else if h.StateDir != "" {
a = append(a, "-l", fmt.Sprintf("com1,autopty=%s/tty,log=%s/console-ring", h.StateDir, h.StateDir))
// -l: LPC device configuration.
{
cfg := "com1"
if h.Console == ConsoleStdio && isTerminal(os.Stdout) {
cfg += fmt.Sprintf(",stdio")
} else {
cfg += fmt.Sprintf(",autopty=%s/tty", h.StateDir)
}
if h.Console == ConsoleLog {
cfg += fmt.Sprintf(",asl")
} else {
cfg += fmt.Sprintf(",log=%s/console-ring", h.StateDir)
}
a = append(a, "-l", cfg)
}

if h.Bootrom == "" {
Expand All @@ -440,6 +455,22 @@ func (h *HyperKit) buildArgs(cmdline string) {
log.Debugf("hyperkit: CmdLine: %#v", h.CmdLine)
}

// openTTY opens the tty files for reading, and returns it.
func (h *HyperKit) openTTY() *os.File {
path := fmt.Sprintf("%s/tty", h.StateDir)
for {
if res, err := os.OpenFile(path, os.O_RDONLY, 0); err != nil {
log.Infof("hyperkit: openTTY: %v, retrying", err)
time.Sleep(10 * time.Millisecond)
} else {
log.Infof("hyperkit: openTTY: got %v", path)
saneTerminal(res)
setRaw(res)
return res
}
}
}

// execute forges the command to run hyperkit, runs and returns it.
// It also plumbs stdin/stdout/stderr.
func (h *HyperKit) execute() (*exec.Cmd, error) {
Expand Down Expand Up @@ -469,20 +500,9 @@ func (h *HyperKit) execute() (*exec.Cmd, error) {
cmd.Stderr = os.Stderr
} else {
go func() {
ttyPath := fmt.Sprintf("%s/tty", h.StateDir)
var tty *os.File
for {
var err error
tty, err = os.OpenFile(ttyPath, os.O_RDONLY, 0)
if err == nil {
break
}
time.Sleep(10 * time.Millisecond)
}
saneTerminal(tty)
setRaw(tty)
tty := h.openTTY()
defer tty.Close()
io.Copy(os.Stdout, tty)
tty.Close()
}()
}
} else if log != nil {
Expand Down
7 changes: 7 additions & 0 deletions src/include/xhyve/log.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#pragma once

/* Initialize ASL logger and local buffer. */
void log_init(void);

/* Send one character to the logger: wait for full lines before actually sending. */
void log_put(uint8_t _c);
50 changes: 50 additions & 0 deletions src/lib/log.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#include <asl.h>
#include <pwd.h>
#include <fcntl.h>
#include <stdio.h>
#include <time.h>

#include <SystemConfiguration/SystemConfiguration.h>

#include <xhyve/log.h>

static aslclient log_client = NULL;
static aslmsg log_msg = NULL;

static unsigned char buf[4096];
/* Index of the _next_ character to insert in the buffer. */
static size_t buf_idx = 0;

/* asl is deprecated in favor of os_log starting with macOS 10.12. */
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"

/* Initialize ASL logger and local buffer. */
void log_init(void)
{
log_client = asl_open(NULL, NULL, 0);
log_msg = asl_new(ASL_TYPE_MSG);
}


/* Send the content of the buffer to the logger. */
static void log_flush(void)
{
buf[buf_idx] = 0;
asl_log(log_client, log_msg, ASL_LEVEL_NOTICE, "%s", buf);
Copy link
Member

@rn rn Apr 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I read the code which calls this function correctly in neither case where this function is called do you null terminate buf. So, how does asl_log() know how long buf is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rn. Again, I'm sorry, but I don't know what you mean. It seems to me that you missed line 32, or I misunderstand your sentence.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, my bad. shouldn't review code without having coffee before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄

buf_idx = 0;
}


/* Send one character to the logger: wait for full lines before actually sending. */
void log_put(uint8_t c)
{
if ((c == '\n') || (c == 0)) {
log_flush();
} else {
if (buf_idx + 2 >= sizeof(buf)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this now seems to waste one byte.

But more in general maybe have something like:

if (buf_idx + 2 >= sizeof(buf)) {
    buf[buf_idx++] = c;
    c = 0;
}
if ((c == '\n') || (c == 0)) {
    buf[buf_idx] = 0;
    asl_log(log_client, log_msg, ASL_LEVEL_NOTICE, "%s", buf);
    buf_idx = 0;
} else {
    buf[buf_idx++] = c;
}

This would keep everything in one function, but it's a matter of taste, i guess

Copy link
Contributor Author

@akimd akimd Apr 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I guess. I dislike assignments such as c=0 (I try to stick to some SSA form), and side-effects in expressions such as buf[buf_idx++]. Besides, in some situations you are now adding a \n (and 0) in the buffer.

log_flush();
}
buf[buf_idx] = c;
++buf_idx;
}
}
34 changes: 21 additions & 13 deletions src/lib/uart_emul.c
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,14 @@
#include <assert.h>
#include <errno.h>
#include <sys/mman.h>
#include <xhyve/log.h>
#include <xhyve/support/ns16550.h>
#include <xhyve/mevent.h>
#include <xhyve/uart_emul.h>

#define COM1_BASE 0x3F8
#define COM1_BASE 0x3F8
#define COM1_IRQ 4
#define COM2_BASE 0x2F8
#define COM2_BASE 0x2F8
#define COM2_IRQ 3

#define DEFAULT_RCLK 1843200
Expand Down Expand Up @@ -91,7 +92,7 @@ struct fifo {
struct ttyfd {
bool opened;
int fd; /* tty device file descriptor */
int sfd;
int sfd;
char *name; /* slave pty name when using autopty*/
struct termios tio_orig, tio_new; /* I/O Terminals */
};
Expand Down Expand Up @@ -121,6 +122,7 @@ struct uart_softc {

struct ttyfd tty;
struct log log;
bool asl; /* Output to Apple logger. */
bool thre_int_pending; /* THRE interrupt pending */

void *arg;
Expand Down Expand Up @@ -427,7 +429,7 @@ uart_write(struct uart_softc *sc, int offset, uint8_t value)
}
}

switch (offset) {
switch (offset) {
case REG_DATA:
if (sc->mcr & MCR_LOOPBACK) {
if (rxfifo_putchar(sc, value) != 0)
Expand All @@ -436,6 +438,8 @@ uart_write(struct uart_softc *sc, int offset, uint8_t value)
ttywrite(&sc->tty, value);
if (sc->log.ring)
ringwrite(&sc->log, value);
if (sc->asl)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

below it looks like the asl and log are exclusive, as a else if is used while here it seems as if they can be used together.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see that they are exclusive. What do you mean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I misread the snippet of code from below. This is fine.

log_put(value);
} /* else drop on floor */
sc->thre_int_pending = true;
break;
Expand Down Expand Up @@ -681,15 +685,15 @@ uart_tty_backend(struct uart_softc *sc, const char *backend)
static char *
copy_up_to_comma(const char *from)
{
char *comma = strchr(from, ',');
char *tmp = NULL;
if (comma == NULL) {
tmp = strdup(from); /* rest of string */
} else {
ptrdiff_t length = comma - from;
tmp = strndup(from, (size_t)length);
}
return tmp;
char *comma = strchr(from, ',');
char *tmp = NULL;
if (comma == NULL) {
tmp = strdup(from); /* rest of string */
} else {
ptrdiff_t length = comma - from;
tmp = strndup(from, (size_t)length);
}
return tmp;
}

int
Expand Down Expand Up @@ -773,6 +777,10 @@ uart_set_backend(struct uart_softc *sc, const char *backend, const char *devname
if (uart_mapring(sc, logname) == -1) {
goto err;
}
} else if (strcmp("asl", backend) == 0) {
sc->asl = true;
log_init();
retval = 0;
} else if (uart_tty_backend(sc, backend) == 0) {
retval = 0;
} else {
Expand Down