On Tue, Jan 20, 2009 at 07:58:07PM +0100, Hans de Goede wrote:
> 
> Hi,
> 
> Konrad Rzeszutek wrote:
> 
> Thanks for the review!
> 
>  > I presume you have run this program (and the test-code) through
>  > valgrind with no memory leaks?
>  >
> 
> Erm, no, has iscsiadm been run through valgrind? If not I'm not going to be 
> running libiscsi through it either (sorry) libiscsi builds on top of many 

I meant the test-cases. 

> open-iscsi userspace code internals, and those are not always pretty (many 
> global variables for example).

I believe valgrind marks those as 'possible memory-leaks'


.. snip ..

>  >  > +#include "iface.h"
>  >
>  > Do these guys need to be in quotes? In the Makefile you did specify the
>  > header
>  > directory so I would think that would work.
>  >
> 
> Using <> might work too, but this way it is clear this are open-iscsi 
> headers, 
> and not system headers.

Sounds fair.

.. snip ..
>  >  > +    CHECK(discovery_sendtargets(&drec, &new_rec_list))
>  >  > +    CHECK(idbm_add_discovery(&drec, 1))
>  >
>  > Make the 1 a #define? Without me looking at the code I had
>  > no idea that 1 stands for 'over-write'. Thought at first it
>  > was the count of records - which looked weird.
>  >
> 
> Internal open-iscsi API, when it becomes a define there I'll happily use it.

How about a comment instead then.

> 
>  >  > +
>  >  > +    /* bind ifaces to node recs so we know what we have */
>  >  > +    list_for_each_entry(rec, &new_rec_list, list)
>  >  > +        CHECK(idbm_bind_ifaces_to_node(rec, NULL, &bound_rec_list))
>  >  > +
>  >  > +    /* now add/update records */
>  >  > +    list_for_each_entry(rec, &bound_rec_list, list) {
>  >  > +        CHECK(idbm_add_node(rec, &drec, 1))
>  >
>  > Ditto.
>  >
> 
> Ditto.

Ditto :-)

