Index bugfix and papercuts #224
Closed
Conversation
This file contains 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
evelikov
force-pushed
the
index-papercuts
branch
from
e0948ed to
422af6b
Compare
evelikov
force-pushed
the
index-papercuts
branch
from
422af6b to
87a36eb
Compare
|
Some brown paperbag fixes 🤦 |
stoeckmann
reviewed
Nov 4, 2024
For non-alias indexes prefix is an empty string, where strbuf_pushchars() returns the number of characters added to the strbuf. Since those are zero, we end up completely skipping the dump process. Cc: Tobias Stoeckmann <[email protected]> Fixes: 889d02b ("libkmod: check strbuf return values") Signed-off-by: Emil Velikov <[email protected]>
As seen with previous commit, the way we pass an empty string to
index_{,mm_}dump() is very error prone.
Swap that with a bool, which makes things a lot more obvious.
Signed-off-by: Emil Velikov <[email protected]>
Signed-off-by: Emil Velikov <[email protected]>
The compiler already creates an empty string as applicable, drop the extra variable. Signed-off-by: Emil Velikov <[email protected]>
Create a simple helper and reuse it instead of duplicating code. Signed-off-by: Emil Velikov <[email protected]>
The callers are interested if the file is within the list of builtins or not. So change the return type and push the free() down the stack. Signed-off-by: Emil Velikov <[email protected]>
The index_mm_{search,read,dump}* API does not mutate the index_mm
struct. So let's annotate it as constant.
Signed-off-by: Emil Velikov <[email protected]>
evelikov
force-pushed
the
index-papercuts
branch
from
87a36eb to
113feec
Compare
stoeckmann
suggested changes
Nov 4, 2024
|
@stoeckmann are you ok with this version? |
Won't stop merge process. I'd prefer |
lucasdemarchi
pushed a commit
that referenced
this pull request
Nov 7, 2024
For non-alias indexes prefix is an empty string, where strbuf_pushchars() returns the number of characters added to the strbuf. Since those are zero, we end up completely skipping the dump process. Cc: Tobias Stoeckmann <[email protected]> Fixes: 889d02b ("libkmod: check strbuf return values") Signed-off-by: Emil Velikov <[email protected]> Link: #224 Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi
pushed a commit
that referenced
this pull request
Nov 7, 2024
As seen with previous commit, the way we pass an empty string to
index_{,mm_}dump() is very error prone.
Swap that with a bool, which makes things a lot more obvious.
Signed-off-by: Emil Velikov <[email protected]>
Link: #224
Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi
pushed a commit
that referenced
this pull request
Nov 7, 2024
Signed-off-by: Emil Velikov <[email protected]> Link: #224 Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi
pushed a commit
that referenced
this pull request
Nov 7, 2024
The compiler already creates an empty string as applicable, drop the extra variable. Signed-off-by: Emil Velikov <[email protected]> Link: #224 Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi
pushed a commit
that referenced
this pull request
Nov 7, 2024
Create a simple helper and reuse it instead of duplicating code. Signed-off-by: Emil Velikov <[email protected]> Link: #224 Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi
pushed a commit
that referenced
this pull request
Nov 7, 2024
The callers are interested if the file is within the list of builtins or not. So change the return type and push the free() down the stack. Signed-off-by: Emil Velikov <[email protected]> Link: #224 Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi
pushed a commit
that referenced
this pull request
Nov 7, 2024
The index_mm_{search,read,dump}* API does not mutate the index_mm
struct. So let's annotate it as constant.
Signed-off-by: Emil Velikov <[email protected]>
Link: #224
Signed-off-by: Lucas De Marchi <[email protected]>
|
@stoeckmann agreed. I created #233 so we don't forget. Let's see what is the clang-analyzer situation with that. Applied, thanks. |
|
We need tests for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
While looking at how to make the index API properly handle error conditions, I noticed that we completely broke the API.
The first commit fixes that + reintroduces earlier proposal of mine. The initial counter argument had a good point but was a bit absolute (aka thinking of a extreme case, that isn't applicable to us).
The rest are bunch of paper cuts, that I've accumulated as part of the initial goal.
/cc @stoeckmann can you check the first commit? can I offer some 🍪 for adding an in-tree test?
Random notes:
index_value::priorityis ignored when dumping... unlike thedepmodequivalent