See comments inline
Can you please send a new patch?
Thanks,
Hans
On 17 January 2014 09:00, Gary Lee <[email protected]> wrote:
> osaf/services/saf/amf/amfd/nodegroup.cc | 35
> ++++++++++++++++++++++++++++++++-
> 1 files changed, 34 insertions(+), 1 deletions(-)
>
>
> * fix out-of-bounds read in ng_ccb_apply_modify_hdlr() when the node to be
> deleted is located at the end of saAmfNGNodeList
> * modify ng_ccb_completed_modify_hdlr() to reject multiple node deletions
> from a node group in a single CCB, as this is not currently supported
> in ng_ccb_apply_modify_hdlr(). Also add a check for attempts to delete
> a node that does not belong to the node group. Previously this caused
> an assert.
>
> diff --git a/osaf/services/saf/amf/amfd/nodegroup.cc
> b/osaf/services/saf/amf/amfd/nodegroup.cc
> --- a/osaf/services/saf/amf/amfd/nodegroup.cc
> +++ b/osaf/services/saf/amf/amfd/nodegroup.cc
> @@ -260,6 +260,24 @@
> }
>
> /**
> + * Determine if a node is in a node group
> + * @param node node
> + * @param ng nodegroup
> + *
> + * @return true if found, otherwise false
> + */
> +static bool is_node_in_nodegroup(const SaNameT* node, const AVD_AMF_NG *ng)
This is code duplication with node_in_ng() in sg.cc. Should have one
of these functions in one place.
If we look at it more object oriented it seems to be an operation of
the node group:
AVD_AVND *nodegroup_find(const SaNameT node_name)
{
}
and thereby placed in nodegroup.cc
I propose you do this change in ver 2 of the patch
> +{
> + unsigned int i;
> + for (i = 0; i < ng->number_nodes; i++) {
> + if (memcmp(&ng->saAmfNGNodeList[i], node, sizeof(SaNameT)) ==
> 0)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/**
> * Validate modification of node group
> * @param opdata
> *
> @@ -275,6 +293,8 @@
> AVD_SU *su;
> int delete_found = 0;
> int add_found = 0;
> + /* currently, we don't support multiple node deletions from a group
> in a CCB */
move the above comment to where the decision is taken below
> + int nodes_deleted = 0;
> AVD_AMF_NG *ng;
>
> TRACE_ENTER();
> @@ -305,6 +325,12 @@
>
> TRACE("DEL %s", ((SaNameT
> *)mod->modAttr.attrValues[j])->value);
>
> + if (is_node_in_nodegroup((SaNameT
> *)mod->modAttr.attrValues[j], ng) == false) {
> + report_ccb_validation_error(opdata,
> "ng modify: node '%s' does not exist in node group",
> + ((SaNameT
> *)mod->modAttr.attrValues[j])->value);
> + goto done;
> + }
> +
> /* Ensure no SU is mapped to this node via
> the node group */
>
> /* for all OpenSAF SUs hosted by this node */
> @@ -327,6 +353,13 @@
> goto done;
> }
> }
> +
> + ++nodes_deleted;
> + if (nodes_deleted > 1) {
> + report_ccb_validation_error(opdata,
> + "ng modify: cannot delete
> more than one node from a node group in a CCB");
> + goto done;
> + }
> }
> }
>
> @@ -509,7 +542,7 @@
>
> TRACE("found node %s", ng->saAmfNGNodeList[j].value);
>
> - for (; j < ng->number_nodes; j++)
> + for (; j < (ng->number_nodes - 1); j++)
> ng->saAmfNGNodeList[j] =
> ng->saAmfNGNodeList[j + 1];
>
> ng->number_nodes -= mod->modAttr.attrValuesNumber;
>
> ------------------------------------------------------------------------------
> CenturyLink Cloud: The Leader in Enterprise Cloud Services.
> Learn Why More Businesses Are Choosing CenturyLink Cloud For
> Critical Workloads, Development Environments & Everything In Between.
> Get a Quote or Start a Free Trial Today.
> http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
> _______________________________________________
> Opensaf-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/opensaf-devel
------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today.
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
