[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/2] Addition of XenAPI support to libvirt



2010/3/5 Sharadha Prabhakar (3P) <sharadha prabhakar citrix com>:
> Patch includes
> 1) Modification of xenapiDomainGetOSType
> 1) global url and SSL flags removed and placed in private driver struct.
> 2) SSL verify on url parsed using function similar to the one in ESX.
> 3) mapDomainPinVcpu updated in xenapi_utils.c

> 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-03-04 16:59:01.000000000 +0000
> @@ -0,0 +1,1738 @@
> +
> +/*
> + * xenapi_driver.c: Xen API driver.
> + * Copyright (C) 2009 Citrix Ltd.
> + * Sharadha Prabhakar <sharadha prabhakar citrix com>
> +*/

A license statement is missing. Using LGPL like the rest of libvirt
would be a good choice.

> +
> +/*
> +*getCapsObject
> +*
> +*Build the capabilities of the hypervisor
> +*Return virCapsPtr on success or NULL on failure
> +*/
> +static virCapsPtr
> +getCapsObject (void)
> +{
> +    virCapsPtr caps = virCapabilitiesNew("x86_64", 0, 0);

You're ignoring the actual host architecture here and assume x86_64
even if the host is just x86.

> +    if (!caps) {
> +        virReportOOMError();
> +        return NULL;
> +    }
> +    virCapsGuestPtr guest1 = virCapabilitiesAddGuest(caps, "hvm", "x86_64", 0, "", "", 0, NULL);
> +    if (!guest1)
> +        goto error_cleanup;
> +    virCapsGuestDomainPtr domain1 = virCapabilitiesAddGuestDomain(guest1, "xen", "", "", 0, NULL);
> +    if (!domain1)
> +        goto error_cleanup;
> +    virCapsGuestPtr guest2 = virCapabilitiesAddGuest(caps, "xen", "x86_64", 0, "", "", 0, NULL);
> +    if (!guest2)
> +        goto error_cleanup;
> +    virCapsGuestDomainPtr domain2 = virCapabilitiesAddGuestDomain(guest2, "xen", "", "", 0, NULL);
> +    if (!domain2)
> +        goto error_cleanup;
> +
> +    return caps;
> +
> +  error_cleanup:
> +    virCapabilitiesFree(caps);
> +    return NULL;
> +}
> +
> +/*
> +*XenapiOpen
> +*
> +*Authenticates and creates a session with the server
> +*Return VIR_DRV_OPEN_SUCCESS on success, else VIR_DRV_OPEN_ERROR
> +*/
> +static virDrvOpenStatus
> +xenapiOpen (virConnectPtr conn, virConnectAuthPtr auth , int flags ATTRIBUTE_UNUSED)
> +{
> +    char *passwd;

Initialize passwd to NULL.

> +    xen_session *session;
> +    struct _xenapiPrivate *privP;
> +    int noVerify=0;
> +
> +    if (!STRCASEEQ(conn->uri->scheme,"XenAPI")) {
> +        xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED ,"Check URI format: 'XenAPI://user server'");

The given URI is not for you, this is no error, so don't report an
error, just return VIR_DRV_OPEN_DECLINED.

> +        return VIR_DRV_OPEN_DECLINED;
> +    }
> +    if (conn->uri->server==NULL) {
> +        xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED ,"Server name not in URI");
> +        return VIR_DRV_OPEN_ERROR;
> +    }
> +    if (auth) {
> +        passwd = xenapiUtil_RequestPassword(auth,conn->uri->user,conn->uri->server);
> +    } else {
> +        xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED ,"Authentication Credentials not found");
> +        return VIR_DRV_OPEN_ERROR;
> +    }
> +    if (!passwd && !conn->uri->user) {

You should use || instead of &&, otherwise you won't catch cases where
passwd is NULL but conn->uri->user isn't and via versa.

> +        xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED ,"Username/Password not valid");

You may leak passwd here. Free is using VIR_FREE(passwd).

> +        return VIR_DRV_OPEN_ERROR;
> +    }
> +    if (VIR_ALLOC(privP) < 0) {
> +        virReportOOMError();

You leak passwd here too.

> +        return VIR_DRV_OPEN_ERROR;
> +    }
> +    if (virAsprintf(&(privP->url),"https://%s",conn->uri->server) < 0) {
> +        virReportOOMError();

You may leak passwd here too.

> +        return VIR_DRV_OPEN_ERROR;
> +    }
> +    privP->SSLflag=2;
> +    xenapiUtil_ParseQuery(conn, conn->uri,&noVerify);
> +    if (noVerify==1) privP->SSLflag=0;
> +    xmlInitParser();
> +    xmlKeepBlanksDefault(0);
> +    xen_init();
> +    curl_global_init(CURL_GLOBAL_ALL);
> +
> +    session = xen_session_login_with_password (call_func, (void *)privP, conn->uri->user, "xenroot", xen_api_latest_version);

No need to cast privP to void *.

Shouldn't the parameter after the username be passwd instead of
hardcoding "xenroot"?

> +
> +    if ( session && session->ok ) {
> +        privP->session = session;
> +        virCapsPtr caps = getCapsObject();
> +        if (caps)
> +            privP->caps = caps;

Why not directly assign caps:

privP->caps = getCapsObject();

getCapsObject() may fail you need to check the return value for NULL,
and report an error.

> +        conn->privateData = privP;

You leak passwd here too.

> +        return VIR_DRV_OPEN_SUCCESS;
> +    } else {
> +        xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED ,"");
> +        VIR_FREE(privP);

You may also get here if session is not NULL but session->ok is false.
I assume session is allocated, so you need to free it here by some
free function, maybe xen_session_logout does this.

You leak passwd here too.

> +        return VIR_DRV_OPEN_ERROR;
> +    }
> +}
> +


