2010/3/3 Sharadha Prabhakar (3P) <[email protected]>:
> 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 <[email protected]>
>> +*/
>> +
>
>
>> +
>> +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 <[email protected]>
>> + */
>> +
>
>> +
>> +/* 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
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
