On Dec 11, 2014, at 5:49 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Mon, Dec 08, 2014 at 02:01:31PM -0800, Jarno Rajahalme wrote:
>> Reject bundle add messages containing messages that should not be bundled.
>>
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>
> Thanks for the patch!
>
> In ofputil_is_bundlable(), there's a "case" that falls through with an
> empty statement:
>> + case OFPTYPE_TABLE_FEATURES_STATS_REPLY:
>> + case OFPTYPE_ROLE_STATUS:
>> + ;
>> + }
>> +
>> + return false;
> I'd tend to write "break;" instead of ";”.
>
OK.
> Here, because of the confusion that OFPERR_OFPBFC_MSG_BAD_XID in place
> of 'error' could cause, it might be worth logging 'error' (with a rate
> limit) if it's nonzero so that the logs at least give a hint:
>> @@ -8513,6 +8601,12 @@ ofputil_decode_bundle_add(const struct ofp_header *oh,
>> return OFPERR_OFPBFC_MSG_BAD_XID;
>> }
>>
>> + /* Reject unbundlable messages. */
>> + error = ofptype_decode(&type, msg->msg);
>> + if (error || !ofputil_is_bundlable(type)) {
>> + return OFPERR_OFPBFC_MSG_UNSUP; /* 'error' could be confusing. */
>> + }
>> +
>> return 0;
>> }
This should fit the bill:
/* Reject unbundlable messages. */
error = ofptype_decode(&type, msg->msg);
- if (error || !ofputil_is_bundlable(type)) {
- return OFPERR_OFPBFC_MSG_UNSUP; /* 'error' could be confusing. */
+ if (error) {
+ VLOG_WARN_RL(&bad_ofmsg_rl, "OFPT14_BUNDLE_ADD_MESSAGE contained "
+ "message is unparsable (%s)", ofperr_get_name(error));
+ return OFPERR_OFPBFC_MSG_UNSUP; /* 'error' would be confusing. */
+ }
+
+ if (!ofputil_is_bundlable(type)) {
+ return OFPERR_OFPBFC_MSG_UNSUP;
}
return 0;
>
> Acked-by: Ben Pfaff <b...@nicira.com>
Thanks for the review, pushed with the above changes,
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
