refactor: stage install files to cache before processing#1646
refactor: stage install files to cache before processing#1646dengbo11 wants to merge 1 commit intoOpenAtom-Linyaps:masterfrom
Conversation
Replace file descriptor-based approach with path-based approach for install file handling. Now the PackageManager stages incoming D-Bus file descriptors into a cache directory and passes the cached file path to subsequent processing. This ensures reliable file access throughout the installation pipeline, as file descriptors from D-Bus can be unreliable. Added CachedInstallFile RAII wrapper for automatic cache cleanup on destruction. Log: Changed install file handling from file descriptor to cached file path approach for improved reliability Influence: 1. Test install from layer file with valid .layer file 2. Test install from UAB file with valid .uab file 3. Verify cached files are created in the cache directory 4. Verify cached files are automatically cleaned up after installation 5. Test with invalid file types and verify proper error handling 6. Verify file descriptor leaks are prevented during installation 7. Test that UAB installation hooks receive correct file path
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengbo11 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the package installation process to use path-based file handling instead of raw file descriptors. It introduces a CachedInstallFile class to manage temporary staged files and updates UABFile and InstallHookManager to operate on file paths. Feedback is provided regarding potential infinite loops in the file reading and writing logic where return values of 0 (EOF or no bytes written) are not explicitly handled.
| auto readBytes = this->read(reinterpret_cast<char *>(buf.data()), bytesRead); | ||
| if (readBytes == -1) { | ||
| if (errno == EINTR) { | ||
| errno = 0; | ||
| continue; | ||
| } | ||
| return LINGLONG_ERR( | ||
| fmt::format("read from sign section error: {}", common::error::errorString(errno))); | ||
| fmt::format("read from sign section error: {}", this->errorString().toStdString())); | ||
| } |
There was a problem hiding this comment.
If this->read returns 0 (EOF) before totalBytes is reached, the loop will hang indefinitely because totalBytes is never decremented. You should check for an unexpected EOF and return an error.
auto readBytes = this->read(reinterpret_cast<char *>(buf.data()), bytesRead);
if (readBytes <= 0) {
return LINGLONG_ERR(
fmt::format("read from sign section error: {}", readBytes == 0 ? "unexpected EOF" : this->errorString().toStdString()));
}| auto bytesWritten = | ||
| ::write(outputFd, buffer.data() + bytesWrittenTotal, bytesRead - bytesWrittenTotal); |
There was a problem hiding this comment.
The write system call can theoretically return 0 if no bytes were written, which would lead to an infinite loop here. While rare for regular files, it is safer to check for bytesWritten <= 0 to prevent potential hangs.
auto bytesWritten =
::write(outputFd, buffer.data() + bytesWrittenTotal, bytesRead - bytesWrittenTotal);
if (bytesWritten <= 0) {
2 participants
Replace file descriptor-based approach with path-based approach for install file handling. Now the PackageManager stages incoming D-Bus file descriptors into a cache directory and passes the cached file path to subsequent processing. This ensures reliable file access throughout the installation pipeline, as file descriptors from D-Bus can be unreliable. Added CachedInstallFile RAII wrapper for automatic cache cleanup on destruction.
Log: Changed install file handling from file descriptor to cached file path approach for improved reliability
Influence: