On Wed, 2010-09-22 at 12:48 +0200, Nikola Pajkovsky wrote:
> + dsc = xstrdup("Crashed executable does not belong to any
installed package");
> + goto reopen_dd_and_save;
> ...
> ...
> ...
> + reopen_dd_and_save:
> + dd = dd_init();
> + if (!dd_opendir(dd, dump_dir_name))
> + {
> + dd_close(dd);
> + result = 1;
> + goto ret;
> + }
>
> I'm not a fan of goto in this context. goto as jump to the end of function is
pretty good, but this ;).
Fixed.
> + /*
> + Checking the MD5 sum requires to run prelink to "un-prelink"
the
> + binaries - this is considered potential security risk so we don't
> + use it, until we find some non-intrusive way
> +
> + Delete?
> + */
> + /*
> + if (!CheckHash(package_short_name.c_str(), executable))
> + {
> + error_msg("Executable '%s' seems to be modified,
"
> + "doesn't match one from package
'%s'",
> + executable, package_short_name.c_str());
> + result = 1;
> + goto ret;
> + }
> + */
>
> yes, it's not a problem dig it from git again.
The purpose of this comment is to stop someone from adding this code
again. The comment can be shortened, though...
> +abrt_action_save_package_data_SOURCES = \
> + rpm.h rpm.c \
> + Settings.h Settings.cpp \
> + abrt-action-save-package-data.cpp
> +abrt_action_save_package_data_CPPFLAGS = \
> + -I$(srcdir)/../../inc \
> + -I$(srcdir)/../../lib/utils \
> + -DBIN_DIR=\"$(bindir)\" \
> + -DVAR_RUN=\"$(VAR_RUN)\" \
> + -DCONF_DIR=\"$(CONF_DIR)\" \
> + -DLOCALSTATEDIR='"$(localstatedir)"' \pacakge
> ^^^ typo
Fixed.
> When I see function 200 lines long. I'm the guy at the position 8.
(
http://www.pakzilla.com/2007/09/05/programmers-life-cartoon/)
The original SavePackageDescriptionToDebugDump() is about
the same size. Things aren't getting worse in this regard.
Please review the attached version 2 of the patch.
commit it. Thanks.
--
Nikola