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.
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?).
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,
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);
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.)
ofp_bundle_find() needs {} around the "return" statement here:
+ if (bundle->id == id)
+ return bundle;
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.
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);
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()).
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);
In ofconn.c, I don't see anything that frees bundles when a connection
closes.
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)
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);
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
