Hans de Goede wrote:
> 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.
>
I understood the first time, but unless someone can show me clean iscsiadm
valgrind results, I'm not going to be spending time on running libiscsi through
valgrind. libiscsi is a wrapper around the open-iscsi userspace code used by
iscsiadm, and I have no time to go and make that code completely valgrind clean.
<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.
>
Will do.
<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.
>
Ok, I have to agree that having all error codes be positive would be the
consistent thing to do, so I'll make this change.
<snip>
> > > > +{
> > > > + 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
> :)
>
> Please do.
>
Will do.
<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.
The thread safety model for libiscsi (if the used internal open-iscsi code ever
becomes thread clean) is that of having one context per thread, since the last
error message is stored in the context, there is no thread safety problem here.
Regards,
Hans
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
