[ovs-dev] [PATCH v3 1/3] Add basic implementation for OpenFlow 1.4 bundles

[Alexandru Copot]

Thanks a lot for the feedback. You can find my answers inline.
We will get back with v4 asap.

On Thu, Apr 10, 2014 at 9:38 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Fri, Apr 04, 2014 at 11:58:29AM +0300, Alexandru Copot wrote:
>> This is only the communication part of the bundles functionality.
>> The actual message pre-validation and commits are not implemented.
>>
>> Signed-off-by: Alexandru Copot <alex.mihai.c at gmail.com>
>> Cc: Daniel Baluta <dbaluta at ixiacom.com>
>
> Hi Alexandru.  Thank you for the patches!  I have some comments.
>
> The change to ofp_print_version() in ofp-print.c doesn't seem related
> to bundles.  I would put it in a separate patch.

I must have missed this. Will send that change separately.

>
> The new files ofproto/bundles.[ch] were not written by Nicira, so the
> copyright notice should not name Nicira.  It should name whoever owns
> the copyright.  (I guess that might be you, or your employer, or Ixia,
> but I do not know for sure.)  The years listed should be the years in
> which you developed the code, which I guess is 2014 (possibly 2013
> also?).

Will change this.

> I don't understand this definition in ofp-msgs.h.  Why is the
> "uint32_t, uint16_t, uint16_t" there?
> +    /* OFPT 1.4+ (34): struct ofp14_bundle_add_msg, uint32_t, uint16_t, uint16_t, uint8_t[8][]. */
> +    OFPRAW_OFPT14_BUNDLE_ADD_MESSAGE,

I forgot the inner message header was already included. So I can remove those.

> In ofp_print_bundle_ctrl(), the cast here is unnecessary:
> +    ofp_print_bit_names(s, (uint32_t)bctrl.flags, bundle_flags_to_name, ' ');
>
> To best use ofp_print_bit_names(), bundle_flags_to_name() should
> return NULL, not "UNKNOWN", for unknown bits.
>
> In ofp_print_bundle_ctrl(), the cast here is unnecessary:
> +    msg = ofp_to_string((const void*)badd.msg, badd.length, verbosity);
>
> Also, we typically don't put spaces around format specifier macro
> names as done here in ofp_print_bundle_ctrl():
> +    ds_put_format(s, " bundle_id=%#" PRIx32 " type=",  bctrl.bundle_id);
> and here in ofp_print_bundle_add():
> +    ds_put_format(s, " bundle_id=%#" PRIx32,  badd.bundle_id);

Agree.

> I don't think it's necessary for ofputil_decode_bundle_add() to make a
> malloc()'d clone of the inner message.  Why does it do that?  (If
> there is a good reason then xmemdup is a better way to do it.)

I needed a way to access the entire contents of the inner message.
If I just copy the pointers, the outer message will be freed by the time
we apply the inner message.

> ofp_bundle_find() needs {} around the "return" statement here:
> +        if (bundle->id == id)
> +            return bundle;

Ok, missed this.

> At first glance it seems to me that bundles would only be relevant in
> the ofproto main thread, so I don't know why there is locking.

I thought they might be needed in the future. We might remove them for now.

> Here in ofp_bundle_open(), it is better not to put an explicit
> new-line at the end of a log message:
> +        VLOG_INFO("Bundle %x already exists.\n", id);

Ok.

> ofp_bundle_hash() passes a basis of 0 to hash_int.  The other uses of
> hash_int() on bundle_ids in the same file pass in a basis of 10.  I
> suspect that this is a bug (and that the other uses should call
> ofp_bundle_hash()).

You are right. I forgot to call ofp_bundle_hash().

> It's better, by the way, not to use an "ofp_" prefix for entities that
> do not come directly from OpenFlow, but to choose some different
> prefix.
>
> Indentation is wrong here in bundles.c (using hard tabs instead of 4
> spaces):
> struct bundle_message {
>         struct ofp_header *msg;
>         struct list       node;  /* Element in 'struct ofp_bundles's msg_list */
> };
>
> In bundles.h, please format prototypes with the return type on the
> same line as the function name, e.g.:
> enum ofperr ofp_bundle_open(struct ofconn *, uint32_t id, uint16_t flags);

Ok, will fix these.

> In ofconn.c, I don't see anything that frees bundles when a connection
> closes.

You are right. Will implement that.

> In ofconn.c, please indent locking annotations, like this:
>     struct hmap *
>     ofconn_lock_bundles(struct ofconn *ofconn)
>         OVS_ACQUIRES(&ofconn->bundles_mutex)
> or put them on the same line as the function parameters, if they fit,
> like this (in this case it doesn't fit):
>     struct hmap *
>     ofconn_lock_bundles(struct ofconn *ofconn) OVS_ACQUIRES(&ofconn->bundles_mutex)

Ok.

> In handle_bundle_control(), there is a misindented "case" here:
> +    switch (bctrl.type) {
> +        case OFPBCT_OPEN_REQUEST:
> +        error = ofp_bundle_open(ofconn, bctrl.bundle_id, bctrl.flags);
> +        reply.type = OFPBCT_OPEN_REPLY;
> +        break;
>
> In handle_bundle_add(), I would write:
> +    error = ofp_bundle_add_message(ofconn, &badd);
> +
> +    return error;
> as
>      return ofp_bundle_add_message(ofconn, &badd);

Alright.