Hi, > > > > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c > > > > index d43cbcf..f88ba2c 100644 > > > > --- a/src/openvpn/block_dns.c > > > > +++ b/src/openvpn/block_dns.c > > > > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index, > > > const ADDRESS_FAMILY family) > > > > } > > > > return ipiface.Metric; > > > > } > > > > - return -err; > > > > + return -(int)err; > > > > } > > > > > > This, I can somewhat agree to, as "err" is an unsigned type so there > > > might be a hidden integer overflow if it happens to be "large". > > > Which it won't be, but still. > > > > Adding the `(int)` resolves the warning C4146: unary minus operator > > applied to unsigned type, result still unsigned. > > https://msdn.microsoft.com/en-us/library/4kh09110.aspx > > > > It doesn't change much, but it shut-ups one compiler warning. > > The original code could be argued to be logically wrong (though perfectly > legal C) if err becomes larger than a signed int. This change does not fix > that. > So what's the point? > > As an example, consider this > > err = 2147483649 (2 more than INT_MAX) > Original will return 2147483647 (not a -ve int) The new code will return the > same 2147483647 That is, the return value is +ve int which the caller will > wrongly interpret as valid result. > > This is hypothetical as Windows system err codes do not get that large. > But then the original is as good as the replacement except for a > C++-trained compiler being silenced.
I totally agree with your point, the original code might be logically wrong. If the interface metric is > INT_MAX (which ULONG theoretically could be), it'll get returned as a negative number => false-positive error condition. The get_interface_metric()also return error codes > INT_MAX as a positive numbers => invalid interface metric instead of error condition. As you described this case yourself. > To repeat, cast does not eliminate a potential for error, it just just hides > it. True and I totally agree with you. I just didn't dare to rewrite the function more thoroughly. Can I, please? Option 1: Add an `if` to limit metric to INT_MAX, and an `if` to return all errors greater than INT_MAX simply as -1. Option 2: redefine function prototype to return error code unmodified, and metrics using an extra parameter passed by reference. I'd prefer option 1 after reviewing all get_interface_metrics() calls. None of them cares about the exact error code. They just test for negative value and set the metrics to -1. As a matter of fact, the get_interface_metrics() itself could be rewritten to return -1 as the metric for any error in the first place, thus making the error checks after each function call redundant, thus simplifying code. Best regards, Simon ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
