[LTP] [RESEND 2/2] network/lib6/asapi_02: Convert into new api
Yang Xu (Fujitsu)
xuyang2018.jy@fujitsu.com
Tue Apr 18 04:56:49 CEST 2023
Hi Petr
> 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.
Look simple and clear.
>> *
>> - * 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.
Of course.
>
> (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.)
Agree.
>
>> *
>> - * 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.
Yes, will remove the previous sentence.
>
>> + *
>> + * 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)
OK.
>
>> + *
>> + * 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.
OK.
>
>> +#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
> ...
Indeed, I don't know which macro was used by this case.
>
>
>> + {"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).
Good catch.
>
>> + {"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.
Yes.
>
> That could be avoided, if values are assigned with help of macro
> (code below would require to rename enum values).
Yes.
>
> 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);
> }
Of course, OK.
> ...
>> +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>
Thanks for your patient review, I will add these two flags and then merged.
Best Regards
Yang Xu
>
> Kind regards,
> Petr
More information about the ltp
mailing list