avamingli commented on code in PR #1589:
URL: https://github.com/apache/cloudberry/pull/1589#discussion_r2871623270
##########
src/bin/initdb/initdb.c:
##########
@@ -2054,12 +2055,16 @@ setup_cdb_schema(FILE *cmdfd)
errno = 0;
#endif
- closedir(dir);
-
- if (errno != 0)
+ if (errno)
{
- /* some kind of I/O error? */
pg_log_error("error while reading cdb_init.d directory: %m");
+ closedir(dir);
+ exit(1);
+ }
+
+ if (closedir(dir))
+ {
+ pg_log_error("error while closing cdb_init.d directory: %m");
Review Comment:
closedir() rarely fails in practice — we have plenty of code that doesn't
check its return value — but it's fine to keep the check here, no objection.
##########
src/bin/initdb/initdb.c:
##########
Review Comment:
Good Catch.
But the current approach still has the same stale errno problem — just from
a different source.
With errno = 0 set only once before the loop, consider: if pg_realloc() or
pg_strdup() internally sets errno (e.g. via mmap/mremap fallback paths) during,
say, the 8th iteration, and subsequent readdir() calls succeed (success does
not clear errno), then when the loop ends normally with readdir() returning
NULL, errno still holds the stale value from pg_realloc(). The if (errno) check
then misinterprets it as a readdir failure — which is the same class of bug
this patch is trying to fix.
The comma-expression idiom resets errno before every readdir() call, so when
the loop exits, errno can only come from the final readdir():
```c
while (errno = 0, (file = readdir(dir)) != NULL)
```
This is the standard pattern in Postgres.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
