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

Re: [libvirt] VMware ESX driver progress



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


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