Hi Ben, Thank you very much for the code review, I will fix and come up with v4 soon.
Best, Yifeng On Wed, Dec 20, 2017 at 9:32 AM, Ben Pfaff <b...@ovn.org> wrote: > On Tue, Dec 19, 2017 at 05:08:42AM -0800, Yifeng Sun wrote: > > This patch is a simple implementation for the proposal discussed in > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-August/337038.html > and > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/340013.html. > > > > It enables ovs-vswitchd to use DNS names when specifying OpenFlow and > > OVSDB remotes. > > > > Below are some of the features and limitations of this patch: > > - The resolving is asynchornous, so it doesn't block the main loop; > > - Both IPv4 and IPv6 are supported; > > - The resolving API is thread-safe; > > - Depends on the unbound library; > > - When multiple ip addresses are returned, only the first one is > used; > > - /etc/nsswitch.conf isn't respected as unbound library doesn't look > at it; > > - Depends on caller to retry later to use the resolved results. > > > > v1 -> v2: refactored and improved code based on reviewer's comments. > > v2 -> v3: add commit message. > > > > Signed-off-by: Yifeng Sun <pkusunyif...@gmail.com> > > Thanks for v3! I think that many users are looking forward to DNS > support. > > Thanks for including a change log (e.g. v1->v2, ...). However, the > change log should go after the --- line in the email; otherwise, "git > am" puts it into the commit message (and it should not be in there). > You can do that by putting --- into your commit message, or by adding > the change log manually within the editor invoked by "git send-email > --annotate". > > This commit should update the documentation. I imagine that in several > places the documentation says that only IP addresses are supported, and > that should now be fixed. > > This commit should add a NEWS entry. > > This commit should update the build instructions to describe the new > optional dependency. > > This commit should add the proper dependencies to the Debian, RHEL, and > Fedora packaging. > > The copyright notices in the new files look wrong to me. This code is > new this year and as far as I know it is not derived from any older > code. If so, then that means that the copyright year should be 2017 and > not any other years. > > The dns-resolve code uses a strategy of making the client understand at > compile time whether the resolver is available, using an #ifdef. This > is usually not the strategy that we use in Open vSwitch, because it > means that the knowledge whether a feature is available has to be spread > throughout the code. Instead, we try to centralize it in a single > location to the extent practical, by writing appropriate stubs that do > nothing or return an error when the feature is not available. You can > see a few examples of this strategy in lib/async-append*.[ch], > lib/route-table*.[ch], and lib/if-notifier*.[ch]. We usually find that > this makes code easier to understand and maintain. > > Please consider the un-ratelimited log messages that this commit adds. > I think that some of them could flood the logs if they triggered. If > so, please ratelimit them. > > Thanks, > > Ben. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev