Ensure sockets are closed when sessions are closed

[openhpi2]

From cf9cc9b59586bcdb410d03c6b26408eb4eb75414 Mon Sep 17 00:00:00 2001
From: Mark Tomlinson mark.tomlinson@alliedtelesis.co.nz
Date: Mon, 29 Feb 2016 13:21:57 +1300
Subject: [PATCH] Ensure sockets are closed when sessions are closed

The cSession class had a per-thread variable m_sockets. This allowed
different threads to share the same session, but each would have a
different socket to communicate with the client. This was implemented
using GStaticPrivate, but as this had been deprecated, changed to
GPrivate. The documentation for GPrivate states: "It is not possible to
destroy a GPrivate after it has been used. As such, it is only ever
acceptable to use GPrivate in static scope, and even then sparingly so."
This is not true for the m_sockets variable.

To achieve the same thing, a hash table is used to allow a different
socket per thread. When the cSession class is deleted, all sockets are
closed as the hash table is freed up.

Signed-off-by: Mark Tomlinson mark.tomlinson@alliedtelesis.co.nz

baselib/session.cpp | 34 ++++++++++++----------------------
1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/baselib/session.cpp b/baselib/session.cpp
index c5edfc8..ca9c2d7 100644
--- a/baselib/session.cpp
+++ b/baselib/session.cpp
@@ -103,11 +103,7 @@ private:
SaHpiDomainIdT m_did;
SaHpiSessionIdT m_sid;
SaHpiSessionIdT m_remote_sid;
-#if GLIB_CHECK_VERSION (2, 32, 0)

• GPrivate m_sockets;
  -#else
• GStaticPrivate m_sockets;
  -#endif
• GHashTable * m_sockets;
  };

@@ -117,16 +113,12 @@ cSession::cSession()
m_sid( 0 ),
m_remote_sid( 0 )
{

• #if GLIB_CHECK_VERSION (2, 32, 0)
• m_sockets = G_PRIVATE_INIT (g_free);
• #else
• wrap_g_static_private_init( &m_sockets );
• #endif
• m_sockets = g_hash_table_new_full( g_direct_hash, g_direct_equal, 0, DeleteSock );
  }

cSession::~cSession()
{

• wrap_g_static_private_free( &m_sockets );
• g_hash_table_destroy( m_sockets );
  }

SaErrorT cSession::GetEntityRoot( SaHpiEntityPathT& entity_root ) const
@@ -213,11 +205,9 @@ SaErrorT cSession::DoRpc( uint32_t id,
}
}

•    #if GLIB_CHECK_VERSION (2, 32, 0)
  

•    wrap_g_static_private_set( &m_sockets, 0);// close socket
  

•    #else
  

•    wrap_g_static_private_set( &m_sockets, 0, 0 ); // close socket
  

•    #endif
  

•    ohc_lock();
  

•    g_hash_table_remove( m_sockets, (void *)pthread_self() ); // close socket
  

•    ohc_unlock();
   g_usleep( NEXT_RPC_ATTEMPT_TIMEOUT );
  

  }
  if ( !rc ) {
  @@ -240,7 +230,9 @@ SaErrorT cSession::DoRpc( uint32_t id,

  SaErrorT cSession::GetSock( cClientStreamSock * & sock )
  {

• gpointer ptr = wrap_g_static_private_get( &m_sockets );

• ohc_lock();

• gpointer ptr = g_hash_table_lookup( m_sockets, (void *)pthread_self() );

• ohc_unlock();
  if ( ptr ) {
  sock = reinterpret_cast<cClientStreamSock *>(ptr);
  } else {
  @@ -266,11 +258,9 @@ SaErrorT cSession::GetSock( cClientStreamSock * & sock )
  /* keepalive_intvl / 1,
  / keepalive_probes */ 3 );

•    #if GLIB_CHECK_VERSION (2, 32, 0)
  

•    wrap_g_static_private_set( &m_sockets, sock );
  

•    #else
  

•    wrap_g_static_private_set( &m_sockets, sock, DeleteSock );
  

•    #endif
  

•    ohc_lock();
  

•    g_hash_table_insert ( m_sockets, (void *)pthread_self(), sock );
  

•    ohc_unlock();
  

  }

  return SA_OK;

  2.7.2

Reported by: mtomlinson

[openhpi2]

Oops, formatting makes this patch a bit messy. I have added it as an attachment.

Original comment by: mtomlinson

[openhpi2]

Hi,

I appiled the attached patch but I am getting below errors while compilation. Do we have any updated patch.

System Details:

LSB Version: :core-4.0-amd64:core-4.0-noarch:graphics-4.0-amd64:graphics-4.0-noarch:printing-4.0-amd64:printing-4.0-noarch
Distributor ID: RedHatEnterpriseServer
Description: Red Hat Enterprise Linux Server release 6.1 (Santiago)
Release: 6.1
Codename: Santiago

Erros while compilation:

session.cpp: In member function ‘SaErrorT cSession::DoRpc(uint32_t, ClientRpcParams&, ClientRpcParams&)’:
session.cpp:209: error: ‘pthread_self’ was not declared in this scope
session.cpp: In member function ‘SaErrorT cSession::GetSock(cClientStreamSock_&)’:
session.cpp:234: error: ‘pthread_self’ was not declared in this scope
make[2]: *_* [session.lo] Error 1
make[2]: Leaving directory /root/praveen/1910/openhpi_april_06_2016/baselib' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory/root/praveen/1910/openhpi_april_06_2016'
make: *** [all] Error 2

Thanks!
Praveen

Original comment by: openhpi2

[openhpi2]

I just downloaded the latest trunk and applied the patch as-is, and it
worked fine for me. I am using Ubuntu, so I guess there's some
difference somewhere between our systems. Even though I can't reproduce
the issue, the only thing that appears to be wrong is not having a
prototype for pthread_self(). Please try adding the following to
session.cpp:

@@ -20,6 +20,7 @@
#include <string.h>

#include <glib.h>
+#include <pthread.h>

#include <oh_error.h>

This compiles fine for me, but then again, it did without this change.

On 07/04/16 03:14, Praveen wrote:

Hi,

I appiled the attached patch but I am getting below errors while
compilation. Do we have any updated patch.

System Details:

LSB Version:
:core-4.0-amd64:core-4.0-noarch:graphics-4.0-amd64:graphics-4.0-noarch:printing-4.0-amd64:printing-4.0-noarch
Distributor ID: RedHatEnterpriseServer
Description: Red Hat Enterprise Linux Server release 6.1 (Santiago)
Release: 6.1
Codename: Santiago

Erros while compilation:

session.cpp: In member function ‘SaErrorT cSession::DoRpc(uint32_t,
ClientRpcParams&, ClientRpcParams&)’:
session.cpp:209: error: ‘pthread_self’ was not declared in this scope
session.cpp: In member function ‘SaErrorT
cSession::GetSock(cClientStreamSock_&)’:
session.cpp:234: error: ‘pthread_self’ was not declared in this scope
make[2]: */[session.lo] Error 1
make[2]: Leaving directory
|/root/praveen/1910/openhpi_april_06_2016/baselib' make[1]: *_*
[all-recursive] Error 1 make[1]: Leaving
directory|/root/praveen/1910/openhpi_april_06_2016'
make: /* [all] Error 2

Thanks!
Praveen

---

[bugs:#1910] https://sourceforge.net/p/openhpi/bugs/1910/ Ensure
sockets are closed when sessions are closed

Status: open
3.7.0: 3.7.0
Created: Tue Mar 01, 2016 02:45 AM UTC by Mark Tomlinson
Last Updated: Tue Mar 01, 2016 02:47 AM UTC
Owner: nobody

From cf9cc9b59586bcdb410d03c6b26408eb4eb75414 Mon Sep 17 00:00:00 2001
From: Mark Tomlinson mark.tomlinson@alliedtelesis.co.nz
mailto:mark.tomlinson@alliedtelesis.co.nz
Date: Mon, 29 Feb 2016 13:21:57 +1300
Subject: [PATCH] Ensure sockets are closed when sessions are closed

The cSession class had a per-thread variable m_sockets. This allowed
different threads to share the same session, but each would have a
different socket to communicate with the client. This was implemented
using GStaticPrivate, but as this had been deprecated, changed to
GPrivate. The documentation for GPrivate states: "It is not possible to
destroy a GPrivate after it has been used. As such, it is only ever
acceptable to use GPrivate in static scope, and even then sparingly so."
This is not true for the m_sockets variable.

To achieve the same thing, a hash table is used to allow a different
socket per thread. When the cSession class is deleted, all sockets are
closed as the hash table is freed up.

Signed-off-by: Mark Tomlinson mark.tomlinson@alliedtelesis.co.nz
<mailto:mark.tomlinson@alliedtelesis.co.nz>

baselib/session.cpp | 34 ++++++++++++----------------------
1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/baselib/session.cpp b/baselib/session.cpp
index c5edfc8..ca9c2d7 100644
--- a/baselib/session.cpp
+++ b/baselib/session.cpp
@@ -103,11 +103,7 @@ private:
SaHpiDomainIdT m_did;
SaHpiSessionIdT m_sid;
SaHpiSessionIdT m_remote_sid;
-#if GLIB_CHECK_VERSION (2, 32, 0)

• GPrivate m_sockets;
  -#else
• GStaticPrivate m_sockets;
  -#endif
• GHashTable * m_sockets;
  };

@@ -117,16 +113,12 @@ cSession::cSession()
m_sid( 0 ),
m_remote_sid( 0 )
{

• #if GLIB_CHECK_VERSION (2, 32, 0)
• m_sockets = G_PRIVATE_INIT (g_free);
• #else
• wrap_g_static_private_init( &m_sockets );
• #endif
• m_sockets = g_hash_table_new_full( g_direct_hash, g_direct_equal, 0,
  DeleteSock );
  }

cSession::~cSession()
{

• wrap_g_static_private_free( &m_sockets );
• g_hash_table_destroy( m_sockets );
  }

SaErrorT cSession::GetEntityRoot( SaHpiEntityPathT& entity_root ) const
@@ -213,11 +205,9 @@ SaErrorT cSession::DoRpc( uint32_t id,
}
}

  if GLIB_CHECK_VERSION (2, 32, 0)

• wrap_g_static_private_set( &m_sockets, 0);// close socket
  *

  else
  

• wrap_g_static_private_set( &m_sockets, 0, 0 ); // close socket
  *

  endif
  

• ohc_lock();

• g_hash_table_remove( m_sockets, (void *)pthread_self() ); // close
  socket

• ohc_unlock();
  g_usleep( NEXT_RPC_ATTEMPT_TIMEOUT );
  }
  if ( !rc ) {
  @@ -240,7 +230,9 @@ SaErrorT cSession::DoRpc( uint32_t id,

SaErrorT cSession::GetSock( cClientStreamSock * & sock )
{

• gpointer ptr = wrap_g_static_private_get( &m_sockets );

• ohc_lock();

• gpointer ptr = g_hash_table_lookup( m_sockets, (void /)pthread_self() );

• ohc_unlock();
  if ( ptr ) {
  sock = reinterpret_cast<cClientStreamSock *="">(ptr);
  } else {
  @@ -266,11 +258,9 @@ SaErrorT cSession::GetSock( cClientStreamSock * &
  sock )
  // keepalive_intvl // 1,
  // keepalive_probes */ 3 );

  if GLIB_CHECK_VERSION (2, 32, 0)
  

  • wrap_g_static_private_set( &m_sockets, sock );
    *

    else

  • wrap_g_static_private_set( &m_sockets, sock, DeleteSock );
    *

    endif

  • ohc_lock();

  • g_hash_table_insert ( m_sockets, (void *)pthread_self(), sock );
    *

    ohc_unlock();
    }

    return SA_OK;
    

    2.7.2

---

Sent from sourceforge.net because you indicated interest in
https://sourceforge.net/p/openhpi/bugs/1910/

To unsubscribe from further messages, please visit
https://sourceforge.net/auth/subscriptions/

Original comment by: mtomlinson

[openhpi2]

Mark,

Thanks for the patch. I applied it with addional printf/CRIT statements and ran the tests using hpitop and hpi_shell. By and large it works well. The hpitop closed the sock properly. The hpi_shell has two threads and somehow the socks are not closed on that. Still looking into it. The modified patch is attached.

Mohan

Original comment by: dr_mohan

[openhpi2]

Mark,

The only client library that uses multiple threads wich sock connections is hpi_shell. Other single threaded applications are closing the sockets properly. At present hpi_shell does not. Even with your patch it is not closing the sockets at the end of the session. Could you please take a look at hpi_shell with your patch?

Thanks
Mohan

Original comment by: dr_mohan