[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



2011/8/20 Eric Blake <eblake redhat com>:
> 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

>> +
>> +
>> +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?

As there are used only once here, I'll add a comment, instead of an
enum for now.

>> +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?

Suspended maps to libvirt's managed save.

>> +++ 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?

They are all documented in MSDN, I'll add a link to it.

>> +++ 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).

This generator is based on the ESX one, therefore it should work in
VPATH builds, otherwise you should have seen problems with the ESX
generator in VPATH builds in the past.

Also the else case with getcwd will only be executed when the
generator is run manually, as the Makefile always passes srcdir to it.

>> +#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'm currently in the process of getting some patches into openwsman
upstream to fix those problems.

> 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.

Here's the interdiff I'm going to apply, before I push the whole series.

-- 
Matthias Bolte
http://photron.blogspot.com
diff --git a/.gitignore b/.gitignore
index 39ecd6d..1e8d5ab 100644
--- a/.gitignore
+++ b/.gitignore
@@ -54,6 +54,7 @@
 /proxy/
 /python/generator.py.stamp
 /sc_*
+/src/hyperv/*.generated.*
 /src/libvirt_iohelper
 /src/locking/qemu-sanlock.conf
 /src/remote/*_client_bodies.h
diff --git a/src/Makefile.am b/src/Makefile.am
index 796e6b8..c8605fc 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -864,7 +864,7 @@ 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
+	$(AM_V_GEN)srcdir=$(srcdir) $(PYTHON) $(srcdir)/hyperv/hyperv_wmi_generator.py
 
 if WITH_HYPERV
 if WITH_DRIVER_MODULES
diff --git a/src/hyperv/.gitignore b/src/hyperv/.gitignore
deleted file mode 100644
index 29e1d48..0000000
--- a/src/hyperv/.gitignore
+++ /dev/null
@@ -1 +0,0 @@
-*.generated.*
diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
index dcfc3f9..517c8ea 100644
--- a/src/hyperv/hyperv_wmi.c
+++ b/src/hyperv/hyperv_wmi.c
@@ -35,6 +35,8 @@
 #include "hyperv_private.h"
 #include "hyperv_wmi.h"
 
+#define WS_SERIALIZER_FREE_MEM_WORKS 0
+
 #define ROOT_CIMV2 \
     "http://schemas.microsoft.com/wbem/wsman/1/wmi/root/cimv2/*";
 
@@ -61,6 +63,8 @@ hyperyVerifyResponse(WsManClient *client, WsXmlDocH response,
         return -1;
     }
 
+    /* Check the HTTP response code and report an error if it's not 200 (OK),
+     * 400 (Bad Request) or 500 (Internal Server Error) */
     if (responseCode != 200 && responseCode != 400 && responseCode != 500) {
         HYPERV_ERROR(VIR_ERR_INTERNAL_ERROR,
                      _("Unexpected HTTP response during %s: %d"),
@@ -246,7 +250,7 @@ hypervEnumAndPull(hypervPrivate *priv, virBufferPtr query, const char *root,
     }
 
     if (data != NULL) {
-#if 0
+#if WS_SERIALIZER_FREE_MEM_WORKS
         /* FIXME: ws_serializer_free_mem is broken in openwsman <= 2.2.6,
          *        see hypervFreeObject for a detailed explanation. */
         if (ws_serializer_free_mem(serializerContext, data,
@@ -268,7 +272,7 @@ void
 hypervFreeObject(hypervPrivate *priv ATTRIBUTE_UNUSED, hypervObject *object)
 {
     hypervObject *next;
-#if 0
+#if WS_SERIALIZER_FREE_MEM_WORKS
     WsSerializerContextH serializerContext;
 #endif
 
@@ -276,14 +280,14 @@ hypervFreeObject(hypervPrivate *priv ATTRIBUTE_UNUSED, hypervObject *object)
         return;
     }
 
-#if 0
+#if WS_SERIALIZER_FREE_MEM_WORKS
     serializerContext = wsmc_get_serialization_context(priv->client);
 #endif
 
     while (object != NULL) {
         next = object->next;
 
-#if 0
+#if WS_SERIALIZER_FREE_MEM_WORKS
         /* 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
@@ -551,7 +555,7 @@ hypervMsvmComputerSystemEnabledStateToDomainState
       case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_PAUSED:
         return VIR_DOMAIN_PAUSED;
 
-      case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_SUSPENDED:
+      case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_SUSPENDED: /* managed save */
         return VIR_DOMAIN_SHUTOFF;
 
       case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_STARTING:
@@ -592,7 +596,7 @@ hypervIsMsvmComputerSystemActive(Msvm_ComputerSystem *computerSystem,
       case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_PAUSED:
         return true;
 
-      case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_SUSPENDED:
+      case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_SUSPENDED: /* managed save */
         return false;
 
       case MSVM_COMPUTERSYSTEM_ENABLEDSTATE_STARTING:
diff --git a/src/hyperv/hyperv_wmi_generator.input b/src/hyperv/hyperv_wmi_generator.input
index da874ac..97f9dff 100644
--- a/src/hyperv/hyperv_wmi_generator.input
+++ b/src/hyperv/hyperv_wmi_generator.input
@@ -17,6 +17,10 @@
 #
 # The property <name> can be followed by [] to define a dynamic array.
 #
+#
+# Based on MSDN Hyper-V WMI Classes:
+# http://msdn.microsoft.com/en-us/library/cc136986%28v=vs.85%29.aspx
+#
 
 
 class Msvm_ComputerSystem
diff --git a/src/hyperv/hyperv_wmi_generator.py b/src/hyperv/hyperv_wmi_generator.py
index 077c3a0..93f5ac2 100755
--- a/src/hyperv/hyperv_wmi_generator.py
+++ b/src/hyperv/hyperv_wmi_generator.py
@@ -287,11 +287,13 @@ def main():
                 block.append((number, line))
 
     # 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")
+    notice = "/* Generated by hyperv_wmi_generator.py */\n\n\n\n"
+
+    header.write(notice)
+    source.write(notice)
+    classes_typedef.write(notice)
+    classes_header.write(notice)
+    classes_source.write(notice)
 
     names = classes_by_name.keys()
     names.sort()

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