2010/3/5 Sharadha Prabhakar (3P) <sharadha.prabhakar@xxxxxxxxxx>:
> 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@xxxxxxxxxx>
> +*/
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@xxxxxxxxxx>
> + */
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@xxxxxxxxxx>
> + */
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@xxxxxxxxxx>
> + */
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@xxxxxxxxxx>
> + */
> +
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list