search

openhpi2 / openhpi_bug_test

0 stars 0 forks source link

Ensure sockets are closed when sessions are closed #1910

Open openhpi2 opened 8 years ago

openhpi2 commented 8 years ago

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)

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

Reported by: mtomlinson

openhpi2 commented 8 years ago

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

Original comment by: mtomlinson

openhpi2 commented 8 years ago

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: ‘pthreadself’ was not declared in this scope session.cpp: In member function ‘SaErrorT cSession::GetSock(cClientStreamSock&)’: session.cpp:234: error: ‘pthreadself’ 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 commented 8 years ago

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

include

+#include

include

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: ‘pthreadself’ 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]: _*

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 commented 8 years ago

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 commented 8 years ago

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