Hi AndersBj,
Publish the new patch with the below comments.
Thanks,
Neel.
On Wednesday 23 July 2014 08:34 PM, Anders Bjornerstedt wrote:
> 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.
>
> [email protected] 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
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
