system/nxpkg: Make the storage root configurable and crash-safe. - #3642
system/nxpkg: Make the storage root configurable and crash-safe.#3642aviralgarg05 wants to merge 2 commits into
Conversation
d33ae3b to
c4f639c
Compare
c4f639c to
f363a8f
Compare
aviralgarg05
changed the title
f363a8f to
45a9292
Compare
|
Rebased onto current The squash also removes an artefact of the previous history: the first commit added The diff is limited to |
|
This is a 2.5k LoC patch with 1 commit. Your PR lists a bunch of functionally different features in this PR (i.e adding an icon, adding SHA checks). Please split this into multiple commits, and ideally open a few PRs. |
I suggest dividing in more commit inside this PR, because he has already many PRs opened and the final evaluation date is arriving |
|
It won't make much difference except that the patches will be easier to review if split into many. |
I will split and stack the PRs, it will be easy for review then |
|
Thank you Aviral! |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
7641371 to
84d3bd3
Compare
The package store, the catalog and the temporary download area were fixed paths spread across /etc, /var/lib and /var/cache. A board rarely has all three, and nothing let it put them somewhere it does have, such as a removable card. Derive all of them from CONFIG_SYSTEM_NXPKG_ROOT instead. Also send the diagnostics to syslog, since a package operation started by a supervisor has no console to print to, and fall back to CONFIG_ARCH_BOARD_CUSTOM_NAME when a board defines no CONFIG_ARCH_BOARD, which is otherwise undefined rather than empty and fails to build. Assisted-by: OpenAI Codex:gpt-5.6-sol Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>
Every file the package manager owns was written in place, so a reset partway through left a truncated catalog or database behind. Locking used only the file modification time, which a card whose clock does not follow CLOCK_REALTIME reports as decades old, so a lock created a moment earlier looked stale to the next caller. Write through a temporary file and rename it, and record a per-boot token and the owner task in each lock so a contender can tell a live owner from an abandoned one. Reject a zero-byte write rather than looping on it, and add the path helpers the download and rollback paths need. Assisted-by: OpenAI Codex:gpt-5.6-sol Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>
84d3bd3 to
ca37b4d
Compare
aviralgarg05
changed the title
|
Split as asked. This PR now carries only the storage layer, and the rest went into three follow-ups:
Each declares the one before it with The content is unchanged from what you already reviewed; it is the same tree, cut into six commits across four PRs. |
| * Private Data | ||
| ****************************************************************************/ | ||
|
|
||
| static pthread_once_t g_pkg_lock_boot_once = PTHREAD_ONCE_INIT; |
There was a problem hiding this comment.
could we remove the global changed variable from nxpkg? so multiple instances could run concurrently.
|
|
||
| /* nxpkg is a one-shot CLI, not a daemon, so a lock file older than this | ||
| * cannot belong to a still-running install under normal use (even a full | ||
| * multi-MB artifact over a slow link finishes well within this window) - |
There was a problem hiding this comment.
How slow of a link? Did you test this on something or is this hypothesizing. 10 minutes seems pretty reasonable for now, but I would remove this claim.
| static inline void *pkg_malloc(size_t size) | ||
| { | ||
| return malloc(size); | ||
| } | ||
|
|
||
| static inline void *pkg_zalloc(size_t size) | ||
| { | ||
| return calloc(1, size); | ||
| } | ||
|
|
||
| static inline void *pkg_realloc(void *ptr, size_t size) | ||
| { | ||
| return realloc(ptr, size); | ||
| } | ||
|
|
||
| static inline void pkg_free(void *ptr) | ||
| { | ||
| free(ptr); | ||
| } | ||
|
|
||
| static inline FAR char *pkg_path_alloc(void) | ||
| { | ||
| return pkg_malloc(PATH_MAX); | ||
| } |
| #elif defined(CONFIG_ARCH_BOARD_CUSTOM_NAME) | ||
| if (CONFIG_ARCH_BOARD_CUSTOM_NAME[0] != '\0') | ||
| { | ||
| return CONFIG_ARCH_BOARD_CUSTOM_NAME; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Why check this if you'll just return the empty string anyways?
| } | ||
|
|
||
| syslog(strcmp(level, "error") == 0 ? LOG_ERR : LOG_INFO, | ||
| "nxpkg: %s: %s", level, message); |
There was a problem hiding this comment.
Don't add level to the message. There is already a syslog option that allows level to be logged.
I would suggest a macro that just compile time prefixes the message with nxpkg if you need that, but there is also a syslog option that allows logging the process name as well which is a better choice.
| /* Compatibility for empty lock files created by older nxpkg images. | ||
| * Their only ownership information is the filesystem timestamp. | ||
| */ | ||
|
|
||
| if (stat(path, &st) < 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| now = time(NULL); | ||
| if (now < st.st_mtime || | ||
| (now - st.st_mtime) < PKG_LOCK_STALE_SECONDS) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| pkg_error("reclaiming legacy stale lock '%s' (age %ld s)", | ||
| path, (long)(now - st.st_mtime)); | ||
| unlink(path); | ||
| } | ||
|
|
There was a problem hiding this comment.
Is compatibility necessary? This application is quite new and unfinished anyways.
| UNUSED(version); | ||
|
|
||
| /* This used to be "PKG_TMP_PKG_DIR/name-version.pkg", which breaks on | ||
| * this SD card's short-name-only FAT mount as soon as name+version |
There was a problem hiding this comment.
Don't mention "this SD card". Just refer to short name FAT. This is an application
| * unlike pkg_store_make_tmp_path()'s already-FAT-safe scheme. The | ||
| * pid is small, bounded, and unique per concurrently running install | ||
| * (each `nxpkg install` is its own process with its own per-name | ||
| * lock), so it can't collide the way a single fixed name would if | ||
| * two different packages were being installed at once. |
There was a problem hiding this comment.
This function just checks a length. We don't need to know about another function's safe naming scheme here, it's confusing. Check the AI generated comments for relevance.
5 participants
Note: Please adhere to Contributing Guidelines.
Summary
This PR has been reduced to the storage layer. The rest of what it used to
contain is now in #3718, #3719 and #3720, in that order.
The package store, the catalog and the temporary download area were fixed
paths spread across
/etc,/var/liband/var/cache. A board rarely hasall three, and nothing let it put them somewhere it does have, such as a
removable card. They are now derived from
CONFIG_SYSTEM_NXPKG_ROOT.Every file the package manager owns was also written in place, so a reset
partway through a write left a truncated catalog or database behind. Writes
now go to a temporary file that is renamed into place.
Locking used only the file modification time. A card whose clock does not
follow
CLOCK_REALTIMEreports a file created a moment ago as decades old,so a live lock looked stale to the next caller and was taken away from its
owner. Each lock now records a per-boot token and the owning task, so a
contender can tell a live owner from an abandoned one, and reclaims the
lock only in the second case.
Diagnostics go to syslog, since a package operation started by a supervisor
has no console to print to.
pkg_runtime_compat()also falls back toCONFIG_ARCH_BOARD_CUSTOM_NAME, because a board with a custom boarddirectory defines no
CONFIG_ARCH_BOARDat all, which does not build.Impact
CONFIG_SYSTEM_NXPKG_ROOT, default/var/lib/nxpkg. A board that mountswritable storage elsewhere sets that symbol.
point
CONFIG_SYSTEM_NXPKG_ROOTat the old location to keep it.Testing
Build host: macOS 26.5, arm64,
xtensa-esp-elf-gcc 14.2.0(
esp-14.2.0_20251107).Target: Xtensa / ESP32-S3, Waveshare ESP32-S3-Touch-LCD-7.
nxstyle,tools/checkpatch.sh,codespellandgit diff --checkonevery changed file
system/nxpkgsources compiled for the targetmasteron its own; the three that follow declare itwith
Depends-OnOn the target: a lock held by a running task was kept while a second caller
asked for it, and locks left by an exited task and by an earlier boot were
both reclaimed.
PR verification Self-Check