> +
> +
> +/*
> +* xenapiGetVersion:
> +*
> +* Gets the version of XenAPI
> +*
> +*/
> +static int
> +xenapiGetVersion (virConnectPtr conn, unsigned long *hvVer)
> +{
> +    xen_host host;
> +    xen_session *session = ((struct _xenapiPrivate *)(conn->privateData))->session;
> +    xen_string_string_map *result=NULL;
> +    if (!(xen_session_get_this_host(session, &host,session))) {
> +        xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR ,NULL);
> +        return -1;
> +    }
> +    if (!(xen_host_get_software_version(session, &result, host))) {
> +        xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR ,NULL);
> +        xen_host_free(host);
> +        return -1;
> +    }
> +    xen_host_free(host);
> +    if (result && result->size>0) {
> +        int i;
> +        char *version=NULL;
> +        for (i=0; i<result->size; i++) {
> +            if (STREQ(result->contents[i].key,"xen")) {
> +                if (!(version = strdup(result->contents[i].val))) {
> +                    xen_string_string_map_free(result);
> +                    virReportOOMError();
> +                    return -1;
> +                }
> +                break;
> +            }
> +        }
> +        if (version) {
> +           unsigned long major=0,minor=0,release=0;
> +           if (sscanf(version,"%ld.%ld.%ld",&major,&minor,&release)!=3) {
> +               virReportOOMError();

If sscanf fails to parse 3 items, then this is no OOM error.

> +               xen_string_string_map_free(result);
> +               VIR_FREE(version);
> +               return -1;
> +           }
> +           *hvVer = major * 1000000 + minor * 1000 + release;
> +           VIR_FREE(version);
> +           xen_string_string_map_free(result);
> +           return 0;
> +        }
> +    }
> +    return -1;
> +}
> +
> +


> +
> +/*
> +* xenapiGetCapabilities:
> +*
> +*
> +* Returns capabilities as an XML string
> +*/
> +static char *
> +xenapiGetCapabilities (virConnectPtr conn)
> +{
> +    virCapsPtr caps = ((struct _xenapiPrivate *)(conn->privateData))->caps;
> +    if (caps) {
> +        char *xml = virCapabilitiesFormatXML(caps);
> +        return xml;

You need to report an OOM error if virCapabilitiesFormatXML returns NULL.

> +    }
> +    xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR ,"Capabilities not available");
> +    return NULL;
> +}
> +
> +
> +/*
> +* xenapiListDomains
> +*
> +* Collects the list of active domains, and store their ID in @maxids
> +* Returns the number of domain found or -1 in case of error
> +*/
> +static int
> +xenapiListDomains (virConnectPtr conn, int *ids, int maxids)
> +{
> +    /* vm.list */
> +    xen_host host;
> +    xen_vm_set *result=NULL;
> +    xen_session *session = ((struct _xenapiPrivate *)(conn->privateData))->session;
> +    if (xen_session_get_this_host(session, &host, session)) {
> +        xen_host_get_resident_vms(session, &result, host);
> +        xen_host_free(host);
> +    } else
> +        xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR ,NULL);
> +
> +    if (result != NULL) {
> +        int i;
> +        for ( i=0; (i < (result->size)) && (i<maxids) ; i++ ) {
> +            int64_t t0;
> +            xen_vm_get_domid(session, &t0, result->contents[i]);

Maybe you should check if the ID doesn't fit into an int and report an
error in that case, instead of just masking out the upper 32bit.

> +            ids[i] = (int)(t0 & 0xffffffff);
> +        }
> +       xen_vm_set_free(result);
> +        return i;
> +    }
> +    return -1;
> +}
> +


> +
> +/*
> +* xenapiDomainCreateXML
> +*
> +* Launches a new domain based on the XML description
> +* Returns the domain pointer or NULL in case of error
> +*/
> +static virDomainPtr
> +xenapiDomainCreateXML (virConnectPtr conn,
> +                       const char *xmlDesc, ATTRIBUTE_UNUSED unsigned int flags)

Normally ATTRIBUTE_UNUSED is placed behind the parameter, not in front of it.

> +{
> +    xen_vm_record *record=NULL;
> +    xen_vm vm=NULL;
> +    virDomainPtr domP=NULL;
> +    xen_session *session = ((struct _xenapiPrivate *)(conn->privateData))->session;
> +    virCapsPtr caps = ((struct _xenapiPrivate *)(conn->privateData))->caps;
> +    if (!caps)
> +        return NULL;
> +
> +    virDomainDefPtr defPtr = virDomainDefParseString(caps, xmlDesc, flags);
> +    createVMRecordFromXml(conn, defPtr, &record, &vm);
> +    virDomainDefFree(defPtr);
> +    if (record) {
> +        unsigned char raw_uuid[VIR_UUID_BUFLEN];
> +       virUUIDParse(record->uuid,raw_uuid);
> +       if (vm) {
> +            if (xen_vm_start(session, vm, false, false)) {
> +               domP = virGetDomain(conn, record->name_label, raw_uuid);
> +                if (!domP) {
> +                    xen_vm_record_free(record);
> +                    xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, "Domain Pointer is invalid");
> +                    return domP;
> +                }
> +                domP->id = record->domid;
> +               xen_vm_free(vm);
> +           }
> +            else
> +                xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, NULL);
> +       }
> +        xen_vm_record_free(record);
> +    }
> +    else
> +        xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, NULL);
> +    return domP;
> +}
> +


> +
> +/*
> +* xenapiDomainLookupByName
> +*
> +* Returns the domain pointer of domain with matching name
> +* or -1 in case of error
> +*/
> +static virDomainPtr
> +xenapiDomainLookupByName (virConnectPtr conn,
> +                          const char *name)
> +{
> +    /* vm.get_by_name_label */
> +    xen_vm_set *vms=NULL;
> +    xen_vm vm;
> +    char *uuid=NULL;
> +    unsigned char raw_uuid[VIR_UUID_BUFLEN];
> +    virDomainPtr domP=NULL;
> +    xen_session *session = ((struct _xenapiPrivate *)(conn->privateData))->session;
> +    if (xen_vm_get_by_name_label(session, &vms, (char *)name)) {
> +        if (vms->size!=1) {
> +            xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR,"Domain name is not unique");

You should distinguish between vms->size < 1 and vms->size > 1. If
there is no domain with a given name you'll report an error that says
that the domain name is not unique.

Or is vms->size >= 1 guaranteed if xen_vm_get_by_name_label succeeds?
If not then this applies to all other fucntions too, where you check
vms->size != 1.

> +            xen_vm_set_free(vms);
> +            return NULL;
> +        }
> +        vm = vms->contents[0];
> +       xen_vm_get_uuid(session, &uuid, vm);
> +        if (uuid!=NULL) {
> +           virUUIDParse(uuid,raw_uuid);
> +           domP = virGetDomain(conn, name, raw_uuid);
> +            if (domP != NULL) {
> +                int64_t domid=-1;
> +                xen_vm_get_domid(session, &domid, vm);
> +                domP->id = domid;
> +                xen_uuid_free(uuid);
> +                xen_vm_set_free(vms);
> +               return domP;
> +            } else {
> +                xen_uuid_free(uuid);
> +                xen_vm_set_free(vms);
> +               if (!(session->ok))
> +                   xenapiSessionErrorHandler(conn, VIR_ERR_NO_DOMAIN, NULL);
> +                return NULL;
> +            }
> +        }

You're leaking vms here.

> +    }
> +    xenapiSessionErrorHandler(conn, VIR_ERR_NO_DOMAIN, NULL);
> +    return NULL;
> +}
> +


> +/*
> +* xenapiDomainGetMaxMemory
> +*
> +* Returns maximum static memory for VM on success
> +* or 0 in case of error
> +*/
> +static unsigned long
> +xenapiDomainGetMaxMemory (virDomainPtr dom)
> +{
> +    int64_t mem_static_max=0;
> +    xen_vm vm;
> +    xen_vm_set *vms;
> +    xen_session *session = ((struct _xenapiPrivate *)(dom->conn->privateData))->session;
> +    if (xen_vm_get_by_name_label(session, &vms, dom->name)) {
> +        if (vms->size!=1) {
> +            xenapiSessionErrorHandler(dom->conn, VIR_ERR_INTERNAL_ERROR,"Domain name is not unique");
> +            xen_vm_set_free(vms);
> +            return 0;
> +        }
> +       vm = vms->contents[0];
> +        xen_vm_get_memory_static_max(session, &mem_static_max, vm);
> +       xen_vm_set_free(vms);
> +        return (unsigned long)(mem_static_max/1024);

Is mem_static_max a value in byte? libvirt expects this value in kilobyte.

> +    } else {
> +        xenapiSessionErrorHandler(dom->conn, VIR_ERR_NO_DOMAIN, NULL);
> +       return 0;
> +    }
> +}
> +
> +/*
> +* xenapiDomainSetMaxMemory
> +*
> +* Sets maximum static memory for VM on success
> +* Returns 0 on success or -1 in case of error
> +*/
> +static int
> +xenapiDomainSetMaxMemory (virDomainPtr dom, unsigned long memory)
> +{
> +    /* vm.set_memory_static_max */
> +    xen_vm vm;
> +    struct xen_vm_set *vms;
> +    xen_session *session = ((struct _xenapiPrivate *)(dom->conn->privateData))->session;
> +    if (xen_vm_get_by_name_label(session, &vms, dom->name)) {
> +        if (vms->size!=1) {
> +            xenapiSessionErrorHandler(dom->conn, VIR_ERR_INTERNAL_ERROR,"Domain name is not unique");
> +            xen_vm_set_free(vms);
> +            return -1;
> +        }
> +        vm = vms->contents[0];

If mem_static_max is in byte then you need to multiply memory by 1024,
because memory is in kilobyte.

> +        if (!(xen_vm_set_memory_static_max(session, vm, memory))) {
> +            xenapiSessionErrorHandler(dom->conn, VIR_ERR_INTERNAL_ERROR, NULL);
> +            xen_vm_set_free(vms);
> +            return -1;
> +        }
> +        xen_vm_set_free(vms);
> +    } else {
> +        xenapiSessionErrorHandler(dom->conn, VIR_ERR_NO_DOMAIN, NULL);
> +        return -1;
> +    }
> +    return 0;
> +}
> +



