Re: PATCH: make open-iscsi userspace tools functionality available as a library
On 21 Jan 2009 at 16:36, Konrad Rzeszutek wrote: On Wed, Jan 21, 2009 at 11:48:41AM +0100, Ulrich Windl wrote: On 20 Jan 2009 at 9:23, Konrad Rzeszutek wrote: I would recommend that you provide as the first variable in all of the structs an unsigned int called 'version'. This way if the structs are extended they would be backwards compatible and there is an easy way to identify which version of structs they are. Hi! When doing an initial version, why not try to do it right at the beginning? Addind a version number to a poor design is terrible, because for compatibility you'll always have to support old versions then. If you do not, why the version number? Because we are humans and we make mistakes. It is one of the mechanism to fix something later on when an Application Binary Interface (ABI) is holy and cannot be changed. Yes sure, (I had written some primitive communication protocol some years ago, where the client asked the server what protocol version it did support (actually it was a feature mask). After about the 7th revision of the protocol, I wondered whether the server should still support all the older protocol versions). For your case: I think defining a library interface is not an add-hoc task for one afternoon; it should be carefully designed (e.g. being thread-safe, even if the implementation is not: Once the implementation is, the application need not be changed -- the interface is stable). Manage your interfaces better than the banks did manage their money ;-) Regards, Ulrich --~--~-~--~~~---~--~~ 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 -~--~~~~--~~--~--~---
Re: PATCH: make open-iscsi userspace tools functionality available as a library
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
--~--~-~--~~~---~--~~
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
-~--~~~~--~~--~--~---
Re: PATCH: make open-iscsi userspace tools functionality available as a library
On 20 Jan 2009 at 9:23, Konrad Rzeszutek wrote: I would recommend that you provide as the first variable in all of the structs an unsigned int called 'version'. This way if the structs are extended they would be backwards compatible and there is an easy way to identify which version of structs they are. Hi! When doing an initial version, why not try to do it right at the beginning? Addind a version number to a poor design is terrible, because for compatibility you'll always have to support old versions then. If you do not, why the version number? My opinion... Regards, Ulrich --~--~-~--~~~---~--~~ 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 -~--~~~~--~~--~--~---
Re: PATCH: make open-iscsi userspace tools functionality available as a library
On Wed, Jan 21, 2009 at 11:48:41AM +0100, Ulrich Windl wrote: On 20 Jan 2009 at 9:23, Konrad Rzeszutek wrote: I would recommend that you provide as the first variable in all of the structs an unsigned int called 'version'. This way if the structs are extended they would be backwards compatible and there is an easy way to identify which version of structs they are. Hi! When doing an initial version, why not try to do it right at the beginning? Addind a version number to a poor design is terrible, because for compatibility you'll always have to support old versions then. If you do not, why the version number? Because we are humans and we make mistakes. It is one of the mechanism to fix something later on when an Application Binary Interface (ABI) is holy and cannot be changed. --~--~-~--~~~---~--~~ 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 -~--~~~~--~~--~--~---
Re: PATCH: make open-iscsi userspace tools functionality available as a library
Bart Van Assche wrote: On Mon, Jan 19, 2009 at 4:35 PM, Hans de Goede hdego...@redhat.com wrote: - libiscsi_discover_sendtargets - maybe (very maybe) the int port could be dropped and const char *address could be of the form address_or_host[:port]. Regarding defaults and all that. Nah, that is ok for a cmdline interface, but cumbersome for an API, we could do something where port == 0 would choose the default port though. Have you considered to replace both const char *address and int port by const struct sockaddr *address, socklen_t address_len ? This would avoid having to figure out inside libiscsi_discover_sendtargets() whether the passed address is in IPv4 or in IPv6 format (if numeric). libiscsi builds on top of the existing open-iscsi userspace code, in this code the const char *address and int port notation is used everywhere. Regards, Hans --~--~-~--~~~---~--~~ 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 -~--~~~~--~~--~--~---
Re: PATCH: make open-iscsi userspace tools functionality available as a library
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.0 +0100
+++ open-iscsi-2.0-870.1/libiscsi/libiscsi.c 2009-01-19 12:28:08.0
+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
Re: PATCH: make open-iscsi userspace tools functionality available as a library
I would recommend that you provide as the first variable in all of the structs an unsigned int called 'version'. This way if the structs are extended they would be backwards compatible and there is an easy way to identify which version of structs they are. Erm, given the amount of programs which will probably end up using this (not all that much) a distro should be able to easily rebuild all those in case of an ABI change and thus a soname bump. I understand what you are trying to say here, but IMHO the added complexity and ugliness is not worth it. I disagree. The ABI in a distro is a holy thing. When a Linux distro releases their libraries (look for example at libvirt), they can't modify it for the next 7 years (sure they can do it underneath the covers, but look at the Red Hat kernel with those #ifdef __GENKSYSM__ to work-around this). I am curious to understand what is complex and ugly about it? Could you be more specific please. --~--~-~--~~~---~--~~ 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 -~--~~~~--~~--~--~---
Re: PATCH: make open-iscsi userspace tools functionality available as a library
On Tue, Jan 20, 2009 at 7:20 PM, Konrad Rzeszutek kon...@virtualiron.com wrote: I would recommend that you provide as the first variable in all of the structs an unsigned int called 'version'. This way if the structs are extended they would be backwards compatible and there is an easy way to identify which version of structs they are. Erm, given the amount of programs which will probably end up using this (not all that much) a distro should be able to easily rebuild all those in case of an ABI change and thus a soname bump. I understand what you are trying to say here, but IMHO the added complexity and ugliness is not worth it. I disagree. The ABI in a distro is a holy thing. When a Linux distro releases their libraries (look for example at libvirt), they can't modify it for the next 7 years (sure they can do it underneath the covers, but look at the Red Hat kernel with those #ifdef __GENKSYSM__ to work-around this). I am curious to understand what is complex and ugly about it? Could you be more specific please. I agree that having a stable binary interface is very important for a shared library. But if glibc doesn't have versioned structs, why would these be needed in an iscsi library ? Please keep in mind that it is possible to define a new version of a shared library in case the ABI changes. Bart. --~--~-~--~~~---~--~~ 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 -~--~~~~--~~--~--~---
Re: PATCH: make open-iscsi userspace tools functionality available as a library
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.c1970-01-01
01:00:00.0 +0100
+++ open-iscsi-2.0-870.1/libiscsi/libiscsi.c2009-01-19
12:28:08.0 +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.
+
+
Re: PATCH: make open-iscsi userspace tools functionality available as a library
On Tue, Jan 20, 2009 at 07:40:08PM +0100, Bart Van Assche wrote: On Tue, Jan 20, 2009 at 7:20 PM, Konrad Rzeszutek kon...@virtualiron.com wrote: I would recommend that you provide as the first variable in all of the structs an unsigned int called 'version'. This way if the structs are extended they would be backwards compatible and there is an easy way to identify which version of structs they are. Erm, given the amount of programs which will probably end up using this (not all that much) a distro should be able to easily rebuild all those in case of an ABI change and thus a soname bump. I understand what you are trying to say here, but IMHO the added complexity and ugliness is not worth it. I disagree. The ABI in a distro is a holy thing. When a Linux distro releases their libraries (look for example at libvirt), they can't modify it for the next 7 years (sure they can do it underneath the covers, but look at the Red Hat kernel with those #ifdef __GENKSYSM__ to work-around this). I am curious to understand what is complex and ugly about it? Could you be more specific please. I agree that having a stable binary interface is very important for a shared library. But if glibc doesn't have versioned structs, why would these be needed in an iscsi library ? Please keep in mind that it is possible to define a new version of a shared library in case the ABI changes. Yeah. I just thought about that when I sent the e-mail about libvirt. I think that is a good compromise - however we still need the ground-work in place so that this library is built as libiscsi.so.1.0.0 and that the linker saves the version information. I guess this implies that the .spec file needs to be changed to include this file as well. --~--~-~--~~~---~--~~ 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 -~--~~~~--~~--~--~---
Re: PATCH: make open-iscsi userspace tools functionality available as a library
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,
Re: PATCH: make open-iscsi userspace tools functionality available as a library
On Mon, Jan 19, 2009 at 2:07 PM, Hans de Goede hdego...@redhat.com wrote: Therefore we would like to export (some) of the functionality of iscsiadm as a C-library. Great ! I've got documentation of the proposed API here: http://people.atrpms.net/~hdegoede/html/libiscsi_8h.html Not so great: libiscsi_get_error_string() 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. This makes it impossible to use libiscsi_get_error_string() in a safe way in multithreaded software. Furthermore, there are some minor spelling issues, e.g. cleanups. Shouldn't this be cleans up ? Bart. --~--~-~--~~~---~--~~ 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 -~--~~~~--~~--~--~---
Re: PATCH: make open-iscsi userspace tools functionality available as a library
Bart Van Assche wrote: On Mon, Jan 19, 2009 at 2:07 PM, Hans de Goede hdego...@redhat.com wrote: Therefore we would like to export (some) of the functionality of iscsiadm as a C-library. Great ! I've got documentation of the proposed API here: http://people.atrpms.net/~hdegoede/html/libiscsi_8h.html Not so great: libiscsi_get_error_string() 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. That should read: This function uses a single buffer per context, thus the result is only valid as long as no other libiscsi calls are made on the same context after the failing function call. Actually this reminds me this function currently isn't even implemented. I'm planning on changing usr/log.c to make this possible. The idea is that dolog becomes a function pointer which can be pointer to either a function doing the current log_daemon behavior, or to one doing the current print to stderr behavior. Then the log_daemon variable can be removed, and libiscsi can supply its own logging function which can store the last error message. But before making these changes I first wanted to discuss this on this list, so are there any objections against this change? This makes it impossible to use libiscsi_get_error_string() in a safe way in multithreaded software. Well, as long as you use one context per thread, it should be safe, atleast API wise, unfortunately the used existing open-iscsi code is full of global variables. So thread safety will come as a future enhancement. Actually the only reason for the whole context paradigm in the current API is to allow a move over to thread safety in the future without breaking ABI. Furthermore, there are some minor spelling issues, e.g. cleanups. Shouldn't this be cleans up ? patches welcome :) Regards, Hans --~--~-~--~~~---~--~~ 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 -~--~~~~--~~--~--~---
Re: PATCH: make open-iscsi userspace tools functionality available as a library
Boaz Harrosh wrote: Hans de Goede wrote: Hi All, The API currently offers pretty minimal functionality (just what we need in anaconda) I'm fine with extending this (patches welcome). But currently I would like to focus on the set of functionality as the current API offers and try to get that right. Most important here is to have a clean, clear and usable API which is also future proof, as I want to freeze the ABI of the available functions asap. WRT this I would especially like feedback on the futrue proofness of the libiscsi_node struct. First of all Thank you very much for doing this. I'm sure it will prove very useful in the future, the issue has been raised in the passed, multiple times. I can see that you have not attempted to refactor iscsiadm so to use the library. Which means you will be shipping the same exact code twice. If you look at the code you will see that there actually is not all that much duplication. Is your long term plan to do that once libiscsi is mature enough, or you would rather keep these two separate and duplicated? To keep them separate, see above. Regarding the API: - The overall state and division of labor looks very nice. :-) - libiscsi_discover_sendtargets - maybe (very maybe) the int port could be dropped and const char *address could be of the form address_or_host[:port]. Regarding defaults and all that. Nah, that is ok for a cmdline interface, but cumbersome for an API, we could do something where port == 0 would choose the default port though. - libiscsi_discover_isns - Looks like it is missing the actual input parameters usually a char *iqn I think? I've checked the (AFAIK currently incomplete) iscsiadm code and that does not have any parameters. Given that this is pretty much a pie in the sky, we could just remove this function completely for now. - libiscsi_discover_firmware - What is that for? Also missing an input parameter. This reads iscsi target info (portal and authentication info) from the BIOS of machines who support booting from iscsi, there is no input parameter, as machines tend to have only one BIOS (to query). - I can't help noticing the missing of query functions, Both query of DB of discovered Nodes, as well as info of logged-in nodes. Ack, those are not needed by anaconda, but should probably be added to make this more generally usefull, patches welcome :) - libiscsi_node_set_parameter - Question: (Sorry have not read the code) Is that persistent, gets saved in DB. Or is just for current session/login? Actually it only changes the database, so it will not influence parameters of the current session. I guess this needs better documentation making this clear. Regards, Hans --~--~-~--~~~---~--~~ 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 -~--~~~~--~~--~--~---
Re: PATCH: make open-iscsi userspace tools functionality available as a library
On Mon, Jan 19, 2009 at 4:35 PM, Hans de Goede hdego...@redhat.com wrote: - libiscsi_discover_sendtargets - maybe (very maybe) the int port could be dropped and const char *address could be of the form address_or_host[:port]. Regarding defaults and all that. Nah, that is ok for a cmdline interface, but cumbersome for an API, we could do something where port == 0 would choose the default port though. Have you considered to replace both const char *address and int port by const struct sockaddr *address, socklen_t address_len ? This would avoid having to figure out inside libiscsi_discover_sendtargets() whether the passed address is in IPv4 or in IPv6 format (if numeric). Bart. --~--~-~--~~~---~--~~ 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 -~--~~~~--~~--~--~---
Re: PATCH: make open-iscsi userspace tools functionality available as a library
On 19 Jan 2009 at 17:16, Boaz Harrosh wrote: [...] - libiscsi_discover_sendtargets - maybe (very maybe) the int port could be dropped and const char *address could be of the form address_or_host[:port]. Regarding defaults and all that. [...] I'd like to comment that I feel the libiscsi_ prefix is a bit too long. Wouldn't just iscsi_ or iSCSI_ be long enough? Regards, Ulrich --~--~-~--~~~---~--~~ 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 -~--~~~~--~~--~--~---
