Re: [PATCH] connectivity: fix portal detection when using HTTP 204 based checks

2018-02-12 Thread Thomas Haller
On Mon, 2018-02-12 at 18:06 +0100, Aleksander Morgado wrote:
> If we're going to use a 'no content' URL (HTTP 204) to check
> connectivity, do not try to match prefix when the content is being
> received. This issue was making the check not work properly, as the
> content
> returned by the captive portal was assumed as expected (given that
> g_str_has_prefix(str,"") always returns TRUE!).
> 
> Also, rework a log message that was being emitted on portal detection
> to avoid specifying that the reason is the content being shorter than
> expected, as that same logic now applies to the case where too much
> content is received and none was expected.
> 
> Fixes: 88416394f8e0a51dae9f40a31ca0c8077f08808a
> ---
> 
> Hey Thomas,
> 
> Looks like the portal detection was broken with the previous
> implementation when using the HTTP 204 based logic :) Now tested with
> a real portal and this commit fixes the detection.
> 
> Also reworked one of the logs dumped when the portal is detected, as
> it didn't make much sense when the HTTP 204 based logic was used. I
> reused the same message that was being used in the other part of the
> code where the portal is also detected, hope that's not an issue
> (we're anyway printing the expected response code, so it would be
> clear from reading the logs which one of them both we're seeing).
> 
> Let me know what you think.
> 

Hi Aleksander,

looks right to me (again) :)

merged: 
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=a25d2e0a17636c799bc7f40e443f7c0131ba8f8d

best,
Thomas

signature.asc
Description: This is a digitally signed message part
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list


[PATCH] connectivity: fix portal detection when using HTTP 204 based checks

2018-02-12 Thread Aleksander Morgado
If we're going to use a 'no content' URL (HTTP 204) to check
connectivity, do not try to match prefix when the content is being
received. This issue was making the check not work properly, as the content
returned by the captive portal was assumed as expected (given that
g_str_has_prefix(str,"") always returns TRUE!).

Also, rework a log message that was being emitted on portal detection
to avoid specifying that the reason is the content being shorter than
expected, as that same logic now applies to the case where too much
content is received and none was expected.

Fixes: 88416394f8e0a51dae9f40a31ca0c8077f08808a
---

Hey Thomas,

Looks like the portal detection was broken with the previous implementation 
when using the HTTP 204 based logic :) Now tested with a real portal and this 
commit fixes the detection.

Also reworked one of the logs dumped when the portal is detected, as it didn't 
make much sense when the HTTP 204 based logic was used. I reused the same 
message that was being used in the other part of the code where the portal is 
also detected, hope that's not an issue (we're anyway printing the expected 
response code, so it would be clear from reading the logs which one of them 
both we're seeing).

Let me know what you think.

---
 src/nm-connectivity.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c
index bdaba91fd..656d02b9d 100644
--- a/src/nm-connectivity.c
+++ b/src/nm-connectivity.c
@@ -165,14 +165,16 @@ curl_check_connectivity (CURLM *mhandle, CURLMcode ret)
} else if (   !cb_data->response[0]
   && (curl_easy_getinfo (msg->easy_handle, 
CURLINFO_RESPONSE_CODE, _code) == CURLE_OK)
   && response_code == 204) {
-   /* If we got a 204 error (No content) and we 
actually requested no content,
-* report full connectivity. */
+   /* If we got a 204 response code (no content) 
and we actually
+* requested no content, report full 
connectivity. */
_LOG2D ("response with no content received, 
check successful");
c = NM_CONNECTIVITY_FULL;
} else {
/* If we get here, it means that 
easy_write_cb() didn't read enough
-* bytes to be able to do a match. */
-   _LOG2I ("response shorter than expected '%s'; 
assuming captive portal.",
+* bytes to be able to do a match, or that we 
were asking for no content
+* (204 response code) and we actually got 
some. Either way, that is
+* an indication of a captive portal */
+   _LOG2I ("response did not match expected 
response '%s'; assuming captive portal.",
cb_data->response);
c = NM_CONNECTIVITY_PORTAL;
}
@@ -309,7 +311,9 @@ easy_write_cb (void *buffer, size_t size, size_t nmemb, 
void *userdata)
memcpy (cb_data->msg + cb_data->msg_size, buffer, len);
cb_data->msg_size += len;

-   if (cb_data->msg_size >= strlen (cb_data->response)) {
+   /* Check matching prefix if a expected response is given */
+   if (   cb_data->response[0]
+   && cb_data->msg_size >= strlen (cb_data->response)) {
/* We already have enough data -- check response */
if (g_str_has_prefix (cb_data->msg, cb_data->response)) {
_LOG2D ("check successful.");
--
2.15.1
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list