7:54 p.m.
2009/5/19 Daniel Veillard <veillard(a)redhat.com>:
> The driver uses the SOAP based VMware VI API. We've tried to
generate
> SOAP client code with gSOAP and looked at other SOAP libraries, but
> the VI API uses inheritance in a way that gSOAP fails to generate C
Yes, I have tried that before, and it looked nasty, basically if you
use the C generator on the bindings you get something enormous, plus the
requirement on the soap library, etc ... 10 times as big as libvirt
itself.
IIRC approx. 17 MB generated C code in one file as we tested gSOAP for
the VMware VI API.
The other possibility was to use the libraries they provide to
interconnect, but there was a licencing problem, plus you can't expect
to have thing installed on the server, so this was a dead end too.
We looked at this closed source communication library provided by
VMware too, but dropped it because of the licensing problems.
> The next item on the todo list is domain creation, because
currently
> the driver can only deal with prior existing domains.
Actually make sure you have dumpXML first, that's probably more
important as a first step since it finalize the XML format.
We need the XML format first, before we can use it to define or create
domains, so that was implied when saying domain creation is next.
> diff --git a/src/Makefile.am b/src/Makefile.am
> index fd692b4..1dc7882 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -144,6 +144,11 @@ UML_DRIVER_SOURCES =
\
> uml_conf.c uml_conf.h \
> uml_driver.c uml_driver.h
>
> +ESX_DRIVER_SOURCES = \
> + esx_driver.c esx_driver.h \
> + esx_util.c esx_util.h \
> + esx_vi.c esx_vi.h
> +
maybe we should use an esx/ subdirectory like for vbox
Will be done.
> diff --git a/src/esx_driver.c b/src/esx_driver.c
> new file mode 100644
> index 0000000..28500b8
> --- /dev/null
> +++ b/src/esx_driver.c
> @@ -0,0 +1,1385 @@
[...]
> +#define ESX_ERROR(conn, code, fmt...) \
> + /* FIXME: remove this VIR_DEBUG0 line when libvirt provides */ \
> + /* file and line information in error messages */ \
> + VIR_DEBUG0 ("this is a dummy message to provide a source " \
> + "position for the following error..."); \
> + virReportErrorHelper (conn, VIR_FROM_ESX, code, __FILE__, \
> + __FUNCTION__, __LINE__, fmt)
interesting, what's the problem ?
virLogMessage() takes filename, funcname and linenr parameters and
outputs them along with the message. This works fine for the VIR_DEBUG
macro. virReportErrorHelper() also takes filename, funcname and linenr
parameters but doesn't use them, because virRaiseError() doesn't
handle this debugging information, because the virError struct has no
place to store them. virRaiseError() also contains a comment "TODO:
pass function name and lineno".
The VIR_DEBUG0 is part of the ESX_ERROR macro just to provide the
filename, funcname and linenr information for debugging, because this
information currently gets lost in virReportErrorHelper() before the
error is reported/logged. Also virLogMessage() only outputs this
information for VIR_LOG_DEBUG level.
> +static virDrvOpenStatus
> +esxOpen(virConnectPtr conn, virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED)
> +{
Any idea if there is a notion of read-only connection in ESX, or is
everything role based ?
It seems everything is account/group/role/privilege based. So there is
no simple common way for a read-only connection. The user may login
via a very restricted account that has been manually preconfigured for
the ESX host, but there seems to be no special, default read-only
access that the driver could use if the read-only flag is set for the
open call.
> +static int
> +esxDomainMigratePrepare(virConnectPtr dconn,
> + char **cookie ATTRIBUTE_UNUSED,
> + int *cookielen ATTRIBUTE_UNUSED,
> + const char *uri_in,
> + char **uri_out,
> + unsigned long flags ATTRIBUTE_UNUSED,
> + const char *dname ATTRIBUTE_UNUSED,
> + unsigned long resource ATTRIBUTE_UNUSED)
> +{
> + if (uri_in == NULL) {
> + if (virAsprintf(uri_out, "%s://%s%s",
> + dconn->uri->scheme + 4, dconn->uri->server,
> + dconn->uri->path) < 0) {
> + virReportOOMError(dconn);
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
Is there really no way to check for compatibility for example ?
As in checking if a proposed migration can succeed before performing
the actual migration? Yes there is, it's just not implemented yet and
a note about it is missing in the code :)
> diff --git a/src/esx_util.c b/src/esx_util.c
> new file mode 100644
> index 0000000..c2e492c
> --- /dev/null
> +++ b/src/esx_util.c
> @@ -0,0 +1,170 @@
[...]
> +char *
> +esxUtil_MigrateStringFromLibXML(xmlChar *xmlString)
> +{
> + char *esxString = NULL;
> + size_t length = 0;
> +
> + if (xmlString == NULL) {
> + return NULL;
> + }
> +
> + length = xmlStrlen(xmlString);
> +
> + if (VIR_ALLOC_N(esxString, length + 1) >= 0) {
> + strncpy(esxString, (const char *) xmlString, length);
> + }
> +
> + xmlFree(xmlString);
> +
> + return esxString;
> +}
Hum, this is a bit over the top, xmlChar * is just an UTF-8 C string,
and unless someone registered specific libxml2 allocators you don't need
to use xmlFree, free() is sufficient. Since other pieces of libvirt
don't xmlFree their libxml2 allocated strings this is superfluous IMHO.
Well, the memory handling is currently written in a way that it uses
the matching alloc/free function pairs and doesn't make any
assumptions about miscibility of these pairs. Because the objects of
the VI API client use VIR_ALLOC/VIR_FREE, all elements of them must
also be allocated using VIR_ALLOC. If you say its okay to free a
pointer allocated by libxml2 with VIR_FREE then
esxUtil_MigrateStringFromLibXML() and esxUtil_Strdup() can be removed,
because their sole purpose is to guarantee the use of matching
alloc/free functions.
> +char *
> +esxUtil_Strdup(const char *string)
> +{
> + char *duplicate = NULL;
> + size_t length = 0;
> +
> + if (string == NULL) {
> + return NULL;
> + }
> +
> + length = strlen(string);
> +
> + if (VIR_ALLOC_N(duplicate, length + 1) >= 0) {
> + strncpy(duplicate, string, length);
> + }
virAllocN() won't do it so NULL should raise an OOM error here.
esxUtil_Strdup() is used like strdup() in the rest of the driver. Its
return value is checked for NULL and an OOM error is raised if it's
NULL. But as mentioned before, esxUtil_Strdup()'s sole purpose is to
guarantee the use of matching alloc/free functions.
> +
> + return duplicate;
> +}
That would have to be made globally, but having it already helps
esxUtil_Strdup() could be replaced by a simple strdup(), see comment
on esxUtil_MigrateStringFromLibXML() above.
> + ctx->curl_headers =
curl_slist_append(ctx->curl_headers, "Content-Type: "
> + "text/xml; charset=UTF-8");
> +
> + /* 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.
> + */
> + ctx->curl_headers = curl_slist_append(ctx->curl_headers,
> + "Expect: nothing");
I guess you had fun finding this !
Took 1-2h to read libcurl code to figure out where this 2sec pause
comes from and how to stop libcurl from doing this.
> +int
> +esxVI_RemoteRequest_Execute(virConnectPtr conn, esxVI_Context *ctx,
> + esxVI_RemoteRequest *remoteRequest,
> + esxVI_RemoteResponse **remoteResponse)
> +{
> + virBuffer buffer = VIR_BUFFER_INITIALIZER;
> + esxVI_Fault *fault = NULL;
> +
> + if (remoteRequest == NULL || remoteRequest->request == NULL ||
> + remoteResponse == NULL || *remoteResponse != NULL) {
> + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Invalid argument");
> + goto failure;
> + }
> +
> + if (esxVI_RemoteResponse_Alloc(conn, remoteResponse) < 0) {
> + goto failure;
> + }
> +
> + virMutexLock(&ctx->curl_lock);
> +
> + curl_easy_setopt(ctx->curl_handle, CURLOPT_ERRORBUFFER,
> + (*remoteResponse)->error);
> + curl_easy_setopt(ctx->curl_handle, CURLOPT_WRITEDATA, &buffer);
> + curl_easy_setopt(ctx->curl_handle, CURLOPT_POSTFIELDS,
> + remoteRequest->request);
> + curl_easy_setopt(ctx->curl_handle, CURLOPT_POSTFIELDSIZE,
> + strlen(remoteRequest->request));
> +
> + if (curl_easy_perform(ctx->curl_handle) != CURLE_OK) {
> + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
> + "curl_easy_perform returned an error");
> + goto unlock;
> + }
> +
> + if (curl_easy_getinfo(ctx->curl_handle, CURLINFO_RESPONSE_CODE,
> + &(*remoteResponse)->code) != CURLE_OK) {
> + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
> + "curl_easy_getinfo returned an error");
> + goto unlock;
> + }
> +
> + virMutexUnlock(&ctx->curl_lock);
Okay one serial operation per connection probably good enough ...
As I understand libcurl a curl handle can only be used by one thread
at the same time. Allowing multiple concurrent operations per
connection would involve using multiple curl handles.
> + (*remoteResponse)->xpathContext =
> + xmlXPathNewContext((*remoteResponse)->document);
if you can deal only with one operation per connection that could be
optimized and the context you be persistent, just attach to the new
document, but not worth worring
I'll note this on the todo list with low priority.
> + if ((*remoteResponse)->xpathContext == NULL) {
> + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
> + "Could not create XPath context");
> + goto failure;
> + }
> +
> + xmlXPathRegisterNs((*remoteResponse)->xpathContext, BAD_CAST
"soapenv",
> + BAD_CAST
"http://schemas.xmlsoap.org/soap/envelope/");
> + xmlXPathRegisterNs((*remoteResponse)->xpathContext, BAD_CAST
"vim",
> + BAD_CAST "urn:vim25");
> +
> + if ((*remoteResponse)->code != 200) {
> + (*remoteResponse)->xpathObject =
> + xmlXPathEval(BAD_CAST
> +
"/soapenv:Envelope/soapenv:Body/soapenv:Fault",
> + (*remoteResponse)->xpathContext);
And libxml2 static XPath experssion could be precompiled, but it's not
worth optimizing at this point :-)
I'll note this on the todo list with low priority too.
looks good though I didn't check the lock carefully...
The curl_lock is used in esxVI_RemoteRequest_Execute() only, to
serialize the access to the curl_handle.
> +}
> +
> +int
> +esxVI_List_Append(virConnectPtr conn, esxVI_List **list, esxVI_List *item)
> +{
> + esxVI_List *next = NULL;
> +
> + if (list == NULL || item == NULL) {
> + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Invalid argument");
> + return -1;
> + }
> +
> + if (*list == NULL) {
> + *list = item;
> + return 0;
> + }
> +
> + next = *list;
> +
> + while (next->_next != NULL) {
> + next = next->_next;
> + }
compared to XPAth processing, a doubly linked list might not be worth
the time it saves...
Are you referring to the O(n) runtime for appending to the list? No
need for a double linked list at this point, single link is good
enough. I assume (but I'm not 100% sure) that the order of items in
lists in SOAP requests and responses is arbitrary, so
esxVI_List_Append() could be replaced with esxVI_List_Prepend() with
O(1) runtime.
> + next->_next = item;
> +
> + return 0;
> +}
an awful lot of type handling, even with the mcro helpers this looks heavy.
Well, yes. SOAP doesn't really map very well to C, if you want to have
a proper layer around the (de)serialization an transport code.
then the task functions
> +int
> +esxVI_WaitForTaskCompletion(virConnectPtr conn, esxVI_Context *ctx,
> + esxVI_ManagedObjectReference *task,
> + esxVI_TaskInfoState *finalState)
> +{
> + int result = 0;
> + esxVI_ObjectSpec *objectSpec = NULL;
> + esxVI_PropertySpec *propertySpec = NULL;
> + esxVI_PropertyFilterSpec *propertyFilterSpec = NULL;
> + esxVI_ManagedObjectReference *propertyFilter = NULL;
> + const char *version = NULL;
> + esxVI_UpdateSet *updateSet = NULL;
> + esxVI_PropertyFilterUpdate *propertyFilterUpdate = NULL;
> + esxVI_ObjectUpdate *objectUpdate = NULL;
> + esxVI_PropertyChange *propertyChange = NULL;
> + esxVI_AnyType *propertyValue = NULL;
> + esxVI_TaskInfoState state = esxVI_TaskInfoState_Undefined;
> +
> + version = esxUtil_Strdup("");
> +
> + if (version == NULL) {
> + virReportOOMError(conn);
> + goto failure;
> + }
> +
> + if (esxVI_ObjectSpec_Alloc(conn, &objectSpec) < 0) {
> + goto failure;
> + }
> +
> + objectSpec->obj = task;
> + objectSpec->skip = esxVI_Boolean_False;
> +
> + if (esxVI_PropertySpec_Alloc(conn, &propertySpec) < 0) {
> + goto failure;
> + }
> +
> + propertySpec->type = task->type;
> +
> + if (esxVI_String_AppendValueToList(conn, &propertySpec->pathSet,
> + "info.state") < 0 ||
> + esxVI_PropertyFilterSpec_Alloc(conn, &propertyFilterSpec) < 0 ||
> + esxVI_PropertySpec_AppendToList(conn, &propertyFilterSpec->propSet,
> + propertySpec) < 0 ||
> + esxVI_ObjectSpec_AppendToList(conn, &propertyFilterSpec->objectSet,
> + objectSpec) < 0 ||
> + esxVI_CreateFilter(conn, ctx, propertyFilterSpec, esxVI_Boolean_True,
> + &propertyFilter) < 0) {
> + goto failure;
> + }
> +
> + while (state != esxVI_TaskInfoState_Success &&
> + state != esxVI_TaskInfoState_Error) {
> + esxVI_UpdateSet_Free(&updateSet);
> +
> + if (esxVI_WaitForUpdates(conn, ctx, version, &updateSet) < 0) {
> + goto failure;
> + }
is ESX sending back a SOAP request here ?
No, esxVI_WaitForUpdate() just polls for state updates.
Interesting, looks like a lot of good work !
Thanks.
> diff --git a/src/esx_vi.h b/src/esx_vi.h
> new file mode 100644
> index 0000000..52e5112
[...]
> +/*
> + * XSD: Boolean
> + */
> +
> +enum _esxVI_Boolean {
> + esxVI_Boolean_Undefined = 0,
> + esxVI_Boolean_True,
> + esxVI_Boolean_False,
> +};
True == 1, False == 2, that's funky !
Many types in the VI API contain optional boolean values and having an
undefined value is a simple way to express them. If a boolean value is
set to undefined it's interpreted as not set. Defining undefined as 0
makes all boolean values in newly allocated VI API objects (allocated
by calloc in VIR_ALLOC) being undefined by default. On the other hand
it results in this funky values for true and false.
Regards,
Matthias