> +
> +/*
> +* xenapiDomainGetVcpus
> +*
> +* Gets Vcpu information
> +* Return number of structures filled on success or -1 in case of error
> +*/
> +static int
> +xenapiDomainGetVcpus (virDomainPtr dom,
> +                      virVcpuInfoPtr info, int maxinfo,
> +                      unsigned char *cpumaps, int maplen)
> +{
> +
> +    xen_vm_set *vms=NULL;
> +    xen_vm vm=NULL;
> +    xen_string_string_map *vcpu_params=NULL;
> +    int nvcpus=0,cpus=0,i;
> +    virDomainInfoPtr domInfo;
> +    virNodeInfo nodeInfo;
> +    virVcpuInfoPtr ifptr;
> +    xen_session *session = ((struct _xenapiPrivate *)(dom->conn->privateData))->session;
> +    char *mask=NULL;
> +    if((cpumaps!=NULL) && (maplen < 1))
> +        return -1;
> +    if (VIR_ALLOC(domInfo)<0) {

Don't allocate domInfo here, just use a virDomainInfo directly on the
stack, like you do it with virNodeInfo.

> +        virReportOOMError();
> +        return -1;
> +    }
> +    if (virDomainGetInfo(dom,domInfo)==0) {

You should not call libvirt public API functions from here. You can
call xenapiDomainGetInfo directly.

> +        nvcpus = domInfo->nrVirtCpu;
> +       VIR_FREE(domInfo);

If you put domInfo on the stack, remove this free call.

> +    } else {
> +        xenapiSessionErrorHandler(dom->conn, VIR_ERR_INTERNAL_ERROR, "Couldn't fetch Domain Information");
> +        return -1;
> +    }
> +    if ( virNodeGetInfo(dom->conn,&nodeInfo)==0)

Just call xenapiNodeGetInfo directly here.

> +        cpus = nodeInfo.cpus;
> +    else {
> +        xenapiSessionErrorHandler(dom->conn, VIR_ERR_INTERNAL_ERROR, "Couldn't fetch Node Information");
> +        return -1;
> +    }
> +    if(nvcpus > maxinfo) nvcpus = maxinfo;
> +
> +    if (cpumaps != NULL)
> +        memset(cpumaps, 0, maxinfo * maplen);
> +    if (!xen_vm_get_by_name_label(session, &vms, dom->name)) return -1;
> +    if (vms->size!=1) {
> +        xenapiSessionErrorHandler(dom->conn, VIR_ERR_INTERNAL_ERROR,"Domain name is not unique");
> +        xen_vm_set_free(vms);
> +        return -1;
> +    }
> +    vm = vms->contents[0];
> +    if (!xen_vm_get_vcpus_params(session, &vcpu_params, vm)) {
> +        xen_vm_set_free(vms);
> +        xenapiSessionErrorHandler(dom->conn, VIR_ERR_INTERNAL_ERROR, NULL);
> +        return -1;
> +    }
> +    for (i=0; i<vcpu_params->size; i++) {
> +        if (STREQ(vcpu_params->contents[i].key,"mask")) {
> +           if (!(mask = strdup(vcpu_params->contents[i].val))){
> +                 xen_vm_set_free(vms);
> +                 xen_string_string_map_free(vcpu_params);
> +                 virReportOOMError();
> +                 return -1;
> +            }
> +            break;
> +         }
> +    }
> +    xen_string_string_map_free(vcpu_params);
> +    for (i=0,ifptr=info ;i<nvcpus; i++,ifptr++) {
> +        ifptr->number = i;
> +       ifptr->state = VIR_VCPU_RUNNING;
> +       ifptr->cpuTime = 0;
> +       ifptr->cpu = 0;
> +       if (mask !=NULL)
> +           getCpuBitMapfromString(mask,VIR_GET_CPUMAP(cpumaps,maplen,i),maplen);
> +    }
> +    VIR_FREE(mask);
> +    xen_vm_set_free(vms);
> +    return i;
> +}
> +


> +
> +/*
> +* xenapiDomainDumpXML
> +*
> +*
> +* Returns XML string of the domain configuration on success or -1 in case of error
> +*/
> +static char *
> +xenapiDomainDumpXML (virDomainPtr dom, ATTRIBUTE_UNUSED int flags)

Put ATTRIBUTE_UNUSED behind the parameter.

