| Message ID | 20210617064653.3193618-1-libaokun1@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [-next] cifsd: fix WARNING: convert list_for_each to entry variant in smb2pdu.c | expand |
On Thu, Jun 17, 2021 at 02:46:53PM +0800, Baokun Li wrote: > convert list_for_each() to list_for_each_entry() where > applicable. > > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Baokun Li <libaokun1@huawei.com> > --- > fs/cifsd/smb2pdu.c | 15 ++++----------- > 1 file changed, 4 insertions(+), 11 deletions(-) > > diff --git a/fs/cifsd/smb2pdu.c b/fs/cifsd/smb2pdu.c > index ac15a9287310..22ef1d9eed1b 100644 > --- a/fs/cifsd/smb2pdu.c > +++ b/fs/cifsd/smb2pdu.c > @@ -74,10 +74,7 @@ static inline int check_session_id(struct ksmbd_conn *conn, u64 id) > struct channel *lookup_chann_list(struct ksmbd_session *sess) > { > struct channel *chann; > - struct list_head *t; > - > - list_for_each(t, &sess->ksmbd_chann_list) { > - chann = list_entry(t, struct channel, chann_list); > + list_for_each_entry(chann, &sess->ksmbd_chann_list, chann_list) { > if (chann && chann->conn == sess->conn) "chan" is the list iterator and it can't be NULL. > return chann; > } regards, dan carpenter
I don't know what the difference is between list_for_each_entry() and list_for_each() for 'struct channel *chann', but I don't think there's any difference here. Would you give me more detial about this, please? Thank you. Best Regards. 在 2021/6/17 17:26, Dan Carpenter 写道: > On Thu, Jun 17, 2021 at 02:46:53PM +0800, Baokun Li wrote: >> convert list_for_each() to list_for_each_entry() where >> applicable. >> >> Reported-by: Hulk Robot <hulkci@huawei.com> >> Signed-off-by: Baokun Li <libaokun1@huawei.com> >> --- >> fs/cifsd/smb2pdu.c | 15 ++++----------- >> 1 file changed, 4 insertions(+), 11 deletions(-) >> >> diff --git a/fs/cifsd/smb2pdu.c b/fs/cifsd/smb2pdu.c >> index ac15a9287310..22ef1d9eed1b 100644 >> --- a/fs/cifsd/smb2pdu.c >> +++ b/fs/cifsd/smb2pdu.c >> @@ -74,10 +74,7 @@ static inline int check_session_id(struct ksmbd_conn *conn, u64 id) >> struct channel *lookup_chann_list(struct ksmbd_session *sess) >> { >> struct channel *chann; >> - struct list_head *t; >> - >> - list_for_each(t, &sess->ksmbd_chann_list) { >> - chann = list_entry(t, struct channel, chann_list); >> + list_for_each_entry(chann, &sess->ksmbd_chann_list, chann_list) { >> if (chann && chann->conn == sess->conn) > "chan" is the list iterator and it can't be NULL. > >> return chann; >> } > regards, > dan carpenter > > .
On Fri, Jun 18, 2021 at 09:44:37AM +0800, libaokun (A) wrote: > I don't know what the difference is between > > list_for_each_entry() and list_for_each() for 'struct channel *chann', > > but I don't think there's any difference here. Correct. There is no difference, but Coccinelle is smart enough to parse list_for_each_entry() and it's not smart enough to parse list_for_each(). > > Would you give me more detial about this, please? There is a Coccinelle script scripts/coccinelle/iterators/itnull.cocci which will complain about the NULL check in the new code so this patch will introduce a new warning. We may as well remove the unnecessary NULL check and avoid the warning. regards, dan carpenter
在 2021/6/18 13:14, Dan Carpenter 写道: > On Fri, Jun 18, 2021 at 09:44:37AM +0800, libaokun (A) wrote: >> I don't know what the difference is between >> >> list_for_each_entry() and list_for_each() for 'struct channel *chann', >> >> but I don't think there's any difference here. > Correct. There is no difference, but Coccinelle is smart enough to > parse list_for_each_entry() and it's not smart enough to parse > list_for_each(). > >> Would you give me more detial about this, please? > There is a Coccinelle script scripts/coccinelle/iterators/itnull.cocci > which will complain about the NULL check in the new code so this patch > will introduce a new warning. We may as well remove the unnecessary > NULL check and avoid the warning. > > regards, > dan carpenter > > . I get your point, but this bug has nothing to do with my patch. It's from e2f34481b24d(cifsd: add server-side procedures for SMB3). Thank you.
On Fri, Jun 18, 2021 at 03:47:12PM +0800, libaokun (A) wrote: > 在 2021/6/18 13:14, Dan Carpenter 写道: > > On Fri, Jun 18, 2021 at 09:44:37AM +0800, libaokun (A) wrote: > > > I don't know what the difference is between > > > > > > list_for_each_entry() and list_for_each() for 'struct channel *chann', > > > > > > but I don't think there's any difference here. > > Correct. There is no difference, but Coccinelle is smart enough to > > parse list_for_each_entry() and it's not smart enough to parse > > list_for_each(). > > > > > Would you give me more detial about this, please? > > There is a Coccinelle script scripts/coccinelle/iterators/itnull.cocci > > which will complain about the NULL check in the new code so this patch > > will introduce a new warning. We may as well remove the unnecessary > > NULL check and avoid the warning. > > > > regards, > > dan carpenter > > > > . > > I get your point, but this bug has nothing to do with my patch. > > It's from e2f34481b24d(cifsd: add server-side procedures for SMB3). > Fine, but when Hulk Robot reports the new warning and someone fixes it, I'm going to insist that the Fixes tag points to your patch. ;) regards, dan carpenter
diff --git a/fs/cifsd/smb2pdu.c b/fs/cifsd/smb2pdu.c index ac15a9287310..22ef1d9eed1b 100644 --- a/fs/cifsd/smb2pdu.c +++ b/fs/cifsd/smb2pdu.c @@ -74,10 +74,7 @@ static inline int check_session_id(struct ksmbd_conn *conn, u64 id) struct channel *lookup_chann_list(struct ksmbd_session *sess) { struct channel *chann; - struct list_head *t; - - list_for_each(t, &sess->ksmbd_chann_list) { - chann = list_entry(t, struct channel, chann_list); + list_for_each_entry(chann, &sess->ksmbd_chann_list, chann_list) { if (chann && chann->conn == sess->conn) return chann; } @@ -6258,7 +6255,6 @@ int smb2_cancel(struct ksmbd_work *work) struct smb2_hdr *hdr = work->request_buf; struct smb2_hdr *chdr; struct ksmbd_work *cancel_work = NULL; - struct list_head *tmp; int canceled = 0; struct list_head *command_list; @@ -6269,9 +6265,8 @@ int smb2_cancel(struct ksmbd_work *work) command_list = &conn->async_requests; spin_lock(&conn->request_lock); - list_for_each(tmp, command_list) { - cancel_work = list_entry(tmp, struct ksmbd_work, - async_request_entry); + list_for_each_entry(cancel_work, command_list, + async_request_entry) { chdr = cancel_work->request_buf; if (cancel_work->async_id != @@ -6290,9 +6285,7 @@ int smb2_cancel(struct ksmbd_work *work) command_list = &conn->requests; spin_lock(&conn->request_lock); - list_for_each(tmp, command_list) { - cancel_work = list_entry(tmp, struct ksmbd_work, - request_entry); + list_for_each_entry(cancel_work, command_list, request_entry) { chdr = cancel_work->request_buf; if (chdr->MessageId != hdr->MessageId ||
convert list_for_each() to list_for_each_entry() where applicable. Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Baokun Li <libaokun1@huawei.com> --- fs/cifsd/smb2pdu.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)