Hi, I agree with Vu. You are mixing NCSCC errors with AIS errors.
Usually, when we introduce some limitation in the library, we must have the same limitation on the service side as well, and both checks return the same error code. This is a bit tricky part. The check is on the service side in the decoding part which is more part of MDS than IMM. If it's not possible to fix the service side and return ERR_INAVLID_PARAM or ERR_NO_RESOURCE, I would suggest to make an exception in this case and return ERR_INVALID_PARAM or ERR_NO_RESOURCE if the problem is caught in the library (so that applications does not need to restart) and return ERR_LIBRARY if the problem is caught on the service side (which means that some other library or earlier IMM library is used). In both places (the service and the library sides) comments need to be added in the code to be visible that we have made an exception in this case. So, on the service side, only comments need to be added since it already returns ERR_LIBRARY. Thanks, Zoran -----Original Message----- From: Vu Minh Nguyen [mailto:[email protected]] Sent: den 27 februari 2018 02:27 To: 'srinivas' <[email protected]>; Zoran Milinkovic <[email protected]> Cc: [email protected] Subject: RE: [PATCH 1/1] imm: return correct error code when working on more than 10000 objects [#2359] Hi Srinivas, I see you added new error code type to the function ` immsv_evt_enc_name_list`. I don't think that is a good idea to mix using 02 different returned error code types in one function. (Actually, SA_AIS_ERR_NO_RESOURCES(18) value is equal to NCSCC_RC_NO_OBJECT(18)) And few minors are inline. Regards, Vu > -----Original Message----- > From: srinivas [mailto:[email protected]] > Sent: Tuesday, February 20, 2018 2:31 PM > To: [email protected]; [email protected] > Cc: [email protected]; srinivas > <[email protected]> > Subject: [PATCH 1/1] imm: return correct error code when working on > more than 10000 objects [#2359] > > --- > src/imm/agent/imma_proc.cc | 10 ++++++++-- > src/imm/common/immsv_evt.c | 2 +- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/src/imm/agent/imma_proc.cc b/src/imm/agent/imma_proc.cc > index 886b50c..af8fb58 100644 > --- a/src/imm/agent/imma_proc.cc > +++ b/src/imm/agent/imma_proc.cc > @@ -3543,8 +3543,14 @@ SaAisErrorT imma_evt_fake_evs(IMMA_CB *cb, > IMMSV_EVT *i_evt, IMMSV_EVT **o_evt, > proc_rc = immsv_evt_enc(i_evt, &uba); > > if (proc_rc != NCSCC_RC_SUCCESS) { > - TRACE_2("ERR_LIBRARY: Failed to pre-pack"); > - rc = SA_AIS_ERR_LIBRARY; > + if (proc_rc != SA_AIS_ERR_NO_RESOURCES) { > + rc = SA_AIS_ERR_LIBRARY; > + TRACE_2("ERR_LIBRARY: Failed to pre-pack"); > + } > + else { [Vu] `else` statement should be on the same line with previous `{`. > + rc = SA_AIS_ERR_NO_RESOURCES; > + TRACE_2("ERR_NO_RESOURCES: Failed to pre-pack"); > + } > goto fail; > } [Vu] Can we simplify above logic by only adding below check after ` immsv_evt_enc`? if (proc_rc == NCSCC_RC_NO_OBJECT) { TRACE_2("ERR_NO_RESOURCES: Failed to pre-pack"); rc = SA_AIS_ERR_NO_RESOURCES; goto fail; } > > diff --git a/src/imm/common/immsv_evt.c b/src/imm/common/immsv_evt.c > index 88c5101..aef00d4 100644 > --- a/src/imm/common/immsv_evt.c > +++ b/src/imm/common/immsv_evt.c > @@ -775,7 +775,7 @@ static uint32_t immsv_evt_enc_name_list(NCS_UBAID > *o_ub, IMMSV_OBJ_NAME_LIST *p) > > if (objs >= IMMSV_MAX_OBJECTS) { > LOG_ER("TOO MANY Object Names line:%u", __LINE__); > - return NCSCC_RC_OUT_OF_MEM; > + return SA_AIS_ERR_NO_RESOURCES; [Vu] I don' think It is a good idea to mix using 02 different returned error code types. > } > return NCSCC_RC_SUCCESS; > } > -- > 2.7.4 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
