Re: [Libvir] OpenVZ driver enhancements

On Thu, Aug 23, 2007 at 02:33:42PM +0530, Shuveb Hussain wrote: 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. 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. 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 ? 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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/