On Mon, Aug 28, 2006 at 04:55:09AM -0400, Daniel Veillard wrote:
> On Mon, Aug 28, 2006 at 12:18:52AM +0100, Daniel P. Berrange wrote:
> > There is intended to be one qemud instance per UNIX user - the so called
> > 'session' instance, and optionally one system global instance runing as
> > root (or a system daemon account?) - the so called 'system' instance. When
> > using libvirt, the URIs for each are 'qemu:///session' and 'qemu:///system'
> > respectively.
>
> I'm wondering what would be the role of the system instance. Aggregate
> the informations from the set of users running qemu when running as root ?
Well, the session instance is intended to fill the desktop user use-case, while
the system instance is more aimed at a data-center use case where an admin
might set up a bunch of persistently running QEMU instances on a server.
> > The UNIX socket for session instances is $HOME/.qemud and is
> > chmod 0700 - no support is provided for users connecting to each other's
> > instances. Although we could trivially extend to create a read-only socket
> > $HOME/.qemud-ro if desired. The system instance is currently not finished
> > but intended to run in /var/run/qemud or some such.
>
> hum .... I'm not too fond of putting the socket directly in the user
> home directory, I would feel better with a .qemud directory, protected 700
> by default and then have the socket mapped in that directory, I think it is
> more secure, a bit cleaner to not have socket in home dir, and gives a place
> to store informations if needed. Actually you already create .qemud.d for
> storage, can we just make it .qemud and store the socket there ?
>
> Or we can use a socket not mapped in the filesystem at all, based on the
> user name, which usually avoid the race when checking and then opening.
Actually, the socket is already in the abstract namespace - i use the
abstract path '\0$HOME/.qemud' - could easily put it in a dir below
though
> > +static int qemudParseUUID(const char *uuid,
> > + unsigned char *rawuuid) {
>
> Should we make a lib subdir to avoid duplicating code like this ?
> Alone that doesn't sound worth it, but if there is more routines duplicated
> could be useful.
Ues, as karel pointed out, there are a number of utility functions from libvirt
that I duplicated in the qemud/ directory primarily because I wanted to avoid
the circular depedancy back onto libvirt from the daemon. I think it would be
worthwhile having the various helper routines in some lib that we can then pull
into both libvirt & the daemon as needed.
>
> > + //virXMLError(VIR_ERR_NO_SOURCE, (const char *) target, 0);
> > + //virXMLError(VIR_ERR_NO_TARGET, (const char *) source, 0);
> > + printf("Bogus floppy\n");
> > + printf("Bogus cdomr\n");
> > + printf("alse %s %s\n", device, target);
>
> Need to unify and make the usual wrappers for error code.
> Even if not linked in the libvirt lib, so that error messages
> could be carried back at the protocol level, if we put much logic in
> the qemud we will need some logging facilities, or transport errors, right ?
Yes, I need to rip out the debug code & formalize the set of error codes
which can be returned to libvirt. Currently I catch every error and just
return a generic -1, internal error. Obviously this needs to be better
before the patch is merged.
> > + obj = xmlXPathEval(BAD_CAST "/domain/devices/graphics", ctxt);
> > + if ((obj == NULL) || (obj->type != XPATH_NODESET) ||
> > + (obj->nodesetval == NULL) || (obj->nodesetval->nodeNr == 0)) {
> > + vm->def.graphicsType = QEMUD_GRAPHICS_NONE;
> > + } else {
> > + prop = xmlGetProp(obj->nodesetval->nodeTab[0], BAD_CAST "type");
> > + if (!strcmp((char *)prop, "vnc")) {
> > + vm->def.graphicsType = QEMUD_GRAPHICS_VNC;
> > + prop = xmlGetProp(obj->nodesetval->nodeTab[0], BAD_CAST "port");
> > + if (prop) {
> > + conv = NULL;
> > + vm->def.vncPort = strtoll((const char*)prop, &conv, 10);
> > + } else {
> > + vm->def.vncPort = 0;
> > + }
> > + }
> > + }
>
> Hum ... if we have no graphic device what's going on ? the QEmu process
> is forked by a daemon, so there is no way to get back to the console...
> The console access is something we didn't tried to address so far in libvirt.
> At least some console logs would be useful, otherwise we have the
> problem of hitting asynchronous interface is we really want to give interactive
> access... Well we can probably live without :-)
There's a couple of different ways this can work - the VNC console, or we can
hook up a serial device to the guest (and assume user puts appropriate kernl
boot command line args in 'console=ttyS0'. I also expect to save the stderr
and stdout from QEMU to a log file so user can see if stuff goes wrong. I
expect VNC is the primary console though.
> > + if (stat(server->configDir, &sb) < 0) {
> > + if (errno == ENOENT) {
> > + if (mkdir(server->configDir, 0700) < 0)
> > + return -1;
> > + } else {
> > + return -1;
> > + }
> > + } else if (!S_ISDIR(sb.st_mode)) {
> > + return -1;
> > + }
>
> Shouldn't we check the mode and owner if the directory already exists ?
> that should be in the stat informations.
Yes, worthwhile doing.
>
> > + if (qemudFindVMByUUID(server, vm->def.uuid)) {
> > + qemudFreeVM(vm);
> > + printf("Duplicate domains, discarding %s\n", file);
> > + return NULL;
> > + }
>
> UUID clashes due to users manually copying files may be more frequent than
> expected. Cloning is gonna be fun in general ...
Yes, I'm not entirely sure what we can do there - either document - "do not
do that", or automatically re-generate the UUID if they have a duplicate (re-saving
the cofig file to disk with the new UUID). I believe VMWare automatically
re-generates the UUID if you manually copy a file, so its a reasonable
approach to take. Could do the same with the Name if neccessary - just append
'-1', '-2', or something on to the end of it.
> > + if (qemudFindVMByName(server, vm->def.name)) {
> > + qemudFreeVM(vm);
> > + printf("Duplicate domains, discarding %s\n", file);
> > + return NULL;
> > + }
> [...]
> > +int qemudScanConfigs(struct qemud_server *server) {
>
> 2 things here:
> 1/ caching if the modification timestamps of the directory and file
> didn't changed. I don't know how often the scanning will be done
> but it's not lightweight since it reparses everything
> 2/ name and uuid clashes could be detected here or when adding some.
If we wanted to be really clever we could use INotify to automatically
re-scan. A simple approach though would just re-scan the directory comparing
timestamps every minute or so & re-loading configs which have changed.
> I scanned quickly the protocol part but I need to get my head around it :-)
There's a couple of important assumptions I apply in the network code which
I should document further to help explain the way it works.
> That's a lot of code ! One think I would like is to try to get all commenting
> at the same level, i.e. having every function carrying a comment header,
> That's something I will help with, maybe as a side effect of reviewing the
> code. Tell me how we could do that ? Maybe I could start with the simplest
> code from qemud.
>
> > -#define MAX_DRIVERS 5
> > +#define MAX_DRIVERS 10
>
> I didn't expect that initially :-)
Hehe - took me a little while to track down that issue too - I just could
not work out why nothing was working at first :-)
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 -=|