More fixes and testing results.

[Suresh Sundriyal-2]

I see some more problems in a couple of files. I haven't gotten a chance
to fix them yet but I shall describe some of them and one memory
leak/segfault patch that I am sending along.

First the patch:-

wsman-cimindication-processor.c
===============================

In create_notification_entity(), we allocate memory for notificationinfo
and then we dereference it without checking for NULL. Secondly, we check
for instance being NULL and return without freeing notificationinfo.
There is also the case where notificationinfo is dereferenced, making it
all the more dangerous. The patch fixes this problem.


Now some stuff that I can see is wrong but I haven't gotten around to
fixing yet but thought to mention it just in case I forget:-

Memory consumption:
===================

In case of stress test involving lots of clients doing enumeration at
the same time, the memory consumption starts growing at an alarming
rate. This is understandable, since Openwsman has to cache the
enumeration results till the client actually pulls them or they expire.
This might not be a problem under normal systems where the memory is not
at a premium but on embedded systems, this will definitely cause problems.

Actually, let me take that back. It will cause problems on normal
systems as well. Somewhere between 1.5.0 and 2.0.0 we got rid of the
max_thread parameter. Currently, I believe each listener thread is setup
to handle 20 clients. In case there is a 21st simultaneous client,
another listener thread is spawned.

Now if one were to look at compat_unix.h, we see this:


#define InitializeCriticalSection(x)    /* FIXME UNIX version is not MT
safe */
#define EnterCriticalSection(x)
#define LeaveCriticalSection(x)


Seems like mutex locking code is def'ed to nothing on unix systems. If
that is the case then the moment the system spawns another listener
thread, there will be problems and since you cannot contain the number
of threads spawned, you have no control over another thread being spawned.

I tried to fix compat_unix.h but that just leads to deadlocks that
Helgrind can't seem to identify.

This seems like a limitation of the spec and the way Openwsman is
engineered. I have a working fix for this albeit in the Openwsman 2.1.0
codebase. Let me try and port it over to 2.2.0 and I shall send the fix
along. Might take me a day or two to get to this.

The fix is not conventional but it is config file driven so it can be
turned on or off on the fly.

wsman-server-api.c
==================

I can't figure out this file. There is some code that is used by
wsman-server.c but there is also some code that seems to be just cruft
floating around. It seems like the method
wsman_server_create_config() is not used anymore, neither is
wsman_server_get_response() and same is the case with
wsman_receive_cim_indication and wsman_server_get_subscription_repos().

Is this just dead code or intentional?

wsman-server.c:
===============

In method wsman_init_subscription_repository(), if soap is NULL we still
try to return soap->subscriptionOpSet. That seems like a segfault
waiting to happen. Same is the case with wsman_init_event_pool(). I
don't know what to return in case of NULL.

wsmand.c:
=========
The return value for chdir() is not checked in method daemonize(), this
is causing compilation failures since the compiler flags the following
warning. (At least, I think it is because of the unchecked value).

cc1: warnings being treated as errors
wsmand.c: In function ‘daemonize’:
wsmand.c:220: error: ignoring return value of ‘chdir’, declared with
attribute warn_unused_result

sfcc-interface.c:
=================
In cim_epr_to_objectpath(), we can potentially pass a NULL epr to
wsman_epr_selector_cb(), which tries to dereference it without any checks.

In xml2interface(), we dereference objectpath, classname without
checking for NULL.

in path2xml, we dereference keyname, classname and namespace without
testing for NULL but free them after a check. Could be or could not be a
problem. There are other places too, where I can see the same problem
within the same file.

The main reason that this bothers me is, we check to see if they are
NULL or not before we free them, which makes me suspicious that they
could be NULL but we don't really check for NULL before dereferencing
them in other places. If that is not the case, then we can ignore this
comment.

wsman_test_stubs.c
==================

Are these just test cases? In which case we can forget about them.
Otherwise, in WsManTest_EventPoll_EP(), line 117, there needs to be
deallocation of notificationinfo and notificationinfo->EventAction
before returning.


GSSAPI support:
===============
I'm not an expert on GSSAPI support but this does not seem to be working
for me. From whatever I read up from the SUN tutorial(there was only one
half-decent tutorial I could find on the subject), it seems that the
server is supposed to keep up the challenge-response till it finds the
correct principal. However, the code picks up the first principal that
it finds in the file and fails if it is not the correct one.

Maybe someone more familiar with the GSSAPI should take a closer look at
the code.

In case it's my setup that is screwed up, feel free to deliver a swift
kick in the behind. :)



Enumeration with expire time:
=============================

This could just be a problem at my end, since I've been playing around
with the code so much that my local client is completely messed up at
the moment (I tend to work off of 2.1.0 code and then patching it with
the changes) but it seems like if I send an enumeration request with an
expire time, the server does not honor the expire time and immidiately
deletes the results. If anyone could chime in and confirm whether this
is a problem or not before I start investigating this, it would be
appreciated.

--
Regards,
Suresh










Index: src/lib/wsman-cimindication-processor.c
===================================================================
--- src/lib/wsman-cimindication-processor.c (revision 3254)
+++ src/lib/wsman-cimindication-processor.c (working copy)
@@ -128,12 +128,24 @@
  WsXmlNodeH indicationnode = NULL;
  WsNotificationInfoH notificationinfo = NULL;
  notificationinfo = u_zalloc(sizeof(*notificationinfo));
+ if(!notificationinfo){
+ return NULL;
+ }
  WsXmlNodeH instance = ws_xml_get_child(node, 0, NULL, CIMXML_EXPMETHODCALL);
- if(instance == NULL) return NULL;
+ if(instance == NULL){
+ u_free(notificationinfo);
+ return NULL;
+ }
  instance = ws_xml_get_child(instance, 0, NULL, CIMXML_EXPPARAMVALUE);
- if(instance == NULL) return NULL;
+ if(instance == NULL){
+ u_free(notificationinfo);
+ return NULL;
+ }
  instance = ws_xml_get_child(instance, 0, NULL, CIMXML_INSTANCE);
- if(instance == NULL) return NULL;
+ if(instance == NULL){
+ u_free(notificationinfo);
+ return NULL;
+ }
  WsXmlAttrH attr = ws_xml_find_node_attr(instance, NULL, CIMXML_CLASSNAME);
  if(attr) {
  classname = ws_xml_get_attr_value(attr);

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Openwsman-devel mailing list
Openwsman-devel@...
https://lists.sourceforge.net/lists/listinfo/openwsman-devel

[Suresh Sundriyal-2]

Suresh Sundriyal wrote:

> Enumeration with expire time:
> =============================
>
> This could just be a problem at my end, since I've been playing around
> with the code so much that my local client is completely messed up at
> the moment (I tend to work off of 2.1.0 code and then patching it with
> the changes) but it seems like if I send an enumeration request with an
> expire time, the server does not honor the expire time and immidiately
> deletes the results. If anyone could chime in and confirm whether this
> is a problem or not before I start investigating this, it would be
> appreciated.
>

I'm sending along a patch for the problem above. It seems that when we
calculate the timestamp for the enumeration results in
wsman_set_expiretime (wsman-soap-envelope.c), we set it to:

*expire = tv.tv_sec + timeout;

This is basically (epoch seconds + offset).

When we check to see whether the enumeration has expired or not in
wsman_get_expired_enuminfos (wsman-soap.c), we test it against:

mytime = tv.tv_sec * 1000 + tv.tv_usec / 1000;

This is epoch seconds converted to microseconds + epoch microseconds
converted to seconds(?!!). This pretty much always ensures that mytime
will always be greater than expire_time. Thus the enumeration results
expire as soon as they are created.

To fix this, I changed mytime to :

mytime = tv.tv_sec

This seems to work fine. I could not correct the code in
wsman_get_expired_enuminfos, since that code is also used by
subscription_expiration and changing the code there makes subscriptions
act weird.

Tested the code with enumeration intrinsic with expiration time (PT45S,
did not get a chance to test it with datetime format but even that code
seems to convert everything to sec, so I think it should be OK). Also
tested the subscriptions to make sure they were working correctly.

--
Suresh

Index: src/lib/wsman-soap.c
===================================================================
--- src/lib/wsman-soap.c (revision 3254)
+++ src/lib/wsman-soap.c (working copy)
@@ -1283,7 +1283,7 @@
  return NULL;
  }
  gettimeofday(&tv, NULL);
