Tails

#1 Updated by intrigeri 2015-05-06 08:15:49

• blocks Feature #8557: Have Tails Installer uploaded and accepted into Debian added

#2 Updated by intrigeri 2015-05-06 08:15:59

• blocks #8538 added

#3 Updated by intrigeri 2015-06-05 18:05:47

• Assignee deleted (intrigeri)
• % Done changed from 0 to 50
• QA Check set to Ready for QA
• Feature Branch set to bugfix/9349-safer-error-logging

#4 Updated by intrigeri 2015-06-05 18:08:01

• Feature Branch changed from bugfix/9349-safer-error-logging to liveusb-creator:bugfix/9349-safer-error-logging

#5 Updated by intrigeri 2015-06-05 18:10:02

• Assignee set to anonym

#6 Updated by intrigeri 2015-06-05 18:11:46

• Status changed from Confirmed to In Progress

#7 Updated by anonym 2015-06-18 04:37:42

• Assignee changed from anonym to intrigeri
• QA Check changed from Ready for QA to Info Needed

Tested by hot-patching liveusb-creator in a running Tails session. The logging works fine, however, even if there’s no error, an empty log file is created (which is quite clear given the usage of NamedTemporaryFile() in _setup_error_log_file(), which is always run). This is different from before, so I think it’s worth noting. Personally I do not like programs that unnecessarily leave cruft like this behind.

We could delay the call of _setup_error_log_file() until write_log() actually is called (i.e. upon error), and have _error_log_filename set to None until then, but since we read this field outside of write_log() we get a dependency that perhaps is a bit ugly. Since write_log() is only used there, we could kill it, and this dependency would be eliminated:

--- a/liveusb/creator.py
+++ b/liveusb/creator.py
@@ -95,7 +95,7 @@ class LiveUSBCreator(object):
                 print >> sys.stderr, _("You must run this application as root")
                 sys.exit(1)
         self._setup_logger()
-        self._error_log_filename = self._setup_error_log_file()
+        self._error_log_filename = None

     def _setup_error_log_file(self):
         temp = tempfile.NamedTemporaryFile(mode='a', delete=False,
@@ -348,7 +348,10 @@ class LiveUSBCreator(object):
         err = unicode_to_utf8(err)
         self.output.write(out + '\n' + err + '\n')
         if proc.returncode:
-            self.write_log()
+            if not self._error_log_filename:
+                self._error_log_filename = self._setup_error_log_file()
+            with open(self._error_log_filename, 'a') as out:
+                out.write(self.output.getvalue())
             if not passive:
                 raise LiveUSBError(_(
                         "There was a problem executing the following command: `%(command)s`.\nA more detailed error log has been written to '%(filename)s'.")
@@ -518,10 +521,6 @@ class LiveUSBCreator(object):
                                          " previous LiveOS: %(message)s")
                                          % {'message': str(e)})

-    def write_log(self):
-        """ Write out our subprocess stdout/stderr to a log file """
-        with open(self._error_log_filename, 'a') as out:
-            out.write(self.output.getvalue())

     def get_liveos(self):
         return self.get_liveos_file_path(config['main_liveos_dir'])

What do you think?

#8 Updated by intrigeri 2015-06-18 07:25:25

• Assignee changed from intrigeri to anonym

anonym wrote:
> Tested by hot-patching liveusb-creator in a running Tails session. The logging works fine, however, even if there’s no error, an empty log file is created (which is quite clear given the usage of NamedTemporaryFile() in _setup_error_log_file(), which is always run). This is different from before, so I think it’s worth noting. Personally I do not like programs that unnecessarily leave cruft like this behind.

Got it.

> We could delay the call of _setup_error_log_file() until write_log() actually is called (i.e. upon error), and have _error_log_filename set to None until then, but since we read this field outside of write_log() we get a dependency that perhaps is a bit ugly. Since write_log() is only used there, we could kill it, and this dependency would be eliminated:
> […]
> What do you think?

I dislike inlining stuff, that previously was in a dedicated function, like this. Besides, we might want to log to that file from other places in the future.

I propose to instead just delete the log file if it’s empty, when successfully exiting the program. What do you think?

#9 Updated by anonym 2015-06-27 06:01:51

• Assignee changed from anonym to intrigeri
• QA Check changed from Info Needed to Dev Needed

intrigeri wrote:
> I propose to instead just delete the log file if it’s empty, when successfully exiting the program. What do you think?

Sure.

#10 Updated by intrigeri 2015-06-28 03:45:30

• Target version changed from Tails_1.4.1 to Tails_1.5

Too late for having this reviewed’n’merged for 1.4.1.

#11 Updated by intrigeri 2015-07-13 05:41:59

• Assignee changed from intrigeri to anonym
• % Done changed from 50 to 60
• QA Check changed from Dev Needed to Ready for QA

Implemented what we agreed upon => anonym (since you had a look at this code already, and you’ll be the RM again in a week), please review’n’merge.

#12 Updated by anonym 2015-07-24 03:30:24

• Status changed from In Progress to Fix committed
• Assignee deleted (anonym)
• % Done changed from 60 to 100
• QA Check changed from Ready for QA to Pass

intrigeri wrote:
> Implemented what we agreed upon => anonym (since you had a look at this code already, and you’ll be the RM again in a week), please review’n’merge.

Looks and works great! Merged!

#13 Updated by BitingBird 2015-08-11 10:42:29

• Status changed from Fix committed to Resolved