> +{
> +    xen_vm vm=NULL;
> +    xen_vm_set *vms;
> +    xen_string_string_map *result=NULL;
> +    xen_session *session = ((struct _xenapiPrivate *)(dom->conn->privateData))->session;
> +    virDomainDefPtr defPtr = NULL;
> +
> +    if(!xen_vm_get_by_name_label(session, &vms, dom->name)) return NULL;
> +    if (vms->size!=1) {
> +        xenapiSessionErrorHandler(dom->conn, VIR_ERR_INTERNAL_ERROR,"Domain name is not unique");
> +        xen_vm_set_free(vms);
> +        return NULL;
> +    }
> +    if (VIR_ALLOC(defPtr)<0) {
> +        virReportOOMError();
> +        xen_vm_set_free(vms);
> +        return NULL;
> +    }
> +    vm = vms->contents[0];
> +    defPtr->virtType = VIR_DOMAIN_VIRT_XEN;
> +    defPtr->id = dom->id;
> +    memcpy((char *)defPtr->uuid,(char *)dom->uuid,VIR_UUID_BUFLEN);
> +    if (!(defPtr->name = strdup(dom->name)))
> +        goto error_cleanup;
> +    char *boot_policy=NULL;
> +    xen_vm_get_hvm_boot_policy(session, &boot_policy, vm);
> +    if (STREQ(boot_policy,"BIOS order")) {
> +        if (!(defPtr->os.type = strdup("hvm"))) {
> +            VIR_FREE(boot_policy);
> +            goto error_cleanup;
> +        }
> +        xen_vm_get_hvm_boot_params(session, &result, vm);
> +        if (result!=NULL) {
> +            int i;
> +            for (i=0; i<(result->size); i++) {
> +                if (STREQ(result->contents[i].key,"order")) {
> +                   int cnt=0;
> +                   while(result->contents[i].val[cnt]!='\0') {
> +                        defPtr->os.bootDevs[cnt] = map2LibvirtBootOrder(result->contents[i].val[cnt]);
> +                       cnt++;
> +                   }
> +                    defPtr->os.nBootDevs = cnt;

I think you can break the for loop here.

> +               }
> +           }
> +           xen_string_string_map_free(result);
> +        }
> +        VIR_FREE(boot_policy);
> +    } else {
> +        if(!(defPtr->os.type = strdup("xen"))) {
> +            VIR_FREE(boot_policy);
> +            goto error_cleanup;
> +        }
> +        if(!(defPtr->os.loader = strdup("pygrub"))) {
> +            VIR_FREE(boot_policy);
> +            goto error_cleanup;
> +        }
> +        char *value=NULL;
> +        xen_vm_get_pv_kernel(session, &value, vm);
> +        if (STRNEQ(value,"")) {
> +            if(!(defPtr->os.kernel = strdup(value))) {
> +                VIR_FREE(boot_policy);
> +                VIR_FREE(value);
> +                goto error_cleanup;
> +            }
> +           VIR_FREE(value);
> +        }
> +        xen_vm_get_pv_ramdisk(session, &value, vm);
> +        if (STRNEQ(value,"")) {
> +            if(!(defPtr->os.initrd = strdup(value))) {
> +                VIR_FREE(boot_policy);
> +                VIR_FREE(value);
> +                goto error_cleanup;
> +            }
> +           VIR_FREE(value);
> +        }
> +        xen_vm_get_pv_args(session, &value, vm);
> +        if (STRNEQ(value,"")) {
> +            if(!(defPtr->os.cmdline = strdup(value))) {
> +                VIR_FREE(boot_policy);
> +                VIR_FREE(value);
> +                goto error_cleanup;
> +            }
> +           VIR_FREE(value);
> +        }
> +       VIR_FREE(boot_policy);
> +        if(!(defPtr->os.bootloader = strdup("pygrub")))
> +            goto error_cleanup;
> +    }
> +    char *val=NULL;
> +    xen_vm_get_pv_bootloader_args(session, &val, vm);
> +    if (STRNEQ(val,"")) {
> +        if(!(defPtr->os.bootloaderArgs = strdup(val))) {
> +            VIR_FREE(val);
> +            goto error_cleanup;
> +        }
> +       VIR_FREE(val);
> +    }
> +    unsigned long memory=0;
> +    memory = xenapiDomainGetMaxMemory(dom);
> +    defPtr->maxmem = memory;
> +    int64_t dynamic_mem=0;
> +    if (xen_vm_get_memory_dynamic_max(session, &dynamic_mem, vm)) {
> +        defPtr->memory = (unsigned long) (dynamic_mem/1024);
> +    } else {
> +        defPtr->memory = memory;
> +    }
> +    defPtr->vcpus = xenapiDomainGetMaxVcpus(dom);
> +    enum xen_on_normal_exit action;
> +    if (xen_vm_get_actions_after_shutdown(session, &action, vm)) {
> +        defPtr->onPoweroff = xenapiNormalExitEnum2virDomainLifecycle(action);
> +    }
> +    if (xen_vm_get_actions_after_reboot(session, &action, vm)) {
> +        defPtr->onReboot = xenapiNormalExitEnum2virDomainLifecycle(action);
> +    }
> +    enum xen_on_crash_behaviour crash;
> +    if (xen_vm_get_actions_after_crash(session, &crash, vm)) {
> +        defPtr->onCrash = xenapiCrashExitEnum2virDomainLifecycle(action);
> +    }
> +    xen_vm_get_platform(session, &result, vm);
> +    if (result!=NULL) {
> +        int i;
> +       for(i=0; i< (result->size); i++) {
> +           if (STREQ(result->contents[i].val,"true")) {
> +                if (STREQ(result->contents[i].key,"acpi"))
> +                    defPtr->features = defPtr->features | (1<<VIR_DOMAIN_FEATURE_ACPI);
> +                else if (STREQ(result->contents[i].key,"apic"))
> +                    defPtr->features = defPtr->features | (1<<VIR_DOMAIN_FEATURE_APIC);
> +                else if (STREQ(result->contents[i].key,"pae"))
> +                    defPtr->features = defPtr->features | (1<<VIR_DOMAIN_FEATURE_PAE);
> +           }
> +       }
> +        xen_string_string_map_free(result);
> +    }
> +    struct xen_vif_set *vif_set=NULL;
> +    xen_vm_get_vifs(session, &vif_set, vm);
> +    if (vif_set) {
> +        int i;
> +       xen_vif vif;
> +       xen_vif_record *vif_rec=NULL;
> +       xen_network network;
> +       char *bridge=NULL;
> +        defPtr->nnets = vif_set->size;
> +        if (VIR_ALLOC_N(defPtr->nets, vif_set->size)<0) {
> +            xen_vif_set_free(vif_set);
> +            goto error_cleanup;
> +        }
> +       for (i=0; i<vif_set->size; i++) {
> +            if (VIR_ALLOC(defPtr->nets[i])<0) {
> +                xen_vif_set_free(vif_set);
> +                goto error_cleanup;
> +            }
> +            defPtr->nets[i]->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
> +           vif = vif_set->contents[i];
> +            xen_vif_get_network(session, &network, vif);
> +           if (network!=NULL) {
> +                xen_network_get_bridge(session, &bridge, network);
> +                if (bridge!=NULL)
> +                    defPtr->nets[i]->data.bridge.brname = bridge;
> +               xen_network_free(network);
> +           }
> +           xen_vif_get_record(session, &vif_rec, vif);
> +            if (vif_rec!=NULL) {
> +                if(virParseMacAddr((const char *)vif_rec->mac,defPtr->nets[i]->mac) < 0)
> +                    xenapiSessionErrorHandler(dom->conn, VIR_ERR_INTERNAL_ERROR, "Unable to parse given mac address");
> +               xen_vif_record_free(vif_rec);
> +           }
> +        }
> +        xen_vif_set_free(vif_set);
> +    }
> +    if (vms) xen_vm_set_free(vms);
> +    char *xml = virDomainDefFormat(defPtr,0);
> +    virDomainDefFree(defPtr);
> +    return xml;
> +
> +  error_cleanup:
> +        virReportOOMError();
> +        xen_vm_set_free(vms);
> +        virDomainDefFree(defPtr);
> +        return NULL;
> +
> +}
> +
> +/*
> +* xenapiListDefinedDomains
> +*
> +* list the defined but inactive domains, stores the pointers to the names in @names
> +* Returns number of names provided in the array or -1 in case of error
> +*/
> +static int
> +xenapiListDefinedDomains (virConnectPtr conn, char **const names,
> +                          int maxnames)
> +{
> +    int i,j=0,doms;
> +    xen_vm_set *result;
> +    xen_vm_record *record;
> +    xen_session *session = ((struct _xenapiPrivate *)(conn->privateData))->session;
> +    xen_vm_get_all(session, &result);
> +    if (result != NULL) {
> +        for (i=0; i< (result->size) && j< maxnames; i++) {
> +           xen_vm_get_record(session, &record, result->contents[i]);
> +           if ( record!=NULL ) {
> +                if ( record->is_a_template == 0 ) {
> +                    char *usenames;
> +                   if (!(usenames = strdup(record->name_label))) {
> +                        virReportOOMError();
> +                        xen_vm_record_free(record);
> +                        xen_vm_set_free(result);

If you've already srtdup'ed some names when an error occurs, then
you're leaking this already strdup'ed names here.

> +                        return -1;
> +                    }
> +                   names[j++]=usenames;
> +               }
> +                xen_vm_record_free(record);
> +           } else {
> +                xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, "Couldn't get VM record");
> +                xen_vm_set_free(result);

And here you may leak already strdup'ed names too. you need to free
them in bot cases.

> +               return -1;
> +            }
> +       }
> +        doms=j;
> +        xen_vm_set_free(result);
> +        return doms;
> +    }
> +    xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, NULL);
> +    return -1;
> +}
> +


