Hi Michal,
I have spent some time with the retrace server codebase and gathered a few suggestions for improvements. Please see below.
Kind regards, Karel
src/retrace/worker.py =====================
line 7: sys.path = ["/usr/share/abrt-retrace/"] + sys.path
sys.path.insert(0, "/usr/share/abrt-retrace/")
The "/usr/share" prefix should not be hardcoded. You can write a new file config.py.in for settings coming from the build system: datadir = "@datadir@"
Then you can generate config.py from config.py.in by adding a rule to Makefile.am: config.py: config.py.in sed -e 's|@datadir[@]|$(datadir)|g' $< > $@
Then you can use it from worker.py: import config sys.path.insert(0, os.path.join(config.datadir, "abrt-retrace"))
Please consider using it also for the rest of the codebase (also plugins/__init__.py hardcodes /usr/share).
line 15: if not LOG or not taskid:
Python idiom for checking for None value is "if LOG is None".
Also the situation where LOG is none there can never occur, and if it would occur, other parts of the script would fail because they assume LOG is not None.
line 18: filepath = "%s/%s/status" % (CONFIG["SaveDir"], taskid)
Proposed: filepath = os.path.join(CONFIG["SaveDir"], taskid, "status")
Please consider using os.path.join instead of composing paths manually everywhere. os.path.join is a Python idiom for joining paths, even when we do not care about portability it makes code more readable.
line 26: LOG.write("%s " % STATUS[statusid])
Proposed: LOG.write("{0} ".format(STATUS[statusid])
Please consider using the string.format method instead of the % operator everywhere. The Python documentation says that the format method of string formatting should be preferred to the % formatting in new code.
src/retrace/retrace.py ======================
lines 13-17: DF_BIN = "/bin/df"
DU_BIN = "/usr/bin/du" GZIP_BIN = "/usr/bin/gzip" TAR_BIN = "/bin/tar" XZ_BIN = "/usr/bin/xz"
The paths should be detected during build. The reason is that it might easily happen that /usr/bin/du has been moved to /bin/du and the transition symlink is going to be removed, or someone tries to run retrace server on non-RHEL-6 server.
You can use AC_PATH_PROG macro in configure.ac: AC_PATH_PROG([DU], [du], [no]) [if test "$DU" = "no"] [then] [echo "The du program was not found in the search path. Please ensure"] [echo "that it is installed and its directory is included in the search path."] [echo "Then run configure again before attempting to build ABRT/retrace server."] [exit 1] [fi]
The macro is documented in http://www.gnu.org/s/hello/manual/autoconf/Generic-Programs.html
Then it should be possible to replace @du@ in config.py.in in Makefile.am: config.py: config.py.in sed -e 's|@datadir[@]|$(datadir)|g' -e 's|@du[@]|$(DU)|g' $< > $@
Then you can use 'import config; config.du' instead of hardcoded DU_BIN.
line 19: TASKID_PARSER = re.compile("^.*/([0-9]+)/*$")
This regexp doesn't seem to be used anywhere.
line 20: PACKAGE_PARSER = re.compile("^(.+)-([0-9]+(.[0-9]+)*-[0-9]+).([^-]+)$")
You can use non-capturing version of parenteses if you want, it makes the regexp more clear: PACKAGE_PARSER = re.compile("^(.+)-([0-9]+(?:.[0-9]+)*-[0-9]+).([^-]+)$") ^^
line 20-24: PACKAGE_PARSER = re.compile("^(.+)-([0-9]+(.[0-9]+)*-[0-9]+).([^-]+)$") ...
It would be helpful to include comments about what the parsers are supposed to do (how the input looks like), as it isn't obvious from them.
line 45: CONFIG_FILE = "/etc/abrt/retrace.conf"
Do not hardcode /etc here, use the value provided by the build system via the config.py.in mechanism.
lines 62-74: STATUS_ANALYZE, STATUS_INIT, STATUS_BACKTRACE, STATUS_CLEANUP, \ STATUS_STATS, STATUS_FINISHING, STATUS_SUCCESS, STATUS_FAIL = xrange(8) ...
Shouldn't it be in worker.py? Status is a worker-specific idiom/expression, isn't it?
lines 109-112: try:
CONFIG[key] = get("retrace", key) except: pass
Retrace server must not start silently when the configuration file is not in supposed format or administrator left a typo in the config.
It should terminate with an error message explaining what is wrong.
line 126: pipe = Popen([DU_BIN, "-s", path], stdout=PIPE).stdout
You can add "-b" here to remove the 1024-bytes block assumption (which might not be valid, depending on system settings) about du from our code. You can then return int(match.group(1)) instead of 1024 * int(match.group(1)).
line 278: def get_running_tasks(ps_output=None):
Since this function is used only in the abrt-retrace-cleanup script, why not to move it there (incl. the WORKER_RUNNING_PARSER regexp)?
retrace.py should contain only code that needs to be shared across multiple script, to keep it simple and manageable. All functions in retrace.py should be there for a reason.
line 326: call(["rm", "-rf", "%s/crash" % savedir, "%s/mock.cfg" % savedir],
Please use Python's shutils.rmtree and os.remove if possible.
lines 352-364: CREATE TABLE IF NOT EXISTS retracestats( taskid INT NOT NULL, package VARCHAR(255) NOT NULL, version VARCHAR(16) NOT NULL, release VARCHAR(16) NOT NULL, arch VARCHAR(8) NOT NULL, starttime INT NOT NULL, duration INT NOT NULL, prerunning TINYINT NOT NULL, postrunning TINYINT NOT NULL, chrootsize BIGINT NOT NULL )
Proposed: CREATE TABLE IF NOT EXISTS retracestats( taskid NOT NULL, package NOT NULL, version NOT NULL, release NOT NULL, arch NOT NULL, starttime NOT NULL, duration NOT NULL, prerunning NOT NULL, postrunning NOT NULL, chrootsize NOT NULL )
No need to use VARCHAR(xy) with SQLite. SQLite allows to store a string of any length to such a field anyway, and it always uses memory depending on the string length, not using the VARCHAR limit anywhere.
Anyway, SQLite uses dynamic typing, so it doesn't prevent you to insert a string value to an INT field. There is no need to set types.
line 394: class logger():
Proposed: class Logger:
Class names shoud start with uppercase letter. The parentheses are only used when you need class inheritance.
line 409: os.rename(self._logfile.name, self._logfile.name.replace("/log", "/retrace_log"))
Proposed: final_name = os.path.join(os.path.dirname(self._logfile.name), "retrace_log") os.rename(self._logfile.name, final_name)
The .replace call breaks things when CONFIG["SaveDir"] contains the "/log" substring as well.
crash-catcher@lists.fedorahosted.org