[libvirt] status on review comments

[Sharadha Prabhakar (3P)]

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.

> +*
> +* 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. 

> 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];
> +    char *ret=NULL;
> +    int i, j;
> +    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.

> +           }
> +        }
> +    }
> +    mapstr[strlen(mapstr) - 1] = 0;
> +    snprintf(buf, sizeof(buf), "%d", vcpu);
> +    ret = strdup(mapstr);

>Use virAsprintf instead of snprintf and strdup.

> +    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