Re: [LTP] [PATCH] kernel/uevent: Adjust the number of uevents dynamically in uevent02

[Dylan Jhong <dylan@andestech.com>]

On Thu, Sep 15, 2022 at 04:58:56PM +0800, Cyril Hrubis wrote:
Hi Cyril,

Thanks for reviewing this patch.

> Hi!
> > Signed-off-by: Dylan Jhong <dylan@andestech.com>
> > ---
> >  testcases/kernel/uevents/uevent02.c | 146 ++++++++++++++--------------
> >  1 file changed, 73 insertions(+), 73 deletions(-)
> > 
> > diff --git a/testcases/kernel/uevents/uevent02.c b/testcases/kernel/uevents/uevent02.c
> > index ce0cf757d..059320f1c 100644
> > --- a/testcases/kernel/uevents/uevent02.c
> > +++ b/testcases/kernel/uevents/uevent02.c
> > @@ -19,11 +19,71 @@
> >  #include <linux/if_tun.h>
> >  
> >  #include "tst_test.h"
> > +#include "tst_private.h"
> 
> This header is called private for a reason, the tst_kconfig_get() is not
> meant to be used from tests, you are supposed to call tst_config_read()
> as it's done in shmget02.c for example.
> 

Thanks. I will fix this in v2 patch.

> >  #include "uevent.h"
> >  
> >  #define TUN_PATH "/dev/net/tun"
> >  
> > +#define MAX_UEVENT 7
> > +
> > +struct uevent_desc add = {
> > +	.msg = "add@/devices/virtual/net/ltp-tun0",
> > +	.value_cnt = 4,
> > +	.values = (const char*[]) {
> > +		"ACTION=add",
> > +		"DEVPATH=/devices/virtual/net/ltp-tun0",
> > +		"SUBSYSTEM=net",
> > +		"INTERFACE=ltp-tun0",
> > +	}
> > +};
> > +struct uevent_desc add_rx = {
> > +	.msg = "add@/devices/virtual/net/ltp-tun0/queues/rx-0",
> > +	.value_cnt = 3,
> > +	.values = (const char*[]) {
> > +		"ACTION=add",
> > +		"DEVPATH=/devices/virtual/net/ltp-tun0/queues/rx-0",
> > +		"SUBSYSTEM=queues",
> > +	}
> > +};
> > +struct uevent_desc add_tx = {
> > +	.msg = "add@/devices/virtual/net/ltp-tun0/queues/tx-0",
> > +	.value_cnt = 3,
> > +	.values = (const char*[]) {
> > +		"ACTION=add",
> > +		"DEVPATH=/devices/virtual/net/ltp-tun0/queues/tx-0",
> > +		"SUBSYSTEM=queues",
> > +	}
> > +};
> > +struct uevent_desc rem_rx = {
> > +	.msg = "remove@/devices/virtual/net/ltp-tun0/queues/rx-0",
> > +	.value_cnt = 3,
> > +	.values = (const char*[]) {
> > +		"ACTION=remove",
> > +		"DEVPATH=/devices/virtual/net/ltp-tun0/queues/rx-0",
> > +		"SUBSYSTEM=queues",
> > +	}
> > +};
> > +struct uevent_desc rem_tx = {
> > +	.msg = "remove@/devices/virtual/net/ltp-tun0/queues/tx-0",
> > +	.value_cnt = 3,
> > +	.values = (const char*[]) {
> > +		"ACTION=remove",
> > +		"DEVPATH=/devices/virtual/net/ltp-tun0/queues/tx-0",
> > +		"SUBSYSTEM=queues",
> > +	}
> > +};
> > +struct uevent_desc rem = {
> > +	.msg = "remove@/devices/virtual/net/ltp-tun0",
> > +	.value_cnt = 4,
> > +	.values = (const char*[]) {
> > +		"ACTION=remove",
> > +		"DEVPATH=/devices/virtual/net/ltp-tun0",
> > +		"SUBSYSTEM=net",
> > +		"INTERFACE=ltp-tun0",
> > +	}
> > +};
> 
> Why do we have to move these outside of the function? I do not see a
> single reason to do so.
> 

I think separating the declaration of uevents and dynamic adding uevents 
will make the program easier to read.
This part is open to discussion, I'm just giving my thoughts.
if everyone think it's a bad idea, I'll change it back in v2 patch.

declaration
----------------------------
struct uevent_desc rem_tx = {}
struct uevent_desc add_rx = {}
struct uevent_desc add_tx = {}
.....

dynamically adding uevent:
--------------------------
uevents[i++] = &add;
if (has_RPS)
        uevents[i++] = &add_rx;
uevents[i++] = &add_tx;
if (has_RPS)
        uevents[i++] = &rem_rx;
uevents[i++] = &rem_tx;
uevents[i++] = &rem;
uevents[i++] = NULL;

> > +	const struct uevent_desc *uevents[MAX_UEVENT];
> > +	int pid, fd, i = 0;
> > +	int has_RPS = tst_kconfig_get("CONFIG_RPS");
> 
> Getting the flag should be done once in the test setup, otherwise kernel
> config will be parsed in each iteration of the test.
> 

If we parse the kernel configuration during test setup, we will block all 
images without CONFIG_RPS from executing this testcase. This is not my 
intention, in this patch I designed to continue executing uevent02 even 
without CONFIG_RPS. Just adjusting uevents can pass this test. So I think 
parsing the kernel config in test_all() should be the correct way.

Best,
Dylan

> > +	uevents[i++] = &add;
> > +	if (has_RPS)
> > +		uevents[i++] = &add_rx;
> > +	uevents[i++] = &add_tx;
> > +	if (has_RPS)
> > +		uevents[i++] = &rem_rx;
> > +	uevents[i++] = &rem_tx;
> > +	uevents[i++] = &rem;
> > +	uevents[i++] = NULL;
> 
> 
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz

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