| Message ID | B135C235-D2C7-4592-93FF-8B0386C90679@siemens.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | channel_curl: Fix getting the protocol for curl >= 7.85.0 | expand |
'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 <christian.storm@siemens.com> Wrote a nitpick below, but: Reviewed-by: Dominique Martinet <dominique.martinet@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!");
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 <christian.storm@siemens.com> > > Wrote a nitpick below, but: > Reviewed-by: Dominique Martinet <dominique.martinet@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
'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.
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 > >>>> + && !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. >
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!");
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 <christian.storm@siemens.com> --- corelib/channel_curl.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)