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

Parse Spaces in kargs Input #4821

Closed
czhang03 opened this issue Feb 8, 2024 · 7 comments · Fixed by ostreedev/ostree#3208
Closed

Parse Spaces in kargs Input #4821

czhang03 opened this issue Feb 8, 2024 · 7 comments · Fixed by ostreedev/ostree#3208
Assignees
Labels
jira for syncing to jira

Comments

@czhang03
Copy link

czhang03 commented Feb 8, 2024

Describe the bug

When send in multiple kargs input in a single string, the kargs seems to treat it as a single key value pair. For example run rpm-ostree karg --delete "init_on_alloc=1 init_on_free=1" will delete neither parameter, and rpm-ostree karg --append-if-missing "init_on_alloc=1 init_on_free=1" will duplicate these two kargs even when they exists

Reproduction steps

  1. run rpm-ostree karg --append "init_on_alloc=1 init_on_free=1", notice that init_on_alloc=1 init_on_free=1 gets added to kargs
  2. run rpm-ostree karg --append-if-missing "init_on_alloc=1 init_on_free=1", will duplicate these two kargs even when they are already present
  3. run rpm-ostree karg --delete "init_on_alloc=1 init_on_free=1" will give an error stating the argument is not found

Expected behavior

rpm-ostree karg --append-if-missing "init_on_alloc=1 init_on_free=1" should not duplicate the kargs, and rpm-ostree karg --delete "init_on_alloc=1 init_on_free=1" should delete the kargs when they are present.

Or at least raise an error/warning to the user about invalid input.

Actual behavior

Described in "Reproduction steps" section

System details

$ rpm-ostree version

rpm-ostree:
 Version: '2024.2'
 Git: 3d9a8755ddd96395a5c1d02b42243eb54ea01193
 Features:
  - rust
  - compose
  - container
  - fedora-integration

Additional information

Related:

@HuijingHei
Copy link
Member

Normally we do this via rpm-ostree kargs, with --append multiple times, also --append-if-missing and delete.

[root@cosa-devsh ~]# rpm-ostree kargs --append init_on_alloc=1 --append init_on_free=1
Staging deployment... done
Changes queued for next boot. Run "systemctl reboot" to start a reboot
[root@cosa-devsh ~]# rpm-ostree kargs
... init_on_alloc=1 init_on_free=1

[root@cosa-devsh ~]# rpm-ostree kargs --append-if-missing init_on_alloc=1 --append-if-missing init_on_free=1
No changes.

[root@cosa-devsh ~]# rpm-ostree kargs --delete init_on_alloc=1 --delete init_on_free=1

@czhang03
Copy link
Author

czhang03 commented Feb 19, 2024

Thank you for your response! I understand your point, but I think the user should at least get a warning about this, otherwise the semantics of rpm-ostree will be misaligned from the semantics of system.

For example, when user do rpm-ostree karg --append "init_on_alloc=1 init_on_free=1", the semantics for rpm-ostree will be "setting the init_on_alloc variable to 1 init_on_free=1", yet once the karg is added, the system will interprete it as "init_on_alloc is set to 1 and init_on_free is set to 1".

This is not terrible, but to me, it feels like inelegant design in the software. And from the link https://discussion.fedoraproject.org/t/how-i-do-to-put-several-kernel-arguments-in-rpm-ostree-in-a-single-command/74113 , people indeed misinterprets the use cases of these commands.

@jlebon
Copy link
Member

jlebon commented Feb 22, 2024

I think it'd be fine to handle this by splitting on whitespace. However, I'd suggest rather than trying to handle quoting, we just detect if any quotes are present and pass through as is in that case.

Edit: so basically, something like this: if there's a literal " in the argument, pass through as is. Otherwise, split on space.

@travier
Copy link
Member

travier commented Feb 26, 2024

Note that there are the " from the Bash command and the ones in the passed arguments:

$ rpm-ostree kargs --append-if-missing "foo=bar" # "Bash" quotes
$ rpm-ostree kargs --append-if-missing "foo=\"bar foo\"" # Quotes in the argument

According to https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html, we can have quoted spaces in kernel command line arguments so we need to support that.

Thus this makes this tricky, as we need to correctly detect spaces outside of quotes to split the arguments to process them.

$ rpm-ostree kargs --append-if-missing "foo=\"bar foo\" bar=foo"

@HuijingHei
Copy link
Member

HuijingHei commented Feb 26, 2024

Seems can append as whole, but failed when delete (if there is space), IMU, we should fix this.

# rpm-ostree kargs --append='foo="bar foo"'
Or # rpm-ostree kargs --append="foo=\"bar foo\""
rpm-ostree kargs
... foo="bar foo"

# rpm-ostree kargs --delete 'foo="bar foo"'
error: No karg 'foo="bar foo"' found
# rpm-ostree kargs --delete "foo=\"bar foo\""
error: No karg 'foo="bar foo"' found

We should split with space as the issue description.

# rpm-ostree kargs --append "foo=1 bar=1"
# rpm-ostree kargs
... foo=1 bar=1
# rpm-ostree kargs --delete "foo=1 bar=1"
error: No karg 'foo=1 bar=1' found

Xerf to Colin's comment openshift/machine-config-operator#3856 (comment).

@HuijingHei HuijingHei self-assigned this Feb 26, 2024
@jlebon
Copy link
Member

