On a minimal OS install, you probably don't care about captive portal detection anyway though, right?

Maybe we should just split that code out into a plugin and split the plugin out into a separate subpackage. Then cloud/server installs could just skip that package, and the libsoup dependency with it.

(Actually, currently we use libsoup in one other place; the SoupTLD API is used to adjust the resolv.conf search entries depending on whether the hostname is directly under a TLD or not... so we'd have to do something with that.)

(In reply to Dan Winship from comment #1)
> On a minimal OS install, you probably don't care about captive portal
> detection anyway though, right?

> Maybe we should just split that code out into a plugin and split the plugin
> out into a separate subpackage. Then cloud/server installs could just skip
> that package, and the libsoup dependency with it.

Wouldn't it be simpler to add a compile time option to either use libcurl or libsoup? If the former is more ubiquitous on Fedora, then we should build the package against libcurl.

Having some plugin infrastructure for connectivity checking seems more involved.


> (Actually, currently we use libsoup in one other place; the SoupTLD API is
> used to adjust the resolv.conf search entries depending on whether the
> hostname is directly under a TLD or not... so we'd have to do something with
> that.)

> > On a minimal OS install, you probably don't care about captive portal
> > detection anyway though, right?

Nope not really. There might be some IoT options where connectivity detection might be useful in a slightly different manner to captive portal but that's out of scope for this one,

> > Maybe we should just split that code out into a plugin and split the plugin
> > out into a separate subpackage. Then cloud/server installs could just skip
> > that package, and the libsoup dependency with it.

That would be one option.

> Wouldn't it be simpler to add a compile time option to either use libcurl or
> libsoup? If the former is more ubiquitous on Fedora, then we should build
> the package against libcurl.

Yes, that's what I was thinking. Both libcurl and libsoup are available but in the case of the former it's used for package updates and other infra so is always there.

> Having some plugin infrastructure for connectivity checking seems more
> involved.

Agreed.

> > (Actually, currently we use libsoup in one other place; the SoupTLD API is
> > used to adjust the resolv.conf search entries depending on whether the
> > hostname is directly under a TLD or not... so we'd have to do something with
> > that.)

I wonder if libcurl has the equiv functionality there too?

> I wonder if libcurl has the equiv functionality there too?

AFAICT, curl does not offer this functionality:

curl master $ git log -n 1 --format=oneline
511838f1d803f926156a99022ac2a715dbd541b4 CODE_STYLE: fix long-line guideline

curl master $ git grep '\bTLD\b'           
CHANGES.0:- For USE_LIBIDN builds: Added Top-Level-Domain (TLD) check of host-name
CHANGES.0:  via 'ace_hostname') are legal according to the TLD tables in libidn.
CHANGES.0:- Dylan Salisbury made libcurl no longer accept cookies set to a TLD only (it
docs/KNOWN_BUGS: Curl sends DNS requests for hostnames with a .onion TLD. This leaks
lib/url.c:    infof(data, "WARNING: TLD check for %s failed; %s\n",
lib/url.c:           "illegal" characters for this TLD */
tests/server/sws.c:         instead, IE we consider the TLD to be the test number. Test 123 can

The only TLD-related code in curl is to verify whether the provided characters are allowed for use in a TLD (using libidn).

I’ve also checked whether curl uses https://publicsuffix.org/list/ in any way (which libsoup does for providing the functionality in question), but can’t find any traces.

Having ported ostree to libsoup, one really awful thing about libcurl is that it doesn't come with a URL library; I ended up forking the libsoup bits:

https://github.com/ostreedev/ostree/blob/031d7898ccb4ab4aa5fe994b016cb785dad2b895/src/libostree/ostree-soup-uri.h

If we do any work in NM on this area I'd like to share code, at least as a git submodule.

Downstream bug: https://bugzilla.redhat.com/show_bug.cgi?id=1430489

(In reply to Colin Walters from comment #5)
> Having ported ostree to libsoup,

I mean libcurl (we also have a libsoup backend obviously)

(In reply to Colin Walters from comment #5)
> Having ported ostree to libsoup, one really awful thing about libcurl is
> that it doesn't come with a URL library; I ended up forking the libsoup bits:

(See also bug 489862)

https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/fg/libcurl_bgo752642

(In reply to Lubomir Rintel from comment #9)
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/fg/
> libcurl_bgo752642

> dns-manager: use libpsl directly


    ...instead of via libsoup. This makes it possible to do gTLD suffix checking
    even if we're building withoup libsoup support. libsoup uses libpsl anyway.

"libsoup uses libpsl" seems wrong to me.

 
typo: "Mozilla Public Suffic List"


+    $(LIBPSL_LIBS) \
     $(LIBSOUP_LIBS)
+    $(LIBCURL_LIBS)

LIBCURL_LIBS not yet added in this commit (and lacks \ in previous line).



while at it, could you implement DOMAIN_IS_VALID() as a static function?
-- instead of a macro the evaluates the argument possibly more then once.

static gboolean
DOMAIN_IS_VALID (const char *domain)
{
    if (!domain || !*domain)
        return FALSE;
#if WITH_LIBPSL
    ...
#endif
    return TRUE;
}





+    priv->curl_mhandle = curl_multi_init ();

curl_multi_init() can return NULL. If curl_global_init() or curl_multi_init() fails, NMConnectivity should gracefully continue to function.

Likewise, curl_easy_init() can fail.



pushed a fixup

7d8f4899333281deab171e3ff49d1685b0fbaf35