Hi Janusz,
[...]
> > > + cpuinfo = fopen("/proc/cpuinfo", "r");
> > > + if (igt_debug_on(!cpuinfo))
> > > + return UINT_MAX;
> > Could this result in running the test on a slower machine
> > unintentionally? fopen() could fail, the bogomips() would return
> > a large value and wrapper function would behave as if the test
> > is running on a faster platform.
>
> Yes, if opening /proc/cpuinfo fails then my approach could result in running
> the test on a machine that occurs too slow. If that happens then an abort
> caused by soft lockup will be reported again, but this time with the fopen()
> failure also pointed out among debug messages. But why could that failing
> fopen() happen in practice?
>
Yeah, you are right, we do not expect this to fail.
> One possible case is an environment with no /proc/cpuinfo (non-Linux, e.g.,
> FreeBSD).
We do not run the same code on non-Linux OSes, do we?
>
> >
> > > +
> > > + while (getline(&line, &size, cpuinfo) != -1) {
> > It is unlikely, but getline() may fail with EINVAL or ENOMEM, so
> > maybe while(getline(&line, &size, cpuinfo) > 0)?
>
> No, getline returns -1 on any error, errno provides those extra error codes,
> if that's what you had on mind.
Yes, you are right, my mistake.
>
> >
> > > + char *colon;
> > > +
> > > + if (strncmp(line, "bogomips", 8))
> > > + continue;
> > > +
> > > + colon = strchr(line, ':');
> > > + if (igt_debug_on(!colon))
> > > + bogomips = 0;
> > > + else
> > > + bogomips = atoi(colon + 1);
> > > +
> > > + if (igt_debug_on(!bogomips))
> > > + break;
> > > +
> > > + ret += bogomips;
> > > + }
> > > + free(line);
> > > + fclose(cpuinfo);
> > > +
> > > + return igt_debug_on(!bogomips) ? UINT_MAX : ret;
> > Same as my first comment in this function: will this not cause
> > sporadic runs on machines that are supposed to skip the test?
>
> Yes, but if you think that's wrong then please first think of e.g. fopen()
> failing on a fast machine for some reason, and then suggest a behaviour that
> you think would be more correct in any of possible cases. My thinking is
> that
> it's more important to execute the selftest and preserve coverage than fail
> on
> potential test issues that don't mean the selftest can't be executed.
No, we can leave it as is. I was overthinking this, I believe.
>
> >
> > > +}
> > > +
> > > +static int wrapper(const char *dynamic_name,
> > The name could be more descriptive about what function does/is
> > used for, so maybe "check_skip_wrapper" would better achieve
> > that?
>
> If that was a global function then sure, its name should be more descriptive,
> but here in this context I don't think that name may introduce any doubts.
> But anyway, I can try to propose a more descriptive name if v2 is needed.
Fair enough. One benefit of keeping this function named
"wrapper" would be that we may put anything related to detecting
environment capabilities before running the test there. On the
other hand, if we'd require more wrappers in the future, then
we may address their naming then.
Reviewed-by: Krzysztof Karas <[email protected]>
--
Best Regards,
Krzysztof
