[Libvir] PATCH: Process QEMU monitor at startup

[Mark McLoughlin]

Hi Dan,

On Fri, 2007-03-02 at 14:35 +0000, Daniel P. Berrange wrote:
>  
> +static int qemudOpenMonitor(struct qemud_vm *vm, const char *monitor)
> {

#define MONITOR_POLL_TIMEOUT 5

> +    int monfd;
> +    char buffer[1024];
> +    int got = 0;
> +
> +    if (!(monfd = open(monitor, O_RDWR))) {
> +        return -1;
> +    }
> +    if (qemudSetCloseExec(monfd) < 0)
> +        goto error;
> +    if (qemudSetNonBlock(monfd) < 0)
> +        goto error;
> +
> +    /* Consume & discard the initial greeting */
> +    for(;;) {
> +        int ret;
> +
> +        ret = read(monfd, buffer+got, sizeof(buffer)-got-1);

	What happens if the buffer fills up ?

> +        if (ret == 0)
> +            goto error;
> +        if (ret < 0) {
> +            struct pollfd fd = { .fd = monfd, .events = POLLIN };
> +            if (errno != EAGAIN &&
> +                errno != EINTR)
> +                goto error;
> +
> +            ret = poll(&fd, 1, 5);

               ret = poll(&fd, 1, MONITOR_POLL_TIMEOUT);

	Giving up after 5 milliseconds? Are we that afraid of commitment?

> +static int qemudWaitForMonitor(struct qemud_vm *vm) {

	We don't set an error in here, so how about returning the appropriate
errno from the function?

> +    char buffer[1024]; /* Plenty of space to get startup greeting */
> +    int got = 0;
> +
> +    for (;;) {
> +        int ret;
> +
> +        ret = read(vm->stderr, buffer+got, sizeof(buffer)-got-1);
> +        if (ret == 0) {
> +            return -1;
> +        }
> +        if (ret < 0) {
> +            struct pollfd fd = { .fd = vm->stderr, .events =
> POLLIN };
> +            if (errno != EAGAIN &&
> +                errno != EINTR) {
> +                return -1;
> +            }
> +
> +            ret = poll(&fd, 1, 5000);

	Again, a #define for the timeout would be nice

> +            if (ret == 0) {
> +                return -1;
> +            } else if (ret < 0) {
> +                if (errno != EINTR)
> +                    return -1;
> +            } else if (fd.revents != POLLIN) {
> +                return -1;
> +            }
> +        } else {
> +            char monitor[100];
> +            char *tmp = buffer;
> +            got += ret;
> +            buffer[got] = '\0';
> +            while (tmp && *tmp) {
> +                if (sscanf(tmp, "char device redirected to %19s", monitor) == 1) {

	Path length of 100, maximum field with of 19? That's all rather
voodooish ... hope about strstr() to find it, buffer length of PATH_MAX
and then just strncpy()? Also, it'd be nice to split that out into e.g.
qemudMonitorPathFromStr(buffer, monitorPath, PATH_MAX)

> +                    if (qemudOpenMonitor(vm, monitor) < 0)
> +                        return -1;
> +                    return 0;
> +                }
> +                tmp = index(tmp, '\n');

	index() is a bit odd, why not strstr() ?

> +static int qemudNextFreeVNCPort(struct qemud_server *server
> ATTRIBUTE_UNUSED) {

	I don't really know the context, but it'd be much nicer if we could
just QEMU find a free port itself and then we query the port - e.g. the
obvious race condition.

> +    int i;
> +
> +    for (i = 5900 ; i < 6000 ; i++) {
> +        int fd;
> +        struct sockaddr_in addr;
> +        addr.sin_family = AF_INET;
> +        addr.sin_port = htons(i);
> +        addr.sin_addr.s_addr = htonl(INADDR_ANY);
> +        fd = socket(PF_INET, SOCK_STREAM, 0);
> +        if (fd < 0)
> +            return -1;
> +
> +        if (connect(fd, (struct sockaddr*)&addr, sizeof(addr)) == 0)

	Um, why not bind() (with SO_REUSEADDR)?

>          ret = 0;
> +
> +        if (qemudWaitForMonitor(vm) < 0) {

	Set an error based on the returned errno ...

>          struct qemud_vm *next = vm->next;
> @@ -1356,6 +1426,7 @@ static int qemudDispatchPoll(struct qemu
>          if (stderrfd != -1) {
>              if (!failed) {
>                  if (fds[fd].revents) {
> +                    printf("%d\n", fds[fd].revents);

	Kill this

Cheers,
Mark.