Copilot commented on code in PR #14447:
URL: https://github.com/apache/cloudstack/pull/14447#discussion_r4325622476
##########
services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java:
##########
@@ -276,13 +276,24 @@ public CreateEntityDownloadURLAnswer
handleCreateEntityURLCommand(CreateEntityDo
// Create the directory structure so that its visible under apache
server root
String extractDir = BASE_EXTRACT_PATH;
extractDir = extractDir + cmd.getFilepathInExtractURL() +
File.separator;
+ // Ensure the base extract directory exists and is owned by www-data
so that
+ // the www-data user can create the per-entity subdirectory inside it.
+ Script baseDirCommand = new Script("/bin/bash", logger);
+ baseDirCommand.add("-c");
+ baseDirCommand.add("mkdir -p " + BASE_EXTRACT_PATH + " && chown
www-data:www-data " + BASE_EXTRACT_PATH);
Review Comment:
Building a `bash -c` command via string concatenation can introduce shell
parsing/injection issues (and can break on spaces/special characters in the
path). Prefer invoking commands without a shell (e.g., run `mkdir` and `chown`
as separate `Script` executions with arguments), or use a single purpose-built
command like `install -d -o www-data -g www-data <path>` to avoid `bash -c`
entirely.
##########
services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java:
##########
@@ -276,13 +276,24 @@ public CreateEntityDownloadURLAnswer
handleCreateEntityURLCommand(CreateEntityDo
// Create the directory structure so that its visible under apache
server root
String extractDir = BASE_EXTRACT_PATH;
extractDir = extractDir + cmd.getFilepathInExtractURL() +
File.separator;
+ // Ensure the base extract directory exists and is owned by www-data
so that
+ // the www-data user can create the per-entity subdirectory inside it.
+ Script baseDirCommand = new Script("/bin/bash", logger);
+ baseDirCommand.add("-c");
+ baseDirCommand.add("mkdir -p " + BASE_EXTRACT_PATH + " && chown
www-data:www-data " + BASE_EXTRACT_PATH);
Review Comment:
This will `chown` the base directory on every URL creation attempt, which
can be unnecessarily expensive and may override an operator’s intentional
ownership settings. Consider only setting ownership when the directory is newly
created, or only when ownership is not already `www-data:www-data` (or use
`install -d -o www-data -g www-data ...`, which is typically idempotent while
avoiding repeated ownership changes when not needed).
##########
services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java:
##########
@@ -276,13 +276,24 @@ public CreateEntityDownloadURLAnswer
handleCreateEntityURLCommand(CreateEntityDo
// Create the directory structure so that its visible under apache
server root
String extractDir = BASE_EXTRACT_PATH;
extractDir = extractDir + cmd.getFilepathInExtractURL() +
File.separator;
+ // Ensure the base extract directory exists and is owned by www-data
so that
+ // the www-data user can create the per-entity subdirectory inside it.
+ Script baseDirCommand = new Script("/bin/bash", logger);
+ baseDirCommand.add("-c");
+ baseDirCommand.add("mkdir -p " + BASE_EXTRACT_PATH + " && chown
www-data:www-data " + BASE_EXTRACT_PATH);
+ String result = baseDirCommand.execute();
+ if (result != null) {
+ String errorString = "Error in creating base extract directory ="
+ result;
Review Comment:
The error message doesn’t include the failing path (`BASE_EXTRACT_PATH`),
which makes diagnosis harder. Include the directory path (and ideally the
attempted operation) in the message so operators can immediately see what
failed.
--
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]
