Hi Neel.
There is some valuable work done in this patch and most of it is ok.
But in its current state I have to give it a NACK.
It needs some adjustments and extension before it can be pushed.
There are two main problems:
1) The PBE is the main OI for the object involved and we actually want
to be able to reach the PBE
(not just the IMMND) to also probe the PBE for resource measurements.
Only the empty OK reply
on PBE resources needs to be implemented in #35, but the problem now is
that this patch shortcircuits the admop
inside the IMMND so that it never reaches the PBE. Instead the admin-op
should reach the PBE and
the PBE should just reply OK for now. In the future the PBE would scan
the list of params for values
that where owned by the PBE and set those vales in a reply.
2) The control flow fetches the IMMND params at the requesting ndoe and
it does so on the invocation.
It should normally wait for the reply (from the PBE) and only fetch the
(local) IMMND params immediately
if there is no PBE (it is disabled).
The good news is that the added new method
ImmModel::resourceDisplay(...) should be ok as is.
But it would usually be invoked later (still only at the IMMND where the
om-client resides).
This is how I would like to see the data flow (assuming the PBE is enabled):
A) The admin-op is invoked in the normal way. It reaches the PBE. The
PBE recognizes
the new resource admin-op and just replies OK. Later we add support for
PBE params
which will be added by the PBE.
B) The reply is received normally (the PBE could be remote so the reply
could be from remote),
and caught in immnd_evt_pbe_admop_rsp(). Inside if(reqConn) { ... }
there should be a check if this
is the new special admin-op. (The same long condition that you currently
have in proc_admop().
If this case matches, then invoke ImmModel::resourceDisplay(...) to
fetch the parameters that you have implemented.
Note that here we could potentiually be appending IMMND parameters to an
existing list of PBE parameters.
For the case where there is no PBE, then the call should bounce sort of
like it does now, by returning ERR_REPAIR_PENDING
which is caught in proc_admop() which invokes
ImmModl::resourceDisplay(). But this would be the rarer 0PBE case.
Some additional detailed review comments below.
reddy.neelaka...@oracle.com wrote:
> osaf/services/saf/immsv/immnd/ImmModel.cc | 279
> ++++++++++++++++++++++++++++-
> osaf/services/saf/immsv/immnd/ImmModel.hh | 10 +-
> osaf/services/saf/immsv/immnd/immnd_evt.c | 43 ++++-
> osaf/services/saf/immsv/immnd/immnd_init.h | 6 +-
> osaf/tools/safimm/immadm/imm_admin.c | 13 +-
> 5 files changed, 333 insertions(+), 18 deletions(-)
>
>
> The enhancement dumps the IMMsv resource utilization data using IMM Admin
> Operation.
> The IMM AdminOperation must be directed to immsv object
> opensafImm=opensafImm,safApp=safImmService.
>
> Presently two types of operationNames for dumping resource utilization is
> supported:
>
> display -- returns the terse output for the requested resource. The number of
> requested resource will be returned.
>
Clarify to: The allocation count of the resource will be returned,
"number" can be misread as some kind of id.
> The supported resource are implementers, adminowners, ccbhandles and
> searchhandels.
>
For ccbs its not really ccbHandles but just ccbs. I suggest the resource
name: 'ccbs'..
Remeber that one ccb-handle can produce a chain of ccb's and even after
a ccb-handle is closed,
the ccb(s) may linger in the system and will be included in the count.
It is the number of "lingering" ccbs that is printed.
Also even when there are zero ccbs lingering in the server, there may be
valid ccb-handles at clients.
Note: It is the correct count that is printed. It is just the
explanation of what is counted that is not quite correct here.
Similarly for "search-handles" I suggest: 'searches'.
> Eg:
>
> immadm -O display -p resource:SA_STRING_T:implementers
> opensafImm=opensafImm,safApp=safImmService
>
> The output is the number of implementers present:
> Name Type Value(s)
> ========================================================================
> count SA_INT64_T 9 (0x9)
>
> displayverbose -- returns the verbose output for the requested resource. The
> verbose output is supported for the resources implementers and adminowners.
> If the number of resource is greater than 127 then the verbose output is
> displayed to syslog.
>
> Eg:
> immadm -O displayverbose -p resource:SA_STRING_T:implementers
> opensafImm=opensafImm,safApp=safImmService
> [using admin-owner: 'safImmService']
> Object: opensafImm=opensafImm,safApp=safImmService
> Name Type Value(s)
> ========================================================================
> safLogService SA_INT64_T 131343
> (0x2010f)
> safClmService SA_INT64_T 131343
> (0x2010f)
> safAmfService SA_INT64_T 131343
> (0x2010f)
> MsgQueueService131343 SA_INT64_T 131343
> (0x2010f)
> safMsgGrpService SA_INT64_T 131343
> (0x2010f)
> safCheckPointService SA_INT64_T 131343
> (0x2010f)
> safEvtService SA_INT64_T 131343
> (0x2010f)
> safLckService SA_INT64_T 131343
> (0x2010f)
> safSmfService SA_INT64_T 131343
> (0x2010f)
>
Here in this case it is not clear to the user what the value printed per
implementer is. It looks like the nodeId (all of them are the same)
but more useful would be the implementerId or the connectionId (from the
OI handle) because the user "knows" on which node they are invoking the
request on
and remember that we only get the reply for the local IMMND (plus PBE
that can be remote).
For the verbose output it would be nice to also get the complete name,
implementer-id, client-id and node-id for the implementer. But that
could be added later.
Either by re-opening the enhancement if you get some time before FC or
later by creating a minor defect ticket.
> If the operationName display-help is used, the return parameters will have
> presently supported resources for dumping resource utilization data.
>
> Eg:
> immadm -O display-help opensafImm=opensafImm,safApp=safImmService
> [using admin-owner: 'safImmService']
> Object: opensafImm=opensafImm,safApp=safImmService
> Name Type Value(s)
> ========================================================================
> supportedResources SA_STRING_T implementers
> supportedResources SA_STRING_T adminowners
> supportedResources SA_STRING_T ccbhandles
> supportedResources SA_STRING_T searchhandels
>
>
> error-string is returned. if any of the dispaly operations fails.
>
> immadm is modified for displaying the returned parameters when dumping of
> resource utilization is supported .
>
> 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
> @@ -1169,11 +1169,12 @@ immModel_adminOperationInvoke(IMMND_CB *
> SaInvocationT inv,
> SaUint32T* implConn,
> SaClmNodeIdT* implNodeId,
> - SaBoolT pbeExpected)
> + SaBoolT pbeExpected,
> + bool* displayRes)
> {
> return ImmModel::instance(&cb->immModel)->
> adminOperationInvoke(req, reqConn, reply_dest, inv,
> - implConn, implNodeId, pbeExpected);
> + implConn, implNodeId, pbeExpected, displayRes);
> }
>
> SaUint32T /* Returns admo-id for object if object exists and active admo
> exists, otherwise zero. */
> @@ -1903,6 +1904,44 @@ immModel_fetchRtUpdate(IMMND_CB *cb,
> }
>
>
> +SaAisErrorT
> +immModel_resourceDisplay(IMMND_CB *cb,
> + const struct ImmsvOmAdminOperationInvoke *req,
> + struct ImmsvAdminOperationParam **rparams)
> +{
> +
> + struct ImmsvAdminOperationParam *params = req->params;
> + SaStringT opName = NULL;
> + SaUint64T searchcount=0;
> +
> + while (params) {
> + if ((strcmp(params->paramName.buf, SA_IMM_PARAM_ADMOP_NAME))==0){
> + opName=params->paramBuffer.val.x.buf;
> + break;
> + }
> + params=params->next;
> + }
> +
> + if ((strcmp(opName,"display")==0)){
> + IMMND_OM_SEARCH_NODE *searchOp;
> + IMMND_IMM_CLIENT_NODE *cl_node =
> + (IMMND_IMM_CLIENT_NODE
> *)ncs_patricia_tree_getnext(&cb->client_info_db, NULL);
> + while(cl_node) {
> + searchOp = cl_node->searchOpList;
> + while(searchOp) {
> + searchcount++;
> + searchOp = searchOp->next;
> + }
> + cl_node = (IMMND_IMM_CLIENT_NODE *)ncs_patricia_tree_getnext(
> + &cb->client_info_db, cl_node->patnode.key_info);
> + }
> +
> + }
> +
> + return ImmModel::instance(&cb->immModel)->
> + resourceDisplay(req, rparams, searchcount);
> +}
> +
> /*====================================================================*/
>
> ImmModel::ImmModel() :
> @@ -10107,7 +10146,7 @@ SaAisErrorT ImmModel::adminOperationInvo
> SaInvocationT& saInv,
> SaUint32T* implConn,
> unsigned int* implNodeId,
> - bool pbeExpected)
> + bool pbeExpected, bool*
> displayRes)
> {
> TRACE_ENTER();
> SaAisErrorT err = SA_AIS_OK;
> @@ -10212,6 +10251,12 @@ SaAisErrorT ImmModel::adminOperationInvo
> fake_obj:
> // Check for call on object implementer
> if(object->mImplementer && object->mImplementer->mNodeId) {
> +
> + if(req->operationId == SA_IMM_PARAM_ADMOP_ID_ESC && objectName ==
> immObjectDn && displayRes){
> + err = updateImmObject2(req, displayRes);
> + goto done;
> + }
> +
>
The above check could I think just as well be put into
ImmModel:resourceDisplay.
> *implConn = object->mImplementer->mConn;
> *implNodeId = object->mImplementer->mNodeId;
>
> @@ -10271,7 +10316,7 @@ SaAisErrorT ImmModel::adminOperationInvo
> } else {
> /* Check for special imm OI support */
> if(!pbeExpected && objectName == immObjectDn) {
> - err = updateImmObject2(req);
> + err = updateImmObject2(req, displayRes);
>
Not sure we have to go to updateImmObject2 because we are no really
updating anything.
We should only be going there if there is no OI (no PBE) and only to
bounce, getting
the special error code ERR_REPAIR_PENDING which actually stands exactly for
"admin op directed at the opensaf imm service object normally handle by
PBE but PBE is detached"
> TRACE_7("Admin op on special object %s whith no implementer
> ret:%u",
> objectName.c_str(), err);
> } else if(objectName == immManagementDn) {
> @@ -10582,7 +10627,7 @@ ImmModel::updateImmObject(std::string ne
> }
>
> SaAisErrorT
> -ImmModel::updateImmObject2(const ImmsvOmAdminOperationInvoke* req)
> +ImmModel::updateImmObject2(const ImmsvOmAdminOperationInvoke* req, bool *
> displayRes)
> {
> SaAisErrorT err = SA_AIS_ERR_REPAIR_PENDING;
> /* Function for handling admin-ops directed at the immsv itself.
> @@ -10621,7 +10666,8 @@ ImmModel::updateImmObject2(const ImmsvOm
> valuep = (ImmAttrValue *) avi->second;
> noStdFlags = valuep->getValue_int();
>
> - if((req->params == NULL) || (req->params->paramType !=
> SA_IMM_ATTR_SAUINT32T)) {
> + if((req->params == NULL) || ((req->params->paramType !=
> SA_IMM_ATTR_SAUINT32T)&&
> + (req->params->paramType !=
> SA_IMM_ATTR_SASTRINGT))) {
> err = SA_AIS_ERR_INVALID_PARAM;
> goto done;
> }
> @@ -10645,9 +10691,25 @@ ImmModel::updateImmObject2(const ImmsvOm
> noStdFlags &= ~flagsToUnSet;
> valuep->setValue_int(noStdFlags);
> LOG_NO("%s changed to: 0x%x", immAttrNostFlags.c_str(), noStdFlags);
> + } else if(req->operationId == SA_IMM_PARAM_ADMOP_ID_ESC && displayRes &&
> req->params) {
> + IMMSV_ADMIN_OPERATION_PARAM* params = req->params;
> +
> + while(params) {
> + if((strcmp(params->paramName.buf,SA_IMM_PARAM_ADMOP_NAME)==0)
> && (params->paramType==SA_IMM_ATTR_SASTRINGT)) {
> +
> if(strncmp(params->paramBuffer.val.x.buf,"display",7)==0) {
> + *displayRes=true;
> + } else {
> + LOG_WA("Invalid OperationName %s for dumping
> IMM resource is not set properly", params->
> paramBuffer.val.x.buf);
> + err = SA_AIS_ERR_INVALID_PARAM;
> + }
> + break;
> + }
> + params = params->next;
> + }
> +
>
Again put the above logic, which is specific to this particular admin-op
in the ImmModel::resourceDisplay methoid,
which is particluar to that admin-op.
> } else {
> LOG_NO("Invalid operation ID %llu, for operation on %s", (SaUint64T)
> req->operationId,
> - immObjectDn.c_str());
> +
> immObjectDn.c_str());
> err = SA_AIS_ERR_INVALID_PARAM;
> }
>
> @@ -10657,6 +10719,209 @@ ImmModel::updateImmObject2(const ImmsvOm
> }
>
> SaAisErrorT
> +ImmModel::resourceDisplay(const struct ImmsvOmAdminOperationInvoke *req,
> + struct ImmsvAdminOperationParam **rparams,
> SaUint64T searchcount)
> +{
> + SaAisErrorT err = SA_AIS_OK;
> + struct ImmsvAdminOperationParam *params = req->params;
> + SaStringT opName = NULL, resourceName = NULL, errStr = NULL;
> + struct ImmsvAdminOperationParam * resparams = NULL;
> +
> + TRACE_ENTER();
> +
> + while (params) {
> + if ((strcmp(params->paramName.buf,
> SA_IMM_PARAM_ADMOP_NAME))==0){
> + opName=params->paramBuffer.val.x.buf;
> + break;
> + }
> + params=params->next;
> + }
> +
> +
> + if (opName){
> + params = req->params;
> + while(params){
> + if((strcmp(params->paramName.buf, "resource"))==0){
> + resourceName=params->paramBuffer.val.x.buf;
> + TRACE_5("The resource is %s", resourceName);
> + break;
> + }
> + params=params->next;
> + }
> + if(!resourceName){
> + if(strcmp(opName,"display-help")==0){
> + TRACE_5("supported IMM resources to be displyed
> by using admin operation");
> + } else {
> + LOG_WA("The resource type is not present in the
> requested parameters for displaying IMM resources");
> + err = SA_AIS_ERR_INVALID_PARAM;
> + int len= strlen("resource type is not present
> in the requested parameters")+1;
> + errStr = (SaStringT)malloc (len);
> + strcpy(errStr, "resource type is not present in
> the requested parameters");
> + }
> + }
> + }
> +
> + if ((strcmp(opName,"display")==0)) {
> + resparams = (struct ImmsvAdminOperationParam *)calloc (1,
> sizeof(struct ImmsvAdminOperationParam));
> + resparams->paramType = SA_IMM_ATTR_SAINT64T;
> + resparams->next = NULL;
> + resparams->paramName.size=strlen("count")+1;
> + resparams->paramName.buf=(char *) malloc
> (resparams->paramName.size);
> + strcpy(resparams->paramName.buf,"count");
> + if((strcmp(resourceName,"implementers")==0)){
> +
> resparams->paramBuffer.val.saint64=sImplementerVector.size();
> + }else if ((strcmp(resourceName,"adminowners")==0)){
> +
> resparams->paramBuffer.val.saint64=sOwnerVector.size();
> + }else if ((strcmp(resourceName,"ccbhandles")==0)){
> +
> resparams->paramBuffer.val.saint64=sCcbVector.size();
> + }else if ((strcmp(resourceName,"searchhandels")==0)){
> +
> resparams->paramBuffer.val.saint64=searchcount;
> + }else {
> + LOG_WA("Display of IMM reources for resourceName %s
> is unsupported", resourceName);
> + err = SA_AIS_ERR_INVALID_PARAM;
> + int cnt = strlen("Display of IMM reources for
> resourceName is unsupported")+1;
> + errStr = (SaStringT)malloc (cnt);
> + strcpy(errStr, "Display of IMM reources for
> resourceName is unsupported");
> + free(resparams);
> + resparams = NULL;
> + goto done;
> + }
> +
> + } else if(strcmp(opName,"displayverbose")==0){
> +
> + struct ImmsvAdminOperationParam * result=NULL;
> + if((strcmp(resourceName,"implementers")==0)){
> + if(sImplementerVector.size() > 0){
> + if(sImplementerVector.size() < 128){
> +
> + ImplementerVector::iterator i;
> + for(i = sImplementerVector.begin(); i
> != sImplementerVector.end(); ++i) {
> + ImplementerInfo* info = (*i);
> + struct ImmsvAdminOperationParam
> * res =
> + (struct
> ImmsvAdminOperationParam *)calloc (1,
> sizeof(struct
> ImmsvAdminOperationParam));
> + res->paramType =
> SA_IMM_ATTR_SAINT64T;
> + res->next = NULL;
> +
> res->paramName.size=info->mImplementerName.length()+1;
> + res->paramName.buf=(char *)
> malloc (res->paramName.size);
> +
> strcpy(res->paramName.buf,info->mImplementerName.c_str());
> +
> res->paramBuffer.val.saint64=info->mNodeId;
> + if(result){
> + result->next = res;
> + result=res;
> + }else {
> + result = res;
> + resparams=res;
> + }
> + }
> +
> + } else {
> + LOG_NO("The Number of implementers are
> greater than 128, displaying the implementers informati
> on to syslog");
> + ImplementerVector::iterator i;
> + for(i = sImplementerVector.begin();
> i != sImplementerVector.end(); ++i) {
> + ImplementerInfo* info = (*i);
> + LOG_IN("Implementer name %s and
> location of the implementer is %u",
> +
> info->mImplementerName.c_str(), info->mNodeId);
> + }
> + }
> + }
> + } else if((strcmp(resourceName,"adminowners")==0)){
> + if(sOwnerVector.size() > 0){
> + if(sOwnerVector.size() < 128){
> + AdminOwnerVector::iterator i;
> + for(i = sOwnerVector.begin(); i !=
> sOwnerVector.end(); ++i) {
> + AdminOwnerInfo* adminOwner =
> (*i);
> + struct
> ImmsvAdminOperationParam * res =
> + (struct
> ImmsvAdminOperationParam *)
> + calloc (1,
> sizeof(struct ImmsvAdminOperationParam));
> + res->paramType =
> SA_IMM_ATTR_SAINT64T;
> + res->next = NULL;
> +
> res->paramName.size=adminOwner->mAdminOwnerName.length()+1;
> + res->paramName.buf=(char *)
> malloc (res->paramName.size);
> +
> strcpy(res->paramName.buf,adminOwner->mAdminOwnerName.c_str());
> +
> res->paramBuffer.val.saint64=adminOwner->mNodeId;
> + if(result){
> + result->next = res;
> + result=res;
> + }else {
> + result = res;
> + resparams=res;
> + }
> + }
> + }
> + LOG_NO("The Number of AdminOwners
> are greater than 128, displaying the adminowner information
> to syslog");
> + AdminOwnerVector::iterator i;
> + for(i = sOwnerVector.begin(); i !=
> sOwnerVector.end(); ++i) {
> + AdminOwnerInfo* adminOwner =
> (*i);
> + LOG_IN("Implementer name %s
> and location of the implementer is %u",
> +
> adminOwner->mAdminOwnerName.c_str(), adminOwner->mNodeId);
> + }
> + }
> + } else {
> + LOG_WA("Verbose display of reourcename %s is
> unsupported", resourceName);
> + err = SA_AIS_ERR_INVALID_PARAM;
> + int cnt = strlen("verbose display of requested
> resourceName is unsupported")+1;
> + errStr = (SaStringT)malloc (cnt);
> + strcpy(errStr, "Verbose display of requested
> resourceName is unsupported");
> + goto done;
> + }
> + } else if((strcmp(opName,"display-help")==0)) {
> + const char *resources[] = {"implementers", "adminowners",
> "ccbhandles", "searchhandels", NULL};
> + int i=0;
> +
> + struct ImmsvAdminOperationParam * result=NULL;
> + while(resources[i]){
> + struct ImmsvAdminOperationParam * res = (struct
> ImmsvAdminOperationParam *)
> + calloc (1,
> sizeof(struct ImmsvAdminOperationParam));
> + res->paramType = SA_IMM_ATTR_SASTRINGT;
> + res->next = NULL;
> + res->paramName.size=strlen("supportedResources")+1;
> + res->paramName.buf=(char *) malloc
> (res->paramName.size);
> + strcpy(res->paramName.buf,"supportedResources");
> + res->paramBuffer.val.x.size= strlen(resources[i])+1;
> + res->paramBuffer.val.x.buf = (char *) malloc
> (res->paramBuffer.val.x.size);
> + strcpy(res->paramBuffer.val.x.buf,resources[i]);
> +
> + if(result){
> + result->next = res;
> + result=res;
> + }
> + else {
> + result = res;
> + resparams=res;
> + }
> + i++;
> + }
> + }else{
> + LOG_WA("OperationName %s is not supported", opName);
> + err = SA_AIS_ERR_INVALID_PARAM;
> + int cnt = strlen("OperationName is not supported")+1;
> + errStr = (SaStringT)malloc (cnt);
> + strcpy(errStr, "OperationName is not supported");
> + }
> +
> +
> + done:
> + if(!resparams && errStr){
> + struct ImmsvAdminOperationParam * errparam = (struct
> ImmsvAdminOperationParam *)
> + calloc (1, sizeof(struct
> ImmsvAdminOperationParam));
> + errparam->paramType = SA_IMM_ATTR_SASTRINGT;
> + errparam->next = NULL;
> + errparam->paramName.size=strlen(SA_IMM_PARAM_ADMOP_ERROR)+1;
> + errparam->paramName.buf=(char *) malloc
> (errparam->paramName.size);
> + strcpy(errparam->paramName.buf,SA_IMM_PARAM_ADMOP_ERROR);
> + errparam->paramBuffer.val.x.size= strlen(errStr)+1;
> + errparam->paramBuffer.val.x.buf = errStr;
> + resparams=errparam;
> + }
> +
> + *rparams = resparams;
> +
> + TRACE_LEAVE();
> + return err;
> +}
> +
> +
> +SaAisErrorT
> ImmModel::admoImmMngtObject(const ImmsvOmAdminOperationInvoke* req)
> {
> SaAisErrorT err = SA_AIS_ERR_INTERRUPT;
> diff --git a/osaf/services/saf/immsv/immnd/ImmModel.hh
> b/osaf/services/saf/immsv/immnd/ImmModel.hh
> --- a/osaf/services/saf/immsv/immnd/ImmModel.hh
> +++ b/osaf/services/saf/immsv/immnd/ImmModel.hh
> @@ -351,7 +351,8 @@ public:
> SaInvocationT& inv,
> SaUint32T* implConn,
> unsigned int* implNodeId,
> - bool pbeExpected);
> + bool pbeExpected,
> + bool* displayRes);
>
> // Objects
>
> @@ -453,6 +454,11 @@ public:
> ImplementerInfo* info,
> bool& subTreeHasPersistent,
> bool& subTreeHasSpecialAppl);
> +
> + SaAisErrorT resourceDisplay(
> + const struct
> ImmsvOmAdminOperationInvoke *req,
> + struct ImmsvAdminOperationParam
> **rparams,
> + SaUint64T searchcount);
>
> SaAisErrorT objectSync(const ImmsvOmObjectSync* req);
> bool fetchRtUpdate(ImmsvOmObjectSync* syncReq,
> @@ -633,7 +639,7 @@ private:
> void updateImmObject(
> std::string newClassName,
> bool remove=false);
> - SaAisErrorT updateImmObject2(const ImmsvOmAdminOperationInvoke*
> req);
> + SaAisErrorT updateImmObject2(const ImmsvOmAdminOperationInvoke*
> req, bool * displayRes);
> SaAisErrorT admoImmMngtObject(const ImmsvOmAdminOperationInvoke*
> req);
>
> void addNoDanglingRefs(ObjectInfo *obj);
> 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
> @@ -4625,6 +4625,7 @@ static void immnd_evt_proc_admop(IMMND_C
> SaUint32T implConn = 0;
> NCS_NODE_ID implNodeId = 0;
> SaBoolT async = SA_FALSE;
> + bool displayRes=false;
>
I was going to say "add a comment clarifying that this new bool is only
for this particular admin-op.".
But I think all this special processing in proc_admop() dissapears and
ends up in proc_admop_rsp() instead.
But then this bool will appear there and so a comment explaining this
special bool will be merited there.
In proc_admop it should suffice to just bounce with REPAIR_PENDING if
this is the special case and the
PBE is missing.
> SaBoolT pbeExpected = cb->mPbeFile && (cb->mRim ==
> SA_IMM_KEEP_REPOSITORY);
>
> async = (evt->type == IMMND_EVT_A2ND_IMM_ADMOP_ASYNC);
> @@ -4644,8 +4645,8 @@ static void immnd_evt_proc_admop(IMMND_C
>
> error = immModel_adminOperationInvoke(cb, &(evt->info.admOpReq),
> originatedAtThisNd ? conn : 0,
> - (SaUint64T)reply_dest, saInv,
> &implConn,
> - &implNodeId, pbeExpected);
> + (SaUint64T)reply_dest, saInv,
> &implConn,
> + &implNodeId, pbeExpected,
> &displayRes);
>
> /*Check if we have an implementer, if so forward the message.
> If there is no implementer then implNodeId is zero.
> @@ -4782,7 +4783,43 @@ static void immnd_evt_proc_admop(IMMND_C
> memset(&send_evt, '\0', sizeof(IMMSV_EVT));
> send_evt.type = IMMSV_EVT_TYPE_IMMA;
>
>
Now we are getting to the case where there is no PBE, i.e. the PBE is
disabled.
But I dont like the code duplication below.
From here:
> - if(error == SA_AIS_ERR_REPAIR_PENDING) {
> + if(error == SA_AIS_ERR_REPAIR_PENDING && displayRes) {
> + IMMSV_ADMIN_OPERATION_PARAM *rparams= NULL;
> + uint32_t rc = NCSCC_RC_SUCCESS, result = SA_AIS_OK;
> + osafassert(displayRes);
> + TRACE("Ok reply for internally handled adminOp when
> operationId is display");
> +
> + result = immModel_resourceDisplay(cb,
> &(evt->info.admOpReq), &rparams);
> +
> + if(!rparams)
> + TRACE_2("rparams from display is NULL");
> + send_evt.info.imma.type = IMMA_EVT_ND2A_ADMOP_RSP_2;
> + send_evt.info.imma.info.admOpRsp.invocation = saInv;
> + send_evt.info.imma.info.admOpRsp.result = result;
> + send_evt.info.imma.info.admOpRsp.error = SA_AIS_OK;
> + send_evt.info.imma.info.admOpRsp.parms = rparams;
> +
> + if (async) {
> + TRACE_2("ASYNCRONOUS special reply %llu %u %u
> to OM",
> + send_evt.info.imma.info.admOpRsp.invocation,
> + send_evt.info.imma.info.admOpRsp.result,
> + send_evt.info.imma.info.admOpRsp.error);
> + rc = immnd_mds_msg_send(cb,
> NCSMDS_SVC_ID_IMMA_OM,
> +
> cl_node->agent_mds_dest, &send_evt);
> + TRACE_2("ASYNC REPLY SENT rc:%u", rc);
> + } else {
> + TRACE_2("NORMAL syncronous reply %llu %u to OM",
> + send_evt.info.imma.info.admOpRsp.invocation,
> + send_evt.info.imma.info.admOpRsp.result);
> + rc = immnd_mds_send_rsp(cb,
> &(cl_node->tmpSinfo), &send_evt);
> + TRACE_2("SYNC REPLY SENT rc:%u", rc);
> + }
> +
> + if (rc != NCSCC_RC_SUCCESS) {
> + LOG_ER("Failure in sending reply for admin-op
> over MDS");
> + }
> +
> + } else if(error == SA_AIS_ERR_REPAIR_PENDING) {
> uint32_t rc = NCSCC_RC_SUCCESS;
> osafassert(!pbeExpected);
> TRACE("Ok reply for internally handled adminOp when PBE
> not configured");
>
...}
to the end of this else if branch.
There should be one if statement catching ERR_REPAIR_PENDING and inside
that an additional
nesting level discriminiating between what is now two cases of admin-ops
normally handled by PBE.
Most of the code should be the same (message type for reply etc).
/AndersBj
> diff --git a/osaf/services/saf/immsv/immnd/immnd_init.h
> b/osaf/services/saf/immsv/immnd/immnd_init.h
> --- a/osaf/services/saf/immsv/immnd/immnd_init.h
> +++ b/osaf/services/saf/immsv/immnd/immnd_init.h
> @@ -98,7 +98,7 @@ extern "C" {
> SaUint32T reqConn,
> SaUint64T reply_dest,
> SaInvocationT inv, SaUint32T
> *implConn,
> - SaClmNodeIdT *implNodeId, SaBoolT
> pbeExpected);
> + SaClmNodeIdT *implNodeId, SaBoolT
> pbeExpected, bool* displayRes);
>
> SaAisErrorT immModel_classCreate(IMMND_CB *cb,
> const struct ImmsvOmClassDescr *req,
> @@ -410,6 +410,10 @@ extern "C" {
> SaAisErrorT immModel_implIsFree(IMMND_CB *cb,
> const SaImmOiImplementerNameT implName);
>
> + SaAisErrorT immModel_resourceDisplay(IMMND_CB *cb,
> + const struct ImmsvOmAdminOperationInvoke *req,
> + struct ImmsvAdminOperationParam **rparams);
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/osaf/tools/safimm/immadm/imm_admin.c
> b/osaf/tools/safimm/immadm/imm_admin.c
> --- a/osaf/tools/safimm/immadm/imm_admin.c
> +++ b/osaf/tools/safimm/immadm/imm_admin.c
> @@ -244,7 +244,7 @@ int main(int argc, char *argv[])
> {"admin-owner", required_argument, 0, 'a'},
> {"help", no_argument, 0, 'h'},
> {"timeout", required_argument, 0, 't'},
> - {"verbose", required_argument, 0, 'v'},
> + {"verbose", no_argument, 0, 'v'},
> {0, 0, 0, 0}
> };
> SaAisErrorT error;
> @@ -269,6 +269,7 @@ int main(int argc, char *argv[])
>
> params = realloc(NULL, sizeof(SaImmAdminOperationParamsT_2 *));
> params[0] = NULL;
> + SaStringT opName = NULL;
>
> while (1) {
> c = getopt_long(argc, argv, "dp:o:O:a:t:hv", long_options,
> NULL);
> @@ -306,6 +307,7 @@ int main(int argc, char *argv[])
> param->paramType = SA_IMM_ATTR_SASTRINGT;
> param->paramBuffer = malloc(sizeof(SaStringT));
> *((SaStringT *)(param->paramBuffer)) = strdup(optarg);
> + opName = strdup(optarg);
> break;
> case 'p':
> params_len++;
> @@ -347,7 +349,7 @@ int main(int argc, char *argv[])
> }
>
> if (operationId == -1) {
> - fprintf(stderr, "error - must specify admin operation ID\n");
> + fprintf(stderr, "error - must specify admin operation ID
> %llx\n", operationId);
> exit(EXIT_FAILURE);
> }
>
> @@ -417,7 +419,7 @@ retry:
> exit(EXIT_FAILURE);
> }
>
> - if (operationReturnValue != SA_AIS_OK) {
> + if (operationReturnValue != SA_AIS_OK ) {
> unsigned int ix = 0;
>
> if ((operationReturnValue == SA_AIS_ERR_TRY_AGAIN) &&
> !disable_tryagain) {
> @@ -444,11 +446,12 @@ retry:
>
> print_params(argv[optind], out_params);
> }
> -
> +
> exit(EXIT_FAILURE);
> }
>
> - if (verbose && out_params && out_params[0]) {
> +
> + if (((opName && (strncmp(opName,"display",7)==0))||verbose)
> &&out_params && out_params[0]) {
> if(!isFirst)
> printf("\n");
> else
>
------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
