29/06/2025 00:49, Stephen Hemminger: > On Sat, 28 Jun 2025 18:45:44 +0200 > Morten Brørup <m...@smartsharesystems.com> wrote: > > > > From: Thomas Monjalon [mailto:tho...@monjalon.net] > > > Sent: Friday, 27 June 2025 20.30 > > > > > > 27/06/2025 19:49, Morten Brørup: > > > > > From: Thomas Monjalon [mailto:tho...@monjalon.net] > > > > > Sent: Friday, 27 June 2025 19.35 > > > > > > > > > > 27/06/2025 18:38, Morten Brørup: > > > > > > > From: Thomas Monjalon [mailto:tho...@monjalon.net] > > > > > > > Sent: Friday, 27 June 2025 17.58 > > > > > > > > > > > > > > 24/06/2025 10:03, Morten Brørup: > > > > > > > > + if ((ssize_t)page_size < 0) > > > > > > > > + rte_panic("sysconf(_SC_PAGESIZE) > > > > > > > > failed: %s", > > > > > > > > + errno == 0 ? > > > > > > > > "Indeterminate" : > > > > > > > strerror(errno)); > > > > > > > > > > > > > > We don't want more rte_panic(). > > > > > > > You could log the problem and return 0 here. > > > > > > > It will be a problem later, but it may allow the application to > > > > > cleanup > > > > > > > instead of abrupting crashing. > > > > > > > > > > > > Disagree. > > > > > > That would be likely to cause crash with division by zero later. > > > > > > Better to fail early. > > > > > > > > > > Which division by zero? > > > > > > > > Functions dividing by page size. E.g.: > > > > > > > https://elixir.bootlin.com/dpdk/v25.03/source/lib/eal/common/eal_common_ > > > memory.c#L313 > > > > > > > > > > > > > > I don't think a library should take this decision on behalf of the > > > app. > > > > > > > > I expect lots of things to break if sysconf(_SC_PAGESIZE) fails, so > > > the purpose of this patch is to centralize error handling here, and only > > > continue/return with non-failing values. > > > > > > > > Otherwise, everywhere using rte_mem_page_size() or > > > sysconf(_SC_PAGESIZE) should implement error handling (or ignore > > > errors). > > > > That's a lot of places, so I'm not going to provide a patch doing > > > that. > > > > > > I understand. > > > > > > The problem is that we don't have an exception mechanism in this > > > language. > > > > Yep. > > And everyone assumes sysconf(_SC_PAGESIZE) never fails, which is probably > > correct, so nobody implemented error handling for it. Not even in > > rte_mem_page_size(). > > Coverity detected the missing error handling, and warns: "Although > > rte_mem_page_size() is declared to return unsigned int, it may actually > > return a negative value." This defect applies to all functions calling > > rte_mem_page_size(). > > This patch adds error handling to ensure that rte_mem_page_size() only > > returns non-negative values, or doesn’t return at all - i.e. fails with > > rte_panic() - so Coverity is satisfied with callers not implementing error > > handling for it. > > > > It would be borderline waste of time fixing all the callers, so I fixed the > > root cause to satisfy Coverity. > > > > From an higher level perspective: > > This is a low level EAL function to determine the page size. I would > > consider it reasonable for such a low level EAL function to never fail. > > If some O/S decides to not have a "system page size", and fail with > > "Indeterminate", e.g. to support multiple page sizes, we would need to > > handle that somehow. But let's ignore that until it actually happens, if > > ever. > > > > If you are skeptical about this patch 2/3 in the series, we can escalate > > the discussion to the tech board. If you really hate this patch 2/3, I will > > honor a NAK from you. The patch is not important for me; I'm just trying to > > clean up. > > > > In such cases, I look at glibc source and see if handles it or not. > Looks like only used a couple of places there, the result of > sysconf(_SC_PAGE_SIZE) is checked > in one of the tests; but is not checked in the loading of locale's. It > expects a valid power of 2 > value there. > > Ok to just die if value isn't valid.
Yes I'm convinced too.
