Re: T070 tests portability

[J Farkas]

On 2016-01-05 at 17:41:06, David Bremner wrote:
> Tomi Ollila  writes:
> 
> > The page https://sourceware.org/gdb/onlinedocs/gdb/Returning.html
> > talks something about gdb not knowing the return type if function
> > was compiled without debug info... well, is this is the reason,
> > perhaps we should allow testing w/o debug info compiled in.
> 
> The test suite overall assumes debug info is present; T000-basic tests
> for this.

I'm not missing the debug infos (well, except for the thread library if I
understand correctly - and that affects a lot of other tests, but not this).
Perhaps it the version of gdb, but not any compilation flags that can be
changed for the notmuch tree.

Without the changes, I'm getting these two kind of errors (and as you can see,
the test for the debugging symbols passes too).

$ ./T000-basic.sh

T000-basic: Testing the test framework itself.
 PASS   success is reported like this
...
 PASS   PATH is set to build directory
 PASS   notmuch is compiled with debugging symbols
$ ./T070-insert.sh

T070-insert: Testing "notmuch insert"
 PASS   Insert zero-length file
...
 FAIL   Insert duplicate message
--- T070-insert.6.expected  2016-01-06 13:31:01.644676102 +
+++ T070-insert.6.output2016-01-06 13:31:01.648675891 +
@@ -1 +1 @@
-2
+  2
 PASS   Duplicate message does not change tags
 PASS   Insert message, add tag
...
 PASS   Tags starting with '-' in new.tags are forbidden
 PASS   Invalid tags set exit code
 FAIL   error exit when add_message returns OUT_OF_MEMORY
--- T070-insert.26.expected 2016-01-06 13:31:06.940397013 +
+++ T070-insert.26.output   2016-01-06 13:31:06.940397013 +
@@ -1 +1 @@
-1
+255
warning: Unable to find libthread_db matching inferior's thread library, thread 
debugging will not be available.
index-file-OUT_OF_MEMORY.gdb:7: Error in sourced command file:
Return value type not available for selected stack frame.
Please use an explicit cast of the value to return.
 FAIL   success exit with --keep when add_message returns OUT_OF_MEMORY
...

(The above is then repeated for all gdb using `insert` tests.)

János

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

T070 tests portability

[J Farkas]

I'm in the process of writing insert tests, but it looks like my
environment is somewhat older than what the current tests are running
on.  The following two trivial changes for the original tests make those
pass cleanly:

The wc I have from GNU textutils 2.0.22 seems to produce extra
whitespace that needs to be cleaned:

diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index e7ec6a6..5864b9b 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -62,3 +62,3 @@ test_begin_subtest "Insert duplicate message"
 notmuch insert +duptag -unread < "$gen_msg_filename"
-output=$(notmuch search --output=files "subject:insert-subject" | wc -l)
+output=$(notmuch search --output=files "subject:insert-subject" | echo $(wc 
-l))
 test_expect_equal "$output" 2

And without the following cast, gdb 7.4 complains about the return type.

index-file-XAPIAN_EXCEPTION.gdb:7: Error in sourced command file:
Return value type not available for selected stack frame.
Please use an explicit cast of the value to return.

diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index e7ec6a6..5864b9b 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -196,3 +196,3 @@ break notmuch_database_add_message
 commands
-return NOTMUCH_STATUS_$code
+return (int)NOTMUCH_STATUS_$code
 continue

Does any of the above look reasonable to reduce the false positives?
With the above, the T070 tests all pass on my system.

Janos

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] cli/insert: do not lose the SMTP envelope

[J Farkas]

On 2016-01-03 at 18:04:39, Jani Nikula wrote:
> On Fri, 01 Jan 2016, J Farkas <jf.hyqohaczlksw4tx...@l2015aftruuq.dns007.net> 
> wrote:
> > From: Janos Farkas <chexum+...@gmail.com>
> > Subject: [PATCH] cli/insert: do not lose the SMTP envelope
> >
> > Make sure we store the envelope sender/recipient if provided by
> > qmail-command(8) in $RPLINE and $DTLINE.
> > ---
> >
> > I just realised that the messages delivered directly into maildir don't have
> > the usual envelope addresses that qmail provides.  This is a piece of
> > information that's important to (at least my) troubleshooting, so I created 
> > a
> > patch that seems to work well, applies cleanly to master (and 0.21), and
> > provided a NEWS entry should it be necessary.
> 
> I'd be more interested in seeing some tests for this...

I was thinking of it, and it could be simply an assurance that the
functionality stays there after changes too.  To be honest, the only
reason I didn't because the test suite is not passing in my environment,
either because of some gdb peculiarity, or some differences with emacs.

Answering your comment about Mallory here -- the DTLINE and RPLINE are
practically qmail's way of splitting the first two headers that *should*
be written in the message, they can only be affected by someone who
is actually saying he wants to deliver a message, not for any other
deliveries.

All the data that is supplied this way to the MDA by qmail (or the local
MTA), is still something that was accepted during the SMTP conversation
and passed basic checks, for a locally acceptable recipient, and after
any possible blocking on the sender.

It's just done in this way because:

- DTLINE is the Delivered-To line and qmail at this point is not sure
  the file will be "delivered", or processed in some other way, only the
  delivering program can actually tell what recipient will it be
  delivered to.  qmail uses this series of headers for loop avoidance,
  so it's essential that all the checkpoints are present.

- RPLINE is the Return-Path header that should be the *first* header in
  the file; if it would become part of the stdio, now all delivering and
  non-delivering programs would have to parse, detach it, and reattach
  to the front after any headers added.

It's basically a way to keep the SMTP conversation details alive along
the delivery pipeline.  Throwing it away is incorrect.  If this is not
ending up in the message, all I lose is the SMTP envelope, that can tell
me what entity was directly responsible to pass this message to my SMTP
server - was it sent by a mailing list, or if it's a direct message.
Or, in some cases, *which* mailing list.  It's a much safer way than
parsing through the Received lines.

Feel free to let me know if it needs further clarification why it is to
be done this way.

> >  NEWS |  9 +
> >  notmuch-insert.c | 28 
> >  2 files changed, 37 insertions(+)
> >
> > diff --git a/NEWS b/NEWS
> > index 6681699..13d45c8 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -1,3 +1,12 @@
> > +
> > +
> > +`notmuch insert` records the envelope addresses if available
> > +
> > +  If the caller provides this information as qmail-command(8) does in
> > +  the RPLINE and DTLINE environment variables, then notmuch insert will
> > +  record it in the maildir file.
> 
> We usually refer to message files. Perhaps you should also mention what
> the RPLINE and DTLINE variables should contain.

I don't think it's worthy for a NEWS entry with an explanation for those
- perhaps you meant in the commit or comments?

> > +
> >  Notmuch 0.21 (2015-10-29)
> >  =
> >  
> > diff --git a/notmuch-insert.c b/notmuch-insert.c
> > index 5205c17..ecc0fa0 100644
> > --- a/notmuch-insert.c
> > +++ b/notmuch-insert.c
> > @@ -284,6 +284,26 @@ copy_fd (int fdout, int fdin)
> >  }
> >  
> >  /*
> > + * Write zero (and LF) terminated string to the output fd.  It's expected 
> > to
> > + * come from getenv(), so it's not checked for correctness.  NULL or empty
> > + * string is ignored, successfully.
> > + * Return TRUE on success, FALSE on errors.
> > + */
> > +static notmuch_bool_t
> > +write_header (int fdout, const char *hdr)
> > +{
> > +ssize_t written,to_write;
> > +
> > +if (hdr && (to_write = strlen (hdr))) {
> > +written = write (fdout, hdr, to_write);
> > +   if (written != to_write)
> > +   return FALSE;
> > +}
> 
> It's not an error for write() to return prematurely with written <
> to_write. Please see the write(2) man page and the copy_fd()
> i

Re: cli/insert: do not lose the SMTP envelope

[J Farkas]

On 2016-01-02 at 13:28:02, Tomi Ollila wrote:
> On Fri, Jan 01 2016, J Farkas <jf.hyqohaczlksw4tx...@l2015aftruuq.dns007.net> 
> wrote:
> > Make sure we store the envelope sender/recipient if provided by
> > qmail-command(8) in $RPLINE and $DTLINE.
> > ---
> 
> Probably good feature, but like
> http://www.qmail.org/man/man8/qmail-command.html 
> says:
> 
>   qmail-local supplies several useful environment variables to
>   command.  WARNING: These environment variables are not
>   quoted.  They may contain special characters.  They are
>   under the control of a possibly malicious remote user.
> 
> Should we check that the contents of RPLINE and DTLINE are well-formed
> before writing these to the mail files ?

Thank you for reviewing and being so careful!

That warning is not applicable for the *LINE variables which are
supposed to end up in the message without further munging (they even
have the LF appended already).

The extra carefulness is only relevant for anyone trying to *parse*
those strings, like $EXT via unsafe languages, when EXT becomes the
part following the dash after the username (considering 
bgates-(){:;};shutd...@example.org for example)

It still should be what the envelope sender was, and what was considered
valid at the time.

I actually checked if there's any relevance for this warning: most
maildir delivering program does it already in one form or the other; in
fact, there is a command in the qmail distribution:
http://www.qmail.org/man/man1/preline.html which does the exact same
getenv and copy to the output.

If you'd liek to confirm, there's one repo for what seems to be the
original qmail source for this file shows even DJB does it the same way:

https://github.com/c-rack/qmail/blob/master/preline.c

I would think it's not worth the extra fork and pipe for this.  I don't
see how anyone could do without these headers saved, to be honest :)

Janos

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] cli/insert: do not lose the SMTP envelope

[J Farkas]

From: Janos Farkas 
Subject: [PATCH] cli/insert: do not lose the SMTP envelope

Make sure we store the envelope sender/recipient if provided by
qmail-command(8) in $RPLINE and $DTLINE.
---

I just realised that the messages delivered directly into maildir don't have
the usual envelope addresses that qmail provides.  This is a piece of
information that's important to (at least my) troubleshooting, so I created a
patch that seems to work well, applies cleanly to master (and 0.21), and
provided a NEWS entry should it be necessary.

 NEWS |  9 +
 notmuch-insert.c | 28 
 2 files changed, 37 insertions(+)

diff --git a/NEWS b/NEWS
index 6681699..13d45c8 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,12 @@
+
+
+`notmuch insert` records the envelope addresses if available
+
+  If the caller provides this information as qmail-command(8) does in
+  the RPLINE and DTLINE environment variables, then notmuch insert will
+  record it in the maildir file.
+
+
 Notmuch 0.21 (2015-10-29)
 =
 
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 5205c17..ecc0fa0 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -284,6 +284,26 @@ copy_fd (int fdout, int fdin)
 }
 
 /*
+ * Write zero (and LF) terminated string to the output fd.  It's expected to
+ * come from getenv(), so it's not checked for correctness.  NULL or empty
+ * string is ignored, successfully.
+ * Return TRUE on success, FALSE on errors.
+ */
+static notmuch_bool_t
+write_header (int fdout, const char *hdr)
+{
+ssize_t written,to_write;
+
+if (hdr && (to_write = strlen (hdr))) {
+written = write (fdout, hdr, to_write);
+   if (written != to_write)
+   return FALSE;
+}
+
+return TRUE;
+}
+
+/*
  * Write fdin to a new temp file in maildir/tmp, return full path to
  * the file, or NULL on errors.
  */
@@ -297,6 +317,14 @@ maildir_write_tmp (const void *ctx, int fdin, const char 
*maildir)
 if (fdout < 0)
return NULL;
 
+/* maildir(5) suggests the message should start with a Return-Path
+ * and Delivered-To lines.  qmail-local(8) supplies these.
+ */
+if (! write_header(fdout, getenv("RPLINE")))
+   goto FAIL;
+if (! write_header(fdout, getenv("DTLINE")))
+   goto FAIL;
+
 if (! copy_fd (fdout, fdin))
goto FAIL;
 
-- 
2.6.3

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch