Thanks so much. That was a really fast turnaround.
On Thu, Mar 22, 2012 at 03:56:00PM -0700, Mehak Mahajan wrote:
> The changes allow the user to specify a separate dscp value for the
> controller connection and the manager connection. The value will take
> effect on resetting the connections. If no value is specified a default
> value of 192 is chosen for each of the connections.
Please put "Feature #10074." and "Requested-by: ..." on separate lines:
> Feature #10074 Requested-by: Rajiv Ramanathan <rramanat...@nicira.com>
> Signed-off-by: Mehak Mahajan <mmaha...@nicira.com>
"git am" again reported some trailing whitespace. Please remove it:
Applying: Allow configuring DSCP on controller and manager connections.
/home/blp/ovs/.git/rebase-apply/patch:48: trailing whitespace.
/* Default value of dscp bits for connection between controller and
/home/blp/ovs/.git/rebase-apply/patch:49: trailing whitespace.
* manager.
/home/blp/ovs/.git/rebase-apply/patch:50: trailing whitespace.
* Set to IPTOS_PREC_INTERNETCONTROL (defined in <netinet/ip.h>)
/home/blp/ovs/.git/rebase-apply/patch:54: trailing whitespace.
/* Invalid dscp value. If the dscp value will not be used, the dscp value
/home/blp/ovs/.git/rebase-apply/patch:55: trailing whitespace.
* passed must be invalid.
warning: squelched 37 whitespace errors
warning: 42 lines add whitespace errors.
I didn't notice before that you added the OPENVSWITCH_DEFAULT_DSCP
definition to include/openvswitch/types.h. That's an odd place for
it. We don't have a really good place, though. socket-util.h is the
best spot I see, although you could create a new dscp.h if you prefer.
I see a bunch of new #includes for <netinet/ip.h>, presumably for
IPTOS_PREC_INTERNETCONTROL. But we use the convention that header
files should be more or less self-contained, so that when I #include
the header to get OPENVSWITCH_DEFAULT_DSCP I don't have to #include
another header to use its definitions. That means that whatever
header has OPENVSWITCH_DEFAULT_DSCP should itself #include
<netinet/ip.h>.
I think you can drop the OPENVSWITCH_ prefix on these two definitions.
It's really long and not necessary. I think I'd name them
DSCP_DEFAULT and DSCP_INVALID.
I see that you replaced the 2011 copyright year by 2012 in
include/sparse/netinet/in.h (or maybe I did mistakenly in my
suggestion?). Either way, please never delete copyright years, just
add new ones, so that this one would now read "2011, 2012".
In inet_open_active(), thank you for explaining 'dscp' in the comment:
* 'dscp' This value is used to set the IP_TOS bits in the IP Header. */
But our usual style is to use a more conversational style instead of
just a list. Maybe like this:
* If 'dscp' is not DSCP_INVALID, then its value becomes the DSCP bits in
* IP headers for the new connection.
Also in inet_open_active():
> + /* The socket options set here ensure that the TOS bits are set during
> + * the connection establishment. If set after connect(), the handshake
> + * SYN frames will be sent with a TOS of 0. */
> + if (dscp >= 0) {
Since 'dscp' is uint8_t now, it's always >= 0. Maybe you meant to
write 'if (dscp != DSCP_INVALID)'?
Same comments for inet_open_passive().
stream-provider.h still has a prototype for the deleted function
stream_set_dscp().
In stream-provider.h, I'd make the comment more specific, maybe
instead of:
* 'dscp' is the value of the DSCP option in the IP Header
the following:
* 'dscp' is the DSCP value that the new connection should use in
* IP packets it sends (DSCP_INVALID to use the system default).
In stream-provider.h, 'listen' should get a comment too.
Thank you for fixing the indentation here and elsewhere:
> static int
> new_ssl_stream(const char *name, int fd, enum session_type type,
> - enum ssl_state state, const struct sockaddr_in *remote,
> - struct stream **streamp)
> + enum ssl_state state, const struct sockaddr_in *remote,
> + struct stream **streamp)
> {
> struct sockaddr_in local;
> socklen_t local_len = sizeof local;
In vconn-provider.h, please make improvements to the comments similar
to those for stream-provider.h.
In ofproto.h there's still an odd change here:
> -#define OFPROTO_FLOW_EVICTON_THRESHOLD_DEFAULT 1000
> -#define OFPROTO_FLOW_EVICTION_THRESHOLD_MIN 100
> +#define OFPROTO_FLOW_EVICTON_THRESHOLD_DEFAULT 1000
> +#define OFPROTO_FLOW_EVICTION_THRESHOLD_MIN 100
I didn't notice before, but the "hmap" in the names and comments for
read_hmap_column() and read_hmap_string_column() is not good, because
"struct hmap" (in lib/hmap.h) isn't involved. (Also, the "h" stands
for "hash" but there's no hashing involved here.) I'd just
s/hmap/map/.
I'd inline read_hmap_column() into read_hmap_string_column(), because
read_hmap_column() is specialized for strings anyway and cannot be
used for any other type.
The documentation in vswitch.xml looks very nice. The only possible
thing I might add is that valid DSCP values have their low 2 bits set
to 0.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
