Bug#863095: screen: syntax changed for -L argument, effectively an API change :/

2017-05-22 Thread Daniel Kahn Gillmor 
On Tue 2017-05-23 00:43:01 +0200, Axel Beckert wrote:
> Control: tag -1 + patch moreinfo
>
> Daniel and others who might be affected by that regression: Please
> test the patch at the end of the mail if it suffices to fix your issue
> with regards the API and if it doesn't cause any other regressions.

I've tested git commit fd125417bafe21915f1160428cdd4daae6300f6a from
https://anonscm.debian.org/git/collab-maint/screen.git and i believe it
resolves the issue for me.  Thanks for your prompt action!

I owe you the beverage of your choosing the next time i see you, Axel :)

  --dkg


signature.asc
Description: PGP signature

Bug#863095: screen: syntax changed for -L argument, effectively an API change :/

2017-05-22 Thread Daniel Kahn Gillmor 
On Sun 2017-05-21 21:03:36 +0200, Axel Beckert wrote:
> Control: forwarded -1 http://savannah.gnu.org/bugs/?50440

hey, thanks for finding that, Axel!

> Can you check if this is fixed in screen 4.5.1-3 from Debian
> Experimental? Because according to the upstream bug report
> http://savannah.gnu.org/bugs/?50440 this should be fixed in 4.5.1.

yes, i can confirm that it is fixed by 4.5.1-3.

my test is pretty simple:

touch $(pwd)/screenrc
screen -L -c $(pwd)/screenrc

> If so, I'll try to cherry-pick the according upstream commits. Seem to
> be these commits:
>
> Code change:
> http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4=18193bc7b237d66a076fffa21d4308dbb4f83cc5
>
> Documentation updates:
> http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4=13183da34eaf9efb3c6b47371c08ff786eaf712b
>  
> http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4=a38de4e6627f01cad43cff57490fd80a988b8e18
> http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4=74c5883c47d84154e322dc18adbafbe7af5795b3

I haven't tested with these commits cherry-picked myself, but it sounds
like you're on the right track.

much appreciation for the rapid diagnosis!

 --dkg


signature.asc
Description: PGP signature

Bug#863095: screen: syntax changed for -L argument, effectively an API change :/

2017-05-22 Thread Axel Beckert 
Control: tag -1 + patch moreinfo

Hi again,

Daniel and others who might be affected by that regression: Please
test the patch at the end of the mail if it suffices to fix your issue
with regards the API and if it doesn't cause any other regressions.

Axel Beckert wrote:
> Axel Beckert wrote:
> > > but if you have any pre-existing code that does something like:
> > > 
> > > screen -L -c foo.screenrc
> > > 
> > > then it fails in 4.5.0 because it doesn't like -c as an argument to
> > > -L.
[...]
> > Seem to be these commits:
> > 
> > Code change:
> > http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4=18193bc7b237d66a076fffa21d4308dbb4f83cc5
> 
> Unfortunately this commit doesn't apply to 4.5.0. But I noticed
> there's an earlier and much smaller commit which explicitly targets the
> API regression:
> 
> http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4=c14e05e7c36c64d85198ed0fc89177427ece48d4
> 
> I'll check if that also suffice.

Unfortunately not. While it seems to suffice for case reported at
upstream, it does not suffice for your case:

Calling "screen -L -c foo.screenrc" with that patch applied starts a
screen session, which immediately shows the message "Cannot exec
'foo.screenrc': No such file or directory" in the status line and then
exits again.

But as far as I understand the code it was just forgetten to revert
the two counters if the argument after -L starts with a dash. So
adding

  av--;
  ac++;

(and curly braces) to that if clause seems to do it for my quick
tests.

Full patch as planned:

Origin: c14e05e7c36c64d85198ed0fc89177427ece48d4
Author: Alexander Naumov 
Description: Ignore logfile's name that begins with the "-" symbol
 This fixes the API:
 .
 To enable logging we use -L option. But in case of
 default logfile name (screenlog.0) we will need to
 define it anyway. Because screen will try to interpret
 next option as a parameter for -L option (which is
 logfile name). It will fails ALWAYS, because next
 parameter will always start with "-" symbol...
 what is not permited for logfile name of course.
 .   
 For example:
 .
 $ screen -L -D -m ./configure
 .
 In this case logfile name is screenlog.0, because "-D"
 will not be interpreted by screen as a name of logfile.
Bug-Debian: https://bugs.debian.org/863095
Bug: https://savannah.gnu.org/bugs/?50440
Reviewd-By: Axel Beckert 

--- a/doc/screen.1
+++ b/doc/screen.1
@@ -262,8 +262,8 @@
 tells
 .I screen
 to turn on automatic output logging for the windows. By default, logfile's name
-is screenlog.1. You can sets new name: add it right after -L option e.g. 
"screen
--L my_logfile".
+is screenlog.0. You can set new name: add it right after -L option e.g. "screen
+-L my_logfile". Keep in mind that name can not start with "-" symbol.
 .TP 5
 .B \-m
 causes
--- a/doc/screen.texinfo
+++ b/doc/screen.texinfo
@@ -334,7 +334,9 @@
 
 @item -L
 Tell @code{screen} to turn on automatic output logging for the
-windows.
+windows. By default, logfile's name is screenlog.0. You can set new name:
+add it right after -L option e.g. "screen -L my_logfile". Keep in mind
+that name can not start with "-" symbol.
 
 @item -m
 Tell @code{screen} to ignore the @code{$STY} environment variable.  When
--- a/screen.c
+++ b/screen.c
@@ -669,8 +669,11 @@
   case 'L':
 if (--ac != 0) {
   screenlogfile = SaveStr(*++av);
-  if (screenlogfile[0] == '-')
-Panic(0, "-L: logfile name can not start with \"-\" symbol");
+  if (screenlogfile[0] == '-') {
+screenlogfile = SaveStr("screenlog.%n");
+av--;
+ac++;
+  }
   if (strlen(screenlogfile) > PATH_MAX)
 Panic(0, "-L: logfile name too long. (max. %d char)", 
PATH_MAX);
 }

You can also checkout the whole source package with that patch from
the branch "stretch-planned" in the packaging git repo:

https://anonscm.debian.org/cgit/collab-maint/screen.git/log/?h=stretch-planned

Regards, Axel
-- 
 ,''`.  |  Axel Beckert , http://people.debian.org/~abe/
: :' :  |  Debian Developer, ftp.ch.debian.org Admin
`. `'   |  4096R: 2517 B724 C5F6 CA99 5329  6E61 2FF9 CD59 6126 16B5
  `-|  1024D: F067 EA27 26B9 C3FC 1486  202E C09E 1D89 9593 0EDE


signature.asc
Description: Digital signature

Bug#863095: screen: syntax changed for -L argument, effectively an API change :/

2017-05-22 Thread Axel Beckert 
Control: found -1 4.5.0-1
Control: fixed -1 4.5.1-1

Hi Daniel,

Axel Beckert wrote:
> Control: tag -1 + upstream fixed-upstream confirmed
> > but if you have any pre-existing code that does something like:
> > 
> > screen -L -c foo.screenrc
> > 
> > then it fails in 4.5.0 because it doesn't like -c as an argument to
> > -L.
> 
> Can you check if this is fixed in screen 4.5.1-3 from Debian
> Experimental? Because according to the upstream bug report
> http://savannah.gnu.org/bugs/?50440 this should be fixed in 4.5.1.

I just checked with that example and an empty foo.screenrc and I can
confirm that this is fixed in Debian Experimental.

> If so, I'll try to cherry-pick the according upstream commits. Seem to
> be these commits:
> 
> Code change:
> http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4=18193bc7b237d66a076fffa21d4308dbb4f83cc5

Unfortunately this commit doesn't apply to 4.5.0. But I noticed
there's an earlier and much smaller commit which explicitly targets the
API regression:

http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4=c14e05e7c36c64d85198ed0fc89177427ece48d4

