Hi Ben, Thanks a lot for your detailed review! Comments inline below.
On Apr 16, 2013, at 2:30 PM, Ben Pfaff <[email protected]> wrote: > On Mon, Mar 25, 2013 at 06:33:02PM -0700, Romain Lenglet wrote: >> Define NXAST_SAMPLE OpenFlow vendor action and the corresponding >> OFPACT_SAMPLE OVS action, to do per-flow packet sampling. >> >> Signed-off-by: Romain Lenglet <[email protected]> > > Thanks for the revised series! Sorry it took a long time to review. > > This time I reviewed the series after squashing everything together. > It was easier than I expected. > > There is still one TODO that looks wrong to me after squashing: > /* TODO: Send a sample packet to each sFlow collector. */ > in handle_flow_sample_upcall(). Removed. > I also wonder about this one in ofproto_set_ipfix(): > /* TODO: Check the options. */ Removed. > Somewhere in this series I think we should mention in the FAQ that for > IPFIX to work we need the out-of-kernel kernel module from OVS version > 1.x or later, or the in-tree kernel module from Linux version 3.y or > later. I don't know off-hand what 'x' or 'y' should be (let me know > if you can't figure them out), but it's whatever version added the > variable-length userdata for upcalls. Done in the "ofproto: Translate NXAST_SAMPLE actions into SAMPLE "flow_sample" dp actions" change, which adds the first non-64-bit cookie structure. Your patch was not included in 1.9.x or upstream, so I documented that this requires the module from OVS 1.10.90 or later. > nicira-ext.h > ------------ > > The comment on 'len' here should state the length is 24: > struct nx_action_sample { > ovs_be16 type; /* OFPAT_VENDOR. */ > ovs_be16 len; /* Length is 16. */ Done. > odp-util.c > ---------- > > In format_odp_userspace_action(), some of the indentation is a little > off in the 'if' statements. This: > } else if (userdata_len == sizeof cookie.slow_path > && cookie.type == USER_ACTION_COOKIE_SLOW_PATH) { > should be: > } else if (userdata_len == sizeof cookie.slow_path > && cookie.type == USER_ACTION_COOKIE_SLOW_PATH) { > and there are extra blank lines e.g. two of them here: > } else if (userdata_len == sizeof cookie.ipfix > && cookie.type == USER_ACTION_COOKIE_IPFIX) { > > ds_put_format(ds, ",ipfix"); > > } else { Done. > parse_odp_action() has some parameter validation that we generally > don't do: > if (probability == 0) { > return -INVAR; > } > We generally don't validate parameters at this level because that > gives us a better opportunity to test implementations (e.g. the kernel > and userspace datapaths) against bad input. (But we don't do much of > that, so if you feel strongly that we should reject this bad > parameter, go ahead and leave the check.) Agreed. Removed. > ofp-actions.c > ------------- > > Indentation is off here: > static void > ofpact_sample_to_nxast(const struct ofpact_sample *os, > struct ofpbuf *out) Done. > ofp-parse.c > ----------- > > There's an doubled blank line here in parse_named_action(): > case OFPUTIL_NXAST_STACK_POP: > nxm_parse_stack_action(ofpact_put_STACK_POP(ofpacts), arg); > break; > > > case OFPUTIL_NXAST_SAMPLE: > parse_sample(ofpacts, arg); > break; Removed. > ofproto-dpif-ipfix.c > -------------------- > > <config.h> should be the first include file. I don't think you need > <netinet/in.h> at all; byte-order.h includes it and it is reasonable > to rely on that. Removed. > The return value convention for dpif_ipfix_exporter_set_options() and > dpif_ipfix_bridge_exporter_set_options() are unusual by OVS standards. > Usually, an OVS function that returns only success or failure has a > "bool" return type. Done: I made those true on success, false on failure. > This comment is puzzling. size_t is at least 32 bits on the systems > we target. > /* Hash into 16 bits since that's the standard size for size_t. */ > #define COLLECTOR_SET_ID_HASH(ID) (((ID) & 0xffff) ^ ((ID) >> 16)) > Also, why not use hash_int() instead of this inlined hash? And why a > macro instead of a function? Replaced with hash_int(). I didn't know about that function, thanks. > dpif_ipfix_set_options() has two assertions that are commented out, > but I don't know why. Are the assertions not actually correct? They are correct, and I have put them back. I wasn't sure about the policy regarding asserts, since there aren't many in the code. > dpif_ipfix_set_options() stops adding flow exporters if adding one > fails, and does not add any more flow exporters or remove flow > exporters that should be removed. Shouldn't it do both? OK, I've changed to just: - log at WARN level; - continue configuring the remaining exporters. > This code in dpif_ipfix_set_options() looks wrong, because the code > after the "break" is unreachable. I think that the options++ should > be outside the inner block: > > for (i = 0; i < n_flow_exporters_options; i++) { > if (node->exporter.options->collector_set_id > == options->collector_set_id) { > break; > options++; > } > } Fixed. Good catch, thanks! > dpif_ipfix_clear() is non-static and has a prototype in > ofproto-dpif-ipfix.h, but I don't think that any code outside > ofproto-dpif-ipfix.c can safely use it because it destroys the ofpbuf > member 'msg' and later uses of ofproto-dpif-ipfix.c functions will use > that member. Made this function static and removed it from ofproto-dpif-ipfix.h. Removed the 'msg' un/initialization, as I switched to a temp buffer, cf. below. > Regarding 'msg', actually, it looks like this is only used temporarily > within ipfix_send_template_msg() and ipfix_send_data_msg(). For that > kind of use, we would usually allocate the buffer on-stack, e.g.: > uint64_t msg_stub[DIV_ROUND_UP(1500, 8)]; > struct ofpbuf msg; > > ofpbuf_use_stub(&msg, msg_stub, sizeof msg_stub); > ... > ofpbuf_uninit(&msg); I've switched to using that. Thanks for the advice. > You could use ofpbuf_use_stack() instead, and avoid the need for > ofpbuf_uninit(), if you are certain that the buffer contents will > never be longer than the allocated space. > > In ipfix_send_data_msg(), many of the local variables could be > declared in inner blocks, which is preferred style in OVS. Done. > In the definition of dpif_ipfix_flow_sample(), the return type > ("void") should be on a line by itself. Done. > ofproto-dpif-ipfix.h > --------------------- > > This header declares of "struct ofproto_ipfix_options" but there is no > definition of this struct anywhere in the tree. Removed. > utilities/ovs-ofctl.8.in > ------------------------ > > The action description should now mention OVS 1.10.90 instead of > 1.9.90 (sorry about that). Fixed. > utilities/ovs-vsctl.c > --------------------- > > It seems that we should change del_bridge() to delete any > FlowSampleCollectorSet records that reference the bridge. Otherwise > the user will get a cryptic error. Done. > vswitchd/bridge.c > ----------------- > > In bridge_configure_ipfix(), some of the local variable names are > really long and make the code harder to read. Maybe, for example, > bridge_exporter_options could be shortened to, say, be_opts, and > bridge_exporter_cfg to be_cfg? Good idea. Done. > bridge_configure_ipfix() dereferences > bridge_exporter_cfg->obs_domain_id and > bridge_exporter_cfg->obs_point_id without checking that they are > nonnull, but they will be nonnull if the user does not configure them > in the database. Fixed. > vswitchd.ovsschema > ------------------ > > I think that Flow_Sample_Collector_Set would better fit the naming > convention we have so far. Done. > vswitch.xml > ----------- > > There is no explanation of FlowSampleCollectorSet's bridge column. Added. > Thanks, > > Ben. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
