Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Mon, 20 Nov 2023 22:45:27 +0530
From:   Yuran Pereira <yuran.pereira@hotmail.com>
To:     Yonghong Song <yonghong.song@linux.dev>
Cc:     bpf@vger.kernel.org, andrii@kernel.org, andrii.nakryiko@gmail.com,
        mykolal@fb.com, ast@kernel.org, martin.lau@linux.dev,
        song@kernel.org, john.fastabend@gmail.com, kpsingh@kernel.org,
        sdf@google.com, haoluo@google.com, jolsa@kernel.org,
        shuah@kernel.org, linux-kselftest@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH bpf-next v2 1/4] selftests/bpf: Replaces the usage of
 CHECK calls for ASSERTs in bpf_tcp_ca
Message-ID: <GV1PR10MB6563596BC3E2B01F664010C7E8B4A@GV1PR10MB6563.EURPRD10.PROD.OUTLOOK.COM>
References: <GV1PR10MB6563AECF8E94798A1E5B36A4E8B6A@GV1PR10MB6563.EURPRD10.PROD.OUTLOOK.COM>
 <GV1PR10MB6563A7938B9B403861CA88F3E8B6A@GV1PR10MB6563.EURPRD10.PROD.OUTLOOK.COM>
 <14c7dea0-242c-4b47-929c-7cbd1d7e202a@linux.dev>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <14c7dea0-242c-4b47-929c-7cbd1d7e202a@linux.dev>
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: GV1PR10MB6563:EE_|GVXPR10MB5816:EE_
X-MS-Office365-Filtering-Correlation-Id: 1df99e4d-e42f-4315-370b-08dbe9ec555e
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: KgnNmONZ2M3DTu+BRL2tz1deS3FK8WAMerR7o491RU9zCFL0Zg2x30iynZA6UrmYrQg2wPhcuQlhLn7k4EwGFUknN0MnDx1ChfYBvitYMPAVmHozbEwjXizlywQctmHJVzt/04nfF37btvUI2nsB1U8V+gF/qwXah0xqNRWDRhO14Cu91FYg6WbKSAdcFW0CCv8CKbxrZMEtxehR+Q3+QVAUFq2lJD/LBzIChQSSQy8Gy6QxCB4LPhtYhOWTIrsJsI3S92GSUc8CJeh0IgagfOjNQ71dR28G21YJQ+GDkSaWjViynXUYnctszJZnx0VLj+nGhrdVnHhfdtd4B0t99E+OTG0kug4Yp4GGmV6BaF0VIfuuIEJ0BNCXHS0vvNlWgLG063T53ZT2GWPxeFHW1tbX4F1jeIh4OeyAOir30CaVe6O+x7O7nIn6SkYnrPq0bhZyW7MuwNBXT1iBz90PV/DP7yb2dGz/EBOrkVLKnxlUdfjPrdvTdd1nCxvVlkeGKv0z2rBKt3lAbPlnGswISXnUnqGWqitwpISEg0/2s4Y=
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?rxytGJiBvNUMIQaaF7TOgKMogBqADD3miG79IylrbRl022nQsaaM68oxKcv8?=
 =?us-ascii?Q?Fz+kae5r5ZwpOm1qVVqgqF2Z60Xt2cNlOLpYoAxdeU4juwubPv6UNpXqMqL4?=
 =?us-ascii?Q?yHTBso84It4Tom0e5CNoFXcKQ/PJpfFFT3xzVRYTsqrVt78LAK2A3PxYNK33?=
 =?us-ascii?Q?AwBQgQyB/MMs6EB/X+n42s0v21v0dl8SCMWjg6FRSOnW6lAHYwu2s1RVtFPu?=
 =?us-ascii?Q?l7eticGejThqDAYXi91HKIJNZ21Z6cNsjJu2X1K68ZEyxL6vL0YDHsePQ725?=
 =?us-ascii?Q?9e4G0LemnbttjVU4pjnhIq6PoC5hhnZJbRgFf8As3Z8QP7e2I/mqSFPKWDwR?=
 =?us-ascii?Q?uEXo6L0p/E/dCg0viFxDAXbpB+MKi2w+W8t9WGOdBtNvTZSn7zio1s/4RgVL?=
 =?us-ascii?Q?6BLObCc/QC1yuwSawR4vNM4jeK39bBO7tcvmWpACq/HTFQCTuYYGN2mDs84g?=
 =?us-ascii?Q?QYEJ9114EMp3K2u5kD30I6CxIfzWXZtR5kob9Z24FISX+AT5FdMg9j/esEBf?=
 =?us-ascii?Q?5wY7PSqjXBcRV1Rz2j1y+e7qUBiX0fkMxKq03gvQWs7reVBboROj16DX3RkM?=
 =?us-ascii?Q?yenAU/pUcljzc4+asegTRis+rkvrcA4vX9NZBFUyR0WxGgso+WmDQyMg3C9J?=
 =?us-ascii?Q?J9ju4QDAyILtwhh4A4TS8gk0XXnu0X2LO+zyh+rcIoiaPGWB4Sp1L8K9hX6C?=
 =?us-ascii?Q?MUurtPIf6QALIxhUx0WAuk2DjuoD/tboZFARj+7NRn3AYbEILZ9efXqfyBjU?=
 =?us-ascii?Q?0XfwJFC70GWmpGA06O3i85RsA20Jk1kZJPANPpOWA8Z8xYQrlH/lD2DkKgVw?=
 =?us-ascii?Q?TZztifl4xHV1fMAVhJO3O5SFmKe7+96+4IJRU+IdN6FEC26byHn88lT2q7kr?=
 =?us-ascii?Q?OawlZrNZsNZa5ECK05V9QRZRLFJJc1yNZp758gns1MQnWTjBmZQye1CSi6Bf?=
 =?us-ascii?Q?lhpliov5vP3ZoSUyyutKcBKlb9+J4WrMXQYTS2KAMJ6hvrxRZR7Szl6AvMdf?=
 =?us-ascii?Q?DSQ+Q6d66P45/QP6iHjXyfmrQW/P617sgg6VyFNKPPyHi3iY9pxJ+4I/dU3j?=
 =?us-ascii?Q?Ik2UcVaGM3IU1ewfbBOjVfUa9Sv6mq/NCukPmEiSP+N+mL9XQIJLx7Up23k9?=
 =?us-ascii?Q?BpG4IK5WdMbJMTLfVKY/oPD5SJBR4f/VifB2uY4MfSredYm0R05Pw2RbjMJk?=
 =?us-ascii?Q?8UfORJZk/QeqvnnNKEwpqYLm6TQGu7eu2g/CB8Dvy5tx4J9VT9QcgRhDPyE?=
 =?us-ascii?Q?=3D?=
