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

["Sharadha Prabhakar (3P)" <sharadha.prabhakar@xxxxxxxxxx>]

Patch 1 is sent as attachment. It contains changes suggested in this
mail. Some comments inline.
Sharadha

-----Original Message-----
From: Matthias Bolte [mailto:matthias.bolte@xxxxxxxxxxxxxx] 
Sent: 05 March 2010 23:32
To: Sharadha Prabhakar (3P)
Cc: libvir-list@xxxxxxxxxx; Ewan Mellor
Subject: Re: [libvirt] [PATCH 1/2] Addition of XenAPI support to libvirt

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


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

X86_64 is the only architecture supported as of now.

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

Yes. If vms->size<=0 then no domain error is returned and if vms->size!=1 
Domain name is not unique error is returned.

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

Yes

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

This has been changed to return 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.

Matthias

Attachment: xenapiPatch1
Description: xenapiPatch1

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list