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

Re: [libvirt] [Patch v3] Add VMware Workstation and Player driver



On Wed, Dec 08, 2010 at 06:55:08PM +0100, Jean-Baptiste Rouault wrote:
> +virCapsPtr
> +vmwareCapsInit(void)
> +{
> +    struct utsname utsname;
> +    virCapsPtr caps = NULL;
> +    virCapsGuestPtr guest = NULL;
> +    virCPUDefPtr cpu = NULL;
> +    union cpuData *data = NULL;
> +
> +    uname(&utsname);
> +
> +    if ((caps = virCapabilitiesNew(utsname.machine, 0, 0)) == NULL)
> +        goto error;
> +
> +    if (nodeCapsInitNUMA(caps) < 0)
> +        goto error;
> +
> +    virCapabilitiesSetMacPrefix(caps, (unsigned char[]) {0x52, 0x54, 0x00});
> +
> +    /* i686 guests are always supported */
> +    if ((guest = virCapabilitiesAddGuest(caps,
> +                                         "hvm",
> +                                         "i686",
> +                                         32,
> +                                         NULL, NULL, 0, NULL)) == NULL)
> +        goto error;
> +
> +    if (virCapabilitiesAddGuestDomain(guest,
> +                                      "vmware",
> +                                      NULL, NULL, 0, NULL) == NULL)
> +        goto error;
> +
> +    /* check if x86_64 guests are supported */
> +    if (VIR_ALLOC(cpu) < 0
> +        || !(cpu->arch = strdup(utsname.machine))) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +
> +    cpu->type = VIR_CPU_TYPE_HOST;
> +
> +    if (!(data = cpuNodeData(cpu->arch))
> +        || cpuDecode(cpu, data, NULL, 0, NULL) < 0) {
> +        goto error;
> +    }
> +
> +    if (cpuHasFeature(utsname.machine, data, "vmx")
> +        || cpuHasFeature(utsname.machine, data, "svm")) {
> +
> +        if ((guest = virCapabilitiesAddGuest(caps,
> +                                             "hvm",
> +                                             "x86_64",
> +                                             64,
> +                                             NULL, NULL, 0, NULL)) == NULL)
> +            goto error;

FYI, you can still get CPUs which are 32-bit only and have vmx/svm
supported.

> +int
> +vmwareLoadDomains(struct vmware_driver *driver)
> +{
> +    virDomainDefPtr vmdef = NULL;
> +    virDomainObjPtr vm = NULL;
> +    char *vmxPath = NULL;
> +    char *vmx = NULL;
> +    vmwareDomainPtr pDomain;
> +    char *directoryName = NULL;
> +    char *fileName = NULL;
> +    int ret = -1;
> +    esxVMX_Context ctx;
> +    size_t len = 0;
> +    char *outbuf = NULL;
> +    char *str;
> +    char *saveptr = NULL;
> +    virCommandPtr cmd;
> +
> +    ctx.parseFileName = esxCopyVMXFileName;
> +
> +    cmd = virCommandNewArgList(VMRUN, "-T",
> +                               driver->type == TYPE_PLAYER ? "player" : "ws",
> +                               "list", NULL);
> +    virCommandSetOutputBuffer(cmd, &outbuf);
> +    if (virCommandRun(cmd, NULL) < 0)
> +        goto cleanup;
> +
> +    for(str = outbuf ; (vmxPath = strtok_r(str, "\n", &saveptr)) != NULL;
> +        str = NULL) {
> +
> +        if (vmxPath[0] != '/')
> +            continue;

> +
> +        //vmrun list only reports running vms
> +        vm->state = VIR_DOMAIN_RUNNING;
> +        vm->def->id = driver->nextvmid++;
> +        vm->persistent = 1;

The VM ID is intended to be stable for the lifetime of a VM. It
seems like this could be unstable, depending on the order in
which vmrun -T returns the list. Is there any way to find a
more stable ID, even if it means using the VM's UNIX PID ?

> +static const char *
> +vmwareGetType(virConnectPtr conn)
> +{
> +    struct vmware_driver *driver = conn->privateData;
> +    int type;
> +
> +    type = driver->type;
> +    return type == TYPE_PLAYER ? "vmware player" : "vmware workstation";
> +}

This should just be returning the same string that's
in the type field of the virDriverPtr struct that
was registered.

> +static int
> +vmwareStartVM(struct vmware_driver *driver, virDomainObjPtr vm)
> +{
> +    const char *cmd[] = {
> +        VMRUN, "-T", PROGRAM_SENTINAL, "start",
> +        PROGRAM_SENTINAL, PROGRAM_SENTINAL, NULL
> +    };
> +
> +    if (vm->state != VIR_DOMAIN_SHUTOFF) {
> +        vmwareError(VIR_ERR_OPERATION_DENIED, "%s",

The OPERATION_DENIED code is for authentication / access control failures.
You want OPERATION_INVALID here, which is for case where the VM is in
the wrong lifecycle state for the operation.

> +                    _("domain is not in shutoff state"));
> +        return -1;
> +    }
> +
> +    vmwareSetSentinal(cmd, vmw_types[driver->type]);
> +    vmwareSetSentinal(cmd, ((vmwareDomainPtr) vm->privateData)->vmxPath);
> +    if (!((vmwareDomainPtr) vm->privateData)->gui)
> +        vmwareSetSentinal(cmd, NOGUI);
> +    else
> +        vmwareSetSentinal(cmd, NULL);
> +
> +    if (virRun(cmd, NULL) < 0) {
> +        vmwareError(VIR_ERR_INTERNAL_ERROR, _("Could not exec %s"), VMRUN);
> +        return -1;
> +    }
> +
> +    vm->def->id = driver->nextvmid++;
> +    vm->state = VIR_DOMAIN_RUNNING;
> +
> +    return 0;
> +}
> +static virDomainPtr
> +vmwareDomainDefineXML(virConnectPtr conn, const char *xml)
> +{
> +    struct vmware_driver *driver = conn->privateData;
> +    virDomainDefPtr vmdef = NULL;
> +    virDomainObjPtr vm = NULL;
> +    virDomainPtr dom = NULL;
> +    char *vmx = NULL;
> +    char *directoryName = NULL;
> +    char *fileName = NULL;
> +    char *vmxPath = NULL;
> +    vmwareDomainPtr pDomain = NULL;
> +    esxVMX_Context ctx;
> +
> +    ctx.formatFileName = esxCopyVMXFileName;
> +
> +    vmwareDriverLock(driver);
> +    if ((vmdef = virDomainDefParseString(driver->caps, xml,
> +                                         VIR_DOMAIN_XML_INACTIVE)) == NULL)
> +        goto cleanup;
> +
> +    vm = virDomainFindByName(&driver->domains, vmdef->name);
> +    if (vm) {
> +        vmwareError(VIR_ERR_OPERATION_FAILED,
> +                    _("Already an VMWARE VM active with the id '%s'"),
> +                    vmdef->name);
> +        goto cleanup;
> +    }

When checking for duplicates you need to check UUID and Name.
There is a convenient function that does this

    if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0)
        goto cleanup;


Also in driver entry points, it is recommended to always use
the FindByUUID method, rather than FindByName or FindByID.
If any of the FindBy methods fail, you should use the 
VIR_ERR_NO_DOMAIN error code if this failure needs to be
reported to the caller.

The INVALID_DOMAIN code is for invalid virDomainPtr pointers
only.


Daniel


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