[PATCH 1/3] Move get_problems_count() to its only user - "abrt-cli status"

Jiri Moskovcak jmoskovc at redhat.com
Mon May 27 14:57:28 UTC 2013


On 05/27/2013 04:30 PM, Nikola Pajkovsky wrote:
> Jiri Moskovcak <jmoskovc at redhat.com> writes:
>
>> On 05/23/2013 01:20 PM, Nikola Pajkovsky wrote:
>>> Jakub Filak <jfilak at redhat.com> writes:
>>>
>>>> On Thursday 23 of May 2013 12:32:33 Nikola Pajkovsky wrote:
>>>>> Denys Vlasenko <dvlasenk at redhat.com> writes:
>>>>>> Logic is not changed.
>>>>>>
>>>>>> Signed-off-by: Denys Vlasenko <dvlasenk at redhat.com>
>>>>>> ---
>>>>>>
>>>>>>    src/cli/status.c          | 49
>>>>>>    +++++++++++++++++++++++++++++++++++++++++++----
>>>>>>    src/include/problem_api.h |  8 --------
>>>>>>    src/lib/problem_api.c     | 38 ------------------------------------
>>>>>>    3 files changed, 45 insertions(+), 50 deletions(-)
>>>>>>
>>>>>> diff --git a/src/cli/status.c b/src/cli/status.c
>>>>>> index 6f228c6..0dbcd1d 100644
>>>>>> --- a/src/cli/status.c
>>>>>> +++ b/src/cli/status.c
>>>>>> @@ -21,13 +21,52 @@
>>>>>>
>>>>>>    #include <sys/types.h>
>>>>>>    #include "problem_api.h"
>>>>>>
>>>>>> +struct time_range {
>>>>>> +    unsigned count;
>>>>>> +    unsigned long since;
>>>>>> +};
>>>>>> +
>>>>>> +static int count_dir_if_newer_than(struct dump_dir *dd, void *arg)
>>>>>> +{
>>>>>> +    struct time_range *me = arg;
>>>>>> +
>>>>>> +    char *time_str = dd_load_text(dd, FILENAME_LAST_OCCURRENCE);
>>>>>> +    long val = atol(time_str);
>>>>>> +    free(time_str);
>>>>>> +    if (val < me->since)
>>>>>> +        return 0;
>>>>>> +
>>>>>> +    me->count++;
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static void count_problems_in_dir(gpointer data, gpointer arg)
>>>>>> +{
>>>>>> +    char *path = data;
>>>>>> +    struct time_range *me = arg;
>>>>>> +
>>>>>> +    VERB2 log("scanning '%s' for problems since %lu", path, me->since);
>>>>>> +
>>>>>> +    for_each_problem_in_dir(path, getuid(), count_dir_if_newer_than, me);
>>>>>
>>>>> I still think, that there is a way, how to write it without
>>>>> callback. like:
>>>>>
>>>>>        dir_for_each(path, iterator, and whatever else you is meaningful) {
>>>>>                count_dir();
>>>>>        }
>>>>>
>>>>> because callbacks are moronic in sense that you you have to pass
>>>>> pointers to functions and that functions the most of time do simple
>>>>> thing. And I don't see any reasons to use callbacks here.
>>>>
>>>> Hmm, interesting, but I cannot imagine what is the magic behind
>>>> dir_for_each(). Is it a macro? Do you have any prototype?
>>>
>>> yep, I think of macro, which will expand to for() or while() and hope,
>>> that macro won't look much horrible. I don't have any prototype
>>> now. It's just a theory. hmm, while I'm writing this mail, I started to
>>> thing of dir_for_each_dentry(). I'll give a shot.
>>>
>>
>> - so you're saying that's better to have a macro which generates some
>> code than callbacks? I'd like to remind you, that we're not coding
>> kernel, so we can afford "expensive" pointers which are much easier to
>> debug than some code generating macro..
>
> point is, that callback is problematic, when you want to check return
> errors. What would happen, when callback in caller function fails and
> you still need to know, which error that is? Especially, when error
> codes don't much on each other. Say, callback bar() with enum ERR_BAR {},
> and foo() with enum ERR_FOO{} witch calls bar(). So to get the result of
> bar() you have to pass yet another argument.
>

You are missing the idea behind the code, it uses callbacks so various 
conditions can be easily added without modifying the logic which 
evaluates the condition chain. Let's say I want to get list of problems 
which are (not older than 1 day && have counter == 3 && are not reported 
yet && backtrace contains "printf"). And I think we both agree that 
extending the one function which was handling --since and --until to 
cover all of the possible combinations is not a good idea.

And still my main problem wasn't with using callbacks or not, my problem 
with these changes was that it's changing some code just because someone 
doesn't like callbacks. Which is wrong and pointless unless you need to 
change it anyway to implement something on top of it. Changing the 
working code just to make it nicer is bad idea and waste of time 
especially when there is a lot of other problems to work on.

> The worst case I see now is
>
>      int bar() { ...; return err; ... }
>
>      int foo(..., &bar, result_callback)
>      {
>           int err_callback = bar();
>           if (err_callback && result_callback)
>                set result_callback
>      }
>
>      int = foo(&bar);
>
> and I think it's wroth to think to make it simple like
>
>     for_each_dentry(...) {
>         int err = bar();
>         if (err)
>             handle error
>     }
>
>
> and as I said, it's an idea.
>



More information about the Crash-catcher mailing list