> +
> +/*
> +* call_func
> +* sets curl options, used with xen_session_login_with_password
> +*/
> +int
> +call_func(const void *data, size_t len, void *user_handle,
> +          void *result_handle, xen_result_func result_func)
> +{
> +    //(void)user_handle;
> +    struct _xenapiPrivate *priv = (struct _xenapiPrivate *)user_handle;
> +    #ifdef PRINT_XML
> +
> +        printf("\n\n---Data to server: -----------------------\n");
> +        printf("%s\n",((char*) data));
> +        fflush(stdout);
> +    #endif
> +    CURL *curl = curl_easy_init();
> +    if (!curl) {
> +      return -1;
> +    }
> +    xen_comms comms = {
> +     .func = result_func,
> +     .handle = result_handle
> +    };
> +    curl_easy_setopt(curl, CURLOPT_URL, priv->url);
> +    curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 1);
> +    #ifdef CURLOPT_MUTE
> +        curl_easy_setopt(curl, CURLOPT_MUTE, 1);
> +    #endif
> +    curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, &write_func);
> +    curl_easy_setopt(curl, CURLOPT_WRITEDATA, &comms);
> +    curl_easy_setopt(curl, CURLOPT_POST, 1);
> +    curl_easy_setopt(curl, CURLOPT_POSTFIELDS, data);
> +    curl_easy_setopt(curl, CURLOPT_POSTFIELDSIZE, len);
> +    curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, priv->SSLflag);
> +    curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, priv->SSLflag);

You set SSLflag to 0 or 2 depending on the no_verify query parameter
in xenapiOpen. For CURLOPT_SSL_VERIFYHOST 2 is the correct value, but
CURLOPT_SSL_VERIFYPEER expects 0 or 1.

Instead of having the SSLflag in the xenapiPrivate struct you could
have the noVerify value there and then decide here to what values this
maps. For example

    curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, priv->noVerify ? 0 : 1);
    curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, priv->noVerify ? 0 : 2);

> +    CURLcode result = curl_easy_perform(curl);
> +    curl_easy_cleanup(curl);
> +    return result;
> +
> +}
> +
> +
> +
> diff -Nur ./libvirt_org/src/xenapi/xenapi_driver.h ./libvirt/src/xenapi/xenapi_driver.h
> --- ./libvirt_org/src/xenapi/xenapi_driver.h    1970-01-01 01:00:00.000000000 +0100
> +++ ./libvirt/src/xenapi/xenapi_driver.h        2010-02-26 13:21:50.000000000 +0000
> @@ -0,0 +1,15 @@
> +/*
> + * xenapi_driver.h.c: Xen API driver header file to be included in libvirt.c.
> + * Copyright (C) 2009 Citrix Ltd.
> + * Sharadha Prabhakar <sharadha prabhakar citrix com>
> + */

As in xenapi_driver.c, a license statement is missing here.

> +
> +#ifndef __VIR_XENAPI_PRIV_H__
> +#define __VIR_XENAPI_PRIV_H__
> +
> +
> +extern int xenapiRegister (void);
> +
> +
> +#endif /* __VIR_XENAPI_PRIV_H__ */
> diff -Nur ./libvirt_org/src/xenapi/xenapi_driver_private.h ./libvirt/src/xenapi/xenapi_driver_private.h
> --- ./libvirt_org/src/xenapi/xenapi_driver_private.h    1970-01-01 01:00:00.000000000 +0100
> +++ ./libvirt/src/xenapi/xenapi_driver_private.h        2010-03-03 16:09:47.000000000 +0000
> @@ -0,0 +1,49 @@
> +/*
> + * xenapi_driver_private.h: Xen API driver's private header file.
> + * Copyright (C) 2009 Citrix Ltd.
> + * Sharadha Prabhakar <sharadha prabhakar citrix com>
> + */

A license statement is missing here too.

