[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