| Message ID | 20241112182810.24761-1-av2082000@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selftests: lsm: Refactor `flags_overset_lsm_set_self_attr` test | expand |
On 11/12/2024 10:28 AM, Amit Vadhavana wrote: > - Remove unnecessary `tctx` variable, use `ctx` directly. > - Simplified code with no functional changes. > > Signed-off-by: Amit Vadhavana <av2082000@gmail.com> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> > --- > tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c > index 66dec47e3ca3..732e89fe99c0 100644 > --- a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c > +++ b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c > @@ -56,16 +56,15 @@ TEST(flags_zero_lsm_set_self_attr) > TEST(flags_overset_lsm_set_self_attr) > { > const long page_size = sysconf(_SC_PAGESIZE); > - char *ctx = calloc(page_size, 1); > + struct lsm_ctx *ctx = calloc(page_size, 1); > __u32 size = page_size; > - struct lsm_ctx *tctx = (struct lsm_ctx *)ctx; > > ASSERT_NE(NULL, ctx); > if (attr_lsm_count()) { > - ASSERT_LE(1, lsm_get_self_attr(LSM_ATTR_CURRENT, tctx, &size, > + ASSERT_LE(1, lsm_get_self_attr(LSM_ATTR_CURRENT, ctx, &size, > 0)); > } > - ASSERT_EQ(-1, lsm_set_self_attr(LSM_ATTR_CURRENT | LSM_ATTR_PREV, tctx, > + ASSERT_EQ(-1, lsm_set_self_attr(LSM_ATTR_CURRENT | LSM_ATTR_PREV, ctx, > size, 0)); > > free(ctx);
On 11/12/24 11:28, Amit Vadhavana wrote: > - Remove unnecessary `tctx` variable, use `ctx` directly. > - Simplified code with no functional changes. > I would rephrase the short to simply say Remove unused variable, as refactor implies more extensive changes than what this patch is actually doing. Please write complete sentences instead of bullet points in the change log. How did you find this problem? Do include the details on how in the change log. > Signed-off-by: Amit Vadhavana <av2082000@gmail.com> > --- > tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c > index 66dec47e3ca3..732e89fe99c0 100644 > --- a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c > +++ b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c > @@ -56,16 +56,15 @@ TEST(flags_zero_lsm_set_self_attr) > TEST(flags_overset_lsm_set_self_attr) > { > const long page_size = sysconf(_SC_PAGESIZE); > - char *ctx = calloc(page_size, 1); > + struct lsm_ctx *ctx = calloc(page_size, 1); Why not name this tctx and avoid changes to the ASSERT_EQs below? > __u32 size = page_size; > - struct lsm_ctx *tctx = (struct lsm_ctx *)ctx; > > ASSERT_NE(NULL, ctx); > if (attr_lsm_count()) { > - ASSERT_LE(1, lsm_get_self_attr(LSM_ATTR_CURRENT, tctx, &size, > + ASSERT_LE(1, lsm_get_self_attr(LSM_ATTR_CURRENT, ctx, &size, > 0)); > } > - ASSERT_EQ(-1, lsm_set_self_attr(LSM_ATTR_CURRENT | LSM_ATTR_PREV, tctx, > + ASSERT_EQ(-1, lsm_set_self_attr(LSM_ATTR_CURRENT | LSM_ATTR_PREV, ctx, > size, 0)); > > free(ctx); You have to change this tctx for sure. With these changes: Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> Paul, James, Please do let me know if you would me to take this through kselftest tree. thanks, -- Shuah
On 11/14/2024 8:25 AM, Shuah Khan wrote: > On 11/12/24 11:28, Amit Vadhavana wrote: >> - Remove unnecessary `tctx` variable, use `ctx` directly. >> - Simplified code with no functional changes. >> > > I would rephrase the short to simply say Remove unused variable, > as refactor implies more extensive changes than what this patch > is actually doing. > > Please write complete sentences instead of bullet points in the > change log. > > How did you find this problem? Do include the details on how > in the change log. > >> Signed-off-by: Amit Vadhavana <av2082000@gmail.com> >> --- >> tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c >> b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c >> index 66dec47e3ca3..732e89fe99c0 100644 >> --- a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c >> +++ b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c >> @@ -56,16 +56,15 @@ TEST(flags_zero_lsm_set_self_attr) >> TEST(flags_overset_lsm_set_self_attr) >> { >> const long page_size = sysconf(_SC_PAGESIZE); >> - char *ctx = calloc(page_size, 1); >> + struct lsm_ctx *ctx = calloc(page_size, 1); > > Why not name this tctx and avoid changes to the ASSERT_EQs > below? In the realm of linux security modules ctx is short for "context". I used tctx here because I was lazy. It would be much better to drop tctx, even if it means a tiny bit more change. > >> __u32 size = page_size; >> - struct lsm_ctx *tctx = (struct lsm_ctx *)ctx; >> ASSERT_NE(NULL, ctx); >> if (attr_lsm_count()) { >> - ASSERT_LE(1, lsm_get_self_attr(LSM_ATTR_CURRENT, tctx, &size, >> + ASSERT_LE(1, lsm_get_self_attr(LSM_ATTR_CURRENT, ctx, &size, >> 0)); >> } >> - ASSERT_EQ(-1, lsm_set_self_attr(LSM_ATTR_CURRENT | >> LSM_ATTR_PREV, tctx, >> + ASSERT_EQ(-1, lsm_set_self_attr(LSM_ATTR_CURRENT | >> LSM_ATTR_PREV, ctx, >> size, 0)); >> free(ctx); > > You have to change this tctx for sure. > > With these changes: > > Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> > > Paul, James, > > Please do let me know if you would me to take this through > kselftest tree. > > thanks, > -- Shuah > >
On 11/14/24 09:55, Casey Schaufler wrote: > > On 11/14/2024 8:25 AM, Shuah Khan wrote: >> On 11/12/24 11:28, Amit Vadhavana wrote: >>> - Remove unnecessary `tctx` variable, use `ctx` directly. >>> - Simplified code with no functional changes. >>> >> >> I would rephrase the short to simply say Remove unused variable, >> as refactor implies more extensive changes than what this patch >> is actually doing. >> >> Please write complete sentences instead of bullet points in the >> change log. >> >> How did you find this problem? Do include the details on how >> in the change log. >> >>> Signed-off-by: Amit Vadhavana <av2082000@gmail.com> >>> --- >>> tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 7 +++---- >>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> diff --git a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c >>> b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c >>> index 66dec47e3ca3..732e89fe99c0 100644 >>> --- a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c >>> +++ b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c >>> @@ -56,16 +56,15 @@ TEST(flags_zero_lsm_set_self_attr) >>> TEST(flags_overset_lsm_set_self_attr) >>> { >>> const long page_size = sysconf(_SC_PAGESIZE); >>> - char *ctx = calloc(page_size, 1); >>> + struct lsm_ctx *ctx = calloc(page_size, 1); >> >> Why not name this tctx and avoid changes to the ASSERT_EQs >> below? > > In the realm of linux security modules ctx is short for "context". > I used tctx here because I was lazy. It would be much better to > drop tctx, even if it means a tiny bit more change. > Makes sense. Amit, you can ignore this comment about tctx and ctx. Please do fix others about the change log and short log. thanks, -- Shuah
On 14/11/24 10:38 pm, Shuah Khan wrote: > On 11/14/24 09:55, Casey Schaufler wrote: >> >> On 11/14/2024 8:25 AM, Shuah Khan wrote: >>> On 11/12/24 11:28, Amit Vadhavana wrote: >>>> - Remove unnecessary `tctx` variable, use `ctx` directly. >>>> - Simplified code with no functional changes. >>>> >>> >>> I would rephrase the short to simply say Remove unused variable, >>> as refactor implies more extensive changes than what this patch >>> is actually doing. >>> >>> Please write complete sentences instead of bullet points in the >>> change log. V2: https://lore.kernel.org/all/20241116152136.10635-1-av2082000@gmail.com/ >>> >>> How did you find this problem? Do include the details on how >>> in the change log. While exploring the kselftest framework. I found the this problem. >>> >>>> Signed-off-by: Amit Vadhavana <av2082000@gmail.com> >>>> --- >>>> tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 7 +++---- >>>> 1 file changed, 3 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c >>>> b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c >>>> index 66dec47e3ca3..732e89fe99c0 100644 >>>> --- a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c >>>> +++ b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c >>>> @@ -56,16 +56,15 @@ TEST(flags_zero_lsm_set_self_attr) >>>> TEST(flags_overset_lsm_set_self_attr) >>>> { >>>> const long page_size = sysconf(_SC_PAGESIZE); >>>> - char *ctx = calloc(page_size, 1); >>>> + struct lsm_ctx *ctx = calloc(page_size, 1); >>> >>> Why not name this tctx and avoid changes to the ASSERT_EQs >>> below? >> >> In the realm of linux security modules ctx is short for "context". >> I used tctx here because I was lazy. It would be much better to >> drop tctx, even if it means a tiny bit more change. >> > > Makes sense. > > Amit, you can ignore this comment about tctx and ctx. Please do fix > others about the change log and short log. > > thanks, > -- Shuah >
diff --git a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c index 66dec47e3ca3..732e89fe99c0 100644 --- a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c +++ b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c @@ -56,16 +56,15 @@ TEST(flags_zero_lsm_set_self_attr) TEST(flags_overset_lsm_set_self_attr) { const long page_size = sysconf(_SC_PAGESIZE); - char *ctx = calloc(page_size, 1); + struct lsm_ctx *ctx = calloc(page_size, 1); __u32 size = page_size; - struct lsm_ctx *tctx = (struct lsm_ctx *)ctx; ASSERT_NE(NULL, ctx); if (attr_lsm_count()) { - ASSERT_LE(1, lsm_get_self_attr(LSM_ATTR_CURRENT, tctx, &size, + ASSERT_LE(1, lsm_get_self_attr(LSM_ATTR_CURRENT, ctx, &size, 0)); } - ASSERT_EQ(-1, lsm_set_self_attr(LSM_ATTR_CURRENT | LSM_ATTR_PREV, tctx, + ASSERT_EQ(-1, lsm_set_self_attr(LSM_ATTR_CURRENT | LSM_ATTR_PREV, ctx, size, 0)); free(ctx);
- Remove unnecessary `tctx` variable, use `ctx` directly. - Simplified code with no functional changes. Signed-off-by: Amit Vadhavana <av2082000@gmail.com> --- tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)