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

curl: use dynbuf for config file buffer management #5946

Closed
wants to merge 5 commits into from

Conversation

@bagder
Copy link
Member

bagder commented Sep 9, 2020

By making the dynbuf functions available as curlx_ verisons, the config file parser in the tool code can use that for its buffer management and for saver reallocs etc.

Once landed, we can move more curl functions over to dynbuf,

(Alternative take to #5941 )

@bagder bagder requested a review from jay Sep 9, 2020
@bagder bagder force-pushed the bagder/curl-dynbuf branch from d4591d7 to 1b54f96 Sep 9, 2020
@jay
Copy link
Member

jay commented Sep 10, 2020

Sharing memory allocation functions between the tool and library is tricky. In Windows the tool and library can each be using a separate CRT if libcurl is a DLL and therefore have separate memory handling. Also it would probably be slower to call into the DLL for each dynbuf memory call.

@bagder
Copy link
Member Author

bagder commented Sep 10, 2020

Yes, but this isn't meant to share allocation functions. It is meant to share the dynbuf.c code and the allocations done by the tool should be done like normal tool-side allocations. Still a bit tricky to get right in the build systems but I think I'm almost there now...

@jay
Copy link
Member

jay commented Sep 10, 2020

If the code is built separately for the tool and library and there's naming conflicts and the tool and library can't both use the same function name (I seem to remember that's happened before) then you could change the function name depending on BUILDING_LIBCURL like

#ifdef BUILDING_LIBCURL
#define DYN_FNAME(x) libcurl_ ## x
#else
#define DYN_FNAME(x) x
#endif

void DYN_FNAME(dyn_nappend) (...

@bagder
Copy link
Member Author

bagder commented Sep 10, 2020

Ah right, it will be a problem in static builds...

@bagder bagder marked this pull request as draft Sep 10, 2020
@bagder
Copy link
Member Author

bagder commented Sep 10, 2020

I opted to plain redefined function names for the tool. I think it looks and feels better.

@bagder
Copy link
Member Author

bagder commented Sep 10, 2020

Can anyone figure out why the visual studio project build fails?

tool_parsecfg.obj : error LNK2019: unresolved external symbol curlx_dyn_init referenced in function parseconfig
tool_parsecfg.obj : error LNK2019: unresolved external symbol curlx_dyn_free referenced in function parseconfig
tool_parsecfg.obj : error LNK2019: unresolved external symbol curlx_dyn_add referenced in function my_get_line
tool_parsecfg.obj : error LNK2019: unresolved external symbol curlx_dyn_reset referenced in function parseconfig
tool_parsecfg.obj : error LNK2019: unresolved external symbol curlx_dyn_ptr referenced in function parseconfig
  1. It was winbuild, not visual studio
  2. I figured it out
@bagder bagder force-pushed the bagder/curl-dynbuf branch from 79925c7 to 1e6d21f Sep 10, 2020
@bagder bagder marked this pull request as ready for review Sep 10, 2020
@bagder
Copy link
Member Author

bagder commented Sep 11, 2020

This looks fine now and I'll land this (unless someone objects) within a day or two.

src/tool_parsecfg.c Outdated Show resolved Hide resolved
src/tool_parsecfg.c Outdated Show resolved Hide resolved
@bagder bagder force-pushed the bagder/curl-dynbuf branch from 06e2b6f to 80e20e4 Sep 11, 2020
@bagder bagder requested a review from jay Sep 11, 2020
src/tool_parsecfg.c Outdated Show resolved Hide resolved
@bagder bagder force-pushed the bagder/curl-dynbuf branch from 80e20e4 to b8fff67 Sep 11, 2020
@bagder
Copy link
Member Author

bagder commented Sep 11, 2020

Yet another force-push

src/tool_parsecfg.c Outdated Show resolved Hide resolved
bagder added 2 commits Sep 9, 2020
... fixes an integer overflow at the same time.

Reported-by: ihsinme on github
Assisted-by: Jay Satiro
@bagder bagder force-pushed the bagder/curl-dynbuf branch from b8fff67 to 2877e66 Sep 13, 2020
@bagder
Copy link
Member Author

bagder commented Sep 13, 2020

Okay, pushed a new version that makes curl actually return an error back out if it runs into an out of memory.

src/tool_parsecfg.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member Author

bagder commented Sep 13, 2020

If you are only having a single line in the buffer at any time then that is not necessary you could just reset it.

This logic is doing exactly one line at a time...

bagder added 3 commits Sep 13, 2020
@bagder bagder closed this in c4ea71a Sep 14, 2020
bagder added a commit that referenced this pull request Sep 14, 2020
... fixes an integer overflow at the same time.

Reported-by: ihsinme on github
Assisted-by: Jay Satiro

Closes #5946
bagder added a commit that referenced this pull request Sep 14, 2020
Closes #5946
bagder added a commit that referenced this pull request Sep 14, 2020
Closes #5946
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.