[Patch 1/2] iscsiadm: login_portal() misses outputting logs for iscsid_req_by_rec()

[Yangkook Kim]

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 <yangk...@gmail.com>

---

--- a/usr/iscsiadm.c 2009-11-12 06:22:10.000000000 +0900
+++ b/usr/iscsiadm.c 2009-11-12 06:23:01.000000000 +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;
}

[Mike Christie]

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.c 2009-11-12 06:22:10.000000000 +0900
> +++ b/usr/iscsiadm.c 2009-11-12 06:23:01.000000000 +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?

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.

[Yangkook Kim]

>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.

[Mike Christie]

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.

[Mike Christie]

Yangkook Kim wrote:
>
> No2, outputting login success message even when calling iscsid_req_by_rec().
>

To fix this, how about the attached patch made over the open-iscsi git tree?

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. This patch
moves the log message you were adding to iscsiadm.c.

I also fixed up the logout side since that had the same issue.

[Yangkook Kim]

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.
So, don't be bothered with my question.

Thanks.

Kim

2009/11/17, Mike Christie <mich...@cs.wisc.edu>:

[Mike Christie]

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.

[Mike Christie]

Here is a corrected patch.

[Yangkook Kim]

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 <mich...@cs.wisc.edu>:

> --
>
> You received this message because you are subscribed to the Google Groups
> "open-iscsi" group.
> To post to this group, send email to open-...@googlegroups.com.
> To unsubscribe from this group, send email to
> open-iscsi+...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/open-iscsi?hl=.
>
>
>

[Mike Christie]

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.

> For more options, visit this group at http://groups.google.com/group/open-iscsi?hl=en.
>
>