> +
> +#ifndef __VIR_XENAPI_H__
> +#define __VIR_XENAPI_H__
> +
> +#include <xen/api/xen_common.h>
> +#include <libxml/tree.h>
> +#include "virterror_internal.h"
> +
> +//#define PRINT_XML
> +#define VIR_FROM_THIS VIR_FROM_XENAPI
> +#define LIBVIRT_MODELNAME_LEN  (32)
> +#define xenapiSessionErrorHandler(conn,errNum,buf) xenapiSessionErrorHandle(conn, errNum, \
> +                                                         buf,__FILE__,__FUNCTION__,__LINE__)
> +
> +void
> +xenapiSessionErrorHandle(virConnectPtr conn, virErrorNumber errNum,
> +                         const char *buf, const char *filename, const char *func, size_t lineno);
> +
> +typedef struct
> +{
> +    xen_result_func func;
> +    void *handle;
> +} xen_comms;
> +
> +
> +int
> +call_func(const void *data, size_t len, void *user_handle,
> +          void *result_handle, xen_result_func result_func);
> +size_t
> +write_func(void *ptr, size_t size, size_t nmemb, void *comms);
> +
> +/* xenAPI driver's private data structure */
> +struct _xenapiPrivate {
> +    xen_session *session;
> +    void *handle;
> +    char *uname;
> +    char *pwd;

I think handle, uname, and pwd are actually unused. If that's true you
could remove them from the struct.

> +    char *url;
> +    int SSLflag;
> +    virCapsPtr caps;
> +};
> +
> +#endif /* __VIR_XENAPI_H__ */
> 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-03-03 18:01:06.000000000 +0000
> @@ -0,0 +1,578 @@
> +/*
> + * xenapi_utils.c: Xen API driver -- utils parts.
> + * Copyright (C) 2009 Citrix Ltd.
> + * Sharadha Prabhakar <sharadha prabhakar citrix com>
> + */

A license statement is missing here too.

> +
> +/* obtains the CPU bitmap from the string passed */
> +void
> +getCpuBitMapfromString(char *mask, unsigned char *cpumap, int maplen)
> +{
> +    int pos;
> +    int max_bits = maplen * 8;
> +    char *num = NULL,*bp=NULL;
> +    bzero(cpumap, maplen);
> +    num = strtok_r (mask, ",", &bp);
> +    while (num != NULL) {
> +        if (sscanf (num, "%d", &pos)!=1)
> +            virReportOOMError();

It isn't an OOM error if sscanf fails.

> +        if (pos<0 || pos>max_bits-1)
> +            VIR_WARN ("number in str %d exceeds cpumap's max bits %d\n", pos, max_bits);

Do not append \n, that'll be done automatically.

> +        else
> +            (cpumap)[pos/8] |= (1<<(pos%8));
> +            num = strtok_r (NULL, ",", &bp);
> +    }
> +}
> +
> +


> +
> +/* allocate a flexible array and fill values(key,val) */
> +int
> +allocStringMap (xen_string_string_map **strings, char *key, char *val)
> +{
> +    int sz = ((*strings) == NULL)?0:(*strings)->size;
> +    sz++;
> +    if(VIR_REALLOC_N(*strings, sizeof(xen_string_string_map)+
> +                            sizeof(xen_string_string_map_contents)*sz)<0) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +    (*strings)->size = sz;
> +    (*strings)->contents[sz-1].key = strdup(key);
> +    (*strings)->contents[sz-1].val = strdup(val);

You should check if strdup returned NULL.

> +    return 0;
> +}
> +
> +/* Error handling function returns error messages from the server if any */
> +void
> +xenapiSessionErrorHandle(virConnectPtr conn, virErrorNumber errNum,
> +                         const char *buf, const char *filename, const char *func, size_t lineno)
> +{
> +    if (buf==NULL) {
> +        char *ret=NULL;
> +        ret = returnErrorFromSession(((struct _xenapiPrivate *)(conn->privateData))->session);
> +        virReportErrorHelper (conn, VIR_FROM_XENAPI, errNum, filename, func, lineno, _("%s\n"), ret);
> +        xen_session_clear_error(((struct _xenapiPrivate *)(conn->privateData))->session);
> +        VIR_FREE(ret);
> +    } else {
> +        virReportErrorHelper (conn, VIR_FROM_XENAPI, errNum, filename, func, lineno, _("%s\n"), buf);
> +    }

No need to append \n here too.

> +}
> +


> +
> +/* Create a VM record from the XML description */
> +int
> +createVMRecordFromXml (virConnectPtr conn, virDomainDefPtr def,
> +                       xen_vm_record **record, xen_vm *vm)
> +{
> +    char uuidStr[VIR_UUID_STRING_BUFLEN];
> +    *record = xen_vm_record_alloc();
> +    if (!((*record)->name_label = strdup(def->name)))
> +        goto error_cleanup;
> +    if (def->uuid) {
> +        virUUIDFormat(def->uuid,uuidStr);
> +        if (!((*record)->uuid = strdup(uuidStr)))
> +            goto error_cleanup;
> +    }
> +    if (STREQ(def->os.type,"hvm")) {
> +        if(!((*record)->hvm_boot_policy = strdup("BIOS order")))
> +            goto error_cleanup;
> +         char *boot_order = NULL;
> +         if (def->os.nBootDevs!=0)
> +             boot_order = createXenAPIBootOrderString(def->os.nBootDevs, &def->os.bootDevs[0]);
> +         if (boot_order!=NULL) {
> +            xen_string_string_map *hvm_boot_params=NULL;
> +            allocStringMap(&hvm_boot_params, (char *)"order",boot_order);
> +             (*record)->hvm_boot_params = hvm_boot_params;
> +            VIR_FREE(boot_order);
> +         }
> +    } else if (STREQ(def->os.type,"xen")) {
> +        if (!((*record)->pv_bootloader = strdup("pygrub")))
> +            goto error_cleanup;
> +        if (def->os.kernel){
> +            if (!((*record)->pv_kernel = strdup(def->os.kernel)))
> +                goto error_cleanup;
> +        }
> +        if (def->os.initrd) {
> +           if (!((*record)->pv_ramdisk = strdup(def->os.initrd)))
> +                goto error_cleanup;
> +        }
> +        if(def->os.cmdline) {
> +            if (!((*record)->pv_args = strdup(def->os.cmdline)))
> +                goto error_cleanup;
> +        }
> +       (*record)->hvm_boot_params = xen_string_string_map_alloc(0);
> +    }
> +    if (def->os.bootloaderArgs)
> +        if(!((*record)->pv_bootloader_args = strdup(def->os.bootloaderArgs)))
> +            goto error_cleanup;
> +
> +    if (def->memory)
> +        (*record)->memory_static_max = (int64_t) (def->memory * 1024);
> +    if (def->maxmem)
> +        (*record)->memory_dynamic_max = (int64_t) (def->maxmem * 1024);
> +    else
> +        (*record)->memory_dynamic_max = (*record)->memory_static_max;
> +
> +    if (def->vcpus) {
> +        (*record)->vcpus_max = (int64_t) def->vcpus;
> +       (*record)->vcpus_at_startup = (int64_t) def->vcpus;
> +    }
> +    if (def->onPoweroff)
> +        (*record)->actions_after_shutdown = actionShutdownLibvirt2XenapiEnum(def->onPoweroff);
> +    if (def->onReboot)
> +        (*record)->actions_after_reboot = actionShutdownLibvirt2XenapiEnum(def->onReboot);
> +    if (def->onCrash)
> +        (*record)->actions_after_crash = actionCrashLibvirt2XenapiEnum(def->onCrash);
> +
> +    xen_string_string_map *strings=NULL;
> +    if (def->features) {
> +        if (def->features & (1<<VIR_DOMAIN_FEATURE_ACPI))
> +            allocStringMap(&strings,(char *)"acpi",(char *)"true");
> +        if (def->features & (1<<VIR_DOMAIN_FEATURE_APIC))
> +            allocStringMap(&strings,(char *)"apic",(char *)"true");
> +        if (def->features & (1<<VIR_DOMAIN_FEATURE_PAE))
> +            allocStringMap(&strings,(char *)"pae",(char *)"true");
> +    }
> +    if (strings!=NULL)
> +        (*record)->platform = strings;
> +
> +    (*record)->vcpus_params = xen_string_string_map_alloc(0);
> +    (*record)->other_config = xen_string_string_map_alloc(0);
> +    (*record)->last_boot_cpu_flags = xen_string_string_map_alloc(0);
> +    (*record)->xenstore_data = xen_string_string_map_alloc(0);
> +    (*record)->hvm_shadow_multiplier = 1.000;
> +    if (!xen_vm_create(((struct _xenapiPrivate *)(conn->privateData))->session,
> +                        vm, *record)) {
> +        xenapiSessionErrorHandler(conn,VIR_ERR_INTERNAL_ERROR,NULL);
> +        return -1;
> +    }
> +
> +    int device_number=0;
> +    char *bridge=NULL,*mac=NULL;
> +    int i;
> +    for (i=0;i<def->nnets;i++) {
> +        if (def->nets[i]->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> +            if (def->nets[i]->data.bridge.brname)
> +                if(!(bridge = strdup(def->nets[i]->data.bridge.brname)))
> +                    goto error_cleanup;
> +            if (def->nets[i]->mac) {
> +                char macStr[VIR_MAC_STRING_BUFLEN];
> +                virFormatMacAddr(def->nets[i]->mac, macStr);
> +                    if(!(mac = strdup(macStr))) {
> +                        if (bridge) VIR_FREE(bridge);
> +                        goto error_cleanup;
> +                    }
> +            }
> +            if (mac!=NULL && bridge!=NULL) {
> +                char device[NETWORK_DEVID_SIZE]="\0";

You defined NETWORK_DEVID_SIZE to be 10, but 10 is to small for an int
value. You need at least two more chars, one for a possible minus sign
and one for the null terminator.

> +                sprintf(device,"%d",device_number);
> +                createVifNetwork(conn, *vm, device, bridge, mac);
> +                VIR_FREE(bridge);
> +                device_number++;
> +            }
> +            if (bridge) VIR_FREE(bridge);
> +           /*if (mac!=NULL && bridge!=NULL) {
> +               char device[NETWORK_DEVID_SIZE]="\0";
> +               sprintf(device,"%d",device_number);
> +               if (createVifNetwork(conn, *vm, device, bridge, mac)<0) {
> +                    VIR_FREE(mac);
> +                    VIR_FREE(bridge);
> +                    xen_vm_record_free(*record);
> +                    xenapiSessionErrorHandler(conn,VIR_ERR_INTERNAL_ERROR,NULL);
> +                    return -1;
> +                }
> +               VIR_FREE(bridge);
> +               device_number++;
> +           } else {
> +                if (bridge)
> +                    VIR_FREE(bridge);
> +                if (mac)
> +                    VIR_FREE(mac);
> +                xen_vm_record_free(*record);
> +                xenapiSessionErrorHandler(conn,VIR_ERR_INTERNAL_ERROR,NULL);
> +                return -1;
> +            }*/
> +        }
> +    }
> +    return 0;
> +
> +  error_cleanup:
> +    virReportOOMError();
> +    xen_vm_record_free(*record);
> +    return -1;
> +}
> +
> diff -Nur ./libvirt_org/src/xenapi/xenapi_utils.h ./libvirt/src/xenapi/xenapi_utils.h
> --- ./libvirt_org/src/xenapi/xenapi_utils.h     1970-01-01 01:00:00.000000000 +0100
> +++ ./libvirt/src/xenapi/xenapi_utils.h 2010-03-03 18:01:19.000000000 +0000
> @@ -0,0 +1,93 @@
> +/*
> + * xenapi_utils.h: Xen API driver -- utils header
> + * Copyright (C) 2009 Citrix Ltd.
> + * Sharadha Prabhakar <sharadha prabhakar citrix com>
> + */
> +

A license statement is missing here too.


> +
> +#define NETWORK_DEVID_SIZE  (10)
> +

Should be at least 12.

Another thing you probably should fix is the definition of varaibles
in the middle of blocks. Variables should be defined at the beginning
of a block.

Okay, mostly minor issues. I think the next version of this patch can
be applied and remaining issues can be fixed by additional patches.

Matthias


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]