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().
I also wonder about this one in ofproto_set_ipfix():
/* TODO: Check the options. */
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.
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. */
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 {
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.)
ofp-actions.c
-------------
Indentation is off here:
static void
ofpact_sample_to_nxast(const struct ofpact_sample *os,
struct ofpbuf *out)
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;
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.
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.
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?
dpif_ipfix_set_options() has two assertions that are commented out,
but I don't know why. Are the assertions not actually correct?
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?
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++;
}
}
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.
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);
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.
In the definition of dpif_ipfix_flow_sample(), the return type
("void") should be on a line by itself.
ofproto-dpif-ipfix.h
---------------------
This header declares of "struct ofproto_ipfix_options" but there is no
definition of this struct anywhere in the tree.
utilities/ovs-ofctl.8.in
------------------------
The action description should now mention OVS 1.10.90 instead of
1.9.90 (sorry about that).
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.
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?
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.
vswitchd.ovsschema
------------------
I think that Flow_Sample_Collector_Set would better fit the naming
convention we have so far.
vswitch.xml
-----------
There is no explanation of FlowSampleCollectorSet's bridge column.
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
