linux-kernel - Re: [PATCH v1 1/1] test_printf: Refactor fwnode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 29 Sep 2022 20:04:04 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Petr Mladek <pmladek@...e.com>, linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: Re: [PATCH v1 1/1] test_printf: Refactor fwnode_pointer() to make it
 more readable

On Thu, Sep 29, 2022 at 12:06:32PM -0400, Steven Rostedt wrote:
> On Wed, 24 Aug 2022 20:05:42 +0300
> Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:

Thank you for review, my answers below.

...

> > +	const struct software_node first = { .name = "first" };
> > +	const struct software_node second = { .name = "second", .parent = &first };
> > +	const struct software_node third = { .name = "third", .parent = &second };
> 
> I personally do not find the above more readable, but honestly, I'm not
> attached to this code at all.
> 
> > +	const struct software_node *group[] = { &first, &second, &third, NULL };
> 
> Could this just be:
> 
> 	const struct software_node *group[] = {
> 		&softnodes[0], &softnodes[1], &softnodes[2], NULL };

It could, but the issue is that it will loose the self-explanatory naming
scheme. It's much easier to see what we test and what we expect in the below
calls...

> >  	const char * const full_name_second = "first/second";
> > +	const char * const full_name_third = "first/second/third";
> >  	const char * const second_name = "second";
> >  	const char * const third_name = "third";
> >  	int rval;
> >  
> > -	rval = software_node_register_nodes(softnodes);
> > +	rval = software_node_register_node_group(group);
> >  	if (rval) {
> >  		pr_warn("cannot register softnodes; rval %d\n", rval);
> >  		return;
> >  	}
> >  
> > -	test(full_name_second, "%pfw", software_node_fwnode(&softnodes[1]));
> > -	test(full_name, "%pfw", software_node_fwnode(&softnodes[2]));
> > -	test(full_name, "%pfwf", software_node_fwnode(&softnodes[2]));
> > -	test(second_name, "%pfwP", software_node_fwnode(&softnodes[1]));
> > -	test(third_name, "%pfwP", software_node_fwnode(&softnodes[2]));
> > +	test(full_name_second, "%pfw", software_node_fwnode(&second));
> > +	test(full_name_third, "%pfw", software_node_fwnode(&third));
> > +	test(full_name_third, "%pfwf", software_node_fwnode(&third));
> > +	test(second_name, "%pfwP", software_node_fwnode(&second));
> > +	test(third_name, "%pfwP", software_node_fwnode(&third));

...here.

> Then the above doesn't need to change.

And that's why I want to change them.

> But again, I'm not maintaining this code, so I'm not attached. Just adding
> my $0.02 to this (as I'm triaging my inbox and found this email).

> > -	software_node_unregister_nodes(softnodes);
> > +	software_node_unregister_node_group(group);
> >  }
> >  
> >  static void __init fourcc_pointer(void)

-- 
With Best Regards,
Andy Shevchenko