linux-kernel - Re: [PATCH] vsprintf: Drop unused assignment of fmt.state

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Z6yUNc9h7T1gxrxp@pathway.suse.cz>
Date: Wed, 12 Feb 2025 13:29:41 +0100
From: Petr Mladek <pmladek@...e.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: I Hsin Cheng <richard120310@...il.com>, rostedt@...dmis.org,
	linux@...musvillemoes.dk, senozhatsky@...omium.org,
	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	jserv@...s.ncku.edu.tw, shuah@...nel.org
Subject: Re: [PATCH] vsprintf: Drop unused assignment of fmt.state

On Mon 2025-02-10 17:08:25, Andy Shevchenko wrote:
> On Mon, Feb 10, 2025 at 02:09:53PM +0100, Petr Mladek wrote:
> > On Thu 2025-02-06 17:32:32, Andy Shevchenko wrote:
> > > On Thu, Feb 06, 2025 at 01:25:07AM +0800, I Hsin Cheng wrote:
> > > > Remove unused assignment of "fmt.state", in both cases the value of
> > > > "fmt.state" will be overwritten by either "FORMAT_STATE_PRECISION" or
> > > > "FORMAT_STATE_NUM", the value "FORMAT_STATE_NONE" isn't going to be used
> > > > after the assignment.
> 
> ...
> 
> > > > struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
> > > 
> > > >  			spec->field_width = -spec->field_width;
> > > >  			spec->flags |= LEFT;
> > > >  		}
> > > > -		fmt.state = FORMAT_STATE_NONE;
> > > > +
> > > >  		goto precision;
> > > >  	}
> > > >  
> > > 
> > > While both are kinda redundant, this is not obvious what's stated in the commit
> > > message. Yes, `goto qualifier;` is straightforward, but not `goto precision;`.
> > > Which makes me think that these assignments can make code robust against
> > > potential future changes to allow to catch up the wrong code paths.
> > 
> > I fully agree with Andy here.
> > 
> > That said, I see the following right below the two conditions modified
> > in this patch:
> > 
> > 	/* By default */
> > 	fmt.state = FORMAT_STATE_NONE;
> > 
> > A good solution would be to move it up. It will be then obvious
> > that we could remove these two initializations. I mean
> > to do the following:
> 
> Which can't be performed (one need to check the old value first somehow) :-)

Grr, humph /o\

OK, I would personally keep the code as it is now. I do not see any
big benefit in removing the duplicity assignment.

IMHO, the assignment make the code more robust for future changes.
Let the compiler to optimize it out.

Best Regards,
Petr