(2014/06/19 21:34), Namhyung Kim wrote:
> Hi Masami,
>
> 2014-06-17 (화), 11:04 +0000, Masami Hiramatsu:
>> +static int __ftrace_add_filter_ip(struct ftrace_ops *ops, unsigned long ip,
>> + int *ref)
>> +{
>> + int ret;
>> +
>> + /* Try to set given ip to filter */
>> + ret = ftrace_set_filter_ip(ops, ip, 0, 0);
>> + if (ret >= 0) {
>
> Hmm.. this doesn't look comfortable. What not using usual pattern?
>
> if (ret < 0)
> return ret;
>
> This way we can reduce a indent level.
OK, I'll do so :)
>
>
>> + (*ref)++;
>> + if (*ref == 1) {
>> + ret = register_ftrace_function(ops);
>> + if (ret < 0) {
>> + /* Rollback refcounter and filter */
>> + (*ref)--;
>> + ftrace_set_filter_ip(ops, ip, 1, 0);
>> + }
>> + }
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int __ftrace_remove_filter_ip(struct ftrace_ops *ops, unsigned long
>> ip,
>> + int *ref)
>> +{
>> + int ret = 0;
>> +
>> + (*ref)--;
>> + if (*ref == 0)
>> + ret = unregister_ftrace_function(ops);
>> + if (ret >= 0)
>> + /* Try to remove given ip to filter */
>> + ret = ftrace_set_filter_ip(ops, ip, 1, 0);
>
> I think any failure at this time can be problematic. Since we already
> unregistered the ops but the refcount will get increased again, future
> attemp to register won't enable the ops anymore IMHO.
Agreed.
> I think it'd better just ignoring faiure of filter removal here. We'll
> miss a filter entry but it'll be usable anyway.
>
> What about this?
OK, I'll use your v2 code :)
Thank you for review!
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu...@hitachi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
