Thanks for the comments Ben.
I will send out a new patch after addressing your concerns.
thanx!
mehak
On Tue, Aug 14, 2012 at 9:33 AM, Ben Pfaff <b...@nicira.com> wrote:
> 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 <mmaha...@nicira.com>
>
> ...
>
> > +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
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
