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

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> > >  #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.

I do not think that we understand each other here. What I proposed is to
call the function that parses kernel config in the test setup and
initialize the has_RPS flag there so that it's not called on each
interation of the run() function (e.g. when test is passed -i 100 on
command line).

-- 
Cyril Hrubis
chrubis@suse.cz

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