I'll check if that also suffice. I'd though be happy if you could have
a look over this commit and tell me if you also think that this commit
is already sufficient.

Regards, Axel
-- 
 ,''`.  |  Axel Beckert , http://people.debian.org/~abe/
: :' :  |  Debian Developer, ftp.ch.debian.org Admin
`. `'   |  4096R: 2517 B724 C5F6 CA99 5329  6E61 2FF9 CD59 6126 16B5
  `-|  1024D: F067 EA27 26B9 C3FC 1486  202E C09E 1D89 9593 0EDE

Bug#863095: screen: syntax changed for -L argument, effectively an API change :/

2017-05-21 Thread Axel Beckert 
Control: tag -1 + upstream fixed-upstream confirmed
Control: forwarded -1 http://savannah.gnu.org/bugs/?50440
Control: severity -1 important

Hi Daniel,

thanks for the bug report.

Daniel Kahn Gillmor wrote:
> before screen 4.5.0, -L took no arguments.
> 
> in 4.5.0 and later, -L takes one argument, the name of the logfile.
> 
> but if you have any pre-existing code that does something like:
> 
> screen -L -c foo.screenrc
> 
> then it fails in 4.5.0 because it doesn't like -c as an argument to
> -L.
> 
> this is effectively an API breakage, which makes existing tools that
> embed screen break.  This has an effect on kvm-manager, for instance.
> 
> https://0xacab.org/dkg/kvm-manager
> 
> it makes it so that kvm-manager can't reuse the same invocation on
> stretch and pre-stretch systems.

Can you check if this is fixed in screen 4.5.1-3 from Debian
Experimental? Because according to the upstream bug report
http://savannah.gnu.org/bugs/?50440 this should be fixed in 4.5.1.

If so, I'll try to cherry-pick the according upstream commits. Seem to
be these commits:

Code change:
http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4=18193bc7b237d66a076fffa21d4308dbb4f83cc5

Documentation updates:
http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4=13183da34eaf9efb3c6b47371c08ff786eaf712b
 
http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4=a38de4e6627f01cad43cff57490fd80a988b8e18
http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4=74c5883c47d84154e322dc18adbafbe7af5795b3

Regards, Axel
-- 
 ,''`.  |  Axel Beckert , http://people.debian.org/~abe/
: :' :  |  Debian Developer, ftp.ch.debian.org Admin
`. `'   |  4096R: 2517 B724 C5F6 CA99 5329  6E61 2FF9 CD59 6126 16B5
  `-|  1024D: F067 EA27 26B9 C3FC 1486  202E C09E 1D89 9593 0EDE

Bug#863095: screen: syntax changed for -L argument, effectively an API change :/

2017-05-21 Thread Daniel Kahn Gillmor 
Package: screen
Version: 4.5.0-5
Severity: normal

before screen 4.5.0, -L took no arguments.

in 4.5.0 and later, -L takes one argument, the name of the logfile.

but if you have any pre-existing code that does something like:

screen -L -c foo.screenrc

then it fails in 4.5.0 because it doesn't like -c as an argument to
-L.

this is effectively an API breakage, which makes existing tools that
embed screen break.  This has an effect on kvm-manager, for instance.

https://0xacab.org/dkg/kvm-manager

it makes it so that kvm-manager can't reuse the same invocation on
stretch and pre-stretch systems.

  --dkg

-- System Information:
Debian Release: 9.0
  APT prefers testing-debug
  APT policy: (500, 'testing-debug'), (500, 'testing'), (200, 
'unstable-debug'), (200, 'unstable'), (1, 'experimental-debug'), (1, 
'experimental')
Architecture: amd64
 (x86_64)
Foreign Architectures: i386

Kernel: Linux 4.9.0-2-amd64 (SMP w/4 CPU cores)
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)

Versions of packages screen depends on:
ii  libc6  2.24-10
ii  libpam0g   1.1.8-3.5
ii  libtinfo5  6.0+20161126-1

screen recommends no packages.

Versions of packages screen suggests:
pn  byobu | screenie | iselect  
ii  ncurses-term6.0+20161126-1

-- no debconf information