[PATCH v2 0/2] Make NNTP Xrefs work better

[PATCH v2 0/2] Make NNTP Xrefs work better

[Jonathan Corbet]

I finally found some time to figure out why the Xref changes weren't having
the desired effect for me; the result is the following two changes.

As always, these patches are likely to reveal that I really haven't done
any perl in decades and that I'm still kind of groping my way around the
public-inbox source.  Hopefully they aren't too bad...

Version 2 makes the tests work and improves the setting of the server name.


Jonathan Corbet (2):
  Put the NNTP server name into Xref lines
  Add Xrefs to over/xover lines

 lib/PublicInbox/NNTP.pm  | 17 +++++++++--------
 lib/PublicInbox/NNTPD.pm |  6 ++++++
 2 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH 1/2] Put the NNTP server name into Xref lines

[Jonathan Corbet]

RFC 5536 sec 3.2.14 says that the server-name in an Xref line is "which
news server generated the header field"; indeed, that is necessary for
newsreaders like gnus to handle references properly.  So pick up the server
name from the config if available (the first name if there's more than
one), from the host name otherwise, and use it rather than the domain
name of the list server.

Tests have been adjusted to match the new behavior.
---
 lib/PublicInbox/NNTP.pm  |  4 ++--
 lib/PublicInbox/NNTPD.pm | 10 ++++++++++
 t/nntp.t                 |  3 ++-
 t/nntpd.t                |  9 ++++++---
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index cdbd8e9..cbd4ecf 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -94,7 +94,7 @@ sub new ($$$) {
 	my $self = fields::new($class);
 	$self->SUPER::new($sock);
 	$self->{nntpd} = $nntpd;
-	res($self, '201 server ready - post via email');
+	res($self, '201 ' . $nntpd->{servername} . ' ready - post via email');
 	$self->{rbuf} = '';
 	$self->watch_read(1);
 	update_idle_time($self);
@@ -410,7 +410,7 @@ sub header_append ($$$) {
 
 sub xref ($$$$) {
 	my ($self, $ng, $n, $mid) = @_;
-	my $ret = "$ng->{domain} $ng->{newsgroup}:$n";
+	my $ret = $self->{nntpd}->{servername} . " $ng->{newsgroup}:$n";
 
 	# num_for is pretty cheap and sometimes we'll lookup the existence
 	# of an article without getting even the OVER info.  In other words,
diff --git a/lib/PublicInbox/NNTPD.pm b/lib/PublicInbox/NNTPD.pm
index 117c9c0..32848d7 100644
--- a/lib/PublicInbox/NNTPD.pm
+++ b/lib/PublicInbox/NNTPD.pm
@@ -6,15 +6,25 @@
 package PublicInbox::NNTPD;
 use strict;
 use warnings;
+use Sys::Hostname;
 require PublicInbox::Config;
 
 sub new {
 	my ($class) = @_;
+	my $pi_config = PublicInbox::Config->new;
+	my $name = $pi_config->{'publicinbox.nntpserver'};
+	if (!defined($name) or $name eq '') {
+		$name = hostname;
+	} elsif (ref($name) eq 'ARRAY') {
+		$name = $name->[0];
+	}
+
 	bless {
 		groups => {},
 		err => \*STDERR,
 		out => \*STDOUT,
 		grouplist => [],
+		servername => $name,
 	}, $class;
 }
 
diff --git a/t/nntp.t b/t/nntp.t
index 57fef48..6df7db8 100644
--- a/t/nntp.t
+++ b/t/nntp.t
@@ -110,7 +110,8 @@ use_ok 'PublicInbox::Inbox';
 	my $mid = 'a@b';
 	my $mime = Email::MIME->new("Message-ID: <$mid>\r\n\r\n");
 	my $hdr = $mime->header_obj;
-	my $mock_self = { nntpd => { grouplist => [] } };
+	my $mock_self = { nntpd => { grouplist => [], 
+				     servername => 'example.com' } };
 	PublicInbox::NNTP::set_nntp_headers($mock_self, $hdr, $ng, 1, $mid);
 	is_deeply([ $mime->header('Message-ID') ], [ "<$mid>" ],
 		'Message-ID unchanged');
diff --git a/t/nntpd.t b/t/nntpd.t
index 960e83c..f859908 100644
--- a/t/nntpd.t
+++ b/t/nntpd.t
@@ -16,6 +16,7 @@ use Fcntl qw(FD_CLOEXEC F_SETFD F_GETFD);
 use Socket qw(SO_KEEPALIVE IPPROTO_TCP TCP_NODELAY);
 use File::Temp qw/tempdir/;
 use Net::NNTP;
+use Sys::Hostname;
 
 my $tmpdir = tempdir('pi-nntpd-XXXXXX', TMPDIR => 1, CLEANUP => 1);
 my $home = "$tmpdir/pi-home";
@@ -140,13 +141,14 @@ EOF
 		'from' => "El\xc3\xa9anor <me\@example.com>",
 		'to' => "El\xc3\xa9anor <you\@example.com>",
 		'cc' => $addr,
-		'xref' => "example.com $group:1",
+		'xref' => hostname . " $group:1",
 		'references' => '<reftabsqueezed>',
 	);
 
 	my $s = IO::Socket::INET->new(%opts);
 	sysread($s, my $buf, 4096);
-	is($buf, "201 server ready - post via email\r\n", 'got greeting');
+	is($buf, "201 " . hostname . " ready - post via email\r\n",
+		'got greeting');
 	$s->autoflush(1);
 
 	ok(syswrite($s, "   \r\n"), 'wrote spaces');
@@ -156,7 +158,8 @@ EOF
 
 	$s = IO::Socket::INET->new(%opts);
 	sysread($s, $buf, 4096);
-	is($buf, "201 server ready - post via email\r\n", 'got greeting');
+	is($buf, "201 " . hostname . " ready - post via email\r\n",
+		'got greeting');
 	$s->autoflush(1);
 
 	syswrite($s, "NEWGROUPS 19990424 000000 GMT\r\n");
-- 
2.17.2

[PATCH 2/2] Add Xrefs to over/xover lines

[Jonathan Corbet]

Putting the Xref field into xover lines allows newsreaders to mark
cross-posted messages read when catching up a group.  That, in turn,
massively improves the life of crazy people who try to follow dozens of
kernel lists, where emails are often heavily cross-posted.
---
 lib/PublicInbox/NNTP.pm | 13 +++++++------
 t/nntpd.t               | 11 ++++++++---
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index cbd4ecf..022bb80 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -28,7 +28,7 @@ use constant {
 
 sub now () { clock_gettime(CLOCK_MONOTONIC) };
 
-my @OVERVIEW = qw(Subject From Date Message-ID References);
+my @OVERVIEW = qw(Subject From Date Message-ID References Xref);
 my $OVERVIEW_FMT = join(":\r\n", @OVERVIEW, qw(Bytes Lines)) . ":\r\n";
 my $LIST_HEADERS = join("\r\n", @OVERVIEW,
 			qw(:bytes :lines Xref To Cc)) . "\r\n";
@@ -812,8 +812,8 @@ sub cmd_xrover ($;$) {
 	});
 }
 
-sub over_line ($$) {
-	my ($num, $smsg) = @_;
+sub over_line ($$$$) {
+	my ($self, $ng, $num, $smsg) = @_;
 	# n.b. field access and procedural calls can be
 	# 10%-15% faster than OO method calls:
 	my $s = join("\t", $num,
@@ -823,7 +823,8 @@ sub over_line ($$) {
 		"<$smsg->{mid}>",
 		$smsg->{references},
 		$smsg->{bytes},
-		$smsg->{lines});
+		$smsg->{lines},
+		"Xref: " . xref($self, $ng, $num, $smsg->{mid}));
 	utf8::encode($s);
 	$s
 }
@@ -839,7 +840,7 @@ sub cmd_over ($;$) {
 		# Only set article number column if it's the current group
 		my $self_ng = $self->{ng};
 		$n = 0 if (!$self_ng || $self_ng ne $ng);
-		more($self, over_line($n, $smsg));
+		more($self, over_line($self, $ng, $n, $smsg));
 		'.';
 	} else {
 		cmd_xover($self, $range);
@@ -861,7 +862,7 @@ sub cmd_xover ($;$) {
 
 		# OVERVIEW.FMT
 		more($self, join("\r\n", map {
-			over_line($_->{num}, $_);
+			over_line($self, $self->{ng}, $_->{num}, $_);
 			} @$msgs));
 		$cur = $msgs->[-1]->{num} + 1;
 	});
diff --git a/t/nntpd.t b/t/nntpd.t
index f859908..9c1d076 100644
--- a/t/nntpd.t
+++ b/t/nntpd.t
@@ -196,7 +196,9 @@ EOF
 			'<nntp@example.com>',
 			'<reftabsqueezed>',
 			$len,
-			'1' ] }, "XOVER range works");
+			'1',
+			'Xref: '. hostname . ' test-nntpd:1'] },
+		"XOVER range works");
 
 	is_deeply($n->xover('1'), {
 		'1' => ["Testing for El\xc3\xa9anor",
@@ -205,7 +207,9 @@ EOF
 			'<nntp@example.com>',
 			'<reftabsqueezed>',
 			$len,
-			'1' ] }, "XOVER by article works");
+			'1',
+			'Xref: '. hostname . ' test-nntpd:1'] },
+		"XOVER by article works");
 
 	is_deeply($n->head(1), $n->head('<nntp@example.com>'), 'HEAD OK');
 	is_deeply($n->body(1), $n->body('<nntp@example.com>'), 'BODY OK');
@@ -225,7 +229,8 @@ EOF
 		is($r[1], "0\tTesting for El\xc3\xa9anor\t" .
 			"El\xc3\xa9anor <me\@example.com>\t" .
 			"Thu, 01 Jan 1970 06:06:06 +0000\t" .
-			"$mid\t<reftabsqueezed>\t$len\t1",
+			"$mid\t<reftabsqueezed>\t$len\t1" .
+			"\tXref: " . hostname . " test-nntpd:0",
 			'OVER by Message-ID works');
 		is($r[2], '.', 'correctly terminated response');
 	}
-- 
2.17.2

Re: [PATCH v2 0/2] Make NNTP Xrefs work better

[Eric Wong]

Jonathan Corbet <corbet@lwn.net> wrote:
> I finally found some time to figure out why the Xref changes weren't having
> the desired effect for me; the result is the following two changes.
> 
> As always, these patches are likely to reveal that I really haven't done
> any perl in decades and that I'm still kind of groping my way around the
> public-inbox source.  Hopefully they aren't too bad...

No worries, Perl (or anything else) feels rusty to me after
only a few days/weeks away :x

> Version 2 makes the tests work and improves the setting of the server name.

Thanks!  Applied and pushed to https://public-inbox.org/ as
commit fbfd63996ddfc9a23b8a1e0f8756e378f2e196ba

> Jonathan Corbet (2):
>   Put the NNTP server name into Xref lines
>   Add Xrefs to over/xover lines

I was concerned about exposing the NNTP server hostname for
people running Tor onions, but having "publicinbox.nntpserver"
already-configured on my mirror onions means no changes were
needed on my end.  Other onion operators (if they exist) may
need to take notice.

I haven't deployed to news.public-inbox.org / ou63pmih66umazou.onion, yet;
but both nntp://hjrcffqmbrq6wope.onion/ and nntp://czquwvybam4bgbro.onion/
are running the new code.