On 08/03/2011 09:00 AM, Matthias Bolte wrote:
Add a generator script to generate the structs and serialization information for OpenWSMAN.
openwsman.h collects workarounds for problems in OpenWSMAN<= 2.2.6. There are also disabled sections that would use ws_serializer_free_mem but can't because it's broken in OpenWSMAN<= 2.2.6. Patches to fix this have been posted upstream. ---
v2: - no changes
po/POTFILES.in | 1 + src/Makefile.am | 25 ++- src/hyperv/.gitignore | 1 +
Can we move this to the top-level .gitignore, instead of adding yet one more file?
src/hyperv/hyperv_private.h | 7 + src/hyperv/hyperv_wmi.c | 684 +++++++++++++++++++++++++++++++++ src/hyperv/hyperv_wmi.h | 121 ++++++ src/hyperv/hyperv_wmi_classes.c | 37 ++ src/hyperv/hyperv_wmi_classes.h | 94 +++++ src/hyperv/hyperv_wmi_generator.input | 294 ++++++++++++++ src/hyperv/hyperv_wmi_generator.py | 309 +++++++++++++++ src/hyperv/openwsman.h | 47 +++
Good - it looks like these are all hand-maintained, and that you left the generated files out of git.
@@ -843,6 +859,12 @@ libvirt_driver_esx_la_DEPENDENCIES = $(ESX_DRIVER_GENERATED) endif
+BUILT_SOURCES += $(HYPERV_DRIVER_GENERATED) + +$(HYPERV_DRIVER_GENERATED): $(srcdir)/hyperv/hyperv_wmi_generator.input \ + $(srcdir)/hyperv/hyperv_wmi_generator.py + $(AM_V_GEN)srcdir=$(srcdir) $(srcdir)/hyperv/hyperv_wmi_generator.py
Don't we need a $(PYTHON) in here, rather than just relying on #!/usr/bin/env python in the .py file header to work?
+++ b/src/hyperv/.gitignore @@ -0,0 +1 @@ +*.generated.*
At the top level, this would be /src/hyperv/*.generated.*
+ + +int +hyperyVerifyResponse(WsManClient *client, WsXmlDocH response, + const char *detail) +{ + int lastError = wsmc_get_last_error(client); + int responseCode = wsmc_get_response_code(client); + WsManFault *fault; + + if (lastError != WS_LASTERR_OK) { + HYPERV_ERROR(VIR_ERR_INTERNAL_ERROR, + _("Transport error during %s: %s (%d)"), + detail, wsman_transport_get_last_error_string(lastError), + lastError); + return -1; + } + + if (responseCode != 200&& responseCode != 400&& responseCode != 500) {
Magic numbers. Should these have names?
+ if (data != NULL) { +#if 0 + /* FIXME: ws_serializer_free_mem is broken in openwsman<= 2.2.6, + * see hypervFreeObject for a detailed explanation. */
Should this be conditional on something like #if WS_SERIALIZER_FREE_MEM_WORKS, instead of #if 0?
+ +#if 0 + /* FIXME: ws_serializer_free_mem is broken in openwsman<= 2.2.6, + * but this is not that critical, because openwsman keeps + * track of all allocations of the deserializer and frees + * them in wsmc_release. So this doesn't result in a real + * memory leak, but just in piling up unused memory until + * the connection is closed. */
Definitely a worthwhile comment.
+ + /* Check return value */ + returnValue = ws_xml_get_xpath_value(response, (char *)"/s:Envelope/s:Body/p:RequestStateChange_OUTPUT/p:ReturnValue");
Nasty that we have to cast away const. Oh well.
+int +hypervMsvmComputerSystemEnabledStateToDomainState + (Msvm_ComputerSystem *computerSystem) +{ + switch (computerSystem->data->EnabledState) { + case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_UNKNOWN: + return VIR_DOMAIN_NOSTATE; + + case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_ENABLED: + return VIR_DOMAIN_RUNNING; + + case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_DISABLED: + return VIR_DOMAIN_SHUTOFF; + + case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_PAUSED: + return VIR_DOMAIN_PAUSED; + + case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_SUSPENDED: + return VIR_DOMAIN_SHUTOFF;
Is suspended really more like shutoff or paused? What's the distinction being used by hyperv for these states?
+ + case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_STARTING: + case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_SNAPSHOTTING: + case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_SAVING: + return VIR_DOMAIN_RUNNING;
Wow - more states than other domains. Wonder if they are worth exposing in the libvirt API eventually; but for now, I'm okay with collapsing them as you have done.
+++ b/src/hyperv/hyperv_wmi_generator.input @@ -0,0 +1,294 @@ +# +# Definitions of WMI classes used as input for the hyperv_wmi_generator.py +# script. +# +# This format is line-based, so end-of-line is important. +# +# +# Class definition: +# +# class<name> +#<type> <name> +# ... +# end +# +# Allowed values for<type> are: boolean, string, datetime, int8, int16, +# int32, int64, uint8, uint16, uint32 and uint64 +# +# The property<name> can be followed by [] to define a dynamic array. +# + + +class Msvm_ComputerSystem + string Caption
I'm trusting that these definitions match a documented layout somewhere. Is there a reference URL worth embedding as a comment here, in case we need to refer back to the original struct definition that this copies from?
+++ b/src/hyperv/hyperv_wmi_generator.py @@ -0,0 +1,309 @@ +#!/usr/bin/env python
My python's weak, but I did glance through all of the generated files, and they look pretty sane, so I'm acking on the basis of the output.
+def main(): + if "srcdir" in os.environ: + input_filename = os.path.join(os.environ["srcdir"], "hyperv/hyperv_wmi_generator.input") + output_dirname = os.path.join(os.environ["srcdir"], "hyperv") + else: + input_filename = os.path.join(os.getcwd(), "hyperv_wmi_generator.input") + output_dirname = os.getcwd()
I'm hoping this plays well with VPATH builds (so far, I've only tested in-tree builds).
+ + # write output files + header.write("/* Generated by hyperv_wmi_generator.py */\n\n\n\n") + source.write("/* Generated by hyperv_wmi_generator.py */\n\n\n\n") + classes_typedef.write("/* Generated by hyperv_wmi_generator.py */\n\n\n\n") + classes_header.write("/* Generated by hyperv_wmi_generator.py */\n\n\n\n") + classes_source.write("/* Generated by hyperv_wmi_generator.py */\n\n\n\n")
Is it worth factoring that message into a reusable constant rather than repeating it five times?
+#ifndef __OPENWSMAN_H__ +# define __OPENWSMAN_H__ + +/* Workaround openwsman<= 2.2.6 unconditionally defining optarg. Just pretend + * that u/os.h was already included. Need to explicitly include time.h because + * wsman-xml-serializer.h needs it and u/os.h would have included it. */ +# include<time.h> +# define _LIBU_OS_H_ +# include<wsman-api.h>
Oh the gross things we have to do. The comment is worth its weight in gold. I pointed out a couple of nits, but nothing jumped out at me as a showstopper. ACK, and I'm happy if you push without posting a v3. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org