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

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

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

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

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

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