>Yep, now I use list_empty to check for list emptiness (it compares
>&c_hlist with c_hlist->head internally). Attaching the patch below.
>Are there any reasons for checking opcode then?
hi Anton Kovalenko
we check task opcode in iscsi_free_task, if task opcode is
ISCSI_OP_SCSI_CMD, we do list list_del(&task->c_hlist); because we
suppose iscsi_free_task is a generic interface to release task，thease
task include ISCSI_OP_SCSI_CMD，ISCSI_OP_NOOP_OUT, ISCSI_OP_SCSI_TMFUNC
and so on， but only ISCSI_OP_SCSI_CMD task is added to
adsession->cmd_list, so， in iscsi_free_task， we check
ISCSI_OP_SCSI_CMD opcode and list_del, This logic is reasonable，
But carefully consider your modify，Logically it is not easy to
understand，only fix current bug(Currently, I'm not sure whether still
potential problems such modifications ),
Iwe carefully consider the logical，why do below task->c_hlist.prev
task->c_hlist.prev check in iscsi_free_task
+ if (task->c_hlist.prev && task->c_hlist.next
+ && !list_empty(&task->c_hlist)) {
+ list_del(&task->c_hlist);
+ }
+
Similarly，besides task->c_hlist， task have task->c_siblings, Why not
do below task->c_siblings relate check?
+ if (task->c_siblings.prev && task->c_siblings.next
+ && !list_empty(&task->c_siblings)) {
+ list_del(&task->c_siblings);
+ }
+
Similarly，besides task->c_hlist， task have task->c_list, Why not do
below task->c_list relate check?
+ if (task->c_list.prev && task->c_list.next
+ && !list_empty(&task->c_list)) {
+ list_del(&task->c_list);
+ }
+
Please think about our proposal！thank you
2016-07-26 16:02 GMT+08:00 Anton Kovalenko <anton.kovalenko@xxxxxxxxxxx>:
>
> Hi,
>
> fx chen <chenfx2014@xxxxxxxxx> writes:
>
>> we also consiger you idea，in iscsi_free_task, do task unlinking from
>> session->cmd_list(list_del(&task->c_hlist) )，but we must know，such as
>> ISCSI_OP_NOOP_OUT, ISCSI_OP_SCSI_TMFUNC, ISCSI_OP_LOGOUT type task,
>> they don‘t add to session->cmd_list, To solve the problem， we offer
>> patch<v2-0001-iscsi-fix-segfault-at-conn_close>。
>
> When a task is freshly allocated by iscsi_alloc_task, it has an empty
> c_hlist (see INIT_LIST_HEAD in iscsi_alloc_task), that will remain the
> same until iscsi_free_task if the task is never linked to a cmd_list.
>
> Therefore doing nothing with an empty c_hlist is enough (even list_del
> on an empty c_hlist would do no harm). That would skip ISCSI tasks which
> are not SCSI commands, exactly because they never get added to cmd_list.
>
> There are also two cases where c_hlist has prev == next == NULL, one of
> them when a task is allocated directly with iscsi_tcp_alloc_task (it
> happens for NOOP-IN "pings", and such a task should never be
> iscsi_free_task'ed), another one when a task used to be on a cmd_list,
> and then list_del was called (list_del sets prev == next == NULL).
>
> So I believe it's enough to check for NULL pointers and for an empty
> list, doing list_del if neither of these conditions is true.
>
>> in addition：after we carefully consideration： you below patch still
>> may happen some task unlinking from session->cmd_list， when there is
>> only one task in session->cmd_list，now task->c_hlist.next and
>> task->c_hlist.prev is equal， according to you patch logic， this task
>> will not do list_del。
>
> Thank you for the heads up!
>
> Yep, now I use list_empty to check for list emptiness (it compares
> &c_hlist with c_hlist->head internally). Attaching the patch below.
>
> Are there any reasons for checking opcode then?
>
>
>
> --
> Regards, Anton Kovalenko | +7(916)345-34-02 | Elektrostal' MO, Russia
>
Attachment:
v2-0001-iscsi-fix-segfault-at-conn_close.patch
Description: Binary data