X-OriginatorOrg: sct-15-20-4755-11-msonline-outlook-6b909.templateTenant
X-MS-Exchange-CrossTenant-Network-Message-Id: 1df99e4d-e42f-4315-370b-08dbe9ec555e
X-MS-Exchange-CrossTenant-AuthSource: GV1PR10MB6563.EURPRD10.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Nov 2023 17:15:46.0520
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: GVXPR10MB5816
X-Spam-Status: No, score=0.6 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,FORGED_HOTMAIL_RCVD2,FREEMAIL_FORGED_FROMDOMAIN,
	FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=no
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Mon, 20 Nov 2023 09:16:33 -0800 (PST)

Hello Yonghong,
On Mon, Nov 20, 2023 at 07:22:59AM -0800, Yonghong Song wrote:
> > -		if (CHECK(!err || errno != ENOENT,
> > -			  "bpf_map_lookup_elem(sk_stg_map)",
> > -			  "err:%d errno:%d\n", err, errno))
> > +		if (!ASSERT_NEQ(err, 0, "bpf_map_lookup_elem(sk_stg_map)") ||
> 
> !ASSERT_ERR(err, "bpf_map_lookup_elem(sk_stg_map)")
> might be simpler than !ASSERT_NEQ(..).
> 
Sure, that makes sense. I'll change it in v3.
> > -	pthread_join(srv_thread, &thread_ret);
> > -	CHECK(IS_ERR(thread_ret), "pthread_join", "thread_ret:%ld",
> > -	      PTR_ERR(thread_ret));
> > +	err = pthread_join(srv_thread, &thread_ret);
> > +	ASSERT_OK(err, "pthread_join");
> 
> The above is not equivalent to the original code.
> The original didn't check pthread_join() return as it
> is very very unlikely to fail. And check 'thread_ret'
> is still needed.
> 
Yes that is true, but the v1 [1] broke the tests because the
ASSERT_OK_PTR(thread_ret, "pthread_join") kept failing, even
though all the asserts within the `server()` function itself
passed.

Also, isn't asserting `thread_ret` technically checking the
`server()` function instead of `pthread_join`? So should we
have two asserts here? One for `server` and one for `pthread_join`
or is it not necessary?
i.e:
```
ASSERT_OK_PTR(thread_ret, "server");
ASSERT_OK(err, "pthread_join");
```

Upon taking a second look, I now think that the reason why
`ASSERT_OK_PTR(thread_ret, "pthread_join");` failed in v1 might
have been because it calls `libbpf_get_error` which returns
`-errno` when the pointer is `NULL`.

Since `server`'s return value is not a bpf address, which
`ASSERT_OK_PTR` expects it to be, do you that think we should
explicitly set `errno = 0` prior to returning NULL on server?
That way that assert would pass even when the pointer is NULL
(which is the case when `server` returns successfuly).

[1] - https://lore.kernel.org/lkml/GV1PR10MB6563A0BE91080E6E8EC2651DE8B0A@GV1PR10MB6563.EURPRD10.PROD.OUTLOOK.COM/

As always, thank you for your feedback.

Yuran Pereira