[Libvir] OpenVZ driver enhancements

[Daniel Veillard]

On Thu, Aug 23, 2007 at 02:33:42PM +0530, Shuveb Hussain wrote:
> Hi,
> 
> I'm glad to make available patches for the OpenVZ driver that provide
> the following features:

  Okay looking at those:

-        if (!memcmp(vm->vmdef->uuid, uuid, VIR_UUID_BUFLEN))
+        if (!memcmp(vm->vmdef->uuid, uuid, OPENVZ_UUID_MAX))

 not okay, I don't see why you should have your own definition for 
VIR_UUID_BUFLEN makes no sense to me, and Dan Berrange took some time to
clean this up last week, to not revert that cleanup patch.

+    if (!
+        (xml =
+         xmlReadDoc(BAD_CAST xmlStr,
+                    displayName ? displayName : "domain.xml", NULL,

 that's really bad formating !
 If you don't have enough space, then pake the assignment first, then
test the value !

+    obj =
+        xmlXPathEval(BAD_CAST "string(/domain/container/profile[1])",
+                     ctxt);

  there is many places where you have that formatting too, keep the xmlXPathEval
on the same line, it fits in 80 colums

-        virUUIDGenerate(uuid);
-        virUUIDFormat(uuid, uuidstr);
+        virUUIDGenerate(new_uuid);
+        bzero(uuid, VIR_UUID_STRING_BUFLEN);
+        for(i = 0; i < VIR_UUID_BUFLEN; i++)
+            sprintf(uuid + (i * 2), "%02x", (unsigned char) new_uuid[i]);

  why ? If the common routines are not okay tehy should be fixed, but we
should not do this kind of duplication

---------------
 struct openvz_vm_def {
     char name[OPENVZ_NAME_MAX];
-    unsigned char uuid[VIR_UUID_BUFLEN];
+    unsigned char uuid[OPENVZ_UUID_MAX];
---------------

   Why keeping OpenVZ specific constantes for UUID lenght ?

---------------
     virDomainPtr dom;

     if (!vm) {
-        error(conn, VIR_ERR_INTERNAL_ERROR, "no domain with matching uuid");
+        error(dom->conn, VIR_ERR_INVALID_DOMAIN, "no domain with matching uuid");
         return NULL;
     }
---------------

  that can't work, you're dereferencing an uninitialized variable, change is
just wrong.

  in openvzDomainDefineXML() you call directly
+    strcat(cmdbuf, " >/dev/null 2>&1");
+    ret = system(cmdbuf);

   didn't we had proper encapsulating functions to avoid going though system()
I think it's something Dan Berrange pointed out on previous patch and this 
had been fixed. Same problem in openvzDomainCreateLinux(), and 
openvzDomainUndefine() the calls to fork and execute a system command should
really be encapsulated and going though a single proper routine.

also at some point

+#define openvzLog(level, msg...) { if(level == OPENVZ_WARN) \

should be replaced by a proper routine, not directly calling fprintf(stderr
but setting up the proper virError structures for integration of error
handling like other parts of libvirt, but that can be postponed for now.

> * virDomainDefineXML - Defines an OpenVZ domain and does not start it.
> Takes XML description of the domain as input.
> * virDomainCreateLinux - Starts a domain based on the provided XML
> description. There is no way to start a domain in OpenVZ without
> defining it. So, it is defined anyway, as of now. :-( There may be a way
> to get around this. We are looking into it. As of now, treat this as
> define + start.
> * virDomainUndefine - removes domain from OpenVZ management. Since
> OpenVZ manages the domain's root file system, it is also lost. This
> behaviour is different from Xen.

  I didn't tried yet, I think it would be better to cleanup the patches
a little bit. I have no doubt it would work, my concerns are more code
maintainance/readability, and make sure there is no leak.

> The XML Format for OpenVZ:
> --------------------------
> 
> <domain type='openvz'>
> 	<name>108</name>
> 	<uuid>8dea22b31d52d8f32516782e98ab8fa0</uuid>
> 	<container>
> 		<filesystem>
> 			<template>fedora-core-3-i386-minimal</template>
> 			<quota level = 'first'>123</quota>
> 			<quota level = 'second' uid = '500'>534</quota>
> 		</filesystem>
> 		<network>
> 			<ipaddress>192.168.1.108</ipaddress>
> 			<hostname>fedora-minimal</hostname>
> 			<gateway>192.168.1.1</gateway>
> 			<nameserver>192.168.1.1</nameserver>
> 			<netmask>255.255.255.0</netmask>
> 		</network>
> 		<profile>vps.basic</profile>
> 	</container>
> </domain>
> 
> The name is the VPS "ID". The VPS ID is not temporary in OpenVZ as in
> Xen. The "<template>" tag in the "<filesystem>" section tells libvirt
> which OS template cache to use to create the VPS file system. Quota is
> not implemented as yet. First and second level quotas are intended to be
> supported. The "<profile>" tag must be a valid profile name from which
> VPS parameter are inherited. Other things, I guess are self explanatory.

  I assume the network children are similar to the one used on virtual network
definition as it was pointed out tehy should be harmonized, right ?

> Other issues:
> -------------
> * Moved some static declarations from the header to the .c files.
> * Fixed a small bug that cause libvirtd to crash on remote client exit.
> 
> Patches are against CVS head.

  Okay, thanks but I guess we need a little bit of cleanup first :-)

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/