On Tue, 23 Apr 2013, Павел Шкраблюк wrote:

> here is the patch I've submitted earlier, now made according to the
> http://curl.haxx.se/dev/contribute.html

First off, I really like this idea and spirit. Thanks! I think we should be
able to get something like this into libcurl. I believe it should also be
possible to re-use existing FTP code and make it work like the same way as
your SFTP approach.

Let me dive straight into my initial remarks on the actual patch:

1 - I got a build error:

url.c:805:42: error: ISO C forbids assignment between function pointer and
'void *' [-Werror=edantic]

2 - There's no documentation for the new options, and no example code. There's
also no test cases.

3 - We always have callbacks done in a *FUNCTION and a *DATA pair, and I think
your new one should follow this style too. Do you really think we need a
CURLOPT_DIRLISTFILES option separately?

4 - your change for CURLE_QUOTE_ERROR to become sftp_libssh2_error_to_CURLE()
is not really related to the new callback and I would ask you to submit that
as a separate patch (which we could merge at once)

5 - I see at least three realloc()s for which you don't check the return code
and thus it will crash libcurl on failure. runtests.pl -t would make it
obvious if you had test cases...

6 - to me it feels like there's a lot of repetition of the code lines for the
fail case:

+ Curl_safefree(sshc->readdir_line);
+ Curl_safefree(sshc->readdir_filename);
+ Curl_safefree(sshc->readdir_longentry);
+ Curl_fileinfo_dtor(0 /*dummy*/, sshc->readdir_fileinfo);
+ sshc->readdir_fileinfo = 0;
+ state(conn, SSH_SFTP_CLOSE);
+ sshc->actualcode = CURLE_OUT_OF_MEMORY;

7 - you don't really follow our source indent style all over, but that's
easily fixed

8 - why call the new struct member "ftp_list_files" when you don't even
support this for FTP? The same goes for "ftp_fileinfo_list_callback". I would
suggest something that works indepdently of the protocol being SFTP or FTP (or
whatever protocol that might be able to use the concept).

-- 
  / daniel.haxx.se

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2013-04-24