From 4074117c5540dabb288989517695a188f4c692d9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Klaus=20K=C3=A4mpf?= <kkaempf@suse.de>
Date: Tue, 13 Jan 2015 11:53:43 +0100
Subject: [PATCH 5/6] Store client option properties as list to keep order

This fixes issue #49

The invoke action requires method parameters as WS-Sequence, keeping
order. The previous hash implementation did not honor order.
(Actually, this is a flaw in the WS-Management standard since method
parameter are passed by name, thus the receiver can regenerate the
correct order)

This patch changes options->properties from hash to list.

As a nice side effect, this also cleans up commit 4916a94, removing
the "<epr>" key prefix.

Ruby: Move ClientOptions#properties= (setter) to openwsman.rb

Implement ClientOptions#properties (getter) with new list_t type of
options->properties.
---
 bindings/ruby/openwsman/openwsman.rb |   8 ++
 bindings/wsman-client_opt.i          |  31 ++---
 include/u/list.h                     |   2 +-
 include/wsman-client-api.h           |   8 +-
 src/lib/wsman-client.c               | 235 +++++++++++++++++++----------------
 5 files changed, 160 insertions(+), 124 deletions(-)

diff --git a/bindings/ruby/openwsman/openwsman.rb b/bindings/ruby/openwsman/openwsman.rb
index c60379f98182..ed677b206ccf 100644
--- a/bindings/ruby/openwsman/openwsman.rb
+++ b/bindings/ruby/openwsman/openwsman.rb
@@ -38,6 +38,14 @@ require 'openwsman/xmldoc'
 # response and dig down through its XmlNode and XmlAttr objects.
 
 module Openwsman
+  class ClientOption
+    # assign hash to properties
+    def properties= value
+      value.each do |k,v|
+        self.add_property k.to_s, v.to_s
+      end
+    end
+  end
   class Transport
     # called when authentication credentials missing or wrong
     def Transport.auth_request_callback client, auth_type
diff --git a/bindings/wsman-client_opt.i b/bindings/wsman-client_opt.i
index a200dd27477d..cea723295366 100644
--- a/bindings/wsman-client_opt.i
+++ b/bindings/wsman-client_opt.i
@@ -474,20 +474,6 @@ typedef struct {} client_opt_t;
 #endif
   
 #if defined(SWIGRUBY)
-  %rename( "properties=" ) set_properties(VALUE hash);
-  /*
-   * Set properties from Hash
-   * * Input parameters to 'invoke'd methods are represented as ClientOption properties
-   *
-   * call-seq:
-   *   options.properties = { "Key" => "Value", ...}
-   *
-   */
-  void set_properties(VALUE hash)
-  {
-    $self->properties = value2hash(NULL, hash, 0);
-  }
-
   %rename( "properties" ) get_properties(void);
   /*
    * Get properties as Hash
@@ -499,7 +485,22 @@ typedef struct {} client_opt_t;
    */
   VALUE get_properties(void)
   {
-    return hash2value($self->properties);
+    VALUE v = Qnil;
+    if (!list_isempty($self->properties)) {
+      v = rb_hash_new();
+      lnode_t *node = list_first($self->properties);
+      while (node) {
+        client_property_t *property = (client_property_t *)node->list_data;
+        if (property->value.type == 0) {
+	  rb_hash_aset( v, makestring(property->key), makestring(property->value.entry.text));
+        }
+        else {
+          rb_hash_aset( v, makestring(property->key), makestring(epr_to_string(property->value.entry.eprp)));
+        }
+        node = list_next($self->properties, node);
+      }
+    }
+    return v;
   }
 #endif
 
diff --git a/include/u/list.h b/include/u/list.h
index 69c3ae10845f..c2f609683d98 100644
--- a/include/u/list.h
+++ b/include/u/list.h
@@ -162,7 +162,7 @@ int ow_list_verify(list_t *);
 #if defined(LIST_IMPLEMENTATION) || !defined(KAZLIB_OPAQUE_DEBUG)
 #define lnode_pool_isempty(P)	((P)->list_free == 0)
 #define list_count(L)		((L)->list_nodecount)
-#define list_isempty(L)		((L)->list_nodecount == 0)
+#define list_isempty(L)		((L == NULL) || ((L)->list_nodecount == 0))
 #define list_isfull(L)		(LIST_SFX_CHECK(L)->list_nodecount == (L)->list_maxcount)
 #define list_next(L, N)		(LIST_SFX_CHECK(N)->list_next == &(L)->list_nilnode ? NULL : (N)->list_next)
 #define list_prev(L, N)		(LIST_SFX_CHECK(N)->list_prev == &(L)->list_nilnode ? NULL : (N)->list_prev)
diff --git a/include/wsman-client-api.h b/include/wsman-client-api.h
index 4b38f0fa5d8f..4ed1a7b0fa2f 100644
--- a/include/wsman-client-api.h
+++ b/include/wsman-client-api.h
@@ -46,6 +46,7 @@ extern "C" {
 #include "wsman-xml-serializer.h"
 #include "wsman-epr.h"
 #include "wsman-filter.h"
+#include "u/list.h"
 
 /**
  * @defgroup Client Client
@@ -188,14 +189,17 @@ typedef enum {
 		float heartbeat_interval;
 		float expires;
 		hash_t *selectors;
-		hash_t *properties;
+		list_t *properties; /* keep properties sorted */
 		unsigned int timeout;
 		unsigned int max_envelope_size;
 		unsigned int max_elements;
 		hash_t *options; /* for WSM_OPTION_SET */
 	} client_opt_t;
 
