GNOME Bugzilla – Bug 711684
[REVIEW] Propose adding wrapper functions for inet_ntop
Last modified: 2013-12-09 16:26:54 UTC
Created attachment 259274 [details] [review] [PATCH v1 1/3] libnm-util: add nm_utils_inet[6]_ntop functions Add utility functions to replace inet_ntop. I propose a patch for review. The problem with inet_ntop are: (1) in NM it usually needs 3 lines of code, just to get a simple printf: char *buf[INET6_ADDRSTRLEN]; inet_ntop (AF_INET6, in6addr, buf, INET6_ADDRSTRLEN); printf ("%s", buf); This can become printf ("%s", nm_utils_inet6_ntop (in6addr)); (2) inet_ntop can report errors, so there are many places in NM where the result is checked. At many places, OTOH it is not checked for failure. Indeed, (looking at the man), it can only fail, if you provide invalid family or buffer size. It cannot(!) fail depending on in_addr_t or struct in6_net * and error checking is pointless. (3) some modules already define their wrapper functions: _nm_utils_inet6_ntop, ip6_address_to_string. Unify them.
Created attachment 259275 [details] [review] [PATCH v1 1/3] refactor to make use of new nm_utils_inet[6]_ntop functions Show, that the above patch really simplifies code. I don't propose to replace every existing use of inet_ntop, but this is just show, that it really simplifies code and is neat to use (IMO)
Created attachment 259276 [details] [review] [PATCH v1 3/3] core: refactor ip6_addr_to_string in nm-dns-dnsmasq Refactor ip6_addr_to_string in nm-dns-dnsmasq
I see what you're up to. But in a single threaded environment you can simplify it even more by using a static buffer in the function. Also consider whether you really want to have two separate functions for it. If yes, please use ip4/ip6 to be consistent with the rest of the code, instead of inet/inet6 (especially try to avoid having names with 6 and without 4), and it might be good to call it *_to_string or something like that.
It uses a static buffer, if you set the buffer to NULL. That is how I use it most of the time. But sometimes it is still nice to provide your own buffer. This way, you have the option. Disadvantage is, that most of the time you will not need it and pass an additional NULL. For example: char buf[INET6_ADDRSTRLEN]; nm_utils_inet6_ntop (&route->network, buf); printf ("route to %s via %s", buf, nm_utils_inet6_ntop (&route->gateway, NULL)); yeah, I know, you could do: char *buf; buf = g_strdup (nm_utils_inet6_ntop (&route->network)); printf ("route to %s via %s", buf, nm_utils_inet6_ntop (&route->gateway, NULL)); g_free (buf); or: char buf[INET6_ADDRSTRLEN]; strcpy (buf, nm_utils_inet6_ntop (&route->network)); printf ("route to %s via %s", buf, nm_utils_inet6_ntop (&route->gateway, NULL)); I like the first best, but it's a matter of taste. I aggree nm_utils_inet4_ntop would be better for symmetry. I would not combine them in one function as inet_ntop does. The name, is also a matter of taste. I like it this way because it makes clear, that it's a wrapper for inet_ntop. Other opinions?
we could also: #define nm_utils_inet4_ntop_s(a) nm_utils_inet4_ntop((a), NULL) #define nm_utils_inet6_ntop_s(a) nm_utils_inet6_ntop((a), NULL) _s for _static (or whatever)... Or we add real functions, but that might be overkill...
(In reply to comment #4) > It uses a static buffer, if you set the buffer to NULL. That is how I use it > most of the time. But sometimes it is still nice to provide your own buffer. > > This way, you have the option. Disadvantage is, that most of the time you will > not need it and pass an additional NULL. Good. That's not a problem if you actually benefit from providing your own buffer often. If it's rare, using strcpy() or similar might be a better idea. Or you might want to have two functions, the basic one having the static buffer and calling the other one which would call inet_ntop on the buffer and return the buffer. > For example: > > char buf[INET6_ADDRSTRLEN]; > > nm_utils_inet6_ntop (&route->network, buf); > printf ("route to %s via %s", buf, > nm_utils_inet6_ntop (&route->gateway, NULL)); What about the following? printf ("route to %s via %s", strdupa(nm_utils_inet6_ntop (&route->network)), strdupa(nm_utils_inet6_ntop (&route->gateway))); > I aggree nm_utils_inet4_ntop would be better for symmetry. I would not combine > them in one function as inet_ntop does. > The name, is also a matter of taste. I > like it this way because it makes clear, that it's a wrapper for inet_ntop. I vote for ip4/ip6 to keep consistent, or even use ip4_to_string/ip6_to_string, I don't think relation (or even call) to libc inet_ntop is of any importance, here. > Other opinions? Nope.
Renamed nm_utils_inet_ntop, and pushed the current version to th/bgo711684_inet_ntop ...
>libnm-util: add nm_utils_inet[46]_ntop functions >+static char _nm_utils_inet4_ntop_buffer[NM_UTILS_INET_ADDRSTRLEN]; odd to have "4" in the name since it's used for both >+ * @dst: the destination buffer, it must contain at least INET6_ADDRSTRLEN >+ * characters. If set to NULL, it will return a pointer to an internal, static named constants in gtk-doc should be prefixed with "%". (INET6_ADDRSTRLEN and NULL). Also, you meant "INET_ADDRSTRLEN" here anyway (nm_utils_inet4_ntop). >+ * Returns: (transfer none): the input buffer @dst, or a pointer to an (You don't need to say "transfer none" if the return value is "const".) >+ if (!in6addr) { >+ if (dst) >+ dst[0] = 0; >+ return NULL; >+ } That's... weird. "If you pass dst, the function returns dst, unless you didn't pass in6addr, in which case it returns NULL, but still modifies dst anyway". It looks like most places that use this in the next patch wouldn't ever pass NULL, and if they did, it would be a mistake. So I think we should just g_return_val_if_fail() instead. >+#define NM_UTILS_INET_ADDRSTRLEN ( INET_ADDRSTRLEN > INET6_ADDRSTRLEN ? INET_ADDRSTRLEN : INET6_ADDRSTRLEN ) We know that INET6_ADDRSTRLEN is longer. You don't have to check. >refactor to make use of nm_utils_inet[46]_ntop functions I think usually we prepend "all:" to commit messages where the changes span across the whole tree You updated nm-ip4-config to use the new macros, but you missed nm-ip6-config. > nm_log_info (LOGD_VPN, " Forbid Default Route: %s", >- nm_ip6_config_get_never_default (priv->ip6_config) ? "yes" : "no"); >+ nm_ip6_config_get_never_default (priv->ip6_config) ? "yes" : "no"); the indentation here was correct before and your patch breaks it. >core: refactor ip6_addr_to_string in nm-dns-dnsmasq You can simplify this even more now, just using g_strdup_printf() rather than having the code measuring the string itself and trying to malloc a buffer of the right size.
> libnm-util: add nm_utils_inet[46]_ntop functions I would write stronger documentation here about the static buffer; for example, you should not use this function passing %NULL for two arguments in the same printf()-style function, for example. Passing %NULL is not thread-safe. When in doubt, always pass a buffer. Or something like that. Just to make it ultra-clear that if you're passing %NULL, you need to be really really sure that's what you want to do. > refactor to make use of nm_utils_inet[46]_ntop functions "str_help" seems wrong to me as a name; perhaps just keep "str_gw", since this variable is only used for the gateway address and the next hop address, and the next hop is actually just a gateway by another name... Rest of the changes look OK.
(In reply to comment #8) Pushed a !fixup for all your suggestions, except: > (You don't need to say "transfer none" if the return value is "const".) > > >+ if (!in6addr) { > >+ if (dst) > >+ dst[0] = 0; > >+ return NULL; > >+ } > > That's... weird. "If you pass dst, the function returns dst, unless you didn't > pass in6addr, in which case it returns NULL, but still modifies dst anyway". > > It looks like most places that use this in the next patch wouldn't ever pass > NULL, and if they did, it would be a mistake. So I think we should just > g_return_val_if_fail() instead. In my opinion, functions should often just accept NULL as valid argument and do something reasonable and documented, especially for to_string(), is_something(), has_something() and get_something() kind of functions. In this case, I consider it reasonable to return NULL. > You updated nm-ip4-config to use the new macros, but you missed nm-ip6-config. Did now also nm-ip6-config. My intention however was not to replace all occurrences (yet). Only a few cases to show that this utility function is indeed useful. If we all agree on it's usefulness, we might use it in other places as well.
(In reply to comment #9) done, and !fixup pushed
(In reply to comment #10) > In my opinion, functions should often just accept NULL as valid argument and do > something reasonable and documented, especially for to_string(), > is_something(), has_something() and get_something() kind of functions. In this > case, I consider it reasonable to return NULL. I guess I agree in the general case, but it doesn't make sense to me here. You can't pass NULL to printf or nm_log or the like, so it's just going to come down to a choice between: if (nexthop) printf ("Next hop: %s\n", nm_utils_inet6_ntop (nexthop, NULL)); or if ((str = nm_utils_inet6_ntop (nexthop, NULL))) printf ("Next hop: %s\n", str); (And the fact that it sets dst to "" in this case is also weird. You wouldn't want to print "Next hop: \n" when there was no next hop...)
> (And the fact that it sets dst to "" in this case is also weird. You wouldn't > want to print "Next hop: \n" when there was no next hop...) A common use case for a to_string function is to print a value while printf debugging. I would want the function to return something, that does not interfere and does not force me to special caseing around NULL. True, you cannot pass NULL to printf without having undefined behaviour. But actually, on my machine it works just fine and prints "(none)" -- good enough for some quick debugging. In the end, it's a matter of opinion. I can surely change it. Anyone?
I often take advantage of printf()'s NULL handling, but usually only for debugging. Some standard libraries will still crash if you pass NULL (eglibc? I forget which ones), though I don't think any of those are relevant to us. Just keep in mind that glibc's printf() behavior isn't universal :) My preference would be to have the function g_return_val_if_fail() on NULL address, since that would usually be a bug? It would be OK for logging stuff, but certainly if we use these functions to construct a command-line (for connection sharing iptables commands, for example) or when writing values to config files, it would be wrong to pass NULL, and it would also be wrong to return an empty buffer since that could cause mis-formatted config files and command lines.
(In reply to comment #14) > My preference would be to have the function g_return_val_if_fail() on NULL > address, since that would usually be a bug?... Ok, !fixup pushed with g_return_val_if_fail()
Pushed to upstream master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=41f8114359404ae2cf14724d84eccfc378e9317c http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=6f2cfe263eabd452ad4efeb979ca9ef1db0578d7 http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=a032d82e2ade03ac0393c6d2e80558221944c6e1