From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932097AbbKBOlH (ORCPT ); Mon, 2 Nov 2015 09:41:07 -0500 Received: from mail-pa0-f41.google.com ([209.85.220.41]:33744 "EHLO mail-pa0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753670AbbKBOlD (ORCPT ); Mon, 2 Nov 2015 09:41:03 -0500 Date: Mon, 2 Nov 2015 23:40:52 +0900 From: Namhyung Kim To: Tom Zanussi Cc: rostedt@goodmis.org, daniel.wagner@bmw-carit.de, masami.hiramatsu.pt@hitachi.com, josh@joshtriplett.org, andi@firstfloor.org, mathieu.desnoyers@efficios.com, peterz@infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v11 21/28] tracing: Remove restriction on string position in hist trigger keys Message-ID: <20151102144052.GB31643@danjae.kornet> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Oct 22, 2015 at 01:14:25PM -0500, Tom Zanussi wrote: > If we assume the maximum size for a string field, we don't have to > worry about its position. Since we only allow two keys in a compound > key and having more than one string key in a given compound key > doesn't make much sense anyway, trading a bit of extra space instead > of introducing an arbitrary restriction makes more sense. > > We also need to use the event field size for static strings when > copying the contents, otherwise we get random garbage in the key. > > Also, cast string return values to avoid warnings on 32-bit compiles. > > Finally, rearrange the code without changing any functionality by > moving the compound key updating code into a separate function. > > Signed-off-by: Tom Zanussi > Tested-by: Masami Hiramatsu > --- [SNIP] > @@ -508,8 +508,8 @@ static int create_key_field(struct hist_trigger_data *hist_data, > goto out; > } > > - if (is_string_field(field)) /* should be last key field */ > - key_size = HIST_KEY_SIZE_MAX - key_offset; > + if (is_string_field(field)) > + key_size = MAX_FILTER_STR_VAL; > else > key_size = field->size; > } > @@ -781,9 +781,36 @@ static void hist_trigger_elt_update(struct hist_trigger_data *hist_data, > } > } > > +static inline void add_to_key(char *compound_key, void *key, > + struct hist_field *key_field, void *rec) > +{ > + size_t size = key_field->size; > + > + if (key_field->flags & HIST_FIELD_STRING) { > + struct ftrace_event_field *field; > + > + /* ensure NULL-termination */ > + size--; No need to do it here since the size will be updated below. > + > + field = key_field->field; > + if (field->filter_type == FILTER_DYN_STRING) > + size = *(u32 *)(rec + field->offset) >> 16; > + else if (field->filter_type == FILTER_PTR_STRING) > + size = strlen(key); > + else if (field->filter_type == FILTER_STATIC_STRING) > + size = field->size; > + > + if (size > key_field->size - 1) > + size = key_field->size - 1; The comment above can be moved here though. Thanks, Namhyung > + } > + > + memcpy(compound_key + key_field->offset, key, size); > +} > +