Re: [libvirt] RFC: virt-console
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