[Merge] lp:~vorlon/ubuntu/utopic/apport/lp.1354318 into lp:~ubuntu-core-dev/ubuntu/utopic/apport/ubuntu

[Steve Langasek]

On Fri, Aug 08, 2014 at 11:47:32AM -0000, Martin Pitt wrote:
> > === modified file 'data/whoopsie-upload-all'
> > --- data/whoopsie-upload-all	2014-07-29 10:24:37 +0000
> > +++ data/whoopsie-upload-all	2014-08-08 10:43:52 +0000
> > @@ -1,4 +1,4 @@
> > -#!/usr/bin/python
> > +#!/usr/bin/python3

> Please revert. Apport works with both Pythons, and setup.py is designed to
> update the hashbangs when called with Python 3.

Ok.  I see that this is in fact inconsistent with all the rest of apport, so
it doesn't make sense to change only this one.

However, it's well past time to make /usr/bin/python3 the default, including
at build time.  I will probably raise this issue again separately. :)

> I don't like writing an entirely new report file here. This will have to
> compress/write the whole initial report again (mostly the core dump). 
> With the current code the existing file is opened in append mode, and only
> new keys are being written (r.write(f, only_new=True)).

Yes, which means that with the current code, the replacement of the existing
report is not atomic.  If the system powers down mid-write, you can have a
half-written .crash file that could result in misprocessing on next boot.  I
don't think that we should be optimizing this at the expense of file
handling atomicity, as this does impact the reliability of the error
reporting system generally.

Would you be happier with this if, instead of writing the full report from
scratch to the new file, we instead copied the base file contents with
straight file operations and then then continued to use only_new=True?

> If there is a reason why this doesn't work somehow: Then this needs to be
> fixed to set the new file to permissions 000 so that nothing else which
> inotifies the directory is trying to read the .new file while it's still
> being written (like the old code).

The only things that watch this directory for inotify look for specific file
extensions.  I don't believe that creating a file with a .new extension will
have any adverse impact.

> > +            fcntl.flock(f, fcntl.LOCK_EX | fcntl.LOCK_NB)
> > +        except IOError as e:

> unused variable "e", I suppose the pyflakes test (test/run) should catch
> this?

Didn't notice any complaints in the pyflakes output during build, fwiw.  But
anyway, fixed.

> > +            print('%s already being processed, skipping' % report)
> > +            return None
> > +
> > +        r = apport.Report()
> > +
> > +        try:
> > +            with open(report, 'rb') as f2:
> > +                r.load(f2, binary='compressed')
> > +                user_details = os.fstat(f2)

> Nitpick: No need to use "f2" here; in these constructions "f" always has a
> tiny scope (only within the f).

> Nitpick 2: could you please s/user_details/report_stat/, so that it's
> clearer what it is?  (It has quite a wide scope)

Fixed both of these, thanks.

> There's a ton of these unlink calls in every exit path. It's more robust
> to do that at the very end in finally: (and ignore errors in case it's
> already gone, because it succeeded and was renamed).  But this is probably
> moot anyway, I'd really like to avoid having to rewrite the entire file
> (see above).

In the meantime, I've adjusted this to use try/finally.

> > @@ -133,8 +153,8 @@
> >  #
> >  parser = argparse.ArgumentParser(description='Noninteractively upload all '
> >                                   'Apport crash reports to errors.ubuntu.com')
> > -parser.add_argument('-t', '--timeout', default=1800, type=int,
> > -                    help='seconds to wait for whoopsie to upload the reports (default: 1800s)')
> > +parser.add_argument('-t', '--timeout', default=0, type=int,
> > +                    help='seconds to wait for whoopsie to upload the reports (default: do not wait)')
> >  opts = parser.parse_args()

> Has this been cleared with CI (Paul Larson/Francis Ginther) that this
> default behaviour change doesn't break CI?  I wrote this script on their
> request to run it at the end of MPs and image testing, where it's crucial
> that it waits until everything has been uploaded.  So this might need a
> change in CI first to set an explicit timeout there?

I haven't cleared this with them.  I checked that this is compatible with
the current behavior of utah, but I will also confirm with them directly
before pushing this change.

> If this turns out to be too unwieldy, it's equivalently possible to modify
> the upstart job to use --timeout=0.

Indeed.

-- 
https://code.launchpad.net/~vorlon/ubuntu/utopic/apport/lp.1354318/+merge/230083
Your team Ubuntu Core Development Team is subscribed to branch lp:~ubuntu-core-dev/ubuntu/utopic/apport/ubuntu.