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);
?
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.
>
>> 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 :)
--
vda