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 >

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 >>> >> >