NACK.
I presume you have run this program (and the test-code) through
valgrind with no memory leaks?
Please see my comments below.
> diff -urN open-iscsi-2.0-870.1.orig/libiscsi/libiscsi.c
> open-iscsi-2.0-870.1/libiscsi/libiscsi.c
> --- open-iscsi-2.0-870.1.orig/libiscsi/libiscsi.c 1970-01-01
> 01:00:00.000000000 +0100
> +++ open-iscsi-2.0-870.1/libiscsi/libiscsi.c 2009-01-19 12:28:08.000000000
> +0100
> @@ -0,0 +1,354 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include "libiscsi.h"
> +#include "idbm.h"
> +#include "iscsiadm.h"
> +#include "log.h"
> +#include "sysfs.h"
> +#include "iscsi_sysfs.h"
> +#include "util.h"
> +#include "sysdeps.h"
> +#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.
Also it might make sense to order these guys in alphabetical order.
> +
> +#define CHECK(a) { rc = a; if (rc) goto leave; }
> +
> +struct libiscsi_context {
> + char libiscsi_error_string[256];
Use a #define for the size.
> +};
> +
> +struct libiscsi_context *libiscsi_init(void)
> +{
> + struct libiscsi_context *context;
> +
> + context = calloc(sizeof *context, 1);
Swap the arguments around. First argument is the number of elements, while the
second
is the size of the element.
> + if (!context)
> + return NULL;
> +
> + /* enable stdout logging */
> + log_daemon = 0;
> + log_init("libiscsi", 1024);
> + sysfs_init();
> + increase_max_files();
> + if (idbm_init(NULL)) {
> + free(context);
No sysfs_cleanup() call? Or log_close()?
> + return NULL;
> + }
> +
> + iface_setup_host_bindings();
> +
> + return context;
> +}
> +
... snip..
> +int libiscsi_discover_sendtargets(struct libiscsi_context *context,
> + const char *address, int port, const struct libiscsi_auth_info
> *auth_info,
> + struct libiscsi_node **found_nodes)
.. snip ..
> + if (!auth_info->chap.username[0])
Would it make sense to add logging here as well? Like
'log_warning("Non-existent CHAP username!")' or such?
> + return -EINVAL;
> + if (!auth_info->chap.password[0])
> + return -EINVAL;
Is against the CHAP to have an empty password? Ah n/m. The
http://www.ietf.org/rfc/rfc1994.txt says: "The CHAP algorithm requires
that the length of the secret MUST be at least 1 octet." Maybe
put a check for that?
BTW, I am not sure if the PPP CHAP RFC == iSCSI CHAP, so I
might be spewing non-sense here.
> + if (auth_info->chap.reverse_username[0] &&
> + !auth_info->chap.reverse_password[0])
> + return -EINVAL;
> +
> + drec.u.sendtargets.auth.authmethod = AUTH_METHOD_CHAP;
> + strlcpy(drec.u.sendtargets.auth.username,
> + auth_info->chap.username, AUTH_STR_MAX_LEN);
> + strlcpy((char *)drec.u.sendtargets.auth.password,
> + auth_info->chap.password, AUTH_STR_MAX_LEN);
> + drec.u.sendtargets.auth.password_length =
> + strlen((char *)drec.u.sendtargets.auth.password);
This doesn't guard against passwords that are of AUTH_STR_MAX_LEN length. Which
means that there might not be an \0 at then end and your password_length
computation ends up looking at the next structure fields. Or worst (shudder)
Unlikely, but it could happen - and the check is easy enough:
drec.u.sendtargets.auth.password_length =
drec.u.sendtargets.auth.password_length > AUTH_STR_MAX_LEN ? AUTH_STR_MAX_LEN
: drec.u.sendtargets.auth.password_length;
Maybe even do that before any 'strlcpy' is done and use this newly
computed length instead of the defines?
> + strlcpy(drec.u.sendtargets.auth.username_in,
> + auth_info->chap.reverse_username, AUTH_STR_MAX_LEN);
> + strlcpy((char *)drec.u.sendtargets.auth.password_in,
> + auth_info->chap.reverse_password, AUTH_STR_MAX_LEN);
> + drec.u.sendtargets.auth.password_in_length =
> + strlen((char *)drec.u.sendtargets.auth.password_in);
And the same check here..
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + 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.
> +
> + /* 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.
> + found++;
> + }
> +
> + if (found_nodes && found) {
> + *found_nodes = calloc(found, sizeof **found_nodes);
Please swap the arguments.
.. snip ..
> +int libiscsi_node_set_auth(struct libiscsi_context *context,
> + const struct libiscsi_node *node,
> + const struct libiscsi_auth_info *auth_info)
> +{
> + int rc = 0;
> +
> + switch(auth_info ? auth_info->method : libiscsi_auth_none) {
> + case libiscsi_auth_none:
> + CHECK(libiscsi_node_set_parameter(context, node,
> + "node.session.auth.authmethod", "None"))
> + CHECK(libiscsi_node_set_parameter(context, node,
> + "node.session.auth.username", ""))
> + CHECK(libiscsi_node_set_parameter(context, node,
> + "node.session.auth.password", ""))
> + CHECK(libiscsi_node_set_parameter(context, node,
> + "node.session.auth.username_in", ""))
> + CHECK(libiscsi_node_set_parameter(context, node,
> + "node.session.auth.password_in", ""))
Would it make sense to print a warning why this failed (if it fails obviously?)
As a developer of this I would not have an idea from the header file why it
failed
and would have to start instrumenting this code to figure what is wrong.
> + break;
> +
> + case libiscsi_auth_chap:
> + if (!auth_info->chap.username[0])
> + return -EINVAL;
> + if (!auth_info->chap.password[0])
Perhaps the same idea. Put in a log_warning? Or maybe log_debug..
> + return -EINVAL;
> + if (auth_info->chap.reverse_username[0] &&
> + !auth_info->chap.reverse_password[0])
> + return -EINVAL;
> +
.. snip ..
> +static void node_to_rec(const struct libiscsi_node *node,
> + struct node_rec *rec)
> +{
> + memset(rec, 0, sizeof *rec);
> + idbm_node_setup_defaults(rec);
> + strlcpy(rec->name, node->name, TARGET_NAME_MAXLEN);
> + rec->tpgt = node->tpgt;
> + strncpy(rec->conn[0].address, node->address, NI_MAXHOST);
You been using 'strlcpy' all the time, but here you use 'strncpy'. Why?
> + rec->conn[0].port = node->port;
> +}
> +
> +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?
> + }
> + 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?
.. 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' ?
> + 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."
> + /* 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?
> +};
> +
> +/** \brief libiscsi CHAP authentication information struct
> + *
> + * Struct holding all data needed for CHAP login / authentication. Note that
> + * \e reverse_username may be a 0 length string in which case only forward
> + * authentication will be done.
> + */
> +struct libiscsi_chap_auth_info {
> + char username[LIBISCSI_VALUE_MAXLEN] /** Username */;
> + char password[LIBISCSI_VALUE_MAXLEN] /** Password */;
> + char reverse_username[LIBISCSI_VALUE_MAXLEN] /** Reverse Username */;
> + char reverse_password[LIBISCSI_VALUE_MAXLEN] /** Reverse Password */;
> +};
> +
> +/** \brief generic libiscsi authentication information struct
> + *
> + * Struct holding authentication information for discovery and login.
> + */
> +struct libiscsi_auth_info {
Add a 'unsigned int version' here.
> + enum libiscsi_auth_t method /** Authentication method to use */;
> + union {
> + struct libiscsi_chap_auth_info chap /** Chap specific info */;
> + } /** Union holding method depenend info */;
> +};
> +
> +/** \brief Initalize libiscsi
> + *
> + * This function creates a libiscsi context and initalizes it. This context
> + * is need to use other libiscsi funtions.
> + *
> + * \return A pointer to the created context, or NULL in case of an error.
> + */
> +struct libiscsi_context *libiscsi_init(void);
> +
> +/** \brief Cleanup libiscsi used resource
> + *
> + * This function cleanups any used resources and then destroys the passed
> + * context. After this the passed in context may no longer be used!
> + *
> + * \param context libiscsi context to operate on.
> + */
> +void libiscsi_cleanup(struct libiscsi_context *context);
> +
> +/** \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
Maybe have an addtional argument for the number of found nodes and use the
return value just for error codes? 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 ..!?)
> + */
> +int libiscsi_discover_sendtargets(struct libiscsi_context *context,
> + const char *address, int port, const struct libiscsi_auth_info
> *auth_info,
> + struct libiscsi_node **found_nodes);
Which would translate this too:
> +int libiscsi_discover_sendtargets(struct libiscsi_context *context,
> + const char *address, int port, const struct libiscsi_auth_info
> *auth_info,
> + struct libiscsi_node **found_nodes, size_t *found_count);
> +
> +/** \brief Discover iSCSI nodes using iSNS and add them to the node db.
> + *
> + * This function discovers iSCSI nodes using the iSNS protocol. Any found
> nodes
> + * are added to the local iSCSI node database and are returned in a
> dynamically
> + * allocated array.
> + *
> + * Note this function currently is a stub which will always return -EINVAL
> + * (IOW it is not yet implemented)
> + *
> + * \param context libiscsi context to operate on.
> + * \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).
Ditto.
> + */
> +int libiscsi_discover_isns(struct libiscsi_context *context,
> + struct libiscsi_node **found_nodes);
> +
> +/** \brief Read iSCSI node info from firmware and add them to the node db.
> + *
> + * This function discovers iSCSI nodes using firmware (ppc or ibft). Any
> found
^^^ - iBFT
> + * nodes are added to the local iSCSI node database and are returned in a
> + * dynamically allocated array.
> + *
> + * Note that unlike sendtargets discovery, this function will also read
> + * authentication info and store that in the database too.
> + *
> + * Note this function is currently *untested* !
> + *
> + * \param context libiscsi context to operate on.
> + * \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).
Ditto.
Thought iBFT is limited to 255 entries so you won't have the possible problem
as in
the other functions.
> + */
> +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? 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?
> diff -urN open-iscsi-2.0-870.1.orig/libiscsi/tests/test_discovery.c
> open-iscsi-2.0-870.1/libiscsi/tests/test_discovery.c
> --- open-iscsi-2.0-870.1.orig/libiscsi/tests/test_discovery.c 1970-01-01
> 01:00:00.000000000 +0100
> +++ open-iscsi-2.0-870.1/libiscsi/tests/test_discovery.c 2009-01-19
> 12:33:09.000000000 +0100
Awesome. Thanks for including test-cases.
Thank you for the library. I am really looking forward to using it instead of
wrapping all calls to iSCSI via 'iscsiadm' and this makes it so much easier.
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---
