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@xxxxxxxxxxxxxx] Sent: 03 March 2010 15:28 To: Sharadha Prabhakar (3P) Cc: libvir-list@xxxxxxxxxx; Ewan Mellor Subject: Re: status on review comments 2010/3/3 Sharadha Prabhakar (3P) <sharadha.prabhakar@xxxxxxxxxx>: > 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@xxxxxxxxxx> >> +*/ >> + > > >> + >> +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@xxxxxxxxxx> >> + */ >> + > >> + >> +/* 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list