| Message ID | 20210521102528.21102-6-rpalethorpe@suse.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cfs_bandwidth01 and CGroup API | expand |
Hi Richard, On Fri, May 21, 2021 at 6:26 PM Richard Palethorpe via ltp <ltp@lists.linux.it> wrote: > > Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> > --- > lib/tst_cgroup.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c > index 74746f13e..6d94ea41c 100644 > --- a/lib/tst_cgroup.c > +++ b/lib/tst_cgroup.c > @@ -599,6 +599,12 @@ void tst_cgroup_require(const char *const ctrl_name, > struct cgroup_ctrl *const ctrl = cgroup_find_ctrl(ctrl_name); > struct cgroup_root *root; > > + if (!ctrl) { > + tst_brk(TBROK, "'%s' controller is unknown to LTP", ctrl_name); > + tst_brk(TBROK, "Calling %s in cleanup?", __func__); > + return; It'd never go here to perform a return because the first tst_brk will break the test directly. And, I don't know why we need the second tst_brk to show calling in cleanup, is that possible? > + } > + > if (!options) > options = &default_opts; > > -- > 2.31.1 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp > -- Regards, Li Wang
Hello Li, Li Wang <liwang@redhat.com> writes: > Hi Richard, > > On Fri, May 21, 2021 at 6:26 PM Richard Palethorpe via ltp > <ltp@lists.linux.it> wrote: >> >> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> >> --- >> lib/tst_cgroup.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c >> index 74746f13e..6d94ea41c 100644 >> --- a/lib/tst_cgroup.c >> +++ b/lib/tst_cgroup.c >> @@ -599,6 +599,12 @@ void tst_cgroup_require(const char *const ctrl_name, >> struct cgroup_ctrl *const ctrl = cgroup_find_ctrl(ctrl_name); >> struct cgroup_root *root; >> >> + if (!ctrl) { >> + tst_brk(TBROK, "'%s' controller is unknown to LTP", ctrl_name); >> + tst_brk(TBROK, "Calling %s in cleanup?", __func__); >> + return; > > It'd never go here to perform a return because the first tst_brk > will break the test directly. And, I don't know why we need the > second tst_brk to show calling in cleanup, is that possible? It can return if it is called during cleanup. tst_cgroup_require should not be called from cleanup. However someone can do it by accident. We probably need two versions of tst_brk. One which can return if called from cleanup and one which does not. I suspect most tst_brk callers assume it will not return. It is really only some safe library functions which can handle that. > > >> + } >> + >> if (!options) >> options = &default_opts; >> >> -- >> 2.31.1 >> >> >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp >>
> >> + if (!ctrl) { > >> + tst_brk(TBROK, "'%s' controller is unknown to LTP", ctrl_name); > >> + tst_brk(TBROK, "Calling %s in cleanup?", __func__); > >> + return; > > > > It'd never go here to perform a return because the first tst_brk > > will break the test directly. And, I don't know why we need the > > second tst_brk to show calling in cleanup, is that possible? > > It can return if it is called during cleanup. tst_cgroup_require should > not be called from cleanup. However someone can do it by accident. Well, I see. But TBH, here worries too much about the unexpected situation:). > We probably need two versions of tst_brk. One which can return if called > from cleanup and one which does not. I suspect most tst_brk callers > assume it will not return. It is really only some safe library functions > which can handle that. Yes, sounds reasonable to me. [Cc Cyril if he has more advice on this] -- Regards, Li Wang
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c index 74746f13e..6d94ea41c 100644 --- a/lib/tst_cgroup.c +++ b/lib/tst_cgroup.c @@ -599,6 +599,12 @@ void tst_cgroup_require(const char *const ctrl_name, struct cgroup_ctrl *const ctrl = cgroup_find_ctrl(ctrl_name); struct cgroup_root *root; + if (!ctrl) { + tst_brk(TBROK, "'%s' controller is unknown to LTP", ctrl_name); + tst_brk(TBROK, "Calling %s in cleanup?", __func__); + return; + } + if (!options) options = &default_opts;
Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> --- lib/tst_cgroup.c | 6 ++++++ 1 file changed, 6 insertions(+)