> > +static int PidDistance(pid_t first, pid_t second);
> 
> Just to cite from linux kernel conding style:
> 
> "mixed-case names are frowned upon"
OK.

> Once again, this comment is useless ;).
OK.

> Here as well.
OK.

> I would remove this one as well. Everybody who plays with lowlevel OS
> api should know how fork works.
OK, I just realised this inconsistent as well.

> Here as well.
OK.

> 
> > +                   if (lc > 0) {
> > +                           printf("last_pid = %d\npid = %d\n", last_pid, 
> > pid);
> > +                           distance = PidDistance(last_pid, pid);
> > +                           if (distance == 0 || distance > 30000) {
> 
> Hmm are you sure that distance > 30000 is good thing to assert? Who says
> that two pids keeps at least 2768 distance and even then, who says that
> 32768 is maximal pid (see below)?
It was using a expert's advise here,
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5fdee8c4a5e1800489ce61963208f8cc55e42ea1

> > +   return 0;
> Rather use tst_exit() here.
check() function is used in main() here,

TEST(check())
cleanup();

As you can see that the test has not finished yet after returned. It might also 
break existing LTP standard options like,

  -c n    Run n copies concurrently
  -I x    Execute test for x seconds

as TEST() seems expecting a non-void function.

> > +   /* capture signals */
> > +   tst_sig(FORK, DEF_HANDLER, cleanup);
> > +
> > +   /* Pause if that option was specified */
> > +   TEST_PAUSE;
> > +
> > +   tst_tmpdir();
> 
> Do you really need temporary directory to test fork() ?
Not really, but as mentioned earlier that those were copied from a standard LTP 
test template that I don't if some LTP internal things were using them. For 
example, LTP internal seems make use of tst_sig() and TEST_PAUSE in setup() for 
those standard options,

  -p      Pause for SIGUSR1 before starting
  -P x    Pause for x seconds between iterations

Therefore, I am concerning about remove those temporary directory creating 
might break other standard options and alike.

> > +/* The distance mod 32768 between two pids, where the first pid is
> > +   expected to be smaller than the second. */
> > +int PidDistance(pid_t first, pid_t second) {
> > +   return (second + 32768 - first) % 32768;
> > +}
> 
> The comment doesn't hold as when second is smaller than first the
> negative value would be smaller than 32768.
Again this is what the expert said. What would be your advise here?

> Moreover the maximal pid value may be changed by writing to
> "/proc/sys/kernel/pid_max", then this code is broken.
One thing I can do here is to backup/restore /proc/sys/kernel/pid_max and set a 
value explicitly to avoid ambiguity.

> Also I'm not sure if the result of such computation couldn't
> overflow/underflow as int may not be the same size as pid_t. 
Is this still an issue after set pid_max a value explicitly like 32768? Thanks.

CAI Qian

------------------------------------------------------------------------------
What happens now with your Lotus Notes apps - do you make another costly 
upgrade, or settle for being marooned without product support? Time to move
off Lotus Notes and onto the cloud with Force.com, apps are easier to build,
use, and manage than apps on traditional platforms. Sign up for the Lotus 
Notes Migration Kit to learn more. http://p.sf.net/sfu/salesforce-d2d
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Reply via email to