-
+        typedef struct {
+          char *key;
+          selector_entry value; /* either char* or epr_t */
+        } client_property_t;
 
 	struct _WsManFault {
 		const char *code;
diff --git a/src/lib/wsman-client.c b/src/lib/wsman-client.c
index 82eac04faeca..6f4bb5f849db 100644
--- a/src/lib/wsman-client.c
+++ b/src/lib/wsman-client.c
@@ -49,17 +49,6 @@
 #include "wsman-faults.h"
 #include "wsman-client.h"
 
-/*
- * Since the options->properties can only handle pointers,
- * we need to flag somehow that the value is not a char pointer
- * but a epr_t pointer.
- * We do this by prefixing the name with "<epr>". Since neither
- * '<' nor '>' are valid property names, this should not conflict
- * with normal options.
- */
-#define EPR_KEY_PREFIX "<epr>"
-#define EPR_KEY_PREFIX_LEN 5
-
 static hash_t *
 get_selectors_from_uri(const char *resource_uri)
 {
@@ -328,6 +317,29 @@ wsmc_options_init(void)
 	return op;
 }
 
+static void
+_wsmc_properties_destroy(list_t *properties)
+{
+  while (!list_isempty(properties)) {
+    lnode_t *node;
+    client_property_t *prop;
+    
+    node = list_del_last(properties);
+    if (!node)
+      break;
+    prop = (client_property_t *)node->list_data;
+    lnode_destroy(node);
+    u_free(prop->key);
+    if (prop->value.type == 0) {
+      u_free(prop->value.entry.text);
+    }
+    else {
+      epr_destroy(prop->value.entry.eprp);
+    }
+    u_free(prop);
+  }
+  list_destroy(properties);
+}
 
 void
 wsmc_options_destroy(client_opt_t * op)
@@ -338,10 +350,7 @@ wsmc_options_destroy(client_opt_t * op)
 	if (op->selectors) {
 		hash_free(op->selectors);
 	}
-	if (op->properties) {
-		hash_free(op->properties);
-	}
-
+        _wsmc_properties_destroy(op->properties);
 	u_free(op->fragment);
 	u_free(op->cim_ns);
 	u_free(op->delivery_uri);
@@ -374,58 +383,87 @@ wsmc_clear_action_option(client_opt_t * options, unsigned int flag)
 
 
 /*
- * free a properties hash entry
- * take care of epr_t vs 'normal' hash data
+ * compare two property list entries by key
+ */
+static int
+_property_key_compare(const void *node1, const char *key2)
+{
+  const char *key1 = ((client_property_t *)node1)->key;
+  return strcmp(key1, key2);
+}
+
+/*
+ * (static function)
+ * Add property to options
+ * either as char* (string set, epr == NULL)
+ * or as epr_t (string == NULL, epr set)
  */
+
 static void
-properties_hnode_free(hnode_t *node, void *context)
+_wsmc_add_property(client_opt_t *options,
+		const char *key,
+		const char *string,
+		const epr_t *epr)
 {
-  const char *key = node->hash_key;
-  if (*key == '<'
-      && strncmp(key, EPR_KEY_PREFIX, EPR_KEY_PREFIX_LEN) == 0) {
-    epr_destroy((epr_t *)node->hash_data);
+  client_property_t *prop;
+  lnode_t *lnode;
+  if ((string != NULL) && (epr != NULL)) {
+    error("Ambiguous call to add_property");
+    return;
+  }
+  if (key == NULL) {
+    error("Can't add property with NULL key");
+    return;
+  }
+  if (options->properties == NULL) {
+    options->properties = list_create(LISTCOUNT_T_MAX);
   }
-  else {
-    u_free((void *)node->hash_data);
+  if (list_find(options->properties, key, _property_key_compare)) {
+    error("duplicate key not added to properties");
+    return;
   }
-  u_free((void *)node->hash_key);
-  free(node);
+  prop = u_malloc(sizeof(client_property_t));
+  if (!prop) {
+    error("No memory for property");
+    return;
+  }
+  prop->key = u_strdup(key);
+  if (string != NULL) {
+    prop->value.type = 0;
+    prop->value.entry.text = u_strdup(string);
+  } else if (epr != NULL) {
+    prop->value.type = 1;
+    prop->value.entry.eprp = epr_copy(epr);
+  } else {
+    error("Can't add NULL as property value");
+    return;
+  }
+  lnode = lnode_create(prop);
+  if (!lnode) {
+    error("No memory for property node");
+    return;
+  }
+  list_append(options->properties, lnode);
 }
 
 
+/*
+ * add a char* as property
+ *
+ */
+
 void
 wsmc_add_property(client_opt_t * options,
 		const char *key,
 		const char *value)
 {
-	if (options->properties == NULL) {
-		options->properties = hash_create3(HASHCOUNT_T_MAX, 0, 0);
-                hash_set_allocator(options->properties, (hnode_alloc_t)NULL,
-                                   properties_hnode_free, NULL);
-        }
-	if (!hash_lookup(options->properties, key)) {
-          char *k = u_strdup(key);
-          char *v = u_strdup(value);
-		if (!hash_alloc_insert(options->properties, k, v)) {
-			error("hash_alloc_insert failed");
-                  u_free(v);
-                  u_free(k);
-		}
-	} else {
-		error("duplicate not added to hash");
-	}
+  _wsmc_add_property(options, key, value, NULL);
 }
 
 
 /*
  * add an EndpointReference as property
  *
- * Since the options->properties can only handle pointers,
- * we need to flag somehow that the value is not a char pointer
- * but a epr_t pointer.
- * We do this by prefixing the name with "<epr>". Since neither
- * '<' nor '>' are valid property names, this should not conflict
- * with normal options.
  */
 
 void
@@ -433,30 +472,7 @@ wsmc_add_property_epr(client_opt_t * options,
 		const char *key,
 		const epr_t *value)
 {
-  if (options->properties == NULL) {
-    options->properties = hash_create3(HASHCOUNT_T_MAX, 0, 0);
-    hash_set_allocator(options->properties, (hnode_alloc_t)NULL,
-                       properties_hnode_free, NULL);
-  }
-  if (!hash_lookup(options->properties, key)) { /* does 'key' exist */
-    char *epr_key = alloca(EPR_KEY_PREFIX_LEN + strlen(key) + 1);
-    sprintf(epr_key, "%s%s", EPR_KEY_PREFIX, key);
-    if (!hash_lookup(options->properties, epr_key)) { /* does '<epr>key' exist ? */
-      char *k = u_strdup(epr_key);
-      epr_t *v = epr_copy(value);
-      if (!hash_alloc_insert(options->properties, k, (char *)v)) {
-        error("hash_alloc_insert failed");
-        epr_destroy(v);
-        u_free(k);
-      }
-    }
-    else {
-      error("duplicate not added to hash");
-    }
-  }
-  else {
-    error("duplicate not added to hash");
-  }
+  _wsmc_add_property(options, key, NULL, value);
 }
 
 /*
@@ -526,7 +542,19 @@ wsmc_add_prop_from_str(client_opt_t * options,
 
         query = u_parse_query(query_string);
 	if (query) {
-		options->properties = query;
+          /* convert query hash to property list */
+          hscan_t hs;
+          hnode_t *hn;
+          _wsmc_properties_destroy(options->properties);
+          options->properties = NULL;
+          hash_scan_begin(&hs, query);
+          while ((hn = hash_scan_next(&hs))) {
+            _wsmc_add_property(options,
+                               (char *)hnode_getkey(hn),
+                               (char *)hnode_get(hn),
+                               NULL);
+          }
+          hash_free(query);
 	}
 }
 
@@ -880,8 +908,6 @@ wsmc_set_put_prop(WsXmlDocH get_response,
 {
 	WsXmlNodeH      resource_node;
 	char           *ns_uri;
-	hscan_t         hs;
-	hnode_t        *hn;
 	WsXmlNodeH      get_body = ws_xml_get_soap_body(get_response);
 	WsXmlNodeH      put_body = ws_xml_get_soap_body(put_request);
 
@@ -889,14 +915,20 @@ wsmc_set_put_prop(WsXmlDocH get_response,
 	resource_node = ws_xml_get_child(put_body, 0, NULL, NULL);
 	ns_uri = ws_xml_get_node_name_ns_uri(resource_node);
 
-	if (!options->properties) {
-		return;
-	}
-	hash_scan_begin(&hs, options->properties);
-	while ((hn = hash_scan_next(&hs))) {
-		WsXmlNodeH      n = ws_xml_get_child(resource_node, 0,
-				ns_uri, (char *) hnode_getkey(hn));
-		ws_xml_set_node_text(n, (char *) hnode_get(hn));
+	if (!list_isempty(options->properties)) {
+          lnode_t *node = list_first(options->properties);
+          while (node) {
+            client_property_t *property = (client_property_t *)node->list_data;
+            WsXmlNodeH n = ws_xml_get_child(resource_node, 0,
+                                            ns_uri, property->key);
+            if (property->value.type == 0) {
+              ws_xml_set_node_text(n, property->value.entry.text);
+            }
+            else {
+              epr_serialize(n, ns_uri, property->key, property->value.entry.eprp, 1);
+            }
+            node = list_next(options->properties, node);
+          }
 	}
 }
 
@@ -1388,35 +1420,26 @@ wsmc_action_invoke(WsManClient * cl,
 
 	body = ws_xml_get_soap_body(request);
 
-	if ((!options->properties ||
-                    hash_count(options->properties) == 0) &&
+	if (list_isempty(options->properties) &&
                 data != NULL) {
 
 		WsXmlNodeH n = ws_xml_get_doc_root(data);
 		ws_xml_duplicate_tree(ws_xml_get_soap_body(request), n);
-        } else if (options->properties &&
-                hash_count(options->properties) > 0 ) {
+        } else if (!list_isempty(options->properties)) {
             if (method) {
-                WsXmlNodeH node = ws_xml_add_empty_child_format(body,
-                        (char *)resource_uri, "%s_INPUT", method);
-                hash_scan_begin(&hs, options->properties);
-                while ((hn = hash_scan_next(&hs))) {
-                  const char *key = hnode_getkey(hn);
-                  if (*key == '<'
-                      && strncmp(key, EPR_KEY_PREFIX, EPR_KEY_PREFIX_LEN) == 0) {
-                    epr_t *val;
-                    key = key + EPR_KEY_PREFIX_LEN;
-                    val = (epr_t *)hnode_get(hn);
-                    epr_serialize(node, (char *)resource_uri, key, val, 1); /* add epr as embedded */
-                  }
-                  else {
-                    const char *val = hnode_get(hn);
-                    ws_xml_add_child(node,
-                            (char *)resource_uri,
-                            (char *) hnode_getkey(hn),
-                            (char *) hnode_get(hn));
-                  }
+              WsXmlNodeH xnode = ws_xml_add_empty_child_format(body,
+                                  (char *)resource_uri, "%s_INPUT", method);
+              lnode_t *lnode = list_first(options->properties);
+              while (lnode) {
+                client_property_t *property = (client_property_t *)lnode->list_data;
+                if (property->value.type == 0) {
+                  ws_xml_add_child(xnode, (char *)resource_uri, (char *)property->key, (char *)property->value.entry.text);
+                }
+                else {
+                  epr_serialize(xnode, (char *)resource_uri, property->key, property->value.entry.eprp, 1);
                 }
+                lnode = list_next(options->properties, lnode);
+              }
             }
         } else if (!strchr(method, '/')) { /* non-custom method without parameters */
             ws_xml_add_empty_child_format(body,
-- 
2.1.4

