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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 26 Mar 2019 15:31:51 +0200
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        Petr Mladek <pmladek@...e.com>, linux-kernel@...r.kernel.org,
        rafael@...nel.org, linux-acpi@...r.kernel.org,
        devicetree@...r.kernel.org, Rob Herring <robh@...nel.org>
Subject: Re: [PATCH v2 6/6] lib/vsprintf: Add %pfw conversion specifier for
 printing fwnode names

Hi Andy, Rasmus,

Thanks for the comments.

On Tue, Mar 26, 2019 at 03:24:50PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 26, 2019 at 02:11:35PM +0100, Rasmus Villemoes wrote:
> > On 26/03/2019 13.41, Sakari Ailus wrote:
> > > Add support for %pfw conversion specifier (with "f" and "P" modifiers) to
> > > support printing full path of the node, including its name ("f") and only
> > > the node's name ("P") in the printk family of functions. The two flags
> > > have equivalent functionality to existing %pOF with the same two modifiers
> > > ("f" and "P") on OF based systems. The ability to do the same on ACPI
> > > based systems is added by this patch.
> 
> > > +	for (pass = false; strspn(fmt, modifiers); fmt++, pass = true) {
> > > +		if (pass) {
> > > +			if (buf < end)
> > > +				*buf = ':';
> > > +			buf++;
> > > +		}
> > > +
> > > +		switch (*fmt) {
> > > +		case 'f':	/* full_name */
> > > +			buf = fwnode_gen_full_name(fwnode, buf, end);
> > > +			break;
> > > +		case 'P':	/* name */
> > > +			buf = string(buf, end, fwnode_get_name(fwnode),
> > > +				     str_spec);
> > > +			break;
> > > +		default:
> > > +			break;
> > > +		}
> > > +	}
> > 
> > This seems awfully complicated. Why would anyone ever pass more than one
> > of 'f' and 'P'? Why not just
> > 
> > switch(*fmt) {
> > case 'P':
> >    ...
> > case 'f':
> > default:
> >    ...
> > }
> > 
> > which avoids the loop and the strcspn. Or, drop the default: case and
> > don't have logic at all for falling back to 'f' if neither is present.

There are just two modifiers supported now ('f' and 'P') but should more be
needed in the future, we'd need the loop again. This is basically modelled
after %pOF implementation.

I can simplify this if you both still think that's the way to go, and
make the implementations diverge.

> > 
> > > +	return widen_string(buf, buf - buf_start, end, spec);
> > > +}
> 
> My point as well (as per sent comments against previous version).
> Sakari, can you add test cases at the same time?
> 
> > >  			return device_node_string(buf, end, ptr, spec, fmt + 1);
> 
> > > +			return fwnode_string(buf, end, ptr, spec, fmt + 1);
> > 
> > Why not pass fmt+2; we know that fmt+1 points at a 'w'. Just to avoid
> > doing the fmt++ inside fwnode_string().
> 
> I guess in order to be consistent with existing %pOF case. But wouldn't be
> better to fix %pOF for that sooner or later?

Agreed.

-- 
Sakari Ailus
sakari.ailus@...ux.intel.com

Powered by blists - more mailing lists