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

Ben Pfaff blp at ovn.org
Wed Dec 13 19:02:18 UTC 2017


On Mon, Dec 04, 2017 at 06:03:40AM -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.
> 
> Signed-off-by: Yifeng Sun <pkusunyifeng at gmail.com>

Thanks a lot!  DNS support will be a great feature for our users.

I have some comments.

Usually, OVS functions use one of two calling conventions for reporting
errors.  coding-style.rst describes them this way:

    For functions that return a success or failure indication, prefer
    one of the following return value conventions:

    - An ``int`` where 0 indicates success and a positive errno value
      indicates a reason for failure.

    - A ``bool`` where true indicates success and false indicates
      failure.

The convention used here for dns_resolve() is different.  I'd prefer to
see it adopt one of the above styles.

Usually, we use a .h file for high-level documentation of a particular
code module, and then document particular functions in the .c file.
Since this module has very few public functions, this convention is not
as important, but I would still like to see it followed.

Usually, we use OVS_LIKELY and OVS_UNLIKELY only in
performance-sensitive inner loops.  I don't think that any of the DNS
code qualifies.

I am concerned about what could happen if the unbound library cannot be
initialized.  In that case, I think that every call to dns_resolve()
will try to initialize it again.  That is likely to log error messages
at a high rate.  I suggest either rate-limiting the log messages (with
VLOG_ERR_RL, etc.) or changing the initialization code so that it only
tries to initialize once and then permanently fails.  My guess is that
the latter is a better choice in this case.

Probably, ub_process() error logging should also be rate-limited.  It is
also worth thinking about resolve_callback__()'s logging, to figure out
whether it is likely to be too voluminous in practice.

I think that resolve_find_or_new__() could be made very slightly simpler
by using shash_find_data().

xcalloc(1, x) can be written xzalloc(x).

In some of the dns_resolve() error cases, *addr is not set to NULL.
It's a better user interface if *addr is always set to NULL on error.

These days, we tend to try to declare and initialize variables at the
same point, so in dns_resolve() I would move the declarations of 'req'
and 'retval' down to their points of first use.

coding-style.rst says:

    Do parenthesize a subexpression that must be split across more than
    one line, e.g.::

        *idxp = ((l1_idx << PORT_ARRAY_L1_SHIFT) |
                 (l2_idx << PORT_ARRAY_L2_SHIFT) |
                 (l3_idx << PORT_ARRAY_L3_SHIFT));
so that:
    return req != NULL
        && req->state == RESOLVE_GOOD
        && !resolve_check_expire__(req);
should become:
    return (req != NULL
            && req->state == RESOLVE_GOOD
            && !resolve_check_expire__(req));

resolve_set_time__() seems simple enough to me that the code would be
clearer if it were just written inline instead of as a function.

I am not sure why the 'addr' member of struct resolve_request is marked
const.  I do not understand the benefit in this case.

I see better why the 'name' member of struct resolve_request is marked
const.  It is because it duplicates the name in the shash_node and
should not be separately freed.  But I think that it would be a cleaner
design in this case to use an hmap directly, by adding an hmap_node
member to struct resolve_request and then changing all_reqs__ from
struct shash to struct hmap.

I do not understand why all_reqs exists.  It seems that, each time it is
used, one could write &all_reqs__ instead.

Initialization in dns_resolve() and destruction in dns_resolve_destroy()
seems asymmetric in at least one way: dns_resolve_destroy() destroys
all_reqs, but dns_resolve_init__() does not initialize all_reqs.  I
believe that this means that calling dns_resolve_destroy(), then
resolving an address, will cause a segfault.  It is better to avoid that
potential problem.

In resolve_callback__(), the cast here is not needed:
    struct resolve_request *req;

    req = (struct resolve_request *)data;
I would write it as:
    struct resolve_request *req = data;

Similarly, in resolve_async__(), this cast is not needed either:
                                  qtype, ns_c_in, (void *)req,

Actually, common OVS style in a case like this is to give the parameter
a _ suffix, like this:

static void
resolve_callback__(void *req_, int err, struct ub_result *result)
    OVS_REQUIRES(dns_mutex__)
{
    struct resolve_request *req = req_;

Thanks,

Ben.


More information about the dev mailing list