New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mime API #1839
Mime API #1839
Conversation
New CURLOPT_MIMEPOST option with a curl_mime * parameters. Old CURLOPT_HTTPPOST option unaltered.
This is required to support character code conversions.
Some calls in different modules were setting the data handle to NULL, causing segmentation faults when using builds that enable character code conversions.
…andle. This fixes segmentation faults, such as they can occur when calling deprecated curl_escape() and curl_unescape() functions with a build that enables character code conversions. Using a NULL handle should be however avoided: a conversion descriptor is created and deleted each time a conversion function is called with a NULL handle, and thus is not very efficient.
- Use read when no seek function is provided. - Disable for mime/form: no sense with multipart. Also properly rewind form when needed.
This allows checking if subparts are not attached twice and will help curl tool finish a multipart.
The case keyword may be followed by a constant expression and thus should allow it to start with an open parenthesis.
This allows substitution of boundaries in mail messages.
Also document --form extended syntax.
Also disable test 1135
Shell profile output makes the SSH server failing and this problem reason is not easy to find when no hint is given.
return TRUE; | ||
return FALSE; | ||
|
||
return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a fix that is totally independent from the mime API work, right? If so I think we should cherry-pick it out from here and land it first, separately so that we don't make this work bigger than it has to be...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I encountered a few of them during this work (like NULL data pointer in calling code conversion functions), and fixed them on the fly. Fixes are tiny however.
@monnerat, I think this is lovely work and I think you've done great work at pulling it this far already. This is awesome! Here are a few comments and ideas that popped up during my first reviews of this large PR:
|
Hi Daniel,
I'll fix the points above in the current PR before we have a new checkpoint. Cheers, |
One more question: do we have to provide a curl_mime_get() function as there currently is a curl_formget() ? |
Sure! We have until Wednesday to merge and then 4 more weeks to polish details. Since this is a new API, the risk of breaking things for old users should be minimal.
I think the problem is quite simply that an application cannot pass a
I was thinking of more specific tests that add stuff and then verifies how the POST ends up, with legal and illegal input etc. But as you say, the basic functionality is verified already pretty good at least for when used the way we think it should be used. We can add more tests after landing too.
I think we can leave that one out until we actually hear from someone asking for that functionality. We added Also, with our support for reading data from files on demand and streaming via callbacks etc, it has a very limited use. (Back when the formget function was added, the entire formpost was alwas generated and kept in memory and thus "easy" to provide.) |
I can confirm there is at least one direct user of The lack of questions might be because it is well documented and that I took time to carefully read it. |
Sorry, I meant to write formget() there... =) (and I'll edit my post now to say so) |
This is not completely true: Cul_getformdata has been completely rewritten and the intermediate form is kept as a mime structure. CURLOPT_HTTPOST may be impacted. In addition, cli tool -F option now uses CURLOPT_MIMEPOST.
I can hardly believe that: it seems to be a very restricting caveat of the OS. In addition, other kind of pointers can be passed to a DLL and pointed data can be accessed by the DLL without problem. AFAICR, libxml2 does it without problem. On the other side, it is true I am not a W$ speciallist (same for you, AFAIK): I don't even have one here. Maybe we should have a file descriptor argument rather than a FILE * and use 0 for stdin ?
That means the old form API "-" file name should already fail on W$. Does it ? |
This is now pushed in the master tree. Thanks for review. |
I get a few warnings for Line 285 in 3baf36e
Line 315 in 3baf36e
Line 477 in 3baf36e
|
In Windows FILE / fd are only supposed to be operated on by the module that opened them. The reason for that is because in Windows each module can have its own independent CRT (C runtime). The same is true of malloc, for example if you malloc in one DLL you can't free that memory in another DLL (it's a little more complicated than that but for brevity just assume that for now). You can get away with it if your modules share the same DLL CRT, but that's not something we can depend on. Regarding 0/1/2 it's tricky. The CRTs can share the same underlying native HANDLEs to start but let's say for example one CRT calls freopen on stdout, that causes the underlying handle to be closed and then in that one CRT stdout has a new HANDLE so it writes to a file. But then the other modules don't know that so they will continue to write to the HANDLE that is closed, or worse some new HANDLE that has the old HANDLE value. In any case printf and other calls will continue to return bytes written as if they wrote something. |
@jay: thanks a lot for reporting the conversion warnings. I've pushed a fix that should avoid them. Thanks also for the FILE in DLLs explanation. This makes me think libcurl may already have some potential problems for years:
We probably may have similar problems on output. Maybe something must be said about it in the documentation ? For the current mime case, I suppressed curl_mime_file() from exported entry points, but I had to retain it internally to support pseudo-file "-" for compatibility with the old form API, thus keeping potential problems. For the feature itself, I guess it should be emulated by the caller using curl_mime_data_cb(). Many thanks again for your comments. |
It actually is noted in CURLOPT_{READ,WRITE}DATA that if one sets the parameter that way then they must set the respective function as well. On another topic, I see in 1ab9e9b just now that you bumped the soname and the order of the API is different. That seems unnecessary, couldn't you have just preserved the existing order of the API? |
We only ever do one SONAME bump per release anyway so it would not have been a problem. I really don't like having function prototypes in curl.h for functions we don't actually provide, but if you will provide this function soon I figure it'll be fine. And as @jay remarks, the DLL problems we have are already noted in the docs and I my remarks on the mime API was in order to avoid us doing more of those same mistakes. @jay:
It's really quirky to maintain the order as then all new prototypes have to be added at the end of |
This broke non-HTTP builds, e.g. https://curl.haxx.se/dev/log.cgi?id=20170908020834-25979#prob4 |
This is probably also the root of this test 650 failure, with CURL_MAX_WRITE_SIZE set to its smallest allowed value of 1024: https://curl.haxx.se/dev/log.cgi?id=20170908020827-25842#prob7 |
Here's a different build failure on another non-HTTP build: https://curl.haxx.se/dev/log.cgi?id=20170908020807-25537#prob1 |
This PR is rather an RFC, since I have push permission.
I already tried such a RFC in the mailing list, without any response, and hope this PR will trigger some.
The commits here implements a new MIME API that can be used to replace the old form API, as well as compose a multipart mail message to be sent by SMTP or appended via IMAP. See CURLOPT_MIMEPOST.
The cli tool has been changed to use it for the -F option instead of CURLOPT_HTTPPOST. It properly generates --libcurl code accordingly. The -F argument syntax has been extended to support multipart mails.
The old form API now uses the mime API for its intermediate data (getformdata).
Some related tests have been added. The only one that fails is 1135 (check entry points order): I did nothing about it yet since it'll probably require an SONAME change too.
As a new behavior: "Expect 100-continue" is not generated for form posts that are measured smaller that the configured limit.
Documentation has been updated accordingly.
The only things that left to implement are:
Should I commit in master branch ?