[ovs-dev] [PATCHv3] DNS: Add basic support for asynchronous DNS resolving

[Yifeng Sun]

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 <blp at 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 <pkusunyifeng at 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.
>