libpriv/kargs: Copy libostree patch to support KEYWORD kargs #1796
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
This is essentially a copy of ostreedev/ostree#1785 to our private copy of the kargs API. Should really dedupe those... The really confusing part though was that that patch was intended to fix the `rpm-ostree kargs --append EMPTYKEY=` case (coreos#1706), yet the `rpmostree kargs --append KEYWORD` case (coreos#1779) wasn't also fixed, even though that same ostree patch clearly fixes and tests for that too. To make a long story short, we were passing buggy kargs to ostree, which before that patch, had itself buggy kargs parsing which conveniently fixed back the kargs we passed for the `KEYWORD` case. Closes: coreos#1779
|
Hmm OK, this is failing tests (I should probably have added some for this anyway). Will check tomorrow. |
|
My bad for not realizing rpm-ostree has an internal copy of kargs.c. It looks this direct cherry-pick failed with a segfault. |
That's the latest available in (f28, el8). Prep for using some new APIs without tripping compiler warnings.
We don't need to modify the key we're passed, so just use `const`.
When we expect a test to not fail, let's `g_assert_no_error` first so that if it fails, we get an exact printout of what error we encountered when we expected none.
Note this patch only touches the *new* APIs that aren't part of libostree. Now that we can use `g_ptr_array_find_with_equal_func`, we can drop our custom `_ostree_ptr_array_find`. Also strengthen our handling of values everywhere to handle the `NULL` case and properly support `KEYWORD` args. I ended up getting rid of `_ostree_kernel_arg_query_status` in the process since it made that assumption a lot and overall added more complexity than necessary.
No worries! Honestly, I had forgotten myself when I started looking into #1779. Anyway, the segfault was a symptom of the new APIs we have here that weren't ready to handle I started on ostreedev/ostree#1833 and upstreaming the new APIs here but don't have the cycles right now to add docstrings to every new API. Might get back to that later (though it'd be a good issue for someone who wanted to get familiar with the ostree <--> rpm-ostree interface!). |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Mar 23, 2019
That's the latest available in (f28, el8). Prep for using some new APIs without tripping compiler warnings. Closes: #1796 Approved by: cgwalters
rh-atomic-bot
pushed a commit
that referenced
this pull request
Mar 23, 2019
We don't need to modify the key we're passed, so just use `const`. Closes: #1796 Approved by: cgwalters
rh-atomic-bot
pushed a commit
that referenced
this pull request
Mar 23, 2019
When we expect a test to not fail, let's `g_assert_no_error` first so that if it fails, we get an exact printout of what error we encountered when we expected none. Closes: #1796 Approved by: cgwalters
rh-atomic-bot
pushed a commit
that referenced
this pull request
Mar 23, 2019
Note this patch only touches the *new* APIs that aren't part of libostree. Now that we can use `g_ptr_array_find_with_equal_func`, we can drop our custom `_ostree_ptr_array_find`. Also strengthen our handling of values everywhere to handle the `NULL` case and properly support `KEYWORD` args. I ended up getting rid of `_ostree_kernel_arg_query_status` in the process since it made that assumption a lot and overall added more complexity than necessary. Closes: #1796 Approved by: cgwalters
|
☀️ Test successful - status-atomicjenkins |
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.
This is essentially a copy of
ostreedev/ostree#1785 to our private copy of the
kargs API. Should really dedupe those...
The really confusing part though was that that patch was intended to fix
the
rpm-ostree kargs --append EMPTYKEY=case (#1706), yet therpmostree kargs --append KEYWORDcase (#1779) wasn't also fixed, eventhough that same ostree patch clearly fixes and tests for that too.
To make a long story short, we were passing buggy kargs to ostree, which
before that patch, had itself buggy kargs parsing which conveniently
fixed back the kargs we passed for the
KEYWORDcase.Closes: #1779