On 07/11/2012 05:09 PM, Denys Vlasenko wrote:
On 07/11/2012 03:38 PM, Jiri Moskovcak wrote:
> On 07/11/2012 03:26 PM, Denys Vlasenko wrote:
>> On 07/11/2012 01:15 PM, Jiri Moskovcak wrote:
>>> On 07/11/2012 11:45 AM, Denys Vlasenko wrote:
>>>> Hi,
>>>>
>>>> I propose the following attached patches:
>>>>
>>>> Patch 1 removes the wrappers. I don't see why we need them.
>>>> Also removes CD_FLAG_FILE, we have CD_FLAG_BIN which is the same.
>>>>
>>>> Patches 2 and 3 simplify save_dump_dir_from_problem_data() code
>>>> and the method it uses to pass back the name of the created directory.
>>>> Also, move its decl into problem_data.h - I feel it belongs here.
>>>>
>>>> These are pretty mechanical changes, which open up the way for
>>>> the next step:
>>>>
>>>
>>> - the API I've created is meant for 3rd party apps which doesn't
know
>> > (adn doesn't want to know!!) ABRT/libreport internals
>>
>> Why is it necessary to insulate 3rd party apps from knowing that
>> libreport package exists?
>>
>> For an analogy: GTK does not wrap GLib APIs. There is no need to hide
>> Glib
>> for GTK users.
>>
>
> - because in this use case, 3rd party apps (and our hooks) just want
> to report problem to ABRT not to study libreport,
> so why should they waste time with libreport
I do not understand this part: "waste time with libreport".
How
problem_data_t *pd = problem_data_new();
is an improvement over
problem_data_t *pd = new_problem_data();
?
Or
if (problem_data_add_item(pd, key, value) != 0)
{
if (error)
*error = xasprintf("Cannot save item '%s'", key);
result = false;
goto handle_new_problem_cleanup;
}
Is better than
add_to_problem_data_ext(pd, key, value, CD_FLAG_TXT);
- consumers don't have to handle the errors if you they don't want to,
Jakub's code need to handle it to propagate the error message back to
the caller (I know it returns 0 everytime, but it's easier to add an
error code later than changing void to int)
- + naming convention:
add_to_problem_data() vs problem_data_save()
- yes, we can 's/add_to_problem_data/problem_data_add/g' but since I
needed it only for ABRT and also needed the new function
problem_data_save() I created the wrapper, so I didn't have to deal with
the changes in libreport and could have it in one header..
?
> and trying to find out which flag to use?
> - that's why I don't want to expose *_ext functions
Let's add add_text_to_problem_data() to
libreport.1/src/include/problem_data.h
instead of creating yet another abstraction layer.
Abstraction layers are not zero price.
- I don't mind the few microseconds or few bytes it will cost I really
prefer the easy to use API over that...
>>
>>> and that's the reason why problem_data_save() should be available
>>> publicly
>>
>> I have nothing against problem_data_save() being exported.
>>
>
> - but your patch hides it..
Because I did not know you insist on it being public :)
- ok, just make my example compilable with your proposed changes and we
can than discuss which solution is simpler