Re: RFC: do we need a new list for kernel patches
On 06/11/2009 07:41 PM, Mike Christie wrote: Hey, It seems like we have a lot of members on the list that are not kernel developers, but we now have 5 iscsi drivers (qla4xxx, bnx2i, cxgb3i, iscsi_tcp and ib_iser) with another being written. So it seems like we are going to have lots of patches. I would also like to start sending my kernel patches out in a way that everyone can see them. Previously to avoid noise on this list, I have been pinging you guys privately which just does not work so well now when we have so many people. What do you people think? As someone more interested in the userspace side of open-iscsi I sure wouldn't mind missing all the kernel driver patches, actually I would welcome that, less mail is more :) 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 3/4] iscsi class, libiscsi: Add net config.
On 06/08/2009 10:24 PM, Mike Christie wrote: Hans de Goede wrote: snip Sorry for jumping in the middle of a thread, I missed the libiscsi in the subject. Can someone please resent me the libiscsi part of this patchset so that I can review it ? (I'm the libiscsi author) There is actually a kernel module called libiscsi. That is what this patch/thread/mail is for. It is not the userpsace libiscsi lib that you did. Ah, my bad. 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: LPGL status of libiscsi
On 06/01/2009 04:19 AM, Mike Christie wrote: Echo Six wrote: What is the current status of libiscsi? Will it ever be released LGPL? I have not merged it because I think it needs iface support. Or if not iface support, then some way for it to be used with bnx2i and cxgb3i. For the license, I do not care what is used. Hans, I relooked at your patches and saw there was not license info. Do you care? Hmm, have I still not added GPL headers ? Strange, I though I had. I would prefer libiscsi to be LGPL, but the code from iscsiadm it uses is GPL, so unless we change the license on that, the license on libiscsi.c itself does not matter much. 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: Issues with intel firmware initiator, ibft and 2-way chap
Konrad Rzeszutek wrote: Sure, if you can tell me how to get the blob? gcc find_ibft.c -o find_ibft sudo ./find_ibft blob Hi All, Sorry for the long delay. Here is a reminder of what this what about, when doing 2 way chap, booting from an intel server nic with ibft, there is no way to specify the reverse username. And the ibft info in sysfs contains a garbage string for the reverse username. As requested here is a dump of the ibft table. Notice the string consisting of a single capital S followed by a 0xff character, that is what gets shown as the reverse username in sysfs. 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 -~--~~~~--~~--~--~--- blob Description: Binary data
Re: Issues with intel firmware initiator, ibft and 2-way chap
Mike Christie wrote: Konrad Rzeszutek wrote: On Mon, Feb 02, 2009 at 11:39:18AM +0100, Hans de Goede wrote: Hi, When using 2-way chap and booting from an intel network card with intel firmware initiator, their is no way to specify the username in the firmware initiator for the reverse chap, nor does it care what username the target provides. However under sysfs (ibft) there is a reverse username attribute (containing garbage) and as this garbage is not provided by the target open-iscsi does not want to talk to the target. That is b/c the ibft module parses the iBFT structure and this is the offset where the reverse username attribute is at. You can insert a bunch of printks in the module and see data. Better yet, try this attached tool. I wrote it up some time with a different purpose in mind and just now converted it over to read from /dev/mem. Try it and see if the data comes out looking valid. (Last time I used it on an Intel NICs, they would insert 0xFF everywhere in the IBFT). So how do we work around this? Try to use the IBM HS-20 (ibm-hs20-iscsi.lab.redhat.com, I think) Hey, so this is a problem with only intel nics? If so let's talk to the intel guys to see if they are going to fix it, or is already fixed by just updating the firmware. Yes, this seems to be a problem with the ibft of the intel nic firmware. I do not have the all FF FF problem, everything is correct, except for the reverseusername, which is not surprising as the firmware BIOS setup interface does not allow one to specify a reverseusername. But then I would expect the reverse username sysfs attribute to be empty which it is not (which undoubtly is a firmware bug). 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: Issues with intel firmware initiator, ibft and 2-way chap
Konrad Rzeszutek wrote: On Tue, Feb 03, 2009 at 10:30:13AM +0100, Hans de Goede wrote: Mike Christie wrote: Konrad Rzeszutek wrote: On Mon, Feb 02, 2009 at 11:39:18AM +0100, Hans de Goede wrote: Hi, When using 2-way chap and booting from an intel network card with intel firmware initiator, their is no way to specify the username in the firmware initiator for the reverse chap, nor does it care what username the target provides. However under sysfs (ibft) there is a reverse username attribute (containing garbage) and as this garbage is not provided by the target open-iscsi does not want to talk to the target. That is b/c the ibft module parses the iBFT structure and this is the offset where the reverse username attribute is at. You can insert a bunch of printks in the module and see data. Better yet, try this attached tool. I wrote it up some time with a different purpose in mind and just now converted it over to read from /dev/mem. Try it and see if the data comes out looking valid. (Last time I used it on an Intel NICs, they would insert 0xFF everywhere in the IBFT). So how do we work around this? Try to use the IBM HS-20 (ibm-hs20-iscsi.lab.redhat.com, I think) Hey, so this is a problem with only intel nics? If so let's talk to the intel guys to see if they are going to fix it, or is already fixed by just updating the firmware. Yes, this seems to be a problem with the ibft of the intel nic firmware. I do not have the all FF FF problem, everything is correct, except for the reverseusername, which is not surprising as the firmware BIOS setup interface does not allow one to specify a reverseusername. But then I would expect the reverse username sysfs attribute to be empty which it is not (which undoubtly is a firmware bug). OK. Can you send me the binary blob? Maybe there is something that can be done in the iscsi_ibft to detect this and do the right thing (ie, not fill reverse username with garbage). Sure, if you can tell me how to get the blob? 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 -~--~~~~--~~--~--~---
Issues with intel firmware initiator, ibft and 2-way chap
Hi, When using 2-way chap and booting from an intel network card with intel firmware initiator, their is no way to specify the username in the firmware initiator for the reverse chap, nor does it care what username the target provides. However under sysfs (ibft) there is a reverse username attribute (containing garbage) and as this garbage is not provided by the target open-iscsi does not want to talk to the target. So how do we work around this? 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: do not use exit()
Ulrich Windl wrote:
On 28 Jan 2009 at 22:51, Hans de Goede wrote:
Hi All,
While testing I noticed that idbm_lock() uses exit when it cannot lock,
leading
to interesting effect when using it from libiscsi, when typing import
libiscsi in python as normal user, my entire python interpreter exited, not
good.
The attached patch instead returns an error code, and fixes all callers to
check this.
Hi!
Let me comment on this:
if (errno != EEXIST) {
+ log_error(Maybe you are not root?);
In case of an error, don't try to spewculate, but give details (i.e. errno)
instead. Would something be wrong with using strerror(errno) here?
We do that in the next line logged, also note that this message was already
there, only the order of the 2 messages is being changed, so that libiscsi
(which saves the last log_error message) gives a reasonable error when this
happens.
Also, for a library when returning error codes, there is no need to print
messages
(unless there is a generic error reporting framework that can be customized).
This is not being printed, instead the last log_error message is kept in a
buffer for the application to retrieve when it wants something more verbose
then just strerror to show to the user.
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: fix iBFT firmware reading with newer kernels
Ulrich Windl wrote:
On 28 Jan 2009 at 22:49, Hans de Goede wrote:
Hi,
While testing I noticed that iscsiadmin -m fw does not work properly on
newer
(rawhide atleast) kernels, the attached patch (already applied to the Fedora
devel packages) fixes this.
Hi!
I have almost no ideas on the implementation, but please see my comments:
diff -up open-iscsi-2.0-870.1/utils/fwparam_ibft/fwparam_ibft_sysfs.c~
open-iscsi-2.0-870.1/utils/fwparam_ibft/fwparam_ibft_sysfs.c
--- open-iscsi-2.0-870.1/utils/fwparam_ibft/fwparam_ibft_sysfs.c~
2009-01-28 22:09:21.0 +0100
+++ open-iscsi-2.0-870.1/utils/fwparam_ibft/fwparam_ibft_sysfs.c
2009-01-28 22:10:29.0 +0100
@@ -186,6 +186,40 @@ static int get_iface_from_device(const c
break;
}
+ closedir(dirfd);
+
+ if (rc != ENODEV)
+ return rc;
+
+ /* If not found try again with newer kernel networkdev sysfs layout */
+ strncat(dev_dir, /net, FILENAMESZ);
What do you thing you gain by using strncat(dev_dir, /net, FILENAMESZ);
instead of strcat(dev_dir, /net);?
Nothing I copied this from the existing code, assuming (wrong) the existing
code was ok, which it is not. The idea is to protect against buffer overflows,
but using strncat this way will not accomplish this.
+
+ if (!file_exist(dev_dir))
+ return rc;
+
+ dirfd = opendir(dev_dir);
+ if (!dirfd)
+ return errno;
+
+ while ((dent = readdir(dirfd))) {
+ if (!strcmp(dent-d_name, .) || !strcmp(dent-d_name, ..))
+ continue;
+
+ /* Take the first regular directory entry */
+ if (strlen(dent-d_name) (sizeof(context-iface) - 1)) {
+ rc = EINVAL;
+ printf(Net device %s too bug for iface buffer.\n,
+dent-d_name);
What is too bug? too long?
too big, another piece I copied and pasted.
Regards,
Hams
--~--~-~--~~~---~--~~
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: fix iBFT firmware reading with newer kernels
Bart Van Assche wrote: On Thu, Jan 29, 2009 at 12:34 AM, Mike Christie micha...@cs.wisc.edu wrote: strncat(dev_dir, /, FILENAMESZ); strncat(dev_dir, dent-d_name, FILENAMESZ); I assume the third argument should have been FILENAMESZ - strlen(dev_dir) ? Yes something like that, maybe just do a snprintf with all components in one go instead of all this strcat pain. 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: fix iBFT firmware reading with newer kernels
Ulrich Windl wrote: On 28 Jan 2009 at 17:34, Mike Christie wrote: strncat(dev_dir, dent-d_name, FILENAMESZ); Hi, once again: The third argument of strncpy() counts the bytes to be added, not the bytes that are already there, so the code may not do what some of you seem to expect! Ack, Copy and paste error, may I suggest to instead just do one snprintf call to build the entire path in one go? That removes all this pain. 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: fix iBFT firmware reading with newer kernels
Mike Christie wrote: Mike Christie wrote: Hans de Goede wrote: Yes it does the same thing, is this from the open-iscsi VCS ? and where do I find that ? Ok, I will merge up your code instead with the fixes in the thread. Thanks! Here is the rediffed patch. I used strncat here: + strncat(dev_dir, /net, FILENAMESZ - strlen(dev_dir)) Looks good, Thanks, 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 -~--~~~~--~~--~--~---
PATCH: fix iBFT firmware reading with newer kernels
Hi,
While testing I noticed that iscsiadmin -m fw does not work properly on newer
(rawhide atleast) kernels, the attached patch (already applied to the Fedora
devel packages) fixes this.
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
-~--~~~~--~~--~--~---
diff -up open-iscsi-2.0-870.1/utils/fwparam_ibft/fwparam_ibft_sysfs.c~
open-iscsi-2.0-870.1/utils/fwparam_ibft/fwparam_ibft_sysfs.c
--- open-iscsi-2.0-870.1/utils/fwparam_ibft/fwparam_ibft_sysfs.c~
2009-01-28 22:09:21.0 +0100
+++ open-iscsi-2.0-870.1/utils/fwparam_ibft/fwparam_ibft_sysfs.c
2009-01-28 22:10:29.0 +0100
@@ -186,6 +186,40 @@ static int get_iface_from_device(const c
break;
}
+ closedir(dirfd);
+
+ if (rc != ENODEV)
+ return rc;
+
+ /* If not found try again with newer kernel networkdev sysfs layout */
+ strncat(dev_dir, /net, FILENAMESZ);
+
+ if (!file_exist(dev_dir))
+ return rc;
+
+ dirfd = opendir(dev_dir);
+ if (!dirfd)
+ return errno;
+
+ while ((dent = readdir(dirfd))) {
+ if (!strcmp(dent-d_name, .) || !strcmp(dent-d_name, ..))
+ continue;
+
+ /* Take the first regular directory entry */
+ if (strlen(dent-d_name) (sizeof(context-iface) - 1)) {
+ rc = EINVAL;
+ printf(Net device %s too bug for iface buffer.\n,
+ dent-d_name);
+ break;
+ }
+
+ strcpy(context-iface, dent-d_name);
+ rc = 0;
+ break;
+ }
+
+ closedir(dirfd);
+
return rc;
}
PATCH: do not use exit()
Hi All,
While testing I noticed that idbm_lock() uses exit when it cannot lock, leading
to interesting effect when using it from libiscsi, when typing import
libiscsi in python as normal user, my entire python interpreter exited, not
good.
The attached patch instead returns an error code, and fixes all callers to
check this.
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
-~--~~~~--~~--~--~---
--- open-iscsi-2.0-870.1/usr/idbm.c~2009-01-28 13:23:47.0 +0100
+++ open-iscsi-2.0-870.1/usr/idbm.c 2009-01-28 13:25:06.0 +0100
@@ -843,7 +843,7 @@ int idbm_lock(void)
if (access(LOCK_DIR, F_OK) != 0) {
if (mkdir(LOCK_DIR, 0660) != 0) {
log_error(Could not open %s. Exiting\n, LOCK_DIR);
- exit(-1);
+ return errno;
}
}
@@ -857,10 +857,10 @@ int idbm_lock(void)
break;
if (errno != EEXIST) {
+ log_error(Maybe you are not root?);
log_error(Could not lock discovery DB: %s: %s,
LOCK_WRITE_FILE, strerror(errno));
- log_error(Maybe you are not root?);
- exit(-1);
+ return errno;
} else if (i == 0)
log_debug(2, Waiting for discovery DB lock);
@@ -915,7 +915,10 @@ static int __idbm_rec_read(node_rec_t *o
if (!info)
return ENOMEM;
- idbm_lock();
+ rc = idbm_lock();
+ if (rc)
+ goto free_info;
+
f = fopen(conf, r);
if (!f) {
log_debug(5, Could not open %s err %d\n, conf, errno);
@@ -931,6 +934,7 @@ static int __idbm_rec_read(node_rec_t *o
unlock:
idbm_unlock();
+free_info:
free(info);
return rc;
}
@@ -1386,14 +1390,18 @@ idbm_discovery_read(discovery_rec_t *out
return ENOMEM;
portal = malloc(PATH_MAX);
- if (!portal)
+ if (!portal) {
+ rc = ENOMEM;
goto free_info;
+ }
snprintf(portal, PATH_MAX, %s/%s,%d, ST_CONFIG_DIR,
addr, port);
log_debug(5, Looking for config file %s\n, portal);
- idbm_lock();
+ rc = idbm_lock();
+ if (rc)
+ goto free_info;
f = idbm_open_rec_r(portal, ST_CONFIG_NAME);
if (!f) {
@@ -1494,7 +1502,9 @@ static int idbm_rec_write(node_rec_t *re
rec-name, rec-conn[0].address, rec-conn[0].port);
log_debug(5, Looking for config file %s, portal);
- idbm_lock();
+ rc = idbm_lock();
+ if (rc)
+ goto free_portal;
rc = stat(portal, statb);
if (rc) {
@@ -1579,13 +1589,16 @@ idbm_discovery_write(discovery_rec_t *re
return ENOMEM;
}
- idbm_lock();
+ rc = idbm_lock();
+ if (rc)
+ goto free_portal;
+
snprintf(portal, PATH_MAX, %s, ST_CONFIG_DIR);
if (access(portal, F_OK) != 0) {
if (mkdir(portal, 0660) != 0) {
log_error(Could not make %s\n, portal);
rc = errno;
- goto free_portal;
+ goto unlock;
}
}
@@ -1596,13 +1609,14 @@ idbm_discovery_write(discovery_rec_t *re
if (!f) {
log_error(Could not open %s err %d\n, portal, errno);
rc = errno;
- goto free_portal;
+ goto unlock;
}
idbm_print(IDBM_PRINT_TYPE_DISCOVERY, rec, 1, f);
fclose(f);
-free_portal:
+unlock:
idbm_unlock();
+free_portal:
free(portal);
return rc;
}
@@ -1722,7 +1736,10 @@ int idbm_add_node(node_rec_t *newrec, di
log_debug(7, node addition making link from %s to %s, node_portal,
disc_portal);
- idbm_lock();
+ rc = idbm_lock();
+ if (rc)
+ goto free_portal;
+
if (symlink(node_portal, disc_portal)) {
if (errno == EEXIST)
log_debug(7, link from %s to %s exists, node_portal,
@@ -2009,7 +2026,10 @@ static int idbm_remove_disc_to_node_link
if (rc)
goto done;
- idbm_lock();
+ rc = idbm_lock();
+ if (rc)
+ goto done;
+
if (!stat(portal, statb)) {
if (unlink(portal)) {
log_error(Could not remove link %s err %d\n,
@@ -2046,7 +2066,10 @@ int
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
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
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
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 -~--~~~~--~~--~--~---