jlebon commented Feb 26, 2024

According to kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html, we can have quoted spaces in kernel command line arguments so we need to support that.

Right, that's what I'm referring to in #4821 (comment). My suggestion is to not try to handle it and just pass through as is if there are literal quotes. OTOH, it wouldn't be very hard to make a quoting-aware splitter so cool with that too. (IIRC we have similar code already for handling this with package requests in manifests too.)

@HuijingHei HuijingHei added the jira for syncing to jira label Feb 28, 2024
HuijingHei added a commit to HuijingHei/ostree that referenced this issue Mar 6, 2024
According to Jonathan's suggestion, should fix this from ostree
repo.

With this patch:
- kargs input like "init_on_alloc=1 init_on_free=1", will be
parsed as 2 seperated arg `init_on_alloc=1` and `init_on_free=1`,
instead of as whole;
- According to https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html,
should keep spaces in double-quotes, like `param="spaces in here"`
will be parsed as whole instead of 3.

Fixes coreos/rpm-ostree#4821
HuijingHei added a commit to HuijingHei/ostree that referenced this issue Mar 6, 2024
According to Jonathan's suggestion, should fix this from ostree
repo.

With this patch:
- kargs input like "init_on_alloc=1 init_on_free=1", will be
parsed as 2 seperated arg `init_on_alloc=1` and `init_on_free=1`,
instead of as whole;
- According to https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html,
should keep spaces in double-quotes, like `param="spaces in here"`
will be parsed as whole instead of 3.

Fixes coreos/rpm-ostree#4821
HuijingHei added a commit to HuijingHei/ostree that referenced this issue Mar 6, 2024
According to Jonathan's suggestion, should fix this from ostree
repo.

With this patch:
- kargs input like "init_on_alloc=1 init_on_free=1", will be
parsed as 2 seperated arg `init_on_alloc=1` and `init_on_free=1`,
instead of as whole;
- According to https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html,
should keep spaces in double-quotes, like `param="spaces in here"`
will be parsed as whole instead of 3.

Fixes coreos/rpm-ostree#4821
HuijingHei added a commit to HuijingHei/ostree that referenced this issue Mar 6, 2024
According to Jonathan's suggestion, should fix the code from
ostree repo.

With this patch:
- kargs input like "init_on_alloc=1 init_on_free=1", will be
parsed as 2 seperated args `init_on_alloc=1` and `init_on_free=1`,
instead of whole;
- According to https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html,
need to keep spaces in double-quotes, like `param="spaces in here"`
will be parsed as whole instead of 3.

Fixes coreos/rpm-ostree#4821
HuijingHei added a commit to HuijingHei/ostree that referenced this issue Mar 6, 2024
According to Jonathan's suggestion, should fix the code from
ostree repo.

With this patch:
- kargs input like "init_on_alloc=1 init_on_free=1", will be
parsed as 2 seperated args `init_on_alloc=1` and `init_on_free=1`,
instead of whole;
- According to https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html,
need to keep spaces in double-quotes, like `param="spaces in here"`
will be parsed as whole instead of 3.

Fixes coreos/rpm-ostree#4821
HuijingHei added a commit to HuijingHei/ostree that referenced this issue Mar 7, 2024
According to Jonathan's suggestion, should fix the code from
ostree repo.

With this patch:
- kargs input like "init_on_alloc=1 init_on_free=1", will be
parsed as 2 seperated args `init_on_alloc=1` and `init_on_free=1`,
instead of whole;
- According to https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html,
need to keep spaces in double-quotes, like `param="spaces in here"`
will be parsed as whole instead of 3.

Fixes coreos/rpm-ostree#4821
HuijingHei added a commit to HuijingHei/ostree that referenced this issue Mar 7, 2024
According to Jonathan's suggestion, should fix the code from
ostree repo.

With this patch:
- kargs input like "init_on_alloc=1 init_on_free=1", will be
parsed as 2 seperated args `init_on_alloc=1` and `init_on_free=1`,
instead of whole;
- According to https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html,
need to keep spaces in double-quotes, like `param="spaces in here"`
will be parsed as whole instead of 3.

Fixes coreos/rpm-ostree#4821
HuijingHei added a commit to HuijingHei/ostree that referenced this issue Mar 8, 2024
According to Jonathan's suggestion, should fix the code from
ostree repo.

With this patch:
- kargs input like "init_on_alloc=1 init_on_free=1", will be
parsed as 2 seperated args `init_on_alloc=1` and `init_on_free=1`,
instead of whole;
- According to https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html,
need to keep spaces in double-quotes, like `param="spaces in here"`
will be parsed as whole instead of 3.

Fixes coreos/rpm-ostree#4821
HuijingHei added a commit to HuijingHei/ostree that referenced this issue Mar 8, 2024
According to Jonathan's suggestion, should fix the code from
ostree repo.

With this patch:
- kargs input like "init_on_alloc=1 init_on_free=1", will be
parsed as 2 seperated args `init_on_alloc=1` and `init_on_free=1`,
instead of whole;
- According to https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html,
need to keep spaces in double-quotes, like `param="spaces in here"`
will be parsed as whole instead of 3.

Fixes coreos/rpm-ostree#4821
@HuijingHei
Copy link
Member

Should be available with https://koji.fedoraproject.org/koji/buildinfo?buildID=2420970

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira for syncing to jira
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants