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

Re: [libvirt] [PATCHv2 3/5] hyperv: Add OpenWSMAN based client for the Hyper-V WMI API



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


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