[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