9:34 a.m.
2009/7/23 Daniel Veillard <veillard(a)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