search

openhpi2 / Open-HPI

Open HPI is an open source implementation of the SA Forum's Hardware Platform Interface (HPI). HPI provides an abstracted interface to managing computer hardware, typically for chassis and rack based servers
Other
3 stars 1 forks source link

Ensure sockets are closed when sessions are closed #2611

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