[ovs-dev] [bundle 5/5] vswitch: Implement bundle action.

[Ethan Jackson]

Thanks for the review, I pushed this series.

Ethan

On Tue, Jul 19, 2011 at 09:20, Ben Pfaff <blp at nicira.com> wrote:
> On Mon, Jul 18, 2011 at 07:00:25PM -0700, Ethan Jackson wrote:
>> On Mon, Jul 18, 2011 at 18:58, Ethan Jackson <ethan at nicira.com> wrote:
>> >>> +static struct nx_action_bundle *
>> >>> +parse_bundle_actions(char *actions)
>> >>> +{
>> >>> + ? ?struct nx_action_bundle *nab;
>> >>> + ? ?struct ofpbuf b;
>> >>> +
>> >>> + ? ?ofpbuf_init(&b, 0);
>> >>> + ? ?bundle_parse(&b, actions);
>> >>> + ? ?nab = ofpbuf_steal_data(&b);
>> >>> + ? ?ofpbuf_uninit(&b);
>> >>> +
>> >>> + ? ?if (ntohs(nab->n_slaves) > BD_MAX_SLAVES) {
>> >>> + ? ? ? ?ovs_fatal(0, "At most %u slaves are supported", BD_MAX_SLAVES);
>> >>> + ? ?}
>> >>
>> >> This implies that bundle_parse() will parse more than BD_MAX_SLAVES
>> >> slaves. ?Shouldn't that be checked in bundle_parse() itself?
>> >
>> > I'm not sure I fully understand this comment. ?Hopefully the
>> > incremental I send out will make it clear that BD_MAX_SLAVES is only
>> > relevant to the testing code, and that the bundle library has it's own
>> > maximum (2048 slaves). ?I'll update bundle_parse to enforce this
>> > limit.
>>
>> Oops, now that I look at the code, bundle_parse() does enforce that
>> only BUNDLE_MAX_SLAVES are used.  If there are more it simply
>> truncates.  I suppose I could have it ovs_fatal() out.  I'll leave it
>> how it is pending a comment.
>
> It's fine, thanks.
>