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.
