Re: [LTP] [PATCH] Add userns01 testcase to verify user namespace.

[Jan Stancek <jstancek@redhat.com>]

On 05/19/2015 09:31 PM, Yuan Sun wrote:
> Signed-off-by: Yuan Sun <sunyuan3@huawei.com>
> ---
>  testcases/kernel/containers/userns/Makefile        |  28 ++++++
>  testcases/kernel/containers/userns/README          |  22 +++++
>  testcases/kernel/containers/userns/userns01.c      | 104 +++++++++++++++++++++
>  testcases/kernel/containers/userns/userns_helper.h |  37 ++++++++
>  4 files changed, 191 insertions(+)
>  create mode 100644 testcases/kernel/containers/userns/Makefile
>  create mode 100644 testcases/kernel/containers/userns/README
>  create mode 100644 testcases/kernel/containers/userns/userns01.c
>  create mode 100644 testcases/kernel/containers/userns/userns_helper.h

Hi,

I suggest using checkpatch.pl, which can help you identify places
where you (presumably) copied also style issues from original code:
  line going over 80 characters
  mixing spaces and tabs
  whitespace at the end of lines

> 
> diff --git a/testcases/kernel/containers/userns/Makefile b/testcases/kernel/containers/userns/Makefile
> new file mode 100644
> index 0000000..6dfe694
> --- /dev/null
> +++ b/testcases/kernel/containers/userns/Makefile
> @@ -0,0 +1,28 @@
> +###############################################################################
> +#                                                                            ##
> +# Copyright (c) International Business Machines  Corp., 2007                 ##

You can use your name/company or "Linux Test Project" with current year

> +#                                                                            ##
> +# 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., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA    ##

Line above is clearly more than 80 characters. You can drop the part about
the address of FSF.

> +#                                                                            ##
> +###############################################################################
> +
> +top_srcdir		?= ../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +include $(abs_srcdir)/../Makefile.inc
> +
> +LDLIBS			:= -lpthread -lrt -lclone -lltp

Is -lpthread and -lrt needed here?

> +
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/containers/userns/README b/testcases/kernel/containers/userns/README
> new file mode 100644
> index 0000000..8011cff
> --- /dev/null
> +++ b/testcases/kernel/containers/userns/README

This README seems redundant, there is description in userns01.c

> @@ -0,0 +1,22 @@
> +################################################################################
> +##                                                                            ##
> +## 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., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA    ##
> +##                                                                            ##
> +################################################################################
> +
> +USERNS testcases Overview:
> +User namespaces allow per-namespace mappings of user and group IDs. This means that a process's user and group IDs inside a user namespace can be different from its IDs outside of the namespace. 
> diff --git a/testcases/kernel/containers/userns/userns01.c b/testcases/kernel/containers/userns/userns01.c
> new file mode 100644
> index 0000000..ca516e7
> --- /dev/null
> +++ b/testcases/kernel/containers/userns/userns01.c
> @@ -0,0 +1,104 @@
> +/*
> +* 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., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

This GPL block looks a bit mangled.

> +*
> +***************************************************************************
> +
> +* File: userns01.c

The above comments like what file we are in and "*" separator lines are useless.

> +*
> +* Description:
> +*  The userns01.c testcase builds into the ltp framework to verify
> +*  the basic functionality of USER Namespace.

The above description is too generic and doesn't really help to explain what
is this test about.

> +*
> +* Verify that:
> +*  If a user ID has no mapping inside the namespace, user ID and group 
> +* ID will be the value defined in the file /proc/sys/kernel/overflowuid, 65534.

This is good, that gives reader some clue what this test is about.

> +*
> +* Total Tests:
> +*
> +* Test Name: userns01
> +*
> +* History:
> +*
> +* FLAG DATE		NAME			DESCRIPTION
> +* 16/05/15  Yuan Sun <sunyuan3@huawei.com> Created this test
> +*
> +*******************************************************************************************/

All above can be removed as it's stored in git history.

> +#define _GNU_SOURCE
> +#include <sys/wait.h>
> +#include <assert.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <errno.h>
> +#include "test.h"
> +#include "libclone.h"
> +#include "userns_helper.h"
> +
> +char *TCID = "user_namespace1";
> +int TST_TOTAL = 1;
> +
> +#define OVERFLOWUID 65534

I'd suggest getting the value from /proc if possible, and rely on
hardcoded value only if that fails, for example in setup() by:
  SAFE_FILE_SCANF()

> +
> +/*
> + * child_fn1() - Inside a new user namespace
> + */
> +static int child_fn1(void *arg)
> +{
> +        int exit_val;
> +        int uid, gid;
> +        uid = geteuid();
> +        gid = getegid();
> +
> +        tst_resm(TINFO, "USERNS test is running in a new user namespace.");
> +        if (uid == OVERFLOWUID && gid == OVERFLOWUID) {
> +                printf("Got expected cpid and ppid\n");

The printf message needs updating, there's no cpid and ppid here.

> +                exit_val = 0;
> +        } else {
> +                printf("Got unexpected result of uid=%d gid=%d\n", uid, gid);
> +                exit_val = 1;
> +        }
> +
> +        return exit_val;
> +}
> +
> +static void setup(void)
> +{
> +	check_newuser();
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	int status;
> +	tst_parse_opts(argc, argv, NULL, NULL);
> +	setup();
> +
> +	TEST(do_clone_unshare_test(T_CLONE, CLONE_NEWUSER, child_fn1, NULL));
> +
> +	if (TEST_RETURN == -1) {
> +		tst_brkm(TFAIL | TTERRNO, NULL, "clone failed");
> +	} else if ((wait(&status)) == -1) {
> +		tst_brkm(TWARN | TERRNO, NULL, "wait failed");
> +	}
> +
> +	if (WIFEXITED(status) && WEXITSTATUS(status) != 0)
> +		tst_resm(TFAIL, "child exited abnormally");
> +	else if (WIFSIGNALED(status)) {
> +		tst_resm(TFAIL, "child was killed with signal = %d",
> +			 WTERMSIG(status));
> +	}

A TPASS message would be nice if everything goes as expected.

Regards,
Jan

> +
> +	tst_exit();
> +}
> +
> diff --git a/testcases/kernel/containers/userns/userns_helper.h b/testcases/kernel/containers/userns/userns_helper.h
> new file mode 100644
> index 0000000..36e75ad
> --- /dev/null
> +++ b/testcases/kernel/containers/userns/userns_helper.h
> @@ -0,0 +1,37 @@
> +/*
> +* 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.
> +*/
> +
> +#include "../libclone/libclone.h"
> +#include "test.h"
> +#include "safe_macros.h"
> +
> +static int dummy_child(void *v)
> +{
> +	(void) v;
> +	return 0;
> +}
> +
> +static int check_newuser(void)
> +{
> +	int pid, status;
> +
> +	if (tst_kvercmp(3, 8, 0) < 0)
> +		tst_brkm(TCONF, NULL, "CLONE_NEWUSER not supported");
> +
> +	pid = do_clone_unshare_test(T_CLONE, CLONE_NEWUSER, dummy_child, NULL);
> +	if (pid == -1)
> +		tst_brkm(TCONF | TERRNO, NULL, "CLONE_NEWUSER not supported");
> +	SAFE_WAIT(NULL, &status);
> +
> +	return 0;
> +}
> 


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list