First, some high-level comments. We definitely need a new action at
the OpenFlow protocol level, and NXAST_DEC_TTL_CNT_IDS is fine. But
at higher levels I do not think that we need to make a sharp
distinction. In particular:
- I think that instead of "dec_ttl_cnt_ids=1:2", we could have the
user write "dec_ttl(ids=1,2)", since I don't know of a reason to
point out that something is different at the OpenFlow level. (We
would keep plain "dec_ttl" meaning what it does now.)
- I think that instead of adding an ofpact_dec_ttl_cnt_ids, we
should modify the existing ofpact_dec_ttl. After all, the
existing dec_ttl is equivalent to dec_ttl_cnt_ids=0. (You can
use the "compat" member of struct ofpact to distinguish
NXAST_DEC_TTL from NXAST_DEC_TTL_CNT_IDS for converting back to
OpenFlow actions.)
Also, I don't see any tests for the encoding and decoding. You should
add some, at least to the "ovs-ofctl parse-flows" test in
ovs-ofctl.at.
> Subject: Adding Nicira vendor extension action
> NXAST_DEC_TTL_CNT_IDS.
Could you just make that "Add" rather than "Adding"? That's the usual
way for wording commit logs.
On Mon, Aug 13, 2012 at 09:51:01PM -0700, Mehak Mahajan wrote:
> Currently, if a controller having a non zero id registers to get a
s/non zero/nonzero/?
> OFPR_INVALID_TTL async message, it will not receive it. This is because
> compose_dec_ttl() only sent the invalid ttl packets to the default controller
> id. NXAST_DEC_TTL_CNT_IDS is a new action that accepts a list of controller
> ids, each separated by `:', to which the OFPR_INVALID_TTL packets must be
> sent.
> The earlier requirement of the controller having to explicitly register to
> receive these asynchronous messages is retained.
> The syntax of this action is:
> dec_ttl_cnt_ids=id1:id2
> where id1, id2 are valid controller ids.
The use of : looks odd to me. How about:
dec_ttl(id1,id2)
instead.
> Signed-off-by: Mehak Mahajan <[email protected]>
...
> +struct nx_action_controller_ids {
> + ovs_be16 type; /* OFPAT_VENDOR. */
> + ovs_be16 len; /* Length including slaves. */
> + ovs_be32 vendor; /* NX_VENDOR_ID. */
> + ovs_be16 subtype; /* NXAST_DEC_TTL_CNT_IDS. */
> +
> + ovs_be16 n_controllers; /* Number of controllers. */
> + uint8_t zeros[4]; /* Must be zero. */
> + /* Followed by 1 or more controller ids.
> + *
> + * uint16_t *controller_ids;
"uint16_t *" is misleading here since there's no pointer. I'd write
"uint16_t controller_ids[]" if you want to describe it precisely, or
even "uint16_t controller_ids[n_controllers]".
I suggest specifying that the padding following the controller ids
must also be zero.
> static enum ofperr
> +dec_ttl_cnt_ids_from_openflow(const struct nx_action_controller_ids *nac_ids,
> + struct ofpbuf *out)
> +{
> + struct ofpact_controller_ids *ids;
> + size_t ids_size;
> + enum ofperr error = 0;
> + int i;
> +
> + ids = ofpact_put_DEC_TTL_CNT_IDS(out);
> + ids->n_controllers = ntohs(nac_ids->n_controllers);
> + ids_size = ntohl(nac_ids->len) - sizeof *nac_ids;
> +
> + if (!is_all_zeros(nac_ids->zeros, sizeof nac_ids->zeros)) {
> + VLOG_WARN_RL(&rl, "reserved field is nonzero");
> + error = OFPERR_OFPBAC_BAD_ARGUMENT;
> + VLOG_ERR("Came in here.\n");
Please use OFPERR_NXBRC_MUST_BE_ZERO for this error. Why not just
return the error at this point? Also I'd think that both log messages
are unnecessary; the caller should log the OFPERR_NXBRC_MUST_BE_ZERO
error, to make the problem clear.
> + }
> + if (ids_size < ids->n_controllers * sizeof(ovs_be16)) {
> + VLOG_WARN_RL(&rl, "Nicira action dec_ttl_cnt_ids only has %zu bytes "
> + "allocated for controller ids. %zu bytes are required
> for "
> + "%"PRIu16" controllers.", ids_size,
> + ids->n_controllers * sizeof(ovs_be16),
> ids->n_controllers);
> + error = OFPERR_OFPBAC_BAD_LEN;
I'd return the error at this point, otherwise the loop below will read
beyond the end of 'nac_ids'.
> + }
...
> static void
> +ofpact_dec_ttl_cnt_ids_to_nxast(const struct ofpact_controller_ids *oc_ids,
> + struct ofpbuf *out)
> +{
> + struct nx_action_controller_ids *nac_ids;
> + int ids_len = ROUND_UP(oc_ids->n_controllers, OFP_ACTION_ALIGN);
I think this needs "2 *" since each port number is two bytes.
> + ovs_be16 *ids;
> + size_t i;
> +
> + nac_ids = ofputil_put_NXAST_DEC_TTL_CNT_IDS(out);
> + nac_ids->len = htons(ntohs(nac_ids->len) + ids_len);
> + nac_ids->n_controllers = htons(oc_ids->n_controllers);
> +
> + ids = ofpbuf_put_zeros(out, ids_len);
> + for (i = 0; i < oc_ids->n_controllers; i++) {
> + ids[i] = htons(oc_ids->controller_ids[i]);
> + }
> +}
...
print_dec_ttl_cnt_ids() doesn't use the syntax that you documented in
the commit message.
In ofp-actions.h, since there isn't enough room for
ofpact_controller_ids in any case, I don't think I'd reindent all the
other lines; it doesn't look any better, especially since the line of
\s doesn't line up anyway. (You could rename the 'controller_ids'
member of struct ofpact_controller_ids to 'ids', which is easier to
read anyway.)
> static void
> +parse_dec_ttl_cnt_ids(struct ofpbuf *b, char *arg)
> +{
> + struct ofpact_controller_ids *ids;
> +
> + if (*arg == '\0') {
> + ovs_fatal(0, "dec_ttl_cnt_ids: expected atleast one controler id.");
s/atleast/at least/
s/controler/controller/
But I'd put the check for the number of controllers after the loop
instead of here. After all, what if someone just supplies, say, ":"
as the argument? Then there'd be no controller id but the argument
would not be empty.
> + }
> +
> + ids = ofpact_put_DEC_TTL_CNT_IDS(b);
> + while(*arg != '\0') {
Add a space following "while", please.
> + char *cntr;
> + uint16_t id;
> +
> + cntr = strtok_r(NULL, ": ", &arg);
I don't think that this works portably. The first call to strtok_r()
is supposed to use the string to parse as the first argument, and NULL
only after that.
> + id = atoi(cntr);
> + ofpbuf_put(b, &id, sizeof id);
> +
> + ids = b->l2;
> + ids->n_controllers++;
> + }
> + ofpact_update_len(b, &ids->ofpact);
> +}
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
