Hi,
I have a few comments inlined. One of them is serious (the issue I
discussed and complained about to you and msivak, the architect).
All in all it's an ack after the remaining problems are removed. Already
looking forward to tell my first reporter to 'go run logpick and come
back later'.
Ales
On 09/08/2010 01:22 PM, Tomas Mlcoch wrote:
+class LogPicker(object):
+
+ def send(self):
+ if not self.archive_obj:
+ raise LogPickerError("Object for sending hasn't been passed.")
+
No need to raise the exception here: in case the object is not properly
configured at the time when send() is called, it is a programming error
so let's just see the traceback.
+ def getlogs(self):
+
+ f = open(self.filename, 'w')
+
+ for miner in self.miners:
+ desc = "%s (%s)" % (miner._name, miner._description)
+ f.write(desc+"\n")
+ try:
+ miner(f).getlog()
+ except (LogMinerError) as e:
+ self._errprint("Warning: %s - %s" % (miner._name, e))
+ f.write("\n%s\n\n\n" % e)
+
+ f.close()
+
diff --git a/utils/log_picker/archiving.py b/utils/log_picker/archiving.py
new file mode 100644
index 0000000..753fc2b
--- /dev/null
+++ b/utils/log_picker/archiving.py
@@ -0,0 +1,117 @@
+import os
+import shutil
+import tempfile
+import tarfile
+import gzip
+
+
+class ArchivationError(Exception):
+ pass
+
+class NoFilesArchivationError(Exception):
+ pass
Perhaps make NoFilesArchivationError inherit from ArchivationError (so
it's often enough to catch just one exception in an except: block)
+
+class ArchiveBaseClass(object):
+
+ _compression = False
+ _ext = ".ext"
+ _mimetype = ""
+
+ def __init__(self, *args, **kwargs):
+ self._tar_ext = ".tar"
+ pass
+
+ @property
+ def support_compression(self):
+ return self._compression
+
+ @property
+ def file_ext(self):
+ return self._ext
+
+ @property
+ def mimetype(self):
+ return self._mimetype
+
+ def _create_tmp_tar(self, filelist):
+ _, tmpfile = tempfile.mkstemp(suffix=self._tar_ext)
+ tar = tarfile.open(tmpfile, "w")
+ for name in filelist:
+ arcname = name.rsplit('/', 1)[-1]
+ tar.add(name, arcname=arcname)
+ tar.close()
+ return tmpfile
+
+ def create_archive(self, outfilename, filelist):
+ raise NotImplementedError()
+
+
+class tarArchive(ArchiveBaseClass):
Do we need to ship this class? I noticed the default is 'GZIP' and
there's no option to switch to tar.
+
+ _compression = False
+ _ext = ".tar"
+ _mimetype = "application/x-tar"
+
+ def __init__(self, *args, **kwargs):
+ ArchiveBaseClass.__init__(self, args, kwargs)
+
+ def create_archive(self, outfilename, filelist):
+ if not filelist:
+ raise NoFilesArchivationError("No files to archive.")
+
+ size = 0
+ for file in filelist:
+ size += os.path.getsize(file)
+ if size<= 0:
+ raise NoFilesArchivationError("No files to archive.")
+
+ tmptar = self._create_tmp_tar(filelist)
+ shutil.move(tmptar, outfilename)
+ return outfilename
+
+
+class gzipArchive(ArchiveBaseClass):
Generaly in Anaconda we try to use uppercase letter at the start of a
class name (whenever it won't make the class name look completely silly
like "Iscsi").
(personally I also try to follow
http://www.python.org/dev/peps/pep-0008/, but the Anaconda code is more
random sometimes).
diff --git a/utils/log_picker/logmining.py b/utils/log_picker/logmining.py
new file mode 100644
index 0000000..61f300b
--- /dev/null
+++ b/utils/log_picker/logmining.py
@@ -0,0 +1,191 @@
+import os
+import shlex
+import time
+import subprocess
+
+
+class LogMinerError(Exception):
+ pass
+
+
+class LogMinerBaseClass(object):
+
+ _name = "name"
+ _description = "Description"
+ _prefer_separate_file = True
+
+ def __init__(self, logfile=None, *args, **kwargs):
+ self.logfile = logfile
+ self._used = False
+
+ def _write_separator(self):
+ self.logfile.write('\n\n')
+
this method name:
+ def _get_file(self, file):
+ self.get_files([file])
+
and this method name:
+ def _get_files(self, files):
+ if self._used:
+ self._write_separator()
+ self._used = True
+
+ for filename in files:
+ self.logfile.write('%s:\n' % filename)
+ try:
+ f = open(filename, 'r')
+ except (IOError) as e:
+ self.logfile.write("Exception while opening: %s\n" % e)
+ continue
+
+ self.logfile.writelines(f)
+ f.close()
are not too fortunate: the methods are not getting anything, have side
effects and return None. Just '_write_file()' or similar?
+class AnacondaLogMiner(LogMinerBaseClass):
+
+ _name = "anaconda_log"
+ _description = "Log dumped from Anaconda."
+ _prefer_separate_file = True
+
+ def _action(self):
+ # Actual state of /tmp
+ old_state = set(os.listdir('/tmp'))
+
+ # Tell Anaconda to dump itself
+ proc = subprocess.Popen(shlex.split("killall -s SIGUSR2 anaconda"),
+ stdout=subprocess.PIPE, stderr=subprocess.PIPE)
It would be much better if you used 'kill' with the pid from
/var/run/anaconda.pid. Anaconda is forked into 3 or so processes but we
only want the traceback from the main process.
+ proc.communicate()
+ #ret = os.system("killall -s SIGUSR2 anaconda")
+ if proc.returncode:
+ raise LogMinerError('Error while sending signal to Anaconda')
+
+ time.sleep(5)
+
+ # Check if new traceback file exists
+ new_state = set(os.listdir('/tmp'))
+ tbpfiles = list(new_state - old_state)
+
+ if not len(tbpfiles):
+ raise LogMinerError('Error: No anaconda traceback file exist')
+
+ for file in tbpfiles:
+ if file.startswith('anaconda-tb-'):
+ tbpfile_name = file
+ break
+ else:
+ raise LogMinerError('Error: No anaconda traceback file exist')
+
+ # Copy anaconda traceback log
+ self._get_file('/tmp/%s' % tbpfile_name)
This method is a root of all evil and I am worried we'll have to change
it at some point. First it depends on the anaconda process still running
and being responsive. Second it doesn't give me what I originally asked
for: that is a copy of anaconda.log, storage.log, program.log etc. just
as it lies in /tmp in the moment of the invocation, no other layer (meh,
exception.py) included and no need to split the resulting file by hand
for quick browsing.
On the other hand, you introduced a good measure of modularity and it
should be simple enough to change this later (or even now!).
diff --git a/utils/logpicker b/utils/logpicker
+class ApplicationScope(object):
+ """Application configuration class."""
+
+ def __init__(self, parser_options={}):
+ # sender
+ self.sender = ""
+ self.bug_id = parser_options.bug_id or ""
+ self.bug_comment = parser_options.bug_comment or ""
+ self.bz_login = parser_options.bz_login or ""
+ self.bz_password = ""
You can also use None when you mean to say 'no value'.
+
+ # archivator
+ self.archivator = ""
+
+ # miners
+ if parser_options.l_all:
+ self.miners = MINERS
How about making this the default so we don't have to tell the reporters
to use '-A' all the time? Are you worried there could be too many files?
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
