2009/7/23 Daniel Veillard <veillard@xxxxxxxxxx>:
> 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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list