Re: [Patch 1/2] iscsiadm: login_portal() misses outputting logs for iscsid_req_by_rec()
Yangkook Kim wrote: Thanks for your patch. I tested your patch and it worked fine. So, next you will upload this patch to the git tree and the patch will become the part of source code in the next release of open-iscsi. Is my understanding correct? Yeah. I merged it and uploaded it to the git tree. The commit id is fb4f2d3072bee96606d01e3535c100dc99b8d331. It can take a couple of hours to show up (the disks have to get synced up or something), so you should see it shortly. When I make the next release it will be included. I am asking this question because I just want to know the normal development process of this and other linux project. No problem. 2009/11/24, Mike Christie micha...@cs.wisc.edu: Mike Christie wrote: Yangkook Kim wrote: Hi, Mike. Thank you for your patch. I do not want to add a login log message to the iscsid_req_* functions because they are generic and could be used for any operation. Yes, that's perfectly right idea. That should be bettet than my patch. I tried your patch, but that still does not output login-success message when calling iscsid_req_by_rec. It seems that log_login_msg() would not be called in either login_portal() or iscsid_logout_reqs_wait() when iscsid_req_by_rec returns success. I probably missed something. I will look at it tomorrow again. Nope. You are right. Nice catch. I messed up. I was only concentrating on the error paths. I will fix up my patch and resend. Thanks. Here is a corrected patch. -- You received this message because you are subscribed to the Google Groups open-iscsi group. To post to this group, send email to open-is...@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?hl=. -- You received this message because you are subscribed to the Google Groups open-iscsi group. To post to this group, send email to open-is...@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?hl=en. -- You received this message because you are subscribed to the Google Groups open-iscsi group. To post to this group, send email to open-is...@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?hl=en.
Re: [Patch 1/2] iscsiadm: login_portal() misses outputting logs for iscsid_req_by_rec()
Thanks for your patch. I tested your patch and it worked fine. So, next you will upload this patch to the git tree and the patch will become the part of source code in the next release of open-iscsi. Is my understanding correct? I am asking this question because I just want to know the normal development process of this and other linux project. 2009/11/24, Mike Christie micha...@cs.wisc.edu: Mike Christie wrote: Yangkook Kim wrote: Hi, Mike. Thank you for your patch. I do not want to add a login log message to the iscsid_req_* functions because they are generic and could be used for any operation. Yes, that's perfectly right idea. That should be bettet than my patch. I tried your patch, but that still does not output login-success message when calling iscsid_req_by_rec. It seems that log_login_msg() would not be called in either login_portal() or iscsid_logout_reqs_wait() when iscsid_req_by_rec returns success. I probably missed something. I will look at it tomorrow again. Nope. You are right. Nice catch. I messed up. I was only concentrating on the error paths. I will fix up my patch and resend. Thanks. Here is a corrected patch. -- You received this message because you are subscribed to the Google Groups open-iscsi group. To post to this group, send email to open-is...@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?hl=. -- You received this message because you are subscribed to the Google Groups open-iscsi group. To post to this group, send email to open-is...@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?hl=en.
Re: [Patch 1/2] iscsiadm: login_portal() misses outputting logs for iscsid_req_by_rec()
Mike Christie wrote:
Yangkook Kim wrote:
Hi, Mike. Thank you for your patch.
I do not want to add a login log message to the iscsid_req_* functions
because they are generic and could be used for any operation.
Yes, that's perfectly right idea. That should be bettet than my patch.
I tried your patch, but that still does not output login-success
message when calling
iscsid_req_by_rec.
It seems that log_login_msg() would not be called in either login_portal() or
iscsid_logout_reqs_wait() when iscsid_req_by_rec returns success.
I probably missed something. I will look at it tomorrow again.
Nope. You are right. Nice catch. I messed up. I was only concentrating
on the error paths. I will fix up my patch and resend. Thanks.
Here is a corrected patch.
--
You received this message because you are subscribed to the Google Groups
open-iscsi group.
To post to this group, send email to open-is...@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?hl=.
diff --git a/usr/iscsiadm.c b/usr/iscsiadm.c
index d4f7925..dbefa39 100644
--- a/usr/iscsiadm.c
+++ b/usr/iscsiadm.c
@@ -277,6 +277,21 @@ match_startup_mode(node_rec_t *rec, char *mode)
return -1;
}
+static void log_login_msg(struct node_rec *rec, int rc)
+{
+ if (rc) {
+ log_error(Could not login to [iface: %s, target: %s,
+ portal: %s,%d]., rec-iface.name,
+ rec-name, rec-conn[0].address,
+ rec-conn[0].port);
+ iscsid_handle_error(rc);
+ } else
+ printf(Login to [iface: %s, target: %s, portal:
+ %s,%d] successful.\n, rec-iface.name,
+ rec-name, rec-conn[0].address,
+ rec-conn[0].port);
+}
+
struct iscsid_async_req {
struct list_head list;
void *data;
@@ -294,23 +309,28 @@ static int iscsid_login_reqs_wait(struct list_head *list)
rec = curr-data;
err = iscsid_req_wait(MGMT_IPC_SESSION_LOGIN, curr-fd);
- if (err) {
- log_error(Could not login to [iface: %s, target: %s,
- portal: %s,%d]: , rec-iface.name,
- rec-name, rec-conn[0].address,
- rec-conn[0].port);
- iscsid_handle_error(err);
- ret = err;
- } else
- printf(Login to [iface: %s, target: %s, portal:
- %s,%d]: successful\n, rec-iface.name,
- rec-name, rec-conn[0].address,
- rec-conn[0].port);
+ log_login_msg(rec, err);
list_del(curr-list);
free(curr);
}
return ret;
}
+
+static void log_logout_msg(struct session_info *info, int rc)
+{
+ if (rc) {
+ log_error(Could not logout of [sid: %d, target: %s,
+ portal: %s,%d]., info-sid,
+ info-targetname,
+ info-persistent_address, info-port);
+ iscsid_handle_error(rc);
+ } else
+ printf(Logout of [sid: %d, target: %s,
+ portal: %s,%d] successful.\n,
+ info-sid, info-targetname,
+ info-persistent_address, info-port);
+}
+
static int iscsid_logout_reqs_wait(struct list_head *list)
{
struct iscsid_async_req *tmp, *curr;
@@ -322,18 +342,9 @@ static int iscsid_logout_reqs_wait(struct list_head *list)
info = curr-data;
err = iscsid_req_wait(MGMT_IPC_SESSION_LOGOUT, curr-fd);
- if (err) {
- log_error(Could not logout of [sid: %d, target: %s,
- portal: %s,%d]: , info-sid,
- info-targetname,
- info-persistent_address, info-port);
- iscsid_handle_error(err);
+ log_logout_msg(info, err);
+ if (err)
ret = err;
- } else
- printf(Logout of [sid: %d, target: %s,
- portal: %s,%d]: successful\n,
- info-sid, info-targetname,
- info-persistent_address, info-port);
list_del(curr-list);
free(curr);
}
@@ -393,26 +404,25 @@ static int __logout_portal(struct session_info *info, struct list_head *list)
}
/* we raced with another app or instance of iscsiadm */
- switch (rc) {
- case MGMT_IPC_ERR_NOT_FOUND:
- rc = 0;
- break;
- case MGMT_IPC_OK:
- if (async_req) {
- list_add_tail(async_req-list, list);
- async_req-fd = fd;
- async_req-data = info;
- }
- return 0;
- default:
- iscsid_handle_error(rc);
- rc = EIO;
- break;
+ if (rc) {
+ log_logout_msg(info, rc);
+ if (async_req)
+ free(async_req);
+
+ if (rc == MGMT_IPC_ERR_NOT_FOUND)
+ return 0;
+
+ return EIO;
}
- if (async_req)
- free(async_req);
- return rc;
+ if (async_req) {
+ list_add_tail(async_req-list, list);
+ async_req-fd = fd;
+ async_req-data = info;
+ } else
+ log_logout_msg(info, rc);
+
+ return 0;
}
static int
@@ -523,8 +533,8 @@ logout_portal(void *data, struct list_head *list, struct session_info *info)
/* we do not support this yet */
if (t-caps CAP_FW_DB) {
- log_error(Could not logout session [sid: %d,
- target: %s, portal: %s,%d], info-sid,
+ log_error(Could not logout session of [sid: %d,
+ target: %s, portal: %s,%d]., info-sid,
info-targetname, info-persistent_address,
info-port);
log_error(Logout not supported for driver: %s., t-name);
@@ -602,15
Re: [Patch 1/2] iscsiadm: login_portal() misses outputting logs for iscsid_req_by_rec()
Yangkook Kim wrote: Hi, Mike. Thank you for your patch. I do not want to add a login log message to the iscsid_req_* functions because they are generic and could be used for any operation. Yes, that's perfectly right idea. That should be bettet than my patch. I tried your patch, but that still does not output login-success message when calling iscsid_req_by_rec. It seems that log_login_msg() would not be called in either login_portal() or iscsid_logout_reqs_wait() when iscsid_req_by_rec returns success. I probably missed something. I will look at it tomorrow again. Nope. You are right. Nice catch. I messed up. I was only concentrating on the error paths. I will fix up my patch and resend. Thanks. -- You received this message because you are subscribed to the Google Groups open-iscsi group. To post to this group, send email to open-is...@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?hl=.
Re: [Patch 1/2] iscsiadm: login_portal() misses outputting logs for iscsid_req_by_rec()
Are you hitting the non async code path? Did you hit the
iscsid_req_by_rec call? How many targets or portals were you logging into?
No, I never hit iscsid_req_by_rec call. I was just reading the codes
of iscsiadm and thought that there is less consideration when calling
iscsid_req_by_rec() than iscsid_req_by_rec_async().
If iscsid_req_by_rec fails, then won't we log an error here?
Sorry, my point was very unclear for you and my patch was also not good.
I basically have two points that I want to improve.
No1, checking the return value of iscisd_request() when calling
iscsid_req_by_rec().
No2, outputting login success message even when calling iscsid_req_by_rec().
In the origicanl code of login_portal() below,
if (async_req)
rc = iscsid_req_by_rec_async(MGMT_IPC_SESSION_LOGIN,
rec, fd);
else
rc = iscsid_req_by_rec(MGMT_IPC_SESSION_LOGIN, rec);
/* we raced with another app or instance of iscsiadm */
if (rc == MGMT_IPC_ERR_EXISTS) {
if (async_req)
free(async_req);
return 0;
} else if (rc) {
iscsid_handle_error(rc);
if (async_req)
free(async_req);
return ENOTCONN;
}
the return value of iscsid_request() is checked when calling
iscsid_req_by_rec_async().
However, the same return value is not checked when calling iscsid_req_by_rec().
What you check here with rc = iscsid_req_by_rec() is not the return value of
iscsid_request(), but that of iscsid_req_wait() as you can see in the
codes below.
int iscsid_req_by_rec(iscsiadm_cmd_e cmd, node_rec_t *rec)
{
int err, fd;
err = iscsid_req_by_rec_async(cmd, rec, fd);
if (err)
return err;
return iscsid_req_wait(cmd, fd);
}
So, I check the return value of iscsid_request() inside iscsid_req_by_rec().
(I actually did not put the codes to achive this in the second patch of util.c.
and maybe that made you confused...
I will send a new patch of util.c that includes below codes.)
The code below will achive my point No1.
int iscsid_req_by_rec(iscsiadm_cmd_e cmd, node_rec_t *rec)
{
int err, fd;
err = iscsid_req_by_rec_async(cmd, rec, fd);
if (err == MGMT_IPC_ERR_EXISTS) {
return 0;
} else if (err) {
iscsid_handle_error(err);
return ENOTCONN;
}
I also check the return value of iscsid_req_wait() inside
iscsid_req_by_rec() and
outputting logs of login status. This will achive my point No2.
err = iscsid_req_wait(cmd, fd);
if (err) {
log_error(Could not login to [iface: %s, target: %s,
portal: %s,%d]: , rec-iface.name,
rec-name, rec-conn[0].address,
rec-conn[0].port);
} else
printf(Login to [iface: %s, target: %s, portal:
%s,%d]: successful\n, rec-iface.name,
rec-name, rec-conn[0].address,
rec-conn[0].port);
return err;
}
Since we checked the return value of iscsid_request() inside
iscsid_req_by_rec(),
what we need to check is only the return value of iscsid_req_wait() of
iscsid_req_by_rec().
So, patched login_portal() should be something like this now.
if (async_req) {
rc = iscsid_req_by_rec_async(MGMT_IPC_SESSION_LOGIN,
rec, fd);
if (rc == MGMT_IPC_ERR_EXISTS) {
if (async_req)
free(async_req);
return 0;
} else if (rc) {
iscsid_handle_error(rc);
if (async_req)
free(async_req);
return ENOTCONN;
}
list_add_tail(async_req-list, list);
async_req-fd = fd;
async_req-data = rec;
return 0;
} else {
rc = iscsid_req_by_rec(MGMT_IPC_SESSION_LOGIN, rec);
if (rc) {
iscsid_handle_error(rc);
return ENOTCONN;
} else
return rc;
}
}
I know that iscsid_req_by_rec() is almost never called in the reality.
But, I just thought the codes looks little better if we take care of
iscsid_req_by_rec() more.
This patch unnecessary? Unclear? Give me your frank opinion, Mike.
Kim.
--~--~-~--~~~---~--~~
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
Re: [Patch 1/2] iscsiadm: login_portal() misses outputting logs for iscsid_req_by_rec()
Yangkook Kim wrote:
int iscsid_req_by_rec(iscsiadm_cmd_e cmd, node_rec_t *rec)
{
int err, fd;
err = iscsid_req_by_rec_async(cmd, rec, fd);
if (err)
return err;
If the iscsid_request call in iscsid_req_by_rec_async failed we would
return the err value here. Then in iscsiadm.c we do:
{
..
rc = iscsid_req_by_rec(MGMT_IPC_SESSION_LOGIN, rec);
In the line above we set rc to the err value returned when
iscsid_req_by_rec called iscsid_req_by_rec_async and failed.
/* we raced with another app or instance of iscsiadm */
if (rc == MGMT_IPC_ERR_EXISTS) {
if (async_req)
free(async_req);
return 0;
} else if (rc) {
And then right here we see rc is a error.
iscsid_handle_error(rc);
Finally here we print out the initiator reported error XYZ here.
if (async_req)
free(async_req);
return ENOTCONN;
}
} else if (rc) {
iscsid_handle_error(rc);
if (async_req)
free(async_req);
return ENOTCONN;
.
}
If the iscsid_req_by_rec call to iscsid_req_by_rec_async is successful
then we will return the error value of that below. And then again we
would set rc to that return value and evaluate like it above in the
iscsiadm.c snippet.
return iscsid_req_wait(cmd, fd);
}
For your #2 issue you are right that is messed up.
--~--~-~--~~~---~--~~
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 1/2] iscsiadm: login_portal() misses outputting logs for iscsid_req_by_rec()
Yangkook Kim wrote:
This patch tries to fix the lack of outputting logs when
iscsid_req_by_rec is used.
When using iscsid_req_by_rec, the current codes does not tell you whether you
are successfully logged in or hitting some errors because you cannot
get into the
loop of list_for_each_entry_safe() in iscsid_login_reqs_wait(). I
modify some codes
so that iscsid_req_by_rec will output logs.
Thanks for the patch! Some questions.
Are you hitting the non async code path? Did you hit the
iscsid_req_by_rec call? How many targets or portals were you logging into?
Question about the patch below.
--- a/usr/iscsiadm.c2009-11-12 06:22:10.0 +0900
+++ b/usr/iscsiadm.c2009-11-12 06:23:01.0 +0900
@@ -597,29 +597,31 @@
INIT_LIST_HEAD(async_req-list);
}
- if (async_req)
- rc = iscsid_req_by_rec_async(MGMT_IPC_SESSION_LOGIN,
-rec, fd);
- else
- rc = iscsid_req_by_rec(MGMT_IPC_SESSION_LOGIN, rec);
- /* we raced with another app or instance of iscsiadm */
- if (rc == MGMT_IPC_ERR_EXISTS) {
- if (async_req)
- free(async_req);
- return 0;
- } else if (rc) {
- iscsid_handle_error(rc);
If iscsid_req_by_rec fails, then won't we log an error here?
- if (async_req)
- free(async_req);
- return ENOTCONN;
- }
-
if (async_req) {
+rc = iscsid_req_by_rec_async(MGMT_IPC_SESSION_LOGIN,
+rec, fd);
+ if (rc == MGMT_IPC_ERR_EXISTS) {
+ if (async_req)
+ free(async_req);
+ return 0;
+ } else if (rc) {
+ iscsid_handle_error(rc);
+ if (async_req)
+ free(async_req);
+ return ENOTCONN;
+ }
list_add_tail(async_req-list, list);
async_req-fd = fd;
async_req-data = rec;
+ return 0;
+ } else {
+ rc = iscsid_req_by_rec(MGMT_IPC_SESSION_LOGIN, rec);
+ if (rc) {
+ iscsid_handle_error(rc);
+ return ENOTCONN;
+ } else
+ return rc;
Does this do almost the same thing? It looks like the only change is
that if we get MGMT_IPC_ERR_EXISTS then iscsid_req_by_rec will now print
out an error message. Did I miss something?
Your patch actually might be clearer though.
--~--~-~--~~~---~--~~
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 1/2] iscsiadm: login_portal() misses outputting logs for iscsid_req_by_rec()
This patch tries to fix the lack of outputting logs when
iscsid_req_by_rec is used.
When using iscsid_req_by_rec, the current codes does not tell you whether you
are successfully logged in or hitting some errors because you cannot
get into the
loop of list_for_each_entry_safe() in iscsid_login_reqs_wait(). I
modify some codes
so that iscsid_req_by_rec will output logs.
Please give me a feedback if any.
-- Yangkook Kim
Signed-off-by: Yangkook Kim yangkook...@gmail.com
---
--- a/usr/iscsiadm.c2009-11-12 06:22:10.0 +0900
+++ b/usr/iscsiadm.c2009-11-12 06:23:01.0 +0900
@@ -597,29 +597,31 @@
INIT_LIST_HEAD(async_req-list);
}
- if (async_req)
- rc = iscsid_req_by_rec_async(MGMT_IPC_SESSION_LOGIN,
-rec, fd);
- else
- rc = iscsid_req_by_rec(MGMT_IPC_SESSION_LOGIN, rec);
- /* we raced with another app or instance of iscsiadm */
- if (rc == MGMT_IPC_ERR_EXISTS) {
- if (async_req)
- free(async_req);
- return 0;
- } else if (rc) {
- iscsid_handle_error(rc);
- if (async_req)
- free(async_req);
- return ENOTCONN;
- }
-
if (async_req) {
+rc = iscsid_req_by_rec_async(MGMT_IPC_SESSION_LOGIN,
+rec, fd);
+ if (rc == MGMT_IPC_ERR_EXISTS) {
+ if (async_req)
+ free(async_req);
+ return 0;
+ } else if (rc) {
+ iscsid_handle_error(rc);
+ if (async_req)
+ free(async_req);
+ return ENOTCONN;
+ }
list_add_tail(async_req-list, list);
async_req-fd = fd;
async_req-data = rec;
+ return 0;
+ } else {
+ rc = iscsid_req_by_rec(MGMT_IPC_SESSION_LOGIN, rec);
+ if (rc) {
+ iscsid_handle_error(rc);
+ return ENOTCONN;
+ } else
+ return rc;
}
- return 0;
}
--~--~-~--~~~---~--~~
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
-~--~~~~--~~--~--~---
--- a/usr/iscsiadm.c 2009-11-12 06:22:10.0 +0900
+++ b/usr/iscsiadm.c 2009-11-12 06:23:01.0 +0900
@@ -597,29 +597,31 @@
INIT_LIST_HEAD(async_req-list);
}
- if (async_req)
- rc = iscsid_req_by_rec_async(MGMT_IPC_SESSION_LOGIN,
- rec, fd);
- else
- rc = iscsid_req_by_rec(MGMT_IPC_SESSION_LOGIN, rec);
- /* we raced with another app or instance of iscsiadm */
- if (rc == MGMT_IPC_ERR_EXISTS) {
- if (async_req)
- free(async_req);
- return 0;
- } else if (rc) {
- iscsid_handle_error(rc);
- if (async_req)
- free(async_req);
- return ENOTCONN;
- }
-
if (async_req) {
+rc = iscsid_req_by_rec_async(MGMT_IPC_SESSION_LOGIN,
+ rec, fd);
+ if (rc == MGMT_IPC_ERR_EXISTS) {
+ if (async_req)
+free(async_req);
+ return 0;
+ } else if (rc) {
+ iscsid_handle_error(rc);
+ if (async_req)
+free(async_req);
+ return ENOTCONN;
+ }
list_add_tail(async_req-list, list);
async_req-fd = fd;
async_req-data = rec;
+ return 0;
+ } else {
+ rc = iscsid_req_by_rec(MGMT_IPC_SESSION_LOGIN, rec);
+ if (rc) {
+ iscsid_handle_error(rc);
+ return ENOTCONN;
+ } else
+ return rc;
}
- return 0;
}
static int __login_portals(void *data, int *nr_found,