- mytime = tv.tv_sec * 1000 + tv.tv_usec / 1000;
+ mytime = tv.tv_sec;
  u_lock(cntx->soap);
  if (hash_isempty(cntx->enuminfos)) {
  u_unlock(cntx->soap);

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Openwsman-devel mailing list
Openwsman-devel@...
https://lists.sourceforge.net/lists/listinfo/openwsman-devel

[Suresh Sundriyal-2]

A couple of days ago, I had sent in an email with the following observation:

>Memory consumption:
>===================
>
>In case of stress test involving lots of clients doing enumeration at
>the same time, the memory consumption starts growing at an alarming
>rate. This is understandable, since Openwsman has to cache the
>enumeration results till the client actually pulls them or they expire.
>This might not be a problem under normal systems where the memory is not
>at a premium but on embedded systems, this will definitely cause problems.
>
>Actually, let me take that back. It will cause problems on normal
>systems as well. Somewhere between 1.5.0 and 2.0.0 we got rid of the
>max_thread parameter. Currently, I believe each listener thread is setup
>to handle 20 clients. In case there is a 21st simultaneous client,
>another listener thread is spawned.
>
>Now if one were to look at compat_unix.h, we see this:
>
>
>#define InitializeCriticalSection(x)    /* FIXME UNIX version is not MT
>safe */
>#define EnterCriticalSection(x)
>#define LeaveCriticalSection(x)
>
>
>Seems like mutex locking code is def'ed to nothing on unix systems. If
>that is the case then the moment the system spawns another listener
>thread, there will be problems and since you cannot contain the number
>of threads spawned, you have no control over another thread being spawned.
>
>I tried to fix compat_unix.h but that just leads to deadlocks that
>Helgrind can't seem to identify.
>
>This seems like a limitation of the spec and the way Openwsman is
>engineered. I have a working fix for this albeit in the Openwsman 2.1.0
>codebase. Let me try and port it over to 2.2.0 and I shall send the fix
>along. Might take me a day or two to get to this.
>
>The fix is not conventional but it is config file driven so it can be
>turned on or off on the fly.

I'm sending in the patch to fix this problem.

The patch reintroduces the max_thread config file parameter and introduces
two new parameters:

* max_connections_per_thread
* thread_stack_size

Explanation for the patch:
================

1. wsmand-daemon.c & wsmand-daemon.h:

The changes here are to read in the config file parameters.

2. wsmand-listener.c:

As I had mentioned earlier, the multithreading capabilities in Openwsman seem
to be busted, since compat_unix.c defines all the locking/unlocking primitives to
blanks. Since we got rid of the max_thread parameter this makes it quite dangerous
since if we have a thread serving 20 clients and a 21st client sends in a request
another thread is spawned which is dangerous without the concurrency primitives
in place.

The change in this file first of all reintroduces the max_thread parameter. I usually
keep this to 1, since I don't want more than 1 listener thread running if there is no
concurrency support. It also uses the max_connections_per_thread parameter to
optimize how many clients you wish to serve by that thread. If more than max_thread
* max_connections_per_thread client comes in it is blocked till one of the threads is
free. I also modified find_not_busy thread to count how many threads are active.

After some profiling it was also a pleasant surprise that Openwsman does not
utilize a lot of stack space. Since each thread by default is given about 2Mb if I
am not wrong, which seems wasteful, so I introduced the thread_stack_size to
optimize the thread stack space. This parameter takes in stack size in bytes and
is used as the input to pthread_attr_setstacksize(). I've tested with as low as
256k(262144 bytes) and Openwsman works just fine even under stress. Your
mileage may vary depending on the OS.

When max_thread is set to 0, Openwsman behaves as it used to. The only
caveat is when max_thread is non-zero, max_connections_per_thread cannot
be zero, since that would effectively mean that the threads cannot serve any
clients.

 In the case of thread_stack_size set to 0, Openwsman again behaves like it used
 to. So set both these parameters to zero to turn off these optimizations.

3. wsman-soap.c:

This is to tackle the problem of Openwsman using lots of memory when there
are too many clients doing Enumerations. Enumerations require Openwsman
to keep the results in memory till they expire or till they are pulled by the client.
On an embedded system, where the memory constraints are stringent, this
could cause problems.

To get around this problem, if there are already max_threads*max_connections_
per_thread number of enumeration results in memory and another enumeration
request is encountered,  I return an error saying "Insufficient Resource.......", till
one of the enumerations is pulled or one of them expires.
max_threads*max_connections_per_thread means that all the available
listeners have been utilized to do enumerations, therefore there could be that
many clients making pull requests. I do not block the pull operations or any other
operations since those do not require Openwsman to cache the results.

This optimization can again be turned off by setting max_threads to 0 which is
the default.

4. wsmand.c:
At least on Ubuntu boxes, the compiler keeps throwing the following error at me:

cc1: warnings being treated as errors
wsmand.c: In function ‘daemonize’:
wsmand.c:220: error: ignoring return value of ‘chdir’, declared with
attribute warn_unused_result

I could get around the problem by not passing in the -Werror flag to gcc but that
prevents the compiler from finding other valid problems.

To fix this I store the return value from chdir and put in an assert in case of failure
just like we do if opening file descriptors fail later on in the code.

Testing:
=====

Before these changes, the idle Openwsman memory usage was about 8MB and with
10 clients doing simultaneous enumearations would climb up to about 50-60MB. With
the following parameters in place:

max_threads=1
max_connections_per_thread=8
thread_stack_size=262144

the idle memory usage was about 4MB and with 10 clients doing enumearation, it went
up to about 19MB.

NOTE:
-------
I could get the idle usage down a lot more with setting the main stack even lower than
the thread stack using `ulimit -s` in the wsman init file. I could set it down to about 64K
and Openwsman worked just fine.

Your mileage may again vary but feel free to play around with that if you are on a memory
starved system.

--
Suresh
[memusage.diff]

---

Index: src/lib/wsman-soap.c
===================================================================
--- src/lib/wsman-soap.c (revision 3254)
+++ src/lib/wsman-soap.c (working copy)
@@ -922,7 +922,34 @@
  return _op->maxsize;
 }
 
+static
+unsigned long get_total_enum_context(WsContextH cntx){
+        hscan_t hs;
+        u_lock(cntx->soap);
+        hash_scan_begin(&hs, cntx->enuminfos);
+        unsigned long total = hash_count(hs.hash_table);
+        u_unlock(cntx->soap);
+        return total;
+}
+
 /**
+ * The following extern methods are defined in wsmand-daemon.c,
+ * which is compiled into openwsmand binary, which in turn links
+ * to libwsman.la. So when a call is made to the following methods
+ * from the openwsmand binary, they should be present.
+ *
+ * However, if they are dlopened from somewhere other than
+ * openwsmand library or linked to some other
+ * binary or shared object, then these methods may or may not be
+ * preset, hence marking them as weak symbols and testing to see
+ * if they are resolved before using them.
+ */
+#pragma weak wsmand_options_get_max_threads
+extern int wsmand_options_get_max_threads(void);
+#pragma weak wsmand_options_get_max_connections_per_thread
+extern int wsmand_options_get_max_connections_per_thread(void);
+
+/**
  * Enumeration Stub for processing enumeration requests
  * @param op SOAP pperation handler
  * @param appData Application data
@@ -948,6 +975,35 @@
  WsXmlDocH       _doc = soap_get_op_doc(op, 1);
  WsContextH      epcntx;
 
+        int max_threads = 0;
+        int max_connections_per_thread = 0;
+        int(* fptr)(void);
+        if((fptr = wsmand_options_get_max_threads) != 0){
+                max_threads = (* fptr)();
+                if((fptr = wsmand_options_get_max_connections_per_thread) != 0){
+                        max_connections_per_thread = (* fptr)();
+                }
+                else{
+                        debug("Could not resolve wsmand_options_get_max_connections_per_thread");
+                        max_threads=0;
+                }
+        }
+        else{
+                debug("Could not resolve wsman_options_get_max_threads");
+        }
+
+        if(max_threads){
+                if(get_total_enum_context(ws_get_soap_context(soap)) >= (max_threads * max_connections_per_thread)){
+                        debug("enum context queue is full, we wait till some expire or are cleared");
+                        doc = wsman_generate_fault(_doc, WSMAN_QUOTA_LIMIT, OWSMAN_NO_DETAILS,
+                                    "The service is busy servicing other requests. Try later.");
+                        if(doc){
+                                soap_set_op_doc(op, doc, 0);
+                        }
+                        return 1;
+                }
+        }
+
  epcntx = ws_create_ep_context(soap, _doc);
  wsman_status_init(&status);
  doc = create_enum_info(op, epcntx, _doc, &enumInfo);
Index: src/server/wsmand-daemon.h
===================================================================
--- src/server/wsmand-daemon.h (revision 3254)
+++ src/server/wsmand-daemon.h (working copy)
@@ -86,6 +86,8 @@
 char * wsmand_options_get_subscription_repository_uri(void);
 char *wsmand_options_get_identify_file(void);
 char *wsmand_options_get_anon_identify_file(void);
+unsigned int wsmand_options_get_thread_stack_size(void);
+int wsmand_options_get_max_connections_per_thread(void);
 
 const char **wsmand_options_get_argv(void);
 int wsmand_read_config(dictionary * ini);
Index: src/server/wsmand.c
===================================================================
--- src/server/wsmand.c (revision 3254)
+++ src/server/wsmand.c (working copy)
@@ -217,7 +217,8 @@
  setsid();
 
  /* Change our CWD to / */
- chdir("/");
+ i=chdir("/");
+        assert(i == 0);
 
  /* Close all file descriptors. */
  for (i = getdtablesize(); i >= 0; --i)
