Re: [libvirt] RFC: virt-console

[John Levon <levon movementarian org>]

On Tue, Jul 01, 2008 at 07:35:56PM +0100, Daniel P. Berrange wrote:

> > http://cr.opensolaris.org/~johnlev/virt-console/
> > 
> > --- libvirt-0.4.0/src/Makefile.in	2008-06-27 06:17:20.865621099 -0700
> > +++ libvirt-1/src/Makefile.in	2008-06-27 06:19:55.983265305 -0700
> 
> You can exclude Makefile.in from patches, since its auto-generated.

Yes, I know, but not for our bits (long story, hopefully fixed soon).

> > +    char *argv[] = { "/usr/bin/virt-console", NULL, NULL, NULL, NULL };
> 
> This should probably be
> 
>     char *argv[] = { BINDIR "/virt-console", NULL, NULL, NULL, NULL };
> 
> so that it auto-adjusts when people pass --prefix to configure

Sure.

> > +    waitpid(pid, NULL, 0);
> 
> Ought to do this in a while loop to handle EINTR.

OK, although I'm not sure it really matters here?

> > --- /dev/null	2008-06-30 17:03:12.000000000 -0700
> > +++ libvirt-new/src/virt-console.c	2008-06-30 16:58:36.079071463 -0700
> 
> I'd prefer to keep the source in  the 'console.c' file instead of renaming
> it, just to make historical diff tracking a little easier.

Really? Surely even subversion can do cross-rename tracking OK?

> > +    *conn = virConnectOpenAuth(conn_name, virConnectAuthPtrDefault, 0);
> 
> We ought to pass VIR_CONNECT_RO as the 3rd arg here, since the console
> doesn't require write access.

OK.

> The  virXPathString() method from xml.h will simplify the following handling
> Can use  virStrToLong_i  from util.h here.

The perils of borrowing code, everyone wants to clean it up. Sure :)

> I think we probably need to wait a little longer than this - 5 times with a
> 1 second sleep perhaps. Under heavy load it can take a while for reboot to
> complete

Yeah, you might be right. Though it's possible to break even this most
likely.

> I like the support for re-connecting after reboot. At the same time I
> wonder if we need to make the an optional command line flag. Some apps
> using virsh console, may rely on the fact that it exits when a VM 
> shuts down. 

I hate --behave-like-you-should flags and will do everything I can to
avoid them. Let's not inconvenience everybody for the sake of some
possible breakage. The perils of people coding to human interfaces. (I
wanted to make --verbose the default, like telnet, but that seemed much
more likely to break someone's scripts).

I'm not adverse to a disconnect-on-shutdown flag, if people think it
would be useful, which would at least make any such breakage that might
exist easy to fix.

Thanks for the review Dan.

regards,
john