Hi Hung,
I am sorry but I have to NACK this patch version also :-(
I can understand if that is enough to make you give up on this
enhancement, but please read the review comments belwo.
The patch is probably functionally correct, but it adds too much
unnecessary memory allocation/de-allocation
and unnecessary string copying.
The original purpose of enhancement #969 was to remove SaNameT from the
imm server side and to prepare
for a distant future removal of SaNameT in messages between client and
server.
The intention was/is to *simplify* the code, except possibly in places
where there is missing a check/correction.
for SaNameT handling in the old code.
This not a high priority enhancement, so one option is just to have it
totally reverted.
Seen in hindsight I should have agreed to Neel's suggestionl on that
point some weeks ago
and just had it reverted out of 4.6. But now we have spent so much time
on this enhancement that as long as the
final fix is a reduction of the current patch(es) we may as well do it.
One thing that confuses me is that I though the reversion patch already
pushed (changesets 6382:6392aa458a95 and
6381:6c6da04af99f) really reverted all of #969. But it appears that it
only reverted the callback-reply message handling
and not the admin-owner message handling.
I therefore missed that you had the same embedded union solution in both
your new versions of the admin-owner message
handling. I dont like that embedded union solution for the same reason
that I did not like it in the callback-reply handling,
it cements the SaNameT dependence into the *new* message type instead of
preparing for its removal.
I also see that this review request has two patches, one new patch for
the callback-reply handling.
But also one new patch for the admin-owner handling. The second patch
however does not illustrate
the change relative to 4.5 but the change relative to the previous fix
for #969 (not reverted out).
This makes it extra difficult for the reviewer (me in this case) to see
what you are changing (hopefully simplifying)
relative to 4.5 in that patch.
We agree that we need to secure NULL termination of a string obtained
from an SaNameT value if and *only* if that
value arrives over the IMMA API from the api-user. This since we can not
rely on the user to guarantee null termination.
But in the code below dealing with replies to callback *from* the
server, the SaNameT value has arrived from the server
in the callback. So you must trust the value to be null-terminated and
simply use osaf_extended_name_borrow.
This should have been the case in the original old code and I dont want
to see a new version of the code that degrades
performance for no reason at all.
The good news is that you should in essence be able to remove a lot of
code from this patch.
The reason for the debacle arround this ticket was that when it was
originally sent for review, then Zoran was bussy
with the SaStringT API enhancement and I was busy with the prototyping
for headless (not in OpenSAF currently)
and the compressed 6 month release cycle signifficantly increases the
overhead percentage for each release.
I am not even sure if I ever reviewed and acke'd the original patches
for this ticket.
Additional comments inline.
On 03/24/2015 12:38 PM, Hung Nguyen wrote:
> osaf/libs/agents/saf/imma/imma_oi_api.c | 21 +++-
> osaf/libs/agents/saf/imma/imma_proc.c | 20 ++-
> osaf/libs/common/immsv/immsv_evt.c | 72 ++++++++++-
> osaf/libs/common/immsv/include/immsv_evt.h | 4 +
> osaf/libs/common/immsv/include/immsv_evt_model.h | 3 +-
> osaf/services/saf/immsv/immnd/ImmModel.cc | 4 +-
> osaf/services/saf/immsv/immnd/immnd_evt.c | 143
> ++++++++++++++++++++++-
> 7 files changed, 248 insertions(+), 19 deletions(-)
>
>
> Add new IMMSV_OCTET_STRING member to IMMSV_OI_CCB_UPCALL_RSP.
> Introduce new message types: IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_3,
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4, IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2.
> Add encoding/decoding for new message types.
> SaNameT is converted to IMMSV_OCTET_STRING when decoding old-typed messages.
> New-typed messages are only forwarded to IMMD when protocol46 is enabled.
> When protocol46 is disabled, the messages are converted to the old type
> before forwarding to IMMD, this is done in immnd_fevs_local_checks().
>
> 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,6 +3621,7 @@ SaAisErrorT saImmOiAugmentCcbInitialize(
> SaVersionT version = {'A', 2, 11};
> SaUint32T adminOwnerId = 0;
> SaUint32T ccbId = 0;
> + IMMSV_OCTET_STRING *objectName = NULL;
>
> TRACE_ENTER();
>
> @@ -3740,11 +3741,21 @@ SaAisErrorT saImmOiAugmentCcbInitialize(
> /* populate the EVT structure */
> memset(&init_evt, 0, sizeof(IMMSV_EVT));
> init_evt.type = IMMSV_EVT_TYPE_IMMND;
> - init_evt.info.immnd.type = IMMND_EVT_A2ND_OI_CCB_AUG_INIT;
> + init_evt.info.immnd.type = IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2;
> 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;
> - init_evt.info.immnd.info.ccbUpcallRsp.name = cbi->name;
The whole business of allocating heap memory and freeing is unnecessary
here.
You can just use osaf_extended_name_borrow() becaue the SaNameT value is
internal here,
arrived via callback from the server.
It is not arriving from the client and so has to be null-terminated
already.
Otherwise we have a bug in our code and should track down and fix that bug.
> +
> + objectName = &init_evt.info.immnd.info.ccbUpcallRsp.name;
> + size_t objectNameLength = osaf_extended_name_length(&cbi->name);
> + objectName->buf = (char*) malloc((objectNameLength + 1) * sizeof(char));
> + if (!objectName->buf) {
> + rc = SA_AIS_ERR_NO_MEMORY;
> + goto done;
> + }
> + memcpy(objectName->buf, osaf_extended_name_borrow(&cbi->name),
> objectNameLength);
> + objectName->buf[objectNameLength] = '\0';
> + objectName->size = objectNameLength + 1;
>
> /* 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,
> @@ -3870,6 +3881,12 @@ 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
> @@ -2613,12 +2613,22 @@ static bool imma_process_callback_info(I
> SaImmHandleT privateAugOmHandle = 0LL;
> memset(&ccbObjDelRpl, 0,
> sizeof(IMMSV_EVT));
> ccbObjDelRpl.type =
> IMMSV_EVT_TYPE_IMMND;
> - ccbObjDelRpl.info.immnd.type =
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP;
> + ccbObjDelRpl.info.immnd.type =
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_3;
>
> ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.result = localEr;
>
> 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;
> -
> osaf_extended_name_lend(osaf_extended_name_borrow(&(callback->name)),
> &(ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.name));
Same comment as above for the code below. The callback->name is from
the server and so has to be null-terminated.
just use borrow, which should be the same as the old code.
in general I never want to see any new memory allocations and null
termination checks, except if it is missing right at the users API.
> +
> + IMMSV_OCTET_STRING *objectName =
> &ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.name;
> + size_t objectNameLength =
> osaf_extended_name_length(&callback->name);
> + objectName->buf = (char*)
> malloc((objectNameLength + 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), objectNameLength);
> + objectName->buf[objectNameLength] =
> '\0';
> + objectName->size = objectNameLength + 1;
>
> osafassert(m_NCS_LOCK(&cb->cb_lock,
> NCS_LOCK_WRITE) == NCSCC_RC_SUCCESS);
> locked = true;
> @@ -2628,7 +2638,7 @@ static bool imma_process_callback_info(I
>
> if (localEr != SA_AIS_OK) {
> if(errorStr) {
> -
> ccbObjDelRpl.info.immnd.type = IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2;
> +
> ccbObjDelRpl.info.immnd.type = IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4;
>
> ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.errorString.size =
>
> strlen(errorStr) + 1;
>
> ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.errorString.buf = errorStr;
> @@ -2666,6 +2676,10 @@ 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
> @@ -183,6 +183,9 @@ static const char *immnd_evt_names[] = {
> "IMMND_EVT_D2ND_ADMINIT_2", /* Admin Owner init reply */
> "IMMND_EVT_A2ND_OBJ_CREATE_2", /* saImmOmCcbObjectCreate_o3 */
> "IMMND_EVT_A2ND_OI_OBJ_CREATE_2", /* saImmOiRtObjectCreate_o3 */
> + "IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_3", /*CcbObjDelete local Reply */
> + "IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4", /*CcbObjDelete local Reply */
> + "IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2",
> "undefined (high)"
> };
>
> @@ -1999,6 +2002,19 @@ static uint32_t immsv_evt_enc_sublevels(
> 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_3) ||
> + (i_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4) ||
> + (i_evt->info.immnd.type ==
> IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2)) {
> + 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;
> + }
> + if (i_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4) {
> + 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_IMM_ADMINIT) {
> IMMSV_OCTET_STRING *os =
> &(i_evt->info.immnd.info.adminitReq.i.adminOwnerName.octetString);
> if (strlen(os->buf) >= IMMSV_MAX_ADMO_NAME_LENGTH) {
> @@ -2532,6 +2548,15 @@ static uint32_t immsv_evt_dec_sublevels(
> 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_3) ||
> + (o_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4) ||
> + (o_evt->info.immnd.type ==
> IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2)) {
> + IMMSV_OCTET_STRING *os =
> &(o_evt->info.immnd.info.ccbUpcallRsp.name);
> + immsv_evt_dec_inline_string(i_ub, os);
> + if (o_evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4) {
> + 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_IMM_ADMINIT) {
> IMMSV_OCTET_STRING *os =
> &(o_evt->info.immnd.info.adminitReq.i.adminOwnerName.octetString);
> immsv_evt_dec_inline_string(i_ub, os);
> @@ -3640,6 +3665,9 @@ static uint32_t immsv_evt_enc_toplevel(I
> case IMMND_EVT_A2ND_PBE_PRTO_DELETES_COMPLETED_RSP:/*Pbe PRTO
> deletes done */
> case IMMND_EVT_A2ND_PBE_PRT_ATTR_UPDATE_RSP:/* Pbe OI rt attr
> update response*/
> case IMMND_EVT_A2ND_OI_CCB_AUG_INIT:/*OI augments CCB inside
> ccb upcall. #1963 */
> + case IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_3: /*CcbObjDelete
> local Reply */
> + case IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4: /*CcbObjDelete
> local Reply */
> + case IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2: /*OI augments CCB
> inside ccb upcall */
> {
> IMMSV_RSRV_SPACE_ASSERT(p8, o_ub, 8);
> ncs_encode_64bit(&p8,
> immndevt->info.ccbUpcallRsp.oi_client_hdl);
> @@ -3662,13 +3690,13 @@ static uint32_t immsv_evt_enc_toplevel(I
> 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);
> + const char* value =
> osaf_extended_name_borrow(&immndevt->info.ccbUpcallRsp.saName);
One thing I dont like here is that you change the name "name" of the
*old* member and
use the name "name" for the new member! Now you have to alter the code
also for
the old message type. I guess the intention was to make clear that it is
an SaNameT.
But lets not add complexity to this change by altering old code that is
working which.
Better instead to name the new member "stringName" or similar and let
the old member
keep the original name "name" and leave the old code as undisturbed as
possible.
> 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 */
> + /* saName.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)) {
> @@ -3684,10 +3712,20 @@ static uint32_t immsv_evt_enc_toplevel(I
> }
> }
>
> + if ((immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_3) ||
> + (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4) ||
> + (immndevt->type ==
> IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2)) {
> + 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((immndevt->type ==
> IMMND_EVT_A2ND_CCB_COMPLETED_RSP_2) ||
> (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_CREATE_RSP_2) ||
> (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2) ||
> - (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_MODIFY_RSP_2)) {
> + (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_MODIFY_RSP_2) ||
> + (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4)) {
> IMMSV_RSRV_SPACE_ASSERT(p8, o_ub, 4);
> ncs_encode_32bit(&p8,
> immndevt->info.ccbUpcallRsp.errorString.size);
> ncs_enc_claim_space(o_ub, 4);
> @@ -5045,6 +5083,9 @@ static uint32_t immsv_evt_dec_toplevel(N
> case IMMND_EVT_A2ND_PBE_PRTO_DELETES_COMPLETED_RSP:/*Pbe PRTO
> deletes done */
> case IMMND_EVT_A2ND_PBE_PRT_ATTR_UPDATE_RSP:/* Pbe OI rt attr
> update response*/
> case IMMND_EVT_A2ND_OI_CCB_AUG_INIT:/*OI augments CCB inside
> ccb upcall. #1963 */
> + case IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_3: /*CcbObjDelete
> local Reply */
> + case IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4: /*CcbObjDelete
> local Reply */
> + case IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2: /*OI augments CCB
> inside ccb upcall */
> {
> IMMSV_FLTN_SPACE_ASSERT(p8, local_data, i_ub, 8);
> immndevt->info.ccbUpcallRsp.oi_client_hdl =
> ncs_decode_64bit(&p8);
> @@ -5070,13 +5111,13 @@ static uint32_t immsv_evt_dec_toplevel(N
> 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)) {
Please keep the old name for the old name
> - /* name.value is top level because type is
> SaNameT */
> + /* saName.value is top level because type is
> SaNameT */
> + /* length can be zero when there's no parent
> name */
> + char* value = (char*) malloc(length + 1);
> if (length) {
> - char* value = (char*) malloc(length +
> 1);
> if (value == NULL ||
> ncs_decode_n_octets_from_uba(i_ub,
> (uint8_t*) value,
> length) !=
> @@ -5086,15 +5127,28 @@ static uint32_t immsv_evt_dec_toplevel(N
> rc = NCSCC_RC_FAILURE;
> break;
> }
> - value[length] = '\0';
> - osaf_extended_name_steal(value,
> &immndevt->info.ccbUpcallRsp.name);
> }
> + value[length] = '\0';
> +
> + /* Convert to new type */
If you are converting to the new type then I think you should change
immndevt->type to the
respective new type.
An alternative is to avoid any new message type at all, but I think this
is more confusing and
could be bad if we have any bugs in the new hansling.
As you can probably understand by now I am for keeping old stuff clear
as being old stuff
and avoiding renaming old stuff to new names because it is confusing
enough as it is even
hen we dont relabel old stuff. We also create a larger diff than necessary.
Cleanup etc can be done but if cleanup is to be done it should be a pure
cleanup patch.
> + immndevt->info.ccbUpcallRsp.name.size = length
> + 1;
> + immndevt->info.ccbUpcallRsp.name.buf = value;
> + }
> +
> + if ((immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_3) ||
> + (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4) ||
> + (immndevt->type ==
> IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2)) {
> + 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 */
> }
>
> if((immndevt->type ==
> IMMND_EVT_A2ND_CCB_COMPLETED_RSP_2) ||
> (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_CREATE_RSP_2) ||
> (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2) ||
> - (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_MODIFY_RSP_2)) {
> + (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_MODIFY_RSP_2) ||
> + (immndevt->type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4)) {
> IMMSV_FLTN_SPACE_ASSERT(p8, local_data, i_ub,
> 4);
> immndevt->info.ccbUpcallRsp.errorString.size =
> ncs_decode_32bit(&p8);
> ncs_dec_skip_space(i_ub, 4);
> diff --git a/osaf/libs/common/immsv/include/immsv_evt.h
> b/osaf/libs/common/immsv/include/immsv_evt.h
> --- a/osaf/libs/common/immsv/include/immsv_evt.h
> +++ b/osaf/libs/common/immsv/include/immsv_evt.h
> @@ -216,6 +216,10 @@ typedef enum immnd_evt_type {
> IMMND_EVT_A2ND_OBJ_CREATE_2 = 99, /* saImmOmCcbObjectCreate_o3 */
> IMMND_EVT_A2ND_OI_OBJ_CREATE_2 = 100, /* saImmOiRtObjectCreate_o3 */
>
> + IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_3 = 101, /*CcbObjDelete local
> Reply */
> + IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4 = 102, /*CcbObjDelete local
> Reply */
> + IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2 = 103, /* Init of OI augmented ccb
> handle */
> +
> IMMND_EVT_MAX
> } IMMND_EVT_TYPE;
> /* Make sure the string array in immsv_evt.c matches the IMMND_EVT_TYPE
> enum. */
> 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,8 +288,9 @@ extern "C" {
> SaUint32T implId;
> SaUint32T inv;
> SaAisErrorT result;
Please keep the old name for the old "name" member and choose
a new different name for the new "name" member.
> - SaNameT name;
> + SaNameT saName;
> IMMSV_OCTET_STRING errorString;
> + IMMSV_OCTET_STRING name;
> } IMMSV_OI_CCB_UPCALL_RSP;
>
> typedef struct ImmsvSyncFevsBase {
> 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(osaf_extended_name_borrow(&rsp->name));
> + std::string objectName(rsp->name.buf);
> 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(osaf_extended_name_borrow(&rsp->name));
> + std::string objectName(rsp->name.buf);
>
> 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,7 +470,6 @@ 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);
> @@ -485,6 +484,21 @@ uint32_t immnd_evt_destroy(IMMSV_EVT *ev
>
> free(evt->info.immnd.info.adminitGlobal.i.adminOwnerName.octetString.buf);
>
> evt->info.immnd.info.adminitGlobal.i.adminOwnerName.octetString.buf = NULL;
>
> evt->info.immnd.info.adminitGlobal.i.adminOwnerName.octetString.size = 0;
> + } else if ((evt->info.immnd.type == IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP)
> ||
> + (evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2) ||
> + (evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_3) ||
> + (evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4) ||
> + (evt->info.immnd.type ==
> IMMND_EVT_A2ND_OI_CCB_AUG_INIT) ||
> + (evt->info.immnd.type ==
> IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2)) {
> + free(evt->info.immnd.info.ccbUpcallRsp.name.buf);
> + evt->info.immnd.info.ccbUpcallRsp.name.buf = NULL;
> + evt->info.immnd.info.ccbUpcallRsp.name.size = 0;
> + if ((evt->info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4) ||
> + (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;
> + }
> }
I would like to avoid the solution below of re-coding fevs messages to
the old message type in fevs_local_checks.
Several solutions are possible but one thing to remember here is that we
are in this case not trying to
discriminate between any new or old functionality. Thus here it is not
necessary to have quick
reactivity to protocol46Allowed for these two cases:
* aug-init from inside OI callback
* reply from delete-callback
That is, it is no big problem if some users continue to use the old
SaNameT based message protocols for
these two message types to the server, indefinitely.
I suggest adding one new member to IMMSV_ND2A_INIT_RSP in immsv_evt.h:
typedef struct immsv_nd2a_init_rsp {
SaImmHandleT immHandle;
SaAisErrorT error;
SaVersionT version; <========== new member
} IMMSV_ND2A_INIT_RSP;
This struct is replied on the request for OM or OI handle initialize.
You would implement encode/(decode for the new version member.
But you would for now only need to set to A.2.15 on OI initialize if and
only if protocol46 is allowed
in the server when the request arrives in the server.
This means that all OI handles that are initialized before protocol 46
is allowed would get it set to _.0.0
or to A.2.14. The important thing is that it is set to A.2.15 only
after/when an initialize is done after
protocol46 is allowed on the server and not set to this value before that.
This value (or just a bool) needs to be stored in the client_node struct
when the reply from oi-initialize is received,
if the error code is OK.
I suggest adding yet one more boolean member:
bool isApplier; /* True => This is an Applier-OI */
bool isAug; /* True => handle internal to OI augmented CCB */
bool isBusy; /* True => handle is locked by a thread until a
function execution is done */
bool isProto46; /* True => If true then the server side supported
protocol46 at handle initialize */ <=========
.....
} IMMA_CLIENT_NODE;
This means that the OI clients that initialize early will never use the
new protocol46 messages
when they initialize before upgrade to 46 is completed. That is until
they restart or re-initialize
a new OI handle AFTER the upgrade to 46 is done.
Again this is not a problem.
After the upgrade is complete and forever thereafter on that system, all
OI clients will
get this flag set, even after a cluster restart.
I prefer such a solution because it is cleaner and generic, can be used
also to support other
upgrade issues at the client, independently of this issue and avoids
adding lots of code that is
never used except during a very short period in the life of the IMM code.
>
> if (onheap) {
> @@ -3208,8 +3222,67 @@ static SaAisErrorT immnd_fevs_local_chec
> case IMMND_EVT_A2ND_OBJ_CREATE:
> case IMMND_EVT_A2ND_OBJ_DELETE:
> case IMMND_EVT_A2ND_CCB_FINALIZE:
> + case IMMND_EVT_A2ND_AUG_ADMO:
> + if(immModel_pbeNotWritable(cb)) {
> + error = SA_AIS_ERR_TRY_AGAIN;
> + }
> + break;
> +
> + case IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2:
> + if (!immModel_protocol46Allowed(cb)) {
> + TRACE("IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2 with protocol46
> disabled");
> + IMMSV_EVT augInitEvt;
> + uint32_t rc = 0;
> + NCS_UBAID ubaid;
> + ubaid.start = NULL;
> + int32_t size = 0;
> + char *tmpData = NULL;
> + char *data = NULL;
> +
> + /* Convert from A2ND_OI_CCB_AUG_INIT_2 to
> A2ND_OI_CCB_AUG_INIT */
> + memset(&augInitEvt, 0, sizeof(IMMSV_EVT));
> + augInitEvt.type = IMMSV_EVT_TYPE_IMMND;
> + augInitEvt.info.immnd.type =
> IMMND_EVT_A2ND_OI_CCB_AUG_INIT;
> + augInitEvt.info.immnd.info.ccbUpcallRsp.ccbId =
> frwrd_evt.info.immnd.info.ccbUpcallRsp.ccbId;
> + augInitEvt.info.immnd.info.ccbUpcallRsp.implId =
> frwrd_evt.info.immnd.info.ccbUpcallRsp.implId;
> + augInitEvt.info.immnd.info.ccbUpcallRsp.inv =
> frwrd_evt.info.immnd.info.ccbUpcallRsp.inv;
> +
> osaf_extended_name_lend(frwrd_evt.info.immnd.info.ccbUpcallRsp.name.buf,
> +
> &augInitEvt.info.immnd.info.ccbUpcallRsp.saName);
> +
> + /* Encode the message */
> + rc = ncs_enc_init_space(&ubaid);
> + if (rc != NCSCC_RC_SUCCESS) {
> + TRACE_2("ERR_LIBRARY: Failed init ubaid");
> + error = SA_AIS_ERR_LIBRARY;
> + break;
> + }
> + rc = immsv_evt_enc(&augInitEvt, &ubaid);
> + if (rc != NCSCC_RC_SUCCESS) {
> + TRACE_2("ERR_LIBRARY: Failed to pre-pack");
> + error = SA_AIS_ERR_LIBRARY;
> + break;
> + }
> +
> + /* Free memory allocated for A2ND_OI_CCB_AUG_INIT_2 */
> + free(fevsReq->msg.buf);
> + fevsReq->msg.buf = NULL;
> +
> + /* Allocate memory to store new message */
> + size = ubaid.ttl;
> + tmpData = malloc(size);
> + osafassert(tmpData);
> + data = m_MMGR_DATA_AT_START(ubaid.start, size, tmpData);
> + fevsReq->msg.size = size;
> + fevsReq->msg.buf = malloc(size);
> + osafassert(fevsReq->msg.buf);
> + memcpy(fevsReq->msg.buf, data, size);
> +
> + free(tmpData);
> + tmpData = NULL;
> + if (ubaid.start) m_MMGR_FREE_BUFR_LIST(ubaid.start);
> + }
> + /* intentional fall through */
> case IMMND_EVT_A2ND_OI_CCB_AUG_INIT:
> - case IMMND_EVT_A2ND_AUG_ADMO:
> if(immModel_pbeNotWritable(cb)) {
> error = SA_AIS_ERR_TRY_AGAIN;
> }
> @@ -3517,6 +3590,69 @@ static SaAisErrorT immnd_fevs_local_chec
> }
> break;
>
> + case IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_3:
> + case IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4:
> + if (!immModel_protocol46Allowed(cb)) {
> +
> TRACE("A2ND_CCB_OBJ_DELETE_RSP_3/A2ND_CCB_OBJ_DELETE_RSP_4 with protocol46
> disabled");
> + IMMSV_EVT ccbObjDelRpl;
> + uint32_t rc = 0;
> + NCS_UBAID ubaid;
> + ubaid.start = NULL;
> + int32_t size = 0;
> + char *tmpData = NULL;
> + char *data = NULL;
> +
> + /* Convert from
> A2ND_CCB_OBJ_DELETE_RSP_3/A2ND_CCB_OBJ_DELETE_RSP_4
> + * to A2ND_CCB_OBJ_DELETE_RSP/A2ND_CCB_OBJ_DELETE_RSP_2
> */
> + memset(&ccbObjDelRpl, 0, sizeof(IMMSV_EVT));
> + ccbObjDelRpl.type = IMMSV_EVT_TYPE_IMMND;
> + ccbObjDelRpl.info.immnd.type =
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP;
> + ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.result =
> frwrd_evt.info.immnd.info.ccbUpcallRsp.result;
> + ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.oi_client_hdl
> = frwrd_evt.info.immnd.info.ccbUpcallRsp.oi_client_hdl;
> + ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.ccbId =
> frwrd_evt.info.immnd.info.ccbUpcallRsp.ccbId;
> + ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.inv =
> frwrd_evt.info.immnd.info.ccbUpcallRsp.inv;
> +
> osaf_extended_name_lend(frwrd_evt.info.immnd.info.ccbUpcallRsp.name.buf,
> +
> &ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.saName);
> +
> + if (frwrd_evt.info.immnd.type ==
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4) {
> + ccbObjDelRpl.info.immnd.type =
> IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2;
> +
> ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.errorString.size =
> frwrd_evt.info.immnd.info.ccbUpcallRsp.errorString.size;
> +
> ccbObjDelRpl.info.immnd.info.ccbUpcallRsp.errorString.buf =
> frwrd_evt.info.immnd.info.ccbUpcallRsp.errorString.buf;
> + }
> +
> + /* Encode new message */
> + rc = ncs_enc_init_space(&ubaid);
> + if (rc != NCSCC_RC_SUCCESS) {
> + TRACE_2("ERR_LIBRARY: Failed init ubaid");
> + error = SA_AIS_ERR_LIBRARY;
> + break;
> + }
> + rc = immsv_evt_enc(&ccbObjDelRpl, &ubaid);
> + if (rc != NCSCC_RC_SUCCESS) {
> + TRACE_2("ERR_LIBRARY: Failed to pre-pack");
> + error = SA_AIS_ERR_LIBRARY;
> + break;
> + }
> +
> + /* Free memory allocated for
> A2ND_CCB_OBJ_DELETE_RSP_3/A2ND_CCB_OBJ_DELETE_RSP_4 */
> + free(fevsReq->msg.buf);
> + fevsReq->msg.buf = NULL;
> +
> + /* Allocate memory to store new message */
> + size = ubaid.ttl;
> + tmpData = malloc(size);
> + osafassert(tmpData);
> + data = m_MMGR_DATA_AT_START(ubaid.start, size, tmpData);
> + fevsReq->msg.size = size;
> + fevsReq->msg.buf = malloc(size);
> + osafassert(fevsReq->msg.buf);
> + memcpy(fevsReq->msg.buf, data, size);
> +
> + free(tmpData);
> + tmpData = NULL;
> + if (ubaid.start) m_MMGR_FREE_BUFR_LIST(ubaid.start);
> + }
> + /* intentional fall through */
> case IMMND_EVT_A2ND_CCB_COMPLETED_RSP:
> case IMMND_EVT_A2ND_CCB_COMPLETED_RSP_2:
> case IMMND_EVT_A2ND_PBE_PRTO_DELETES_COMPLETED_RSP:
> @@ -7973,6 +8109,7 @@ immnd_evt_proc_fevs_dispatch(IMMND_CB *c
> break;
>
> case IMMND_EVT_A2ND_OI_CCB_AUG_INIT:
> + case IMMND_EVT_A2ND_OI_CCB_AUG_INIT_2:
> immnd_evt_ccb_augment_init(cb, &frwrd_evt.info.immnd,
> originatedAtThisNd, clnt_hdl, reply_dest);
> break;
>
> @@ -8046,6 +8183,8 @@ immnd_evt_proc_fevs_dispatch(IMMND_CB *c
>
> case IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP:
> case IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2:
> + case IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_3:
> + case IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_4:
> immnd_evt_proc_ccb_obj_delete_rsp(cb, &frwrd_evt.info.immnd,
> originatedAtThisNd, clnt_hdl, reply_dest);
> break;
>
So finally I would like to see first a complete reversion patch to 4.5
(if that is not already the case).
Then a new patch that avoids the extra memory allocations and free
except close to the users API
for input SaNameT parameters.
/AndersBj
------------------------------------------------------------------------------
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
