Update all calls to add/replace_grub_cmdline_settings #8727

wjn740 · 2019-10-18T08:13:10Z

I modified all add/replace_grub_cmdline_settings function prototype.
It blocked some testings like 'security'. I update all 'calls' where that want to update
config in time.

@pevik @asmorodskyi @jouyingbin

Signed-off-by: James Wang jnwang@suse.com

pevik · 2019-10-18T08:57:04Z

@wjn740 thanks, acked. But, can you, please, do one verify run? (it's obviously ok, but we've done some breaks in the past :)).
BTW: I was told by coolo in the past not to use Signed-off-by: (os-autoinst projects are the only one where I don't use it).

pevik · 2019-10-18T09:00:11Z

tests/security/ima/evm_protection_hmacs.pm

@@ -50,7 +50,7 @@ sub run {
    assert_script_run "setfattr -x security.ima $sample_app";
    validate_script_output "getfattr -m security.evm -d $sample_app", sub { m/^$/ };

-    replace_grub_cmdline_settings('evm=fix ima_appraise=fix', '', 1);
+    replace_grub_cmdline_settings('evm=fix ima_appraise=fix', '', update_grub => 1);


BTW I wonder if it work as
replace_grub_cmdline_settings('evm=fix ima_appraise=fix', update_grub => 1);
i.e. having $new as undef. It'd probably introduce a warning, there would need to be added support for it in change_grub_config(). And explicitly set new as '' is more readable.

sorry, the first two parameters are fixed, you should pass them. can not ignore. user need to make sure $old, $new is not undef, I think.

1029 sub change_grub_config { 1030 die((caller(0))[3] . ' expects from 3 to 5 arguments') unless (@_ >= 3 && @_ <= 5); 1031 my ($old, $new, $search, $modifiers, $update_grub) = @_; 1032 $modifiers //= ''; 1033 $update_grub //= 0; + 1034 $new //= ''; 1035 $search = "/$search/" if defined $search; 1036 1037 assert_script_run("sed -ie '${search}s/${old}/${new}/${modifiers}' " . GRUB_DEFAULT_FILE);

Yes, that's exactly what I meant (+ mention it in perldoc and commit message).

http://10.67.134.217/tests/5372#step/sestatus/37

@jouyingbin Please take a look about the grub operation.

coolo · 2019-10-18T09:06:46Z

BTW: I was told by coolo in the past not to use Signed-off-by: (os-autoinst projects are the only one where I don't use it).

Huh? Why would I say that?

wjn740 · 2019-10-18T09:07:31Z

@wjn740 thanks, acked. But, can you, please, do one verify run? (it's obviously ok, but we've done some breaks in the past :)).
I'm searching for their testcases to verifying.
BTW: I was told by coolo in the past not to use Signed-off-by: (os-autoinst projects are the only one where I don't use it).

OK.

pevik · 2019-10-18T09:49:01Z

BTW: I was told by coolo in the past not to use Signed-off-by: (os-autoinst projects are the only one where I don't use it).

Huh? Why would I say that?

You:
os-autoinst/openQA#1182 (comment)
Oliver:
#2250 (comment)

Not worth of arguing (long time ago, maybe I didn't understand you at the time), I just stop adding Signed-off-by: then :)

coolo · 2019-10-18T10:01:18Z

ok, that wasnt' meant as a general rule though.

pevik · 2019-10-18T10:02:11Z

OK, thanks for explanation :)

pevik · 2019-10-18T11:25:32Z

lib/bootloader_setup.pm

@@ -1022,6 +1022,7 @@ sub grep_grub_cmdline_settings {
    change_grub_config($old, $new [, $search ] [, $modifiers ], [, $update_grub ]);

 Replace C<$old> with C<$new> in /etc/default/grub, using sed.
+C<$new> will be keep '' null string when it is undef.


I'd define it as: C<$new>, default empty string

pevik · 2019-10-18T11:26:49Z

tests/security/ima/evm_protection_hmacs.pm

@@ -50,7 +50,7 @@ sub run {
    assert_script_run "setfattr -x security.ima $sample_app";
    validate_script_output "getfattr -m security.evm -d $sample_app", sub { m/^$/ };

-    replace_grub_cmdline_settings('evm=fix ima_appraise=fix', '', 1);
+    replace_grub_cmdline_settings('evm=fix ima_appraise=fix', '', update_grub => 1);


I'd remove '', as it's now not needed :).

@pevik Hi sir, why this '', is not needed,

1087 sub replace_grub_cmdline_settings { 1088 my $old = shift; 1089 my $new = shift; 1090 my %args = testapi::compat_args( 1091 { 1092 old => $old, 1093 new => $new, 1094 update_grub => 0, 1095 search => get_cmdline_var(), 1096 }, ['old', 'new', 'update_grub', 'search'], @_); 1097 change_grub_config($old, $new, $args{search}, "g", $args{update_grub}); 1098 } if remove it, line 1089, what will do ?

sorry, Just a little bit confuse this in my brain, ...

lilyeyes · 2019-10-21T01:57:20Z

Hi James,
I have reviewed all "Security" related code changes,they looks good to me.
But, could you please offer the verify run otherwise I could not give you an "approval"?

BTW, it is close to sle12sp5 GMC hope this PR can be resolved asap.

wjn740 · 2019-10-21T02:19:08Z

Hi James,
I have reviewed all "Security" related code changes,they looks good to me.
But, could you please offer the verify run otherwise I could not give you an "approval"?

BTW, it is close to sle12sp5 GMC hope this PR can be resolved asap.

http://10.67.134.217/tests/5372#step/sestatus/37

lilyeyes · 2019-10-21T02:53:02Z

Hi James,
I have reviewed all "Security" related code changes,they looks good to me.
But, could you please offer the verify run otherwise I could not give you an "approval"?
BTW, it is close to sle12sp5 GMC hope this PR can be resolved asap.

http://10.67.134.217/tests/5372#step/sestatus/37
Thanks, the simple verify run is PASS.
Though I would like to see more verify runs since test case "SELinux" only verifies set_*() can work not include replace_*().

I would like to give you changes an "approval".

lilyeyes

I only checked the "security test cases" related changes and the changes lgtm.

asmorodskyi · 2019-10-21T07:03:33Z

LGTM for me, just doubt about unanswered questions from @pevik . @wjn740 can you please take a look ?

wjn740 · 2019-10-21T07:04:51Z

LGTM for me, just doubt about unanswered questions from @pevik . @wjn740 can you please take a look ?

I missed it

pevik · 2019-10-21T07:15:39Z

@wjn740 C<$new> will be keep '' null string when it is undef. is a bit strange wording, I'd use something like C<$new>, default empty string.
Otherwise LGTM.

wjn740 · 2019-10-21T07:17:01Z

@wjn740 C<$new> will be keep '' null string when it is undef. is a bit strange wording, I'd use something like C<$new>, default empty string.
Otherwise LGTM.

Sorry for my English. I have updated the code, now.

wjn740 · 2019-10-21T07:19:40Z

There are more verifications:
http://10.67.134.217/tests/5387
http://10.67.134.217/tests/5388

lilyeyes · 2019-10-21T07:25:18Z

@wjn740
As we all know SLE12SP5 GMC will be released at this week (in 2 or 3 days) and we (not limited security QA team) need the openQA results. If the merge of 8727 will not fix all unexpected issues that will be a big risk as we have no enough time to do manual testings.
Based on above concerns we suggest to revert "Xen command-line" feature. If we do revert please make sure to revert the correct version.

pevik · 2019-10-21T07:27:45Z

@lilyeyes it haven't been merged yet, so nothing to be reverted yet :).

wjn740 · 2019-10-21T07:29:09Z

@lilyeyes it haven't been merged yet, so nothing to be reverted yet :).

She means revert 5362a6a

commit 5362a6a (origin/jnwang/add_gurb_xen_support, jnwang/add_gurb_xen_support)
Author: James Wang jnwang@suse.com
Date: Wed Oct 9 17:29:29 2019 +0800

Fix "Add some API for Xen command-line" patch

Fix compat_args function wasn't exported.

lilyeyes · 2019-10-21T07:38:07Z

@lilyeyes it haven't been merged yet, so nothing to be reverted yet :).

Thanks for the replying I do not mean #8727 I mean the PR before #8727 (maybe it is #8631 or the one before #8631 )

wjn740 · 2019-10-21T07:49:14Z

There is another verification:
http://10.67.134.217/tests/5385

jouyingbin · 2019-10-21T08:27:07Z

There is another verification:
http://10.67.134.217/tests/5385

I just check the verification run looks good to me,
The grub_mkconfig() can be executed correctly after appending update_grub => 1.
BTW, It will be great if you could collect all verification run in the top description.
Thanks for the updated.

I modified all add/replace_grub_cmdline_settings function prototype. It blocked some testings like 'security'. I update all 'calls' where that want update config in time.

wjn740 · 2019-10-21T08:29:01Z

There is another verification:
http://10.67.134.217/tests/5385

I just check the verification run looks good to me,
The grub_mkconfig() can be executed correctly after appending update_grub => 1.
BTW, It will be great if you could collect all verification run in the top description.
Thanks for the updated.

I don't know them before @lilyeyes told me. ;-)

jouyingbin

LGTM. Thanks for the changes. :-)

pevik approved these changes Oct 18, 2019

View reviewed changes

wjn740 force-pushed the jnwang/update_all_add_grub_cmdline_setting branch 3 times, most recently from 3dce103 to 7c2b01b Compare October 18, 2019 10:49

pevik reviewed Oct 18, 2019

View reviewed changes

lilyeyes approved these changes Oct 21, 2019

View reviewed changes

coolgw requested a review from asmorodskyi October 21, 2019 06:39

wjn740 force-pushed the jnwang/update_all_add_grub_cmdline_setting branch 2 times, most recently from f437874 to e07379c Compare October 21, 2019 07:10

wjn740 force-pushed the jnwang/update_all_add_grub_cmdline_setting branch from e07379c to 0b49752 Compare October 21, 2019 07:38

Update all calls to add/replace_grub_cmdline_settings

f3c5958

I modified all add/replace_grub_cmdline_settings function prototype. It blocked some testings like 'security'. I update all 'calls' where that want update config in time.

wjn740 force-pushed the jnwang/update_all_add_grub_cmdline_setting branch from 0b49752 to f3c5958 Compare October 21, 2019 08:27

asmorodskyi merged commit e7c558b into os-autoinst:master Oct 21, 2019

jouyingbin reviewed Oct 21, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update all calls to add/replace_grub_cmdline_settings #8727

Update all calls to add/replace_grub_cmdline_settings #8727

wjn740 commented Oct 18, 2019

pevik commented Oct 18, 2019

pevik Oct 18, 2019

wjn740 Oct 18, 2019

wjn740 Oct 18, 2019 •

edited

pevik Oct 18, 2019

wjn740 Oct 18, 2019 •

edited

coolo commented Oct 18, 2019

wjn740 commented Oct 18, 2019

pevik commented Oct 18, 2019

coolo commented Oct 18, 2019

pevik commented Oct 18, 2019

pevik Oct 18, 2019

wjn740 Oct 21, 2019

pevik Oct 18, 2019

wjn740 Oct 21, 2019

wjn740 Oct 21, 2019

wjn740 Oct 21, 2019

lilyeyes commented Oct 21, 2019

wjn740 commented Oct 21, 2019

lilyeyes commented Oct 21, 2019

lilyeyes left a comment

asmorodskyi commented Oct 21, 2019

wjn740 commented Oct 21, 2019

pevik commented Oct 21, 2019

wjn740 commented Oct 21, 2019

wjn740 commented Oct 21, 2019

lilyeyes commented Oct 21, 2019

pevik commented Oct 21, 2019

wjn740 commented Oct 21, 2019

lilyeyes commented Oct 21, 2019

wjn740 commented Oct 21, 2019

jouyingbin commented Oct 21, 2019

wjn740 commented Oct 21, 2019

jouyingbin left a comment

Update all calls to add/replace_grub_cmdline_settings #8727

Update all calls to add/replace_grub_cmdline_settings #8727

Conversation

wjn740 commented Oct 18, 2019

pevik commented Oct 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjn740 Oct 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjn740 Oct 18, 2019 • edited

Choose a reason for hiding this comment

coolo commented Oct 18, 2019

wjn740 commented Oct 18, 2019

pevik commented Oct 18, 2019

coolo commented Oct 18, 2019

pevik commented Oct 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilyeyes commented Oct 21, 2019

wjn740 commented Oct 21, 2019

lilyeyes commented Oct 21, 2019

lilyeyes left a comment

Choose a reason for hiding this comment

asmorodskyi commented Oct 21, 2019

wjn740 commented Oct 21, 2019

pevik commented Oct 21, 2019

wjn740 commented Oct 21, 2019

wjn740 commented Oct 21, 2019

lilyeyes commented Oct 21, 2019

pevik commented Oct 21, 2019

wjn740 commented Oct 21, 2019

lilyeyes commented Oct 21, 2019

wjn740 commented Oct 21, 2019

jouyingbin commented Oct 21, 2019

wjn740 commented Oct 21, 2019

jouyingbin left a comment

Choose a reason for hiding this comment

wjn740 Oct 18, 2019 •

edited

wjn740 Oct 18, 2019 •

edited