[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH 1/2] Add logpicker tool into utils



Hi Tomas,

don't forget about a nice commit msg before pushing.

Comments are inlined:

diff --git a/utils/log_picker/archiving.py b/utils/log_picker/archiving.py
+    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]

Do not leave commented out code behind, unless there is a compelling reason (for instance an example how someone got it wrong previously).

+            pieces = name.rsplit('/', 2)

You want to check out the os.path.split method:
http://docs.python.org/library/os.path.html?highlight=path#os.path.split

diff --git a/utils/log_picker/argparser.py b/utils/log_picker/argparser.py
new file mode 100644
index 0000000..8a4c4e1
--- /dev/null
+++ b/utils/log_picker/argparser.py
@@ -0,0 +1,280 @@
+import optparse

I just found out optparse is deprecated in python 2.7, so optionally if you decide to change the help sections (see below) you can as well switch for argparse.

+        if sending.RHBZ in sending.NOT_AVAILABLE:
+            title = "Send the report to the Red Hat Ticketing system | options"
+            params_info = \
+            "-r, --rhel                                                     \n"\
+            "                    Send the report to the Red Hat Ticketing   \n"\
+            "                    system.                                    \n"\
+            "-i ID, --idbug=ID                                              \n"\
+            "                    The case number in the Red Hat Ticketing   \n"\
+            "                    system.                                    \n"\
+            "-l USERNAME, --login=USERNAME                                  \n"\
+            "                    Set the Red Hat Customer Portal username.  \n"
+
+            bzg = SimpleOptionGroup(self.parser, title, params_info)
+            bzg.add_option("-r", "--rhel", action="store_true", dest="strata")

Hm, isn't it easier just to use the help= keyword argument with add_option? Or what made you spell out the entire help sections?


diff --git a/utils/log_picker/logmining.py b/utils/log_picker/logmining.py
+    def set_logfile(self, logfile):
+        self.logfile = logfile

Did you want to make this a @property?

+
+    def _write_separator(self):
+        self.logfile.write('\n\n')
+
+    def _write_files(self, files):
+        if not isinstance(files, list):
+            files = [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

Or even more cute:
with open(filename, r') as f:
	...

diff --git a/utils/log_picker/sending/scpsender.py b/utils/log_picker/sending/scpsender.py
+    def _scp_auth(self, master, childpid, password):
+        childout = ""
+        while True:
+            # Read up to password prompt.  Propagate OSError exceptions, which
+            # can occur for anything that causes scp to immediately die (bad
+            # hostname, host down, etc.)
+            buf = os.read(master, 4096)
+            childout += buf
+            if buf.lower().find("password: ") != -1:
+                os.write(master, password+"\n")
+                # read the space and newline that get echoed back
+                buf = os.read(master, 2)
+                childout += buf
+                break
+
+        while True:
+            try:
+                buf = os.read(master, 4096)
+                childout += buf
+            except (OSError, EOFError):
+                break
+
+        (_, childstat) = os.waitpid (childpid, 0)
+        return (childstat, childout)


Isn't it possible to have scp read the password directly from stdin of logpicker (i.e. so when user is typing her precious ssh password it is handled by scp directly, not by us)? I can imagine that this could be a security concern.

+if __name__ == "__main__":
+
+    # Argument parsing
+    try:
+        options = argparser.ArgParser().parse()
+    except (ArgError) as e:
+        sys.stderr.write("Argument error: %s\n" % e)
+        sys.exit(1)
+
+    # Application scope
+    scope = ApplicationScope(options)
+    if scope.sender == sending.RHBZ or scope.sender == sending.STRATA or \
+       scope.sender == sending.SCP or \
+       (scope.sender == sending.FTP and scope.login):
+        scope.password = getpass.getpass("Password: ")

Let's hope holding the user's ssh or bugzilla password can be considered secure.

You will be ready to push after we resolve the ssh password handling problem. Also please look at every line in your code starting with # and remove commented out code.

Ales


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]