Thanks Suka for your review comments. 

Veerendra,

Can you please update us with a new patch satisfying Suka´s comments ?

Regards--
Subrata

On Fri, 2008-10-17 at 19:46 -0700, [EMAIL PROTECTED] wrote:
> Veerendra [EMAIL PROTECTED] wrote:
> > Hi
> >
> >   Attaching the patch for the testcase on PIDNS.
> 
> I just have some coding style/nits and a couple of minor comments below.
> 
> >
> > Assertion:
> > 1. kill -9 1 from inside a container does not kill container
> > Steps:
> > a) create container
> > b) kill -9 1
> > c) Should not kill the containers
> >
> > Test Result: Currently this is failing, which eventually be passed.
> >
> > # ./pidns04
> > pid_namespace4    0  INFO  :  PIDNS test is running inside container
> > pid_namespace4    1  FAIL  :  Container init is killed by SIGKILL !!!
> > pid_namespace4    2  FAIL  :  Container init pid got killed by signal 9
> >
> > Regards
> > Veerendra C
> >
> >
> 
> | diff -uprN testcases/kernel/containers/pidns.old/pidns04.c 
> testcases/kernel/containers/pidns/pidns04.c
> | --- testcases/kernel/containers/pidns.old/pidns04.c 1970-01-01 
> 05:30:00.000000000 +0530
> | +++ testcases/kernel/containers/pidns/pidns04.c     2008-10-14 
> 16:48:28.000000000 +0530
> | @@ -0,0 +1,165 @@
> | +/*
> | +* Copyright (c) International Business Machines Corp., 2007
> | +* This program is free software; you can redistribute it and/or modify
> | +* it under the terms of the GNU General Public License as published by
> | +* the Free Software Foundation; either version 2 of the License, or
> | +* (at your option) any later version.
> | +* This program is distributed in the hope that it will be useful,
> | +* but WITHOUT ANY WARRANTY; without even the implied warranty of
> | +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
> | +* the GNU General Public License for more details.
> | +* You should have received a copy of the GNU General Public License
> | +* along with this program; if not, write to the Free Software
> | +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> | +*
> | +***************************************************************************
> | +
> | +* File: pidns04.c
> | +*
> | +* Description:
> | +*  The pidns04.c testcase builds into the ltp framework to verify
> | +*  the basic functionality of PID Namespace.
> | +*
> | +* Verify that:
> | +* 1. When parent clone a process with flag CLONE_NEWPID, the process ID of
> | +* child should be one.
> | +*
> | +* 2. When parent clone a process with flag CLONE_NEWPID, the parent 
> process ID
> | +* of should be zero.
> | +*
> | +* 3. The container init process (one), should not get killed by the 
> SIGKILL in 
> | +* the childNS
> | +*
> | +* Total Tests:
> | +*
> | +* Test Name: pidns04
> | +*
> | +* Test Assertion & Strategy:
> | +*
> | +* From main() clone a new child process with passing the clone_flag as 
> | +* CLONE_NEWPID. 
> | +* The container init, should not get killed by the SIGKILL inside the 
> child NS.
> | +* Usage: <for command-line>
> | +* pidns04
> | +*
> | +* History:
> | +*
> | +* FLAG DATE        NAME                            DESCRIPTION
> | +* 08/10/08      Veerendra C <[EMAIL PROTECTED]> Verifies killing of cont 
> init.
> | +*
> | 
> +*******************************************************************************/
> | +#define _GNU_SOURCE 1
> | +#include <sys/wait.h>
> | +#include <assert.h>
> | +#include <stdio.h>
> | +#include <stdlib.h>
> | +#include <unistd.h>
> | +#include <string.h>
> | +#include <errno.h>
> | +#include <usctest.h>
> | +#include <test.h>
> | +#include <libclone.h>
> | +
> | +#define INIT_PID        1
> | +#define CHILD_PID       1
> 
> Replace CHILD_PID with CINIT_PID ?
> 
> | +#define PARENT_PID      0
> | +
> | +char *TCID = "pid_namespace4";
> | +int TST_TOTAL=1;
> | +int     fd[2] ;
> 
> white-space
> 
> | +void cleanup(void);
> | +
> | +/*
> | + * child_fn1() - Inside container
> | +*/
> | +static int child_fn1(void *ttype)
> | +{
> | +   pid_t cpid, ppid;
> | +   cpid = getpid();
> | +   ppid = getppid();
> | +   char mesg[] = "I was not killed !";
> | +           /* Child process closes up read side of pipe */
> | +           close(fd[0]);
> 
> indentation
> 
> | +
> | +   /* Comparing the values to make sure pidns is created correctly */
> | +   if(( cpid == CHILD_PID) && ( ppid == PARENT_PID ) ) {
> | +           tst_resm(TINFO, "PIDNS test is running inside container");
> | +           kill(INIT_PID, SIGKILL);
> | +           /* Verifying whether the container init is not killed, "
> | +            If so writing into the pipe created in the parent NS" */
> 
> Coding style - space after '((' and before ')'
> 
> The comments don't seem to help explaining what is going on here unless
> we go back and note that CHILD_PID == INIT_PID.
> 
> How about:
> 
>       if ((cpid == CINIT_PID && ((ppid == PARENT_PID)) {
> 
>               kill(CINIT_PID, SIGKILL);
>               /* We did not kill ourselves. Let parent know */
>       }
> 
> | +
> | +           /* Send "mesg" through the write side of pipe */
> | +           write(fd[1], mesg, (strlen(mesg)+1));
> | +   }
> | +   else {
> | +           tst_resm(TFAIL, "FAIL: Got unexpected result of"
> | +           " cpid=%d ppid=%d\n", cpid, ppid);
> | +   }
> | +   close(fd[1]);
> | +   cleanup();
> | +
> | +   /* NOT REACHED */
> | +   return 0;
> | +}
> | +
> | +/***********************************************************************
> | +*   M A I N
> | +***********************************************************************/
> | +
> | +int main(int argc, char *argv[])
> | +{
> | +   int ret, status, nbytes;
> | +        char    readbuffer[80];
> 
> indentation
> 
> | +
> | +   pipe(fd);
> 
> Unlikely to fail, but would be good to check for error from pipe().
> 
> | +   ret = do_clone_unshare_test(T_CLONE, CLONE_NEWPID, child_fn1, NULL);
> | +   if ((wait(&status)) < 0) {
> | +           tst_resm(TWARN, "wait() failed, skipping this test case");
> | +           /* Cleanup & continue with next test case */
> | +           cleanup();
> | +   }
> 
> If relying on the message in pipe, do we still need the wait() or
> the if(WTERMSIG()) below
> 
> | +   if (ret == -1) {
> | +           tst_resm(TFAIL, "clone() Failed, errno = %d :"
> | +                   " %s", ret, strerror(ret));
> | +           /* Cleanup & continue with next test case */
> | +           cleanup();
> | +   }
> | +
> | +   /* Parent process closes up write side of pipe */
> | +   close(fd[1]);
> | +   /* Read in a string from the pipe */
> | +   nbytes = read(fd[0], readbuffer, sizeof(readbuffer));
> | +
> | +   if (nbytes !=0 ) {
> 
> nit-pick: (nbytes > 0) would be better (in case read() fails for some
> unlikely reason - or check (nbytes < 0) separately.
> 
> | +           tst_resm(TPASS, "Container init : %s", readbuffer);
> | +   }
> | +   else {
> | +           tst_resm(TFAIL, "Container init is killed by SIGKILL !!!");
> | +   }
> | +
> | +   if (WTERMSIG(status)) {
> | +           tst_resm(TFAIL, "Container init pid got killed by signal %d",
> | +           WTERMSIG(status));
> | +   }
> 
> Strictly speaking, WTERMSIG() is meaningful only if WIFSIGNALED() is true.
> 
> | +        /* cleanup and exit */
> | +   cleanup();
> | +   close(fd[0]);
> | +
> | +   /*NOTREACHED*/
> | +   return 0;
> | +
> | +}  /* End main */
> | +
> | +/*
> | + * cleanup() - performs all ONE TIME cleanup for this test at
> | + *             completion or premature exit.
> | + */
> | +void
> | +cleanup()
> | +{
> | +   /* Clean the test testcase as LTP wants*/
> | +   TEST_CLEANUP;
> | +
> | +   /* exit with return code appropriate for results */
> | +   tst_exit();
> | +}
> | diff -uprN testcases/kernel/containers/pidns.old/runpidnstest.sh 
> testcases/kernel/containers/pidns/runpidnstest.sh
> | --- testcases/kernel/containers/pidns.old/runpidnstest.sh   2008-09-30 
> 18:06:38.000000000 +0530
> | +++ testcases/kernel/containers/pidns/runpidnstest.sh       2008-10-14 
> 11:25:40.000000000 +0530
> | @@ -35,4 +35,9 @@ if [ $? -ne 0 ]; then
> |     exit_code="$?"
> |     exit $exit_code
> |  fi
> | +pidns04
> | +if [ $? -ne 0 ]; then
> | +   exit_code="$?"
> | +   exit $exit_code
> | +fi
> |  exit $exit_code
> 
> 
> -------------------------------------------------------------------------
> This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
> Build the coolest Linux based applications with Moblin SDK & win great prizes
> Grand prize is a trip for two to an Open Source event anywhere in the world
> http://moblin-contest.org/redirect.php?banner_id=100&url=/
> _______________________________________________
> Ltp-list mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/ltp-list


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Reply via email to