Re: [devel] [PATCH 3 of 5] imm: Checking of Imm limits [#195]
Hi Neelakanta,
Ack on all patches.
No need for sending the code on another review. They are all minor changes
mostly related on code-style, and not on functionality.
Thanks,
Zoran
-Original Message-
From: Neelakanta Reddy [mailto:reddy.neelaka...@oracle.com]
Sent: den 16 augusti 2016 10:07
To: Zoran Milinkovic; Hung Duc Nguyen
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [PATCH 3 of 5] imm: Checking of Imm limits [#195]
Hi Zoran,
All the attribute definition will be moved to the top of the file.
The response for the remaining comments.
Thanks,
Neel.
On 2016/08/12 07:36 PM, Zoran Milinkovic wrote:
> Hi Neelakanta,
>
> Find my comments inline
>
> -Original Message-
> From: reddy.neelaka...@oracle.com [mailto:reddy.neelaka...@oracle.com]
> Sent: den 27 juli 2016 10:32
> To: Zoran Milinkovic; Hung Duc Nguyen
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: [PATCH 3 of 5] imm: Checking of Imm limits [#195]
>
> osaf/services/saf/immsv/immnd/ImmModel.cc | 322
> +-
> osaf/services/saf/immsv/immnd/ImmModel.hh |5 +-
> 2 files changed, 324 insertions(+), 3 deletions(-)
>
>
> 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
> @@ -1154,6 +1154,13 @@ immModel_protocol50Allowed(IMMND_CB *cb)
> SA_TRUE : SA_FALSE;
> }
>
> +SaBoolT
> +immModel_protocol51Allowed(IMMND_CB *cb) {
> +return (ImmModel::instance(>immModel)->protocol51Allowed()) ?
> +SA_TRUE : SA_FALSE;
> +}
> +
> OsafImmAccessControlModeT
> immModel_accessControlMode(IMMND_CB *cb)
> {
> @@ -3271,6 +3278,21 @@ ImmModel::classCreate(const ImmsvOmClass
> return SA_AIS_ERR_INVALID_PARAM;
> }
>
> +ObjectMap::iterator oit = sObjectMap.find(immObjectDn);
> +if(protocol51Allowed() && oit != sObjectMap.end() && !isLoading ){
> +std::string immMaxClasses(OPENSAF_IMMSV_MAX_CLASSES);
> +ObjectInfo* immObject = oit->second;
> +ImmAttrValueMap::iterator avi =
> immObject->mAttrValueMap.find(immMaxClasses);
> +osafassert(avi != immObject->mAttrValueMap.end());
> +osafassert(!(avi->second->isMultiValued()));
> +ImmAttrValue* valuep = avi->second;
> +unsigned int maxClasses = valuep->getValue_int();
> +if( sClassMap.size() >= maxClasses){
> +LOG_NO("ERR_NO_RESOURCES: maximum class limit %d has been reched
> for the cluster",
> + maxClasses);
> +return SA_AIS_ERR_NO_RESOURCES;
> +}
> +}
>
> [Zoran] "std::string immMaxClasses(OPENSAF_IMMSV_MAX_CLASSES);" can be moved
> to the top of the file with other definition of strings.
> I would create a common method for finding max number of classes and use it
> in the block above. The code is not wrong. I'll leave it to you if you want
> to create a common method.
>
> ClassMap::iterator i = sClassMap.find(className);
> if (i == sClassMap.end()) {
> /* Class-name is unique case-sensitive.
> @@ -3597,6 +3619,8 @@ ImmModel::classCreate(const ImmsvOmClass
> }
> }
>
> +
> +
> if(illegal) {
> if(err == SA_AIS_OK) {
> LOG_NO("ERR_INVALID_PARAM: Problem with new class '%s'",
> className.c_str()); @@ -3966,6 +3990,34 @@ ImmModel::protocol50Allowed()
> return noStdFlags & OPENSAF_IMM_FLAG_PRT50_ALLOW;
> }
>
> +
> +bool
> +ImmModel::protocol51Allowed()
> +{
> +//TRACE_ENTER();
> +/* Assume that all nodes are running the same version when loading */
> +if (sImmNodeState == IMM_NODE_LOADING) {
> +return true;
> +}
> +ObjectMap::iterator oi = sObjectMap.find(immObjectDn);
> +if(oi == sObjectMap.end()) {
> +TRACE_LEAVE();
> +return false;
> +}
> +
> +ObjectInfo* immObject = oi->second;
> +ImmAttrValueMap::iterator avi =
> +immObject->mAttrValueMap.find(immAttrNostFlags);
> +osafassert(avi != immObject->mAttrValueMap.end());
> +osafassert(!(avi->second->isMultiValued()));
> +ImmAttrValue* valuep = avi->second;
> +unsigned int noStdFlags = valuep->getValue_int();
> +
> +//TRACE_LEAVE();
> +return noStdFlags & OPENSAF_IMM_FLAG_PRT51_ALLOW; }
> +
> +
> bool
> ImmModel::protocol41Allowed()
> {
> @@ -4114,6 +4166,44 @@ ImmModel::verifySchemaChange(const std::
> verifyFailed = notCompatibleAtt(className, newClassInfo,
> attName, NULL, newAttr, NULL) ||
> verifyFailed;
> newAttrs[inew->first] = newAttr;
> +if(!verifyFailed && (className == immClassName)){
> +unsigned int val;
> +if ( attName == OPENSAF_IMMSV_MAX_CLASSES) {
>
> [Zoran] Same as in earlier comment. Create a string for
> OPENSAF_IMMSV_MAX_CLASSES in the top of the file with strings for other
> attributes.
>
> +
Re: [devel] [PATCH 3 of 5] imm: Checking of Imm limits [#195]
Hi Zoran,
All the attribute definition will be moved to the top of the file.
The response for the remaining comments.
Thanks,
Neel.
On 2016/08/12 07:36 PM, Zoran Milinkovic wrote:
> Hi Neelakanta,
>
> Find my comments inline
>
> -Original Message-
> From: reddy.neelaka...@oracle.com [mailto:reddy.neelaka...@oracle.com]
> Sent: den 27 juli 2016 10:32
> To: Zoran Milinkovic; Hung Duc Nguyen
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: [PATCH 3 of 5] imm: Checking of Imm limits [#195]
>
> osaf/services/saf/immsv/immnd/ImmModel.cc | 322
> +-
> osaf/services/saf/immsv/immnd/ImmModel.hh |5 +-
> 2 files changed, 324 insertions(+), 3 deletions(-)
>
>
> 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
> @@ -1154,6 +1154,13 @@ immModel_protocol50Allowed(IMMND_CB *cb)
> SA_TRUE : SA_FALSE;
> }
>
> +SaBoolT
> +immModel_protocol51Allowed(IMMND_CB *cb)
> +{
> +return (ImmModel::instance(>immModel)->protocol51Allowed()) ?
> +SA_TRUE : SA_FALSE;
> +}
> +
> OsafImmAccessControlModeT
> immModel_accessControlMode(IMMND_CB *cb)
> {
> @@ -3271,6 +3278,21 @@ ImmModel::classCreate(const ImmsvOmClass
> return SA_AIS_ERR_INVALID_PARAM;
> }
>
> +ObjectMap::iterator oit = sObjectMap.find(immObjectDn);
> +if(protocol51Allowed() && oit != sObjectMap.end() && !isLoading ){
> +std::string immMaxClasses(OPENSAF_IMMSV_MAX_CLASSES);
> +ObjectInfo* immObject = oit->second;
> +ImmAttrValueMap::iterator avi =
> immObject->mAttrValueMap.find(immMaxClasses);
> +osafassert(avi != immObject->mAttrValueMap.end());
> +osafassert(!(avi->second->isMultiValued()));
> +ImmAttrValue* valuep = avi->second;
> +unsigned int maxClasses = valuep->getValue_int();
> +if( sClassMap.size() >= maxClasses){
> +LOG_NO("ERR_NO_RESOURCES: maximum class limit %d has been reched
> for the cluster",
> + maxClasses);
> +return SA_AIS_ERR_NO_RESOURCES;
> +}
> +}
>
> [Zoran] "std::string immMaxClasses(OPENSAF_IMMSV_MAX_CLASSES);" can be moved
> to the top of the file with other definition of strings.
> I would create a common method for finding max number of classes and use it
> in the block above. The code is not wrong. I'll leave it to you if you want
> to create a common method.
>
> ClassMap::iterator i = sClassMap.find(className);
> if (i == sClassMap.end()) {
> /* Class-name is unique case-sensitive.
> @@ -3597,6 +3619,8 @@ ImmModel::classCreate(const ImmsvOmClass
> }
> }
>
> +
> +
> if(illegal) {
> if(err == SA_AIS_OK) {
> LOG_NO("ERR_INVALID_PARAM: Problem with new class '%s'",
> className.c_str());
> @@ -3966,6 +3990,34 @@ ImmModel::protocol50Allowed()
> return noStdFlags & OPENSAF_IMM_FLAG_PRT50_ALLOW;
> }
>
> +
> +bool
> +ImmModel::protocol51Allowed()
> +{
> +//TRACE_ENTER();
> +/* Assume that all nodes are running the same version when loading */
> +if (sImmNodeState == IMM_NODE_LOADING) {
> +return true;
> +}
> +ObjectMap::iterator oi = sObjectMap.find(immObjectDn);
> +if(oi == sObjectMap.end()) {
> +TRACE_LEAVE();
> +return false;
> +}
> +
> +ObjectInfo* immObject = oi->second;
> +ImmAttrValueMap::iterator avi =
> +immObject->mAttrValueMap.find(immAttrNostFlags);
> +osafassert(avi != immObject->mAttrValueMap.end());
> +osafassert(!(avi->second->isMultiValued()));
> +ImmAttrValue* valuep = avi->second;
> +unsigned int noStdFlags = valuep->getValue_int();
> +
> +//TRACE_LEAVE();
> +return noStdFlags & OPENSAF_IMM_FLAG_PRT51_ALLOW;
> +}
> +
> +
> bool
> ImmModel::protocol41Allowed()
> {
> @@ -4114,6 +4166,44 @@ ImmModel::verifySchemaChange(const std::
> verifyFailed = notCompatibleAtt(className, newClassInfo,
> attName, NULL, newAttr, NULL) ||
> verifyFailed;
> newAttrs[inew->first] = newAttr;
> +if(!verifyFailed && (className == immClassName)){
> +unsigned int val;
> +if ( attName == OPENSAF_IMMSV_MAX_CLASSES) {
>
> [Zoran] Same as in earlier comment. Create a string for
> OPENSAF_IMMSV_MAX_CLASSES in the top of the file with strings for other
> attributes.
>
> +val = newAttr->mDefaultValue.getValue_int();
> +if( sClassMap.size() > val){
> +LOG_NO("The Number of classes in the cluster %lu
> greater than the schema change"
> + "value %d", sClassMap.size(), val);
> +verifyFailed = true;
> + }
> +}
> +
> +if ( !verifyFailed &&
Re: [devel] [PATCH 3 of 5] imm: Checking of Imm limits [#195]
Hi Neelakanta,
Find my comments inline
-Original Message-
From: reddy.neelaka...@oracle.com [mailto:reddy.neelaka...@oracle.com]
Sent: den 27 juli 2016 10:32
To: Zoran Milinkovic; Hung Duc Nguyen
Cc: opensaf-devel@lists.sourceforge.net
Subject: [PATCH 3 of 5] imm: Checking of Imm limits [#195]
osaf/services/saf/immsv/immnd/ImmModel.cc | 322 +-
osaf/services/saf/immsv/immnd/ImmModel.hh |5 +-
2 files changed, 324 insertions(+), 3 deletions(-)
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
@@ -1154,6 +1154,13 @@ immModel_protocol50Allowed(IMMND_CB *cb)
SA_TRUE : SA_FALSE;
}
+SaBoolT
+immModel_protocol51Allowed(IMMND_CB *cb)
+{
+return (ImmModel::instance(>immModel)->protocol51Allowed()) ?
+SA_TRUE : SA_FALSE;
+}
+
OsafImmAccessControlModeT
immModel_accessControlMode(IMMND_CB *cb)
{
@@ -3271,6 +3278,21 @@ ImmModel::classCreate(const ImmsvOmClass
return SA_AIS_ERR_INVALID_PARAM;
}
+ObjectMap::iterator oit = sObjectMap.find(immObjectDn);
+if(protocol51Allowed() && oit != sObjectMap.end() && !isLoading ){
+std::string immMaxClasses(OPENSAF_IMMSV_MAX_CLASSES);
+ObjectInfo* immObject = oit->second;
+ImmAttrValueMap::iterator avi =
immObject->mAttrValueMap.find(immMaxClasses);
+osafassert(avi != immObject->mAttrValueMap.end());
+osafassert(!(avi->second->isMultiValued()));
+ImmAttrValue* valuep = avi->second;
+unsigned int maxClasses = valuep->getValue_int();
+if( sClassMap.size() >= maxClasses){
+LOG_NO("ERR_NO_RESOURCES: maximum class limit %d has been reched
for the cluster",
+ maxClasses);
+return SA_AIS_ERR_NO_RESOURCES;
+}
+}
[Zoran] "std::string immMaxClasses(OPENSAF_IMMSV_MAX_CLASSES);" can be moved to
the top of the file with other definition of strings.
I would create a common method for finding max number of classes and use it in
the block above. The code is not wrong. I'll leave it to you if you want to
create a common method.
ClassMap::iterator i = sClassMap.find(className);
if (i == sClassMap.end()) {
/* Class-name is unique case-sensitive.
@@ -3597,6 +3619,8 @@ ImmModel::classCreate(const ImmsvOmClass
}
}
+
+
if(illegal) {
if(err == SA_AIS_OK) {
LOG_NO("ERR_INVALID_PARAM: Problem with new class '%s'",
className.c_str());
@@ -3966,6 +3990,34 @@ ImmModel::protocol50Allowed()
return noStdFlags & OPENSAF_IMM_FLAG_PRT50_ALLOW;
}
+
+bool
+ImmModel::protocol51Allowed()
+{
+//TRACE_ENTER();
+/* Assume that all nodes are running the same version when loading */
+if (sImmNodeState == IMM_NODE_LOADING) {
+return true;
+}
+ObjectMap::iterator oi = sObjectMap.find(immObjectDn);
+if(oi == sObjectMap.end()) {
+TRACE_LEAVE();
+return false;
+}
+
+ObjectInfo* immObject = oi->second;
+ImmAttrValueMap::iterator avi =
+immObject->mAttrValueMap.find(immAttrNostFlags);
+osafassert(avi != immObject->mAttrValueMap.end());
+osafassert(!(avi->second->isMultiValued()));
+ImmAttrValue* valuep = avi->second;
+unsigned int noStdFlags = valuep->getValue_int();
+
+//TRACE_LEAVE();
+return noStdFlags & OPENSAF_IMM_FLAG_PRT51_ALLOW;
+}
+
+
bool
ImmModel::protocol41Allowed()
{
@@ -4114,6 +4166,44 @@ ImmModel::verifySchemaChange(const std::
verifyFailed = notCompatibleAtt(className, newClassInfo, attName,
NULL, newAttr, NULL) ||
verifyFailed;
newAttrs[inew->first] = newAttr;
+if(!verifyFailed && (className == immClassName)){
+unsigned int val;
+if ( attName == OPENSAF_IMMSV_MAX_CLASSES) {
[Zoran] Same as in earlier comment. Create a string for
OPENSAF_IMMSV_MAX_CLASSES in the top of the file with strings for other
attributes.
+val = newAttr->mDefaultValue.getValue_int();
+if( sClassMap.size() > val){
+LOG_NO("The Number of classes in the cluster %lu
greater than the schema change"
+ "value %d", sClassMap.size(), val);
+verifyFailed = true;
+ }
+}
+
+if ( !verifyFailed && attName ==
OPENSAF_IMMSV_MAX_IMPLEMENTERS) {
[Zoran] Same here for OPENSAF_IMMSV_MAX_IMPLEMENTERS.
+val = newAttr->mDefaultValue.getValue_int();
+if( sImplementerVector.size() > val){
+LOG_NO("The Number of Implementers in the cluster %lu
greater than the schema change"
+ "value %d", sImplementerVector.size(), val);
+verifyFailed = true;
+
