[LTP] [RESEND 2/2] network/lib6/asapi_02: Convert into new api
Petr Vorel
pvorel@suse.cz
Mon Apr 17 16:47:58 CEST 2023
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 –
* void ICMP6_FILTER_SETPASSALL(struct icmp6_filter *filterp)
* – 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
More information about the ltp
mailing list