On Thu, Sep 22, 2011 at 1:56 PM, Ben Pfaff <b...@nicira.com> wrote: > On Wed, Sep 21, 2011 at 03:21:46PM -0700, Pravin Shelar wrote: >> v3: Fixed according to comments from Ben >> - Probability calculation >> - Coding style fixes >> - Now SAMPLE actions is not optional >> - Using offset to fix user-action-cookie. >> >> I will post another patch to handle mirror output port case for sFlow. > > "git am" says: > > /home/blp/db/.git/rebase-apply/patch:249: space before tab in indent. > * OVS_ACTION_ATTR_SAMPLE, as it has nested actions list which > /home/blp/db/.git/rebase-apply/patch:250: space before tab in indent. > * is variable size. */ > warning: 2 lines add whitespace errors. ok.
> > I didn't really read the kernel changes. > > > It's not so great to have to call netdev_get_stats() for every sflow > packet we send. Maybe we could rate-limit the calls and cache old > values? OK, I will keep 1 sec interval for get stats. > > Please add "0x" here to make it obvious the value is in hex: > + VLOG_WARN_RL(&rl, "invalid user cookie : %"PRIx64, upcall->userdata); > > Please use a pointer type (e.g. uint8_t * or char *) to do pointer > arithmetic: > + user_cookie_offset = (unsigned long) cookie - > + (unsigned long) odp_actions->data; > > compose_controller_action() leaves some fields uninitialized. > As other values are only required in case of sFlow. There is not need to initialize it. > I don't know why add_sflow_action() initially uses > USER_ACTION_COOKIE_UNSPEC only to change it to > USER_ACTION_COOKIE_SFLOW later in fix_sflow_action(). > I wanted to catch error when fix_sflow_action() is not called for add_sflow_action(). But I think it is not required. I am initializing it to sFlow flag now. > In struct action_xlate_ctx, I'd make sflow_n_outputs an 'int' or > 'unsigned int'. There could be more than 255 outputs. You can cap it > at 255 when you assign it to the cookie in fix_sflow_action(). > > It looks to me like the compose_actions() changes fix an existing bug > related to keeping track of the current VLAN correctly. If so, we > should break that out as a separate commit. > OK. > Thanks, > > Ben. > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
