| Message ID | 1375418489-18089-1-git-send-email-ivan.hu@canonical.com |
|---|---|
| State | Rejected |
| Headers | show |
On 02/08/13 05:41, Ivan Hu wrote: > From: IvanHu <ivan.hu@canonical.com> > > Moving the uefirtvariable, uefirttime and uefirtmisc UEFI runtime service > tests in the the UEFI category instead of using unsafe category in order > to make it more specific and let users to be more aware of the tests. > > Signed-off-by: Ivan Hu <ivan.hu@canonical.com> > --- > src/lib/src/fwts_framework.c | 10 ++++++++-- > src/uefi/uefirtmisc/uefirtmisc.c | 2 +- > src/uefi/uefirttime/uefirttime.c | 2 +- > src/uefi/uefirtvariable/uefirtvariable.c | 2 +- > 4 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/src/lib/src/fwts_framework.c b/src/lib/src/fwts_framework.c > index 1581681..28c2ce8 100644 > --- a/src/lib/src/fwts_framework.c > +++ b/src/lib/src/fwts_framework.c > @@ -38,7 +38,8 @@ > FWTS_FLAG_INTERACTIVE_EXPERIMENTAL | \ > FWTS_FLAG_POWER_STATES | \ > FWTS_FLAG_UTILS | \ > - FWTS_FLAG_UNSAFE) > + FWTS_FLAG_UNSAFE | \ > + FWTS_FLAG_TEST_UEFI) > > static fwts_list tests_to_skip; > > @@ -81,6 +82,7 @@ static fwts_option fwts_framework_options[] = { > { "filter-error-keep", "", 1, "Keep errors that match any of the specified labels." }, > { "acpica-debug", "", 0, "Enable ACPICA debug/warning messages." }, > { "acpica", "", 1, "Enable ACPICA run time options." }, > + { "uefi", "", 0, "Run UEFI tests." }, > { NULL, NULL, 0, NULL } > }; > > @@ -227,6 +229,7 @@ static void fwts_framework_show_tests(fwts_framework *fw, const bool full) > { "Power States", FWTS_FLAG_POWER_STATES }, > { "Utilities", FWTS_FLAG_UTILS }, > { "Unsafe", FWTS_FLAG_UNSAFE }, > + { "UEFI", FWTS_FLAG_TEST_UEFI }, > { NULL, 0 }, > }; > > @@ -241,7 +244,7 @@ static void fwts_framework_show_tests(fwts_framework *fw, const bool full) > fwts_list_init(&sorted); > fwts_list_foreach(item, &fwts_framework_test_list) { > test = fwts_list_data(fwts_framework_test *, item); > - if ((test->flags & FWTS_FLAG_RUN_ALL) == categories[i].flag) > + if ((test->flags & FWTS_FLAG_RUN_ALL) & categories[i].flag) > fwts_list_add_ordered(&sorted, test, > fwts_framework_compare_test_name); > } > @@ -1130,6 +1133,9 @@ int fwts_framework_options_handler(fwts_framework *fw, int argc, char * const ar > if (fwts_framework_acpica_parse(fw, optarg) != FWTS_OK) > return FWTS_ERROR; > break; > + case 38: /* --uefi */ > + fw->flags |= FWTS_FLAG_TEST_UEFI; > + break; > } > break; > case 'a': /* --all */ > diff --git a/src/uefi/uefirtmisc/uefirtmisc.c b/src/uefi/uefirtmisc/uefirtmisc.c > index 8fd4b4c..d94b885 100644 > --- a/src/uefi/uefirtmisc/uefirtmisc.c > +++ b/src/uefi/uefirtmisc/uefirtmisc.c > @@ -224,4 +224,4 @@ static fwts_framework_ops uefirtmisc_ops = { > .minor_tests = uefirtmisc_tests > }; > > -FWTS_REGISTER("uefirtmisc", &uefirtmisc_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); > +FWTS_REGISTER("uefirtmisc", &uefirtmisc_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_TEST_UEFI | FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); > diff --git a/src/uefi/uefirttime/uefirttime.c b/src/uefi/uefirttime/uefirttime.c > index 2094677..c02a1aa 100644 > --- a/src/uefi/uefirttime/uefirttime.c > +++ b/src/uefi/uefirttime/uefirttime.c > @@ -497,4 +497,4 @@ static fwts_framework_ops uefirttime_ops = { > .minor_tests = uefirttime_tests > }; > > -FWTS_REGISTER("uefirttime", &uefirttime_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); > +FWTS_REGISTER("uefirttime", &uefirttime_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_TEST_UEFI | FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); > diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c > index e223f82..4425029 100644 > --- a/src/uefi/uefirtvariable/uefirtvariable.c > +++ b/src/uefi/uefirtvariable/uefirtvariable.c > @@ -1654,4 +1654,4 @@ static fwts_framework_ops uefirtvariable_ops = { > .options_check = options_check, > }; > > -FWTS_REGISTER("uefirtvariable", &uefirtvariable_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); > +FWTS_REGISTER("uefirtvariable", &uefirtvariable_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_TEST_UEFI | FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); > This is more like it. The changes are good. My concern is that the potentially damaging "unsafe" tests are enabled for the --all option which may be a risk. But "--all" does mean all, so that is fair really I guess. So, I'm happy with these changes as long as we believe the risk of enabling the "unsafe" tests with the --all option is low enough. Otherwise, perhaps we should remove unsafe tests from --all just to avoid potentially bricking users machines. So anyone like to also comment on that? Colin
On 02/08/13 05:41, Ivan Hu wrote: > From: IvanHu <ivan.hu@canonical.com> > > Moving the uefirtvariable, uefirttime and uefirtmisc UEFI runtime service > tests in the the UEFI category instead of using unsafe category in order > to make it more specific and let users to be more aware of the tests. > > Signed-off-by: Ivan Hu <ivan.hu@canonical.com> > --- > src/lib/src/fwts_framework.c | 10 ++++++++-- > src/uefi/uefirtmisc/uefirtmisc.c | 2 +- > src/uefi/uefirttime/uefirttime.c | 2 +- > src/uefi/uefirtvariable/uefirtvariable.c | 2 +- > 4 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/src/lib/src/fwts_framework.c b/src/lib/src/fwts_framework.c > index 1581681..28c2ce8 100644 > --- a/src/lib/src/fwts_framework.c > +++ b/src/lib/src/fwts_framework.c > @@ -38,7 +38,8 @@ > FWTS_FLAG_INTERACTIVE_EXPERIMENTAL | \ > FWTS_FLAG_POWER_STATES | \ > FWTS_FLAG_UTILS | \ > - FWTS_FLAG_UNSAFE) > + FWTS_FLAG_UNSAFE | \ > + FWTS_FLAG_TEST_UEFI) > > static fwts_list tests_to_skip; > > @@ -81,6 +82,7 @@ static fwts_option fwts_framework_options[] = { > { "filter-error-keep", "", 1, "Keep errors that match any of the specified labels." }, > { "acpica-debug", "", 0, "Enable ACPICA debug/warning messages." }, > { "acpica", "", 1, "Enable ACPICA run time options." }, > + { "uefi", "", 0, "Run UEFI tests." }, > { NULL, NULL, 0, NULL } > }; > > @@ -227,6 +229,7 @@ static void fwts_framework_show_tests(fwts_framework *fw, const bool full) > { "Power States", FWTS_FLAG_POWER_STATES }, > { "Utilities", FWTS_FLAG_UTILS }, > { "Unsafe", FWTS_FLAG_UNSAFE }, > + { "UEFI", FWTS_FLAG_TEST_UEFI }, > { NULL, 0 }, > }; > > @@ -241,7 +244,7 @@ static void fwts_framework_show_tests(fwts_framework *fw, const bool full) > fwts_list_init(&sorted); > fwts_list_foreach(item, &fwts_framework_test_list) { > test = fwts_list_data(fwts_framework_test *, item); > - if ((test->flags & FWTS_FLAG_RUN_ALL) == categories[i].flag) > + if ((test->flags & FWTS_FLAG_RUN_ALL) & categories[i].flag) > fwts_list_add_ordered(&sorted, test, > fwts_framework_compare_test_name); > } > @@ -1130,6 +1133,9 @@ int fwts_framework_options_handler(fwts_framework *fw, int argc, char * const ar > if (fwts_framework_acpica_parse(fw, optarg) != FWTS_OK) > return FWTS_ERROR; > break; > + case 38: /* --uefi */ > + fw->flags |= FWTS_FLAG_TEST_UEFI; > + break; > } > break; > case 'a': /* --all */ > diff --git a/src/uefi/uefirtmisc/uefirtmisc.c b/src/uefi/uefirtmisc/uefirtmisc.c > index 8fd4b4c..d94b885 100644 > --- a/src/uefi/uefirtmisc/uefirtmisc.c > +++ b/src/uefi/uefirtmisc/uefirtmisc.c > @@ -224,4 +224,4 @@ static fwts_framework_ops uefirtmisc_ops = { > .minor_tests = uefirtmisc_tests > }; > > -FWTS_REGISTER("uefirtmisc", &uefirtmisc_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); > +FWTS_REGISTER("uefirtmisc", &uefirtmisc_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_TEST_UEFI | FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); > diff --git a/src/uefi/uefirttime/uefirttime.c b/src/uefi/uefirttime/uefirttime.c > index 2094677..c02a1aa 100644 > --- a/src/uefi/uefirttime/uefirttime.c > +++ b/src/uefi/uefirttime/uefirttime.c > @@ -497,4 +497,4 @@ static fwts_framework_ops uefirttime_ops = { > .minor_tests = uefirttime_tests > }; > > -FWTS_REGISTER("uefirttime", &uefirttime_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); > +FWTS_REGISTER("uefirttime", &uefirttime_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_TEST_UEFI | FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); > diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c > index e223f82..4425029 100644 > --- a/src/uefi/uefirtvariable/uefirtvariable.c > +++ b/src/uefi/uefirtvariable/uefirtvariable.c > @@ -1654,4 +1654,4 @@ static fwts_framework_ops uefirtvariable_ops = { > .options_check = options_check, > }; > > -FWTS_REGISTER("uefirtvariable", &uefirtvariable_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); > +FWTS_REGISTER("uefirtvariable", &uefirtvariable_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_TEST_UEFI | FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); > I guess we may need to also include the following tests too: csm securebootcert Colin
On Sat, 03 Aug, at 10:10:43AM, Colin Ian King wrote: > This is more like it. The changes are good. My concern is that the > potentially damaging "unsafe" tests are enabled for the --all option > which may be a risk. But "--all" does mean all, so that is fair > really I guess. > > So, I'm happy with these changes as long as we believe the risk of > enabling the "unsafe" tests with the --all option is low enough. > Otherwise, perhaps we should remove unsafe tests from --all just to > avoid potentially bricking users machines. So anyone like to also > comment on that? I guess it comes down to who we expect to run --all, and whether they would be able to recover their machines if they become bricks. For example, when running fwts at UEFI plugfests we *want* users to trigger any serious bugs, without making it unnecessarily complicated to run all the tests, including the unsafe ones. FWIW, I'm all for including the unsafe tests in --all provided that the change is made blatantly obvious, so that users are not caught unawares.
On 08/04/2013 06:30 PM, Matt Fleming wrote: > On Sat, 03 Aug, at 10:10:43AM, Colin Ian King wrote: >> This is more like it. The changes are good. My concern is that the >> potentially damaging "unsafe" tests are enabled for the --all option >> which may be a risk. But "--all" does mean all, so that is fair >> really I guess. >> >> So, I'm happy with these changes as long as we believe the risk of >> enabling the "unsafe" tests with the --all option is low enough. >> Otherwise, perhaps we should remove unsafe tests from --all just to >> avoid potentially bricking users machines. So anyone like to also >> comment on that? > > I guess it comes down to who we expect to run --all, and whether they > would be able to recover their machines if they become bricks. > > For example, when running fwts at UEFI plugfests we *want* users to > trigger any serious bugs, without making it unnecessarily complicated to > run all the tests, including the unsafe ones. > > FWIW, I'm all for including the unsafe tests in --all provided that the > change is made blatantly obvious, so that users are not caught unawares. > I suggest we could add another option, for example --all-safe, which excludes these unsafe tests. Just let --all run all tests, and users who don't want to run these unsafe tests could use --all-safe for all safe tests. Ivan
diff --git a/src/lib/src/fwts_framework.c b/src/lib/src/fwts_framework.c index 1581681..28c2ce8 100644 --- a/src/lib/src/fwts_framework.c +++ b/src/lib/src/fwts_framework.c @@ -38,7 +38,8 @@ FWTS_FLAG_INTERACTIVE_EXPERIMENTAL | \ FWTS_FLAG_POWER_STATES | \ FWTS_FLAG_UTILS | \ - FWTS_FLAG_UNSAFE) + FWTS_FLAG_UNSAFE | \ + FWTS_FLAG_TEST_UEFI) static fwts_list tests_to_skip; @@ -81,6 +82,7 @@ static fwts_option fwts_framework_options[] = { { "filter-error-keep", "", 1, "Keep errors that match any of the specified labels." }, { "acpica-debug", "", 0, "Enable ACPICA debug/warning messages." }, { "acpica", "", 1, "Enable ACPICA run time options." }, + { "uefi", "", 0, "Run UEFI tests." }, { NULL, NULL, 0, NULL } }; @@ -227,6 +229,7 @@ static void fwts_framework_show_tests(fwts_framework *fw, const bool full) { "Power States", FWTS_FLAG_POWER_STATES }, { "Utilities", FWTS_FLAG_UTILS }, { "Unsafe", FWTS_FLAG_UNSAFE }, + { "UEFI", FWTS_FLAG_TEST_UEFI }, { NULL, 0 }, }; @@ -241,7 +244,7 @@ static void fwts_framework_show_tests(fwts_framework *fw, const bool full) fwts_list_init(&sorted); fwts_list_foreach(item, &fwts_framework_test_list) { test = fwts_list_data(fwts_framework_test *, item); - if ((test->flags & FWTS_FLAG_RUN_ALL) == categories[i].flag) + if ((test->flags & FWTS_FLAG_RUN_ALL) & categories[i].flag) fwts_list_add_ordered(&sorted, test, fwts_framework_compare_test_name); } @@ -1130,6 +1133,9 @@ int fwts_framework_options_handler(fwts_framework *fw, int argc, char * const ar if (fwts_framework_acpica_parse(fw, optarg) != FWTS_OK) return FWTS_ERROR; break; + case 38: /* --uefi */ + fw->flags |= FWTS_FLAG_TEST_UEFI; + break; } break; case 'a': /* --all */ diff --git a/src/uefi/uefirtmisc/uefirtmisc.c b/src/uefi/uefirtmisc/uefirtmisc.c index 8fd4b4c..d94b885 100644 --- a/src/uefi/uefirtmisc/uefirtmisc.c +++ b/src/uefi/uefirtmisc/uefirtmisc.c @@ -224,4 +224,4 @@ static fwts_framework_ops uefirtmisc_ops = { .minor_tests = uefirtmisc_tests }; -FWTS_REGISTER("uefirtmisc", &uefirtmisc_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); +FWTS_REGISTER("uefirtmisc", &uefirtmisc_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_TEST_UEFI | FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); diff --git a/src/uefi/uefirttime/uefirttime.c b/src/uefi/uefirttime/uefirttime.c index 2094677..c02a1aa 100644 --- a/src/uefi/uefirttime/uefirttime.c +++ b/src/uefi/uefirttime/uefirttime.c @@ -497,4 +497,4 @@ static fwts_framework_ops uefirttime_ops = { .minor_tests = uefirttime_tests }; -FWTS_REGISTER("uefirttime", &uefirttime_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); +FWTS_REGISTER("uefirttime", &uefirttime_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_TEST_UEFI | FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c index e223f82..4425029 100644 --- a/src/uefi/uefirtvariable/uefirtvariable.c +++ b/src/uefi/uefirtvariable/uefirtvariable.c @@ -1654,4 +1654,4 @@ static fwts_framework_ops uefirtvariable_ops = { .options_check = options_check, }; -FWTS_REGISTER("uefirtvariable", &uefirtvariable_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV); +FWTS_REGISTER("uefirtvariable", &uefirtvariable_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_TEST_UEFI | FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV);