Re: [PATCH] libxenlight driver

[Jim Fehlig <jfehlig@novell.com>]

Stefano Stabellini wrote:
> On Fri, 18 Feb 2011, Jim Fehlig wrote:
>   
>> +static int
>> +libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config *d_config)
>> +{
>> +    virDomainDiskDefPtr *l_disks = def->disks;
>> +    int ndisks = def->ndisks;
>> +    libxl_device_disk *x_disks;
>> +    int i;
>> +
>> +    if (VIR_ALLOC_N(x_disks, ndisks) < 0) {
>> +        virReportOOMError();
>> +        return -1;
>> +    }
>> +
>> +    for (i = 0; i < ndisks; i++) {
>> +        if (l_disks[i]->src &&
>> +             (x_disks[i].physpath = strdup(l_disks[i]->src)) == NULL) {
>> +            virReportOOMError();
>> +            goto error;
>> +        }
>> +
>> +        if (l_disks[i]->dst &&
>> +            (x_disks[i].virtpath = strdup(l_disks[i]->dst)) == NULL) {
>> +            virReportOOMError();
>> +            goto error;
>> +        }
>> +
>> +        if (l_disks[i]->driverName) {
>> +            if (STREQ(l_disks[i]->driverName, "tap") ||
>> +                STREQ(l_disks[i]->driverName, "tap2")) {
>> +                if (l_disks[i]->driverType &&
>> +                    STREQ(l_disks[i]->driverType, "qcow2"))
>> +                    x_disks[i].phystype = PHYSTYPE_QCOW2;
>> +                else if (l_disks[i]->driverType &&
>> +                         STREQ(l_disks[i]->driverType, "aio"))
>> +                    x_disks[i].phystype = PHYSTYPE_AIO;
>> +                else if (l_disks[i]->driverType &&
>> +                         STREQ(l_disks[i]->driverType, "vhd"))
>> +                    x_disks[i].phystype = PHYSTYPE_VHD;
>> +            } else if (STREQ(l_disks[i]->driverName, "file")) {
>> +                x_disks[i].phystype = PHYSTYPE_FILE;
>> +            } else if (STREQ(l_disks[i]->driverName, "phy")) {
>> +                x_disks[i].phystype = PHYSTYPE_PHY;
>> +            }
>> +        } else {
>> +            /* Default to file?? */
>> +            x_disks[i].phystype = PHYSTYPE_FILE;
>> +        }
>>     
>
> few days ago the patch that removes phystype and introduces backend and
> format has been applied, so this code needs an update
>   

Thanks for the review and comments.  I've posted a new version based on
xen-unstable c/s 22940.

>> +static void libxlEventHandler(int watch,
>> +                              int fd,
>> +                              int events,
>> +                              void *data)
>> +{
>> +    libxlDriverPrivatePtr driver = libxl_driver;
>> +    virDomainObjPtr vm = data;
>> +    libxlDomainObjPrivatePtr priv;
>> +    libxl_event event;
>> +    libxl_dominfo info;
>> +
>> +    libxlDriverLock(driver);
>> +    virDomainObjLock(vm);
>> +    libxlDriverUnlock(driver);
>> +
>> +    priv = vm->privateData;
>> +
>> +    memset(&event, 0, sizeof(event));
>> +    memset(&info, 0, sizeof(info));
>> +
>> +    if (priv->waiterFD != fd || priv->eventHdl != watch) {
>> +        virEventRemoveHandle(watch);
>> +        goto cleanup;
>> +    }
>> +
>> +    if (!(events & VIR_EVENT_HANDLE_READABLE)) {
>> +        goto cleanup;
>> +    }
>> +
>> +    if (libxl_get_event(&priv->ctx, &event)) {
>> +        goto cleanup;
>> +    }
>> +
>> +    if (event.type == LIBXL_EVENT_DOMAIN_DEATH) {
>> +        /* libxl_event_get_domain_death_info returns 1 if death
>> +         * event was for this domid */
>> +        if (libxl_event_get_domain_death_info(&priv->ctx,
>> +                                              vm->def->id,
>> +                                              &event,
>> +                                              &info) != 1) {
>> +            goto cleanup;
>> +        }
>> +
>> +        virEventRemoveHandle(watch);
>> +        if (info.shutdown_reason == SHUTDOWN_poweroff) {
>> +            libxl_domain_destroy(&priv->ctx, vm->def->id, 0);
>> +            if (vm->persistent) {
>> +                vm->def->id = -1;
>> +                vm->state = VIR_DOMAIN_SHUTOFF;
>> +            } else {
>> +                libxl_free_waiter(priv->dWaiter);
>> +                VIR_FREE(priv->dWaiter);
>> +                virDomainRemoveInactive(&driver->domains, vm);
>> +                vm = NULL;
>> +            }
>> +        } else if (info.shutdown_reason == SHUTDOWN_reboot) {
>> +            libxl_domain_destroy(&priv->ctx, vm->def->id, 0);
>> +            vm->def->id = -1;
>> +            vm->state = VIR_DOMAIN_SHUTOFF;
>> +            sleep(1);
>> +            libxlVmStart(vm, 0);
>>     
>
> we need to handle at least SHUTDOWN_suspend and SHUTDOWN_crash too
>   

The updated patch handles SHUTDOWN_crash similar to SHUTDOWN_poweroff. 
I've ignored SHUTDOWN_suspend since the driver does not yet implement
domainSave function.  Also notice I've ignored any user-specified
actions on these events, e.g. coredump and restart.  But IMO, these type
of improvements can be added incrementally.  I'd like to get a basic
driver upstream so others can easily contribute.

>   
>> +        } else {
>> +            VIR_INFO("Unhandled shutdown_reason %d", info.shutdown_reason);
>> +        }
>> +    }
>>     
>
> we should call libxl_free_event before returning, otherwise we leak two
> strings
>   

Fixed in updated patch.

[...]

>> +
>> +    libxl_driver->caps->privateDataAllocFunc = libxlDomainObjPrivateAlloc;
>> +    libxl_driver->caps->privateDataFreeFunc = libxlDomainObjPrivateFree;
>>     
>
> If I understand correctly privateDataAllocFunc is called at each domain creation
>   

It is called when a domain is introduced to libvirt, either by defining
a persistent domain (e.g. virsh define dom.xml) or creating a transient
domain (e.g. virsh create dom.xml).  The free func is called when a
domain is removed from libvirt, either by undefining a persistent domain
(e.g. virsh undefine dom-name) or poweroff of a transient domain. 
Starting/stopping a persistent domain does not invoke these functions.

Regards,
Jim