RE: [PATCH v3 2/3] eal: handle sysconf(_SC_PAGESIZE) negative return value

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Friday, 27 June 2025 20.30
> 
> 27/06/2025 19:49, Morten Brørup:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Friday, 27 June 2025 19.35
> > >
> > > 27/06/2025 18:38, Morten Brørup:
> > > > > From: Thomas Monjalon [mailto:thomas@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.