[libvirt] status on review comments

[Sharadha Prabhakar (3P)]

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 at googlemail.com] 
Sent: 03 March 2010 15:28
To: Sharadha Prabhakar (3P)
Cc: libvir-list at redhat.com; Ewan Mellor
Subject: Re: status on review comments

2010/3/3 Sharadha Prabhakar (3P) <sharadha.prabhakar at 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 at 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 at 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