Nack on patch 2 with the following comments:
-----------------------
(1)
Instead of the embedded union "name" in IMMSV_OI_CCB_UPCALL_RSP:
typedef struct immsv_oi_ccb_upcall_rsp {
SaImmOiHandleT oi_client_hdl; //needed ?? does not seem like
it
SaUint32T ccbId;
SaUint32T implId;
SaUint32T inv;
SaAisErrorT result;
union {
SaNameT saName;
IMMSV_OCTET_STRING octetString;
} name;
IMMSV_OCTET_STRING errorString;
} IMMSV_OI_CCB_UPCALL_RSP;
Just add a new member at the end:
typedef struct immsv_oi_ccb_upcall_rsp {
SaImmOiHandleT oi_client_hdl; //needed ?? does not seem like
it
SaUint32T ccbId;
SaUint32T implId;
SaUint32T inv;
SaAisErrorT result;
SaNameT saName;
IMMSV_OCTET_STRING errorString;
IMMSV_OCTET_STRING name;
} IMMSV_OI_CCB_UPCALL_RSP;
At some point in the future we can then more easily replace
IMMSV_OI_CCB_UPCALL_RSP
with IMMSV_OI_CCB_UPCALL_RSP_2:
typedef struct immsv_oi_ccb_upcall_rsp {
SaUint32T ccbId;
SaUint32T implId;
SaUint32T inv;
SaAisErrorT result;
/*SaNameT saName;*/
IMMSV_OCTET_STRING errorString;
IMMSV_OCTET_STRING name;
} IMMSV_OI_CCB_UPCALL_RSP_2;
To be part of a new info2 union in IMMND_EVT struct.
The point is that the impact on the latest code will be minimal.
IMMND_EVT has three unused booleans at the top level, one of shich could
Be used to branch to the new message types in new code.
-----------------------
(2)
Avoid doing both borrow and malloc a new string copy when this is not needed!
While it is true that borrow does not take care of guaranteeing null
termination,
that should only be a concern if the SaNameT arrives over the public OM or OI
API from
the user/client/application. Any SameT value that arrives from the server must
be null terminated.
Imm library code that receives SaNameT values from the client MUST
ensure/correct for null termination
before continuing to process such names. SaNameT values are traced in lots of
places in both server and
library code and thus MUST be null terminated inside the IMM implementation.
This also reduces the need
For heap allocations and string copying.
So any null termination correction shoould be done ONCE as close as possible to
the external source.
In deeper library layers and in all server code the names are always
null-terminated otherwise we have a bug.
------------------------
Other than than these two issues of principle, the design looks ok.
Please send a new version of this second patch for review.
The first (undo) patch has already been pushed to both 4.6 and default branches.
/AndersBj
-----Original Message-----
From: Anders Björnerstedt [mailto:[email protected]]
Sent: den 20 mars 2015 10:21
To: Hung Nguyen D; Reddy Neelakanta
Cc: [email protected]
Subject: Re: [PATCH 1 of 2] imm: Revert the change on IMMSV_OI_CCB_UPCALL_RSP
in #969 [#1267]
Ack on this reversion patch 1 of 2.
I assume it reverts enough of #969 to allow upgrade from 4.5 to this changeset
to be successfully tested with protocol4.6 flag off. I have not tested that.
But this patch could be pushed now and ticket #1267 updated with the changeset
without closing #1267.
I am still reviewing the second patch (the new version of the actual
#969 enhancement).
I am having doubts about the solution of the embedded union in the
IMMSV_OI_CCB_UPCALL_RSP struct.
The alternative would be to add a new IMMSV_OCTET_STRING at the end of
CCB_UPCALL_RSP, preferrably with a name like 'objName' instead of octetString.
This could in theory cost some extra bytes, but UPCALL_RSP is embedded in the
bigger IMMND_EVT union so I doubt that.
The advantage is less change in legacy code and that in the future it would be
simpler to actually really remove the SaNameT type from these structs (the
intention of #969).
That would be done at some point in the future (OpenSAF 5.0 perhaps) where we
consolidate and shift to a complete new set of message types. An upgrade to
that system would need to be made in more than one step.
But it would allow the 5.0 code to be cleaned up from old legacy message
handling and the SaNameT type in the server, which was the point of enhancement
#969. Of course that would mean that the upgrade to 5.0 would not be "baseline
free".
But at some point we need to be able to remove support for very old and
obsolete message types.
/AndersBj
On 03/17/2015 01:42 PM, Hung Nguyen wrote:
> osaf/libs/agents/saf/imma/imma_oi_api.c | 19 +-----
> osaf/libs/agents/saf/imma/imma_proc.c | 16 +----
> osaf/libs/common/immsv/immsv_evt.c | 78
> +++++++++++++----------
> osaf/libs/common/immsv/include/immsv_evt_model.h | 2 +-
> osaf/services/saf/immsv/immnd/ImmModel.cc | 4 +-
> osaf/services/saf/immsv/immnd/immnd_evt.c | 13 +---
> 6 files changed, 50 insertions(+), 82 deletions(-)
>
>
> Revert the change on IMMSV_OI_CCB_UPCALL_RSP in #969
>
> diff --git a/osaf/libs/agents/saf/imma/imma_oi_api.c
> b/osaf/libs/agents/saf/imma/imma_oi_api.c
> --- a/osaf/libs/agents/saf/imma/imma_oi_api.c
> +++ b/osaf/libs/agents/saf/imma/imma_oi_api.c
> @@ -3621,7 +3621,6 @@ SaAisErrorT saImmOiAugmentCcbInitialize(
> SaVersionT version = {'A', 2, 11};
> SaUint32T adminOwnerId = 0;
> SaUint32T ccbId = 0;
> - IMMSV_OCTET_STRING *objectName = NULL;
>
> TRACE_ENTER();
>
> @@ -3745,17 +3744,7 @@ SaAisErrorT saImmOiAugmentCcbInitialize(
> init_evt.info.immnd.info.ccbUpcallRsp.ccbId = ccbId;
> init_evt.info.immnd.info.ccbUpcallRsp.implId = cbi->implId;
> init_evt.info.immnd.info.ccbUpcallRsp.inv = cbi->inv;
> -
> - objectName = &init_evt.info.immnd.info.ccbUpcallRsp.name;
> - size_t length = osaf_extended_name_length(&cbi->name);
> - objectName->buf = (char*) malloc((length + 1) * sizeof(char));
> - if (!objectName->buf) {
> - rc = SA_AIS_ERR_NO_MEMORY;
> - goto done;
> - }
> - memcpy(objectName->buf, osaf_extended_name_borrow(&cbi->name),
> length);
> - objectName->buf[length] = '\0';
> - objectName->size = length + 1;
> + init_evt.info.immnd.info.ccbUpcallRsp.name = cbi->name;
>
> /* Note that we register using the new OM handle as client for
> the aug-ccb */
> rc = imma_evt_fake_evs(cb, &init_evt, &out_evt,
> cl_node->syncr_timeout, @@ -3881,12 +3870,6 @@ SaAisErrorT
> saImmOiAugmentCcbInitialize(
> out_evt = NULL;
> }
>
> - if (objectName && objectName->buf) {
> - free(objectName->buf);
> - objectName->buf = NULL;
> - objectName->size = 0;
> - }
> -
> lock_fail:
> TRACE_LEAVE();
> return rc;
> diff --git a/osaf/libs/agents/saf/imma/imma_proc.c
> b/osaf/libs/agents/saf/imma/imma_proc.c
> --- a/osaf/libs/agents/saf/imma/imma_proc.c
> +++ b/osaf/libs/agents/saf/imma/imma_proc.c
> @@ -2618,17 +2618,7 @@ static bool imma_process_callback_info(I
> ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.oi_client_hdl =
> callback->lcl_imm_hdl;
> ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.ccbId = callback->ccbID;
> ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.inv =
> callback->inv;
> -
> - IMMSV_OCTET_STRING *objectName =
> &ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.name;
> - size_t length =
> osaf_extended_name_length(&callback->name);
> - objectName->buf = (char*) malloc((length + 1) *
> sizeof(char));
> - if (!objectName->buf) { /* Just log error and
> drop this reply. */
> - LOG_ER("Failed to allocate memory, drop this
> reply");
> - break;
> - }
> - memcpy(objectName->buf,
> osaf_extended_name_borrow(&callback->name), length);
> - objectName->buf[length] = '\0';
> - objectName->size = length + 1;
> +
> osaf_extended_name_lend(osaf_extended_name_borrow(&(callback->name)),
> &(ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.name));
>
> osafassert(m_NCS_LOCK(&cb->cb_lock,
> NCS_LOCK_WRITE) == NCSCC_RC_SUCCESS);
> locked = true;
> @@ -2676,10 +2666,6 @@ static bool imma_process_callback_info(I
> /*Cant do anything but log error and drop
> this reply. */
> TRACE_3("CcbObjectDeleteCallback: send reply
> to IMMND failed");
> }
> -
> - free(objectName->buf);
> - objectName->buf = NULL;
> - objectName->size = 0;
> } else {
> /* callback->inv == 0 means PBE (CCB or PRTO) or
> applier upcall, no reply. */
> osafassert(cl_node->isPbe || cl_node->isApplier);
> diff --git a/osaf/libs/common/immsv/immsv_evt.c
> b/osaf/libs/common/immsv/immsv_evt.c
> --- a/osaf/libs/common/immsv/immsv_evt.c
> +++ b/osaf/libs/common/immsv/immsv_evt.c
> @@ -1991,27 +1991,14 @@ static uint32_t immsv_evt_enc_sublevels(
> */
> } else if ((i_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_COMPLETED_RSP_2) ||
> (i_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_MODIFY_RSP_2) ||
> - (i_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_CREATE_RSP_2)) {
> + (i_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_CREATE_RSP_2) ||
> + (i_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2))
> + {
> IMMSV_OCTET_STRING *os =
> &(i_evt->info.immnd.info.ccbUpcallRsp.errorString);
> if(!immsv_evt_enc_inline_text(__LINE__, o_ub, os)) {
> return NCSCC_RC_OUT_OF_MEM;
> }
> - } else if (i_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2) {
> - IMMSV_OCTET_STRING *os =
> &(i_evt->info.immnd.info.ccbUpcallRsp.errorString);
> - if(!immsv_evt_enc_inline_text(__LINE__, o_ub, os)) {
> - return NCSCC_RC_OUT_OF_MEM;
> - }
> - os = &(i_evt->info.immnd.info.ccbUpcallRsp.name);
> - if(!immsv_evt_enc_inline_text(__LINE__, o_ub, os)) {
> - return NCSCC_RC_OUT_OF_MEM;
> - }
> - } else if ((i_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP) ||
> - (i_evt->info.immnd.type ==
> IMMND_EVT_A2ND_OI_CCB_AUG_INIT)) {
> - IMMSV_OCTET_STRING *os =
> &(i_evt->info.immnd.info.ccbUpcallRsp.name);
> - if(!immsv_evt_enc_inline_text(__LINE__, o_ub, os)) {
> - return NCSCC_RC_OUT_OF_MEM;
> - }
> } else if (i_evt->info.immnd.type ==
> IMMND_EVT_A2ND_IMM_ADMINIT) {
> IMMSV_OCTET_STRING *os =
> &(i_evt->info.immnd.info.adminitReq.i.adminOwnerName.octetString);
> if (strlen(os->buf) >= IMMSV_MAX_ADMO_NAME_LENGTH) { @@
> -2539,19 +2526,12 @@ static uint32_t immsv_evt_dec_sublevels(
> }
> } else if ((o_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_COMPLETED_RSP_2) ||
> (o_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_MODIFY_RSP_2) ||
> - (o_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_CREATE_RSP_2)) {
> + (o_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_CREATE_RSP_2) ||
> + (o_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2))
> + {
> IMMSV_OCTET_STRING *os =
> &(o_evt->info.immnd.info.ccbUpcallRsp.errorString);
> immsv_evt_dec_inline_string(i_ub, os);
> - } else if (o_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2) {
> - IMMSV_OCTET_STRING *os =
> &(o_evt->info.immnd.info.ccbUpcallRsp.errorString);
> - immsv_evt_dec_inline_string(i_ub, os);
> - os = &(o_evt->info.immnd.info.ccbUpcallRsp.name);
> - immsv_evt_dec_inline_string(i_ub, os);
> - } else if ((o_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP) ||
> - (o_evt->info.immnd.type ==
> IMMND_EVT_A2ND_OI_CCB_AUG_INIT)) {
> - IMMSV_OCTET_STRING *os =
> &(o_evt->info.immnd.info.ccbUpcallRsp.name);
> - immsv_evt_dec_inline_string(i_ub, os);
> } else if (o_evt->info.immnd.type ==
> IMMND_EVT_A2ND_IMM_ADMINIT) {
> IMMSV_OCTET_STRING *os =
> &(o_evt->info.immnd.info.adminitReq.i.adminOwnerName.octetString);
> immsv_evt_dec_inline_string(i_ub, os); @@ -3681,13
> +3661,27 @@ static uint32_t immsv_evt_enc_toplevel(I
> ncs_encode_32bit(&p8, immndevt->info.ccbUpcallRsp.result);
> ncs_enc_claim_space(o_ub, 4);
>
> + IMMSV_RSRV_SPACE_ASSERT(p8, o_ub, 2);
> + const char* value =
> osaf_extended_name_borrow(&immndevt->info.ccbUpcallRsp.name);
> + size_t length = strlen(value);
> + osafassert(length <= 0xffff);
> + ncs_encode_16bit(&p8, length);
> + ncs_enc_claim_space(o_ub, 2);
> +
> + /* name.value is top level because type is SaNameT */
> if((immndevt->type == IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP) ||
> (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2) ||
> (immndevt->type == IMMND_EVT_A2ND_OI_CCB_AUG_INIT)) {
> - IMMSV_RSRV_SPACE_ASSERT(p8, o_ub, 4);
> - ncs_encode_32bit(&p8,
> immndevt->info.ccbUpcallRsp.name.size);
> - ncs_enc_claim_space(o_ub, 4);
> - /* immndevt->info.ccbUpcallRsp.name.buf encoded by
> sublevel */
> + if (length) {
> + if(ncs_encode_n_octets_in_uba(o_ub,
> + (uint8_t*) value,
> + length)
> + != NCSCC_RC_SUCCESS) {
> + LOG_WA("Failure inside
> ncs_encode_n_octets_in_uba");
> + rc = NCSCC_RC_FAILURE;
> + break;
> + }
> + }
> }
>
> if((immndevt->type == IMMND_EVT_A2ND_CCB_COMPLETED_RSP_2)
> || @@ -5072,13 +5066,29 @@ static uint32_t immsv_evt_dec_toplevel(N
> immndevt->info.ccbUpcallRsp.result = ncs_decode_32bit(&p8);
> ncs_dec_skip_space(i_ub, 4);
>
> + IMMSV_FLTN_SPACE_ASSERT(p8, local_data, i_ub, 2);
> + size_t length = ncs_decode_16bit(&p8);
> + ncs_dec_skip_space(i_ub, 2);
> +
> + osaf_extended_name_clear(&immndevt->info.ccbUpcallRsp.name);
> if((immndevt->type == IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP) ||
> (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2) ||
> (immndevt->type == IMMND_EVT_A2ND_OI_CCB_AUG_INIT)) {
> - IMMSV_FLTN_SPACE_ASSERT(p8, local_data, i_ub, 4);
> - immndevt->info.ccbUpcallRsp.name.size =
> ncs_decode_32bit(&p8);
> - ncs_dec_skip_space(i_ub, 4);
> - /* immndevt->info.ccbUpcallRsp.name.buf decoded by
> sublevel */
> + /* name.value is top level because type is SaNameT */
> + if (length) {
> + char* value = (char*) malloc(length + 1);
> + if (value == NULL ||
> ncs_decode_n_octets_from_uba(i_ub,
> + (uint8_t*) value,
> + length) !=
> + NCSCC_RC_SUCCESS) {
> + free(value);
> + LOG_WA("Failure inside
> ncs_decode_n_octets_from_uba");
> + rc = NCSCC_RC_FAILURE;
> + break;
> + }
> + value[length] = '\0';
> + osaf_extended_name_steal(value,
> &immndevt->info.ccbUpcallRsp.name);
> + }
> }
>
> if((immndevt->type == IMMND_EVT_A2ND_CCB_COMPLETED_RSP_2)
> || diff --git a/osaf/libs/common/immsv/include/immsv_evt_model.h
> b/osaf/libs/common/immsv/include/immsv_evt_model.h
> --- a/osaf/libs/common/immsv/include/immsv_evt_model.h
> +++ b/osaf/libs/common/immsv/include/immsv_evt_model.h
> @@ -288,7 +288,7 @@ extern "C" {
> SaUint32T implId;
> SaUint32T inv;
> SaAisErrorT result;
> - IMMSV_OCTET_STRING name;
> + SaNameT name;
> IMMSV_OCTET_STRING errorString;
> } IMMSV_OI_CCB_UPCALL_RSP;
>
> diff --git a/osaf/services/saf/immsv/immnd/ImmModel.cc
> b/osaf/services/saf/immsv/immnd/ImmModel.cc
> --- a/osaf/services/saf/immsv/immnd/ImmModel.cc
> +++ b/osaf/services/saf/immsv/immnd/ImmModel.cc
> @@ -5959,7 +5959,7 @@ ImmModel::ccbAugmentInit(immsv_oi_ccb_up
>
> TRACE_ENTER();
> /*Note: objectName is parent-name for the create case! */
> - std::string objectName(rsp->name.buf);
> + std::string objectName(osaf_extended_name_borrow(&rsp->name));
> osafassert(nameCheck(objectName)||nameToInternal(objectName));
>
> i = std::find_if(sCcbVector.begin(), sCcbVector.end(),
> CcbIdIs(ccbId)); @@ -9611,7 +9611,7 @@
> ImmModel::ccbObjDelContinuation(immsv_oi
> SaUint32T* reqConn, bool* augDelete) {
> TRACE_ENTER();
> - std::string objectName(rsp->name.buf);
> + std::string objectName(osaf_extended_name_borrow(&rsp->name));
>
> SaUint32T ccbId = rsp->ccbId;
> CcbInfo* ccb = 0;
> diff --git a/osaf/services/saf/immsv/immnd/immnd_evt.c
> b/osaf/services/saf/immsv/immnd/immnd_evt.c
> --- a/osaf/services/saf/immsv/immnd/immnd_evt.c
> +++ b/osaf/services/saf/immsv/immnd/immnd_evt.c
> @@ -470,23 +470,12 @@ uint32_t immnd_evt_destroy(IMMSV_EVT *ev
> immsv_evt_free_name_list(evt->info.immnd.info.admReq.objectNames);
> evt->info.immnd.info.admReq.objectNames = NULL;
> } else if ((evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_CREATE_RSP_2) ||
> + (evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2) ||
> (evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_MODIFY_RSP_2) ||
> (evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_COMPLETED_RSP_2)) {
> free(evt->info.immnd.info.ccbUpcallRsp.errorString.buf);
> evt->info.immnd.info.ccbUpcallRsp.errorString.buf = NULL;
> evt->info.immnd.info.ccbUpcallRsp.errorString.size = 0;
> - } else if (evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2) {
> - free(evt->info.immnd.info.ccbUpcallRsp.errorString.buf);
> - evt->info.immnd.info.ccbUpcallRsp.errorString.buf = NULL;
> - evt->info.immnd.info.ccbUpcallRsp.errorString.size = 0;
> - free(evt->info.immnd.info.ccbUpcallRsp.name.buf);
> - evt->info.immnd.info.ccbUpcallRsp.name.buf = NULL;
> - evt->info.immnd.info.ccbUpcallRsp.name.size = 0;
> - } else if ((evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP) ||
> - (evt->info.immnd.type == IMMND_EVT_A2ND_OI_CCB_AUG_INIT)) {
> - free(evt->info.immnd.info.ccbUpcallRsp.name.buf);
> - evt->info.immnd.info.ccbUpcallRsp.name.buf = NULL;
> - evt->info.immnd.info.ccbUpcallRsp.name.size = 0;
> } else if (evt->info.immnd.type == IMMND_EVT_A2ND_IMM_ADMINIT) {
> free(evt->info.immnd.info.adminitReq.i.adminOwnerName.octetString.buf)
> ;
> evt->info.immnd.info.adminitReq.i.adminOwnerName.octetString.buf =
> evt->NULL;
>
------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the
conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
