Re: [LTP] [RESEND 2/2] network/lib6/asapi_02: Convert into new api

[Petr Vorel <pvorel@suse.cz>]

Hi Xu,

> 1.make use of SAFE_RECV/SAFE_SENDTO macro
> 2.add detail description for this case
> 3.use more meaningful variable name or struct name

...
> diff --git a/testcases/network/lib6/asapi_02.c b/testcases/network/lib6/asapi_02.c
...
> +/*\
> + * [Description]
>   *
> - * 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.
> + * It is a basic test for ICMP6_FILTER.
nit: I'd prefer just: Basic test for ICMP6_FILTER.
>   *
> - * You should have received a copy of the GNU General Public License
> - * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + * For ICMP6_FILTER usage, refer to the following url
> + * https://man.openbsd.org/icmp6

nit: how about just:
For ICMP6_FILTER usage, refer to: https://man.openbsd.org/icmp6.

(Besides URL is an abbreviation, thus should be upper case, it will be in our
docs on single line, thus looks bad without dot in the end.)

>   *
> - * Author: David L Stevens
> + * Because of the extra functionality of ICMPv6 in comparison to ICMPv4, a
> + * larger number of messages may be potentially received on an ICMPv6 socket.
> + * Input filters may therefore be used to restrict input to a subset of the
> + * incoming ICMPv6 messages so only interesting messages are returned by the
> + * recv(2) family of calls to an application.
> +
> + * The icmp6_filter structure may be used to refine the input message set
> + * according to the ICMPv6 type. By default, all messages types are allowed
> + * on newly created raw ICMPv6 sockets. The following macros may be used to
> + * refine the input set:
> + *
> + * Macros used to manipulate filter value
nit: this and previous sentence are strange to be put together.

> + *
> + * void ICMP6_FILTER_SETPASSALL(struct icmp6_filter *filterp);
> + * Allow all incoming messages. filterp is modified to allow all message types.
We copy paste functions with their descriptions from https://man.openbsd.org/icmp6,
I was thinking if it's really useful, but let's keep it.

This is inline in our docs, thus looks bad:

Therefore I'd remove ';' and add &ndash;
* void ICMP6_FILTER_SETPASSALL(struct icmp6_filter *filterp)
* &ndash; Allow all incoming messages. filterp is modified to allow all message types.

That will result in docs:
void ICMP6_FILTER_SETBLOCK(int, struct icmp6_filter *filterp) – Ignore ICMPv6 messages with the given type. filterp is modified to ignore such messages.
instead of
void ICMP6_FILTER_SETBLOCK(int, struct icmp6_filter *filterp); Ignore ICMPv6 messages with the given type. filterp is modified to ignore such messages.
(both on single line)

> + *
> + * void ICMP6_FILTER_SETBLOCKALL(struct icmp6_filter *filterp);
> + * Ignore all incoming messages. filterp is modified to ignore all message types.
> + *
> + * void ICMP6_FILTER_SETPASS(int, struct icmp6_filter *filterp);
> + * Allow ICMPv6 messages with the given type. filterp is modified to allow such
> + * messages.
> + *
> + * void ICMP6_FILTER_SETBLOCK(int, struct icmp6_filter *filterp);
> + * Ignore ICMPv6 messages with the given type. filterp is modified to ignore
> + * such messages.
> + *
> + * int ICMP6_FILTER_WILLPASS(int, const struct icmp6_filter *filterp);
> + * Determine if the given filter will allow an ICMPv6 message of the given type.
> + *
> + * int ICMP6_FILTER_WILLBLOCK(int, const struct icmp6_filter *);
> + * Determine if the given filter will ignore an ICMPv6 message of the given type.
> + *
> + * The getsockopt(2) and setsockopt(2) calls may be used to obtain and install
> + * the filter on ICMPv6 sockets at option level IPPROTO_ICMPV6 and name ICMP6_FILTER
> + * with a pointer to the icmp6_filter structure as the option value.
>   */

NOTE: from system headers only <netinet/icmp6.h> is needed for compilation.
While some of headers are needed but included by library headers, at least
<sys/wait.h> is not needed.

