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 <b...@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.miha...@gmail.com>
>> Cc: Daniel Baluta <dbal...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
