MDEV-30432: Refactor connect to use libcurl instead of cpprestsdk/curl #2996
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grooverdan
requested changes
Jan 19, 2024
| /***********************************************************************/ | ||
| /* WriteMemoryCallback: Curl callback function */ | ||
| /***********************************************************************/ | ||
| static size_t WriteMemoryCallback(void *contents, |
There was a problem hiding this comment.
The default function writes to a file. Couldn't CURLOPT_WRITEDATA be used to set the FILE* pointer?
There was a problem hiding this comment.
I have tried this as the first approach in first testing, but the data returned in file were cut, since size cannot be known in advance.
There was a problem hiding this comment.
this sounds odd. Most web requests that curl would be programmed to do won't know the file size in advance.
There was a problem hiding this comment.
Worked with this:
diff --git a/storage/connect/tabrest.cpp b/storage/connect/tabrest.cpp
index 7413f6029b9..f976773f9a0 100644
--- a/storage/connect/tabrest.cpp
+++ b/storage/connect/tabrest.cpp
@@ -277,14 +277,11 @@ int RESTDEF::curl_run(PGLOBAL g)
struct MemoryStruct chunk;
chunk.memory = (char *)malloc(1);
chunk.size = 0;
+ FILE *f= fopen(Fn, "wb");
if ((curl_res= curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, curl_errbuf)) !=
CURLE_OK ||
- (curl_res= curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION,
- WriteMemoryCallback)) !=
- CURLE_OK ||
- (curl_res= curl_easy_setopt(curl, CURLOPT_WRITEDATA,
- (void *)&chunk)) !=
+ (curl_res= curl_easy_setopt(curl, CURLOPT_WRITEDATA, f)) !=
CURLE_OK ||
(curl_res = curl_easy_setopt(curl, CURLOPT_URL, buf)) != CURLE_OK ||
(curl_res = curl_easy_perform(curl)) != CURLE_OK ||
@@ -303,8 +300,6 @@ int RESTDEF::curl_run(PGLOBAL g)
}
}
curl_easy_cleanup(curl);
- FILE *f= fopen(Fn, "wb");
- fprintf(f, "%s", chunk.memory);
fclose(f);
free(chunk.memory);
bool is_error = http_code < 200 || http_code >= 300;
- https://curl.se/libcurl/c/CURLOPT_WRITEDATA.html says libcurl DLL must have a WRITEFUNCTION.
- BB currently doesn't have/detect libcurl on amd-windows. MDBF-644
- Internally in curl,
fwriteis used directly as the CURLOPT_WRITEFUNCTION. Unsure if that will directly work on Windows.
| curl_errbuf[0] = '\0'; | ||
| if (Uri) | ||
| { | ||
| if (*Uri == '/' || Http[strlen(Http) - 1] == '/') |
There was a problem hiding this comment.
This looks really odd. What's it doing?
There was a problem hiding this comment.
This is inherited from old code, about parsing the argument from table options.
an3l
force-pushed
the
bb-11.4-anel-connect-curl-MDEV-30432-v1
branch
from
cac2d30
to
bed924a
Compare
|
before I forget - need to revert #2466 as part of this. |
an3l
force-pushed
the
bb-11.4-anel-connect-curl-MDEV-30432-v1
branch
from
bed924a
to
afd313a
Compare
- Remove GetRest occurance (cpprestsdk references) - Drop execv(curl) references - Link to libcurl (Unix/Windows) - Use IMPORTED target, so that INCLUDE_DIRECTORIES be called implicity - Add libcurl feature to ConnectSE to issue HTPP requests - This patch closes MDEV-26727 (tested with Docker) - Reviewer: <>
an3l
force-pushed
the
bb-11.4-anel-connect-curl-MDEV-30432-v1
branch
from
afd313a
to
3ed2341
Compare
an3l
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Check linkage
Check linkage on Windows
Check without libcurl
Remove the library and check the patch
Install DB and run server locally ✔️
$ sudo apt remove libcurl4-openssl-dev $ cmake --build . --target connect [ 0%] Built target uca-dump [ 8%] Built target mysqlservices [ 8%] Built target GenUnicodeDataSource [ 58%] Built target mysys [ 75%] Built target strings [ 75%] Built target dbug [ 75%] Built target comp_err [ 75%] Built target GenError make[3]: *** No rule to make target '/usr/lib/x86_64-linux-gnu/libcurl.so', needed by 'storage/connect/ha_connect.so'. Stop. make[3]: *** Waiting for unfinished jobs.... [ 75%] Building CXX object storage/connect/CMakeFiles/connect.dir/tabrest.cpp.o /home/anel/GitHub/mariadb/server/src/connect-curl-11.4/storage/connect/tabrest.cpp:39:10: fatal error: curl/curl.h: No such file or directory 39 | #include | ^~~~~~~~~~~~~ compilation terminated.Check in container
Check container from CLI
Basing the PR against the correct MariaDB version
PR quality check