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 -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list