feat: support curl_blob options #300
Conversation
There was a problem hiding this comment.
@johnwchadwick just some minor comments, everything else looks great. Thanks so much for the PR!
Related to your question, go with options 2:
manually adjust generated options to only include newly-enabled BLOB options?
I will add a note related to that to the CONTRIBUTING docs. If you have any other feedback related to the Developer Experience, please do let me know, I would love to improve it.
src/Easy.cc
Outdated
| Nan::Utf8String value(value); | ||
|
|
||
| size_t length = static_cast<size_t>(value.length()); | ||
|
|
||
| struct curl_blob blob; | ||
| blob.data = *value; | ||
| blob.len = length; | ||
| blob.flags = CURL_BLOB_COPY; | ||
|
|
||
| setOptRetCode = curl_easy_setopt(obj->ch, static_cast<CURLoption>(optionId), &blob); |
There was a problem hiding this comment.
| Nan::Utf8String value(value); | |
| size_t length = static_cast<size_t>(value.length()); | |
| struct curl_blob blob; | |
| blob.data = *value; | |
| blob.len = length; | |
| blob.flags = CURL_BLOB_COPY; | |
| setOptRetCode = curl_easy_setopt(obj->ch, static_cast<CURLoption>(optionId), &blob); | |
| Nan::Utf8String utf8StringValue(value); | |
| size_t length = static_cast<size_t>(utf8StringValue.length()); | |
| struct curl_blob blob; | |
| blob.data = *utf8StringValue; | |
| blob.len = length; | |
| blob.flags = CURL_BLOB_COPY; | |
| setOptRetCode = curl_easy_setopt(obj->ch, static_cast<CURLoption>(optionId), &blob); |
Just to make it more clear what value is and to not use the same variable name that was declared in the outer scope
| struct curl_blob blob; | ||
| blob.data = *value; | ||
| blob.len = length; | ||
| blob.flags = CURL_BLOB_COPY; |
There was a problem hiding this comment.
Do you think this could give a small perf hit down the road (for bigger payloads)? Honest question, I'm ok with just copying the data for now.
Another approach would be to keep the data internally. Check the ToFree data structure.
There was a problem hiding this comment.
I agree it would be ideal to not copy the data if it isn't needed, but I worry more about correctness than performance. Particularly because Buffers are mutable, but also because copying frees us of having to worry about the memory management of all of these containers. Even the CA certs bundle is only around 200 KiB, which isn't too bad. So I think for now, I would prefer to punt on it, although it would probably be worth looking into.
| it('should be able to set blob value back to null', () => { | ||
| curl.setOpt('SSLKEY_BLOB', Buffer.from([])) | ||
| curl.setOpt('SSLKEY_BLOB', null) | ||
| }) |
There was a problem hiding this comment.
Can you add another 2 test cases for:
- String value
- Buffer with data (the above one is empty)
johnwchadwick
force-pushed
the
add-blob-opt-support
branch
from
5a24ec6
to
163df49
Compare
libmetalink is no longer supported by curl; specifying this option is now an error as of curl 7.78.0/265b14d6.
|
Thanks @johnwchadwick ! |
Fixes #253.
Because I ran
gen:constants, a bunch of new constants were added from curl.se. Should I:BLOBoptions?I am new to the project and not sure what the best course of action is.