[PATCH] channel_curl: Fix getting the protocol for curl >= 7.85.0

[Storm, Christian]

CURLINFO_PROTOCOL, deprecated in 7.85.0, uses a long to represent
the protocol used while the newer CURLINFO_SCHEME uses a string.

See https://curl.se/libcurl/c/CURLINFO_PROTOCOL.html
and https://curl.se/libcurl/c/CURLINFO_SCHEME.html

Signed-off-by: Christian Storm <christi...@siemens.com>
---
corelib/channel_curl.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c
index 368330bd..c68898f0 100644
--- a/corelib/channel_curl.c
+++ b/corelib/channel_curl.c
@@ -307,7 +307,11 @@ char *channel_get_redirect_url(channel_t *this)
channel_op_res_t channel_map_http_code(channel_t *this, long *http_response_code)
{
char *url = NULL;
- long protocol;
+ #if LIBCURL_VERSION_NUM >= 0x75500
+ char *protocol = NULL;
+ #else
+ long protocol = 0;
+ #endif
channel_curl_t *channel_curl = this->priv;
CURLcode curlrc =
curl_easy_getinfo(channel_curl->handle, CURLINFO_RESPONSE_CODE,
@@ -329,7 +333,14 @@ channel_op_res_t channel_map_http_code(channel_t *this, long *http_response_code
CURLINFO_PROTOCOL,
#endif
&protocol);
- if (curlrc == CURLE_OK && protocol == CURLPROTO_FILE) {
+ if (curlrc == CURLE_OK
+ #if LIBCURL_VERSION_NUM >= 0x75500
+ && protocol
+ && !strncasecmp(protocol, "file", 4)
+ #else
+ && protocol == CURLPROTO_FILE
+ #endif
+ ) {
return CHANNEL_OK;
}
DEBUG("No HTTP response code has been received yet!");
--
2.45.2

[Dominique MARTINET]

'Storm, Christian' via swupdate wrote on Fri, Jun 07, 2024 at 08:13:33PM +0000:
> CURLINFO_PROTOCOL, deprecated in 7.85.0, uses a long to represent
> the protocol used while the newer CURLINFO_SCHEME uses a string.

Good catch!
It's a shame the API doesn't allow type checking, there's no way the
current code would work as intended for this version...

> See https://curl.se/libcurl/c/CURLINFO_PROTOCOL.html
> and https://curl.se/libcurl/c/CURLINFO_SCHEME.html
>
> Signed-off-by: Christian Storm <christi...@siemens.com>

Wrote a nitpick below, but:
Reviewed-by: Dominique Martinet <dominique...@atmark-techno.com>

> ---
> corelib/channel_curl.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c
> index 368330bd..c68898f0 100644
> --- a/corelib/channel_curl.c
> +++ b/corelib/channel_curl.c
> @@ -307,7 +307,11 @@ char *channel_get_redirect_url(channel_t *this)
> channel_op_res_t channel_map_http_code(channel_t *this, long *http_response_code)
> {
> char *url = NULL;
> - long protocol;
> + #if LIBCURL_VERSION_NUM >= 0x75500

(ugh, #if statements in this files are split between about half starting
from start of line and the other half being indented...)

> + char *protocol = NULL;
> + #else
> + long protocol = 0;
> + #endif
> channel_curl_t *channel_curl = this->priv;
> CURLcode curlrc =
> curl_easy_getinfo(channel_curl->handle, CURLINFO_RESPONSE_CODE,
> @@ -329,7 +333,14 @@ channel_op_res_t channel_map_http_code(channel_t *this, long *http_response_code
> CURLINFO_PROTOCOL,
> #endif
> &protocol);
> - if (curlrc == CURLE_OK && protocol == CURLPROTO_FILE) {
> + if (curlrc == CURLE_OK
> + #if LIBCURL_VERSION_NUM >= 0x75500
> + && protocol
> + && !strncasecmp(protocol, "file", 4)

I don't see the merit of the 'n' variant here, we don't know how big an
allocation curl made so we can only trust it, and in these case I
generally use the non-bound variant to avoid counting twice how long
static strings are.
In this particular case, with 4 you'd accept URLs like filewhatever://,
I'm not sure such scheme exists but is that intended?

> + #else
> + && protocol == CURLPROTO_FILE
> + #endif
> + ) {
> return CHANNEL_OK;
> }
> DEBUG("No HTTP response code has been received yet!");

--

Dominique

[Storm, Christian]

Hi Dominique,

>> CURLINFO_PROTOCOL, deprecated in 7.85.0, uses a long to represent
>> the protocol used while the newer CURLINFO_SCHEME uses a string.
>
> Good catch!
> It's a shame the API doesn't allow type checking, there's no way the
> current code would work as intended for this version...

Thanks! :)

>> See https://curl.se/libcurl/c/CURLINFO_PROTOCOL.html
>> and https://curl.se/libcurl/c/CURLINFO_SCHEME.html
>>
>> Signed-off-by: Christian Storm <christi...@siemens.com>
>
> Wrote a nitpick below, but:
> Reviewed-by: Dominique Martinet <dominique...@atmark-techno.com>
>
>> ---
>> corelib/channel_curl.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c
>> index 368330bd..c68898f0 100644
>> --- a/corelib/channel_curl.c
>> +++ b/corelib/channel_curl.c
>> @@ -307,7 +307,11 @@ char *channel_get_redirect_url(channel_t *this)
>> channel_op_res_t channel_map_http_code(channel_t *this, long *http_response_code)
>> {
>> char *url = NULL;
>> - long protocol;
>> + #if LIBCURL_VERSION_NUM >= 0x75500
>
> (ugh, #if statements in this files are split between about half starting
> from start of line and the other half being indented...)

Yes, I tend to prefer the preprocessor stuff starting at the beginning of the line but as it is all over the place in channel_curl.c, I tend to like the indented form more because otherwise I can't see the forest for the trees :)
Regardless, I don't have strong opinions in one way or another if we settle on one, consistently...

>> + char *protocol = NULL;
>> + #else
>> + long protocol = 0;
>> + #endif
>> channel_curl_t *channel_curl = this->priv;
>> CURLcode curlrc =
>> curl_easy_getinfo(channel_curl->handle, CURLINFO_RESPONSE_CODE,
>> @@ -329,7 +333,14 @@ channel_op_res_t channel_map_http_code(channel_t *this, long *http_response_code
>> CURLINFO_PROTOCOL,
>> #endif
>> &protocol);
>> - if (curlrc == CURLE_OK && protocol == CURLPROTO_FILE) {
>> + if (curlrc == CURLE_OK
>> + #if LIBCURL_VERSION_NUM >= 0x75500
>> + && protocol
>> + && !strncasecmp(protocol, "file", 4)
>
> I don't see the merit of the 'n' variant here, we don't know how big an
> allocation curl made so we can only trust it, and in these case I
> generally use the non-bound variant to avoid counting twice how long
> static strings are.
> In this particular case, with 4 you'd accept URLs like filewhatever://,
> I'm not sure such scheme exists but is that intended?

Actually yes, as it's the protocol, not the URL. The protocol reads `http`, `file`, ... either in UPPERCASE or lowercase as mentioned in the curl docs. Since I'm interested in `file` only, I opted for bounding it.
However, I don't care too much and can switch to the non-`n` variant?

>> + #else
>> + && protocol == CURLPROTO_FILE
>> + #endif
>> + ) {
>> return CHANNEL_OK;
>> }
>> DEBUG("No HTTP response code has been received yet!");

Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T CED OES-DE
Otto-Hahn-Ring 6, 81739 Munich, Germany

[Dominique MARTINET]

'Storm, Christian' via swupdate wrote on Mon, Jun 10, 2024 at 08:21:49AM +0000:
> > (ugh, #if statements in this files are split between about half starting
> > from start of line and the other half being indented...)
>
> Yes, I tend to prefer the preprocessor stuff starting at the beginning
> of the line but as it is all over the place in channel_curl.c, I tend
> to like the indented form more because otherwise I can't see the
> forest for the trees :)
> Regardless, I don't have strong opinions in one way or another if we
> settle on one, consistently...

Yes, I was mostly complaining about consistently, I'll let Stefano
decide -- either way that doesn't belong to this patch :)

> >> + && !strncasecmp(protocol, "file", 4)
> >
> > I don't see the merit of the 'n' variant here, we don't know how big an
> > allocation curl made so we can only trust it, and in these case I
> > generally use the non-bound variant to avoid counting twice how long
> > static strings are.
> > In this particular case, with 4 you'd accept URLs like filewhatever://,
> > I'm not sure such scheme exists but is that intended?
>
> Actually yes, as it's the protocol, not the URL. The protocol reads
> `http`, `file`, ... either in UPPERCASE or lowercase as mentioned in
> the curl docs. Since I'm interested in `file` only, I opted for
> bounding it.
> However, I don't care too much and can switch to the non-`n` variant?

Yes, the scheme is the part before :// in the url, so if someone
requests an url of filesomething://path and that somehow manages to
succeed we'll match here.

I didn't go all the way earlier but curiosity got the best of me, and
there's currently no curl handler registering anything besides 'file'
itself in the curl source tree, so with the current curl version the
only possible match is 'file' (or 'FILE' or any other case variant as
you pointed out); at which point it doesn't really matter either way but
I'd tend to prefer being strict here in case that changes or someone has
a custom curl.

With that being said, I don't care too much either, I just wondered why
the explicit length here and fell into a rabbit hole; this is fine as it
is as long as that point is understood.

--
Dominique

[Stefano Babic]

On 10.06.24 10:38, Dominique MARTINET wrote:
> 'Storm, Christian' via swupdate wrote on Mon, Jun 10, 2024 at 08:21:49AM +0000:
>>> (ugh, #if statements in this files are split between about half starting
>>> from start of line and the other half being indented...)
>>
>> Yes, I tend to prefer the preprocessor stuff starting at the beginning
>> of the line but as it is all over the place in channel_curl.c, I tend
>> to like the indented form more because otherwise I can't see the
>> forest for the trees :)
>> Regardless, I don't have strong opinions in one way or another if we
>> settle on one, consistently...
>
> Yes, I was mostly complaining about consistently, I'll let Stefano
> decide -- either way that doesn't belong to this patch :)

Agree - I prefer (just because I use this style) that preprocessor
starts at the beginning of the line, and this rule is followed by the
most sources in SWupdate. This can fix with a follow-up patch.

Best regards,
Stefano

[Storm, Christian]

CURLINFO_PROTOCOL, deprecated in 7.85.0, uses a long to represent
the protocol used while the newer CURLINFO_SCHEME uses a string.

See https://curl.se/libcurl/c/CURLINFO_PROTOCOL.html
and https://curl.se/libcurl/c/CURLINFO_SCHEME.html

Signed-off-by: Christian Storm <christi...@siemens.com>

---
corelib/channel_curl.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c

index 368330bd..36db2284 100644

--- a/corelib/channel_curl.c
+++ b/corelib/channel_curl.c
@@ -307,7 +307,11 @@ char *channel_get_redirect_url(channel_t *this)
channel_op_res_t channel_map_http_code(channel_t *this, long *http_response_code)
{
char *url = NULL;
- long protocol;
+#if LIBCURL_VERSION_NUM >= 0x75500

+ char *protocol = NULL;
+#else
+ long protocol = 0;
+#endif
channel_curl_t *channel_curl = this->priv;
CURLcode curlrc =
curl_easy_getinfo(channel_curl->handle, CURLINFO_RESPONSE_CODE,
@@ -329,7 +333,14 @@ channel_op_res_t channel_map_http_code(channel_t *this, long *http_response_code
CURLINFO_PROTOCOL,
#endif
&protocol);
- if (curlrc == CURLE_OK && protocol == CURLPROTO_FILE) {
+ if (curlrc == CURLE_OK

+#if LIBCURL_VERSION_NUM >= 0x75500
+ && protocol
+ && !strcasecmp(protocol, "file")
+#else

+ && protocol == CURLPROTO_FILE
+#endif
+ ) {
return CHANNEL_OK;
}
DEBUG("No HTTP response code has been received yet!");

--
2.45.2

[Dominique MARTINET]

'Storm, Christian' via swupdate wrote on Tue, Jun 11, 2024 at 11:09:19AM +0000:
> CURLINFO_PROTOCOL, deprecated in 7.85.0, uses a long to represent
> the protocol used while the newer CURLINFO_SCHEME uses a string.
>
> See https://curl.se/libcurl/c/CURLINFO_PROTOCOL.html
> and https://curl.se/libcurl/c/CURLINFO_SCHEME.html
>
> Signed-off-by: Christian Storm <christi...@siemens.com>

Thanks for v2!

Reviewed-by: Dominique Martinet <dominique...@atmark-techno.com>