| Message ID | 20211002131212.130629-8-slow@samba.org |
|---|---|
| State | New |
| Headers | show |
| Series | Buffer validation patches | expand |
2021-10-02 22:12 GMT+09:00, Ralph Boehme <slow@samba.org>: > No change in behaviour. It is better to add patch subject here if there is nothing to write patch description. i.e. "Use ksmbd_req_buf_next() in ksmbd_smb2_check_message()" > > Cc: Namjae Jeon <linkinjeon@kernel.org> > Cc: Tom Talpey <tom@talpey.com> > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > Cc: Steve French <smfrench@gmail.com> > Cc: Hyunchul Lee <hyc.lee@gmail.com> > Signed-off-by: Ralph Boehme <slow@samba.org> > --- > fs/ksmbd/smb2misc.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c > index c1f0f10ca9f9..76f53db7db8d 100644 > --- a/fs/ksmbd/smb2misc.c > +++ b/fs/ksmbd/smb2misc.c > @@ -329,17 +329,12 @@ static int smb2_validate_credit_charge(struct smb2_hdr > *hdr) > > int ksmbd_smb2_check_message(struct ksmbd_work *work) > { > - struct smb2_pdu *pdu = work->request_buf; > + struct smb2_pdu *pdu = ksmbd_req_buf_next(work); > struct smb2_hdr *hdr = &pdu->hdr; > int command; > __u32 clc_len; /* calculated length */ > __u32 len = get_rfc1002_len(pdu); > > - if (work->next_smb2_rcv_hdr_off) { > - pdu = ksmbd_req_buf_next(work); > - hdr = &pdu->hdr; > - } > - > if (le32_to_cpu(hdr->NextCommand) > 0) { > len = le32_to_cpu(hdr->NextCommand); > } else if (work->next_smb2_rcv_hdr_off) { > -- > 2.31.1 > >
Am 03.10.21 um 02:59 schrieb Namjae Jeon: > 2021-10-02 22:12 GMT+09:00, Ralph Boehme <slow@samba.org>: >> No change in behaviour. > It is better to add patch subject here if there is nothing to write > patch description. > i.e. "Use ksmbd_req_buf_next() in ksmbd_smb2_check_message()" See the mail subject, that's the patch subject. In Samba we often help reviewers reviewing large patchsets that may include a lot of small preparational/cleanup patches with a sentence like "No change in behaviour" to give a hint which patches are critical and need extra care when reviewing (ie all others) and which may not require this. -slow
Typically kernel style encourages even a brief description in all changesets (even trivial ones) e.g. "Simplifies message parsing slightly. No change in behavior" On Mon, Oct 4, 2021 at 11:29 PM Ralph Boehme <slow@samba.org> wrote: > > Am 03.10.21 um 02:59 schrieb Namjae Jeon: > > 2021-10-02 22:12 GMT+09:00, Ralph Boehme <slow@samba.org>: > >> No change in behaviour. > > It is better to add patch subject here if there is nothing to write > > patch description. > > i.e. "Use ksmbd_req_buf_next() in ksmbd_smb2_check_message()" > See the mail subject, that's the patch subject. > > In Samba we often help reviewers reviewing large patchsets that may > include a lot of small preparational/cleanup patches with a sentence > like "No change in behaviour" to give a hint which patches are critical > and need extra care when reviewing (ie all others) and which may not > require this. > > -slow > > -- > Ralph Boehme, Samba Team https://samba.org/ > SerNet Samba Team Lead https://sernet.de/en/team-samba
Am 05.10.21 um 20:43 schrieb Steve French: > Typically kernel style encourages even a brief description in all > changesets (even trivial ones) e.g. > > "Simplifies message parsing slightly. No change in behavior" Sure, I could add this. Otoh bcf130f9dfbaccf91376a44b18f51ed8007840d6 :) To me it doesn't make sense. Cheers! -slow
On Tue, Oct 5, 2021 at 2:28 PM Ralph Boehme <slow@samba.org> wrote: > > Am 05.10.21 um 20:43 schrieb Steve French: > > Typically kernel style encourages even a brief description in all > > changesets (even trivial ones) e.g. > > > > "Simplifies message parsing slightly. No change in behavior" > > Sure, I could add this. Otoh > > bcf130f9dfbaccf91376a44b18f51ed8007840d6 > > :) > > To me it doesn't make sense. The patch submission guidelines for the kernel (see Documentation/process/submitting-patches.rst) are not too bad to understand (you can see why slightly more description is needed from some examples mentioned there), and seem reasonably logical. Also checkpatch already auto-verifies a few of the things mentioned in the submitting-patches guidelines. Note that your example is an old patch (from 10 years ago); rules have gotten a bit stricter. Here is a more recent patch from the same committer, note that he no longer uses the minimalist description ("No change in behavior") see below (and another example from same committer commit 4b03d99794eeed27650597a886247c6427ce1055) commit ebd9d2c2f5a7ebaaed2d7bb4dee148755f46033d Author: J. Bruce Fields <bfields@redhat.com> Date: Fri Apr 16 14:00:17 2021 -0400 nfsd: reshuffle some code No change in behavior, I'm just moving some code around to avoid forward references in a following patch. (To do someday: figure out how to split up nfs4state.c. It's big and disorganized.) Signed-off-by: J. Bruce Fields <bfields@redhat.com> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c index c1f0f10ca9f9..76f53db7db8d 100644 --- a/fs/ksmbd/smb2misc.c +++ b/fs/ksmbd/smb2misc.c @@ -329,17 +329,12 @@ static int smb2_validate_credit_charge(struct smb2_hdr *hdr) int ksmbd_smb2_check_message(struct ksmbd_work *work) { - struct smb2_pdu *pdu = work->request_buf; + struct smb2_pdu *pdu = ksmbd_req_buf_next(work); struct smb2_hdr *hdr = &pdu->hdr; int command; __u32 clc_len; /* calculated length */ __u32 len = get_rfc1002_len(pdu); - if (work->next_smb2_rcv_hdr_off) { - pdu = ksmbd_req_buf_next(work); - hdr = &pdu->hdr; - } - if (le32_to_cpu(hdr->NextCommand) > 0) { len = le32_to_cpu(hdr->NextCommand); } else if (work->next_smb2_rcv_hdr_off) {
No change in behaviour. Cc: Namjae Jeon <linkinjeon@kernel.org> Cc: Tom Talpey <tom@talpey.com> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> Cc: Steve French <smfrench@gmail.com> Cc: Hyunchul Lee <hyc.lee@gmail.com> Signed-off-by: Ralph Boehme <slow@samba.org> --- fs/ksmbd/smb2misc.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)