Hi Ben, Thanks for your comments.
Please find my questions/comments inline. On Fri, Mar 23, 2012 at 1:25 PM, Ben Pfaff <b...@nicira.com> wrote: > On Thu, Mar 22, 2012 at 10:24:33PM -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. > > > > Feature #10074 > > Requested-by: Rajiv Ramanathan <rramanat...@nicira.com> > > Signed-off-by: Mehak Mahajan <mmaha...@nicira.com> > > Thank you. This is very close. > > Before you submit this you will need to rebase against master. That > will cause a few conflicts but resolving them should not be too hard. > > "git am" reported some trailing whitespace, please remove it: > > /home/blp/ovs/.git/rebase-apply/patch:722: trailing whitespace. > error = stream_open_block(stream_open(unix_path, &stream, > DSCP_DEFAULT), > /home/blp/ovs/.git/rebase-apply/patch:1120: trailing whitespace. > > /home/blp/ovs/.git/rebase-apply/patch:1178: trailing whitespace. > manager_get_other_config(const struct ovsdb_row *row, > > Done ... Hopefully this is the last set of whitespaces :) > I get a compiler error: > > ../ofproto/connmgr.c: In function ‘set_pvconns’: > ../ofproto/connmgr.c:676: error: ‘OPENVSWITCH_INVALID_DSCP’ undeclared > (first use in this function) > ../ofproto/connmgr.c:676: error: (Each undeclared identifier is > reported only once > ../ofproto/connmgr.c:676: error: for each function it appears in.) > > Done. The patch has this, but my branch does not have the compile error. I guess i might have corrected it after i generated the patch (can't think of another explanation) ... > I think you figured out a way to drop > #define IPTOS_PREC_INTERNETCONTROL 0xc0 > from socket-util.h, can you post a version without that? > > Yes i took out the <linux/ip.h> and put the <netinet/ip.h> in socket-util.h. Since the compile goes through fine with this change, I assume that we should be good. > We should mention this new feature in NEWS. > > Not sure what is NEWS ? > You added some #if 0'd code in util.c, please remove it. > > This was related to another bug. The one where the strtoll was creating an issue. I had created a separate branch for this. Not sure why this is showing up in my dscp changes branch. I had committed the changes of both these branches before i generated a diff. I though commit would only commit to the branch. Isn't that correct ? > I noticed this in connmgr_run() in connmgr.c: > /* Passing default value for creation of the rconn */ > rconn = rconn_create(ofservice->probe_interval, 0, > DSCP_DEFAULT); > but I think that we can get the dscp value from struct ofservice > (adding a new 'dscp' member), which ofservice_reconfigure() would > initialize. > > This I assume is for the controller connection. So you are suggesting I should add a dscp member to the ofcreate structure as well. Am I correct ? > Before, when I asked you to inline read_map_column() into > read_map_string_column(), I didn't mean to add the "inline" keyword to > its definition. (I should have worded my comment more carefully.) I > meant to merge its code into read_map_string_column(). > > My bad. I will merge these two function. > Thanks, > > Ben. >
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
