[libvirt] [PATCHv2 3/5] hyperv: Add OpenWSMAN based client for the Hyper-V WMI API
Eric Blake
eblake at redhat.com
Fri Aug 19 23:03:11 UTC 2011
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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list