[libvirt] RFC: virt-console
John Levon
levon at movementarian.org
Tue Jul 1 18:50:02 UTC 2008
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
More information about the libvir-list
mailing list