From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752429AbbDBN1P (ORCPT <rfc822;w@1wt.eu>);
	Thu, 2 Apr 2015 09:27:15 -0400
Received: from smtprelay0099.hostedemail.com ([216.40.44.99]:57744 "EHLO
	smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL)
	by vger.kernel.org with ESMTP id S1751566AbbDBN1M (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 2 Apr 2015 09:27:12 -0400
X-Session-Marker: 726F737465647440676F6F646D69732E6F7267
X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,rostedt@goodmis.org,:::::::::::::::::,RULES_HIT:41:355:379:541:599:800:960:968:973:988:989:1260:1277:1311:1313:1314:1345:1359:1437:1515:1516:1518:1534:1542:1593:1594:1605:1711:1730:1747:1777:1792:2194:2198:2199:2200:2393:2553:2559:2562:2693:2895:2898:3138:3139:3140:3141:3142:3622:3865:3867:3868:3870:3871:3872:3873:3874:4362:4419:5007:6119:6261:7875:7903:8660:9010:9040:10004:10400:10848:10967:11026:11232:11473:11658:11914:12043:12296:12438:12517:12519:12740:13148:13161:13229:13230:13255:14096:14097:21080,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0
X-HE-Tag: star42_2161f284a463e
X-Filterd-Recvd-Size: 3724
Date: Thu, 2 Apr 2015 09:27:08 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: Namhyung Kim <namhyung@kernel.org>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Guilherme Cox <cox@computer.org>, Tony Luck <tony.luck@gmail.com>,
        Xie XiuQi <xiexiuqi@huawei.com>
Subject: Re: [RFC][PATCH 06/17 v2] tracing: Add TRACE_DEFINE_ENUM() macro to
 map enums to their values
Message-ID: <20150402092708.0616199d@gandalf.local.home>
In-Reply-To: <20150402074740.GA23913@sejong>
References: <20150402015648.249824760@goodmis.org>
	<20150402020940.953163951@goodmis.org>
	<20150402074740.GA23913@sejong>
X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.25; x86_64-pc-linux-gnu)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, 2 Apr 2015 16:47:40 +0900
Namhyung Kim <namhyung@kernel.org> wrote:


> > +static void update_event_printk(struct ftrace_event_call *call,
> > +				struct trace_enum_map *map)
> > +{
> > +	char *ptr;
> > +	int quote = 0;
> > +	int len = strlen(map->enum_string);
> > +
> > +	for (ptr = call->print_fmt; *ptr; ptr++) {
> > +		if (*ptr == '\\') {
> > +			ptr++;
> > +			/* paranoid */
> > +			if (!*ptr)
> > +				break;
> > +			continue;
> > +		}
> > +		if (*ptr == '"') {
> > +			quote ^= 1;
> > +			continue;
> > +		}
> > +		if (quote)
> > +			continue;
> > +		if (isdigit(*ptr)) {
> > +			/* skip numbers */
> > +			do {
> > +				ptr++;
> > +			} while (isalnum(*ptr) || *ptr == '_');
> > +			/* we went one too many */
> 
> Hmm.. it looks like to skip variable name after a number.  Shouldn't it be 
> 
> 			do { ptr++; } while (isdigit(*ptr));
> 
> ?

A variable can not be appended to a number in C. But alpha characters
like 1ULL can be, and they need to be skipped. Hmm, I probably could
remove the check for '_' though.

> 
> 
> > +			ptr--;
> > +			continue;
> > +		}
> > +		if (isalpha(*ptr) || *ptr == '_') {
> > +			if (strncmp(map->enum_string, ptr, len) == 0 &&
> > +			    !isalnum(ptr[len]) && ptr[len] != '_') {
> > +				ptr = enum_replace(ptr, map, len);
> > +				/* Hmm, enum string smaller than value */
> > +				if (WARN_ON_ONCE(!ptr))
> > +					return;
> > +				continue;
> 
> I guess it also needs to decrease the ptr here.

I thought about this when writing it and realized that it just replaced
a variable and two variables can not be bumped against each other.
There must be something in between them, and we can skip whatever that
something is.

I could add a comment about that.

> 
> 
> > +			}
> > +			do {
> > +				ptr++;
> > +			} while (isalnum(*ptr) || *ptr == '_');
> > +			/* we went one too many */
> > +			ptr--;
> > +			continue;
> > +		}
> > +	}
> > +}
> > +
> > +void trace_event_enum_update(struct trace_enum_map *map, int len)
> > +{
> > +	struct ftrace_event_call *call, *p;
> > +	const char *last_system = NULL;
> > +	int last_i;
> > +	int i;
> > +
> > +	down_write(&trace_event_sem);
> > +	list_for_each_entry_safe(call, p, &ftrace_events, list) {
> > +		/* events are usually grouped together with systems */
> > +		if (!last_system || call->class->system != last_system)
> > +			last_i = 0;
> 
> I think it needs to update the last_system here.

Ouch, yeah you're right!

Will fix.

Thanks,

-- Steve