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
open-iscsi userspace code internals, and those are not always pretty (many
global variables for example).
> 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.
>
Using <> might work too, but this way it is clear this are open-iscsi headers,
and not system headers.
> > +
> > +#define CHECK(a) { rc = a; if (rc) goto leave; }
> > +
> > +struct libiscsi_context {
> > + char libiscsi_error_string[256];
>
> Use a #define for the size.
>
Why? Once we will actually start using this, it will be filled with snprintf,
with a sizeof as second parameter, so I see no reason to use a define.
> > +};
> > +
> > +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.
>
You are right, will fix.
> > + 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()?
>
log_close() does not exist (weird open-iscsi internals), but yes it needs a
sysfs_cleanup() call.
> > + 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?
>
Yes, that is the plan when I actually get error logging implemented, see the
comments about this in my initial mail (I'm waiting on feedback on some
proposed changes to the open-iscsi internal logging API).
> > + 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.
It does, first the passed in string gets strlcpy-ed to
drec.u.sendtargets.auth.password, strlcpy will copy at max AUTH_STR_MAX_LEN - 1
bytes and will always 0 terminate the dest.
Then the lenght of drec.u.sendtargets.auth.password gets used, not the length
of what was passed in but the length of the (limited length) copy.
<snip>
> > + 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.
>
Internal open-iscsi API, when it becomes a define there I'll happily use it.
> > +
> > + /* 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.
> > + 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.
>
> .. 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.
>
Notice the CHECK macro, if it fails it will stop at the first failing call, and
return an error code (and once implemented save an error string which can be
retrieved later).
> > + 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..
>
Ack.
> > + 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?
>
My bad, thanks for catching this, that should be a strlcpy (strncpy sucks
majorly!).
> > + 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?
>
The only functions returning negative codes are the discovery functions, as
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
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?
> > + 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.
>
> > + /* 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>
> > +/** \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.
> 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.
> 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.
<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"
> 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.
>
> > 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.
>
Well, they are a bit hardcoded to my setup, I think I'll add an /etc/ietd.conf
which can be used to run the tests with, but they still won't run out of the
box on any system. But with that said, yes tests are good!
>
> Thank you for the library.
I'm glad you like it :)
> I am really looking forward to using it
> instead of
> wrapping all calls to iSCSI via 'iscsiadm' and this makes it so much
> easier.
I know, wrapping iscsiadm can be a pain, that is why I wrote this lib.
Regards,
Hans
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
