[Libvir] [QEMU 3/3] the qemu driver & daemon

[Daniel P. Berrange]

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  -=|