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

Add support for post_fields without buffer copy #332

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dovreshef
Copy link

Hi

I wanted to add support for uploading data without the buffer copy between curl and rust, and without copying it in parts into a read function.

Basically support CURLOPT_POSTFIELDS, but unlike today, without setting CURLOPT_COPYPOSTFIELDS.

I've added both the option to support uploading 'static data, which I think is simple.

I'm not so sure about my approach to solving it for Transfer. I'm open to any other approach really, if you have any pointers. This was for me high magic, and I hope I've got it right.

And, of course, all that is in case that is something that you're interested in adding support for.

@alexcrichton
Copy link
Owner

I think the reason I didn't originally bind this was the lifetime issue with data. I think we'll probably want to either make this unsafe or figure out a safe operation, because having &'static isn't too too helpful I think?

@dovreshef
Copy link
Author

I think the reason I didn't originally bind this was the lifetime issue with data. I think we'll probably want to either make this unsafe or figure out a safe operation, because having &'static isn't too too helpful I think?

You're right. I mostly added it out of completion (to have all the options) and being useful for tests I guess.

What about tying it the lifetime in some way into a transfer object? Does it have to be unsafe?

I'm not sure that what I did is safe, but if not, surely there is some way to make sure that the upload buffer is set to null when the transfer goes out of scope?

@alexcrichton
Copy link
Owner

I'm not very familiar with the safety here or this option, and I don't have a ton of time to dig in right now unfortunately. It might be possible though to tie it to the Transfer object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants