Propose /boot for encrypted root on PowerPC #155
Conversation
In case we already have one /boot converted, set have_ppc_boot flag, so others will be untouched. This makes possible to have /boot proposed on PowerPC. For example in encrypted fs case. Signed-off-by: Dinar Valeev <dvaleev@suse.com>
|
As I already wrote in bugzilla this is a hack. For example if the proposal reuses partitions the ordering of the two /boot partitions generated in the proposal may change and then the sizes of /boot and PReP are also swapped. And a /boot partition with 7MiB to too small to even hold a filesystem. |
|
aren't boot and boot2 different, boot will always be passed as first hence converted. |
|
You likely also missed the code in get_proposal_vm(). |
|
It's not obvious that boot will always be passed as first and is always the one that has to be converted to PReP. Can you show that? Even if, it's still a hack and we are required to improve code quality and not add one more hack. Things have changed in the last years. |
PowerPC should be handled specially similar what is done with Efi. We need to define boot2, so /boot will be proposed for encrypted root Signed-off-by: Dinar Valeev <dvaleev@suse.com>
|
We could improve SpecialBoothandling by looking at partition size, if it less than 2 cyls then convert it |
|
That is also a hack. The proper fix is to make the proposal code output the correct partitions from the beginning (as already stated in bugzilla). |
|
|
||
| # testedfiles: helper.rb | ||
|
|
||
| module Yast |
There was a problem hiding this comment.
I'm missing a description of this test. Please add at least a comment what it tests and how. And what is the expected output.
There was a problem hiding this comment.
I don't see any other tests are doing this.
There was a problem hiding this comment.
Yes, you are right, I would like to see it for all the tests from now on. Everything is for the first time once...
|
I agree with Arvin, that hacks must not be added to the code anymore. I'm expecting a cleanup rather than adding something that will take five times more to fix later. It might help to have a chat with Arvin/HuHa at first, before changing the code. It might be faster. |
|
@kobliha do you want refactor at RC1? |
|
@k0da No, what I want is a working code that does not add more hacks. I know that you want to help us to "make it work", but please, consult with @aschnell and @shundhammer how to make it properly. As I've already written in Bugzilla, we need a specification for location and appearance of the PReP partition before implementing anything. We can't be sure that it will work otherwise. |
| @@ -5624,11 +5624,16 @@ def get_inst_prop_vm(target, key) | |||
| Ops.set(ret, "target", target) | |||
|
|
|||
| boot2 = {} | |||
| if GetProposalEncrypt() && Partitions.EfiBoot | |||
| if GetProposalEncrypt() && (Partitions.EfiBoot||Partitions.PrepBoot) | |||
There was a problem hiding this comment.
BTW, and this is pure nitpicking, Ruby style requires spaces around ||
|
@k0da do you plan to do something with this PR, it is currently outdated and the related bug is already closed so probably we could also close it. |
|
storage is replaced by storage-ng so closing. |
No description provided.