| Message ID | 20210311103446.5dwjcopeggy7k6gg@kewl-virtual-machine (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] net: core: datagram.c: Fix use of assignment in if condition | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: ktkhai@virtuozzo.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 1 this patch: 8 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 90 lines checked |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 1 this patch: 8 |
| netdev/header_inline | success | Link |
On Thu, Mar 11, 2021 at 11:34 AM Shubhankar Kuranagatti <shubhankarvk@gmail.com> wrote: > > The assignment inside the if condition has been changed to > initialising outside the if condition. > > Signed-off-by: Shubhankar Kuranagatti <shubhankarvk@gmail.com> > --- > net/core/datagram.c | 31 ++++++++++++++++++++----------- > 1 file changed, 20 insertions(+), 11 deletions(-) > > diff --git a/net/core/datagram.c b/net/core/datagram.c > index 15ab9ffb27fe..7b2204f102b7 100644 > --- a/net/core/datagram.c > +++ b/net/core/datagram.c > @@ -427,7 +427,8 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset, > offset += n; > if (n != copy) > goto short_copy; > - if ((len -= copy) == 0) > + len -= copy > + if ((len) == 0) > return 0; > Quite frankly I prefer the current style. It also seems you have not even compiled your change, this is not a good start. Lets keep reviewer time to review patches that really bring an improvement, since stylistic changes like that make our backports more likely to have conflicts. Thanks.
Hi Shubhankar,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
[also build test ERROR on linus/master v5.12-rc2 next-20210311]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Shubhankar-Kuranagatti/net-core-datagram-c-Fix-use-of-assignment-in-if-condition/20210311-184120
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 34bb975126419e86bc3b95e200dc41de6c6ca69c
config: x86_64-randconfig-r025-20210311 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 574a9dabc63ba1e7a04c08d4bde2eacd61b44ce1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/89811231e3ec535f3e5188fb8578535e13c1f1ba
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Shubhankar-Kuranagatti/net-core-datagram-c-Fix-use-of-assignment-in-if-condition/20210311-184120
git checkout 89811231e3ec535f3e5188fb8578535e13c1f1ba
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> net/core/datagram.c:430:14: error: expected ';' after expression
len -= copy
^
;
net/core/datagram.c:443:22: error: expected ';' after expression
copy = end - offset
^
;
net/core/datagram.c:457:15: error: expected ';' after expression
len -= copy
^
;
net/core/datagram.c:477:15: error: expected ';' after expression
len -= copy
^
;
net/core/datagram.c:591:15: error: expected ';' after expression
len -= copy
^
;
5 errors generated.
vim +430 net/core/datagram.c
406
407 INDIRECT_CALLABLE_DECLARE(static size_t simple_copy_to_iter(const void *addr,
408 size_t bytes,
409 void *data __always_unused,
410 struct iov_iter *i));
411
412 static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
413 struct iov_iter *to, int len, bool fault_short,
414 size_t (*cb)(const void *, size_t, void *,
415 struct iov_iter *), void *data)
416 {
417 int start = skb_headlen(skb);
418 int i, copy = start - offset, start_off = offset, n;
419 struct sk_buff *frag_iter;
420
421 /* Copy header. */
422 if (copy > 0) {
423 if (copy > len)
424 copy = len;
425 n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
426 skb->data + offset, copy, data, to);
427 offset += n;
428 if (n != copy)
429 goto short_copy;
> 430 len -= copy
431 if ((len) == 0)
432 return 0;
433 }
434
435 /* Copy paged appendix. Hmm... why does this look so complicated? */
436 for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
437 int end;
438 const skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
439
440 WARN_ON(start > offset + len);
441
442 end = start + skb_frag_size(frag);
443 copy = end - offset
444 if ((copy) > 0) {
445 struct page *page = skb_frag_page(frag);
446 u8 *vaddr = kmap(page);
447
448 if (copy > len)
449 copy = len;
450 n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
451 vaddr + skb_frag_off(frag) + offset - start,
452 copy, data, to);
453 kunmap(page);
454 offset += n;
455 if (n != copy)
456 goto short_copy;
457 len -= copy
458 if (!(len))
459 return 0;
460 }
461 start = end;
462 }
463
464 skb_walk_frags(skb, frag_iter) {
465 int end;
466
467 WARN_ON(start > offset + len);
468
469 end = start + frag_iter->len;
470 copy = end - offset;
471 if ((copy) > 0) {
472 if (copy > len)
473 copy = len;
474 if (__skb_datagram_iter(frag_iter, offset - start,
475 to, copy, fault_short, cb, data))
476 goto fault;
477 len -= copy
478 if ((len) == 0)
479 return 0;
480 offset += copy;
481 }
482 start = end;
483 }
484 if (!len)
485 return 0;
486
487 /* This is not really a user copy fault, but rather someone
488 * gave us a bogus length on the skb. We should probably
489 * print a warning here as it may indicate a kernel bug.
490 */
491
492 fault:
493 iov_iter_revert(to, offset - start_off);
494 return -EFAULT;
495
496 short_copy:
497 if (fault_short || iov_iter_count(to))
498 goto fault;
499
500 return 0;
501 }
502
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/net/core/datagram.c b/net/core/datagram.c index 15ab9ffb27fe..7b2204f102b7 100644 --- a/net/core/datagram.c +++ b/net/core/datagram.c @@ -427,7 +427,8 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset, offset += n; if (n != copy) goto short_copy; - if ((len -= copy) == 0) + len -= copy + if ((len) == 0) return 0; } @@ -439,7 +440,8 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset, WARN_ON(start > offset + len); end = start + skb_frag_size(frag); - if ((copy = end - offset) > 0) { + copy = end - offset + if ((copy) > 0) { struct page *page = skb_frag_page(frag); u8 *vaddr = kmap(page); @@ -452,7 +454,8 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset, offset += n; if (n != copy) goto short_copy; - if (!(len -= copy)) + len -= copy + if (!(len)) return 0; } start = end; @@ -464,13 +467,15 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset, WARN_ON(start > offset + len); end = start + frag_iter->len; - if ((copy = end - offset) > 0) { + copy = end - offset; + if ((copy) > 0) { if (copy > len) copy = len; if (__skb_datagram_iter(frag_iter, offset - start, to, copy, fault_short, cb, data)) goto fault; - if ((len -= copy) == 0) + len -= copy + if ((len) == 0) return 0; offset += copy; } @@ -558,7 +563,8 @@ int skb_copy_datagram_from_iter(struct sk_buff *skb, int offset, copy = len; if (copy_from_iter(skb->data + offset, copy, from) != copy) goto fault; - if ((len -= copy) == 0) + len -= copy; + if ((len) == 0) return 0; offset += copy; } @@ -571,7 +577,8 @@ int skb_copy_datagram_from_iter(struct sk_buff *skb, int offset, WARN_ON(start > offset + len); end = start + skb_frag_size(frag); - if ((copy = end - offset) > 0) { + copy = end - offset; + if ((copy) > 0) { size_t copied; if (copy > len) @@ -581,8 +588,8 @@ int skb_copy_datagram_from_iter(struct sk_buff *skb, int offset, copy, from); if (copied != copy) goto fault; - - if (!(len -= copy)) + len -= copy + if (!(len)) return 0; offset += copy; } @@ -595,14 +602,16 @@ int skb_copy_datagram_from_iter(struct sk_buff *skb, int offset, WARN_ON(start > offset + len); end = start + frag_iter->len; - if ((copy = end - offset) > 0) { + copy = end - offset; + if ((copy) > 0) { if (copy > len) copy = len; if (skb_copy_datagram_from_iter(frag_iter, offset - start, from, copy)) goto fault; - if ((len -= copy) == 0) + len -= copy; + if ((len) == 0) return 0; offset += copy; }
The assignment inside the if condition has been changed to initialising outside the if condition. Signed-off-by: Shubhankar Kuranagatti <shubhankarvk@gmail.com> --- net/core/datagram.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-)