12:22 p.m.
I've updated the patch with the changes discussed in this mail.
Patch just posted.
Regards,
Sharadha
-----Original Message-----
From: Matthias Bolte [mailto:matthias.bolte@googlemail.com]
Sent: 03 March 2010 15:28
To: Sharadha Prabhakar (3P)
Cc: libvir-list(a)redhat.com; Ewan Mellor
Subject: Re: status on review comments
2010/3/3 Sharadha Prabhakar (3P) <sharadha.prabhakar(a)citrix.com>:
I've sent a patch containing most of the changes you'd
suggested, except the
Following ones. My comments inline.
> diff -Nur ./libvirt_org/src/xenapi/xenapi_driver.c
./libvirt/src/xenapi/xenapi_driver.c
> --- ./libvirt_org/src/xenapi/xenapi_driver.c 1970-01-01 01:00:00.000000000 +0100
> +++ ./libvirt/src/xenapi/xenapi_driver.c 2010-02-26 15:27:00.000000000 +0000
> @@ -0,0 +1,1564 @@
> +
> +/*
> + * xenapi_driver.c: Xen API driver.
> + * Copyright (C) 2009 Citrix Ltd.
> + * Sharadha Prabhakar <sharadha.prabhakar(a)citrix.com>
> +*/
> +
> +
> +char *url=NULL;
>You should move this into the xenapiPrivate struct, otherwise you'll
>have trouble using multiple XenAPI connections at the same time,
>because multiple calls to xenapiOpen will overwrite the pointer an
>leak the previous value.
url is passed to call_func() which is used by curl to talk to the server.
Call_func() doesn't have access to 'conn', hence it can't be embedded
there.
I'll figure out a way to do this. The recent patch also has a SSL_verfiy flag
which is global and used by call_func that should also be embedded similarly.
The second parameter for xen_session_login_with_password is a void
pointer for user data. You can pass a pointer to the xenapiPrivate
struct there. Then libxenserver will pass it to the call_func function
as the user_handle parameter (I just verified this by looking at the
libxenserver codebase).
Regarding the no_verify query parameter: You should look at
esxUtil_ParseQuery how the qparam_query_parse function is used there
instead of parsing the URI yourself using strtok_r.
> +*
> +* Returns OS version on success or NULL in case of error
> +*/
> +static char *
> +xenapiDomainGetOSType (virDomainPtr dom)
> +{
> + /* vm.get_os-version */
> + int i;
> + xen_vm vm;
> + char *os_version=NULL;
> + xen_vm_record *record;
> + xen_string_string_map *result;
> + char uuid[VIR_UUID_STRING_BUFLEN];
> + xen_session *session = ((struct _xenapiPrivate
*)(dom->conn->privateData))->session;
> + virUUIDFormat(dom->uuid,uuid);
> + if (xen_vm_get_by_uuid(session, &vm, uuid)) {
> + xen_vm_get_record(session, &record, vm);
> + if (record) {
> + xen_vm_guest_metrics_get_os_version(session, &result,
record->guest_metrics->u.handle);
> + if (result) {
> + for (i=0; i<(result->size); i++) {
> + if (STREQ(result->contents[i].key, "distro")) {
> + if (STREQ(result->contents[i].val, "windows"))
{
>Is distro != windows a good indicator for paravirtualization mode? How
>do you detect the case when you have a non-windows system in HVM mode?
As of now, the hypervisor supports only windows in HVM.
I already installed Linux in Xen's HVM mode, that's why I asked. In
that case your code would report paravirtualization mode, instead of
HVM.
> diff -Nur ./libvirt_org/src/xenapi/xenapi_utils.c
./libvirt/src/xenapi/xenapi_utils.c
> --- ./libvirt_org/src/xenapi/xenapi_utils.c 1970-01-01 01:00:00.000000000 +0100
> +++ ./libvirt/src/xenapi/xenapi_utils.c 2010-02-26 15:49:24.000000000 +0000
> @@ -0,0 +1,433 @@
> +/*
> + * xenapi_utils.c: Xen API driver -- utils parts.
> + * Copyright (C) 2009 Citrix Ltd.
> + * Sharadha Prabhakar <sharadha.prabhakar(a)citrix.com>
> + */
> +
> +
> +/* converts bitmap to string of the form '1,2...' */
> +char *
> +mapDomainPinVcpu(unsigned int vcpu, unsigned char *cpumap, int maplen)
> +{
> + char buf[VIR_UUID_BUFLEN], mapstr[sizeof(cpumap_t) * 64];
Okay, you could change it like this:
- char buf[VIR_UUID_BUFLEN], mapstr[sizeof(cpumap_t) * 64];
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
+ size_t len;
> + char *ret=NULL;
> + int i, j;
> + mapstr[0] = 0;
- mapstr[0] = 0;
> + for (i = 0; i < maplen; i++) {
> + for (j = 0; j < 8; j++) {
> + if (cpumap[i] & (1 << j)) {
> + snprintf(buf, sizeof(buf), "%d,", (8 * i) + j);
> + strcat(mapstr, buf);
>Use the virBuffer API instea of snprintf and strcat.
- snprintf(buf, sizeof(buf), "%d,", (8 * i) + j);
- strcat(mapstr, buf);
+ virBufferVSprintf(&buf, "%d,", (8 * i) + j);
The buffer calls append new content and don't overwrite the existing
buffer content.
> + }
> + }
> + }
> + mapstr[strlen(mapstr) - 1] = 0;
> + snprintf(buf, sizeof(buf), "%d", vcpu);
> + ret = strdup(mapstr);
>Use virAsprintf instead of snprintf and strdup.
- mapstr[strlen(mapstr) - 1] = 0;
- snprintf(buf, sizeof(buf), "%d", vcpu);
- ret = strdup(mapstr);
+ if (virBufferError(&buf)) {
+ virReportOOMError();
+ virBufferFreeAndReset(&buf);
+ return NULL;
+ }
Do error checking.
+ ret = virBufferContentAndReset(&buf);
+ len = strlen(ret);
+ if (len > 0 && ret[len - 1] == ',')
+ ret[len - 1] = 0;
Strip a possibly trailing comma.
> + return ret;
> +}
> +
I couldn't find a way to match virBuffer APIs to do the exact operations as
Above. Is there a strcat substitute in virBuffer APIs?
Regards,
Sharadha
PS: Sorry, I missed your second patch for the configure script and
stuff. I just looked at it and the logic for the libcurl check needs
to be changed. I'll do a detailed review later. For the main patch, I
think we should apply it after the 0.7.7 release, once the remaining
major issues like the global variables are fixed, and fix remaining
minor issues in additional patches.
Matthias