* [PATCH] selftests/rtc:Fix a resource leak @ 2024-07-10 6:07 Zhu Jun 2024-07-10 7:16 ` Alexandre Belloni 0 siblings, 1 reply; 6+ messages in thread From: Zhu Jun @ 2024-07-10 6:07 UTC (permalink / raw) To: alexandre.belloni, shuah Cc: linux-rtc, linux-kselftest, linux-kernel, zhujun2 The opened file should be closed before exit, otherwise resource leak will occur that this problem was discovered by reading code Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com> --- tools/testing/selftests/rtc/setdate.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/testing/selftests/rtc/setdate.c b/tools/testing/selftests/rtc/setdate.c index b303890b3de2..17a00affb0ec 100644 --- a/tools/testing/selftests/rtc/setdate.c +++ b/tools/testing/selftests/rtc/setdate.c @@ -65,6 +65,7 @@ int main(int argc, char **argv) retval = ioctl(fd, RTC_RD_TIME, ¤t); if (retval == -1) { perror("RTC_RD_TIME ioctl"); + close(fd); exit(errno); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] selftests/rtc:Fix a resource leak 2024-07-10 6:07 [PATCH] selftests/rtc:Fix a resource leak Zhu Jun @ 2024-07-10 7:16 ` Alexandre Belloni 2024-07-10 7:43 ` [PATCH v2] " Zhu Jun 0 siblings, 1 reply; 6+ messages in thread From: Alexandre Belloni @ 2024-07-10 7:16 UTC (permalink / raw) To: Zhu Jun; +Cc: shuah, linux-rtc, linux-kselftest, linux-kernel On 09/07/2024 23:07:43-0700, Zhu Jun wrote: > The opened file should be closed before exit, otherwise resource leak > will occur that this problem was discovered by reading code Can you elaborate on the leak? All the fds are getting closed on exit. > > Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com> > --- > tools/testing/selftests/rtc/setdate.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/testing/selftests/rtc/setdate.c b/tools/testing/selftests/rtc/setdate.c > index b303890b3de2..17a00affb0ec 100644 > --- a/tools/testing/selftests/rtc/setdate.c > +++ b/tools/testing/selftests/rtc/setdate.c > @@ -65,6 +65,7 @@ int main(int argc, char **argv) > retval = ioctl(fd, RTC_RD_TIME, ¤t); > if (retval == -1) { > perror("RTC_RD_TIME ioctl"); > + close(fd); > exit(errno); > } > > -- > 2.17.1 > > > -- Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] selftests/rtc:Fix a resource leak 2024-07-10 7:16 ` Alexandre Belloni @ 2024-07-10 7:43 ` Zhu Jun 2024-07-10 7:55 ` Alexandre Belloni 0 siblings, 1 reply; 6+ messages in thread From: Zhu Jun @ 2024-07-10 7:43 UTC (permalink / raw) To: alexandre.belloni Cc: linux-kernel, linux-kselftest, linux-rtc, shuah, zhujun2 The opened file should be closed before exit, otherwise resource leak will occur that this problem was discovered by code reading Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com> --- From a good programming practice perspective, especially in more complex programs, explicitly freeing allocated memory is a good habit! tools/testing/selftests/rtc/setdate.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/testing/selftests/rtc/setdate.c b/tools/testing/selftests/rtc/setdate.c index b303890b3de2..17a00affb0ec 100644 --- a/tools/testing/selftests/rtc/setdate.c +++ b/tools/testing/selftests/rtc/setdate.c @@ -65,6 +65,7 @@ int main(int argc, char **argv) retval = ioctl(fd, RTC_RD_TIME, ¤t); if (retval == -1) { perror("RTC_RD_TIME ioctl"); + close(fd); exit(errno); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] selftests/rtc:Fix a resource leak 2024-07-10 7:43 ` [PATCH v2] " Zhu Jun @ 2024-07-10 7:55 ` Alexandre Belloni 2024-07-10 16:00 ` Shuah Khan 0 siblings, 1 reply; 6+ messages in thread From: Alexandre Belloni @ 2024-07-10 7:55 UTC (permalink / raw) To: Zhu Jun; +Cc: linux-kernel, linux-kselftest, linux-rtc, shuah On 10/07/2024 00:43:09-0700, Zhu Jun wrote: > The opened file should be closed before exit, otherwise resource leak > will occur that this problem was discovered by code reading The question is still why should it be closed before exit as it will be closed on exit? > > Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com> > --- > From a good programming practice perspective, especially in more complex programs, > explicitly freeing allocated memory is a good habit! > > > tools/testing/selftests/rtc/setdate.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/testing/selftests/rtc/setdate.c b/tools/testing/selftests/rtc/setdate.c > index b303890b3de2..17a00affb0ec 100644 > --- a/tools/testing/selftests/rtc/setdate.c > +++ b/tools/testing/selftests/rtc/setdate.c > @@ -65,6 +65,7 @@ int main(int argc, char **argv) > retval = ioctl(fd, RTC_RD_TIME, ¤t); > if (retval == -1) { > perror("RTC_RD_TIME ioctl"); > + close(fd); > exit(errno); > } > > -- > 2.17.1 > > > -- Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] selftests/rtc:Fix a resource leak 2024-07-10 7:55 ` Alexandre Belloni @ 2024-07-10 16:00 ` Shuah Khan 2024-07-16 17:58 ` John Hubbard 0 siblings, 1 reply; 6+ messages in thread From: Shuah Khan @ 2024-07-10 16:00 UTC (permalink / raw) To: Alexandre Belloni, Zhu Jun Cc: linux-kernel, linux-kselftest, linux-rtc, shuah, Shuah Khan On 7/10/24 01:55, Alexandre Belloni wrote: > On 10/07/2024 00:43:09-0700, Zhu Jun wrote: >> The opened file should be closed before exit, otherwise resource leak >> will occur that this problem was discovered by code reading > > The question is still why should it be closed before exit as it will be > closed on exit? > Zhu Jun, +1 on this. I have responded to your other patches that do the same in other tests. There is no need to make such changes. thanks, -- Shuah ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] selftests/rtc:Fix a resource leak 2024-07-10 16:00 ` Shuah Khan @ 2024-07-16 17:58 ` John Hubbard 0 siblings, 0 replies; 6+ messages in thread From: John Hubbard @ 2024-07-16 17:58 UTC (permalink / raw) To: Shuah Khan, Alexandre Belloni, Zhu Jun Cc: linux-kernel, linux-kselftest, linux-rtc, shuah On 7/10/24 9:00 AM, Shuah Khan wrote: > On 7/10/24 01:55, Alexandre Belloni wrote: >> On 10/07/2024 00:43:09-0700, Zhu Jun wrote: >>> The opened file should be closed before exit, otherwise resource leak >>> will occur that this problem was discovered by code reading >> >> The question is still why should it be closed before exit as it will be >> closed on exit? >> > > Zhu Jun, > > +1 on this. I have responded to your other patches that do the same > in other tests. There is no need to make such changes. > > thanks, > -- Shuah > Yes. What Shuah and Alexandre said. And the same reasoning also applies to memory buffers: they are also released by the kernel at program exit. This blind adherence to "good programming practices" is not helpful here, because it turns out that selftests are small, short-running, and simple. These are all desirable properties, because remember, there are no tests to verify the selftests, so we rely on keeping them simple and very easy to quickly read and understand. "Good programming practices" is not a "one approach to all programs" thing. So adding line count to free something a few lines before program exit, for tiny programs, is actually counter productive in this environment. Zhu, if you're going to keep working on selftests, I think you would really benefit from some of the literature that goes into the design philosophy for unit tests. Google has the best (and most recent) work IMHO [1], but there are others of course. [1] Software Engineering at Google (Apr 7, 2020), by Titus Winters thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-07-16 17:59 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-07-10 6:07 [PATCH] selftests/rtc:Fix a resource leak Zhu Jun 2024-07-10 7:16 ` Alexandre Belloni 2024-07-10 7:43 ` [PATCH v2] " Zhu Jun 2024-07-10 7:55 ` Alexandre Belloni 2024-07-10 16:00 ` Shuah Khan 2024-07-16 17:58 ` John Hubbard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).