| Message ID | 20231116010259.628527-1-gerickson@nuovations.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Address Redundant IPv4 Reachability Checks | expand |
Hi Grant, > This addresses an issue in which, when 'EnableOnlineCheck' is > asserted, two (2) concurrent IPv4 and one (1) IPv6 reachability checks > get queued. > > The issue is addressed by introducing and leveraging > '__connman_wispr_cancel' in 'cancel_online_check' to ensure that a > pending or in-flight WISPr portal detection request or an "online" > HTTP-based Internet reachability check is canceled not only in the > service module but in the WISPr module as well. > > This prevents two concurrent, outstanding, and redundant IPv4 WISPr > requests from being in-flight when EnableOnlineCheck is asserted. > > Prior to these changes, this redundancy was set in motion by: > > 1. The two back-to-back IPv4 probes get triggered by both of the > following paths: > > a. Triggered from 'default_changed' and > 'start_wispr_if_connected'. This is the newer path, > chronologically, in the code base. > > b. Triggered from 'service_ip_bound', 'address_updated', and > 'start_online_check'. This is the older path, chronologically, > in the code base. > > 2. With the following commits addressed: > > a. 812e171 ("Close three '__connman_wisp_start' failure closure > holes.") > > * With this fix alone, at minimum, the first probe above (1) > would have resulted, eventually in a failure closure with > 'complete_online_check'. > > b. 5f95851 ("wispr: Avoid 'connman_proxy_lookup' call for > 'UNKNOWN' proxy method.") fixed, the first IPv4 probe from > (1) no longer "falls down a 'hole'" and gets lost (closure- > and accounting-wise). > > * With this fix, the first IPv4 probe is no longer a > complete throwaway--it actually succeeds. However, since > it does so, it becomes the first of the two redundant IPv4 > checks. > > By employing '__connman_wispr_cancel', 'cancel_online_check' quells > the first IPv4 check triggered from 'default_changed' and > 'start_wispr_if_connected' and replaces it, fully (in both the service > and WISpr modules), with the second IPv4 check triggered from > 'service_ip_bound', 'address_updated', and 'start_online_check'. > > Grant Erickson (17): > wispr: Rename context proxy-related data members. > wispr: Add and leverage 'is_wispr_supported'. > wispr: Add documentation to 'is_wispr_supported'. > service: Document service unref calls in 'cancel_online_check'. > wispr: Add documentation to 'wispr_portal_detect'. > wispr: Add documentation to '__connman_wispr_stop'. > wispr: Add and leverage 'cancel_connman_wispr_portal_context'. > wispr: Add documentation to 'cancel_connman_wispr_portal_context'. > gweb: Add optional OS error status parameter to 'g_web_request_*'. > wispr: Add and leverage an OS error parameter to > '__connman_wispr_cb_t'. > service: Add IP configuration type to 'cancel_online_check'. > gweb: Add reference count debug statements. > gweb: Factor out GWeb destruction into 'g_web_free'. > wispr: Add '__connman_wispr_cancel' interface. > wispr: Add documentation to '__connman_wispr_cancel'. > wispr: Avoid double-free in 'free_wispr_routes'. > service: Leverage '__connman_wispr_cancel'. > > gweb/gweb.c | 88 ++++++++---- > gweb/gweb.h | 8 +- > src/6to4.c | 2 +- > src/connman.h | 5 +- > src/service.c | 70 ++++++++-- > src/wispr.c | 341 ++++++++++++++++++++++++++++++++++++++++------- > tools/web-test.c | 3 +- > tools/wispr.c | 8 +- > 8 files changed, 433 insertions(+), 92 deletions(-) I got this when trying to build the patch set. CC gweb/gweb.o gweb/gweb.c: In function ‘do_request’: gweb/gweb.c:2357:57: error: comparison of unsigned expression in ‘< 0’ is always false [-Werror=type-limits] 2357 | status = session->resolv_action < 0 ? | ^ gweb/gweb.c:2359:49: error: operand of ‘?:’ changes signedness from ‘int’ to ‘guint’ {aka ‘unsigned int’} due to unsignedness of other operand [-Werror=sign-compare] 2359 | -ENOENT; | ^ cc1: all warnings being treated as errors REgards Marcel
On 19.11.2023 06:12, Marcel Holtmann wrote: > > [ ... ] > > I got this when trying to build the patch set. > > CC gweb/gweb.o > gweb/gweb.c: In function ‘do_request’: > gweb/gweb.c:2357:57: error: comparison of unsigned expression in ‘< 0’ > is always false [-Werror=type-limits] > 2357 | status = session->resolv_action < 0 ? > | ^ > gweb/gweb.c:2359:49: error: operand of ‘?:’ changes signedness from > ‘int’ to ‘guint’ {aka ‘unsigned int’} due to unsignedness of other > operand [-Werror=sign-compare] > 2359 | -ENOENT; > | ^ > cc1: all warnings being treated as errors Marcel, Thank you; I've addressed this error and that in 'is_wispr_supported'. Submitted as a v2 on this thread. Best, Grant
This addresses an issue in which, when 'EnableOnlineCheck' is asserted, two (2) concurrent IPv4 and one (1) IPv6 reachability checks get queued. The issue is addressed by introducing and leveraging '__connman_wispr_cancel' in 'cancel_online_check' to ensure that a pending or in-flight WISPr portal detection request or an "online" HTTP-based Internet reachability check is canceled not only in the service module but in the WISPr module as well. This prevents two concurrent, outstanding, and redundant IPv4 WISPr requests from being in-flight when EnableOnlineCheck is asserted. Prior to these changes, this redundancy was set in motion by: 1. The two back-to-back IPv4 probes get triggered by both of the following paths: a. Triggered from 'default_changed' and 'start_wispr_if_connected'. This is the newer path, chronologically, in the code base. b. Triggered from 'service_ip_bound', 'address_updated', and 'start_online_check'. This is the older path, chronologically, in the code base. 2. With the following commits addressed: a. 812e171 ("Close three '__connman_wisp_start' failure closure holes.") * With this fix alone, at minimum, the first probe above (1) would have resulted, eventually in a failure closure with 'complete_online_check'. b. 5f95851 ("wispr: Avoid 'connman_proxy_lookup' call for 'UNKNOWN' proxy method.") fixed, the first IPv4 probe from (1) no longer "falls down a 'hole'" and gets lost (closure- and accounting-wise). * With this fix, the first IPv4 probe is no longer a complete throwaway--it actually succeeds. However, since it does so, it becomes the first of the two redundant IPv4 checks. By employing '__connman_wispr_cancel', 'cancel_online_check' quells the first IPv4 check triggered from 'default_changed' and 'start_wispr_if_connected' and replaces it, fully (in both the service and WISpr modules), with the second IPv4 check triggered from 'service_ip_bound', 'address_updated', and 'start_online_check'. Grant Erickson (17): wispr: Rename context proxy-related data members. wispr: Add and leverage 'is_wispr_supported'. wispr: Add documentation to 'is_wispr_supported'. service: Document service unref calls in 'cancel_online_check'. wispr: Add documentation to 'wispr_portal_detect'. wispr: Add documentation to '__connman_wispr_stop'. wispr: Add and leverage 'cancel_connman_wispr_portal_context'. wispr: Add documentation to 'cancel_connman_wispr_portal_context'. gweb: Add optional OS error status parameter to 'g_web_request_*'. wispr: Add and leverage an OS error parameter to '__connman_wispr_cb_t'. service: Add IP configuration type to 'cancel_online_check'. gweb: Add reference count debug statements. gweb: Factor out GWeb destruction into 'g_web_free'. wispr: Add '__connman_wispr_cancel' interface. wispr: Add documentation to '__connman_wispr_cancel'. wispr: Avoid double-free in 'free_wispr_routes'. service: Leverage '__connman_wispr_cancel'. gweb/gweb.c | 88 ++++++++---- gweb/gweb.h | 8 +- src/6to4.c | 2 +- src/connman.h | 5 +- src/service.c | 70 ++++++++-- src/wispr.c | 341 ++++++++++++++++++++++++++++++++++++++++------- tools/web-test.c | 3 +- tools/wispr.c | 8 +- 8 files changed, 433 insertions(+), 92 deletions(-)