Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754309Ab3CUEZG (ORCPT ); Thu, 21 Mar 2013 00:25:06 -0400 Received: from mail-ie0-f180.google.com ([209.85.223.180]:42510 "EHLO mail-ie0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752496Ab3CUEZE (ORCPT ); Thu, 21 Mar 2013 00:25:04 -0400 MIME-Version: 1.0 In-Reply-To: <20130321033344.GA4973@Krystal> References: <1363749497-12176-1-git-send-email-kpark3469@gmail.com> <1363800712.6345.17.camel@gandalf.local.home> <20130320180113.GA24537@Krystal> <1363820491.6345.21.camel@gandalf.local.home> <20130321024502.GB3874@Krystal> <20130321033344.GA4973@Krystal> Date: Thu, 21 Mar 2013 13:25:03 +0900 Message-ID: Subject: Re: [PATCH] tracepoints: prevents null probe from being added From: Keun-O Park To: Mathieu Desnoyers Cc: Steven Rostedt , keun-o.park@windriver.com, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4679 Lines: 131 On Thu, Mar 21, 2013 at 12:33 PM, Mathieu Desnoyers wrote: > * Keun-O Park (kpark3469@gmail.com) wrote: >> On Thu, Mar 21, 2013 at 11:45 AM, Mathieu Desnoyers >> wrote: >> > * Keun-O Park (kpark3469@gmail.com) wrote: >> >> On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt wrote: >> >> > On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote: >> >> >> * Steven Rostedt (rostedt@goodmis.org) wrote: >> >> >> > On Wed, 2013-03-20 at 12:18 +0900, kpark3469@gmail.com wrote: >> >> >> > > From: Sahara >> >> >> > > >> >> >> > > Somehow tracepoint_entry_add/remove_probe functions allow a null probe >> >> >> > > function. >> >> >> > >> >> >> > You actually hit this in practice, or is this just something that you >> >> >> > observe from code review? >> >> >> > >> >> >> > > Especially on getting a null probe in remove function, it seems >> >> >> > > to be used to remove all probe functions in the entry. >> >> >> > >> >> >> > Hmm, that actually sounds like a feature. >> >> >> >> >> >> Yep. It's been a long time since I wrote this code, but the removal code >> >> >> seems to use NULL probe pointer to remove all probes for a given >> >> >> tracepoint. >> >> >> >> >> >> I'd be tempted to just validate non-NULL probe within >> >> >> tracepoint_entry_add_probe() and let other sites as is, just in case >> >> >> anyone would be using this feature. >> >> >> >> >> >> I cannot say that I have personally used this "remove all" feature much >> >> >> though. >> >> >> >> >> > >> >> > I agree. I don't see anything wrong in leaving the null probe feature in >> >> > the removal code. But updating the add code looks like a proper change. >> >> > >> >> > -- Steve >> >> > >> >> > >> >> >> >> Hello Steve & Mathieu, >> >> If we want to leave the null probe feature enabled, I think it would >> >> be better modifying the code like the following for code efficiency. >> >> >> >> @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry, >> >> int nr_probes = 0; >> >> struct tracepoint_func *old, *new; >> >> >> >> - WARN_ON(!probe); >> >> + if (WARN_ON(!probe)) >> >> + return ERR_PTR(-EINVAL); >> >> >> >> debug_print_probes(entry); >> >> old = entry->funcs; >> >> @@ -152,14 +153,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *ent >> >> >> >> debug_print_probes(entry); >> >> /* (N -> M), (N > 1, M >= 0) probes */ >> >> - for (nr_probes = 0; old[nr_probes].func; nr_probes++) { >> >> - if (!probe || >> >> - (old[nr_probes].func == probe && >> >> - old[nr_probes].data == data)) >> >> - nr_del++; >> >> + if (probe) { >> >> + for (nr_probes = 0; old[nr_probes].func; nr_probes++) { >> >> + if (old[nr_probes].func == probe && >> >> + old[nr_probes].data == data) >> >> + nr_del++; >> >> + } >> >> } >> >> >> >> - if (nr_probes - nr_del == 0) { >> >> + if (!probe || nr_probes - nr_del == 0) { >> > >> > We might want to do: >> > >> > if (probe) { >> > ... >> > } else { >> > nr_del = nr_probes; >> > } >> > >> > if (nr_probes - nr_del == 0) { >> > ... >> > } >> >> This code has a problem. >> nr_probes is initialized as zero. > > yes, > >> And, in order to get correct count of probes, >> we need to go through the for-loop even though probe is null. >> So with above code, nr_del will be zero. Anyhow, the code will fall >> through if-clause(nr_probes-nr_del==0). >> It looks odd to me. > > Ah, I see what you mean: the nr_del = nr_probes assignment is useless, > because both nr_probes and nr_del are equal to 0. So we could go for: > > if (probe) { > for (nr_probes = 0; old[nr_probes].func; nr_probes++) { > if (old[nr_probes].func == probe && > old[nr_probes].data == data) > nr_del++; > } > } > > if (nr_probes - nr_del == 0) { > ... > } else { > ... > } > > Does it look better ? > > Thanks, > > Mathieu Yes, it does, only if you don't think this code is hard to follow. ;-) Thanks. -- Kpark -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/