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 <pkusunyif...@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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev