Skip to content
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

Remove curl_mimepart object from UserDefined structure when CURL_DISABLE_MIME flag is active #12948

Closed
wants to merge 1 commit into from

Conversation

MAntoniak
Copy link
Contributor

Reduce size of UserDefined structure. For the x86 architecture, the structure size decreased by 368 bytes, for x64 440 bytes. It is therefore a significant reduction in the size of the structure.

Also remove unreachable code: when CURL_DISABLE_MIME is set, httpreq can never have HTTPREQ_POST_MIME value and the same goes for the CURL_DISABLE_FORM_API flag and the HTTPREQ_POST_FORM value.

As for the name of the function I created in smtp.c and imap.c I am open to suggestions.

@MAntoniak MAntoniak marked this pull request as draft February 16, 2024 07:07
@MAntoniak MAntoniak force-pushed the remove_mime branch 2 times, most recently from 6e81fb6 to baae225 Compare February 17, 2024 20:24
@MAntoniak MAntoniak marked this pull request as ready for review February 17, 2024 22:45
lib/imap.c Outdated
@@ -1513,10 +1522,10 @@ static CURLcode imap_done(struct Curl_easy *data, CURLcode status,
}
else if(!data->set.connect_only && !imap->custom &&
(imap->uid || imap->mindex || data->state.upload ||
data->set.mimepost.kind != MIMEKIND_NONE)) {
IMAP_MIMEKIND_IS_NOT_NONE(data))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't this be a single shorter macro somewhere like urldata and make it different for when mime is disabled like

#ifndef CURL_DISABLE_MIME
#define IS_MIME_POST(a) ((a)->set.mimepost.kind != MIMEKIND_NONE)
#else
#define IS_MIME_POST(a) FALSE
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved the macro definition to urldata.h.

…BLE_MIME flag is active. Reduce size of UserDefined structure.

Also remove unreachable code: when CURL_DISABLE_MIME is set, httpreq can never have HTTPREQ_POST_MIME value and the same goes for the CURL_DISABLE_FORM_API flag and the HTTPREQ_POST_FORM value
@jay
Copy link
Member

jay commented Feb 22, 2024

Did you test this? Though it passes the CI (since we don't test CURL_DISABLE_MIME) there are warnings and it fails to compile:

..\..\..\..\lib\formdata.c(575): warning C4013: 'Curl_mime_contenttype' undefined; assuming extern returning int
..\..\..\..\lib\formdata.c(575): warning C4047: '=' : 'const char *' differs in levels of indirection from 'int'
..\..\..\..\lib\formdata.c(726): error C2064: term does not evaluate to a function taking 305 arguments

formdata.c(726):

    size_t nread = Curl_mime_read(buffer, 1, sizeof(buffer), &toppart);

mime.h(169):

#define Curl_mime_read NULL

It should be a function-like macro such as

#define Curl_mime_read(a,b,c,d) NULL

but that causes different errors because of code that passes the function as a pointer even when CURL_DISABLE_MIME, which doesn't seem right. for example http.c(2582):

    data->state.fread_func = (curl_read_callback) Curl_mime_read;

@MAntoniak
Copy link
Contributor Author

Yes, I saw the warning and error. They ocur, when user defines CURL_DISABLE_MIME flag without CURL_DISABLE_FORM_API. In the project I'm working on I have both of these flags applied and only tested this scenario. This problems occur also without this PR.

The problem with Curl_mime_contenttype is easy to solve. Simply write

#define Curl_mime_contenttype NULL

in mime.h in section where CURL_DISABLE_MIME is defined.

More complex task is solve the problem with Curl_mime_read. I don't quite know how to solve it. It is the public function. I didn't analyze what is the right behavour when CURL_DISABLE_MIME is defined.

@MAntoniak
Copy link
Contributor Author

MAntoniak commented Feb 22, 2024

If I understand code correctly the Curl_getformdata function return CURLE_OUT_OF_MEMORY and also curl_formget return the same error code. Maybe the solution is:

--- a/src/lib/curl/lib/formdata.c
+++ b/src/lib/curl/lib/formdata.c
@@ -712,6 +712,7 @@ CURLFORMcode curl_formadd(struct curl_httppost **httppost,
 int curl_formget(struct curl_httppost *form, void *arg,
                  curl_formget_callback append)
 {
+#ifndef CURL_DISABLE_MIME
   CURLcode result;
   curl_mimepart toppart;
 
@@ -737,6 +738,9 @@ int curl_formget(struct curl_httppost *form, void *arg,
 
   Curl_mime_cleanpart(&toppart);
   return (int) result;
+#else
+  return CURLE_OUT_OF_MEMORY;
+#endif
 }

Or better return CURL_FORMADD_DISABLED code.

@jay
Copy link
Member

jay commented Feb 24, 2024

@monnerat

@monnerat
Copy link
Contributor

They ocur, when user defines CURL_DISABLE_MIME flag without CURL_DISABLE_FORM_API

This case is a nonsense since the form api uses the mime api. There already is a guard against it in configure.ac and a test for it ought to be be added in CMakeLists.txt by someone who masters cmake. Introducing macros and/or conditionals for that situation only decreases the code readability.

If the ultimate goal of this PR is to spare room in the UserDefined structure, I've had a good result by simply shortening the curl_mimedata structure to an empty one:

-- a/lib/mime.h
+++ b/lib/mime.h
@@ -108,6 +108,9 @@ struct curl_mime {
 
 /* A mime part. */
 struct curl_mimepart {
+#ifdef CURL_DISABLE_MIME
+  char dummy[0];
+#else
   curl_mime *parent;               /* Parent mime structure. */
   curl_mimepart *nextpart;         /* Forward linked list. */
   enum mimekind kind;              /* The part kind. */
@@ -128,6 +131,7 @@ struct curl_mimepart {
   const struct mime_encoder *encoder; /* Content data encoder. */
   struct mime_encoder_state encstate; /* Data encoder state. */
   size_t lastreadstatus;           /* Last read callback returned status. */
+#endif
 };
 
 CURLcode Curl_mime_add_header(struct curl_slist **slp, const char *fmt, ...)

Some code bytes are saved by this PR though.

@bagder bagder closed this in e26c362 Feb 26, 2024
@bagder
Copy link
Member

bagder commented Feb 26, 2024

Thanks!

@MAntoniak MAntoniak deleted the remove_mime branch February 26, 2024 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants