[Libvir] PATCH: Process QEMU monitor at startup

[Daniel P. Berrange]

On Fri, Mar 02, 2007 at 03:06:56PM +0000, Mark McLoughlin wrote:
> 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 ?

Sorry, wrong version of the patch - the for(;;) should have been
    while (got < (sizeof(buffer)-1)) {

I'll repost the correct patch when I've done of the other fixes.

> 
> > +        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?

Again, wrong patch - it should have been 5 seconds.

> > +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?

I'll add in some error reporting.

> > +                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)

I'll have a go at splitting it out.

> 
> > +                    if (qemudOpenMonitor(vm, monitor) < 0)
> > +                        return -1;
> > +                    return 0;
> > +                }
> > +                tmp = index(tmp, '\n');
> 
> 	index() is a bit odd, why not strstr() ?

Well we're only looking for a single character match - index() is for
single characters, strstr() is for strings.

> > +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.

Currently QEMU can't auto-allocate itself a patch - I'm planning to port
the Xen auto-allocation patch to mainline QEMU. Until then, this code is
better than the the current situation, even though there is an obvious
race.

> > +        if (connect(fd, (struct sockaddr*)&addr, sizeof(addr)) == 0)
> 
> 	Um, why not bind() (with SO_REUSEADDR)?

No particular reason - either will work just fine.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|