Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/kargs: allow empty-list arguments #1785

Closed
wants to merge 1 commit into from

Conversation

lucab
Copy link
Member

@lucab lucab commented Dec 10, 2018

This adds support for empty-list arguments (e.g. acpi_osi=), which
are semantically different from simple-keyword arguments.

Ref: coreos/rpm-ostree#1706

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Just one small indentation nit.

src/libostree/ostree-kernel-args.c Outdated Show resolved Hide resolved
@cgwalters
Copy link
Member

Yep, LGTM otherwise. I tried to push a fixup! but:

To ssh://github.com/lucab/ostree
 ! [remote rejected]   lucab/ups/kargs-empty-list -> lucab/ups/kargs-empty-list (permission denied)

Do you explicitly uncheck the "allow pushes to your PR" box?

This adds support for empty-list arguments (e.g. `acpi_osi=`), which
are semantically different from simple-keyword arguments.

Ref: coreos/rpm-ostree#1706
@cgwalters
Copy link
Member

@rh-atomic-bot r+ ba0645c

@rh-atomic-bot
Copy link

⌛ Testing commit ba0645c with merge 3ecbdd8...

@lucab
Copy link
Member Author

lucab commented Dec 10, 2018

@cgwalters nothing specific on this PR/branch (maybe some other default?). Also, the "allow edits from maintainers" box is checked here. Not sure why you can't push.

@dustymabe
Copy link
Contributor

thanks @lucab

@lucab
Copy link
Member Author

lucab commented Dec 10, 2018

(Side note, this module looks like a good low hanging fruit candidate for oxidation, in case there are future plans for that)

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing 3ecbdd8 to master...

@cgwalters
Copy link
Member

(Side note, this module looks like a good low hanging fruit candidate for oxidation, in case there are future plans for that)

I think we should raise that on the mailing list...it was last discussed nearly 2 years ago. I am obviously in favor of oxidizing more here but I want to be sure enough of our users are ready for it.

@jlebon
Copy link
Member

jlebon commented Jan 19, 2019

Potential regression: https://pagure.io/teamsilverblue/issue/66

@lucab
Copy link
Member Author

lucab commented Jan 19, 2019

@jlebon likely correlated, yes. However the test cases in this PR should be exercising that exact codepath.

jlebon added a commit to jlebon/rpm-ostree that referenced this pull request Mar 21, 2019
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
rh-atomic-bot pushed a commit to coreos/rpm-ostree that referenced this pull request Mar 23, 2019
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 the
`rpmostree kargs --append KEYWORD` case (#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: #1779

Closes: #1796
Approved by: cgwalters
@lucab lucab deleted the ups/kargs-empty-list branch January 4, 2022 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants