Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 2, 2022, 22:46 UTC
Message-ID: <221202.86y1rpe7ma.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y4pl5BzQnw0Fm+5S@tapette.crustytoothpaste.net>

On Fri, Dec 02 2022, brian m. carlson wrote:

Show 26 quoted lines

> [[PGP Signed Part:Undecided]]
> On 2022-12-02 at 18:10:57, Ævar Arnfjörð Bjarmason wrote:
>> 
>> But (and especially if you're interested) we really should follow-up
>> here and fix the "error()" etc. part of this. After this we have cases
>> in-tree where we on failure:
>> 
>>  * Call die_errno() (good)
>>  * Call die(), error() etc., but with a manual strerror() argument,
>>    these should just use the *_errno() helper.
>>  * Don't report on the errno at all, e.g. in this case shown here.
>> 
>> It seems to me that all of these should be using die_errno(),
>> error_errno() etc.
>
> Actually, I don't think that's correct.
>
>> Or maybe it's the other way around, and we should not rely on the global
>> "errno", but always capture the return value, and give that to
>> strerror() (or set "errno = ret", and call {die,error,warning}_errno()).
>
> Yeah, I think we need to do this.  That's because unlike most other
> functions, the pthread functions _don't_ set errno, and instead return
> the error value.  That's why on a typical Unix system, we would have
> never failed before this patch: because errno values are always
> positive.

I was skimming the POSIX docs earlier, which seem to indicate that you're not promised anyhting about "errno" being set, just the return value.

But at the same time I was reading glibc's pthread implementation, where a lot of the time (but not all the time!) you'll also get errno, just as an artifact of the library carrying forward an error from an internal API which failed while setting errno (e.g. malloc()).

In any case, the best thing to do for our codebase is probably:

	if ((errno = pthread_create(...)))
        	die_errno(...);

Since that gives our usag.[ch] library the chance to do something more clever than doing the same strerror() formatting hardcoded at every callsite.