| Message ID | 26b7f215-4f83-413c-9dab-737d790053c0@web.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | thunderbolt: Use common error handling code in update_property_block() | expand |
On Wed, Sep 25, 2024 at 10:10:38AM +0200, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Wed, 25 Sep 2024 09:39:16 +0200 > > Add a jump target so that a bit of exception handling can be better reused > at the end of this function implementation. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/thunderbolt/xdomain.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c > index 11a50c86a1e4..8e3cf95ca99c 100644 > --- a/drivers/thunderbolt/xdomain.c > +++ b/drivers/thunderbolt/xdomain.c > @@ -670,23 +670,19 @@ static void update_property_block(struct tb_xdomain *xd) > ret = tb_property_format_dir(dir, NULL, 0); > if (ret < 0) { > dev_warn(&xd->dev, "local property block creation failed\n"); > - tb_property_free_dir(dir); > - goto out_unlock; > + goto out_free_dir; > } > > block_len = ret; > block = kcalloc(block_len, sizeof(*block), GFP_KERNEL); > - if (!block) { > - tb_property_free_dir(dir); > - goto out_unlock; > - } > + if (!block) > + goto out_free_dir; > > ret = tb_property_format_dir(dir, block, block_len); > if (ret) { > dev_warn(&xd->dev, "property block generation failed\n"); > - tb_property_free_dir(dir); > kfree(block); > - goto out_unlock; > + goto out_free_dir; > } > > tb_property_free_dir(dir); > @@ -701,6 +697,11 @@ static void update_property_block(struct tb_xdomain *xd) > out_unlock: > mutex_unlock(&xd->lock); > mutex_unlock(&xdomain_lock); > + return; > + > +out_free_dir: > + tb_property_free_dir(dir); > + goto out_unlock; No way, this kind of spaghetti is really hard to follow.
>> Add a jump target so that a bit of exception handling can be better reused >> at the end of this function implementation. … >> +++ b/drivers/thunderbolt/xdomain.c >> @@ -670,23 +670,19 @@ static void update_property_block(struct tb_xdomain *xd) >> ret = tb_property_format_dir(dir, NULL, 0); >> if (ret < 0) { >> dev_warn(&xd->dev, "local property block creation failed\n"); >> - tb_property_free_dir(dir); >> - goto out_unlock; >> + goto out_free_dir; >> } >> >> block_len = ret; >> block = kcalloc(block_len, sizeof(*block), GFP_KERNEL); >> - if (!block) { >> - tb_property_free_dir(dir); >> - goto out_unlock; >> - } >> + if (!block) >> + goto out_free_dir; >> >> ret = tb_property_format_dir(dir, block, block_len); >> if (ret) { >> dev_warn(&xd->dev, "property block generation failed\n"); >> - tb_property_free_dir(dir); >> kfree(block); >> - goto out_unlock; >> + goto out_free_dir; >> } >> >> tb_property_free_dir(dir); >> @@ -701,6 +697,11 @@ static void update_property_block(struct tb_xdomain *xd) >> out_unlock: >> mutex_unlock(&xd->lock); >> mutex_unlock(&xdomain_lock); >> + return; >> + >> +out_free_dir: >> + tb_property_free_dir(dir); >> + goto out_unlock; > > No way, this kind of spaghetti is really hard to follow. Under which circumstances would you follow advice more from the section “7) Centralized exiting of functions” (according to a well-known information source)? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.11#n526 How do you think about to increase the application of scope-based resource management? Regards, Markus
On Wed, Sep 25, 2024 at 11:20:45AM +0200, Markus Elfring wrote: > >> out_unlock: > >> mutex_unlock(&xd->lock); > >> mutex_unlock(&xdomain_lock); > >> + return; > >> + > >> +out_free_dir: > >> + tb_property_free_dir(dir); > >> + goto out_unlock; > > > > No way, this kind of spaghetti is really hard to follow. > > Under which circumstances would you follow advice more from the section > “7) Centralized exiting of functions” (according to a well-known information source)? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.11#n526 > > How do you think about to increase the application of scope-based resource management? It is fine to use goto as it is described in the document you linked but this what you are doing is certainly not fine, at least in the code I'm maintaining: out_unlock: mutex_unlock(&xd->lock); mutex_unlock(&xdomain_lock); return; out_free_dir: tb_property_free_dir(dir); goto out_unlock; This "goto out_unlock" adds another goto to upwards which makes it really hard to follow because the flow is not anymore just downwards.
> It is fine to use goto as it is described in the document you linked but > this what you are doing is certainly not fine, at least in the code I'm > maintaining: > > out_unlock: > mutex_unlock(&xd->lock); > mutex_unlock(&xdomain_lock); > return; > > out_free_dir: > tb_property_free_dir(dir); > goto out_unlock; > > This "goto out_unlock" adds another goto to upwards which makes it > really hard to follow because the flow is not anymore just downwards. Would you like to benefit any more from the application of scope-based resource management? Regards, Markus
On Wed, Sep 25, 2024 at 11:40:09AM +0200, Markus Elfring wrote: > > It is fine to use goto as it is described in the document you linked but > > this what you are doing is certainly not fine, at least in the code I'm > > maintaining: > > > > out_unlock: > > mutex_unlock(&xd->lock); > > mutex_unlock(&xdomain_lock); > > return; > > > > out_free_dir: > > tb_property_free_dir(dir); > > goto out_unlock; > > > > This "goto out_unlock" adds another goto to upwards which makes it > > really hard to follow because the flow is not anymore just downwards. > > Would you like to benefit any more from the application of > scope-based resource management? Hi, This is the semi-friendly patch-bot of Greg Kroah-Hartman. Markus, you seem to have sent a nonsensical or otherwise pointless review comment to a patch submission on a Linux kernel developer mailing list. I strongly suggest that you not do this anymore. Please do not bother developers who are actively working to produce patches and features with comments that, in the end, are a waste of time. Patch submitter, please ignore Markus's suggestion; you do not need to follow it at all. The person/bot/AI that sent it is being ignored by almost all Linux kernel maintainers for having a persistent pattern of behavior of producing distracting and pointless commentary, and inability to adapt to feedback. Please feel free to also ignore emails from them. thanks, greg k-h's patch email bot
Hi Markus, kernel test robot noticed the following build errors: [auto build test ERROR on westeri-thunderbolt/next] [also build test ERROR on linus/master v6.11 next-20240925] [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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Markus-Elfring/thunderbolt-Use-common-error-handling-code-in-update_property_block/20240925-161308 base: https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git next patch link: https://lore.kernel.org/r/26b7f215-4f83-413c-9dab-737d790053c0%40web.de patch subject: [PATCH] thunderbolt: Use common error handling code in update_property_block() config: arc-randconfig-001-20240926 (https://download.01.org/0day-ci/archive/20240926/202409260728.uNVNmTvy-lkp@intel.com/config) compiler: arceb-elf-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240926/202409260728.uNVNmTvy-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409260728.uNVNmTvy-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/thunderbolt/xdomain.c: In function 'update_property_block': >> drivers/thunderbolt/xdomain.c:703:30: error: 'dir' undeclared (first use in this function); did you mean 'idr'? 703 | tb_property_free_dir(dir); | ^~~ | idr drivers/thunderbolt/xdomain.c:703:30: note: each undeclared identifier is reported only once for each function it appears in vim +703 drivers/thunderbolt/xdomain.c 645 646 static void update_property_block(struct tb_xdomain *xd) 647 { 648 mutex_lock(&xdomain_lock); 649 mutex_lock(&xd->lock); 650 /* 651 * If the local property block is not up-to-date, rebuild it now 652 * based on the global property template. 653 */ 654 if (!xd->local_property_block || 655 xd->local_property_block_gen < xdomain_property_block_gen) { 656 struct tb_property_dir *dir; 657 int ret, block_len; 658 u32 *block; 659 660 dir = tb_property_copy_dir(xdomain_property_dir); 661 if (!dir) { 662 dev_warn(&xd->dev, "failed to copy properties\n"); 663 goto out_unlock; 664 } 665 666 /* Fill in non-static properties now */ 667 tb_property_add_text(dir, "deviceid", utsname()->nodename); 668 tb_property_add_immediate(dir, "maxhopid", xd->local_max_hopid); 669 670 ret = tb_property_format_dir(dir, NULL, 0); 671 if (ret < 0) { 672 dev_warn(&xd->dev, "local property block creation failed\n"); 673 goto out_free_dir; 674 } 675 676 block_len = ret; 677 block = kcalloc(block_len, sizeof(*block), GFP_KERNEL); 678 if (!block) 679 goto out_free_dir; 680 681 ret = tb_property_format_dir(dir, block, block_len); 682 if (ret) { 683 dev_warn(&xd->dev, "property block generation failed\n"); 684 kfree(block); 685 goto out_free_dir; 686 } 687 688 tb_property_free_dir(dir); 689 /* Release the previous block */ 690 kfree(xd->local_property_block); 691 /* Assign new one */ 692 xd->local_property_block = block; 693 xd->local_property_block_len = block_len; 694 xd->local_property_block_gen = xdomain_property_block_gen; 695 } 696 697 out_unlock: 698 mutex_unlock(&xd->lock); 699 mutex_unlock(&xdomain_lock); 700 return; 701 702 out_free_dir: > 703 tb_property_free_dir(dir); 704 goto out_unlock; 705 } 706
Hi Markus, kernel test robot noticed the following build errors: [auto build test ERROR on westeri-thunderbolt/next] [also build test ERROR on linus/master v6.11 next-20240925] [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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Markus-Elfring/thunderbolt-Use-common-error-handling-code-in-update_property_block/20240925-161308 base: https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git next patch link: https://lore.kernel.org/r/26b7f215-4f83-413c-9dab-737d790053c0%40web.de patch subject: [PATCH] thunderbolt: Use common error handling code in update_property_block() config: x86_64-randconfig-001-20240926 (https://download.01.org/0day-ci/archive/20240926/202409260928.qBlg2dBU-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240926/202409260928.qBlg2dBU-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409260928.qBlg2dBU-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/thunderbolt/xdomain.c:703:23: error: use of undeclared identifier 'dir' 703 | tb_property_free_dir(dir); | ^ 1 error generated. vim +/dir +703 drivers/thunderbolt/xdomain.c 645 646 static void update_property_block(struct tb_xdomain *xd) 647 { 648 mutex_lock(&xdomain_lock); 649 mutex_lock(&xd->lock); 650 /* 651 * If the local property block is not up-to-date, rebuild it now 652 * based on the global property template. 653 */ 654 if (!xd->local_property_block || 655 xd->local_property_block_gen < xdomain_property_block_gen) { 656 struct tb_property_dir *dir; 657 int ret, block_len; 658 u32 *block; 659 660 dir = tb_property_copy_dir(xdomain_property_dir); 661 if (!dir) { 662 dev_warn(&xd->dev, "failed to copy properties\n"); 663 goto out_unlock; 664 } 665 666 /* Fill in non-static properties now */ 667 tb_property_add_text(dir, "deviceid", utsname()->nodename); 668 tb_property_add_immediate(dir, "maxhopid", xd->local_max_hopid); 669 670 ret = tb_property_format_dir(dir, NULL, 0); 671 if (ret < 0) { 672 dev_warn(&xd->dev, "local property block creation failed\n"); 673 goto out_free_dir; 674 } 675 676 block_len = ret; 677 block = kcalloc(block_len, sizeof(*block), GFP_KERNEL); 678 if (!block) 679 goto out_free_dir; 680 681 ret = tb_property_format_dir(dir, block, block_len); 682 if (ret) { 683 dev_warn(&xd->dev, "property block generation failed\n"); 684 kfree(block); 685 goto out_free_dir; 686 } 687 688 tb_property_free_dir(dir); 689 /* Release the previous block */ 690 kfree(xd->local_property_block); 691 /* Assign new one */ 692 xd->local_property_block = block; 693 xd->local_property_block_len = block_len; 694 xd->local_property_block_gen = xdomain_property_block_gen; 695 } 696 697 out_unlock: 698 mutex_unlock(&xd->lock); 699 mutex_unlock(&xdomain_lock); 700 return; 701 702 out_free_dir: > 703 tb_property_free_dir(dir); 704 goto out_unlock; 705 } 706
diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c index 11a50c86a1e4..8e3cf95ca99c 100644 --- a/drivers/thunderbolt/xdomain.c +++ b/drivers/thunderbolt/xdomain.c @@ -670,23 +670,19 @@ static void update_property_block(struct tb_xdomain *xd) ret = tb_property_format_dir(dir, NULL, 0); if (ret < 0) { dev_warn(&xd->dev, "local property block creation failed\n"); - tb_property_free_dir(dir); - goto out_unlock; + goto out_free_dir; } block_len = ret; block = kcalloc(block_len, sizeof(*block), GFP_KERNEL); - if (!block) { - tb_property_free_dir(dir); - goto out_unlock; - } + if (!block) + goto out_free_dir; ret = tb_property_format_dir(dir, block, block_len); if (ret) { dev_warn(&xd->dev, "property block generation failed\n"); - tb_property_free_dir(dir); kfree(block); - goto out_unlock; + goto out_free_dir; } tb_property_free_dir(dir); @@ -701,6 +697,11 @@ static void update_property_block(struct tb_xdomain *xd) out_unlock: mutex_unlock(&xd->lock); mutex_unlock(&xdomain_lock); + return; + +out_free_dir: + tb_property_free_dir(dir); + goto out_unlock; } static void start_handshake(struct tb_xdomain *xd)