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

Add option in /proc/cmdline to enable/disable the generator #43

Merged
merged 2 commits into from
Jan 7, 2021

Conversation

keszybz
Copy link
Member

@keszybz keszybz commented Jul 9, 2020

Closes #42.

src/config.rs Outdated Show resolved Hide resolved
@cmurf
Copy link

cmurf commented Jul 13, 2020

Anaconda folks have decided to deprecate inst.zram and leave it up to zram-generator folks if there's a use case for command line inhibition.
https://www.redhat.com/archives/anaconda-devel-list/2020-July/msg00025.html

@ignatenkobrain
Copy link
Member

In that case, I think it is not worth implementing some option to disable it.

@cmurf
Copy link

cmurf commented Jul 13, 2020

In that case, I think it is not worth implementing some option to disable it.

Yeah, I think its "defaultness" (the state of whether it is or is not enabled by default) is so fundamental to the image in question that it must be correct.

@keszybz
Copy link
Member Author

keszybz commented Jul 13, 2020

I think it'd be worthwhile to have a kernel-commandline option. I'm not sure about the name though. systemd.zram=0|1 ?

src/main.rs Outdated Show resolved Hide resolved
@nabijaczleweli
Copy link
Collaborator

systemd.zram= sounds good

@keszybz
Copy link
Member Author

keszybz commented Jul 14, 2020

Updated with systemd.zram[=0|1].

@keszybz keszybz changed the title Disable generator when inst.zram is in /proc/cmdline Add option in /proc/cmdline to enable/disable the generator Jul 14, 2020
man/zram-generator.md Outdated Show resolved Hide resolved
src/main.rs Outdated
@@ -65,14 +65,22 @@ fn main() -> Result<()> {

let _ = kernlog::init_with_level(log_level);

let kernel_override = || match config::kernel_zram_option(root) {
Some(false) => std::process::exit(0),
Copy link
Collaborator

@nabijaczleweli nabijaczleweli Jul 14, 2020

Choose a reason for hiding this comment

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

sth like {info!("Disabled by kernel cmdline option"); return Ok(())} would be better

Copy link
Member Author

Choose a reason for hiding this comment

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

But that'd just return from the closure, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I'd missed the closure part; IIRC the log is explicitly flushed, so even though "no destructors on the current stack or any other thread's stack will be run", an info!(); exit(0) should be fine, if imperfect, but all alternatives I came up with are even uglier, so.

@keszybz
Copy link
Member Author

keszybz commented Jan 7, 2021

Pfff, time flies. Updated with a few minor typo fixes and {info!("Disabled by kernel cmdline option"); return Ok(())} as suggested.

See the man page diff for description of semantics.
Looking at the confusion in https://bugzilla.redhat.com/show_bug.cgi?id=1861463,
I think it's always good to be very explicit where some file came
from.
@keszybz
Copy link
Member Author

keszybz commented Jan 7, 2021

... and rebased.

@keszybz keszybz merged commit 81ef18f into systemd:master Jan 7, 2021
@keszybz keszybz deleted the anaconda-compat branch January 13, 2021 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

RFE: parse boot parameters for an Anaconda inbibit parameter
4 participants