From: Erich Keane Date: Wed, 10 Sep 2014 18:08:09 +0000 (-0700) Subject: Provided a mechanism for deleting the context pointer when the C Stack deletes callba... X-Git-Tag: 1.2.0+RC1~2260 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=8ba379a507720d2c902cd5b62b8c9a027b7dac69;p=platform%2Fupstream%2Fiotivity.git Provided a mechanism for deleting the context pointer when the C Stack deletes callback. Otherwise, the callback context will be leaked. Fixed some line endings. Fixed some line lengths. Fixed a series of memory leaks in C++ callbacks which assumed the C stack owned/deleted OCCallbackData Patch 2: Removed deletion of function pointers becaues GCC says this violates the C spec. Patch 3: Comments from Joey regarding the comment on the OCClientContextDeleter typedef Patch 4: changed deleter to deleteCallback per sashi's suggestion Change-Id: I0fb71896afac93356c1bd2f4916eb9f3eaa0ad13 --- diff --git a/csdk/stack/include/internal/occlientcb.h b/csdk/stack/include/internal/occlientcb.h index 17ccada..105e8b4 100644 --- a/csdk/stack/include/internal/occlientcb.h +++ b/csdk/stack/include/internal/occlientcb.h @@ -37,11 +37,14 @@ typedef struct ClientCB { OCClientResponseHandler callBack; // callback context data void * context; + // callback method to delete context data + OCClientContextDeleter deleteCallback; // when a response is recvd with this token, above callback will be invoked OCCoAPToken * token; // Invocation handle tied to original call to OCDoResource() OCDoHandle handle; - // This is used to determine if all responses should be consumed or not. (For now, only pertains to OC_REST_OBSERVE_ALL Vs. OC_REST_OBSERVE functionality) + // This is used to determine if all responses should be consumed or not. + // (For now, only pertains to OC_REST_OBSERVE_ALL Vs. OC_REST_OBSERVE functionality) OCMethod method; // This is the sequence identifier the server applies to the invocation tied to 'handle'. uint32_t sequenceNumber; diff --git a/csdk/stack/include/ocstack.h b/csdk/stack/include/ocstack.h index 92d7e3e..c0b2605 100644 --- a/csdk/stack/include/ocstack.h +++ b/csdk/stack/include/ocstack.h @@ -196,8 +196,14 @@ typedef enum { /** * Client applications implement this callback to consume responses received from Servers. */ -typedef OCStackApplicationResult (* OCClientResponseHandler)(void *context, OCDoHandle handle, OCClientResponse * clientResponse); +typedef OCStackApplicationResult (* OCClientResponseHandler)(void *context, OCDoHandle handle, + OCClientResponse * clientResponse); +/** + * Client applications using a context pointer implement this callback to delete the + * context upon removal of the callback/context pointer from the internal callback-list + */ +typedef void (* OCClientContextDeleter)(void *context); /* * This info is passed from application to OC Stack when initiating a request to Server @@ -205,6 +211,7 @@ typedef OCStackApplicationResult (* OCClientResponseHandler)(void *context, OCDo typedef struct { void *context; OCClientResponseHandler cb; + OCClientContextDeleter cd; } OCCallbackData; /** diff --git a/csdk/stack/samples/linux/SimpleClientServer/occlient.cpp b/csdk/stack/samples/linux/SimpleClientServer/occlient.cpp index aca0d50..dd755c8 100644 --- a/csdk/stack/samples/linux/SimpleClientServer/occlient.cpp +++ b/csdk/stack/samples/linux/SimpleClientServer/occlient.cpp @@ -123,7 +123,7 @@ OCStackResult InvokeOCDoResource(std::ostringstream &query, cbData.cb = cb; cbData.context = (void*)CTX_VAL; - + cbData.cd = NULL; ret = OCDoResource(&handle, method, query.str().c_str(), 0, (method == OC_REST_PUT) ? putPayload.c_str() : NULL, qos, &cbData); @@ -346,6 +346,7 @@ int InitDiscovery() } cbData.cb = discoveryReqCB; cbData.context = (void*)CTX_VAL; + cbData.cd = NULL; ret = OCDoResource(&handle, OC_REST_GET, szQueryUri, 0, 0, OC_NON_CONFIRMABLE, &cbData); if (ret != OC_STACK_OK) { diff --git a/csdk/stack/samples/linux/SimpleClientServer/occlientcoll.cpp b/csdk/stack/samples/linux/SimpleClientServer/occlientcoll.cpp index 44713c3..c691df4 100644 --- a/csdk/stack/samples/linux/SimpleClientServer/occlientcoll.cpp +++ b/csdk/stack/samples/linux/SimpleClientServer/occlientcoll.cpp @@ -151,6 +151,7 @@ int InitGetRequestToUnavailableResource(OCClientResponse * clientResponse) getQuery << "coap://" << getIPAddrTBServer(clientResponse) << ":" << getPortTBServer(clientResponse) << "/SomeUnknownResource"; cbData.cb = getReqCB; cbData.context = (void*)CTX_VAL; + cbData.cd = NULL; ret = OCDoResource(&handle, OC_REST_GET, getQuery.str().c_str(), 0, 0, OC_NON_CONFIRMABLE, &cbData); if (ret != OC_STACK_OK) { @@ -169,6 +170,7 @@ int InitObserveRequest(OCClientResponse * clientResponse) obsReg << "coap://" << getIPAddrTBServer(clientResponse) << ":" << getPortTBServer(clientResponse) << getQueryStrForGetPut(clientResponse->resJSONPayload); cbData.cb = getReqCB; cbData.context = (void*)CTX_VAL; + cbData.cd = NULL; OC_LOG_V(INFO, TAG, "PUT payload from client = %s ", putPayload.c_str()); ret = OCDoResource(&handle, OC_REST_OBSERVE, obsReg.str().c_str(), 0, 0, OC_NON_CONFIRMABLE, &cbData); if (ret != OC_STACK_OK) @@ -194,6 +196,7 @@ int InitPutRequest(OCClientResponse * clientResponse) "/a/sroom?if=oc.mi.b"; cbData.cb = putReqCB; cbData.context = (void*)CTX_VAL; + cbData.cd = NULL; OC_LOG_V(INFO, TAG, "PUT payload from client = %s ", putPayload.c_str()); ret = OCDoResource(&handle, OC_REST_PUT, getQuery.str().c_str(), 0, putPayload.c_str(), OC_NON_CONFIRMABLE, &cbData); if (ret != OC_STACK_OK) @@ -228,6 +231,7 @@ int InitGetRequest(OCClientResponse * clientResponse) cbData.cb = getReqCB; cbData.context = (void*)CTX_VAL; + cbData.cd = NULL; ret = OCDoResource(&handle, OC_REST_GET, getQuery.str().c_str(), 0, 0, OC_NON_CONFIRMABLE, &cbData); if (ret != OC_STACK_OK) { @@ -250,6 +254,7 @@ int InitDiscovery() cbData.cb = discoveryReqCB; cbData.context = (void*)CTX_VAL; + cbData.cd = NULL; ret = OCDoResource(&handle, OC_REST_GET, szQueryUri, 0, 0, OC_NON_CONFIRMABLE, &cbData); if (ret != OC_STACK_OK) { diff --git a/csdk/stack/src/occlientcb.c b/csdk/stack/src/occlientcb.c index 9843513..b737223 100644 --- a/csdk/stack/src/occlientcb.c +++ b/csdk/stack/src/occlientcb.c @@ -38,6 +38,7 @@ OCStackResult AddClientCB(ClientCB** clientCB, OCCallbackData* cbData, if (cbNode) { cbNode->callBack = cbData->cb; cbNode->context = cbData->context; + cbNode->deleteCallback = cbData->cd; cbNode->token = token; cbNode->handle = handle; cbNode->method = method; @@ -60,6 +61,11 @@ void DeleteClientCB(ClientCB * cbNode) { OCFree(cbNode->token); OCFree(cbNode->handle); OCFree(cbNode->requestUri); + if(cbNode->deleteCallback) + { + cbNode->deleteCallback(cbNode->context); + } + #ifdef WITH_PRESENCE if(cbNode->presence) { OCFree(cbNode->presence->timeOut); @@ -75,7 +81,8 @@ ClientCB* GetClientCB(OCCoAPToken * token, OCDoHandle * handle, unsigned char * ClientCB* out = NULL; if(token) { LL_FOREACH(cbList, out) { - if((out->token->tokenLength == token->tokenLength) && (memcmp(out->token->token, token->token, token->tokenLength) == 0) ) { + if((out->token->tokenLength == token->tokenLength) && + (memcmp(out->token->token, token->token, token->tokenLength) == 0) ) { return out; } } diff --git a/csdk/stack/test/stacktests.cpp b/csdk/stack/test/stacktests.cpp index 6778880..e8a303c 100644 --- a/csdk/stack/test/stacktests.cpp +++ b/csdk/stack/test/stacktests.cpp @@ -129,6 +129,7 @@ TEST(StackTest, DoResourceDeviceDiscovery) { strcpy(szQueryUri, OC_WELL_KNOWN_QUERY); cbData.cb = asyncDoResourcesCallback; cbData.context = (void*)CTX_VAL; + cbData.cd = NULL; EXPECT_EQ(OC_STACK_OK, OCDoResource(&handle, OC_REST_GET, szQueryUri, 0, 0, OC_NON_CONFIRMABLE, &cbData)); //EXPECT_EQ(OC_STACK_OK, OCUpdateResources(SERVICE_URI)); EXPECT_EQ(OC_STACK_OK, OCStop()); @@ -158,6 +159,7 @@ TEST(StackTest, UpdateResourceNullURI) { strcpy(szQueryUri, OC_WELL_KNOWN_QUERY); cbData.cb = asyncDoResourcesCallback; cbData.context = (void*)CTX_VAL; + cbData.cd = NULL; EXPECT_EQ(OC_STACK_OK, OCDoResource(&handle, OC_REST_GET, szQueryUri, 0, 0, OC_NON_CONFIRMABLE, &cbData)); //EXPECT_EQ(OC_STACK_INVALID_URI, OCUpdateResources(0)); EXPECT_EQ(OC_STACK_OK, OCStop()); diff --git a/src/InProcClientWrapper.cpp b/src/InProcClientWrapper.cpp index 29bb7d1..b7b38e2 100644 --- a/src/InProcClientWrapper.cpp +++ b/src/InProcClientWrapper.cpp @@ -18,6 +18,7 @@ // //-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= +#include #include "InProcClientWrapper.h" #include "ocstack.h" @@ -217,14 +218,15 @@ namespace OC { OCStackResult result; - OCCallbackData* cbdata = new OCCallbackData(); + OCCallbackData cbdata = {0}; ListenContext* context = new ListenContext(); context->callback = callback; context->clientWrapper = shared_from_this(); - cbdata->context = static_cast(context); - cbdata->cb = listenCallback; + cbdata.context = static_cast(context); + cbdata.cb = listenCallback; + cbdata.cd = [](void* c){delete static_cast(c);}; auto cLock = m_csdkLock.lock(); if(cLock) @@ -235,7 +237,7 @@ namespace OC resourceType.c_str(), nullptr, nullptr, static_cast(m_cfg.QoS), - cbdata); + &cbdata); } else { @@ -387,11 +389,13 @@ namespace OC const std::string& uri, const QueryParamsMap& queryParams, GetCallback& callback) { OCStackResult result; - OCCallbackData* cbdata = new OCCallbackData(); + OCCallbackData cbdata = {0}; + GetContext* ctx = new GetContext(); ctx->callback = callback; - cbdata->context = static_cast(ctx); - cbdata->cb = &getResourceCallback; + cbdata.context = static_cast(ctx); + cbdata.cb = &getResourceCallback; + cbdata.cd = [](void* c){delete static_cast(c);}; // TODO: in the future the cstack should be combining these two strings! ostringstream os; @@ -407,7 +411,7 @@ namespace OC result = OCDoResource(&handle, OC_REST_GET, os.str().c_str(), nullptr, nullptr, static_cast(m_cfg.QoS), - cbdata); + &cbdata); } else { @@ -487,17 +491,19 @@ namespace OC const QueryParamsMap& queryParams, PutCallback& callback) { OCStackResult result; - OCCallbackData* cbdata = new OCCallbackData(); + OCCallbackData cbdata = {0}; + SetContext* ctx = new SetContext(); ctx->callback = callback; - cbdata->cb = &setResourceCallback; + cbdata.cb = &setResourceCallback; + cbdata.cd = [](void* c){delete static_cast(c);}; + cbdata.context = static_cast(ctx); // TODO: in the future the cstack should be combining these two strings! ostringstream os; os << host << assembleSetResourceUri(uri, queryParams).c_str(); // TODO: end of above - cbdata->context = static_cast(ctx); auto cLock = m_csdkLock.lock(); if(cLock) @@ -508,7 +514,7 @@ namespace OC os.str().c_str(), nullptr, assembleSetResourcePayload(attributes).c_str(), static_cast(m_cfg.QoS), - cbdata); + &cbdata); } else { @@ -543,11 +549,13 @@ namespace OC ObserveCallback& callback) { OCStackResult result; - OCCallbackData* cbdata = new OCCallbackData(); + OCCallbackData cbdata = {0}; + ObserveContext* ctx = new ObserveContext(); ctx->callback = callback; - cbdata->context = static_cast(ctx); - cbdata->cb = &observeResourceCallback; + cbdata.context = static_cast(ctx); + cbdata.cb = &observeResourceCallback; + cbdata.cd = [](void* c){delete static_cast(c);}; OCMethod method; if (observeType == ObserveType::Observe) @@ -577,7 +585,7 @@ namespace OC os.str().c_str(), nullptr, nullptr, static_cast(m_cfg.QoS), - cbdata); + &cbdata); } else { @@ -625,12 +633,13 @@ namespace OC const std::string& host, SubscribeCallback& presenceHandler) { OCStackResult result; - OCCallbackData* cbdata = new OCCallbackData(); + OCCallbackData cbdata = {0}; + SubscribePresenceContext* ctx = new SubscribePresenceContext(); ctx->callback = presenceHandler; - cbdata->cb = &subscribePresenceCallback; - cbdata->context = static_cast(ctx); - + cbdata.cb = &subscribePresenceCallback; + cbdata.context = static_cast(ctx); + cbdata.cd = [](void* c){delete static_cast(c);}; auto cLock = m_csdkLock.lock(); std::ostringstream os; @@ -642,7 +651,7 @@ namespace OC if(cLock) { result = OCDoResource(handle, OC_REST_PRESENCE, os.str().c_str(), nullptr, nullptr, - OC_NON_CONFIRMABLE, cbdata); + OC_NON_CONFIRMABLE, &cbdata); } else {