2009/7/23 Daniel Veillard <veillard@redhat.com>:
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 ?
The phantom mode allows for using domain-xml-to/from-native without an actual ESX server. But besides that, it has no real use.
+ 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)
The driver open function has the URI available is parsed xmlURIPtr form only.
[...]
+ + 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 :-)
The driver needs to talk to different hosts for different functionality. Most stuff can be done with the ESX host itself, but a vCenter is needed for migration. If the connection URI contains a vCenter parameter the open function requests authentication for the ESX host and the vCenter independently, because authentication information can be different for each of them.
[...]
+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 :-)
See esxDomainXMLFromNative().
+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 :)
The underscore is superfluous, will be cleaned up. The loop iterates over all domains and searches for the matching one. esxVI_GetVirtualMachineIdentity() allocates the name, so the name must be freed before calling esxVI_GetVirtualMachineIdentity() again to free the allocated name from the previous iteration. Otherwise memory could leak.
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
Already on the todo list.
+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()
Added to the todo list.
+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.
Already on the todo list.
+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):
Added to the todo list.
+ 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.
I'll have a look at the virXPath* functions. Added to the todo list.
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 !!!
Well, our client implements a small subset of the VI API, only the methods and types that are really needed for the driver. csoap generates code for the whole thing.
+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 ?
As the comment states, the remoteRequest->xpathExpression is a static string (not allocated) and esxVI_RemoteRequest_Free() would free it. So setting it to NULL before freeing prevents esxVI_RemoteRequest_Free() from freeing non-allocated memory and doesn't generate a leak. Maybe I'll reverse the logic and esxVI_RemoteRequest_Free() doesn't free xpathExpression anymore, because in most cases xpathExpression is non-allocated. Maybe I'll also precompile the static XPath expressions as you or DPB suggested earlier. Anyway, it's already on the todo list to have a look at the XPath handling again.
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 _() ...
The string could be moved out of the macro. Before the macro: static const char *someErrorMessage = gettext_noop("XML node doesn't contain text, expecting an %s value"); And inside the macro: ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR, gettext(someErrorMessage), _xsdType); Or even this inside the macro may work: ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR, _("XML node doesn't contain text, expecting an %s value"), _xsdType);
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 ...
Do you refer to "# FIXME: maybe not supported by ESX?"? This is just a remark for me. http://www.sanbarrow.com/vmx.html states that there are 4 modes for networks. But it's not clear if all of them are supported by the ESX host, because http://www.sanbarrow.com/vmx.html contains information about the general VMX format that VMware uses for its other virtualisation software too. I just haven't tested yet if hostonly and nat mode are supported by ESX.
+## 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.
Already on the todo list. Regards, Matthias