On 2019-10-23 20:37, Honggang LI wrote:
> On Wed, Oct 23, 2019 at 01:41:06PM -0700, Bart Van Assche wrote:
>> + mutex_lock(&sport->port_gid_id.mutex);
>> + list_for_each_entry(stpg, &sport->port_gid_id.tpg_list, entry) {
>> + if (!IS_ERR_OR_NULL(ch->sess))
> ^^^^^^^
>> + break;
>> + ch->sess = target_setup_session(&stpg->tpg, tag_num,
> ^^^^^^^^^^^^^^^^^^^^
>> tag_size, TARGET_PROT_NORMAL, i_port_id,
>> ch, NULL);
>> - /* Retry without leading "0x" */
>> - if (sport->port_gid_id.tpg.se_tpg_wwn && IS_ERR_OR_NULL(ch->sess))
>> - ch->sess = target_setup_session(&sport->port_gid_id.tpg, tag_num,
>> + if (!IS_ERR_OR_NULL(ch->sess))
> ^
>> + break;
>
> I'm confused about this 'if' statement. In case you repeated the
> validation as previous 'if' statement, it is redundance.
>
> In case you check the return of the first target_setup_session,
> it seems wrong, we only need to retry in case first target_setup_session
> was failed. But you break out, and skip the second target_setup_session.
>
>> + /* Retry without leading "0x" */
>> + ch->sess = target_setup_session(&stpg->tpg, tag_num,
> ^^^^^^^^^^^^^^^^^^^^
>> tag_size, TARGET_PROT_NORMAL,
>> i_port_id + 2, ch, NULL);
Hi Honggang,
The purpose of this code is to keep iterating until a session has been
created. The "if (!IS_ERR_OR_NULL(ch->sess)) break" code prevents
further target_setup_session() calls after a session has been created
successfully.
Bart.
