traceprobe_parse_event_name() already validate group and event name,
there is no need to call is_good_name() after it.
Signed-off-by: Linyu Yuan <[email protected]>
---
v2: drop v1 change as it is NACK.
add it to remove duplicate is_good_name().
v3: move it as first patch.
v4: no change
kernel/trace/trace_eprobe.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 7d44785..17d64e3 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -878,16 +878,12 @@ static int __trace_eprobe_create(int argc, const char *argv[])
sanitize_event_name(buf1);
event = buf1;
}
- if (!is_good_name(event) || !is_good_name(group))
- goto parse_error;
sys_event = argv[1];
ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2,
sys_event - argv[1]);
if (ret || !sys_name)
goto parse_error;
- if (!is_good_name(sys_event) || !is_good_name(sys_name))
- goto parse_error;
mutex_lock(&event_mutex);
event_call = find_and_get_event(sys_name, sys_event);
--
2.7.4
Hi Linhu,
On Thu, 2022-06-02 at 20:10 +0800, Linyu Yuan wrote:
> traceprobe_parse_event_name() already validate group and event name,
> there is no need to call is_good_name() after it.
>
> Signed-off-by: Linyu Yuan <[email protected]>
> ---
> v2: drop v1 change as it is NACK.
> add it to remove duplicate is_good_name().
> v3: move it as first patch.
> v4: no change
>
> kernel/trace/trace_eprobe.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/kernel/trace/trace_eprobe.c
> b/kernel/trace/trace_eprobe.c
> index 7d44785..17d64e3 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -878,16 +878,12 @@ static int __trace_eprobe_create(int argc,
> const char *argv[])
> sanitize_event_name(buf1);
> event = buf1;
> }
> - if (!is_good_name(event) || !is_good_name(group))
> - goto parse_error;
traceprobe_parse_event_name() is only called if (event). In the
!event case, wouldn't the is_good_name() checks still be needed (since
in that case buf1 is assigned to event)?
>
> sys_event = argv[1];
> ret = traceprobe_parse_event_name(&sys_event, &sys_name,
> buf2,
> sys_event - argv[1]);
> if (ret || !sys_name)
> goto parse_error;
> - if (!is_good_name(sys_event) || !is_good_name(sys_name))
> - goto parse_error;
I agree this one isn't needed.
Thanks,
Tom
>
> mutex_lock(&event_mutex);
> event_call = find_and_get_event(sys_name, sys_event);
hi Tom,
On 6/14/2022 5:01 AM, Tom Zanussi wrote:
> Hi Linhu,
>
> On Thu, 2022-06-02 at 20:10 +0800, Linyu Yuan wrote:
>> traceprobe_parse_event_name() already validate group and event name,
>> there is no need to call is_good_name() after it.
>>
>> Signed-off-by: Linyu Yuan <[email protected]>
>> ---
>> v2: drop v1 change as it is NACK.
>> add it to remove duplicate is_good_name().
>> v3: move it as first patch.
>> v4: no change
>>
>> kernel/trace/trace_eprobe.c | 4 ----
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/kernel/trace/trace_eprobe.c
>> b/kernel/trace/trace_eprobe.c
>> index 7d44785..17d64e3 100644
>> --- a/kernel/trace/trace_eprobe.c
>> +++ b/kernel/trace/trace_eprobe.c
>> @@ -878,16 +878,12 @@ static int __trace_eprobe_create(int argc,
>> const char *argv[])
>> sanitize_event_name(buf1);
>> event = buf1;
>> }
>> - if (!is_good_name(event) || !is_good_name(group))
>> - goto parse_error;
> traceprobe_parse_event_name() is only called if (event). In the
> !event case, wouldn't the is_good_name() checks still be needed (since
> in that case buf1 is assigned to event)?
when user input no event name, it will generate event name from second
SYSTEM.EVENT,
and it will validate with following traceprobe_parse_event_name().
(
if you agree, i will send a new version to update a minor issue in
second patch,
sys_event = argv[1];
- ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2,
- sys_event - argv[1]);
- if (ret || !sys_name)
+ ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
+ if (!sys_event || !sys_name)
goto parse_error;
)
>
>>
>> sys_event = argv[1];
>> ret = traceprobe_parse_event_name(&sys_event, &sys_name,
>> buf2,
>> sys_event - argv[1]);
>> if (ret || !sys_name)
>> goto parse_error;
>> - if (!is_good_name(sys_event) || !is_good_name(sys_name))
>> - goto parse_error;
> I agree this one isn't needed.
>
> Thanks,
>
> Tom
>
>>
>> mutex_lock(&event_mutex);
>> event_call = find_and_get_event(sys_name, sys_event);
Hi Linyu,
On Tue, 2022-06-14 at 08:48 +0800, Linyu Yuan wrote:
> hi Tom,
>
> On 6/14/2022 5:01 AM, Tom Zanussi wrote:
> > Hi Linhu,
> >
> > On Thu, 2022-06-02 at 20:10 +0800, Linyu Yuan wrote:
> > > traceprobe_parse_event_name() already validate group and event
> > > name,
> > > there is no need to call is_good_name() after it.
> > >
> > > Signed-off-by: Linyu Yuan <[email protected]>
> > > ---
> > > v2: drop v1 change as it is NACK.
> > > add it to remove duplicate is_good_name().
> > > v3: move it as first patch.
> > > v4: no change
> > >
> > > kernel/trace/trace_eprobe.c | 4 ----
> > > 1 file changed, 4 deletions(-)
> > >
> > > diff --git a/kernel/trace/trace_eprobe.c
> > > b/kernel/trace/trace_eprobe.c
> > > index 7d44785..17d64e3 100644
> > > --- a/kernel/trace/trace_eprobe.c
> > > +++ b/kernel/trace/trace_eprobe.c
> > > @@ -878,16 +878,12 @@ static int __trace_eprobe_create(int argc,
> > > const char *argv[])
> > > sanitize_event_name(buf1);
> > > event = buf1;
> > > }
> > > - if (!is_good_name(event) || !is_good_name(group))
> > > - goto parse_error;
> > traceprobe_parse_event_name() is only called if (event). In the
> > !event case, wouldn't the is_good_name() checks still be needed
> > (since
> > in that case buf1 is assigned to event)?
>
> when user input no event name, it will generate event name from
> second
> SYSTEM.EVENT,
>
> and it will validate with following traceprobe_parse_event_name().
>
>
Right, but that happens in your second patch '[PATCH v5 2/3] tracing:
auto generate event name when create a group of events', not this one.
So if you apply only this patch, the !event case will assign event but
it will remain unchecked when used later in this function.
It would make more sense to remove this check in patch 2/3 along with
the code that does the generating...
> (
>
> if you agree, i will send a new version to update a minor issue in
> second patch,
>
>
> sys_event = argv[1];
> - ret = traceprobe_parse_event_name(&sys_event, &sys_name,
> buf2,
> - sys_event - argv[1]);
> - if (ret || !sys_name)
> + ret = traceprobe_parse_event_name(&sys_event, &sys_name,
> buf2, 0);
> + if (!sys_event || !sys_name)
> goto parse_error;
>
> )
>
> >
> > >
> > > sys_event = argv[1];
> > > ret = traceprobe_parse_event_name(&sys_event, &sys_name,
> > > buf2,
> > > sys_event - argv[1]);
> > > if (ret || !sys_name)
> > > goto parse_error;
> > > - if (!is_good_name(sys_event) || !is_good_name(sys_name))
> > > - goto parse_error;
> > I agree this one isn't needed.
But keep this one in this patch, since it's useful on its own as a
standalone cleanup regardless of whether or not patch 2/3 gets merged.
Tom
> > Thanks,
> >
> > Tom
> >
> > >
> > > mutex_lock(&event_mutex);
> > > event_call = find_and_get_event(sys_name, sys_event);
hi Tom,
On 6/21/2022 2:38 AM, Tom Zanussi wrote:
> Hi Linyu,
>
> On Tue, 2022-06-14 at 08:48 +0800, Linyu Yuan wrote:
>> hi Tom,
>>
>> On 6/14/2022 5:01 AM, Tom Zanussi wrote:
>>> Hi Linhu,
>>>
>>> On Thu, 2022-06-02 at 20:10 +0800, Linyu Yuan wrote:
>>>> traceprobe_parse_event_name() already validate group and event
>>>> name,
>>>> there is no need to call is_good_name() after it.
>>>>
>>>> Signed-off-by: Linyu Yuan <[email protected]>
>>>> ---
>>>> v2: drop v1 change as it is NACK.
>>>> add it to remove duplicate is_good_name().
>>>> v3: move it as first patch.
>>>> v4: no change
>>>>
>>>> kernel/trace/trace_eprobe.c | 4 ----
>>>> 1 file changed, 4 deletions(-)
>>>>
>>>> diff --git a/kernel/trace/trace_eprobe.c
>>>> b/kernel/trace/trace_eprobe.c
>>>> index 7d44785..17d64e3 100644
>>>> --- a/kernel/trace/trace_eprobe.c
>>>> +++ b/kernel/trace/trace_eprobe.c
>>>> @@ -878,16 +878,12 @@ static int __trace_eprobe_create(int argc,
>>>> const char *argv[])
>>>> sanitize_event_name(buf1);
>>>> event = buf1;
>>>> }
>>>> - if (!is_good_name(event) || !is_good_name(group))
>>>> - goto parse_error;
>>> traceprobe_parse_event_name() is only called if (event). In the
>>> !event case, wouldn't the is_good_name() checks still be needed
>>> (since
>>> in that case buf1 is assigned to event)?
>> when user input no event name, it will generate event name from
>> second
>> SYSTEM.EVENT,
>>
>> and it will validate with following traceprobe_parse_event_name().
>>
>>
> Right, but that happens in your second patch '[PATCH v5 2/3] tracing:
> auto generate event name when create a group of events', not this one.
>
> So if you apply only this patch, the !event case will assign event but
> it will remain unchecked when used later in this function.
>
> It would make more sense to remove this check in patch 2/3 along with
> the code that does the generating...
thanks, will do like this.
>
>> (
>>
>> if you agree, i will send a new version to update a minor issue in
>> second patch,
>>
>>
>> sys_event = argv[1];
>> - ret = traceprobe_parse_event_name(&sys_event, &sys_name,
>> buf2,
>> - sys_event - argv[1]);
>> - if (ret || !sys_name)
>> + ret = traceprobe_parse_event_name(&sys_event, &sys_name,
>> buf2, 0);
>> + if (!sys_event || !sys_name)
>> goto parse_error;
>>
>> )
>>
>>>>
>>>> sys_event = argv[1];
>>>> ret = traceprobe_parse_event_name(&sys_event, &sys_name,
>>>> buf2,
>>>> sys_event - argv[1]);
>>>> if (ret || !sys_name)
>>>> goto parse_error;
>>>> - if (!is_good_name(sys_event) || !is_good_name(sys_name))
>>>> - goto parse_error;
>>> I agree this one isn't needed.
> But keep this one in this patch, since it's useful on its own as a
> standalone cleanup regardless of whether or not patch 2/3 gets merged.
>
> Tom
>
>>> Thanks,
>>>
>>> Tom
>>>
>>>>
>>>> mutex_lock(&event_mutex);
>>>> event_call = find_and_get_event(sys_name, sys_event);