Thank you for implementing this feature!
On Tue, Mar 20, 2012 at 01:35:02PM -0700, Mehak Mahajan wrote:
> From: root <root@debian.localdomain>
To get rid of this, run
git commit --amend --author='Mehak Mahajan <mmaha...@nicira.com>'
and then exit the editor that it invokes to allow you to edit the
commit message.
> The changes allow the user to specify a separate dscp value for the
> controller connection and the manager connection. The value will take
> effect only on resetting the connections. If no value is specified
> a default value of 192 is chosen for each of the connections.
>
> Issue #10074
Since it's a "feature" issue I usually would write it as:
Feature #10074.
Rajiv requested this so can you add:
Requested-by: Rajiv Ramanathan <rramanat...@nicira.com>
near the "Issue #"/"Feature #".
Finally you will need to add:
Signed-off-by: Mehak Mahajan <mmaha...@nicira.com>
when you believe that this is ready to commit. (The SubmittingPatches
file talks about what Signed-off-by means.)
Rajiv pointed out a couple of things off-list, I guess you will
consider those comments.
"git am" said:
Applying: Allow configuring DSCP on controller and manager connections.
/home/blp/ovs/.git/rebase-apply/patch:46: trailing whitespace.
/* The default value of dscp bits for connection between controller and
/home/blp/ovs/.git/rebase-apply/patch:64: trailing whitespace.
return stream_open_with_default_ports(name, JSONRPC_TCP_PORT,
/home/blp/ovs/.git/rebase-apply/patch:65: trailing whitespace.
JSONRPC_SSL_PORT, streamp,
/home/blp/ovs/.git/rebase-apply/patch:86: trailing whitespace.
error = jsonrpc_stream_open(name, &s->stream,
/home/blp/ovs/.git/rebase-apply/patch:104: trailing whitespace.
jsonrpc_session_set_dscp(struct jsonrpc_session *s,
warning: squelched 45 whitespace errors
warning: 50 lines add whitespace errors.
One high-level comment: I believe that DSCP is a single byte, but this
commit always uses an 'int'. It is worth considering switching to
uint8_t.
unix_open() and punix_open() in lib/stream-unix.c need a dscp
parameter, even if they won't use it (you should mark it OVS_UNUSED).
(This should have caused a compiler error, I'm surprised that it
didn't?)
You probably haven't set up "sparse" yet, which is fine for now, but
to make it accept this change I needed to add a few more definitions
to the "sparse" substitute headers files that we ship. So can you
fold the following change in to your patch? Thanks.
diff --git a/include/sparse/netinet/in.h b/include/sparse/netinet/in.h
index d86431a..e9a8e4c 100644
--- a/include/sparse/netinet/in.h
+++ b/include/sparse/netinet/in.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2011 Nicira Networks.
+ * Copyright (c) 2011, 2012 Nicira Networks.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -60,6 +60,27 @@ extern const struct in6_addr in6addr_any;
#define IPPROTO_NONE 59
#define IPPROTO_DSTOPTS 60
+/* All the IP options documented in Linux ip(7). */
+#define IP_ADD_MEMBERSHIP 0
+#define IP_DROP_MEMBERSHIP 1
+#define IP_HDRINCL 2
+#define IP_MTU 3
+#define IP_MTU_DISCOVER 4
+#define IP_MULTICAST_IF 5
+#define IP_MULTICAST_LOOP 6
+#define IP_MULTICAST_TTL 7
+#define IP_NODEFRAG 8
+#define IP_OPTIONS 9
+#define IP_PKTINFO 10
+#define IP_RECVERR 11
+#define IP_RECVOPTS 12
+#define IP_RECVTOS 13
+#define IP_RECVTTL 14
+#define IP_RETOPTS 15
+#define IP_ROUTER_ALERT 16
+#define IP_TOS 17
+#define IP_TTL 18
+
#define INADDR_ANY 0x00000000
#define INADDR_BROADCAST 0xffffffff
#define INADDR_NONE 0xffffffff
We usually put only one blank line between top-level definitions, so
please drop the following change:
> @@ -144,6 +145,7 @@ static bool is_connected_state(enum state);
> static bool is_admitted_msg(const struct ofpbuf *);
> static bool rconn_logging_connection_attempts__(const struct rconn *);
>
> +
> /* Creates and returns a new rconn.
> *
> * 'probe_interval' is a number of seconds. If the interval passes once
In reconnect() below, I don't see a reason to get a new dscp out of
the vconn. At this point we have just told the vconn what dscp we
want, I don't yet see the rationale for pulling it back out?
> @@ -335,11 +344,12 @@ reconnect(struct rconn *rc)
> VLOG_INFO("%s: connecting...", rc->name);
> }
> rc->n_attempted_connections++;
> - retval = vconn_open(rc->target, OFP10_VERSION, &rc->vconn);
> + retval = vconn_open(rc->target, OFP10_VERSION, &rc->vconn, rc->dscp);
> if (!retval) {
> rc->remote_ip = vconn_get_remote_ip(rc->vconn);
> rc->local_ip = vconn_get_local_ip(rc->vconn);
> rc->remote_port = vconn_get_remote_port(rc->vconn);
> + rc->dscp = vconn_get_dscp(rc->vconn);
> rc->backoff_deadline = time_now() + rc->backoff;
> state_transition(rc, S_CONNECTING);
> } else {
Below, the result of setsockopt() assigned to error is only used for
comparison against zero, so I'd be inclined to put the setsockopt()
call right into the 'if' statement. More importantly, the ip(7)
manpage says that the argument is supposed to be a byte, and we're
passing an int. I bet that won't work out properly on big-endian
systems; we should really pass in a byte:
> @@ -571,6 +571,18 @@ inet_open_active(int style, const char *target, uint16_t
> default_port,
> goto exit_close;
> }
>
> + /* 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) {
> + error = setsockopt(fd, IPPROTO_IP, IP_TOS, &dscp, sizeof dscp);
> + if (error) {
> + VLOG_ERR("%s: socket: %s", target, strerror(errno));
> + error = errno;
> + goto exit;
> + }
> + }
Similarly in inet_open_passive().
Please update the comments on inet_open_active() and
inet_open_passive() to explain the new 'dscp' parameter.
I don't see anything that initializes the 'dscp' member in a stream,
but I do see one place that reads it (indirectly, in reconnect()
above). I see two ways to go here:
* Initialize struct stream's 'dscp' properly, probably by
adding it as a parameter to stream_init().
* Drop 'dscp' entirely from struct stream and from struct
vconn, assuming that you decide that we can drop its use
from reconnect(). There aren't any other users, so we can
do without it. (If we need it later for some reason, we can
add it.)
I'm inclined toward the latter choice, since it is simpler.
The following change looks spurious to me:
> @@ -185,8 +187,8 @@ int ofproto_port_dump_done(struct ofproto_port_dump *);
> : (ofproto_port_dump_done(DUMP), false)); \
> )
>
> -#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
>
> int ofproto_port_add(struct ofproto *, struct netdev *, uint16_t *ofp_portp);
> int ofproto_port_del(struct ofproto *, uint16_t ofp_port);
Please put a space only before the '*' in writing a pointer type:
> @@ -98,7 +98,9 @@ struct ovsdb_jsonrpc_remote {
> };
>
> static struct ovsdb_jsonrpc_remote *ovsdb_jsonrpc_server_add_remote(
> - struct ovsdb_jsonrpc_server *, const char *name);
> + struct ovsdb_jsonrpc_server *, const char *name,
> + const struct ovsdb_jsonrpc_options * options
> +);
If you drop "Contains the" from the comment then you should be able to
fit it on one line:
> @@ -28,6 +28,8 @@ void ovsdb_jsonrpc_server_destroy(struct
> ovsdb_jsonrpc_server *);
> struct ovsdb_jsonrpc_options {
> int max_backoff; /* Maximum reconnection backoff, in msec. */
> int probe_interval; /* Max idle time before probing, in msec. */
> + int dscp; /* Contains the dscp value for manager
> + connection. */
> };
> struct ovsdb_jsonrpc_options *ovsdb_jsonrpc_default_options(void);
In ovsdb-server.c, the comment on read_hmap_column() doesn't match the
style we usually use. Can you try to make it fit in better? We have
some advice on this in CodingStyle and you should be able to find
examples around the source tree.
Also in read_hmap_column(), please put a space only after ";" here:
> + for (i = 0; i < datum->n ; i++) {
The assertion and the comment here makes me nervous. If we're only
working with string columns for now, then let's just implement a
function that only works with string columns (that is, only your
function read_hmap_string_column()). If the problem becomes more
general later, we can write a more general function. But I don't like
the idea much of a half-general function that contains internal
assumptions like this:
> + /* XXX As of now, the key-value pairs are expected to be of type
> + * string. If in future this assumption changes, revise the assert
> */
> + atom = &datum->keys[i];
> + assert(atom->string != NULL);
Please write a space between "if" and "(":
> + if(!strcmp(atom->string, key)){
> + return &datum->values[i];
> + }
> + }
> + return NULL;
> +}
In manager_get_other_config() please drop the space between the
function name and the "(" here:
> + if (read_hmap_string_column (row, "other_config", &temp_string, "dscp"))
> {
> + options->dscp = atoi(temp_string);
> + } else {
> + options->dscp = OPENVSWITCH_DEFAULT_DSCP;
> + }
> +
Please drop this "return" statement, it just takes up space:
> + return;
> +
> +}
Ethan recently added some functions that should make
controller_get_other_config() now unnecessary. (If it isn't, please
either improve the comment or delete it. Also the indentation is
wrong in the prototype.)
> +/* Controller's Other Configs */
> +static const char *
> +controller_get_other_config(const struct ovsrec_controller *controller,
> + const char *key,
> + const char *default_value)
> +{
> + const char *value;
> + value = get_ovsrec_key_value(controller->key_other_config,
> + controller->value_other_config,
> + controller->n_other_config, key);
> + return value ? value : default_value;
> +}
> +/* Extracts the dscp values for the controller. */
> +static void
> +controller_configure_dscp(const struct ovsrec_controller *c,
> + struct ofproto_controller *oc)
> +{
> + char default_value[5];
> +
> + snprintf(default_value, sizeof(default_value), "%d",
> + OPENVSWITCH_DEFAULT_DSCP);
> + oc->dscp = atoi(controller_get_other_config(c,
> + "dscp",
> + default_value));
Please drop this "return" statement:
> + return;
> +}
> +
>
> /* Port mirroring. */
In vswitch.xml, since we added new "other_config" columns to
Controller and Manager we need to mention them in "Common Columns",
like this:
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index e8771d5..3e5dccb 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -2681,6 +2681,7 @@
Columns</code> at the beginning of this document.
<column name="external_ids"/>
+ <column name="other_config"/>
</group>
</table>
@@ -2921,6 +2922,7 @@
Columns</code> at the beginning of this document.
<column name="external_ids"/>
+ <column name="other_config"/>
</group>
</table>
> + <group title="Connection Parameters">
> + <p>
In vswitch.xml, usually we capitalize the names of tables only if we
are talking about the table itself, rather than the concept that the
table represents. So I would write "controller" in lowercase here:
> + Additional configuration for a connection between the Controller
> + and the Open vSwitch.
> + </p>
> +
> + <column name="other_config" key="dscp"
> + type='{"type": "integer"}'>
Here, it would be nice to add a few words that describe what a DSCP
value is and what the meaning of the default value is:
> + The DSCP value passed when establishing the connection between
> + the Controller and the Open vSwitch. The connection must be reset
> + for the new DSCP values to take effect. If no value is
> + specified, a default value of 192 is chosen for connection
> + establishment.
> + </column>
> + </group>
> +
> +
> <group title="Common Columns">
> The overall purpose of these columns is described under <code>Common
> Columns</code> at the beginning of this document.
Similarly for the Manager table changes.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
