Thanks a lot Ben for your review. I will apply your advice in v2. Yifeng
On Wed, Dec 13, 2017 at 11:02 AM, Ben Pfaff <b...@ovn.org> wrote: > 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
