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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list