Index: src/server/wsmand-daemon.c
===================================================================
--- src/server/wsmand-daemon.c (revision 3254)
+++ src/server/wsmand-daemon.c (working copy)
@@ -92,6 +92,8 @@
 static int max_threads = 1;
 static int min_threads = 4;
 static unsigned long enumIdleTimeout = 100 * 1000;
+static char *thread_stack_size="0";
+static int max_connections_per_thread=20;
 
 static char *config_file = NULL;
 
@@ -179,14 +181,32 @@
     iniparser_getstr(ini, "server:basic_authenticator_arg");
  log_location = iniparser_getstr(ini, "server:log_location");
  min_threads = iniparser_getint(ini, "server:min_threads", 1);
- max_threads = iniparser_getint(ini, "server:max_threads", 4);
+ max_threads = iniparser_getint(ini, "server:max_threads", 0);
  uri_subscription_repository = iniparser_getstring(ini, "server:subs_repository", DEFAULT_SUBSCRIPTION_REPOSITORY);
+        max_connections_per_thread = iniparser_getint(ini, "server:max_connextions_per_thread", 20);
+        thread_stack_size = iniparser_getstring(ini, "server:thread_stack_size", "0");
 #ifdef ENABLE_EVENTING_SUPPORT
  wsman_server_set_subscription_repos(uri_subscription_repository);
 #endif
  return 1;
 }
 
+int wsmand_options_get_max_connections_per_thread(void)
+{
+        return max_connections_per_thread;
+}
+
+unsigned int wsmand_options_get_thread_stack_size(void)
+{
+        errno=0;
+        unsigned int stack_size = strtoul(thread_stack_size,NULL,10);
+        if(errno){
+                debug("failed to convert string to unsigned int : %s", strerror(errno));
+                return 0;
+        }
+        return stack_size;
+}
+
 const char *wsmand_options_get_config_file(void)
 {
  char *p;
@@ -305,6 +325,9 @@
 
 int wsmand_options_get_max_threads(void)
 {
+//XXX: we might wish to return a constant 1 till the
+//MT issues on unix are solved. See compat_unix.h for
+//details on the issue.
  return max_threads;
 }
 
Index: src/server/wsmand-listener.c
===================================================================
--- src/server/wsmand-listener.c (revision 3254)
+++ src/server/wsmand-listener.c (working copy)
@@ -83,7 +83,6 @@
 #endif
 #include <sys/socket.h>
 
-#define MAX_CONNECTIONS_PER_THREAD 20
 
 static pthread_mutex_t shttpd_mutex;
 static pthread_cond_t shttpd_cond;
@@ -600,6 +599,13 @@
  return ret;
  }
  return 1;
+        size_t thread_stack_size = wsmand_options_get_thread_stack_size();
+        if(thread_stack_size){
+                if(( r = pthread_attr_setstacksize(pattrs, thread_stack_size)) !=0) {
+                        debug("pthread_attr_setstacksize failed = %d", r);
+                        return ret;
+                }
+        }
 }
 
 static void *thread_function(void *param)
@@ -636,13 +642,14 @@
 
 
 static struct thread *
-find_not_busy_thread(void)
+find_not_busy_thread(int *num_threads, int max_connections_per_thread)
 {
     struct thread   *thread;
 
-    for (thread = threads; thread != NULL; thread = thread->next) {
- debug("Active sockets: %d", shttpd_active(thread->ctx) );
-        if (shttpd_active(thread->ctx) < MAX_CONNECTIONS_PER_THREAD)
+    for (thread = threads, *num_threads=0; thread != NULL; thread = thread->next) {
+ debug("Active sockets: %d, Thread Number: %d", shttpd_active(thread->ctx), *num_threads );
+        (*num_threads)++;
+        if (shttpd_active(thread->ctx) < max_connections_per_thread)
             return (thread);
  }
 
@@ -665,6 +672,13 @@
  WsManListenerH *listener = wsman_dispatch_list_new();
  listener->config = ini;
  WsContextH cntx = wsman_init_plugins(listener);
+        int num_threads=0;
+        int max_threads=wsmand_options_get_max_threads();
+        int max_connections_per_thread = wsmand_options_get_max_connections_per_thread();
+        if(max_threads && !max_connections_per_thread){
+                error("max_threads: %d and max_connections_per_thread : %d", max_threads, max_connections_per_thread);
+                return listener;
+        }
 
 #ifdef ENABLE_EVENTING_SUPPORT
  wsman_event_init(cntx->soap);
@@ -710,10 +724,20 @@
  continue;
  }
  debug("Sock %d accepted", sock);
-        if ((thread = find_not_busy_thread()) == NULL)
-            thread = spawn_new_thread(pattrs, soap);
-
-        shttpd_add_socket(thread->ctx, sock, use_ssl);
- }
- return listener;
+                if ((thread = find_not_busy_thread(&num_threads, max_connections_per_thread)) == NULL){
+                        if(max_threads){
+                                if(num_threads < max_threads){
+                                        thread = spawn_new_thread(pattrs, soap);
+                                }
+                                else{
+                                        continue;
+                                }
+                        }
+                        else{
+                                thread = spawn_new_thread(pattrs, soap);
+                        }
+ }
+                shttpd_add_socket(thread->ctx, sock, use_ssl);
+        }
+        return listener;
 }
Index: etc/openwsman.conf
===================================================================
--- etc/openwsman.conf (revision 3254)
+++ etc/openwsman.conf (working copy)
@@ -7,7 +7,9 @@
 basic_password_file = /etc/openwsman/simple_auth.passwd
 
 min_threads = 4
-max_threads = 10
+max_threads = 0
+max_connections_per_thread=20
+#thread_stack_size=262144
 
 #use_digest is OBSOLETED, see below.
 

---

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Openwsman-devel mailing list
Openwsman-devel@...
https://lists.sourceforge.net/lists/listinfo/openwsman-devel

[Suresh Sundriyal-2]

Hi Klaus,

I'm glad you did not check this particular change in (enum-expires.diff), yet. :-)
It does fix the problem where enumerations with explicit expiretime are
deleted immediately but it breaks normal enumerations.

Looking at the code it seems like the way Openwsman handles time is
messed up. According to the comment in __WsEnumerateInfo the expires
is expected to be in milliseconds since epoch time. However, the method
wsman_set_expiretime() sets it in seconds and time_expired() also checks
for this time in seconds. However, with normal enumeration requests with
no explicit expire time, the timestamp is set to milliseconds and in
wsman_get_expired_enuminfos, the timestamp as well as the expires field
in the enuminfos are checked against mytime which is in milliseconds. Which
is why if I fix one check the other breaks.

There are two ways to solve this problem, one is I convert all the methods
that set/serialize/deserialize expiretime to set the time in milliseconds, this
is pervasive since lots more code to fix. However, looking at the comment
in the __WsEnumerateInfo enum, it would be according to the original
design. I think I already have a changeset for this which I will be testing
tomorrow.

The other way would be to just change the code where enum timestamp is
set to use seconds instead of milliseconds. This should be pretty easy to do,
I think since I see only one place where the timestamp is set. However, I
haven't explored this one as well as the solution I described above

Let me know which solution you prefer?

--
Suresh
 
________________________________________
From: Suresh Sundriyal [ssundriy@...]
Sent: Wednesday, September 16, 2009 4:37 PM
To: Klaus Kaempf; openwsman-devel@...
Subject: Re: [Openwsman-devel] More fixes and testing results.

Suresh Sundriyal wrote:

> Enumeration with expire time:
> =============================
>
> This could just be a problem at my end, since I've been playing around
> with the code so much that my local client is completely messed up at
> the moment (I tend to work off of 2.1.0 code and then patching it with
> the changes) but it seems like if I send an enumeration request with an
> expire time, the server does not honor the expire time and immidiately
> deletes the results. If anyone could chime in and confirm whether this
> is a problem or not before I start investigating this, it would be
> appreciated.
>

I'm sending along a patch for the problem above. It seems that when we
calculate the timestamp for the enumeration results in
wsman_set_expiretime (wsman-soap-envelope.c), we set it to:

*expire = tv.tv_sec + timeout;

This is basically (epoch seconds + offset).

When we check to see whether the enumeration has expired or not in
wsman_get_expired_enuminfos (wsman-soap.c), we test it against:

mytime = tv.tv_sec * 1000 + tv.tv_usec / 1000;

This is epoch seconds converted to microseconds + epoch microseconds
converted to seconds(?!!). This pretty much always ensures that mytime
will always be greater than expire_time. Thus the enumeration results
expire as soon as they are created.

To fix this, I changed mytime to :

mytime = tv.tv_sec

This seems to work fine. I could not correct the code in
wsman_get_expired_enuminfos, since that code is also used by
subscription_expiration and changing the code there makes subscriptions
act weird.

Tested the code with enumeration intrinsic with expiration time (PT45S,
did not get a chance to test it with datetime format but even that code
seems to convert everything to sec, so I think it should be OK). Also
tested the subscriptions to make sure they were working correctly.

