Hi,
thank you for your comment.
I followed your advices.
----- "Ales Kozumplik" <akozumpl@xxxxxxxxxx> wrote:
> 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.
Exception has been removed.
> > + 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)
Fixed.
> > +
> > +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.
You're right. I've removed all unused class.
> > +
> > + _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).
I've fixed that.
> > 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?
Good point. I've changed names to _write_*.
> > +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.
Fixed.
> > + 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!).
I agree, I'll change it in future. At least for now I've added method for split anaconda log.
> > 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'.
Changed "" -> None
> > +
> > + # 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?
All reporters are now used by default.
Tomas
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
