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

dynbuf: introduce internal generic dynamic buffer functions #5300

Closed
wants to merge 2 commits into from

Conversation

@bagder
Copy link
Member

@bagder bagder commented Apr 26, 2020

A common set of functions instead of many separate implementations for
creating buffers that can grow when appending data to them. Existing
functionality has been ported over.

In my early basic testing, the total number of allocations seem at
roughly the same amount as before, possibly a few less.

See docs/DYNBUF.md for a description of the API.

Note that this patch is a net removal of many lines of code. Not immediately noticeable because this patch includes documentation and new source file headers etc.

@mback2k
Copy link
Member

@mback2k mback2k commented Apr 27, 2020

I think this is great and could also be used inside the Schannel code if it supported memmove'ing things from the back to the front, eg. removing data from the front of the dynamic buffer.

@bagder
Copy link
Member Author

@bagder bagder commented Apr 27, 2020

@mback2k do you have a suggestion on what the API for that would look like? Just a number of bytes to keep (from the end of the buffer)? Like Curl_dyn_tail(struct dynbuf *s, size_t bytes); ? (where bytes would have to be smaller or equal to the length of the existing buffer or trigger an error)

@bagder bagder force-pushed the bagder/dynbuf branch from 91f8228 to b3b7988 Apr 27, 2020
@bagder
Copy link
Member Author

@bagder bagder commented Apr 27, 2020

@mback2k: proposed function now in a separate commit (I may still rebase so it won't have a fixed commit hash yet) - I just don't think I will adapt the schannel code.

@bagder bagder self-assigned this Apr 27, 2020
@bagder bagder force-pushed the bagder/dynbuf branch from 7ada21d to ed808f1 Apr 27, 2020
@mback2k
Copy link
Member

@mback2k mback2k commented Apr 27, 2020

@badger thanks! Sorry, didn’t have more time for this yet, but I will look into porting Schannel over during the next feature window. Besides a function to skip bytes from the beginning that will also need functions to add more space to the buffer, because Schannel itself will want to write the encrypted or decrypted bytes. We can add those during next feature window of course.

@bagder bagder force-pushed the bagder/dynbuf branch from c26a79c to 9f2e29e Apr 28, 2020
@bagder
Copy link
Member Author

@bagder bagder commented Apr 28, 2020

functions to add more space to the buffer, because Schannel itself will want to write the encrypted or decrypted bytes.

I would prefer to not add that to the dynbuf API. The tail function is different because that's really not possible to do in any other way. Appending bytes to the buffer is already done by, yeah, appending data to the buffer. It is much nicer if the code would then read data into a buffer and then add N byes at once with Curl_dyn_add. "add more buffer space" is not a concept that exists for dynbuf: it is a buffer with a length (an independent "allocated size"). There's no extra "uninitialized buffer area at the end".

We can add those during next feature window of course.

Yes, this will be easier to work once this PR has landed.

@mback2k
Copy link
Member

@mback2k mback2k commented Apr 28, 2020

functions to add more space to the buffer, because Schannel itself will want to write the encrypted or decrypted bytes.

I would prefer to not add that to the dynbuf API. The tail function is different because that's really not possible to do in any other way. Appending bytes to the buffer is already done by, yeah, appending data to the buffer. It is much nicer if the code would then read data into a buffer and then add N byes at once with Curl_dyn_add. "add more buffer space" is not a concept that exists for dynbuf: it is a buffer with a length (an independent "allocated size"). There's no extra "uninitialized buffer area at the end".

The problem with Schannel is: I have to hand it a buffer with some space for it to write data into. If I still wanted to use dynbuf with that concept, I would have to allocate some memory first, let Schannel write to that and then copy the data over to the dynbuf. That may work, but could mean some overhead.

@bagder
Copy link
Member Author

@bagder bagder commented Apr 28, 2020

I would have to allocate some memory first, let Schannel write to that and then copy the data over to the dynbuf. That may work, but could mean some overhead.

Yes I understand that. But to avoid that extra copying, we would have to complicate dynbuf significantly or break its concept:

dynbuf buffers have a length and the data in the buffer is there and valid. A variable amount of data in a buffer. That's it.

You ask for a "margin" with uninitialized data somehow at the end of the data. We would need code and API to deal with that margin. If the margin is not uninitialized data, we instead need to initialize it and then we lose some of benefit plus we need another API to "overwrite" a part of the buffer from a certain position.

And this complication of dynbuf would only be used by the schannel code. I'm not convinced.

bagder added 2 commits Apr 26, 2020
A common set of functions instead of many separate implementations for
creating buffers that can grow when appending data to them. Existing
functionality has been ported over.

In my early basic testing, the total number of allocations seem at
roughly the same amount as before, possibly a few less.

See docs/DYNBUF.md for a description of the API.
Removes a 16K static buffer from the easy handle. Simplifies the code.
@bagder bagder force-pushed the bagder/dynbuf branch from 9f2e29e to 63a264c Apr 28, 2020
@bagder
Copy link
Member Author

@bagder bagder commented Apr 29, 2020

@mback2k: I'll start out with landing the *tail() function '#if 0'ed when I land this PR as there's currently no user of the function. When/if we get a need for it, it will be easy to activate it. If nothing wrong happens, I'll do it early next week.

@bagder bagder marked this pull request as ready for review Apr 29, 2020
@bagder bagder changed the title [WIP] dynbuf: introduce internal generic dynamic buffer functions dynbuf: introduce internal generic dynamic buffer functions Apr 29, 2020
@mback2k
Copy link
Member

@mback2k mback2k commented Apr 29, 2020

@mback2k: I'll start out with landing the *tail() function '#if 0'ed when I land this PR as there's currently no user of the function. When/if we get a need for it, it will be easy to activate it. If nothing wrong happens, I'll do it early next week.

i think it could also be left out for now if there is no usage for it yet. Your decision though.

@bagder bagder closed this in ed35d65 May 4, 2020
@bagder bagder deleted the bagder/dynbuf branch May 10, 2020
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.