Copilot commented on code in PR #1678:
URL: https://github.com/apache/cloudberry/pull/1678#discussion_r3130089123
##########
src/interfaces/libpq/fe-secure-openssl.c:
##########
@@ -1133,6 +1132,7 @@ initialize_SSL(PGconn *conn)
/* Colon, but not in second character, treat as
engine:key */
char *engine_str = strdup(conn->sslkey);
char *engine_colon;
+ EVP_PKEY *pkey;
Review Comment:
`pkey` is now declared without initialization. If any error/cleanup path in
this scope frees or checks `pkey` before it’s assigned (or if assignment is
conditional), this becomes undefined behavior. Initialize it to `NULL` at
declaration (and keep cleanup logic consistent).
```suggestion
EVP_PKEY *pkey = NULL;
```
##########
gpcontrib/gp_stats_collector/Makefile:
##########
@@ -7,7 +7,7 @@ PROTO_BASES = gpsc_plan gpsc_metrics gpsc_set_service
PROTO_OBJS = $(patsubst %,src/protos/%.pb.o,$(PROTO_BASES))
C_OBJS = $(patsubst %.c,%.o,$(wildcard src/*.c src/*/*.c))
-CPP_OBJS = $(patsubst %.cpp,%.o,$(wildcard src/*.cpp src/*/*.cpp))
+CPP_OBJS = $(patsubst %.cpp,%.o,$(wildcard src/*.cpp src/log/*.cpp
src/memory/*.cpp))
Review Comment:
Hard-coding specific subdirectories makes the build fragile: adding a new
C++ source directory under `src/` won’t be picked up, and the omission may
silently drop objects. If the intent is to exclude `src/protos/*.cpp` (to avoid
duplicate compilation with `PROTO_OBJS`), consider keeping a broad wildcard and
explicitly `filter-out` the proto-generated paths instead.
```suggestion
CPP_OBJS = $(filter-out $(PROTO_OBJS),$(patsubst %.cpp,%.o,$(wildcard
src/*.cpp src/*/*.cpp)))
```
##########
src/backend/utils/adt/xml.c:
##########
@@ -65,6 +65,16 @@
#if LIBXML_VERSION >= 20704
#define HAVE_XMLSTRUCTUREDERRORCONTEXT 1
#endif
+
+/*
+ * libxml2 2.12 decided to insert "const" into the error handler API.
+ */
+#if LIBXML_VERSION >= 21200
+#define PgXmlErrorPtr const xmlError *
+#else
+#define PgXmlErrorPtr xmlErrorPtr
Review Comment:
Using a macro as a type alias can lead to harder-to-debug errors (e.g.,
unexpected expansion in other macro contexts) and less clear diagnostics.
Prefer a conditional `typedef` (or a guarded `typedef` block) for
`PgXmlErrorPtr` rather than `#define`.
```suggestion
typedef const xmlError *PgXmlErrorPtr;
#else
typedef xmlErrorPtr PgXmlErrorPtr;
```
##########
devops/build/automation/cloudberry/scripts/configure-cloudberry.sh:
##########
@@ -121,7 +121,7 @@ log_section "Initial Setup"
execute_cmd sudo rm -rf ${BUILD_DESTINATION} || exit 2
execute_cmd sudo chmod a+w /usr/local || exit 2
execute_cmd sudo mkdir -p ${BUILD_DESTINATION}/lib || exit 2
-if [[ "$OS_ID" == "rocky" && "$OS_VERSION" =~ ^(8|9) ]]; then
+if [[ "$OS_ID" == "rocky" && "$OS_VERSION" =~ ^(8|9|10) ]]; then
Review Comment:
This regex will also match versions like `100` (because it’s not
end-anchored). To make the match precise while still allowing minor versions,
consider anchoring (e.g., `^(8|9|10)(\\..*)?$`) based on how `$OS_VERSION` is
formatted in your environment.
```suggestion
if [[ "$OS_ID" == "rocky" && "$OS_VERSION" =~ ^(8|9|10)(\..*)?$ ]]; then
```
##########
contrib/xml2/xpath.c:
##########
@@ -74,8 +74,6 @@ pgxml_parser_init(PgXmlStrictness strictness)
/* Initialize libxml */
xmlInitParser();
Review Comment:
Removing `xmlLoadExtDtdDefaultValue = 1;` changes parsing behavior:
documents relying on external DTDs for entity declarations may no longer work,
while later calls now only pass `XML_PARSE_NOENT` (entity substitution) without
explicitly enabling external DTD loading. If preserving prior behavior is
required, consider adding the appropriate `xmlReadMemory()` options; if the
behavior change is intentional (e.g., to avoid deprecated globals / improve
security), it should be documented since it can be user-visible for `xml2`
functions.
--
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]
