The patch does not include the new abrt_packages.cpp file and it does
not apply to latest git (I fixed that for me, but cannot proceed without
abrt_packages.cpp).
Please re-send the patch.
On 06/29/2010 03:45 PM, Jiri Moskovcak wrote:
Here is a new version of the patch, should address all errors in the
latest review, so please re-review. Btw, the code for getting the action
for the crash is also used on 5 different places and should be merged
into a function - I'll do that asap, but not as part of this patch.
Thanks,
Jirka
On 06/28/2010 03:10 PM, Karel Klic wrote:
> Please see the comments below, there are some small glitches to be fixed
> before committing this.
>
>
> --- a/src/CLI/report.cpp
> +++ b/src/CLI/report.cpp
> @@ -481,12 +481,13 @@ static vector_string_t
> - std::string str_package_nvr(package_nvr);
> - std::string package_name = str_package_nvr.substr(0,
> str_package_nvr.rfind("-
> + char * package_name = get_package_name_from_NVR_or_NULL(package_nvr);
>
> package_name should be free()d
>
- fixed
> + //what if package name is NULL?
> + char* package_specific_analyzer = xasprintf("%s:%s", analyzer,
> package_name);
>
> the package_name NULL value should be handled to prevent abrt-cli crash
> on invalid input
>
- fixed
>
> --- a/src/Daemon/MiddleWare.cpp
> +++ b/src/Daemon/MiddleWare.cpp
> @@ -662,13 +664,14 @@ static mw_result_t
> SavePackageDescriptionToDebugDump(
>
> + packageName = strdup("kernel");
>
> xstrdup() does not return NULL, so you do not have to check it.
>
>
- fixed
> --- a/src/Daemon/RPM.cpp
> +++ b/src/Daemon/RPM.cpp
> @@ -167,8 +167,7 @@ std::string GetComponent(const char* pFileName)
>
> - std::string srcrpm(srpm);
> - ret = srcrpm.erase(srcrpm.rfind('-', srcrpm.rfind('-')-1));
> + ret = get_package_name_from_NVR_or_NULL(srpm);
>
> A memory leak here. The value returned by
> get_package_name_from_NVR_or_NULL should be free()d.
>
>
- fixed
> It seems that there is one more place where
> get_package_name_from_NVR_or_NULL should be used.
> It is in function Report() in MiddleWare.cpp.
>
- fixed
>
> AFAICT the code would be simpler and less error-prone if
> get_package_name_from_NVR_or_NULL would lose that _or_NULL, and it would
> just call error_msg_aAnd_die when packageNVR is NULL, as you do not
> check the returned value for NULL anyway.
>
>
> On 06/25/2010 09:01 PM, Jiri Moskovcak wrote:
>> On 06/25/2010 04:18 PM, Karel Klic wrote:
>>> Here is my review. I tested the patch and generally it works well.
>>> Please see the improvement suggestions below, IMHO those should be
>>> addressed before moving to other things.
>>>
>>> + /* package name has NVR format: foo-1.2.3-1.el6
>>> + so we need to cut the name from the second dash from the end
>>> + */
>>> + char* package_name = NULL;
>>> + if(package != NULL)
>>> + {
>>> + VERB1 log("package %s", package);
>>> + package_name = strdup(package);
>>> + char *pos = strrchr(package_name, '-');
>>> + if(pos != NULL)
>>> + {
>>> + *pos = 0;
>>> + pos = strrchr(package_name, '-');
>>> + if(pos != NULL)
>>> + {
>>> + *pos = 0;
>>> + }
>>> + }
>>> + }
>>>
>>> - Please use xstrdup(), plain strdup() can return NULL.
>>>
>>> - This code should really be a separate function in lib/Utils/...
>>> (maybe
>>> stringops.cpp?).
>>>
>>> Something like:
>>> /**
>>> * Package NVR (name, version, release) format: foo-1.2.3-1.el6
>>> * We cut the name from the second dash from the end.
>>> * Caller must free() the returned value.
>>> */
>>> char *package_name_from_nvr(char *package_nvr);
>>>
>>
>> - done
>>
>>> Then it can be called from your code, and also from:
>>> - function Report in MiddleWare.cpp
>>> - function SavePackageDescriptionToDebugDump in MiddleWare.cpp
>>> - function get_enabled_reporters in src/CLI/report.cpp
>>> - function GetComponent in src/Daemon/RPM.cpp
>>
>> - hopefully done
>>
>>>
>>> We have the same code 5x in ABRT.
>>>
>>> - The sample code regarding this feature in /etc/abrt/abrt.conf is
>>> misleading:
>>> #CCpp:xorg-x11-apps = RunApp("date", "date.txt")
>>>
>>> If user enables something like this, it will disallow reporting app
>>> crashes. The sample line should include some reporter:
>>> #CCpp:xorg-x11-apps = RunApp("date", "date.txt"), Logger
>>>
>>
>> - yes, then it can't be reported (GUI should probably say, that there is
>> no reporter plugin..in big and red font :))
>>
>>
>> Thanks,
>> Jirka
>>
>>>
>>> On 06/24/2010 11:47 PM, Jiri Moskovcak wrote:
>>>> On 06/24/2010 10:58 AM, Nikola Pajkovsky wrote:
>>>>> On 06/23/2010 10:43 PM, Jiri Moskovcak wrote:
>>>>>> On 06/23/2010 11:13 AM, Nikola Pajkovsky wrote:
>>>>>>> On 06/23/2010 11:02 AM, Karel Klic wrote:
>>>>>>>> On 06/22/2010 10:27 PM, Jiri Moskovcak wrote:
>>>>>>>>> This patch hopefully fixes rhbz#606917, run tested.
>>>>>>>>>
>>>>>>>>> Please review,
>>>>>>>>> Jirka
>>>>>>>>>
>>>>>>>>
>>>>>>>> > + /*try to find analyzer:component first*/
>>>>>>>> > + /* +2 because of \0 and ':' */
>>>>>>>> > + char *analyzer_component =
>>>>>>>> (char*)xzalloc(strlen(pAnalyzer)+strlen(pComponent)+2);
>>>>>>>> > + strcat(analyzer_component,pAnalyzer);
>>>>>>>> > + analyzer_component[strlen(analyzer_component)] =
':';
>>>>>>>> > + strcat(analyzer_component,pComponent);
>>>>>>>> > + map_analyzer_actions_and_reporters_t::iterator
analyzer = >
>>>>>>>>
s_mapAnalyzerActionsAndReporters.find(analyzer_component);
>>>>>>>>
>>>>>>>> We use package name instead of component name for the
same
>>>>>>>> purpose on
>>>>>>>> other places (in CLI, in Report function in
MiddleWare.cpp, in
>>>>>>>> GUI).
>>>>>>>>
>>>>>>>> The component name is the name of SRPM, whereas the
package
>>>>>>>> name is
>>>>>>>> the
>>>>>>>> name of the RPM.
>>>>>>>
>>>>>>
>>>>>> you're right, changed to use the package name
>>>>>>
>>>>>>> instead of this machinery
>>>>>>> + char *analyzer_component =
>>>>>>> (char*)xzalloc(strlen(pAnalyzer)+strlen(pComponent)+2);
>>>>>>> + strcat(analyzer_component,pAnalyzer);
>>>>>>> + analyzer_component[strlen(analyzer_component)] =
':';
>>>>>>> + strcat(analyzer_component,pComponent);
>>>>>>>
>>>>>>> use
>>>>>>>
>>>>>>> char *analyzer_component = xasprintf("%s:%s",
pAnalyzer,
>>>>>>> pComponent);
>>>>>>
>>>>>> fixed
>>>>>>
>>>>>> J.
>>>>>
>>>>> + std::string package = get_crash_data_item_content(pCrashData,
>>>>> FILENAME_PACKAGE);
>>>>> use
>>>>> get_crash_data_item_content_or_NULL
>>>>>
>>>>
>>>> ok.
>>>>
>>>>> + std::string packageName = package.substr(0,
package.rfind("-",
>>>>> package.rfind("-") - 1));
>>>>> use C function
>>>>
>>>> ok, you've asked for it :)
>>>>
>>>>
>>>> Jirka
>>>
>>
>