On 20 September 2011 22:50, Jernej Kovacic <jkovacic_at_gmail.com> wrote:
>
> The suggested functionality is desired in my application, I believe it
> could be useful to the others, so I decided to share it.
We're very glad that you did so. I hope you understand any critisms
are to make sure we make these changes in the best possible way. With
a library such as libssh2, one we establish an API we *cannot change
it* (at least for a very long time) so it's critical that we get it
right from the start.
> On Mon, Sep 19, 2011 at 11:40 PM, Alexander Lamaison <swish_at_lammy.co.uk> wrote:
>> The function is called libssh2_session_supported_algs but it doesn't
>> actually take a session argument. I'm not familiar with what this
>> function is designed to do. How is it's task independent of the
>> session?
>
> The function returns a list of supported algorithms for each method
> (e.g. symetric encryption, HMAC etc.) which depends on the chosen
> cryptographic library, configure options, probably on libssh2 version
> etc. The function is typically called before a session is established
> and, if you take a look into its implementation in kex.c, you'll
> notice that it doesn't need anything from LIBSSH2_SESSION.
And yet _libssh2_comp_methods takes a session parameter so it would
appear that it is at least partially dependent on the session?
> Well, it could be an option to pass a session argument too and not use
> it at all, but IMHO there's no point in it at all.
That would of course be silly. If the function truly is independent
of the session, it should not take a pointless parameter. However ...
> Or do you mean that the function should be renamed?
If the function truly is independent of the session, it shouldn't have
session in the name. libssh2_supported_algs would suffice.
>> ...
>>> +int libssh2_session_supported_algs(int method_type, char** algs, unsigned int nalgs);
>>> +.SH DESCRIPTION
>>> +Get a list of supported algorithms for each method_type. The method_type parameter is equivalent
>>> +to method_type in \fIlibssh2_session_method_pref(3)\fP. Algs must be user preallocated array of pointers to strings
>>> +(e.g. char* methods[N_METHODS]) and nalgs is a number of preallocated elemnts in this array.
>>> +If successful, the function will fill this array with supported algorithms (the same names as defined in RFC 4253)
>>> +and terminate it with a NULL pointer.
>>
>> Who is responsible for freeing the strings added to the array? How
>> does the caller know how big to make the array?
>
> No strings are added to the array, only pointers to existing strings
> (they were created during compile time and are used by
> libssh2_session_method_prefs as well) which were initially not visible
> to the application. As no extra memory is (m)allocated, there is no
> need to free anything.
If so, this needs to be very clearly stated. We can't expect the
users to guess. However, see below where i'm suggesting we leave the
details of memory management to the library.
> The caller must guess and provide a reasonable size (20 elements
> should be more than enough) of this array. If the array is too short,
> an error will be returned. Of course I made every effort to prevent
> any buffer overflow in this case.
Almost any time a library API expects the user to guess something,
especially the maximum size of something, this is a flaw in the design
of the API. Any time a library can hide complexity from the user,
without loss of generality, it should do so. This new API would seem
to meet both criteria. Therefore, my suggestion would be that
libssh2_supported_algs allocate the array itself and return the size
of the array. This removes all the guesswork as the library knows
exactly how big this array needs to be. Then the user just needs to
call LIBSSH2_FREE on the output pointer when they are finished with
it.
>>> +.SH RETURN VALUE
>>> +On success, a number of returned algorithms (i.e a positive number will be returned).
>>> --- a/include/libssh2.h
>>> +++ b/include/libssh2.h
>>> +/*
>>> + Retrieve a list of supported algorithms for given method_type (see ibssh2_session_method_pref)
>>> + algs - user preallocated array of pointers to string which will be filled (and NULL terminated) in by the function
>>> + nalgs - number of reserved elements in algs, incl. the NULL termination
>>
>> Erm ... why would nalgs be NULL terminated? It's an array of pointers
>> and its size is passed around explicitly.
>
> Your question is not clear, possibly due a typing error. (algs will be
> NULL terminated, not nalgs which is an integer).
You're quite right, this was a typo. I meant to ask, why would algs
be NULL terminated?
> Well, this means if 7 algorithms are returned, algs[0 to 6] will be pointers to their
> names, algs[7] will be NULL, indicating that algs[8] to algs[nalgs-1]
> (remaining elements of the preallocated array) are not "interesting"
> at all.
This seems very unusual to me, especially as the caller knows how many
entries are valid - you are returning that value from the function.
Just set the available entries and leave the rest of the array
untouched. Or, better yet, as I mentioned earlier, allocate the array
inside the library to exactly the size needed.
Alex
-- Swish - Easy SFTP for Windows Explorer (http://www.swish-sftp.org) _______________________________________________ libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-develReceived on 2011-09-21