Hans Feldt wrote: > On 2 July 2013 10:11, Anders Bjornerstedt > <[email protected]> wrote: > >> Hi >> Two minor comments inlined. >> >> praveen malviya wrote: >> >>> Hi, >>> One more change that can be brought into this patch. Ccb modification of >>> saAmfSgtDefAutoRepair in SaAmfSGType is allowed only in the context of >>> sufailover which is currently implemented for only 2N and NoRed. So for >>> other red models modify operation should be rejected like this: >>> >>> diff --git a/osaf/services/saf/avsv/avd/avd_sgtype.cc >>> b/osaf/services/saf/avsv/avd/avd_sgtype.cc >>> --- a/osaf/services/saf/avsv/avd/avd_sgtype.cc >>> +++ b/osaf/services/saf/avsv/avd/avd_sgtype.cc >>> @@ -338,6 +338,12 @@ static SaAisErrorT sgtype_ccb_completed_ >>> continue; >>> >>> if (!strcmp(attr_mod->modAttr.attrName, >>> "saAmfSgtDefAutoRepair")) { >>> + if ((sgt->saAmfSgtRedundancyModel != >>> SA_AMF_2N_REDUNDANCY_MODEL) && >>> + (sgt->saAmfSgtRedundancyModel != SA_AMF_NO_REDUNDANCY_MODEL)) { >>> + rc = SA_AIS_ERR_BAD_OPERATION; >>> + LOG_ER("Modification of >>> saAmfSgtDefAutoRepair supported for 2N and NoRed model only"); >>> >>> >> (1) >> This would be a user error so the log severity should not be ERor but >> NOtice. >> >> (2) >> If you are ambitious you could add a return error string, which >> could/should reach the >> end users tool/interface. For example, immcfg would pick it up and >> display it in conjunction >> with printing the error code. >> Like this: >> >> saImmOiCcbSetErrorString(immOiHandle, ccbId, >> "Modification of saAmfSgtDefAutoRepair supported for 2N >> and NoRed model only"); >> >> Otherwise the user will simply get the error code BAD_OPERATION and >> most likely not have >> a clue what the problem was. >> > > This is a very good comment but I think it should be addressed in its > own ticket which already exist. At least it used to exist in trac... > All admin op responses and CCB reject should be report error strings > maybe instead of using syslog. > > Same goes for IMM, remember ;-) All those errors catched by IMM should > be reported back with a complementary string. > Sure but since comment (1) still applies (log severity needs to be reduced), the patch should be updated in this spot. So why not incrementally add allso the the error string. It would not hurt and this case is a particularly good example where the error string really is needed.
Separate ticket I suppose should be created also for AMF for the generic overhaul of error strings if it does not exist already. But those kind of overhaul tickets tend to be quite daunting for developers. They tend to get postponed. So why not use also the complementary approach of adding error strings incrementally and opportunistically ? /AndersBj > /Hans > ------------------------------------------------------------------------------ This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
