linux-kernel - Re: [PATCH][RFC] vsprintf: unify the format decoding layer for its 3 users

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090228091305.GA20533@elte.hu>
Date:	Sat, 28 Feb 2009 10:13:05 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>,
	Lai Jiangshan <laijs@...fujitsu.com>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH][RFC] vsprintf: unify the format decoding layer for its
	3 users


* Frederic Weisbecker <fweisbec@...il.com> wrote:

> > instead? Wouldn't that be nicer? I suspect it would make the 
> > code look nicer too (instead of doing "*base = x", you'd see 
> > "spec->base = x" and it would look less like line noise in 
> > the callee, an the caller could just do a single "struct 
> > format_spec spec = { 0, }" to initialize that thing).
> > 
> > 		Linus
> 
> You're right, that's much proper.
> See the V2 below:

Just a few (very) small code style pet peeves:

> +struct printf_spec {
> +	enum format_type type;
> +	int flags;			/* flags to number() */
> +	int field_width;		/* width of output field */
> +	int base;
> +	/* min. # of digits for integers; max number of chars for from string */
> +	int precision;
> +	int qualifier;
> +};

doesnt it look a bit tidier this way:

 struct printf_spec {
	enum format_type	type;
	int			flags;		/* flags to number()     */
	int			field_width;	/* width of output field */
	int			base;
	int			precision;	/* # of digits/chars     */
	int			qualifier;
 };

?

> +		case '+':
> +			spec->flags |= PLUS;
> +			break;
> +		case ' ':
> +			spec->flags |= SPACE;
> +			break;
> +		case '#':
> +			spec->flags |= SPECIAL;
> +			break;
> +		case '0':
> +			spec->flags |= ZEROPAD;
> +			break;
> +		default:
> +			found = false;

btw., this is one of the cases where i think the original style 
was more useful:

> +		case '+':	spec->flags |= PLUS;  break;
> +		case ' ':	spec->flags |= SPACE; break;
[etc.]

as it's always good to compress repetitive patterns of code.

(If checkpatch complains about this then ignore checkpatch.)

> +	case 'n':
> +		/* FIXME:
> +		* What does C99 say about the overflow case here? */

(this comment looks a bit funny.)

> +		default: {
> +			enum format_type type = spec.type;
> +
> +			if (type == FORMAT_TYPE_LONG_LONG)
> +				num = get_arg(long long);
> +			else if (type == FORMAT_TYPE_ULONG)
> +				num = get_arg(unsigned long);
> +			else if (type == FORMAT_TYPE_LONG)
> +				num = get_arg(unsigned long);
> +			else if (type == FORMAT_TYPE_SIZE_T)
> +				num = get_arg(size_t);
> +			else if (type == FORMAT_TYPE_PTRDIFF)
> +				num = get_arg(ptrdiff_t);
> +			else if (type == FORMAT_TYPE_USHORT)
> +				num = get_arg(unsigned short);
> +			else if (type == FORMAT_TYPE_SHORT)
> +				num = get_arg(short);
> +			else if (type == FORMAT_TYPE_UINT)
> +				num = get_arg(unsigned int);
> +			else
> +				num = get_arg(int);

Wouldnt it be cleaner as a switch() statement and to put into a 
helper function?

Also, could you please resend the current stuff with a 0/ 
description and a diffstat in the 0 mail so that we can all see 
all the patches again and the total impact?

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/