--
Suresh

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Openwsman-devel mailing list
Openwsman-devel@...
https://lists.sourceforge.net/lists/listinfo/openwsman-devel

[Suresh Sundriyal-2]

Here's the correct fix for the problem.

Problem:
=====
When you send an enumeration request with an expiretime to Openwsman the
enumerations are not cached and it is immidiately deleted. In order to reproduce
the problem, send an enumeration request to Openwsman with the expiretime
set to 45 seconds. Then try to retrieve the results with the returned enumcontext
and Openwsman returns an invalid enumcontext.

Root cause analysis:
============
The enumerations are tested for expiration in the method wsman_get_expired_enumcontext()
in file wsman-soap.c, where the enumInfo->timeStamp and enumInfo->expiretime are
both tested against the mytime variable which contains the currect epoch time in
milliseconds.

However, the method wsman_set_expiretime() in wsman-soap-envelop.c sets
enumInfo->expireTime in seconds. Thus if enumInfo->expireTime is tested against
mytime, enumInfo->expireTime will always be less. If we change the code such that
mytime is set to current time in epoch seconds (which my previously sent diff did),
then (enumInfo->timeStamp + enumIdletimeout) will always be greater than mytime,
thus breaking normal enumerations without the expiretime.

Fix:
==
As I mentioned before, there were two choices, I could either convert everything to
milliseconds or convert everything to seconds. I decided to go with the latter since:

1) Changing everything to use milliseconds required touching a lot of code, since
subscriptions also use wsman_set_expiretime() and some parts of sfcc-interface
also assume the time to be in seconds.
2) enumInfo->expiretime, enumInfo->timeStamp and mytime are all unsigned long
and on a 32-bit system unsigned long does not have enough bits to store the
current epoch time in milliseconds which as of right now is 1254455904000. (Storing
current epoch time in milliseconds in unsigned long is a bug as well).
3) The cleanup thread runs once every second, so the smallest resolution of time
that you get is a second anyway.

The fix changes mytime to store the current epoch time in seconds. The timestamp
is also set to seconds. The enumIdleTimeout is set to 100 seconds in wsmand-daemon.c
file instead of 100000 milliseconds.

--
Regards,
Suresh

________________________________________
From: Suresh Sundriyal
Sent: Tuesday, September 29, 2009 10:36 PM
To: Suresh Sundriyal; Klaus Kaempf; openwsman-devel@...
Subject: RE: [Openwsman-devel] More fixes and testing results.

Hi Klaus,

I'm glad you did not check this particular change in (enum-expires.diff), yet. :-)
It does fix the problem where enumerations with explicit expiretime are
deleted immediately but it breaks normal enumerations.

Looking at the code it seems like the way Openwsman handles time is
messed up. According to the comment in __WsEnumerateInfo the expires
is expected to be in milliseconds since epoch time. However, the method
wsman_set_expiretime() sets it in seconds and time_expired() also checks
for this time in seconds. However, with normal enumeration requests with
no explicit expire time, the timestamp is set to milliseconds and in
wsman_get_expired_enuminfos, the timestamp as well as the expires field
in the enuminfos are checked against mytime which is in milliseconds. Which
is why if I fix one check the other breaks.

There are two ways to solve this problem, one is I convert all the methods
that set/serialize/deserialize expiretime to set the time in milliseconds, this
is pervasive since lots more code to fix. However, looking at the comment
in the __WsEnumerateInfo enum, it would be according to the original
design. I think I already have a changeset for this which I will be testing
tomorrow.

The other way would be to just change the code where enum timestamp is
set to use seconds instead of milliseconds. This should be pretty easy to do,
I think since I see only one place where the timestamp is set. However, I
haven't explored this one as well as the solution I described above

Let me know which solution you prefer?

--
Suresh

________________________________________
From: Suresh Sundriyal [ssundriy@...]
Sent: Wednesday, September 16, 2009 4:37 PM
To: Klaus Kaempf; openwsman-devel@...
Subject: Re: [Openwsman-devel] More fixes and testing results.

Suresh Sundriyal wrote:

> Enumeration with expire time:
> =============================
>
> This could just be a problem at my end, since I've been playing around
> with the code so much that my local client is completely messed up at
> the moment (I tend to work off of 2.1.0 code and then patching it with
> the changes) but it seems like if I send an enumeration request with an
> expire time, the server does not honor the expire time and immidiately
> deletes the results. If anyone could chime in and confirm whether this
> is a problem or not before I start investigating this, it would be
> appreciated.
>

I'm sending along a patch for the problem above. It seems that when we
calculate the timestamp for the enumeration results in
wsman_set_expiretime (wsman-soap-envelope.c), we set it to:

