On Tue, Jul 21, 2009 at 04:58:50PM +0200, Matthias Bolte wrote:
>
Now reviewing the main code,
[...]
> +typedef struct _esxPrivate {
> + esxVI_Context *host;
> + esxVI_Context *vcenter;
> + int phantom; // boolean
Do we really need phantom ? this is basically a debug mode with no real
ESX server, right ?
> + char *transport;
> + int32_t nvcpus_max;
> + esxVI_Boolean supports_vmotion;
> + int32_t usedCpuTimeCounterId;
> +} esxPrivate;
> +
[...]
> + /* Check for 'esx:///phantom' URI */
> + if (conn->uri->server == NULL && conn->uri->path != NULL &&
> + STREQ(conn->uri->path, "/phantom")) {
> + phantom = 1;
> + }
Comparing against the exact string would have been simpler/more
accurate if still available (to avoid esx:///phantom#foo or
esx:///phantom?bar)
[...]
> +
> + password = esxUtil_RequestPassword(auth, username, conn->uri->server);
[...]
> +
> + if (vcenter != NULL) {
> + if (virAsprintf(&url, "%s://%s/sdk", priv->transport,
> + vcenter) < 0) {
> + virReportOOMError(conn);
> + goto failure;
> + }
> +
> + if (esxVI_Context_Alloc(conn, &priv->vcenter) < 0) {
> + goto failure;
> + }
> +
> + username = esxUtil_RequestUsername(auth, "administrator", vcenter);
> +
> + if (username == NULL) {
> + ESX_ERROR(conn, VIR_ERR_AUTH_FAILED,
> + "Username request failed");
> + goto failure;
> + }
> +
> + password = esxUtil_RequestPassword(auth, username, vcenter);
I'm not sure I understand why there are 2 requests for authentication
needed but there must be a reason :-)
[...]
> +static esxVI_Boolean
> +esxSupportsVMotion(virConnectPtr conn)
> +{
> + esxPrivate *priv = (esxPrivate *)conn->privateData;
> + esxVI_String *propertyNameList = NULL;
> + esxVI_ObjectContent *hostSystem = NULL;
> + esxVI_DynamicProperty *dynamicProperty = NULL;
> +
> + if (priv->phantom) {
> + ESX_ERROR(conn, VIR_ERR_OPERATION_INVALID,
> + "Not possible with a phantom connection");
> + goto failure;
> + }
I still didn't found the use for phantom :-)
> + if (priv->supports_vmotion != esxVI_Boolean_Undefined) {
> + return priv->supports_vmotion;
> + }
> +
> + if (esxVI_EnsureSession(conn, priv->host) < 0) {
> + goto failure;
> + }
> +
> + if (esxVI_String_AppendValueToList(conn, &propertyNameList,
> + "capability.vmotionSupported") < 0 ||
> + esxVI_GetObjectContent(conn, priv->host, priv->host->hostFolder,
> + "HostSystem", propertyNameList,
> + esxVI_Boolean_True, &hostSystem) < 0) {
> + goto failure;
> + }
> +
> + if (hostSystem == NULL) {
> + ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
> + "Could not retrieve the HostSystem object");
> + goto failure;
> + }
> +
> + for (dynamicProperty = hostSystem->propSet; dynamicProperty != NULL;
> + dynamicProperty = dynamicProperty->_next) {
> + if (STREQ(dynamicProperty->name, "capability.vmotionSupported")) {
> + if (esxVI_AnyType_ExpectType(conn, dynamicProperty->val,
> + esxVI_Type_Boolean) < 0) {
> + goto failure;
> + }
> +
> + priv->supports_vmotion = dynamicProperty->val->boolean;
> + break;
> + } else {
> + VIR_WARN("Unexpected '%s' property", dynamicProperty->name);
> + }
> + }
> +
> + cleanup:
> + esxVI_String_Free(&propertyNameList);
> + esxVI_ObjectContent_Free(&hostSystem);
> +
> + return priv->supports_vmotion;
> +
> + failure:
> + priv->supports_vmotion = esxVI_Boolean_Undefined;
> +
> + goto cleanup;
> +}
Okay most driver entry points are similar query/response/parsing and
conversion to expected output
[...]
> +static virDomainPtr
> +esxDomainLookupByID(virConnectPtr conn, int id)
> +{
[...]
> + char *name_ = NULL;
why the underscore. I was a bit puzzled by the way name_ is freed in
the loop but in retrospection that works :)
[...]
> +static virDomainPtr
> +esxDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid)
> +{
[...]
> + char *name_ = NULL;
same here.
> + goto failure;
> + }
One thing we need to fix is all the erro strings need to be localized
> + if (powerState != esxVI_VirtualMachinePowerState_PoweredOn) {
> + ESX_ERROR(domain->conn, VIR_ERR_OPERATION_INVALID,
> + "Domain is not powered on");
should become _("Domain is not powered on")
But that's really simple to change
> +static char *
> +esxDomainGetOSType(virDomainPtr dom ATTRIBUTE_UNUSED)
> +{
> + return strdup("hvm");
we need to check for failure and use the out of memory handler here.
like in esxDomainGetSchedulerType()
[...]
> +static char *
> +esxDomainDumpXML(virDomainPtr domain, int flags)
> +{
[..]
> + /* expected format: "[<datastoreName>] <vmxPath>" */
> + if (sscanf(vmPathName, "[%a[^]%]] %as", &datastoreName, &vmxPath) != 2) {
> + ESX_ERROR(domain->conn, VIR_ERR_OPERATION_INVALID,
> + "'config.files.vmPathName' property '%s' doesn't have "
> + "expected format '[<datastore>] <vmx>'", vmPathName);
> + goto failure;
> + }
> +
> + if (virAsprintf(&url, "%s://%s/folder/%s?dcPath=%s&dsName=%s",
> + priv->transport, domain->conn->uri->server,
> + vmxPath, priv->host->datacenter->value,
> + datastoreName) < 0) {
> + virReportOOMError(domain->conn);
> + goto failure;
> + }
> +
> + if (esxVI_Context_Download(domain->conn, priv->host, url, &vmx) < 0) {
> + goto failure;
> + }
> +
> + def = esxVMX_ParseConfig(domain->conn, vmx);
> +
> + if (def != NULL) {
> + xml = virDomainDefFormat(domain->conn, def, flags);
> + }
Rather cool :-) though the details lies in esxVMX_ParseConfig, a long
function but not too nasty.
> +static int
> +esxDomainGetSchedulerParameters(virDomainPtr domain,
> + virSchedParameterPtr params, int *nparams)
> +{
Hum, the scheduler API is so free form that some details of the
scheduling parameter handled and their semantic need to be documented,
here, and in the future HTML driver page.
> + snprintf (params[i].field, VIR_DOMAIN_SCHED_FIELD_LENGTH, "%s",
> + "reservation");
[...]
> + snprintf (params[i].field, VIR_DOMAIN_SCHED_FIELD_LENGTH, "%s",
> + "limit");
[...]
> + snprintf (params[i].field, VIR_DOMAIN_SCHED_FIELD_LENGTH, "%s",
> + "shares");
and same for here:
> +static int
> +esxDomainSetSchedulerParameters(virDomainPtr domain,
> + virSchedParameterPtr params, int nparams)
> +{
Pointers to the 3 definitions on VMWare doc could be sufficient though
> +
> + /* Validate the purposed migration */
> + if (esxVI_ValidateMigration(domain->conn, priv->vcenter,
> + virtualMachine->obj,
> + esxVI_VirtualMachinePowerState_Undefined,
> + NULL, resourcePool, hostSystem->obj,
> + &eventList) < 0) {
> + goto failure;
> + }
Migration checking, that's something we were planning at the libvirt
API level too !
[...]
> +static virDriver esxDriver = {
> + VIR_DRV_ESX,
> + "ESX",
> + esxOpen, /* open */
> + esxClose, /* close */
> + esxSupportsFeature, /* supports_feature */
> + esxGetType, /* type */
> + esxGetVersion, /* version */
> + esxGetHostname, /* hostname */
> + NULL, /* getMaxVcpus */
> + esxNodeGetInfo, /* nodeGetInfo */
> + NULL, /* getCapabilities */
> + esxListDomains, /* listDomains */
> + esxNumberOfDomains, /* numOfDomains */
> + NULL, /* domainCreateXML */
> + esxDomainLookupByID, /* domainLookupByID */
> + esxDomainLookupByUUID, /* domainLookupByUUID */
> + esxDomainLookupByName, /* domainLookupByName */
> + esxDomainSuspend, /* domainSuspend */
> + esxDomainResume, /* domainResume */
> + esxDomainShutdown, /* domainShutdown */
> + esxDomainReboot, /* domainReboot */
> + esxDomainDestroy, /* domainDestroy */
> + esxDomainGetOSType, /* domainGetOSType */
> + esxDomainGetMaxMemory, /* domainGetMaxMemory */
> + esxDomainSetMaxMemory, /* domainSetMaxMemory */
> + esxDomainSetMemory, /* domainSetMemory */
> + esxDomainGetInfo, /* domainGetInfo */
> + NULL, /* domainSave */
> + NULL, /* domainRestore */
> + NULL, /* domainCoreDump */
> + esxDomainSetVcpus, /* domainSetVcpus */
> + NULL, /* domainPinVcpu */
> + NULL, /* domainGetVcpus */
> + esxDomainGetMaxVcpus, /* domainGetMaxVcpus */
> + NULL, /* domainGetSecurityLabel */
> + NULL, /* nodeGetSecurityModel */
> + esxDomainDumpXML, /* domainDumpXML */
> + esxDomainXMLFromNative, /* domainXmlFromNative */
> + NULL, /* domainXmlToNative */
> + esxListDefinedDomains, /* listDefinedDomains */
> + esxNumberOfDefinedDomains, /* numOfDefinedDomains */
> + esxDomainCreate, /* domainCreate */
> + NULL, /* domainDefineXML */
> + NULL, /* domainUndefine */
> + NULL, /* domainAttachDevice */
> + NULL, /* domainDetachDevice */
> + NULL, /* domainGetAutostart */
> + NULL, /* domainSetAutostart */
> + esxDomainGetSchedulerType, /* domainGetSchedulerType */
> + esxDomainGetSchedulerParameters, /* domainGetSchedulerParameters */
> + esxDomainSetSchedulerParameters, /* domainSetSchedulerParameters */
> + esxDomainMigratePrepare, /* domainMigratePrepare */
> + esxDomainMigratePerform, /* domainMigratePerform */
> + esxDomainMigrateFinish, /* domainMigrateFinish */
> + NULL, /* domainBlockStats */
> + NULL, /* domainInterfaceStats */
> + NULL, /* domainBlockPeek */
> + NULL, /* domainMemoryPeek */
> + NULL, /* nodeGetCellsFreeMemory */
> + NULL, /* nodeGetFreeMemory */
> + NULL, /* domainEventRegister */
> + NULL, /* domainEventDeregister */
> + NULL, /* domainMigratePrepare2 */
> + NULL, /* domainMigrateFinish2 */
> + NULL, /* nodeDeviceDettach */
> + NULL, /* nodeDeviceReAttach */
> + NULL, /* nodeDeviceReset */
> +};
That's not complete, but IMHO well worth providing at this point !
> +char *
> +esxUtil_RequestUsername(virConnectAuthPtr auth, const char *default_username,
> + const char *server)
> +{
[...]
> +char *
> +esxUtil_RequestPassword(virConnectAuthPtr auth, const char *username,
> + const char *server)
> +{
auth hooks looks clean to me
might be worth documenting what some of those low level utilities do, as
it's not clear just from reading them, especially with things like
handling different server version (assuming I understood):
> +int
> +esxUtil_ParseQuery(virConnectPtr conn, char **transport, char **vcenter)
> +{
[...]
> +}
[...]
> + if (errcode != 0) {
> + ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
> + "IP address lookup for host '%s' failed: %s", hostname,
error message localisation is missing in this module too.
[...]
> diff --git a/src/esx/esx_util.h b/src/esx/esx_util.h
[...]
> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
[...]
> + /*
> + * Add a dummy expect header to stop CURL from waiting for a response code
> + * 100 (Continue) from the server before continuing the POST operation.
> + * Waiting for this response would slowdown each communication with the
> + * server by approx. 2 sec, because the server doesn't send the expected
> + * 100 (Continue) response and the wait times out resulting in wasting
> + * approx. 2 sec per POST operation.
> + */
I remember that one from my first review :-)
> + if (ctx->vmFolder == NULL || ctx->hostFolder == NULL) {
> + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
> + "The 'datacenter' object is missing the "
> + "'vmFolder'/'hostFolder' propoerty");
should be 'property' and between _() :-)
> + if ((*remoteResponse)->response_code == 500) {
> + (*remoteResponse)->xpathObject =
> + xmlXPathEval(BAD_CAST
> + "/soapenv:Envelope/soapenv:Body/soapenv:Fault",
> + (*remoteResponse)->xpathContext);
> +
> + if (esxVI_RemoteResponse_DeserializeXPathObject
> + (conn, *remoteResponse,
> + (esxVI_RemoteResponse_DeserializeFunc)
> + esxVI_Fault_Deserialize,
> + (void **)&fault) < 0) {
> + goto failure;
> + }
the XPath code and handling could probably be made a bit easier and
more resistant by using libvirt own wrapper, for example
virXPathNode(NULL, "/soapenv:Envelope/soapenv:Body/soapenv:Fault"
(*remoteResponse)->xpathContext);
or virXPathNodeSet if multiple soapenv:Fault are allowed.
> + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
> + "HTTP response code %d. VI Fault: %s - %s",
> + (int)(*remoteResponse)->response_code,
> + fault->faultcode, fault->faultstring);
> +
> + goto failure;
> + } else {
> + (*remoteResponse)->xpathObject =
> + xmlXPathEval(BAD_CAST remoteRequest->xpathExpression,
> + (*remoteResponse)->xpathContext);
> + }
> + } else if ((*remoteResponse)->response_code != 200) {
> + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
> + "HTTP response code %d",
> + (int)(*remoteResponse)->response_code);
> +
> + goto failure;
> + }
but that's not urgent in any way
[...]
> +int
> +esxVI_RemoteResponse_DeserializeXPathObject
> + (virConnectPtr conn, esxVI_RemoteResponse *remoteResponse,
> + esxVI_RemoteResponse_DeserializeFunc deserializeFunc, void **item)
> +{
> + xmlNodePtr node = NULL;
> +
> + if (remoteResponse->xpathObject != NULL &&
> + remoteResponse->xpathObject->type == XPATH_NODESET) {
> + if (remoteResponse->xpathObject->nodesetval->nodeNr != 1) {
that would just avoid this kind of nasty code present in all
deserialization routines.
[...]
> +int
> +esxVI_BuildFullTraversalSpecList(virConnectPtr conn,
> + esxVI_SelectionSpec **fullTraversalSpecList)
> +{
> + if (fullTraversalSpecList == NULL || *fullTraversalSpecList != NULL) {
> + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Invalid argument");
> + return -1;
> + }
> +
> + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList,
> + "visitFolders",
> + "Folder", "childEntity",
> + "visitFolders\0"
> + "datacenterToVmFolder\0"
> + "datacenterToHostFolder\0"
> + "computeResourceToHost\0"
> + "computeResourceToResourcePool\0"
> + "HostSystemToVm\0"
> + "resourcePoolToVm\0") < 0) {
> + goto failure;
> + }
> +
> + /* Traversal through vmFolder branch */
> + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList,
> + "datacenterToVmFolder",
> + "Datacenter", "vmFolder",
> + "visitFolders\0") < 0) {
> + goto failure;
> + }
> +
> + /* Traversal through hostFolder branch */
> + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList,
> + "datacenterToHostFolder",
> + "Datacenter", "hostFolder",
> + "visitFolders\0") < 0) {
> + goto failure;
> + }
> +
> + /* Traversal through host branch */
> + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList,
> + "computeResourceToHost",
> + "ComputeResource", "host",
> + NULL) < 0) {
> + goto failure;
> + }
> +
> + /* Traversal through resourcePool branch */
> + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList,
> + "computeResourceToResourcePool",
> + "ComputeResource", "resourcePool",
> + "resourcePoolToResourcePool\0"
> + "resourcePoolToVm\0") < 0) {
> + goto failure;
> + }
> +
> + /* Recurse through all resource pools */
> + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList,
> + "resourcePoolToResourcePool",
> + "ResourcePool", "resourcePool",
> + "resourcePoolToResourcePool\0"
> + "resourcePoolToVm\0") < 0) {
> + goto failure;
> + }
> +
> + /* Recurse through all hosts */
> + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList,
> + "HostSystemToVm",
> + "HostSystem", "vm",
> + "visitFolders\0") < 0) {
> + goto failure;
> + }
> +
> + /* Recurse through all resource pools */
> + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList,
> + "resourcePoolToVm",
> + "ResourcePool", "vm", NULL) < 0) {
> + goto failure;
> + }
> +
> + return 0;
> +
> + failure:
> + esxVI_SelectionSpec_Free(fullTraversalSpecList);
> +
> + return -1;
> +}
Now that I see this again I remember how perplex I was about the
VI API when I tried working with it at the time ...
[...]
> diff --git a/src/esx/esx_vi.h b/src/esx/esx_vi.h
The header is incomparably simpler than what would be generated by
csoap !!!
> +int
> +esxVI_SessionIsActive(virConnectPtr conn, esxVI_Context *ctx,
> + const char *sessionID, const char *userName,
> + esxVI_Boolean *active)
> +{
[...]
> + cleanup:
> + /*
> + * Remove static values from the data structures to prevent them from
> + * being freed by the call to esxVI_RemoteRequest_Free().
> + */
> + if (remoteRequest != NULL) {
> + remoteRequest->xpathExpression = NULL;
> + }
Hum, and that doesn't generate a leak ?
I see that others routines do the same, or is
remoteRequest->xpathExpression still stored somewhere else ?
> + esxVI_RemoteRequest_Free(&remoteRequest);
> + esxVI_RemoteResponse_Free(&remoteResponse);
> +
> + return result;
> +
> + failure:
> + free(virBufferContentAndReset(&buffer));
> +
> + result = -1;
> +
> + goto cleanup;
> +}
> diff --git a/src/esx/esx_vi_methods.h b/src/esx/esx_vi_methods.h
[...]
> diff --git a/src/esx/esx_vi_types.c b/src/esx/esx_vi_types.c
Hum more localized strings needed here ... even in convenience macros
> + if (string == NULL) { \
> + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR, \
> + "XML node doesn't contain text, expecting an " \
> + _xsdType" value"); \
The problem is that the string is generated as part of the macro, I
wonder how this will play with _() ...
Not much to say about the type handling, except it's heavy !!!
> diff --git a/src/esx/esx_vi_types.h b/src/esx/esx_vi_types.h
even the list of entry point weights quite a bit.
> diff --git a/src/esx/esx_vmx.c b/src/esx/esx_vmx.c
[...]
> +## nets: bridged ###############################################################
> +
> +...
> +->type = _NET_TYPE_BRIDGE <=> ethernet0.connectionType = "bridged" # defaults to "bridged"
> +
> +
> +## nets: hostonly ##############################################################
> +
> +... # FIXME: maybe not supported by ESX?
> +->type = _NET_TYPE_NETWORK <=> ethernet0.connectionType = "hostonly" # defaults to "bridged"
> +
> +
> +## nets: nat ###################################################################
> +
> +... # FIXME: maybe not supported by ESX?
> +->type = _NET_TYPE_USER <=> ethernet0.connectionType = "nat" # defaults to "bridged"
> +
I not so sure about what the question is there ...
> +## nets: custom ################################################################
> +
> +...
> +->type = _NET_TYPE_BRIDGE <=> ethernet0.connectionType = "custom" # defaults to "bridged"
> +->data.bridge.brname = <value> <=> ethernet0.vnet = "<value>"
One thing which should be added is test cases for the VMX <-> XML
formats conversions, should be relatively easy to add in the
infrastructure once we have the examples.
> diff --git a/src/esx/esx_vmx.h b/src/esx/esx_vmx.h
Okay, beside those comments I think this is in a shape good enough
for commit, then I suggest to start refinements as incremental patches.
One of the challenges will be the localization of strings, I expect a
huge amount to result from those modules.
I think the .syms will need adjustment too, especially if we want to
hook up regression tests for the VMX parsing.
ACK from me, that's an impressive patch, but I'm not tempted to
rereview it once again in full :-)
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list