Merge lp:~rbalint/ubuntu-release-upgrader/ubuntu-release-upgrader-screen into lp:ubuntu-release-upgrader

[Balint Reczey]

[Balint Reczey (rbalint)]

Testing this is a bit tricky since on Zesty the upgrade process downloads a tarball from Artful and uses the code from there.
After Artful has this code the test can be added to Zesty, too.

[Brian Murray (brian-murray)]

One manual way to test release upgrades is to put the dist_upgrader tarball, from the package build, on a system you want to upgrade. Then you can extract the tarball which will contain a file named after the release to which you are upgrading and run that file with the appropriate arguments e.g.

./artful --frontend DistUpgradeViewText

That's just from memory though so I may be slightly wrong.

[Balint Reczey (rbalint)]

I used this dirty patch to build a test package:

--- a/DistUpgrade/DistUpgradeFetcherCore.py
+++ b/DistUpgrade/DistUpgradeFetcherCore.py
@@ -106,6 +106,8 @@ class DistUpgradeFetcherCore(object):
         except tarfile.ReadError as e:
             logging.error("failed to open tarfile (%s)" % e)
             return False
+ print("fix screen usage")
+ subprocess.call(["cp","/usr/lib/python3/dist-packages/DistUpgrade/DistUpgradeMain.py",self.tmpdir])
         return True

     def verifyDistUprader(self):

[Brian Murray (brian-murray)]

Thanks for working on this! The bug report indicates that the argument to "-L" is optional, if that is the case wouldn't it be simpler to just make "-L" the last argument to screen? This way screen would just use whatever the default log file name is and we'd be consistent across releases.

[Balint Reczey (rbalint)]

The last arguments are the original cmd line args, thus IMO we can't really do that.

This could have worked but it did not:
$ screen -S fdsfa -L -- top -d 1
-L: logfile name can not start with "-" symbol

[Brian Murray (brian-murray)]

Okay, I was just trying to find a way that would reduce the amount of changes necessary to resolve the issue. Given that we are distributing and using our own screenrc file I thought we might get it to work by modifying it. I added the following lines to screenrc:

deflog on
log on

I also removed the passing of the -L switch to screen and that seemed to work i.e. /var/log/dist-upgrade/screenlog.0 was written to. What do you think about that instead?

It's also worth noting that the apport package hooks collects the screenlog and expects the log file name to be screenlog.0 so we should keep that log file name the same.

[Balint Reczey (rbalint)]

I would not like screen logging by default. There can be sensitive information in the terminal from remote systems for example.

I have updated the patch to use screenlog.0, but there seems to be an even better option, updating screen in Zesty :-):

I'm glad to announce that the new version of GNU Screen (4.5.1) has been released today!

https://savannah.gnu.org/forum/forum.php?forum_id=8807 :

 ...
 4.5.1 is a bugfix/security release.
 We fixed few crashes and, of course, problem with privilege escalation.
 We also changed behavior of '-L' option: now you can set new logfile name using '-Logfile' option and '-L' just tells screen to turn on logging.
 ...

[Balint Reczey (rbalint)]

Since is it not clear when the screen package gets updated for Zesty and also not clear whether the update bumps the version number the fix checked in the original version I've updated the fix to check the actual behavior, not the version.

[Brian Murray (brian-murray)]

On Tue, May 02, 2017 at 08:33:33AM -0000, Balint Reczey wrote:
> I would not like screen logging by default. There can be sensitive information in the terminal from remote systems for example.

I'm not sure I understand this point, the screenrc file in the
dist-upgrader tarball is only used when the distribution upgrade process
calls screen and it logged whenever it called screen anyway so whether
we try and use -L or set the log options in the screenrc file the
results are the same.

> I have updated the patch to use screenlog.0, but there seems to be an even better option, updating screen in Zesty :-):

Fixing screen does seem like the best idea though.

--
Brian Murray

[lp:~rbalint/ubuntu-release-upgrader/ubuntu-release-upgrader-screen]

3049. By Launchpad Translations on behalf of ubuntu-core-dev <Unknown> on 2017-05-03

Launchpad automatic translations update.

3050. By Balint Reczey on 2017-05-03

Turn on screen's logging in screenrc instead of via command line because
the command line syntax is changed in screen 4.5.0 (LP: #1686117)

[Balint Reczey (rbalint)]

> On Tue, May 02, 2017 at 08:33:33AM -0000, Balint Reczey wrote:
> > I would not like screen logging by default. There can be sensitive
> information in the terminal from remote systems for example.
>
> I'm not sure I understand this point, the screenrc file in the
> dist-upgrader tarball is only used when the distribution upgrade process
> calls screen and it logged whenever it called screen anyway so whether
> we try and use -L or set the log options in the screenrc file the
> results are the same.

I thought you were referring to screen package's screenrc not the one we use here.
I think using the screenrc in the downloaded tarball is the best option indeed and
even if screen gets fixed in Zesty it is cleaner.

I have updated the patch to use screenrc and dropped the autopkgtest because it is
not really worth it to run it only for this small change.

>
> > I have updated the patch to use screenlog.0, but there seems to be an even
> better option, updating screen in Zesty :-):
>
> Fixing screen does seem like the best idea though.
>
> --
> Brian Murray

[Brian Murray (brian-murray)]

Alright, thanks! I'll go ahead and get this merged and uploaded.

review: Approve

=== modified file 'DistUpgrade/DistUpgradeMain.py'
--- DistUpgrade/DistUpgradeMain.py	2016-01-13 17:53:27 +0000
+++ DistUpgrade/DistUpgradeMain.py	2017-05-03 11:02:55 +0000
@@ -193,7 +193,6 @@
     # unset escape key with -e, enable log with -L, set name with -S
     cmd = ["screen", 
            "-e", "\\0\\0",
-           "-L", 
            "-c", "screenrc",
            "-S", SCREENNAME]+sys.argv
     logging.info("re-exec inside screen: '%s'" % cmd)

=== modified file 'DistUpgrade/screenrc'
--- DistUpgrade/screenrc	2010-11-29 09:23:24 +0000
+++ DistUpgrade/screenrc	2017-05-03 11:02:55 +0000
@@ -1,3 +1,5 @@
+deflog on
+log on
 logfile /var/log/dist-upgrade/screenlog.%n
 logtstamp on
 zombie xr
\ No newline at end of file

=== modified file 'debian/changelog'
--- debian/changelog	2017-04-24 22:08:48 +0000
+++ debian/changelog	2017-05-03 11:02:55 +0000
@@ -1,3 +1,10 @@
+ubuntu-release-upgrader (1:17.10.2) UNRELEASED; urgency=medium
+
+  * Turn on screen's logging in screenrc instead of via command line because
+    the command line syntax is changed in screen 4.5.0 (LP: #1686117)
+
+ -- Balint Reczey <[e-mail]>  Thu, 27 Apr 2017 18:33:00 +0200
+
 ubuntu-release-upgrader (1:17.10.1) artful; urgency=medium
 
   * DistUpgrade/{EOL,Devel}ReleaseAnnouncement,