> +#include "tst_test.h"
...
> -enum tt {
> -	T_WILLPASS,
> -	T_WILLBLOCK,
> +enum filter_macro {
>  	T_SETPASS,
>  	T_SETBLOCK,
>  	T_SETPASSALL,
> -	T_SETBLOCKALL
> +	T_SETBLOCKALL,
> +	T_WILLBLOCK,
> +	T_WILLPASS
>  };
...
> +static struct tcase {
> +	char *tname;
> +	unsigned char send_type;
> +	unsigned char filter_type;
> +	enum filter_macro test_macro;
> +	int pass_packet;
> +} tcases[] = {
> +	{"ICMP6_FILTER_SETPASS send type 20 filter type 20", 20, 20, T_SETPASS, 1},

First, I wonder what are these cryptic numbers 20, 21, 22.
It'd be great to replace numbers with constants.

For maybe one of these:

$ grep -E -e define.*[[:space:]]20$ -e define.*0x14 include/net*
include/netinet/igmp.h:#define IGMP_PIM                 0x14
include/netinet/in.h:#define IP_ORIGDSTADDR     20
include/netinet/in.h:#define IP_MAX_MEMBERSHIPS              20
include/netinet/in.h:#define IPV6_JOIN_GROUP         20
...

$ grep -E -e define.*[[:space:]]21$ -e define.*0x15 include/net*
include/netinet/igmp.h:#define IGMP_TRACE                       0x15
include/netinet/in.h:#define IP_MINTTL          21
include/netinet/in.h:#define IPV6_LEAVE_GROUP        21
...

$ grep -E -e define.*[[:space:]]22$ -e define.*0x16 include/net*
include/netinet/igmp.h:#define IGMP_V2_MEMBERSHIP_REPORT        0x16
include/netinet/in.h:#define IPPROTO_IDP      22
include/netinet/in.h:#define IP_NODEFRAG        22
include/netinet/in.h:#define IPV6_ROUTER_ALERT       22
...


> +	{"ICMP6_FILTER_SETPASS send type 20 filter type 21", 20, 21, T_SETPASS, 0},
> +	{"ICMP6_FILTER_SETBLOCK send type 20 filter type 20", 20, 20, T_SETBLOCK, 0},
> +	{"ICMP6_FILTER_SETBLOCK send type 20 filter type 21", 20, 21, T_SETBLOCK, 1},
> +	{"ICMP6_FILTER_PASSALL send type 20", 20, 0, T_SETPASSALL, 1},
> +	{"ICMP6_FILTER_PASSALL send type 20", 21, 0, T_SETPASSALL, 1},
Here is mistake: you claim "send type 20", but it's actually 21. That's actually
error from original code (copy pasted by you).

> +	{"ICMP6_FILTER_BLOCKALL send type 20", 20, 0, T_SETBLOCKALL, 0},
> +	{"ICMP6_FILTER_BLOCKALL send type 20", 21, 0, T_SETBLOCKALL, 0},
And here as well.

That could be avoided, if values are assigned with help of macro
(code below would require to rename enum values).

enum filter_macro {
	FILTER_SETPASS,
	FILTER_SETBLOCK,
	FILTER_PASSALL,
	FILTER_BLOCKALL,
	FILTER_WILLBLOCK,
	FILTER_WILLPASS
};

#define DESC(x, y, z) .tname = "ICMP6_" #x ", send type: " #y ", filter type: " \
	#z, .send_type = y, .filter_type = z, .test_macro = x

static struct tcase {
	char *tname;
	unsigned char send_type;
	unsigned char filter_type;
	enum filter_macro test_macro;
	int pass_packet;
} tcases[] = {
	{DESC(FILTER_SETPASS, 20, 20), .pass_packet = 1},
	{DESC(FILTER_SETPASS, 20, 21)},
	{DESC(FILTER_SETBLOCK, 20, 20)},
	{DESC(FILTER_SETBLOCK, 20, 21), .pass_packet = 1},
	{DESC(FILTER_PASSALL, 20, 20), .pass_packet = 1},
	{DESC(FILTER_PASSALL, 21, 0), .pass_packet = 1},
	{DESC(FILTER_BLOCKALL, 20, 0)},
	{DESC(FILTER_BLOCKALL, 21, 0)},
	{DESC(FILTER_WILLBLOCK, 20, 21)},
	{DESC(FILTER_WILLBLOCK, 20, 20), .pass_packet = 1},
	{DESC(FILTER_WILLPASS, 20, 21)},
	{DESC(FILTER_WILLPASS, 22, 22), .pass_packet = 1},
};
> +	{"ICMP6_FILTER_WILLBLOCK send type 20 filter type 21", 20, 21, T_WILLBLOCK, 0},
> +	{"ICMP6_FILTER_WILLBLOCK send type 20 filter type 20", 20, 20, T_WILLBLOCK, 1},
> +	{"ICMP6_FILTER_WILLPASS send type 20 filter type 21", 20, 21, T_WILLPASS, 0},
> +	{"ICMP6_FILTER_WILLPASS send type 22 filter type 22", 22, 22, T_WILLPASS, 1},
>  };

...
> +static void verify_icmp6_filter(unsigned int n)
>  {
> +	struct tcase *tc = &tcases[n];
>  	struct icmp6_filter i6f;
...
> +	int rc;
> +
> +	tst_res(TINFO, "Testing %s", tc->tname);
Please add space here (readability)
> +	switch (tc->test_macro) {
> +	case T_SETPASS:
> +		ICMP6_FILTER_SETBLOCKALL(&i6f);
> +		ICMP6_FILTER_SETPASS(tc->filter_type, &i6f);
> +		break;
> +	case T_SETPASSALL:
> +		ICMP6_FILTER_SETPASSALL(&i6f);
> +		break;
> +	case T_SETBLOCK:
> +		ICMP6_FILTER_SETPASSALL(&i6f);
> +		ICMP6_FILTER_SETBLOCK(tc->filter_type, &i6f);
> +		break;
> +	case T_SETBLOCKALL:
> +		ICMP6_FILTER_SETBLOCKALL(&i6f);
> +		break;
> +	case T_WILLBLOCK:
> +		ICMP6_FILTER_SETPASSALL(&i6f);
> +		ICMP6_FILTER_SETBLOCK(tc->filter_type, &i6f);
> +		rc = ICMP6_FILTER_WILLBLOCK(tc->send_type, &i6f);
> +		goto check_will_rc;
> +	case T_WILLPASS:
> +		ICMP6_FILTER_SETBLOCKALL(&i6f);
> +		ICMP6_FILTER_SETPASS(tc->filter_type, &i6f);
> +		rc = ICMP6_FILTER_WILLPASS(tc->send_type, &i6f);
> +		goto check_will_rc;
I'm not really a big fun of big switch, when some parts need goto.
TST_EXP_EXPR() and return can help to avoid it (see below).


> +	default:
> +		tst_brk(TBROK, "unknown test type %d", tc->filter_type);
> +		break;
>  	}
> +	SAFE_SETSOCKOPT(sf, IPPROTO_ICMPV6, ICMP6_FILTER, &i6f, sizeof(i6f));
> +	ic6_send(tc->send_type);
> +	rc = ic6_recv();
> +	if (rc < 0)
> +		return;
And please space here.
> +	if (rc != tc->pass_packet) {
> +		tst_res(TFAIL, "%s packet type %d unexpectedly",
> +				rc ? "pass" : "block", tc->send_type);
> +	} else {
> +		tst_res(TPASS, "%s packet type %d as expected",
> +				tc->pass_packet ? "pass" : "block", tc->send_type);
> +	}
This can be replaced by TST_EXP_EXPR().
> +	return;

> -	sf = SAFE_SOCKET(NULL, PF_INET6, SOCK_RAW, IPPROTO_ICMPV6);
> -
> -	int rv;
> +check_will_rc:
> +	if (rc != tc->pass_packet)
> +		tst_res(TFAIL, "rc %d != expected %d", rc, tc->pass_packet);
> +	else
> +		tst_res(TPASS, "expected rc %d", rc);
> +}

How about this? (goto is gone, TST_EXP_EXPR() simplifies the code).

	switch (tc->test_macro) {
	case FILTER_SETPASS:
		ICMP6_FILTER_SETBLOCKALL(&i6f);
		ICMP6_FILTER_SETPASS(tc->filter_type, &i6f);
		break;
	case FILTER_PASSALL:
		ICMP6_FILTER_SETPASSALL(&i6f);
		break;
	case FILTER_SETBLOCK:
		ICMP6_FILTER_SETPASSALL(&i6f);
		ICMP6_FILTER_SETBLOCK(tc->filter_type, &i6f);
		break;
	case FILTER_BLOCKALL:
		ICMP6_FILTER_SETBLOCKALL(&i6f);
		break;
	case FILTER_WILLBLOCK:
		ICMP6_FILTER_SETPASSALL(&i6f);
		ICMP6_FILTER_SETBLOCK(tc->filter_type, &i6f);
		rc = ICMP6_FILTER_WILLBLOCK(tc->send_type, &i6f);
		TST_EXP_EXPR(rc == tc->pass_packet, "%d (%d)", tc->pass_packet, rc);
		return;
	case FILTER_WILLPASS:
		ICMP6_FILTER_SETBLOCKALL(&i6f);
		ICMP6_FILTER_SETPASS(tc->filter_type, &i6f);
		rc = ICMP6_FILTER_WILLPASS(tc->send_type, &i6f);
		TST_EXP_EXPR(rc == tc->pass_packet, "%d (%d)", tc->pass_packet, rc);
		return;
	default:
		tst_brk(TBROK, "unknown test type %d", tc->filter_type);
		break;
	}

	SAFE_SETSOCKOPT(sf, IPPROTO_ICMPV6, ICMP6_FILTER, &i6f, sizeof(i6f));
	ic6_send(tc->send_type);
	rc = ic6_recv();
	if (rc < 0)
		return;

	TST_EXP_EXPR(rc == tc->pass_packet, "%s packet type %d",
				 rc ? "pass" : "block", tc->send_type);
}
...
> +static void cleanup(void)
> +{
> +	if (sall > -1)
> +		SAFE_CLOSE(sall);
Please add blank line here (readability).
> +	if (sf > -1)
> +		SAFE_CLOSE(sf);
>  }
> +
> +static struct tst_test test = {
> +	.needs_root = 1,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test = verify_icmp6_filter,
> +	.tcnt = ARRAY_SIZE(tcases)
> +};

NOTE: most of my comments are implemented in my fork, branch
yang_xu/asapi_02.fixes. Feel free to reuse it.

https://raw.githubusercontent.com/pevik/ltp/yang_xu/asapi_02.fixes/testcases/network/lib6/asapi_02.c

If you like the changes and use them, you can merge with

Reviewed-by: Petr Vorel <pvorel@suse.cz>

or even with :)
Co-developed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp