Re: [PATCH v2] builtin/gc: protect against sysctl() failure in total_ram

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Date: Jul 2, 2025, 20:22 UTC
Message-ID: <4vz7mwsyckx4ie74qdlitcmhg4vyuoiuwzufd4nktgzu5ppzfx@cqmznirthjcp>
In-Reply-To: <xmqqikkacu30.fsf@gitster.g>

On Wed, Jul 02, 2025 at 11:42:59AM -0800, Junio C Hamano wrote:

Show 11 quoted lines

> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
> 
> > In the unlikely scenario that sysctl() fails, and uninitialized
> > value could be returned.
> >
> > Initialize the variable used and make sure its expected size
> > was correct before using it.
> 
> Oh, that's interesting.  I wonder if the system returns physical
> memory in 32 bits, we would want fall back doing something silly
> like

Yes, a FreeBSD 32bit x86 system will report at most 4294963200 from this call, even if using PAE and more memory is available.

Eitherway, since the caller for this is ok with a smaller size than real, shouldn't be that big of an issue IMHO.

Show 8 quoted lines

> 	if (!sysctl(mib, 2, &i64_tmp, &length, NULL, 0)) {
> 		if (length == sizeof(i64_tmp))
> 			return i64_tmp;
> 		else if (length == 4 &&
>                 	 !sysctl(mib, 2, &i32_tmp, &length, NULL, 0) &&
> 			length == 4)
> 			return i32_tmp;
> 	}

probably a little bit less silly in 20250702202118.48742-1-carenas@gmail.com

Show 5 quoted lines

> > While at it, add a cast for consistency.
> 
> OK, I do not mind being more explicit than necessary, but wouldn't
> "return X" take care of casting X to the expected return type of
> that function?

yes it does, but the "implicit" sign conversion will trigger a warning eitherway, I had removed the cast in v3.

Carlo