*expire = tv.tv_sec + timeout;

This is basically (epoch seconds + offset).

When we check to see whether the enumeration has expired or not in
wsman_get_expired_enuminfos (wsman-soap.c), we test it against:

mytime = tv.tv_sec * 1000 + tv.tv_usec / 1000;

This is epoch seconds converted to microseconds + epoch microseconds
converted to seconds(?!!). This pretty much always ensures that mytime
will always be greater than expire_time. Thus the enumeration results
expire as soon as they are created.

To fix this, I changed mytime to :

mytime = tv.tv_sec

This seems to work fine. I could not correct the code in
wsman_get_expired_enuminfos, since that code is also used by
subscription_expiration and changing the code there makes subscriptions
act weird.

Tested the code with enumeration intrinsic with expiration time (PT45S,
did not get a chance to test it with datetime format but even that code
seems to convert everything to sec, so I think it should be OK). Also
tested the subscriptions to make sure they were working correctly.

--
Suresh

[enum-expires.diff]

---

Index: src/lib/wsman-soap.c
===================================================================
--- src/lib/wsman-soap.c (revision 3254)
+++ src/lib/wsman-soap.c (working copy)
@@ -350,7 +350,7 @@
 
  u_lock(cntx->soap);
  gettimeofday(&tv, NULL);
- enumInfo->timeStamp = tv.tv_sec * 1000 + tv.tv_usec / 1000;
+ enumInfo->timeStamp = tv.tv_sec;
  if (create_context_entry(cntx->enuminfos, enumInfo->enumId, enumInfo)) {
  retVal = 0;
  }
@@ -424,7 +424,7 @@
  return;
  }
  enumInfo->flags &= ~WSMAN_ENUMINFO_INWORK_FLAG;
- enumInfo->timeStamp = tv.tv_sec * 1000 + tv.tv_usec / 1000;
+ enumInfo->timeStamp = tv.tv_sec;
  u_unlock(cntx->soap);
 }
 
@@ -1283,7 +1283,7 @@
  return NULL;
  }
  gettimeofday(&tv, NULL);
- mytime = tv.tv_sec * 1000 + tv.tv_usec / 1000;
+ mytime = tv.tv_sec;
  u_lock(cntx->soap);
  if (hash_isempty(cntx->enuminfos)) {
  u_unlock(cntx->soap);
Index: src/server/wsmand-daemon.c
===================================================================
--- src/server/wsmand-daemon.c (revision 3254)
+++ src/server/wsmand-daemon.c (working copy)
@@ -91,7 +91,7 @@
 static char *basic_authenticator = DEFAULT_BASIC_AUTH;
 static int max_threads = 1;
 static int min_threads = 4;
-static unsigned long enumIdleTimeout = 100 * 1000;
+static unsigned long enumIdleTimeout = 100;
 
 static char *config_file = NULL;
 
@@ -116,7 +116,7 @@
  "Set the verbosity of syslog output.", "0-6"},
  {"enum-idle-timeout", 'e', U_OPTION_ARG_INT,
  &enumIdleTimeout,
- "Enumeration Idle timeout in msecs", "default 100000"},
+ "Enumeration Idle timeout in secs", "default 100"},
  {"config-file", 'c', U_OPTION_ARG_STRING, &config_file,
  "Alternate configuration file", "<file>"},
  {"pid-file", 'p', U_OPTION_ARG_STRING, &pid_file,
@@ -159,7 +159,7 @@
  enumIdleTimeout =
     (unsigned long) iniparser_getint(ini,
      "server:enum_idle_timeout",
-     100 * 1000);
+     100);
  service_path =
     iniparser_getstring(ini, "server:service_path", "/wsman");
  ssl_key_file = iniparser_getstr(ini, "server:ssl_key_file");

---

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Openwsman-devel mailing list
Openwsman-devel@...
https://lists.sourceforge.net/lists/listinfo/openwsman-devel

[Klaus Kaempf]

Hi Suresh,

* Suresh Sundriyal <ssundriy@...> [Sep 21. 2009 08:14]:
>
> The patch reintroduces the max_thread config file parameter and introduces
> two new parameters:
>
> * max_connections_per_thread
> * thread_stack_size

finally I found some time again to work on the 2.2.2 release of
openwsman. I just applied your patch as svn rev 3296 and added your
explanation as svn rev 3297.

Thanks again for your work !

Klaus
---
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg)


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Openwsman-devel mailing list
Openwsman-devel@...
https://lists.sourceforge.net/lists/listinfo/openwsman-devel