Hi Ben, Thanks for the review and comments. Will rectify the issues and get back with the new patch.
Regards, Jai ________________________________ From: Ben Pfaff <[email protected]> Sent: 04 August 2017 02:20 To: Jai Singh Rana Cc: [email protected]; Rana, JaiSingh Subject: Re: [ovs-dev] [PATCH] controller: Remote connection option to OpenFlow switch. On Mon, Jul 31, 2017 at 04:31:23PM +0530, Jai Singh Rana wrote: > Currently ovn-controller uses default unix domain socket in Open Vswitch's > "run" directory to connect to OpenFlow switch. If > ovn-controller/ovsdb-server and vswitchd is running on different systems, > remote connection method needs to be provided. > > Added configuration option to override default connection by using OVSDB > external-ids "ovn-ofswitch-remote". Using this external-id, desired tcp > or unix connection to OpenFlow switch can be specified. > > Tested this by using tcp/unix method configured through external-id > "ovn-ofswitch-remote" and confirmed connection as flows getting updated in > Open vSwitch. > > Signed-off-by: Jai Singh Rana <[email protected]> Thank you for your contribution. Clang says: ../ovn/controller/ofctrl.c:464:28: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] ../ovn/controller/pinctrl.c:1034:28: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] GCC gives similar warnings. + <dt><code>external_ids:ovn-ofswitch-remote</code></dt> + <dd> The following line seems out of place. Remove it? + <code>ovn-ofswitch-remote</code> Please capitalize "OpenFlow" correctly: + The OpenFLow connection method that this system can connect to reach Please don't check for NULL before calling free (see the coding style guide): + if (ovn_ofswitch_remote) { + free(ovn_ofswitch_remote); + } This patch breaks the logic of what switch to contact into multiple places. I don't understand why. I think that get_ovn_ofswitch_remote() should always return the right switch to contact, instead of sometimes returning NULL. Thanks, Ben. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
