[9front] [PATCH] upas: allow send to parse e-mail descriptions like marshal

[9front] [PATCH] upas: allow send to parse e-mail descriptions like marshal

[Romano]

	upas/marshal/marshal.c:/^printfrom parses an e-mail with a description
	(e.g., "A Name <a.name@example.com>") and sets the from to just the
	e-mail address portion. This does the same for upas/send so that
	upasname='A name <a.name@example.com>' can be used to both set the From:
	in marshal with a description and to match the correct from in send
	for sending via smtp.
---
diff a327175a3c01d18e3e4c061ce4579cc420ee3561 daebe6f5e2a6df18729e08319c0ab70aad8e1666
--- a/sys/src/cmd/upas/send/message.c
+++ b/sys/src/cmd/upas/send/message.c
@@ -1,6 +1,7 @@
 #include "common.h"
 #include "send.h"
 #include <regexp.h>
+#include <ctype.h>
 #include "../smtp/smtp.h"
 #include "../smtp/rfc822.tab.h"
 
@@ -18,6 +19,24 @@
 static String*	getstring(Node *p);
 static String*	getaddr(Node *p);
 
+char *
+userfrom(char *cp)
+{
+	char *s, *r;
+	int n;
+
+	if((n = strlen(cp)) > 4 && cp[n-1] == '>'){
+		if((s = strrchr(cp, '<')) != nil && s != cp && isspace(s[-1])) {
+			s++;
+			strncpy(r, s, n - sizeof s);
+			r[strlen(s)-1] = '\0';
+			return r;
+		}
+	}
+
+	return cp;
+}
+
 int
 default_from(message *mp)
 {
@@ -32,7 +51,7 @@
 		return -1;
 	}
 	if(cp && *cp)
-		s_append(mp->sender, cp);
+		s_append(mp->sender, userfrom(cp));
 	else
 		s_append(mp->sender, lp);
 	free(cp);

Re: [9front] [PATCH] upas: allow send to parse e-mail descriptions like marshal

[Romano]

Looking at my initial patch, I don't think *r is needed at all:
*cp can just be used. Attached is an updated patch, which I submit
for consideration.

