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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/