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

[Dylan Jhong <dylan@andestech.com>]

Hi Cyril,

Oh, i got it!!! Sorry for the misunderstanding.
I thought this was talking about adding the .needs_kconfigs property.
I will add setup() function in v2 patch.

And about the adjustment of uevent declaration, any comments that I can
do in v2 patch?

Best,
Dylan

On Thu, Sep 15, 2022 at 06:09:07PM +0800, Cyril Hrubis wrote:
> 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