lib/gis: Return the child exit status from G_popen_close

[Pranav-error]

Fixes #7735. Draft — I asked two contract questions on the issue and haven't heard back, so this is a concrete proposal to react to rather than a settled design. Happy to change either decision.

G_popen_close() waited for the child but dropped G_wait()'s value, so callers could not tell whether the child succeeded. That is one of the links that made #7734 silent.

The two decisions I made

1. What to return when there is no child. You asked for "something distinguishable", and a plain -1 would not be: G_wait() already returns -1 when waitpid fails, and returns small positives for WTERMSIG. So this adds G_POPEN_NO_CHILD (-2), which is outside the set G_wait() can produce, next to struct Popen in gis.h.

2. Raw vs decoded status. This passes G_wait()'s value straight through, so it is consistent with the rest of the library — decoded WEXITSTATUS/WTERMSIG on POSIX, the process exit code on Windows. It is therefore not interchangeable with a pclose() result, which is raw. I preferred consistency with G_wait(); say the word if you would rather it match pclose().

One thing I added beyond the issue

fp and pid are cleared once handled. Without that, a second G_popen_close() on the same state waits on an already-reaped pid, gets -1 from waitpid, and reports a failure for a pipe that closed fine. That only became observable once the value was returned, so it seemed to belong here — but I am happy to drop it if you would rather keep the diff to the return value alone.

Compatibility

All 33 call sites of G_popen_close() outside popen.c ignore the return value today, so void → int does not require touching any of them. A full build shows no errors.

Verification

Exercised through the real API against a built library:

true      -> status=0   (expect 0)
false     -> status=1   (expect 1)
sh -c "echo hello; exit 3" -> read="hello" status=3   (expect 3)
no child  -> status=-2  (G_POPEN_NO_CHILD)
double close #1 -> status=0
double close #2 -> status=-2  (not -1)

clang-format reports no changes.

I used an AI assistant while preparing this. I understand the change and can explain it.

Source compatibility of the void -> int change

Verified: G_popen_close has call sites in 18 files outside popen.c and the header — lib/gis/pager.c, raster/r.mode, r.rescale, r.rescale.eq, r.coin, r.topmodel and the eleven raster/r.statistics/o_*.c — and every one discards the value. No assignment, no use in a condition, no return. So widening the return type breaks nothing in tree.

Thanks to @Valyrian-Code for prompting both this and the Windows correction below.

[Pranav-error]

@wenzeslaus taking this out of draft, since the two questions I asked on #7735 are easier to answer against code than in the abstract, and leaving it as a draft mostly means nobody looks.

The defaults it ships with are G_POPEN_NO_CHILD (-2) for the no-child case, and G_wait()'s decoded value passed straight through. Both are one-line changes if you would rather have something else — I am not attached to either, I just needed to pick something to make the rest reviewable.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Pranav-error]

Thanks — I checked both, and it makes the guard more important than I had treated it.

G_wait() does not special-case its argument. It goes straight to waitpid(pid, &status, 0), and waitpid gives -1 a dedicated meaning: wait for any child, not "no child". So G_wait(-1) on a process that has other children blocks until one of them exits, reaps it, and then — because n != pid — returns -1 as though the wait had failed. It both steals an unrelated child and reports failure for it. Only when the process has no children at all does it return ECHILD:

process has NO children at all:
  waitpid(-1)     -> n=-1 errno=No child processes
  waitpid(99999)  -> n=-1 errno=No child processes

That matters here because do_popen() and G_popen_clear() both set state->pid = -1, so a struct Popen that was cleared, or whose pipe()/fork() failed, carries exactly the value waitpid treats as a wildcard. The existing if (state->pid != -1) guard is what stops that, and I have kept it — but it is load-bearing rather than defensive, which I had not spelled out.

It also answers the question the other way round from how I framed it in the description. I justified G_POPEN_NO_CHILD by saying -1 would be ambiguous with G_wait()'s failure return. The stronger reason is that the no-child case can never reach G_wait() at all — it must not, or we reap somebody else's child — so it needs a value of its own regardless of what G_wait() returns.

On the range: G_wait() can return WEXITSTATUS (0-255), WTERMSIG (positive), -1, or -0x100. -2 does not collide with any of those.

One thing I noticed while checking, which is outside this PR: G_wait() itself has no guard, so any caller that passes an uninitialised or cleared pid gets the wildcard behaviour. Happy to open a separate issue for that if you think it is worth one.

[Valyrian-Code]

Not a maintainer, but you asked for reactions and this has been sitting a while.

On the API change: there are 32 call sites of G_popen_close in the tree (pager.c, r.coin, r.rescale, r.rescale.eq, r.mode, r.topmodel, r.statistics/o_*.c) and none of them use the return value, so void to int is source compatible. Might be worth saying in the description, since that is the first thing a reviewer would check.

On the sentinel: the comment says G_POPEN_NO_CHILD is distinct from what G_wait() can return. On POSIX that holds (0-255, a signal number, -1, or -0x100). On Windows G_wait() does return (int)exitcode; from a DWORD, so a child exiting 0xFFFFFFFE gives exactly -2 and you cannot tell it from "no child". Unlikely exit code, but the comment states it as a property. -0x101 would avoid it, or just soften the wording.

state->pid = -1 after the wait looks right, it matches what G_popen_clear() and do_popen() already do. The docblock might mention that a second close now returns G_POPEN_NO_CHILD instead of waiting again, since that is the part a caller could get wrong.

I have not built this, so the Windows bit is read from source, not reproduced.

[Pranav-error]

Thanks — the Windows point is a real error in the patch, and I checked all three rather than taking them on trust.

The sentinel. You are right, and it is worse than you put it. G_wait() on Windows does return (int)exitcode; from a DWORD, so the cast spans the whole int range:

child exits 0xFFFFFFFE (4294967294)  ->  (int) = -2
child exits 0xFFFFFEFF (4294967039)  ->  (int) = -257
child exits 0xFFFFFFFF (4294967295)  ->  (int) = -1

So -0x101 would not fix it either — it is reachable from 0xFFFFFEFF. No sentinel value is provably distinct on Windows, because every int is a possible return. Swapping the constant would just move the collision somewhere less likely while leaving the docblock making a claim that is still false.

So I have kept -2 and corrected the wording instead: distinct from everything G_wait() can return on POSIX, and on Windows distinct from every exit code except the one that casts to it. That is the accurate statement, and it is the one a caller needs in order to decide whether they care.

The 32 call sites. Confirmed — 18 files outside popen.c and the header, and every one discards the value:

lib/gis/pager.c:37, 67 · raster/r.mode/main.c:135, 136 · raster/r.rescale/main.c:150
raster/r.coin/make_coin.c:72 · raster/r.topmodel/topmodel.c:72 · raster/r.statistics/o_*.c ...

No assignment, no condition, no return, so void -> int is source compatible. Agreed it belongs in the description — added.

The docblock. Also agreed. A second close returning G_POPEN_NO_CHILD rather than waiting again is the part a caller could get wrong, so it is now stated.

Appreciate the review — the Windows cast is not something I would have found by reading the POSIX branch, which is all I had been looking at.

[nilason]

Let us keep this as simple as possible. What we want here is G_popen_close to return the exit status of the G_wait.
waitpid can fail with either ECHILD, EINTR and EINVAL. EINTR is taken care of by G_wait, EINVAL is not an issue (options is 0), and ECHILD is dealt with:

grass/lib/gis/spawn.c

Lines 974 to 975 in 55de52a

	if (n != pid)
	return -1;

A caller code could look like:

int res = G_popen_close(&child);
if (res != EXIT_SUCCESS)
    G_fatal_error(_("child process failed with code: %d"), res);

(should have linked to posix wait for a more comprehensive description)

[nilason]

See #7912

[Pranav-error]

Agreed — simplified to exactly that. G_POPEN_NO_CHILD is gone and the function just passes G_wait()'s value through, with -1 when there was no child:

int G_popen_close(struct Popen *state)
{
    int status = -1;

    if (state->fp) {
        fclose(state->fp);
        state->fp = NULL;
    }

    if (state->pid != -1) {
        status = G_wait(state->pid);
        state->pid = -1;
    }

    return status;
}

Your caller shape works unchanged, and it also disposes of the Windows problem @Valyrian-Code found — with no sentinel there is nothing for an exit code to collide with. Net -18 lines, +3.

One thing I have kept, and I want to flag it rather than have it look like leftover defensiveness: the state->pid != -1 guard. waitpid reads -1 as "any child" rather than "no child", so calling G_wait() on a cleared or never-opened struct Popen would block until some unrelated child exited, reap it, and then return -1 because n != pid. do_popen() and G_popen_clear() both set pid = -1, so that state is reachable. The guard is what keeps -1 meaning "no child" instead of "went and took somebody else's".

Thanks for #7912 — the missing G_wait() documentation is exactly what sent me down the sentinel path in the first place.

[nilason]

I suggest a more concrete description of the function, and clearing up state with G_popen_clear. One important difference is the default return value of 0. A state with state->pid != -1 is already a very defensive guard. G_popen_* functions both return state with that value if G_spawn_ex fails, and returns NULL, which must be checked long before closing the pipe with G_popen_close. Let us keep this as close as possible to the original void functionality.

If the review suggestion snippets are difficult to follow, here is my suggestion as a whole:

int G_popen_close(struct Popen *state)
{
    int status = 0;

    if (state->fp)
        fclose(state->fp);

    if (state->pid != -1)
        status = G_wait(state->pid);

    G_popen_clear(state);

    return status;
}

[Pranav-error]

Taken as written — your version is in, docblock included, and the branch is rebased onto current main.

Your reasoning on the default settles the point I had been arguing from the wrong end. I was treating a state with pid == -1 as something the function had to report on, but G_popen_read() and G_popen_write() already return NULL when G_spawn_ex() fails, so a caller that has a usable struct Popen at all has a child. Returning 0 there keeps it closest to the previous void behaviour, which is the right thing to optimise for.

G_popen_clear() is also plainly better than clearing the two fields by hand — I had not thought to reuse it.

The net change is now 15 insertions and 19 deletions, so the file is shorter than before.

[nilason]

Thanks!

[Pranav-error]

@nilason thanks for the review and the rewrite — it is a better shape than what I had.

This is approved with CI green, so I think it just needs someone with the button. Flagging it rather than because I am in a hurry with it: new contributors here are limited to five open non-draft PRs and I am at that limit, so #7853 is sitting as a draft waiting for a slot, as is the testing-guide section @wenzeslaus asked for in #7862. Merging this frees one.

No rush if there is a reason to hold it.