> 
>  >  > +        found++;
>  >  > +    }
>  >  > +
>  >  > +    if (found_nodes && found) {
>  >  > +        *found_nodes = calloc(found, sizeof **found_nodes);
>  >
>  > Please swap the arguments.
>  >
> 
> Erm, no, this time I got it right the first time.

Right. 

.. snip ..
>  >  > +int login_helper(void *data, node_rec_t *rec)
>  >  > +{
>  >  > +    int rc = iscsid_req_by_rec(MGMT_IPC_SESSION_LOGIN, rec);
>  >  > +    if (rc) {
>  >  > +        iscsid_handle_error(rc);
>  >  > +        rc = ENOTCONN;
>  >
>  > Should that have a - in front of it? Hmm.. I thought Mike wanted
>  > all of the return codes to be a positive number. Which means all
>  > the other functions you have should _NOT_ have the '-'?
>  >
>  > Looking at the 'idbm_*' all of its return codes are positive. Perhaps
>  > a global search and replace for the -E to E?
>  >
> 
> The only functions returning negative codes are the discovery functions, as 

I think having the same mechanism and behavior to return error codes will
save folks from trouble down the line. Having all functions return a positive
error codes.


> they return the number of nodes found on success. If people dislike this I 
> can 
> return the number of found nodes through a pointer instead.


> 
>  >  > +    }
>  >  > +    return rc;
>  >  > +}
>  >  > +
>  >  > +int libiscsi_node_login(struct libiscsi_context *context,
>  >  > +    const struct libiscsi_node *node)
>  >  > +{
>  >  > +    int nr_found = 0, rc;
>  >  > +
>  >  > +    CHECK(idbm_for_each_iface(&nr_found, NULL, login_helper,
>  >  > +        (char *)node->name, node->tpgt,
>  >  > +        (char *)node->address, node->port))
>  >  > +    if (nr_found == 0)
>  >  > +        rc = ENODEV;
>  >  > +leave:
>  >  > +    return rc;
>  >  > +}
>  >  > +
>  >  > +static int logout_helper(void *data, struct session_info *info)
>  >  > +{
>  >  > +    int rc;
>  >  > +    struct node_rec *rec = data;
>  >  > +
>  >  > +    if (!iscsi_match_session(rec, info))
>  >  > +        return -1; /* Not a match */
>  >
>  > Oh boy. Can you put a comment of why you are mixing standard error
>  > codes with negative numbers? At first I thought this was an error
>  > until I looked in 'iscsi_sysfs_for_each_session' and found:
>  > "if less than zero it means it was not a match"
>  >
>  > Can you just say that 'iscsi_sysfs_for_each_session' requires this
>  > type of return code for not a match please?
>  >
> 
> You mean make the comment more verbose, sure if that makes you happy :) This 

Please do.

> are those infamous open-iscsi internals again, which I'm trying really hard 
> to 
> hide from the outside (iow when only looking at libiscsi.h) once you start 
> looking at libiscsi.c, well ...
> 
> 
>  > .. snip ..
>  >
>  >  > +static int get_parameter_helper(void *data, node_rec_t *rec)
>  >  > +{
>  >  > +    struct db_set_param *param = data;
>  >  > +    recinfo_t *info;
>  >  > +    int i;
>  >  > +
>  >  > +    info = idbm_recinfo_alloc(MAX_KEYS);
>  >  > +    if (!info)
>  >  > +        return ENOMEM;
>  >  > +
>  >  > +    idbm_recinfo_node(rec, info);
>  >  > +
>  >  > +    for (i = 0; i < MAX_KEYS; i++) {
>  >  > +        if (!info[i].visible)
>  >  > +            continue;
>  >  > +
>  >  > +        if (strcmp(param->name, info[i].name))
>  >
>  > How about 'strncmp' ?
>  >
> 
> Why?

Knee-jerk reaction. In this case I can see you don't do anything
else beside 'continue' so it doesn't matter.

> 
>  >  > +            continue;
>  >  > +
>  >  > +        strlcpy(param->value, info[i].value, LIBISCSI_VALUE_MAXLEN);
>  >  > +        return 0;
>  >  > +    }
>  >  > +
>  >  > +    return EINVAL; /* No such parameter */
>  >  > +}
>  >  > +
>  >
>  > .. snip ..
>  >
>  >  > diff -urN open-iscsi-2.0-870.1.orig/libiscsi/libiscsi.h
>  > open-iscsi-2.0-870.1/libiscsi/libiscsi.h
>  >  > --- open-iscsi-2.0-870.1.orig/libiscsi/libiscsi.h    1970-01-01
>  > 01:00:00.000000000 +0100
>  >  > +++ open-iscsi-2.0-870.1/libiscsi/libiscsi.h    2009-01-19
>  > 12:19:04.000000000 +0100
>  >  > @@ -0,0 +1,236 @@
>  >  > +#include <netdb.h>
>  >  > +
>  >  > +/** \brief Maximum length for iSCSI values.
>  >  > + *
>  >  > + * Maximum length for iSCSI values such as hostnames and parameter
>  > values.
>  >  > + */
>  >  > +#define LIBISCSI_VALUE_MAXLEN 256
>  >  > +
>  >  > +/** \brief supported authentication methods
>  >  > + *
>  >  > + * This enum lists all supported authentication methods.
>  >  > + */
>  >  > +enum libiscsi_auth_t {
>  >  > +    libiscsi_auth_none   /** No authentication */,
>  >  > +    libiscsi_auth_chap   /** CHAP authentication */,
>  >  > +};
>  >  > +
>  >  > +/** \brief libiscsi context struct
>  >  > + *
>  >  > + * Note: even though libiscsi uses a context struct, the underlying
>  > open-iscsi
>  >  > + * code does not, so libiscsi is not thread safe, not even when
>  > using one
>  >  > + * context per thread!
>  >  > + */
>  >  > +struct libiscsi_context;
>  >  > +
>  >  > +/** \brief iSCSI node record
>  >  > + *
>  >  > + * Struct holding data uniquely identifying an iSCSI node.
>  >  > + */
>  >  > +struct libiscsi_node {
>  >
>  > Add     'unsigned int version' in case this structs grows in the future.
>  > The
>  > version for right could be '100'.
>  >
>  >  > +    char name[LIBISCSI_VALUE_MAXLEN]     /** iSCSI iqn for the node.
>  > */;
>  >  > +    int tpgt                             /** Portal group number. */;
>  >
>  > Not unsigned int? The RFC says " - Target Portal Group Tag: A numerical
>  > identifier (16-bit) for an
>  >      iSCSI Target Portal Group."
>  >
> 
> open-iscsi-2.0-870.1/usr/config.h: 211:
>          int                     tpgt;
> 
> This is from the already existing code.

ARGH!

> 
>  >
>  >  > +    /* Note open-iscsi has some code in place for multiple
>  > connections in one
>  >  > +       node record and thus multiple address / port combi's, but
>  > this does not
>  >  > +       get used anywhere, so we keep things simple and assume one
>  > connection */
>  >  > +    char address[NI_MAXHOST]             /** Portal hostname or
>  > IP-address. */;
>  >  > +    int port                             /** Portal port number. */;
>  >
>  > The same. unsigned int?
>  >
> 
> open-iscsi-2.0-870.1/usr/config.h: 225:
>          int                     port;
> 
> <snip>

ARGH!

> 
>  >  > +/** \brief Discover iSCSI nodes using sendtargets and add them to
>  > the node db.
>  >  > + *
>  >  > + * This function connects to the given address and port and then
>  > tries to
>  >  > + * discover iSCSI nodes using the sendtargets protocol. Any found
>  > nodes are
>  >  > + * added to the local iSCSI node database and are returned in a
>  > dynamically
>  >  > + * allocated array.
>  >  > + *
>  >  > + * Note that the (optional) authentication info is for
>  > authenticating the
>  >  > + * discovery, and is not for the found nodes! If the connection(s)
>  > to the
>  >  > + * node(s) need authentication too, you can set the username /
>  > password for
>  >  > + * those (which can be different!) using the
>  > libiscsi_node_set_auth() function.
>  >  > + *
>  >  > + * \param context                libiscsi context to operate on.
>  >  > + * \param address                Hostname or IP-address to connect to.
>  >  > + * \param port                   Port to connect to.
>  >  > + * \param auth_info              Authentication information, or NULL.
>  >  > + * \param found_nodes            The address of the dynamically
>  > allocated array
>  >  > + *                               of found nodes will be returned
>  > through this
>  >  > + *                               pointer if not NULL. The caller
>  > must free this
>  >  > + *                               array using free().
>  >  > + * \return                       The number of found nodes or, in
>  > case of error,
>  >  > + *                               a negative standard error code
>  > (from errno.h).
>  >
>  > Values in errno are positive. Look in /usr/include/asm-generic/errno-base.h
>  >
> 
> When stored in errno, yes but for example when passed along inside the kernel 
> they are negative.

Correct. We are not in the kernel land, but in the userland where these values
are positive. Hence they should be positive.


> 
>  > Maybe have an addtional argument for the number of found nodes and use the
>  > return value just for error codes?
> 
> We could do that, but I'm not entirely happy with doing things that way, iow 
> I 
> like the current method better. Still if there are more people in favor of 
> making this change I won't block it.

In my experience maintability is more important that cleaverness when working
on libraries. Having them do exactly one thing and only one thing will
make it easier to maintain the code.

> 
>  > Also that would mean using the proper
>  > declaration for the number of nodes. Imagine finding 32,768 nodes, and
>  > trying
>  > to demonstrate that as a 'int'. You would end up with -1, which in your
>  > case means -EPERM!
>  >
>  > (Yeah I know, who in their right mind would want to have have 32768
>  > nodes ..!?)
>  >
> 
> Actually an int is atleast 32 bit on any relevant system, so that would be 2 
> billion nodes and then some more.

That is true. Thought having one mechanism to do two things (error returning
and the count) smacks of programmers error in the future. Having an extra
argument that clearly defines it IMHO saves us from trouble down the road.

> 
> <snip>
> 
>  >  > + */
>  >  > +int libiscsi_discover_firmware(struct libiscsi_context *context,
>  >  > +    struct libiscsi_node **found_nodes);
>  >
>  >
>  >  .. snip ..
>  >  > +/** \brief Get human readable string describing the last libiscsi
>  > error.
>  >  > + *
>  >  > + * This function can be called to get a human readable error string
>  > when a
>  >  > + * libiscsi function has returned an error. This function uses a
>  > static buffer
>  >  > + * thus the result is only valid as long as no other libiscsi calls
>  > are made
>  >  > + * after the failing function call.
>  >  > + *
>  >  > + * \param context       libiscsi context to operate on.
>  >  > + *
>  >  > + * \return human readable string describing the last libiscsi error.
>  >  > + */
>  >  > +const char *libiscsi_get_error_string(struct libiscsi_context
>  > *context);
>  >
>  > Why is this neccessary? 'strerror' won't work?
> 
> Sure but that will only give you a very vague error like "no such file or 
> directory, while as this will return something like:
> "Error trying to open 
> /var/lib/iscsi/nodes/iqn.foo.bar:testdisk/127.0.0.1,2670:
>   no such file or directory"

OK. I think the previous concerns about thread safety are important. The idea I 
proposed
below would fix that.

> 
>  > If you want to carry
>  > error messages
>  > maybe have all the functions return a struct, such as this:
>  >
>  > struct iscsi_err {
>  >     int    errno;
>  >     const char *msg;
>  > }
>  >
>  > Which would carry the relevant error code and the message?
>  >
> 
> Becuase that makes using the library harder, esp when one is not interested 
> in 
> such verbose error messages.

I am definitly interested in these errors. And this is thread safe so you won't
have to worry about it. It would also remove the need for 
libiscis_get_error_string
function.

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~----------~----~----~----~------~----~------~--~---

Reply via email to