On Thu, 2012-08-23 at 11:28 +0800, Benjamin Wang wrote:
> Hi Nicholas:
> Here you are. Thanks for your comment. ^^
>
Hi Benjamin,
So typically when posting patches for review, you'll want to inline the
patch into the emainl so that it's easier for reviewers to comment on..
Inlining your patch below so that I can give feedback. (see below)
Also, the filename in your patch does not reflect the full pathname
(eg: drivers/target/iscsi/iscsi_target_login.c). Please fix that up
too.
--- iscsi_target_login_old.c 2012-08-23 10:44:37.000000000 +0800
+++ iscsi_target_login.c 2012-08-23 10:54:20.000000000 +0800
@@ -226,7 +226,8 @@
{
struct iscsi_session *sess = NULL;
struct iscsi_login_req *pdu = (struct iscsi_login_req *)buf;
-
+ int ret;
+
sess = kzalloc(sizeof(struct iscsi_session), GFP_KERNEL);
if (!sess) {
iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
@@ -262,8 +263,15 @@
return -ENOMEM;
}
spin_lock(&sess_idr_lock);
- idr_get_new(&sess_idr, NULL, &sess->session_index);
+ ret = idr_get_new(&sess_idr, NULL, &sess->session_index);
spin_unlock(&sess_idr_lock);
+ // Benjamin Wang 20120823:In case the sess_idr space is completely exhausted.
<NAB> No need to add a comment for something like this..
+ if (ret == -EAGAIN) {
<NAB> What if idr_get_new ever returns a different non zero ret..? It will end
up silently failing here. BAD. Please change this to check for all
(ret < 0) failure cases to future-proof this conditional.
+ iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
+ ISCSI_LOGIN_STATUS_NO_RESOURCES);
+ pr_err("idr_get_new() for sess_idr failed\n");
+ return -ENOMEM;
+ }
sess->creation_time = get_jiffies_64();
spin_lock_init(&sess->session_stats_lock);
--
Go ahead and include these three changes into a -v2 patch, and post it to
target-devel@xxxxxxxxxxxxxxx with a short title+descriptor along with your
'Signed-off-by: ' ownership of the patch.
Also, if your planning to send more kernel patches, you might want to
consider learning how to use git with a 'git format-patch' ->
'git-send-email' workflow for submitting patches. It's much more
efficent for generating -> sending patching compared gnu diff against
multiple kernel trees. :)
Thanks!
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
