On Fri, Oct 10, 2014 at 07:11:45PM +0530, Rishi Bamba wrote:
> Hi Ben,
>
> # Thank you for reviewing the patch.As per the comments received ,all the
> changes are made and incorporated for this patch i.e Clang
> # compiler doesn't report any error now and all the test cases are successful
> including the one added by us for the same.Also along
> # with this patch sending three other patches related to the addition of
> importance in a rule which includes the changes as asked by
> # you plus replace-flows and diff-flows CLI enhancement after the addition of
> "importance" parameter in a rule as per OF14.
> ---
> This patch enables a user to set importance for a new rule via add-flow
> OF1.1+ in the OVS and display the same via dump-flows command OF1.1+ .
> The changes are made in accordance with OpenFlow 1.4 specs to implement
> Eviction on the basis of "importance".
Thanks for the patch. I have some comments.
I'd appreciate it if you would be more careful about formatting your
patch emails. The commit message should be at the top, and any
additional commentary should be below the ---. Only the part before
--- actually goes into the commit message, so this ensures that the
commit message has the content that you intend.
In this hunk, please follow the same formatting pattern as the other
lines:
> @@ -248,6 +248,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command,
> char *string,
> enum {
> F_OUT_PORT = 1 << 0,
> F_ACTIONS = 1 << 1,
> + F_IMPORTANCE=1 << 2,
> F_TIMEOUT = 1 << 3,
> F_PRIORITY = 1 << 4,
> F_FLAGS = 1 << 5,
OFP_FLOW_PERMANENT is meant to be used for hard_timeout and
idle_timeout. I don't think that we should reuse it for importance
also (please just write 0):
> + if (fs->importance != OFP_FLOW_PERMANENT) {
>
It looks like some places where 'importance' should be set were
missed. When I grep for ofputil_flow_mod, I see that, for example,
learn_execute() needs to set 'importance'. Please do your own grep
and make sure that importance is set everywhere it should be.
After you fix those problems, I think that this patch will be ready.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