For a bit more background, this patch is ideal for someone who is using
upas with different smtp providers, or using the typical '+'/'-' format
to the localpart for categorization. For instance, in /mail/lib/fromfiles,
a file can be specified listing an email address and smtp server separated
by whitespace. /mail/lib/remotemail can then be modified to account
for the output of aliasmail and then connect the the appropriate smtp
server:

	cpu% cat /mail/lib/remotemail 
	#!/bin/rc
	shift
	sender=$1
	shift
	addr=$1
	shift
	fd=`{/bin/upas/aliasmail -f $sender}
	switch($fd){
	case smtp.server1.com
		addr=(-a -t -u username1 tcp!$fd!ssmtp)
	case smtp.server2.com
		addr=(-a -t -u username tcp!$fd!ssmtp)
	case *.*
		;
	case *
		fd=your.domain
		# for now, just fail at this point
	    echo `{date} 'no host found in remotemail! sender='^$sender >> /sys/log/remotemail
	    exit `{date} 'no host found in remotemail!'
	}
	exec /bin/upas/smtp -h $fd $addr $sender $*

As explained below, upas/marshal already gets the right sender
when given an e-mail description defined in $upasname, so this is
making it so that further parsing and processing doesn't need to be
done by remotemail.

To me, this makes sense, but is this a bad idea? Is there a reason
why upas/marshal would take the pains to extract the e-mail from
$upasname correctly but not upas/send?

From: Romano <unobe@cpan.org>
Date: Wed, 15 May 2024 09:34:51 +0000
Subject: [PATCH] upas: allow send to parse e-mail descriptions like marshal


	upas/marshal/marshal.c:/^printfrom parses an e-mail with a description
	(e.g., "A Name <a.name@example.com>") and sets the from to just the
	e-mail address portion. This does the same for upas/send so that
	upasname='A name <a.name@example.com>' can be used to both set the From:
	in marshal with a description and to match the correct from in send
	for sending via smtp.
---
diff e51d4aa069548de51d0e88a6d621d278e9138cd0 7f8780f7b92fbbc37704d4d48ccd15ab1b1e5a27
--- a/sys/src/cmd/upas/send/message.c
+++ b/sys/src/cmd/upas/send/message.c
@@ -1,6 +1,7 @@
 #include "common.h"
 #include "send.h"
 #include <regexp.h>
+#include <ctype.h>
 #include "../smtp/smtp.h"
 #include "../smtp/rfc822.tab.h"
 
@@ -18,6 +19,24 @@
 static String*	getstring(Node *p);
 static String*	getaddr(Node *p);
 
+char *
+userfrom(char *cp)
+{
+	char *s;
+	int n;
+
+	if((n = strlen(cp)) > 4 && cp[n-1] == '>'){
+		if((s = strrchr(cp, '<')) != nil && s != cp && isspace(s[-1])) {
+			s++;
+			strncpy(cp, s, n - sizeof s);
+			cp[strlen(s)-1] = '\0';
+			return cp;
+		}
+	}
+
+	return cp;
+}
+
 int
 default_from(message *mp)
 {
@@ -32,7 +51,7 @@
 		return -1;
 	}
 	if(cp && *cp)
-		s_append(mp->sender, cp);
+		s_append(mp->sender, userfrom(cp));
 	else
 		s_append(mp->sender, lp);
 	free(cp);

Re: [9front] [PATCH] upas: allow send to parse e-mail descriptions like marshal

[Romano]

Crap, after taking some more time to test, I realized that both
patches were copying more than need be at times, and the second
patch actually truncated the address too much most of the time. I
RTFM for strncpy because I didn't get why the last patch I sent
was so faulty. I thought it might have been char-by-char copying
backwards or something in the implementation. Alas, no, it was
me not understanding what was happening with re: to the index
where I was setting the null byte.

This is the last patch I'll send re: this until I get some feedback
or until June. And my apologies for anyone who read the previous
iterations already.

From: Romano <me+git@fallglow.com>
Date: Fri, 17 May 2024 08:08:34 +0000
Subject: [PATCH] upas/send: parse e-mail descriptions like marshal


/sys/src/cmd/upas/marshal/marshal.c:/^printfrom parses an e-mail with a
description (e.g., "A Name <a.name@example.com>") and sets the from to
just the e-mail address portion. This does the same for upas/send so that
upasname='A name <a.name@example.com>' can be used to both set the From:
in marshal with a description and to match the correct from in upas/send
for sending via smtp.
---
diff e51d4aa069548de51d0e88a6d621d278e9138cd0 863b4ae53732ddeaf96dc23b0e9924651f2b4090
--- a/sys/src/cmd/upas/send/message.c
+++ b/sys/src/cmd/upas/send/message.c
@@ -1,6 +1,7 @@
 #include "common.h"
 #include "send.h"
 #include <regexp.h>
+#include <ctype.h>
 #include "../smtp/smtp.h"
 #include "../smtp/rfc822.tab.h"
 
@@ -18,6 +19,23 @@
 static String*	getstring(Node *p);
 static String*	getaddr(Node *p);
 
+char *
+userfrom(char *cp)
+{
+	char *s;
+	int n;
+
+	if((n = strlen(cp)) > 4 && cp[n-1] == '>'){
+		if((s = strrchr(cp, '<')) != nil && s != cp && isspace(s[-1])) {
+			s++;
+			cp[n-1] = '\0';
+			strcpy(cp, s);
+		}
+	}
+
+	return cp;
+}
+
 int
 default_from(message *mp)
 {
@@ -32,7 +50,7 @@
 		return -1;
 	}
 	if(cp && *cp)
-		s_append(mp->sender, cp);
+		s_append(mp->sender, userfrom(cp));
 	else
 		s_append(mp->sender, lp);
 	free(cp);

Re: [9front] [PATCH] upas: allow send to parse e-mail descriptions like marshal

[Romano]

Ping. Here's a cleaned up patch:
http://okturing.com/src/20104/body

Similarly with the ramfs patch, any comments/concerns?

Re: [9front] [PATCH] upas: allow send to parse e-mail descriptions like marshal

[Jacob Moody]

On 6/18/24 13:05, Romano wrote:
> Ping. Here's a cleaned up patch:
> http://okturing.com/src/20104/body
> 
> Similarly with the ramfs patch, any comments/concerns?

The general idea seems fine I think.
I don't use upas, but I did find some
issues with the implementation purposed:

+char *
Typical style is to make the return type 'char*' here

+userfrom(char *cp)
+{
+	char *s;
+	int n;
+
+	if((n = strlen(cp)) > 4 && cp[n-1] == '>'){
+		if((s = strrchr(cp, '<')) != nil && s != cp && isspace(s[-1])) {
+			s++;
+			cp[n-1] = '\0';
+			strcpy(cp, s);

I don't love this loop, we're essentially walking the string three times here.
Once for the first strlen, again for strrchr, and a third time with the strcpy.
Because we return a pointer here we could get rid of this strcpy entirely and just
make this inner block two lines like so:
+			cp[n-1] = '\0';
+			return s + 1;

However at this point we'd still be walking the string twice. I wanted to see if
we could just walk the string once without having the code suck and this is what
I came up with:

char*
userfrom(char *cp)
{
	char *p, *e;

	if((p = strchr(cp, '<')) == nil)
		return cp;
	if(p == cp || !isspace(p[-1]))
		return cp;
	p++;
	if((e = strchr(p, '>')) == nil)
		return cp;
	*e = '\0';
	return p;
}

Not that the length is going to be super long here, but being a bit more efficient doesn't
make the code any more complicated (imo).

Also I believe the commit message is incorrect given it's understanding of what
printform is doing. I pulled the code out to play with it, and from what I see
when it is given a "A name <a@example.com>" it will print out the full thing,
not just the email. The point of this code seems to ensure that the "name" portion
of this description is passed through the rfc2047fmt function (installed as %U).

/sys/src/cmd/upas/marshal/marshal.c:820
Specifically prints both portions there, with the only difference from the default
case being that it passes the name through %U.

So I think the description of what is going on in the commit message is not quite
correct either.

Thanks,
moody

Re: [9front] [PATCH] upas: allow send to parse e-mail descriptions like marshal

[Romano]

On Wed Jun 19 15:55:59 -0700 2024, moody@posixcafe.org wrote:
> On 6/18/24 13:05, Romano wrote:
> > Ping. Here's a cleaned up patch:
> > http://okturing.com/src/20104/body
> > 
> > Similarly with the ramfs patch, any comments/concerns?
> 
> The general idea seems fine I think.
> I don't use upas, but I did find some
> issues with the implementation purposed:
> 
> +char *
> Typical style is to make the return type 'char*' here

Got it. So parameters and declarations look to have 'char *',
but return types remove the space.

> +userfrom(char *cp)
> +{
> +	char *s;
> +	int n;
> +
> +	if((n = strlen(cp)) > 4 && cp[n-1] == '>'){
> +		if((s = strrchr(cp, '<')) != nil && s != cp && isspace(s[-1])) {
> +			s++;
> +			cp[n-1] = '\0';
> +			strcpy(cp, s);
> 
> I don't love this loop, we're essentially walking the string three times here.
> Once for the first strlen, again for strrchr, and a third time with the strcpy.
> Because we return a pointer here we could get rid of this strcpy entirely and just
> make this inner block two lines like so:
> +			cp[n-1] = '\0';
> +			return s + 1;
> 
> However at this point we'd still be walking the string twice. I wanted to see if
> we could just walk the string once without having the code suck and this is what
> I came up with:
> 
> char*
> userfrom(char *cp)
> {
> 	char *p, *e;
> 
> 	if((p = strchr(cp, '<')) == nil)
> 		return cp;
> 	if(p == cp || !isspace(p[-1]))
> 		return cp;
> 	p++;
> 	if((e = strchr(p, '>')) == nil)
> 		return cp;
> 	*e = '\0';
> 	return p;
> }
> 
> Not that the length is going to be super long here, but being a bit more efficient doesn't
> make the code any more complicated (imo).

I like it--easier to follow. Admittedly, I had cargo-culted from the marshal code (you
link to below). I did that poorly.

> Also I believe the commit message is incorrect given it's understanding of what
> printform is doing. I pulled the code out to play with it, and from what I see
> when it is given a "A name <a@example.com>" it will print out the full thing,
> not just the email. The point of this code seems to ensure that the "name" portion
> of this description is passed through the rfc2047fmt function (installed as %U).
> 
> /sys/src/cmd/upas/marshal/marshal.c:820
> Specifically prints both portions there, with the only difference from the default
> case being that it passes the name through %U.
> 
> So I think the description of what is going on in the commit message is not quite
> correct either.

Yeah, my description is flat out wrong. Is this better?

 /sys/src/cmd/upas/marshal/marshal.c:/^printfrom parses $upasname (or
 the login name) as an e-mail with description (e.g., "A Name
 <a.name@example.com>") to set the From: line. This does similarly for
 upas/send, but only parses the e-mail portion so that
 upasname='A name <a.name@example.com>' can be used to both set the
 From: in marshal with a description and to match the correct from in
 upas/send for sending via smtp.

> Thanks,
> moody

Thank you for taking the time to give a better implementation (imo).