[Common-dev] RE: [Client-dev] CR:new network API implementation for Symbian
Ashish.As.Gupta at nokia.com Ashish.As.Gupta at nokia.comThanks for the comments. Please see inline response within []. -----Original Message----- From: ext Greg Wright [mailto:gwright at real.com] Sent: 18 August, 2005 21:52 To: Gupta Ashish.As (Nokia-TP-MSW/Irving); client-dev at helixcommunity.org; common Subject: Re: [Client-dev] CR:new network API implementation for Symbian This CR covers *ONLY* the diffs sent in a 4 of the 10 new files: > common/netio/platform/symbian/hxsymbiansocket.cpp > common/netio/pub/platform/symbian/hxsymbiansocket.h > client/netwksvc/platform/symbian/hxsymbianapmanao.cpp > client/netwksvc/platform/symbian/hxsymbianapmanao.h> We will get to the rest of these files tomorrow: > common/netio/platform/symbian/hxsymbiannet.cpp > common/netio/platform/symbian/hxsymbianresolver.cpp > common/netio/platform/symbian/hxsymbiansockaddr.cpp > common/netio/pub/platform/symbian/hxsymbiannet.h > common/netio/pub/platform/symbian/hxsymbianresolver.h > common/netio/pub/platform/symbian/hxsymbiansockaddr.h First, a general question. Will all these new files compile under all versions of Symbian, if not, which are the requirments? We can certainly drop support for 6.1, I would like to keep support for 70s and above if we can. Any problems here supporting 70s? If so, how big are they and can we work around it? [It compiles for 70s, 8, 9.1 and works. It will not compile for 6.1. ] >Index: umakepf/helix-client-s60-common.pfi >=================================================================== >RCS file: /cvsroot/ribosome/build/umakepf/helix-client-s60-common.pfi,v >retrieving revision 1.13 >diff -u -w -r1.13 helix-client-s60-common.pfi >--- umakepf/helix-client-s60-common.pfi 16 Mar 2005 02:33:37 -0000 1.13 >+++ umakepf/helix-client-s60-common.pfi 5 Aug 2005 19:27:00 -0000 >@@ -61,7 +61,7 @@ > # project.AddDefines('HELIX_FEATURE_LOGGING_UI') > > project.AddDefines('HELIX_FEATURE_AUDIO_INACCURATESAMPLING') >-project.AddDefines('HELIX_FEATURE_NETSERVICES_SHIM') >+#project.AddDefines('HELIX_FEATURE_NETSERVICES_SHIM') > > # Do not Edit Below this Line > ##################################################################### >@@ -70,4 +70,4 @@ > # plus brings in features from *3gpp profiles that are not common nor > # advanced. > exec_profile_file("helix-client-mobile-common.pfi") >-project.AddDefines('HELIX_FEATURE_NETSERVICES_SHIM') >+#project.AddDefines('HELIX_FEATURE_NETSERVICES_SHIM') We should just remove these lines if they are not needed. Also, we can only remove this if the changes get put onto *all* branches. This same .pf file will be used regardless of what branch you compile on. These changes will only be merged to HEAD, so that would leave 150Cay broken. What I would suggest is that we remove the SHIM feature from the above file and add into the .bif file for the 150Cay branch in the symbian target section. [Fine. I will remove project.AddDefines('HELIX_FEATURE_NETSERVICES_SHIM') line from here. SHIM flag can be added to .bif for 150Cay.] > Index: client/netwksvc/platform/symbian/hxsymbianapman.cpp > =================================================================== > RCS file: /cvsroot/client/netwksvc/platform/symbian/hxsymbianapman.cpp,v > retrieving revision 1.5 > diff -u -w -r1.5 hxsymbianapman.cpp > --- client/netwksvc/platform/symbian/hxsymbianapman.cpp 14 Mar 2005 20:32:07 -0000 1.5 > +++ client/netwksvc/platform/symbian/hxsymbianapman.cpp 5 Aug 2005 19:14:08 -0000 > @@ -488,6 +394,19 @@ > > HXBOOL bQueueResponse = (pResp) ? TRUE : FALSE; > > + // make sure that RConnection, RSocketServ are ready for the connection. > + if(DoInit() != HXR_OK) > + { > + if (pResp) > + { > + pResp->ConnectDone(res); > + } You may want to set res=DoInit() so that we can pass up the correct return code to ConnectDone(). [Fine.] ================ NEW FILES CR =========================== hxsymbiansocket.h and hxsymbiansocket.cpp >class CHXSymbianSocket : public IHXSocket > ,public IHXMulticastSocket We have seen some very hard to find bugs when a class that supports IUnknown does not inherit from it. I would suggest you add a 'public IUnknown' to all your classes that are COM objects. I will just point out this one above. [Fine.] CHXBuffer* m_pWriteBuf; This does not seemed to be used anywhere. [Fine. It can be removed.] CHXListeningSocket::CHXListeningSocket(IHXSocket* pSock) : m_nRefCount(0), m_pResponse(NULL), m_pSock(pSock) { HX_ADDREF(m_pSock); } CHXListeningSocket::~CHXListeningSocket(void) { Close(); HX_RELEASE(m_pSock); HX_RELEASE(m_pResponse); } I dont' see m_pResponse used anywhere for the listening socket. [OK.] STDMETHODIMP CHXSymbianSocket::Init(HXSockFamily f, HXSockType t, HXSockProtocol p) { HX_RESULT hxr = HXR_OK; m_pConnector = new CHXSymbianSocketConnector(this); m_pWriter = new CHXSymbianSocketWriter(this); m_pReader = new CHXSymbianSocketReader(this); if( !m_pConnector || !m_pWriter || !m_pReader ) { return HXR_OUTOFMEMORY; } If only one of the above 'new's fail, you will leak the other two. You should add HX_DELETE()s for each one (it checks for null). if (m_pAPManager == NULL) { hxr = HXR_FAILED; } else if( (m_pAPResponse = new HXAccessPointConnectResp(this, static_APConnectDone)) == NULL ) { hxr = HXR_OUTOFMEMORY; } else { HX_ADDREF(m_pAPResponse); m_state = EOpen; } For each one of these error conditions I think you want to delete m_pConnector, m_pWriter and m_pReader correct? [ Fine. ] STDMETHODIMP CHXSymbianSocket::SetOption(HXSockOpt name, UINT32 val) { DPRINTF(D_INFO, ("CHXSymbianSocket::SetOption name=%d val=%d\n", name, val)); // XXXAG not all options are implemented. add other options as required. switch (name) ... } Can you switch to using the new HXLOG debug statements, DPRINTF is depricated. We are switching over all the old code as we fix stuff in it. Since this is new code, it would be best to just use HXLOG from the start. [HXLOG were not in working condition for symbian. Can this be done once HXLOG is known to be working fine? I can send the CR to replace it, once HXLOG is known to be working fine for 7, 8 and 9. ] if(m_bInitialized) { m_bInitialized = FALSE; m_sock.Close(); } // Send a Close Event. if (HX_SOCK_EVENT_CLOSE & m_ulEventMask && m_pResponse) { m_pResponse->EventPending(HX_SOCK_EVENT_CLOSE, hxr); } Should we only send the CLOSE event iff we were in an initialized state (it had been opened)? [Fine. I will remove Logic to send the Close event from here. It will be moved at line 844 and DoClose() call will be removed. ] HX_RESULT CHXSymbianSocket::TransferBuffer(REF(IHXBuffer *)pInBuf, REF(IHXBuffer *)pOutBuf, UINT32 bytes) { HX_RESULT hxr = HXR_OK; if (HX_SOCK_READBUF_SIZE_COPY == m_readBufAlloc) { // allocate a new buffer so buffer does not waste unused memory space hxr = CreateReadBuffer(pOutBuf, bytes); if (HXR_OK == hxr) { memcpy(pOutBuf->GetBuffer(), pInBuf->GetBuffer(), bytes); HX_RELEASE(pInBuf); } } else { HX_ASSERT(HX_SOCK_READBUF_SIZE_DEFAULT == m_readBufAlloc); // no copy. pOutBuf = pInBuf; pOutBuf->AddRef(); HX_RELEASE(pInBuf); pOutBuf->SetSize(bytes); } return hxr; } I am a bit confused about this function TranserBuffer(). It isn't real obvious what it does. First, if pOutBuf is not NULL, it is an AddRef'ed IHXBuffer. If you go into CreateReadBuffer() then it doesn't get Release()'ed, it just get overridden with a new IHXBuffer, so we leak one. If it goes into the 'else' clause then we have: pOutBuf = pInBuf; pOutBuf->AddRef(); HX_RELEASE(pInBuf); pOutBuf->SetSize(bytes); This seems to be the equiv of just: pOutBuf = pInBuf; pOutBuf->SetSize(bytes); Not sure what is really meant to be happening here. [pOutBuf is always passed as NULL to TransferBuffer. The function either creates a new Buffer by calling CreatReadBuffer and does a memcpy or assigns the pInBuf to pOutBuf. An ASSERT is there before the function is called. I can put one more ASSERT in the function to clarify the same. ] // Multicast operation requires IPv6 addresses if(pGroupAddr->GetFamily() == HX_SOCK_FAMILY_IN4) dstAddr.ConvertToV4Mapped(); Why does multicast require IPv6? [API is like that. Please see TIp6Mreq] void CHXSymbianSocket::SendEvents(INT32 events, HX_RESULT hxr) { if (m_pResponse) { // send from the reverse order to make sure connect // occurs before read/write for (UINT32 i = HX_SOCK_EVENT_LAST; i > 0; i >>= 1) { if (events & m_ulEventMask & i) { // Send the event m_pResponse->EventPending(i, hxr); } } } } This 'ordering' of events will only work because you know what the bit order is. This bit order can change over time and should be transparent to the users. So, if there are certain events that must always be sent before others, you should test for them explictly. For example: if (m_pResponse) { //Send connect before all other events if( m_ulEventMask && HX_SOCK_EVENT_CONNECT ) { m_pResponse->EventPending(HX_SOCK_EVENT_CONNECT, hxr); //Clear that one event. m_ulEventMask = m_ulEventMask & ~HX_SOCK_EVENT_CONNECT; } for (UINT32 i = HX_SOCK_EVENT_LAST; i > 0; i >>= 1) { if (events & m_ulEventMask & i) { m_pResponse->EventPending(i, hxr); } } } [Fine. I will put Connect and Close before the loop.] I will get to the rest of the new files tomorrow. --greg.