embeddedmz / ftpclient-cpp

C++ client for making FTP requests
MIT License
211 stars 67 forks source link

Uploading from std::istream. #39

Closed andrewerf closed 2 years ago

andrewerf commented 2 years ago

The CFTPClient missing ability to upload data stored in memory. My proposal is the following: extend the CFTPClient with public methods: 1) bool UploadFile(read_callback_t read_callback, void* userData, const std::string &strRemoteFile, const bool &bCreateDir = false) const; 2) bool UploadFile(std::istream &isData, const std::string &strRemoteFile, const bool &bCreateDir = false) const;

First one correspond to CURLOPT_READFUNCTION and second one is useful wrapper for std::istream. Implementation of bool UploadFile(const std::string &strLocalFile, const std::string &strRemoteFile, const bool &bCreateDir = false) const; could be updated therefore to open strLocalFile with std::ifstream and call overload 2).

embeddedmz commented 2 years ago

Thank you for your pull request. I'm reviewing your PR then I will run unit tests (FTP and SFTP) on Windows 10 and see if the units tests "TestUploadAndRemoveFile", "TestUploadFileNameWithAccents" (Windows and unsecure FTP only), "TestUploadAndRemoveFile10Times" (strange test but I have added it because I was having a strange behavior with FileZilla Server, since I'm using other FTP servers, that unit test is useless) and "TestUploadFailure" succeed or not.

andrewerf commented 2 years ago

I've tested it on Arch linux with sftp and it works. I don't have Windows unfortunately, so waiting for your results.

embeddedmz commented 2 years ago

@andrewerf fileSize should be curl_off_t (in fact it's "long long"). By the way, when working with embedded systems, I prefer using types like int64_t, uint32_t etc... (stdint.h/cstdint header file)

The unit tests have succeeded.

Isn't it better to use a method signature like the one used for DownloadFile : bool CFTPClient::DownloadFile(const std::string &strRemoteFile, std::vector<char> &data) if you have a memory content you want to upload (or a pointer to the data + their count) ?

off-topic : I mean, I've never subclassed std::istream and I don't know in which scenario it can be useful. I don't know if it can be useful to manage a byte stream. Usually what I do is I manage an array/collection of bytes myself and move the processed elements to the left (I shift them from the buffer). Why do I do this ? Because the problem with streams is that once you read one or more items, they disappear from the internal buffer and I prefer to keep them in the buffer because I am waiting for other items to be received (e.g. an XML file or a line of text that needs to end with an LF character) and this saves me from using an extra buffer (in which I will store the bytes I read from the stream) and provides more performance (no need to use an extra buffer). After all, we can have a stream that satisfies what I described before, but I prefer the old school way of handling things (what if the stream is unable to spit out an object? e.g. std::cin blocks the program until the user press 'enter', but what should a custom istream do? return an empty object ? should we test if the stream is in a normal state, is there a state to reset ? etc... = complexity = bad).

In C#, there's a MemoryStream class (a byte stream), but I don't think there's something similar in C++.

andrewerf commented 2 years ago

1) On linux, curl_off_t is long, so I agree that it's better to use curl_off_t in the interface. I'll fix this. 2) It just different use case. While I don't work with embedded systems, I could easily think of example of usage std::istream. Imagine that user want to send 100gb folder to the ftp in archive (tar is an on-the-fly archiver). If we use streams, we could perform archivation and uploading without storing all the content in the memory (which could be impossible at all). In fact, the 1) overloading is the most general and could fit this use case, but some C++ libraries return streams (like wxWidgets, wxStream is easily convertible to std::istream). And that is why I think it's a great feature to support. 3) "What if the stream is unable to spit out an object"? You are talking about formatted input, I suppose. But in my PR unformatted input is used. Doc says that It's two possibilities for std::istream::read: 3.1) count characters were extracted and stored 3.2) end of file condition occurs on the input sequence (in which case, setstate(failbit|eofbit) is called). The number of successfully extracted characters can be queried using gcount(). And also we check state of the stream before reading: if (!pFileStream->fail()) and return 0 if it is in the bad state.

It may be reasonable to add some tests covering streams functionality. I'll try to think of something, that checks that it's possible to upload streams with unknown size, or that it processes bad states reliably.

embeddedmz commented 2 years ago

You can change the long to curl_off_t and I will merge the pull request. Don't bother with more tests. Thanks.

andrewerf commented 2 years ago

I've already added simple test. And fixed curl_off_t.

embeddedmz commented 2 years ago

Thank you for your pull request