Open
Conversation
Contributor
Reviewer's GuideAdds hash-based caching for mport upgrade operations on MidnightBSD to avoid repeating expensive SQLite index and moved-lookup queries, threads the cache through recursive update calls, and bumps the library version. Sequence diagram for cached index_check and moved_lookup in mport_upgradesequenceDiagram
participant Caller
participant mport_upgrade
participant moved_cache
participant index_cache
participant mport_moved_lookup
participant mport_index_check
Caller->>mport_upgrade: call mport_upgrade(mport)
activate mport_upgrade
mport_upgrade->>mport_upgrade: ohash_init(h)
mport_upgrade->>mport_upgrade: ohash_init(moved_cache)
mport_upgrade->>mport_upgrade: ohash_init(index_cache)
loop for each pack in packs
mport_upgrade->>moved_cache: ohash_qlookup(origin)
mport_upgrade->>moved_cache: ohash_find(slot)
alt moved lookup cached
moved_cache-->>mport_upgrade: return cached entries
else not cached
mport_upgrade->>mport_moved_lookup: mport_moved_lookup(origin, entries)
alt movedEntries is NULL or empty
mport_upgrade->>moved_cache: ecalloc cache_entry
mport_upgrade->>moved_cache: ohash_insert(negative)
mport_upgrade->>mport_upgrade: continue
else entries present
mport_upgrade->>moved_cache: ecalloc cache_entry
mport_upgrade->>moved_cache: ohash_insert(positive)
end
end
alt package needs update or origin match
mport_upgrade->>index_cache: ohash_qlookup(name)
mport_upgrade->>index_cache: ohash_find(slot)
alt index_check cached
index_cache-->>mport_upgrade: return cached result
else not cached
mport_upgrade->>mport_index_check: mport_index_check(pack)
mport_index_check-->>mport_upgrade: match
mport_upgrade->>index_cache: ecalloc cache_entry
mport_upgrade->>index_cache: ohash_insert(result)
end
end
end
mport_upgrade->>mport_upgrade: free caches with efree
mport_upgrade->>mport_upgrade: ohash_delete(moved_cache)
mport_upgrade->>mport_upgrade: ohash_delete(index_cache)
deactivate mport_upgrade
mport_upgrade-->>Caller: return updated
Sequence diagram for recursive mport_update_down with shared index_cachesequenceDiagram
participant mport_upgrade
participant mport_update_down
participant index_cache
participant mport_index_check
mport_upgrade->>mport_update_down: mport_update_down(mport, pack, info, h, index_cache)
activate mport_update_down
loop for each dependency pack in depends
mport_update_down->>mport_update_down: check h for pack
alt not yet processed
mport_update_down->>mport_update_down: recurse
mport_update_down->>mport_update_down: mport_update_down(mport, depends, info, h, index_cache)
mport_update_down->>index_cache: ohash_qlookup(depends->name)
mport_update_down->>index_cache: ohash_find(slot)
alt cached result exists
index_cache-->>mport_update_down: return cached result
else no cached result
mport_update_down->>mport_index_check: mport_index_check(depends)
mport_index_check-->>mport_update_down: match
alt index_cache not NULL
mport_update_down->>index_cache: ecalloc cache_entry
mport_update_down->>index_cache: ohash_insert(result)
end
end
alt match is true
mport_update_down->>mport_update_down: perform update
mport_update_down->>mport_update_down: insert name into h
end
end
end
mport_update_down->>index_cache: ohash_qlookup(pack->name)
mport_update_down->>index_cache: ohash_find(slot)
alt cached result exists
index_cache-->>mport_update_down: return cached result
else no cached result
mport_update_down->>mport_index_check: mport_index_check(pack)
mport_index_check-->>mport_update_down: match
alt index_cache not NULL
mport_update_down->>index_cache: ecalloc cache_entry
mport_update_down->>index_cache: ohash_insert(result)
end
end
alt match is true
mport_update_down->>mport_update_down: update pack and insert name into h
end
deactivate mport_update_down
mport_update_down-->>mport_upgrade: ret
Class diagram for new cache structs and updated functionsclassDiagram
class mportInstance
class mportPackageMeta {
char* name
char* origin
int action
bool automatic
}
class mportIndexMovedEntry {
char date
char* moved_to_pkgname
}
class mportIndexEntry {
char* pkgname
}
class index_check_cache_t {
int result
bool cached
}
class moved_lookup_cache_t {
mportIndexMovedEntry** entries
bool cached
}
class ohash_info {
int unused
void* data
}
class ohash {
unsigned int size
}
class mport_upgrade {
+int mport_upgrade(mportInstance* mport)
}
class mport_update_down {
+int mport_update_down(mportInstance* mport, mportPackageMeta* pack, ohash_info* info, ohash* h, ohash* index_cache)
}
class mport_moved_lookup {
+int mport_moved_lookup(mportInstance* mport, char* origin, mportIndexMovedEntry*** movedEntries)
}
class mport_index_check {
+int mport_index_check(mportInstance* mport, mportPackageMeta* pack)
}
class memory_helpers {
+void* ecalloc(size_t size, void* data)
+void efree(void* p, size_t s1, void* data)
}
mport_upgrade --> mportInstance
mport_upgrade --> mportPackageMeta
mport_upgrade --> ohash_info
mport_upgrade --> ohash : h
mport_upgrade --> ohash : moved_cache
mport_upgrade --> ohash : index_cache
mport_upgrade --> mport_moved_lookup
mport_upgrade --> mport_index_check
mport_upgrade --> mport_update_down
mport_upgrade --> memory_helpers
mport_update_down --> mportInstance
mport_update_down --> mportPackageMeta
mport_update_down --> ohash_info
mport_update_down --> ohash : h
mport_update_down --> ohash : index_cache
mport_update_down --> mport_index_check
mport_update_down --> memory_helpers
moved_lookup_cache_t --> mportIndexMovedEntry
index_check_cache_t ..> mport_index_check
moved_lookup_cache_t ..> mport_moved_lookup
ohash ..> index_check_cache_t
ohash ..> moved_lookup_cache_t
Flow diagram for cache lifecycle in mport_upgradeflowchart TD
A[start mport_upgrade] --> B[ohash_init h]
B --> C[ohash_init moved_cache]
C --> D[ohash_init index_cache]
D --> E[iterate packs]
E --> F{pack already in h?}
F -->|yes| E
F -->|no| G[moved_cache lookup by origin]
G --> H{moved lookup cached?}
H -->|yes| I[use cached movedEntries]
H -->|no| J[mport_moved_lookup]
J --> K{entries found?}
K -->|no| L[insert negative moved_lookup_cache_t
for origin] --> E
K -->|yes| M[insert positive moved_lookup_cache_t
with entries] --> N
I --> N{movedEntries indicate move or expiry?}
N -->|yes| O[handle move or expire
set actions and continue] --> E
N -->|no| P[index_cache lookup by name]
P --> Q{index_check cached?}
Q -->|yes| R[use cached result]
Q -->|no| S[mport_index_check
store result in index_check_cache_t]
R --> T{result requires update?}
S --> T
T -->|no| E
T -->|yes| U[call mport_update_down
with shared index_cache]
U --> E
E --> V{no more packs?}
V -->|no| E
V -->|yes| W[free moved_cache entries
with efree]
W --> X[ohash_delete moved_cache]
X --> Y[free index_cache entries
with efree]
Y --> Z[ohash_delete index_cache]
Z --> AA[ohash_delete h]
AA --> AB[end mport_upgrade]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The index/moved cache lookup and insert logic is duplicated in several places in
mport_upgradeandmport_update_down; consider extracting small helper functions (e.g.,index_cache_get_or_fill(...),moved_cache_get_or_fill(...)) to make the flow easier to read and reduce the chance of inconsistencies on future changes. - Both cache structs carry a
cachedboolean even though the presence of an entry in theohashalready distinguishes cached vs. non-cached; you could simplify the code by treating a found entry as cached and encoding negative results via the stored value (e.g.,entries == NULLorresult == 0) instead of maintaining a separate flag.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The index/moved cache lookup and insert logic is duplicated in several places in `mport_upgrade` and `mport_update_down`; consider extracting small helper functions (e.g., `index_cache_get_or_fill(...)`, `moved_cache_get_or_fill(...)`) to make the flow easier to read and reduce the chance of inconsistencies on future changes.
- Both cache structs carry a `cached` boolean even though the presence of an entry in the `ohash` already distinguishes cached vs. non-cached; you could simplify the code by treating a found entry as cached and encoding negative results via the stored value (e.g., `entries == NULL` or `result == 0`) instead of maintaining a separate flag.
## Individual Comments
### Comment 1
<location> `libmport/upgrade.c:171` </location>
<code_context>
}
- if ((*movedEntries)->moved_to_pkgname != NULL && (*movedEntries)->moved_to_pkgname[0]!= '\0') {
+ if ((*movedEntries)->moved_to_pkgname[0] != '\0') {
mport_call_msg_cb(mport, "Package %s has moved to %s. Migrating %s\n", (*packs)->name, (*movedEntries)->moved_to_pkgname, (*movedEntries)->moved_to_pkgname);
(*packs)->action = MPORT_ACTION_UPGRADE;
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential NULL dereference on moved_to_pkgname after removing the NULL check.
The original code safely handled a possible NULL `moved_to_pkgname` via `(*movedEntries)->moved_to_pkgname != NULL && ...`. The new version dereferences it directly with `(*movedEntries)->moved_to_pkgname[0]`, which will crash if it’s NULL. Unless there is now a strong guarantee this pointer is always non-NULL, please restore the NULL check or enforce non-NULL earlier in the flow.
</issue_to_address>
### Comment 2
<location> `libmport/upgrade.c:126-129` </location>
<code_context>
continue;
}
+
+ /* Check cache for moved lookup */
+ slot = ohash_qlookup(&moved_cache, (*packs)->origin);
+ moved_lookup_cache_t *cached_moved = (moved_lookup_cache_t *)ohash_find(&moved_cache, slot);
+ if (cached_moved != NULL && cached_moved->cached) {
+ movedEntries = cached_moved->entries;
+ use_cached_moved = true;
</code_context>
<issue_to_address>
**issue (bug_risk):** Cached negative moved-lookup results are not handled consistently, leading to NULL dereferences.
For cached negative results, `cached_moved->cached` is true but `cached_moved->entries` is NULL. This branch then sets `movedEntries` and `use_cached_moved = true` and skips `mport_moved_lookup`, but later code assumes `movedEntries`/`*movedEntries` are non-NULL and dereferences them, causing a crash. You could either `continue` when `cached_moved->entries == NULL` (matching the non-cached negative path) or explicitly represent a negative cache entry and branch before dereferencing.
</issue_to_address>
### Comment 3
<location> `libmport/upgrade.c:64` </location>
<code_context>
free(p);
}
+/* Cache structure for index check results */
+typedef struct {
+ int result; /* 0 = no update, 1 = update available, 2 = origin match */
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new caching code by removing the explicit cached flags and extracting common cache lookup/update/cleanup logic into small helper functions to reduce duplication and conditionals.
You can keep the caching feature but significantly reduce complexity and repetition with a couple of small helpers and by simplifying the cache structs.
### 1. Drop the `cached` flag and use `NULL` as the sentinel
Right now every call does:
```c
if (cached != NULL && cached->cached) {
...
}
```
You can simplify the structs and logic so that presence in the hash means “cached”:
```c
typedef struct {
int result; /* 0 = no update, 1 = update available, 2 = origin match */
} index_check_cache_t;
typedef struct {
mportIndexMovedEntry **entries; /* NULL means "no moved entries" */
} moved_lookup_cache_t;
```
Then lookup becomes:
```c
slot = ohash_qlookup(&index_cache, (*packs)->name);
index_check_cache_t *cached = ohash_find(&index_cache, slot);
if (cached != NULL) {
match = cached->result;
use_cached_index = true;
}
```
This removes the extra boolean and the `&& cached->cached` checks everywhere.
### 2. Factor out the repeated index-check caching pattern
The `index_cache` lookup/fill pattern is repeated in `mport_upgrade` and `mport_update_down`. A small helper keeps the upgrade logic clean and encapsulates all `#if` and `ohash` details:
```c
#if defined(__MidnightBSD__)
static int
cached_index_check(mportInstance *mport,
mportPackageMeta *pack,
struct ohash *index_cache)
{
unsigned int slot;
index_check_cache_t *cached;
if (index_cache != NULL) {
slot = ohash_qlookup(index_cache, pack->name);
cached = ohash_find(index_cache, slot);
if (cached != NULL) {
return cached->result;
}
}
int match = mport_index_check(mport, pack);
if (index_cache != NULL) {
index_check_cache_t *entry = ecalloc(sizeof(*entry), NULL);
entry->result = match;
ohash_insert(index_cache, slot, entry);
}
return match;
}
#else
static int
cached_index_check(mportInstance *mport,
mportPackageMeta *pack,
void *unused)
{
(void)unused;
return mport_index_check(mport, pack);
}
#endif
```
Then all call sites become:
```c
int match = cached_index_check(mport, pack, index_cache);
```
and
```c
int match = cached_index_check(mport, *depends, index_cache);
```
This collapses the repeated `match/use_cached/#if` blocks into one place and makes the signature change to `mport_update_down` easier to justify.
### 3. Factor out moved-lookup caching
Similarly, the moved lookup caching block in `mport_upgrade` can be moved to a helper:
```c
#if defined(__MidnightBSD__)
static mportIndexMovedEntry **
cached_moved_lookup(mportInstance *mport,
const char *origin,
struct ohash *moved_cache)
{
unsigned int slot;
moved_lookup_cache_t *cached;
mportIndexMovedEntry **entries = NULL;
if (moved_cache != NULL) {
slot = ohash_qlookup(moved_cache, origin);
cached = ohash_find(moved_cache, slot);
if (cached != NULL) {
return cached->entries; /* may be NULL for negative cache */
}
}
if (mport_moved_lookup(mport, origin, &entries) != MPORT_OK) {
return NULL;
}
if (moved_cache != NULL) {
moved_lookup_cache_t *entry = ecalloc(sizeof(*entry), NULL);
entry->entries = entries; /* may be NULL: negative result */
ohash_insert(moved_cache, slot, entry);
}
return entries;
}
#else
static mportIndexMovedEntry **
cached_moved_lookup(mportInstance *mport,
const char *origin,
void *unused)
{
(void)unused;
mportIndexMovedEntry **entries = NULL;
if (mport_moved_lookup(mport, origin, &entries) != MPORT_OK)
return NULL;
return entries;
}
#endif
```
Then the loop in `mport_upgrade` becomes:
```c
mportIndexMovedEntry **movedEntries =
cached_moved_lookup(mport, (*packs)->origin, &moved_cache);
if (movedEntries == NULL || *movedEntries == NULL) {
packs++;
continue;
}
```
This removes the inline `use_cached_moved` logic and cache-entry allocation from the main control flow.
### 4. Factor out cache cleanup
The cleanup loops for both caches are almost identical. A small helper reduces boilerplate and keeps `mport_upgrade` focused on the high-level logic:
```c
#if defined(__MidnightBSD__)
static void
free_index_cache(struct ohash *cache)
{
unsigned int slot;
const char *key = NULL;
for (slot = ohash_first(cache, &key);
key != NULL;
slot = ohash_next(cache, &slot)) {
index_check_cache_t *entry = ohash_find(cache, slot);
if (entry != NULL) {
efree(entry, sizeof(*entry), NULL);
}
}
ohash_delete(cache);
}
static void
free_moved_cache(struct ohash *cache)
{
unsigned int slot;
const char *key = NULL;
for (slot = ohash_first(cache, &key);
key != NULL;
slot = ohash_next(cache, &slot)) {
moved_lookup_cache_t *entry = ohash_find(cache, slot);
if (entry != NULL) {
efree(entry, sizeof(*entry), NULL);
}
}
ohash_delete(cache);
}
#endif
```
Then:
```c
#if defined(__MidnightBSD__)
free_moved_cache(&moved_cache);
free_index_cache(&index_cache);
ohash_delete(&h);
#endif
```
This removes two near-identical loops from `mport_upgrade`.
---
These changes preserve the new caching behavior, but:
- remove the redundant `cached` flag,
- centralize the cache logic and `#if` noise in a few small helpers,
- reduce repeated patterns in `mport_upgrade` and `mport_update_down`,
- and simplify the function bodies back towards their original shape.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+126
to
+129
| /* Check cache for moved lookup */ | ||
| slot = ohash_qlookup(&moved_cache, (*packs)->origin); | ||
| moved_lookup_cache_t *cached_moved = (moved_lookup_cache_t *)ohash_find(&moved_cache, slot); | ||
| if (cached_moved != NULL && cached_moved->cached) { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Cached negative moved-lookup results are not handled consistently, leading to NULL dereferences.
For cached negative results, cached_moved->cached is true but cached_moved->entries is NULL. This branch then sets movedEntries and use_cached_moved = true and skips mport_moved_lookup, but later code assumes movedEntries/*movedEntries are non-NULL and dereferences them, causing a crash. You could either continue when cached_moved->entries == NULL (matching the non-cached negative path) or explicitly represent a negative cache entry and branch before dereferencing.
| free(p); | ||
| } | ||
|
|
||
| /* Cache structure for index check results */ |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider simplifying the new caching code by removing the explicit cached flags and extracting common cache lookup/update/cleanup logic into small helper functions to reduce duplication and conditionals.
You can keep the caching feature but significantly reduce complexity and repetition with a couple of small helpers and by simplifying the cache structs.
1. Drop the cached flag and use NULL as the sentinel
Right now every call does:
if (cached != NULL && cached->cached) {
...
}You can simplify the structs and logic so that presence in the hash means “cached”:
typedef struct {
int result; /* 0 = no update, 1 = update available, 2 = origin match */
} index_check_cache_t;
typedef struct {
mportIndexMovedEntry **entries; /* NULL means "no moved entries" */
} moved_lookup_cache_t;Then lookup becomes:
slot = ohash_qlookup(&index_cache, (*packs)->name);
index_check_cache_t *cached = ohash_find(&index_cache, slot);
if (cached != NULL) {
match = cached->result;
use_cached_index = true;
}This removes the extra boolean and the && cached->cached checks everywhere.
2. Factor out the repeated index-check caching pattern
The index_cache lookup/fill pattern is repeated in mport_upgrade and mport_update_down. A small helper keeps the upgrade logic clean and encapsulates all #if and ohash details:
#if defined(__MidnightBSD__)
static int
cached_index_check(mportInstance *mport,
mportPackageMeta *pack,
struct ohash *index_cache)
{
unsigned int slot;
index_check_cache_t *cached;
if (index_cache != NULL) {
slot = ohash_qlookup(index_cache, pack->name);
cached = ohash_find(index_cache, slot);
if (cached != NULL) {
return cached->result;
}
}
int match = mport_index_check(mport, pack);
if (index_cache != NULL) {
index_check_cache_t *entry = ecalloc(sizeof(*entry), NULL);
entry->result = match;
ohash_insert(index_cache, slot, entry);
}
return match;
}
#else
static int
cached_index_check(mportInstance *mport,
mportPackageMeta *pack,
void *unused)
{
(void)unused;
return mport_index_check(mport, pack);
}
#endifThen all call sites become:
int match = cached_index_check(mport, pack, index_cache);and
int match = cached_index_check(mport, *depends, index_cache);This collapses the repeated match/use_cached/#if blocks into one place and makes the signature change to mport_update_down easier to justify.
3. Factor out moved-lookup caching
Similarly, the moved lookup caching block in mport_upgrade can be moved to a helper:
#if defined(__MidnightBSD__)
static mportIndexMovedEntry **
cached_moved_lookup(mportInstance *mport,
const char *origin,
struct ohash *moved_cache)
{
unsigned int slot;
moved_lookup_cache_t *cached;
mportIndexMovedEntry **entries = NULL;
if (moved_cache != NULL) {
slot = ohash_qlookup(moved_cache, origin);
cached = ohash_find(moved_cache, slot);
if (cached != NULL) {
return cached->entries; /* may be NULL for negative cache */
}
}
if (mport_moved_lookup(mport, origin, &entries) != MPORT_OK) {
return NULL;
}
if (moved_cache != NULL) {
moved_lookup_cache_t *entry = ecalloc(sizeof(*entry), NULL);
entry->entries = entries; /* may be NULL: negative result */
ohash_insert(moved_cache, slot, entry);
}
return entries;
}
#else
static mportIndexMovedEntry **
cached_moved_lookup(mportInstance *mport,
const char *origin,
void *unused)
{
(void)unused;
mportIndexMovedEntry **entries = NULL;
if (mport_moved_lookup(mport, origin, &entries) != MPORT_OK)
return NULL;
return entries;
}
#endifThen the loop in mport_upgrade becomes:
mportIndexMovedEntry **movedEntries =
cached_moved_lookup(mport, (*packs)->origin, &moved_cache);
if (movedEntries == NULL || *movedEntries == NULL) {
packs++;
continue;
}This removes the inline use_cached_moved logic and cache-entry allocation from the main control flow.
4. Factor out cache cleanup
The cleanup loops for both caches are almost identical. A small helper reduces boilerplate and keeps mport_upgrade focused on the high-level logic:
#if defined(__MidnightBSD__)
static void
free_index_cache(struct ohash *cache)
{
unsigned int slot;
const char *key = NULL;
for (slot = ohash_first(cache, &key);
key != NULL;
slot = ohash_next(cache, &slot)) {
index_check_cache_t *entry = ohash_find(cache, slot);
if (entry != NULL) {
efree(entry, sizeof(*entry), NULL);
}
}
ohash_delete(cache);
}
static void
free_moved_cache(struct ohash *cache)
{
unsigned int slot;
const char *key = NULL;
for (slot = ohash_first(cache, &key);
key != NULL;
slot = ohash_next(cache, &slot)) {
moved_lookup_cache_t *entry = ohash_find(cache, slot);
if (entry != NULL) {
efree(entry, sizeof(*entry), NULL);
}
}
ohash_delete(cache);
}
#endifThen:
#if defined(__MidnightBSD__)
free_moved_cache(&moved_cache);
free_index_cache(&index_cache);
ohash_delete(&h);
#endifThis removes two near-identical loops from mport_upgrade.
These changes preserve the new caching behavior, but:
- remove the redundant
cachedflag, - centralize the cache logic and
#ifnoise in a few small helpers, - reduce repeated patterns in
mport_upgradeandmport_update_down, - and simplify the function bodies back towards their original shape.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a performance optimization for mport_upgrade to cut down on redundant sqlite queries.
Summary by Sourcery
Introduce caching of package index checks and moved package lookups in the upgrade path to reduce redundant database queries and bump the library version.
Enhancements:
Build: