Subject: Re: Patch to ticket 228

Re: Patch to ticket 228

From: Jernej Kovacic <jkovacic_at_gmail.com>
Date: Mon, 14 Nov 2011 22:50:26 +0100

Hello,

finally found some time. Let me reply to some remarks first:

> I would favor using ** and taking a length parameter, so that we work
> toward less malloc within libssh2 instead of more

That was my initial proposal but then it was agreed that the function
should allocate memory itself.

> Can there really be NULL names within method lists? I do not think so.
> kex.c has many loops over these method lists and none skips over NULL
> names like done here. Some ends on NULL names, some assumes name is
> alway valid.

It's perfect if it really never occurs but it never hurts to recheck
it. It's just my "defensive" (or you may call it "paranoid")
developing approach that I got used to. In this case, if something
"suspicious" or meaningless is found, it should not be returned it as
it may confuse or even crash a poorly written application.

Alternatively, the function could return an error if something like
this is detected.

> I only have one comment left here: 'char ***arg' is a really unfortunate argument type. I bet that will scare weak-hearted users really good. The least we can do is to offer an example in the man page showing how to use it...

Unfortunately this is a case if a function must return an allocated
array of "strings". If the number of asterisks is really such a
problem, they could be "hidden" by wrapping the list into a one-member
struct. But in my opinion this really doesn't make any sense. Instead
I have prepared a short example to be inserted into the man page.
Please review it, let me know about any comments about its style
(khmm, are there any Nroff tags to mark an example of code?), should
something be added or cut? Then I will prepare and send the patch.

------------ sample start -------------------
.SH EXAMPLE
#include "libssh2.h"

const char **algorithms;
int rc;
LIBSSH2_SESSION *session;

/* initilize session */
session = libssh2_session_init();
rc = libssh2_session_supported_algs(session,
                                     LIBSSH2_METHOD_CRYPT_CS,
                                    &algorithms);
if (rc>0) {
    /* the call succeeded, do sth. with the list of algorithms,
       (e.g. list them)... */
    int i;
    printf("Supported symmetric algorithms:\n");
    for ( i=0; i<rc; i++ )
        printf("\t%s\n", algorithms[i]);

    /* ... and free the allocated memory when not needed anymore */
    libssh2_free(session, algorithms);
}
else {
    /* call failed, error handling */
}
---------------- sample end ---------------------

Regards, Jernej
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
Received on 2011-11-14