On Tue, May 19, 2009 at 04:52:04PM +0200, Matthias Bolte wrote: > Hello, Hello Matthias, > I'm participate in a project of the Paderborn Center for Parallel > Computing, an institute of the University of Paderborn: > http://pc2.uni-paderborn.de > > The project's goal is to use virtualization in a supercomputer > environment. We've decided to use libvirt to manage different > hypervisor. A subgoal is to extend the driver base of libvirt. We've Sounds coool :-) > started an VMware ESX driver and are investigating Hyper-V support > (see next mail). I will reply separately... > The ESX driver isn't complete yet, currently it supports: > > - domain lookup by ID, UUID and name > - domain listing > - domain suspend and resume > - domain reboot, if the VMware tools are installed inside the domain > - domain start and shutdown > - domain migration from a feature POV once you have the dumpXML for domains that would be good enough to be commited IMHO. > 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. 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. > code for. Because of this we wrote a minimal SOAP client based on > libcurl and libxml2 that can handle this inheritance problem in C. Agreed that's the best way from a code perspective but takes far more development. It's a bit annoying to add one more dependancy, but libcurl is sane, and nanohttp from libxml2 is really unsufficient, actually we advise to use libcurl when one need to go beyond. So yes that sounds very reasonnable independancly of any code review the development choices are sound. > 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 think this code might be useful for others and would be glad if the > driver could be merged into libvirt in medium term. Just need some review. At a first glance, this looks rather nice > All of our work is licensed under LGPLv2+. Looks good. A very quick review ... > --- a/configure.in > +++ b/configure.in looks fine > --- a/include/libvirt/virterror.h > +++ b/include/libvirt/virterror.h idem > 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 > diff --git a/src/driver.h b/src/driver.h > index b8e0a04..544329a 100644 > --- a/src/driver.h > +++ b/src/driver.h > @@ -21,6 +21,7 @@ typedef enum { Looks fine > diff --git a/src/libvirt.c b/src/libvirt.c > index f1f81b3..135e090 100644 idem > diff --git a/src/virterror.c b/src/virterror.c > index e12608d..4b73c2c 100644 > --- a/src/virterror.c > +++ b/src/virterror.c idem > 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 ? > +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 ? [...] > +static int > +esxDomainGetMaxVcpus(virDomainPtr domain ATTRIBUTE_UNUSED) > +{ > + /* FIXME: need a method to query the host via VI API for this limit */ > + int nvcpus_max = 4; > + > + return nvcpus_max; > +} Heh :-) > +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 ? > +static int > +esxDomainMigratePerform(virDomainPtr domain, > + const char *cookie ATTRIBUTE_UNUSED, > + int cookielen ATTRIBUTE_UNUSED, > + const char *uri, > + unsigned long flags ATTRIBUTE_UNUSED, > + const char *dname, > + unsigned long bandwidth ATTRIBUTE_UNUSED) > +{ Interesting, I will have to reread this again :-) > +} > + > +static virDomainPtr > +esxDomainMigrateFinish(virConnectPtr dconn, > + const char *dname, > + const char *cookie ATTRIBUTE_UNUSED, > + int cookielen ATTRIBUTE_UNUSED, > + const char *uri ATTRIBUTE_UNUSED, > + unsigned long flags ATTRIBUTE_UNUSED) > +{ > + return esxDomainLookupByName(dconn, dname); > +} > + > +static virDriver esxDriver = { > + VIR_DRV_ESX, > + "ESX", > + esxOpen, /* open */ > + esxClose, /* close */ > + esxSupportsFeature, /* supports_feature */ > + esxGetType, /* type */ > + esxGetVersion, /* version */ > + NULL, /* hostname */ > + NULL, /* getMaxVcpus */ > + NULL, /* nodeGetInfo */ > + NULL, /* getCapabilities */ > + esxListDomains, /* listDomains */ > + esxNumOfDomains, /* numOfDomains */ > + NULL, /* domainCreateXML */ This > + esxDomainLookupByID, /* domainLookupByID */ > + esxDomainLookupByUUID, /* domainLookupByUUID */ > + esxDomainLookupByName, /* domainLookupByName */ > + esxDomainSuspend, /* domainSuspend */ > + esxDomainResume, /* domainResume */ > + esxDomainShutdown, /* domainShutdown */ > + esxDomainReboot, /* domainReboot */ > + NULL, /* domainDestroy */ > + NULL, /* domainGetOSType */ > + NULL, /* domainGetMaxMemory */ > + NULL, /* domainSetMaxMemory */ > + NULL, /* domainSetMemory */ > + esxDomainGetInfo, /* domainGetInfo */ > + NULL, /* domainSave */ > + NULL, /* domainRestore */ > + NULL, /* domainCoreDump */ > + esxDomainSetVcpus, /* domainSetVcpus */ > + NULL, /* domainPinVcpu */ > + NULL, /* domainGetVcpus */ > + esxDomainGetMaxVcpus, /* domainGetMaxVcpus */ > + NULL, /* domainGetSecurityLabel */ > + NULL, /* nodeGetSecurityModel */ > + NULL, /* domainDumpXML */ and this are really the big missing entry points, I'm a bit surprized by the lack of Destroy or Set/GetMaxMemory which I would expect to be simple, on the other hand migration is a fairly big piece. > + esxListDefinedDomains, /* listDefinedDomains */ > + esxNumOfDefinedDomains, /* numOfDefinedDomains */ > + esxDomainCreate, /* domainCreate */ > + NULL, /* domainDefineXML */ > + NULL, /* domainUndefine */ > + NULL, /* domainAttachDevice */ > + NULL, /* domainDetachDevice */ > + NULL, /* domainGetAutostart */ > + NULL, /* domainSetAutostart */ > + NULL, /* domainGetSchedulerType */ > + NULL, /* domainGetSchedulerParameters */ > + NULL, /* domainSetSchedulerParameters */ > + esxDomainMigratePrepare, /* domainMigratePrepare */ > + esxDomainMigratePerform, /* domainMigratePerform */ > + esxDomainMigrateFinish, /* domainMigrateFinish */ > + NULL, /* domainBlockStats */ > + NULL, /* domainInterfaceStats */ > + NULL, /* domainBlockPeek */ > + NULL, /* domainMemoryPeek */ > + NULL, /* nodeGetCellsFreeMemory */ > + NULL, /* nodeGetFreeMemory */ > + NULL, /* domainEventRegister */ > + NULL, /* domainEventDeregister */ > + NULL, /* domainMigratePrepare2 */ > + NULL, /* domainMigrateFinish2 */ > + NULL, /* nodeDeviceDettach */ > + NULL, /* nodeDeviceReAttach */ > + NULL, /* nodeDeviceReset */ > +}; [...] > diff --git a/src/esx_driver.h b/src/esx_driver.h > new file mode 100644 > index 0000000..85f4b32 > --- /dev/null > +++ b/src/esx_driver.h > @@ -0,0 +1,29 @@ okay > 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. > +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. > + > + return duplicate; > +} That would have to be made globally, but having it already helps > diff --git a/src/esx_util.h b/src/esx_util.h > new file mode 100644 > index 0000000..d3a6346 > --- /dev/null > +++ b/src/esx_util.h okay > diff --git a/src/esx_vi.c b/src/esx_vi.c > new file mode 100644 > index 0000000..26a7d85 > --- /dev/null > +++ b/src/esx_vi.c > @@ -0,0 +1,4437 @@ > + > +/* > + * esx_vi.c: client for the VMware VI API 2.5 to manage ESX hosts That's the hard piece :-) One thing I'm wondering is about parallelism, as far as I remember lot of the VMWare API were asynchronous (and the docs gave me headache!) so I wonder how all this works in practice :-) [...] > + 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 ! [...] > +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 ... > + (*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 > + 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 :-) looks good though I didn't check the lock carefully... > +} > + > +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... > + next->_next = item; > + > + return 0; > +} an awful lot of type handling, even with the mcro helpers this looks heavy. [...] 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 ? > + VIR_FREE(version); > + version = esxUtil_Strdup(updateSet->version); > + > + if (version == NULL) { > + virReportOOMError(conn); > + goto failure; > + } > + > + if (updateSet->filterSet == NULL) { > + continue; > + } > + > + for (propertyFilterUpdate = updateSet->filterSet; > + propertyFilterUpdate != NULL; > + propertyFilterUpdate = propertyFilterUpdate->_next) { > + for (objectUpdate = propertyFilterUpdate->objectSet; > + objectUpdate != NULL; objectUpdate = objectUpdate->_next) { > + for (propertyChange = objectUpdate->changeSet; > + propertyChange != NULL; > + propertyChange = propertyChange->_next) { > + if (STREQ(propertyChange->name, "info.state")) { > + if (propertyChange->op == esxVI_PropertyChangeOp_Add || > + propertyChange->op == esxVI_PropertyChangeOp_Assign) { > + propertyValue = propertyChange->val; > + } else { > + propertyValue = NULL; > + } > + } > + } > + } > + } > + > + if (propertyValue == NULL) { > + continue; > + } > + > + if (esxVI_TaskInfoState_CastFromAnyType(conn, propertyValue, > + &state) < 0) { > + goto failure; > + } > + } > + > + if (esxVI_DestroyPropertyFilter(conn, ctx, propertyFilter) < 0) { > + VIR_DEBUG0("DestroyPropertyFilter failed"); > + } > + > + if (esxVI_TaskInfoState_CastFromAnyType(conn, propertyValue, > + finalState) < 0) { > + goto failure; > + } > + > + cleanup: > + /* Remove values given by the caller from the data structures to prevent > + * them from beeing freed by the call to esxVI_PropertyFilterSpec_Free. > + */ > + if (objectSpec != NULL) { > + objectSpec->obj = NULL; > + } > + > + if (propertySpec != NULL) { > + propertySpec->type = NULL; > + } > + > + esxVI_PropertyFilterSpec_Free(&propertyFilterSpec); > + esxVI_ManagedObjectReference_Free(&propertyFilter); > + VIR_FREE(version); > + esxVI_UpdateSet_Free(&updateSet); > + > + return result; > + > + failure: > + result = -1; > + > + goto cleanup; > +} Interesting, looks like a lot of good work ! > 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 ! I didn't really spot where libxml2 and curl were interfaced, I will have to reread it again, but this lookd fairly good for a first submitted patch, congrats ! thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list