Re: patch unveil fail
On 2023/10/25 13:38:37 +0200, Alexander Bluhm wrote:
> Index: patch.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/usr.bin/patch/patch.c,v
> diff -u -p -r1.74 patch.c
> --- patch.c 19 Jul 2023 13:26:20 - 1.74
> +++ patch.c 24 Oct 2023 17:13:28 -
> @@ -32,6 +32,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -213,11 +214,27 @@ main(int argc, char *argv[])
> perror("unveil");
> my_exit(2);
> }
> - if (filearg[0] != NULL)
> + if (filearg[0] != NULL) {
> + char *origdir;
> +
> if (unveil(filearg[0], "rwc") == -1) {
> perror("unveil");
> my_exit(2);
> }
> + if ((origdir = dirname(filearg[0])) == NULL) {
Not sure if we're interested in it, but dirname(3) theoretically alter
the passed string. our dirname doesn't do it, but per posix it can,
IIUC. This could cause issues since filearg[0] is used later.
If we care about portability here, we should pass a copy to dirname.
don't know if we care thought.
> + perror("dirname");
> + my_exit(2);
> + }
> + if (unveil(origdir, "rwc") == -1) {
> + perror("unveil");
> + my_exit(2);
> + }
> + } else {
> + if (unveil(".", "rwc") == -1) {
> + perror("unveil");
> + my_exit(2);
> + }
> + }
> if (filearg[1] != NULL)
> if (unveil(filearg[1], "r") == -1) {
> perror("unveil");
Re: smtpd: implement nullmx RFC 7505
On 2023/10/18 08:40:14 +0100, Stuart Henderson wrote:
> On 2023/10/17 22:27, Philipp wrote:
> > [2023-10-17 17:32] Omar Polo
> > > There is one part of the RFC7505 that I'd like to quote and discuss
> > > with you however. The last paragraph of the section 3 says:
> > >
> > > : A domain that advertises a null MX MUST NOT advertise any other MX
> > > : RR.
> > >
> > > Your implementation handles the case where there are more than one MX
> > > by setting the `nullmx' field in the shared struct when we encounter
> > > one, and then don't process the other MXs when that field is set.
> > > This is fine.
> > >
> > > However, I think we can simplify here by not honouring the Null MX
> > > when there are other MX records. We're not violating the RFC. See
> > > diff below to see what I mean. The only difference is in dns.c, the
> > > rest is the same with yours.
> >
> > My reasaning for that was: When you set Null MX you explicitly opt out
> > from reciving mail even when you violate the spec. But if you want
> > it diffrent I can change it.
>
> I think in that situation, the recipient's DNS admin has *not* set a
> valid nullmx, and delivery to other MXes should still be attempted.
That's what I though too.
> > But I don't think your proposed patch is a good solution, because the
> > result depend on the order of the RR in the repsonse. The problem is
> > when the first entry in the response is a Null MX your patch truncate
> > the other results and a bounce is generated. But when the Null MX is
> > later in the response the other entries are used to deliver the mail.
that's true; I fell for a shorter diff. I've attached below a version
that doesn't depend on the order.
> > > So far I've only compile-tested the code. I don't have a spare domain
> > > to test and don't know of anyone using a Null MX.
> >
> > To test Null MX you can use example.com. To test the localhost patch a
> > friend of me has set up mxtest.yannikenss.de.
thanks!
> [...]
> > [1] I still belive OpenSMTPD should only connect to public routable
> > addresses for relay. So don't connect to something like 127.0.0.1/8,
> > ::1/128, RFC1918 addresses, ULA, ...
> > But this is independent of this patch.
>
> Support for nullmx makes sense to me. Perhaps also the localhost
> handling. But some intranet setups may require connections to
> RFC1918/ULA addresses - I don't think an MTA should prevent these.
Completely agree.
diff 2d025d839f99dc09ee525c11a4ed09a0f3bbe7d0
02bb94351d3865e61483023cab9fa02bcac2970d
commit - 2d025d839f99dc09ee525c11a4ed09a0f3bbe7d0
commit + 02bb94351d3865e61483023cab9fa02bcac2970d
blob - 4cf5d23d1d14b5400c6f4429dae0a4f6490073d4
blob + 552a5cf9115401b4fac3d131d6e5c022ebb8b7fd
--- usr.sbin/smtpd/dns.c
+++ usr.sbin/smtpd/dns.c
@@ -232,10 +232,10 @@ dns_dispatch_mx(struct asr_result *ar, void *arg)
struct dns_rrrr;
char buf[512];
size_t found;
+ int nullmx = 0;
if (ar->ar_h_errno && ar->ar_h_errno != NO_DATA &&
ar->ar_h_errno != NOTIMP) {
-
m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
m_add_id(s->p, s->reqid);
if (ar->ar_rcode == NXDOMAIN)
@@ -259,13 +259,29 @@ dns_dispatch_mx(struct asr_result *ar, void *arg)
unpack_rr(, );
if (rr.rr_type != T_MX)
continue;
+
print_dname(rr.rr.mx.exchange, buf, sizeof(buf));
buf[strlen(buf) - 1] = '\0';
+
+ if (rr.rr.mx.preference == 0 && !strcmp(buf, "")) {
+ nullmx = 1;
+ continue;
+ }
+
dns_lookup_host(s, buf, rr.rr.mx.preference);
found++;
}
free(ar->ar_data);
+ if (nullmx && found == 0) {
+ m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
+ m_add_id(s->p, s->reqid);
+ m_add_int(s->p, DNS_NULLMX);
+ m_close(s->p);
+ free(s);
+ return;
+ }
+
/* fallback to host if no MX is found. */
if (found == 0)
dns_lookup_host(s, s->name, 0);
blob - c0d0fc02931b90409441035d2744923af9e42df1
blob + 25e158b68a88c8485428a2476c1c557f8c60404d
--- usr.sbin/smtpd/mta.c
+++ usr.sbin/smtpd/mta.c
@@ -1088,6 +1088,10 @@ mta_on_mx(void *tag, void *arg, void *data)
else
relay->failstr = "No MX found for domain";
break;
+ case DNS_NULLMX:
+
Re: smtpd: implement nullmx RFC 7505
Hello,
sorry for the terrifc delay.
On 2023/10/01 14:59:15 +0200, Philipp wrote:
> Hi
>
> Setting Null MX is a way for domainowners to indicate that the domain
> does not accept mail. Currently a Null MX causes a tempfail and the
> mail will be queued and tried to resubmitted till a timeout. With the
> attached patch a Null MX causes a permfail. This way the sender will
> directly get a bounce with the message "Domain does not accept mail".
I'm not sure how much widespread the usage is, but it doesn't seem a
bad feature. It's simple to implement and it provides some (very
small) value.
In general I'm OK with the idea, but would need some OKs from other
developers too.
There is one part of the RFC7505 that I'd like to quote and discuss
with you however. The last paragraph of the section 3 says:
: A domain that advertises a null MX MUST NOT advertise any other MX
: RR.
Your implementation handles the case where there are more than one MX
by setting the `nullmx' field in the shared struct when we encounter
one, and then don't process the other MXs when that field is set.
This is fine.
However, I think we can simplify here by not honouring the Null MX
when there are other MX records. We're not violating the RFC. See
diff below to see what I mean. The only difference is in dns.c, the
rest is the same with yours.
So far I've only compile-tested the code. I don't have a spare domain
to test and don't know of anyone using a Null MX.
> Because some domains set the MX record to "localhost." to get a similar
> efect the secound patch ignores "localhost." MX entries and handles a MX
> containing only "localhost." records like a Null MX.
This could be simplified too. On top of diff below, it would need
just something among the lines of:
- if (rr.rr.mx.preference == 0 && !strcmp(buf, "")) {
+ if ((rr.rr.mx.preference == 0 && !strcmp(buf, "")) ||
+ !strcasecmp(buf, "localhost")) {
if (found)
continue;
m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
I'm still not convinced about this part however. It feels wrong to me
to hardcode such a check, it seems too much paranoic. The follow-up
would be to check for localhost in dns_dispatch_host too? ;)
Thanks,
Omar Polo
diff 2d025d839f99dc09ee525c11a4ed09a0f3bbe7d0
8d6138e5b1e0bc112ff2584d8528e6bc95a39b6f
commit - 2d025d839f99dc09ee525c11a4ed09a0f3bbe7d0
commit + 8d6138e5b1e0bc112ff2584d8528e6bc95a39b6f
blob - 4cf5d23d1d14b5400c6f4429dae0a4f6490073d4
blob + effc07d142895fb2b622242efcd5c69da7f3b67c
--- usr.sbin/smtpd/dns.c
+++ usr.sbin/smtpd/dns.c
@@ -259,8 +259,22 @@ dns_dispatch_mx(struct asr_result *ar, void *arg)
unpack_rr(, );
if (rr.rr_type != T_MX)
continue;
+
print_dname(rr.rr.mx.exchange, buf, sizeof(buf));
buf[strlen(buf) - 1] = '\0';
+
+ if (rr.rr.mx.preference == 0 && !strcmp(buf, "")) {
+ if (found)
+ continue;
+ m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
+ m_add_id(s->p, s->reqid);
+ m_add_int(s->p, DNS_NULLMX);
+ m_close(s->p);
+ free(s);
+ found++;
+ break;
+ }
+
dns_lookup_host(s, buf, rr.rr.mx.preference);
found++;
}
blob - c0d0fc02931b90409441035d2744923af9e42df1
blob + 25e158b68a88c8485428a2476c1c557f8c60404d
--- usr.sbin/smtpd/mta.c
+++ usr.sbin/smtpd/mta.c
@@ -1088,6 +1088,10 @@ mta_on_mx(void *tag, void *arg, void *data)
else
relay->failstr = "No MX found for domain";
break;
+ case DNS_NULLMX:
+ relay->fail = IMSG_MTA_DELIVERY_PERMFAIL;
+ relay->failstr = "Domain does not accept mail";
+ break;
default:
fatalx("bad DNS lookup error code");
break;
blob - 6781286928da45e6159bce81ff2437011ebdef72
blob + 9f4732d1c27f6ef9f62951f0207f209a83038dcf
--- usr.sbin/smtpd/smtpd.h
+++ usr.sbin/smtpd/smtpd.h
@@ -1026,6 +1026,8 @@ enum dns_error {
DNS_EINVAL,
DNS_ENONAME,
DNS_ENOTFOUND,
+ /* rfc 7505 */
+ DNS_NULLMX,
};
enum lka_resp_status {
Re: Send international text with mail(1) - proposal and patches
Hello, Walter: I'm happy that you've been hacking on mail and at least in principle I think what you're doing makes sense; however, let's try to get one bit committed at a time. Let's start with the MIME needed for sending utf-8 messages. I've going through the various mail and I think it's here where things started to go off the rails. Ingo provided some valuable feedback, I've updated your diff to address it. Other additions, such as doing checks on the content, adding other headers, etc... can be done as a follow-up after this goes in IMHO. On 2023/09/20 17:30:08 +0200, Ingo Schwarze wrote: > Hi, > > i checked the following points: > > * Even though RFC 2049 section 2 bullet point 1 only *requires* >MIME-conformant MUAs to always write the header "MIME-Version: >1.0" - and mail(1) is most certainly not MIME-conformant - RFC 2049 >section 2 bullet point 8 explicitly *recommends* that even non-MIME >MUAs always set appropriate MIME headers. RFC 2046 section 4.1.2 >paragraph 8 also "strongly" recommends the explicit inclusion of a >"charset" parameter even for us-ascii. > >Consequently, i believe that when sending a message in US-ASCII, >mail(1) should include these headers: > >MIME-Version: 1.0 >Content-Transfer-Encoding: 7bit >Content-Type: text/plain; charset=us-ascii > > * Adding a "Content-Transfer-Encoding: ..." header is indeed required >for sending UTF-8 messages, see RFC 2049 section 2 bullet point 2. >"8bit" is one of the valid values that MUAs must support for >receiving messages by default. >Using it seems sane because it is most likely to work with receiving >MUAs that are not MIME-conformant, like our mail(1) itself. >I think nowadays, that's a bigger concern than MTAs that are not >8-bit clean, in particular when maintaining a low-level program >like our mail(1). >Consequently, i think using 8bit is indeed better for our mail(1) >than quoted-printable or base64. > > * Adding "Content-Type: text/plain; charset=utf-8" is required by >RFC 2049 section 2 bullet point 4 (for the simplest kind of UTF-8 >encoded messages). > > * The Content-Disposition: header is defined in RFC 2183, clearly >optional, and not useful in single-part messages. Consequently, >mail(1) should not write it. > > So apart from writing the headers for us-ascii, i think you are > almost there. > > Given that the charset cannot be inferred from the environment > and that setting it per-system or per-user in a configuration file > is also inadequate - it shouldn't be uncommon for users to sometimes > send US-ASCII and sometimes UTF-8 mail - i think that a new option > is indeed needed. > > Regarding the naming of the option, compatibility with POSIX > https://pubs.opengroup.org/onlinepubs/9699919799/utilities/mailx.html > is paramount, which kills the tentative idea to use -u for "UTF-8" > because -u already means "user". > > Compatibility with other mailx(1) implementations is also a > consideration. See, for example, > https://linux.die.net/man/1/mail > and -m is indeed among the very few options still available over there. > I would document it focussing on a "multibyte character encoding" > mnemonic. The "mime" mnemonic feels far too broad because MIME can > be used for lots of other purposes besides specifying a character > encoding. > > The -m option is also free here: > https://man.freebsd.org/cgi/man.cgi?query=mail(1) > https://man.netbsd.org/mail.1 > https://docs.oracle.com/cd/E88353_01/html/E37839/mailx-1.html > https://www.ibm.com/docs/en/aix/7.3?topic=m-mail-command-1 > None of those appears to support command line selection of the > character set for sending mail, so i don't see any immediate > logioc clashes either. > > The -m option does clash with this one: > https://www.sdaoden.eu/code-nail.html > But i think dismissing Steffen Daode Nurpmeso as a lunatic is obviously > the way to go. Try to listen to that person and you will never get > anything done. > > The mailx(1) documented on die.net appears to be the Heirloom one. > It does not have an option to select sending US-ASCII or UTF-8. > Instead, it has a "sendcharsets" configuration variable. That's > clearly overengineering, but even when hardcoding the equivalent of > > sendcharsets=utf-8 > > which is also the default, that's nasty because it silently switches to > UTF-8 as soon as a non-ASCII character appears in the input. I think > at least in interactive mode, explicit confirmation from the user would > be required to send UTF-8, instead writing dead.letter if the user > rejects the request, such that they can clean up the file and try again. > > That would certainly be more complicated than requiring an option > up front, not only from the implementation perspective, but arguably > also from the user perspective. So unless other developers think this > should be fully automatic with confirmation
mg: "support" for exuberant/universal ctags tags files
TL;DR "support" because this is not about supporting the fancy new
stuff, but just not breaking on a universal-ctags generated tag file.
I'd just like to tell mg enough of the file format to ignore the
extensions over plain tag files.
The diff is mostly from troglobit' mg:
https://github.com/troglobit/mg/commit/64cd07ecf5c521d50966eca8ba04026e7c9f3be8
The documentation about the universal-ctags extension over tags files
is documented in utags(5) after you install the universal-ctags
package. Tags file entries follows this scheme:
{tagname}{tagfile}{tagaddress}
where tagaddress is an ex command (could be anything, I haven't
checked how vi(1) actually implements it, but mg assumes it's a search
and just does that. Works most of the times).
To store extra metadata about the tag itself, the address is put a
comment in the tagaddress. For example, here's how the main() entry
would look like. Not the type information after ;"
% uctags *.c
% g ^main tags
tags:526: main main.c /^main(int argc, char **argv)$/;" f
typeref:typename:int
Similarly, universal-ctags introduced support for some meta-tag using
some dummy entries under the !_TAG_ "namespace"
(e.g. !_TAG_FILE_FORMAT, !_TAG_FILE_SORTED, ...)
So, the idea is to just ignore tags starting with !_TAG_ and ignore
comments. As simple as that :-)
Index: tags.c
===
RCS file: /home/cvs/src/usr.bin/mg/tags.c,v
retrieving revision 1.27
diff -u -p -r1.27 tags.c
--- tags.c 29 Mar 2023 19:09:04 - 1.27
+++ tags.c 27 Sep 2023 16:21:42 -
@@ -281,6 +281,10 @@ loadtags(const char *fn)
}
while ((l = fparseln(fd, NULL, NULL, "\0",
FPARSELN_UNESCCONT | FPARSELN_UNESCREST)) != NULL) {
+ if (!strncmp(l, "!_TAG_", 6)) {
+ free(l);
+ continue;
+ }
if (addctag(l) == FALSE) {
fclose(fd);
return (FALSE);
@@ -340,7 +344,7 @@ int
addctag(char *s)
{
struct ctag *t = NULL;
- char *l;
+ char *l, *c;
if ((t = malloc(sizeof(struct ctag))) == NULL) {
dobeep();
@@ -357,6 +361,15 @@ addctag(char *s)
*l++ = '\0';
if (*l == '\0')
goto cleanup;
+
+ /*
+* Newer universal ctags format abuse vi comments in the
+* pattern to store extra metadata. Since we don't support it
+* remove it so the pattern is not mangled.
+*/
+ if ((c = strstr(l, ";\"")) != NULL)
+ *c = '\0';
+
t->pat = strip(l, strlen(l));
if (RB_INSERT(tagtree, , t) != NULL) {
free(t);
Re: Send international text with mail(1) - proposal and patches
> + s[i] = '\0'; > + > + i = nou8 = 0; > + while (i != len) ...and even then, mbtowc is easier to use. See Ingo' /src/usr/bin/ls/utf8.c for an example usage. > + if (s[i] <= 0x7f) > + ++i; > + /* Two bytes case */ > + else if (s[i] >= 0xc2 && s[i] < 0xe0 && > + s[i + 1] >= 0x80 && s[i + 1] <= 0xbf) > + i += 2; > + /* Special three bytes case */ > + else if ((s[i] == 0xe0 && > + s[i + 1] >= 0xa0 && s[i + 1] <= 0xbf && > + s[i + 2] >= 0x80 && s[i + 2] <= 0xbf) || > [...] > } Admittedly I haven't followed the thread too closely, although I hope something like this will end up in mail(1) sometimes :-) Thanks, Omar Polo
Re: [feature] ssh-agent: new -A option (like -a) that overwrites existing sockets
On 2023/09/13 15:08:40 +0200, Moritz Fain wrote:
> Most of the code is already there; it's basically just adding a new flag.
>
> Happy to hear your feedback!
can't comment on the diff itself, but the patch was mangled and so it
doesn't apply.
> --- a/usr.bin/ssh/ssh-agent.c
> +++ b/usr.bin/ssh/ssh-agent.c
> @@ -2003,7 +2003,7 @@ usage(void)
> int
> main(int ac, char **av)
> {
> - int c_flag = 0, d_flag = 0, D_flag = 0, k_flag = 0, s_flag = 0;
> + int c_flag = 0, d_flag = 0, D_flag = 0, k_flag = 0, s_flag =
> 0, overwrite_agentsocket = 0;
> int sock, ch, result, saved_errno;
> char *shell, *format, *pidstr, *agentsocket = NULL;
> struct rlimit rlim;
>[...]
> @@ -2163,7 +2165,7 @@ main(int ac, char **av)
> * the parent.
> */
> prev_mask = umask(0177);
> - sock = unix_listener(socket_name, SSH_LISTEN_BACKLOG, 0);
> + sock = unix_listener(socket_name, SSH_LISTEN_BACKLOG,
> overwrite_agentsocket);
> if (sock < 0) {
> /* XXX - unix_listener() calls error() not perror() */
> *socket_name = '\0'; /* Don't unlink any existing file */
Re: [patch] netcat: support --crlf
On 2023/08/25 09:07:35 -0600, "Theo de Raadt" wrote: > Pietro Cerutti wrote: > > > The motivation is that several network protocols are line oriented > > with CRLF as line terminators. SMTP and HTTP are among the most > > popular. > > Yet, all servers of those protocols and and will accept the simpler 1-byte > line terminator. Not http at least. Try nc www.openbsd.org 80 and type an http request, httpd(8) will be happily waiting for a \r. opensmtpd is less picky and is happy with just a line feed. (I don't have any opinion on adding a flag. To be fair, even if it were added I would probably forget and just use ^V^M^M instead of RET to terminate the lines when manually testing something.)
Re: bioctl: do not confirm new passphrases on stdin
sorry for the noise, noticed just now re-reading the diff.
On 2023/08/17 09:32:43 +, Klemens Nanni wrote:
> --- bioctl.8 6 Jul 2023 21:08:50 - 1.111
> +++ bioctl.8 17 Aug 2023 09:24:28 -
> @@ -288,7 +288,7 @@ is specified as "auto", the number of ro
> based on system performance.
> Otherwise the minimum is 4 rounds and the default is 16.
> .It Fl s
> -Read the passphrase for the selected crypto volume from
> +Omit prompts and read passphrases without confirmation from
> .Pa /dev/stdin
> rather than
> .Pa /dev/tty .
"read passphrases" hints that it reads more than one, which is not
what it would do now.
what about something like
+Read the passphrase from standard input without prompting.
I'm not sure how to fit "without confirmation" nicely and whether the
singular "passphrase" is enough to convey what it does.
also, to correct my previous mail
On 2023/08/17 16:28:52 +0200, Omar Polo wrote:
> On 2023/08/17 09:32:43 +, Klemens Nanni wrote:
> > [...]
> > @@ -1316,6 +1316,7 @@ derive_key(u_int32_t type, int rounds, u
> > size_t pl;
> > struct stat sb;
> > charpassphrase[1024], verifybuf[1024];
> > + int rpp_flag = RPP_ECHO_OFF;
>
> since this is the default...
>
> > if (!key)
> > errx(1, "Invalid key");
> > @@ -1351,6 +1352,8 @@ derive_key(u_int32_t type, int rounds, u
> >
> > fclose(f);
> > } else {
> > + rpp_flag |= interactive ? RPP_REQUIRE_TTY : RPP_STDIN;
> > +
>
> I'd find slightly easier to read
>
> + rpp_flag = interactive ? RPP_REQUIRE_TTY : RPP_STDIN
>
> but no strong opinion.
obviously i meant for this line to be moved earlier in my suggestion.
> > if (readpassphrase(prompt, passphrase, sizeof(passphrase),
> > rpp_flag) == NULL)
> > err(1, "unable to read passphrase");
Re: bioctl: do not confirm new passphrases on stdin
On 2023/08/17 09:32:43 +, Klemens Nanni wrote:
> > I think it would be less ugly to have an iteractive global (or similar)
> > and clear that when -s is given (the correct way to write the above would
> > require masking rpp_flag).
>
> Done. This makes all of the following behave as expected
> bioctl -cC -lvnd0a softraid0
> bioctl -d sd2
> bioctl -s -cC -lvnd0a softraid0
> bioctl -P sd2
> bioctl -s -P sd2
>
> Feedback? OK?
I like this more since using the flags in the global was meh.
> [...]
> @@ -1316,6 +1316,7 @@ derive_key(u_int32_t type, int rounds, u
> size_t pl;
> struct stat sb;
> charpassphrase[1024], verifybuf[1024];
> + int rpp_flag = RPP_ECHO_OFF;
since this is the default...
> if (!key)
> errx(1, "Invalid key");
> @@ -1351,6 +1352,8 @@ derive_key(u_int32_t type, int rounds, u
>
> fclose(f);
> } else {
> + rpp_flag |= interactive ? RPP_REQUIRE_TTY : RPP_STDIN;
> +
I'd find slightly easier to read
+ rpp_flag = interactive ? RPP_REQUIRE_TTY : RPP_STDIN
but no strong opinion.
> if (readpassphrase(prompt, passphrase, sizeof(passphrase),
> rpp_flag) == NULL)
> err(1, "unable to read passphrase");
still ok for me whichever option you prefer.
Thanks,
Omar Polo
Re: bioctl: do not confirm new passphrases on stdin
On 2023/08/17 02:21:18 +, Klemens Nanni wrote: > On Fri, Aug 11, 2023 at 03:44:46PM +, Klemens Nanni wrote: > > On Wed, Aug 02, 2023 at 10:37:36AM +, Klemens Nanni wrote: > > > Creating new volumes prompts > > > Passphrase: > > > Re-type passphrase: > > > which is sane for interative usage, but -s (which omits prompts) to read > > > from stdin also prompts twice. > > > > > > I think that's neither intuitive nor ergonomical and as intended for > > > non-interactive scripts, -s should take a new passphase just once. > > > > > > Until a month ago, the manual errorneously said -s could not be used > > > during > > > initial creation anyway, so I worry little about existing scripts like > > > > > > printf '%s\n%s\n' "$pw" "$pw" | bioctl -s -cC -lsd0a softraid0 > > > > > > Diff below makes -s create new volumes with a single passphase: > > > > > > # echo secret | bioctl -s -Cforce -cC -lvnd0a softraid0 > > > bioctl: Passphrases did not match > > > # echo secret | ./obj/bioctl -s -Cforce -cC -lvnd0a softraid0 > > > softraid0: CRYPTO volume attached as sd3 > > > > > > Feedback? Objection? OK? > > > > Ping. > > I'll commit this in a few days unless I hear objections. > > The current -s behaviour is stupid; nothing else I know *silently* prompts > for passphrase *and* confirmation without anything in between. > > This stuff must be either interactive or quiet and one-shot, not in between. fwiw I agree, i find it dumb too to read the password twice when reading from, for e.g., a pipe. Outside of my comfort zone but it reads fine so ok op@. > Index: bioctl.c > === > RCS file: /cvs/src/sbin/bioctl/bioctl.c,v > retrieving revision 1.151 > diff -u -p -r1.151 bioctl.c > --- bioctl.c 18 Oct 2022 07:04:20 - 1.151 > +++ bioctl.c 17 Aug 2023 02:16:37 - > @@ -989,7 +989,7 @@ bio_kdf_generate(struct sr_crypto_kdfinf > derive_key(kdfinfo->pbkdf.generic.type, kdfinfo->pbkdf.rounds, > kdfinfo->maskkey, sizeof(kdfinfo->maskkey), > kdfinfo->pbkdf.salt, sizeof(kdfinfo->pbkdf.salt), > - "New passphrase: ", 1); > + "New passphrase: ", rpp_flag == RPP_REQUIRE_TTY ? 1 : 0); > } > > int
Re: httpd server "default" is not what I expected
On 2023/08/13 11:27:18 +0100, Jason McIntyre wrote:
> On Sun, Aug 13, 2023 at 11:21:39AM +0100, Stuart Henderson wrote:
> > On 2023/08/13 11:13, Omar Polo wrote:
> > > @@ -179,7 +179,8 @@ section starts with a declaration of the server
> > > Each
> > > .Ic server
> > > section starts with a declaration of the server
> > > -.Ar name :
> > > +.Ar name .
> > > +If no one matches the request the first one defined is used.
> > > .Bl -tag -width Ds
> > > .It Ic server Ar name Brq ...
> > > Match the server name using shell globbing rules.
> >
> > The rest looks good, but I think this might be a little more clear as:
> >
>
> i just got to stuart's mail. this reads nice and clear too.
> jmc
Stuart, Jason, thanks! I like the updated text.
Crystal Kolipe correctly noted offlist that it's not correct to say
"the first defined", it's the first defined server matching the port.
diff /usr/src
commit - a7b17fe845fceeb2940fa5924ec5843681aa2c64
path + /usr/src
blob - 16b086a9ee00cd6d8e796a890e9774968556f147
file + usr.sbin/httpd/httpd.conf.5
--- usr.sbin/httpd/httpd.conf.5
+++ usr.sbin/httpd/httpd.conf.5
@@ -98,7 +98,7 @@ server "default" {
For example:
.Bd -literal -offset indent
ext_ip="10.0.0.1"
-server "default" {
+server "example.com" {
listen on $ext_ip port 80
}
.Ed
@@ -179,7 +179,11 @@ section starts with a declaration of the server
Each
.Ic server
section starts with a declaration of the server
-.Ar name :
+.Ar name .
+If a request does not match any server name, it is handled by the
+first defined
+.Ic server
+section that matches the listening port.
.Bl -tag -width Ds
.It Ic server Ar name Brq ...
Match the server name using shell globbing rules.
@@ -779,7 +783,7 @@ server "default" {
.Bd -literal -offset indent
prefork 2
-server "default" {
+server "example.com" {
listen on * port 80
}
@@ -800,7 +804,7 @@ server "default" {
.Qq egress
group.
.Bd -literal -offset indent
-server "default" {
+server "example.com" {
listen on egress port 80
}
.Ed
Re: httpd server "default" is not what I expected
[moving to tech@, there's a diff for the manpage below]
On 2023/08/13 01:04:11 -0700, Alfred Morgan wrote:
> I was surprised that `server "default"` didn't act like I expected. In this
> example I expected `test1` to get 200 and everything else to get 404 but
> this is not the case. In this example server "test1" actually catches all:
> localhost, test1, and test2 will get code 200.
>
> /etc/hosts:
> 127.0.0.1 localhost test1 test2
>
> /tmp/httpd.conf:
> server "test1" {
> listen on localhost port 8080
> block return 200
> }
>
> server "default" {
> listen on localhost port 8080
> block return 404
> }
>
> httpd -df /tmp/httpd.conf &
as you've found out, there's no special meaning behind the "default"
server name. It just means you're defining a virtua host called
"default".
let's go through your tests.
> ftp -o - http://localhost:8080/ #200
no `server' block match "localhost", so httpd uses the first server.
(this is actually undocumented AFAICS)
> ftp -o - http://test1:8080/ #200
this matches your first server.
> ftp -o - http://test2:8080/ #200
This also doesn't match any server block, so httpd uses the first one.
> man httpd.conf says:
> "Match the server name using shell globbing rules. This can be an explicit
> name, www.example.com, or a name including wildcards, *.example.com."
>
> There is no mention as to what `server "default"` does even though it is
> used several times in the man page. I find the behaviour to be odd
> for it not to be documented. It isn't until I change the line to `server
> "*"` when it starts doing what I expected:
>
> ftp -o- http://localhost:8080/ #404
> ftp -o- http://test1:8080/ #200
> ftp -o- http://test2:8080/ #404
>
> This is a gotcha in general. I would think the examples should use server
> "*" instead and document what server "default" actually does.
I agree that's a gotcha and it's easy to misunderstand from the
manpage. I'd prefer to use "example.com" as is done on many other
manpages and sample configurations. Diff below.
While here, add a note that if there's no match the first one is used.
IMHO it's not a great choice, I would have preferred if it returned a
4XX error instead (not found or a generic bad request maybe).
> and while we are here. Why does running httpd as a user say:
> httpd: need root privileges
>
> does it...?
If it say so... :)
httpd needs to chroot and run as 'www' user so needs to be started as
root. It also may need to read private keys which are also owned by
root.
diff /usr/src
commit - a7b17fe845fceeb2940fa5924ec5843681aa2c64
path + /usr/src
blob - 16b086a9ee00cd6d8e796a890e9774968556f147
file + usr.sbin/httpd/httpd.conf.5
--- usr.sbin/httpd/httpd.conf.5
+++ usr.sbin/httpd/httpd.conf.5
@@ -98,7 +98,7 @@ server "default" {
For example:
.Bd -literal -offset indent
ext_ip="10.0.0.1"
-server "default" {
+server "example.com" {
listen on $ext_ip port 80
}
.Ed
@@ -179,7 +179,8 @@ section starts with a declaration of the server
Each
.Ic server
section starts with a declaration of the server
-.Ar name :
+.Ar name .
+If no one matches the request the first one defined is used.
.Bl -tag -width Ds
.It Ic server Ar name Brq ...
Match the server name using shell globbing rules.
@@ -779,7 +780,7 @@ server "default" {
.Bd -literal -offset indent
prefork 2
-server "default" {
+server "example.com" {
listen on * port 80
}
@@ -800,7 +801,7 @@ server "default" {
.Qq egress
group.
.Bd -literal -offset indent
-server "default" {
+server "example.com" {
listen on egress port 80
}
.Ed
Re: Remove ENGINE use from relayd
On 2023/07/13 05:44:03 +0200, Theo Buehler wrote: > This is analogous to the change that op committed to smtpd a few days > ago. Instead of using ENGINE to make RSA use privsep via imsg, create > an RSA method that has custom priv_enc/priv_dec methods, replace the > default RSA method. Ditch numerous wrappers that extract the default > methods on the fly only to add a log call. > > This removes a lot of boilerplate and shows more clearly where the > actual magic happens. Regress exercises this code and passes. nice to see all that redundant code go; ok op
Re: patch(1): don't run off the end in num_components.
On 2023/07/12 12:54:54 +0200, Florian Obser wrote:
> Found with afl, if path ends in '/', num_components will run off the end
> of the string.
>
> OK?
ok op
> (this is on top of tb's fix on bugs but should be independent and not
> cause conflicts.)
>
> diff --git pch.c pch.c
> index 63543a609fb..8c58dc9ffe5 100644
> --- pch.c
> +++ pch.c
> @@ -1484,7 +1484,8 @@ num_components(const char *path)
> size_t n;
> const char *cp;
>
> - for (n = 0, cp = path; (cp = strchr(cp, '/')) != NULL; n++, cp++) {
> + for (n = 0, cp = path; (cp = strchr(cp, '/')) != NULL; n++) {
> + cp++;
> while (*cp == '/')
> cp++; /* skip consecutive slashes */
> }
Re: httpd: use the host name in SERVER_NAME
On 2023/06/30 15:29:07 +0200, Omar Polo wrote:
> duh. sorry for the dumb question, it was obvious. Here's a better
> diff.
>
> I've made strictier the syntax checks for IPv6 (after the closing ']'
> there's the optional port but then nothing else) and joined together
> the cases for the host:port or hostname alone to simplify the validity
> check.
>
> last thing I'm unsure if whether we should log as-is the original Host
> in case of error, as it can contain anything.
>
> Thanks!
now with a 100% correct return value in the ipv6 case. The memmove
rewinds buf while start points at buf+1. i've called getaddrinfo()
with buf, but we still return `start', so that needs to be fixed.
diff /usr/src
commit - 9832f5035d799ae12e9f848cff5650481b673260
path + /usr/src
blob - 091cc86fec946a4a9d422c04a48f5cbfae015ce0
file + usr.sbin/httpd/server_http.c
--- usr.sbin/httpd/server_http.c
+++ usr.sbin/httpd/server_http.c
@@ -23,6 +23,7 @@
#include
#include
+#include
#include
#include
@@ -831,11 +832,49 @@ char *
return (buf);
}
+static int
+valid_domain(char *name, const char **errstr)
+{
+ size_t i;
+ unsigned char c, last = '\0';
+
+ *errstr = NULL;
+
+ if (*name == '\0') {
+ *errstr = "empty domain name";
+ return 0;
+ }
+ if (!isalpha((unsigned char)name[0]) &&
+ !isdigit((unsigned char)name[0])) {
+ *errstr = "domain name starts with an invalid character";
+ return 0;
+ }
+ for (i = 0; name[i] != '\0'; ++i) {
+ c = tolower((unsigned char)name[i]);
+ name[i] = c;
+ if (last == '.' && c == '.') {
+ *errstr = "domain name contains consecutive separators";
+ return 0;
+ }
+ if (c != '.' && c != '-' && !isalnum(c) &&
+ c != '_') /* technically invalid, but common */ {
+ *errstr = "domain name contains invalid characters";
+ return 0;
+ }
+ last = c;
+ }
+ if (name[i - 1] == '.')
+ name[i - 1] = '\0';
+ return 1;
+}
+
char *
server_http_parsehost(char *host, char *buf, size_t len, int *portval)
{
+ struct addrinfo hints, *ai;
char*start, *end, *port;
const char *errstr = NULL;
+ int err;
if (strlcpy(buf, host, len) >= len) {
log_debug("%s: host name too long", __func__);
@@ -849,18 +888,38 @@ server_http_parsehost(char *host, char *buf, size_t le
/* Address enclosed in [] with port, eg. [2001:db8::1]:80 */
start++;
*end++ = '\0';
- if ((port = strchr(end, ':')) == NULL || *port == '\0')
- port = NULL;
- else
- port++;
+ if (*end == ':') {
+ *end++ = '\0';
+ port = end;
+ } else if (*end != '\0') {
+ log_debug("%s: gibberish after IPv6 address: %s",
+ __func__, host);
+ return (NULL);
+ }
+
memmove(buf, start, strlen(start) + 1);
- } else if ((end = strchr(start, ':')) != NULL) {
- /* Name or address with port, eg. www.example.com:80 */
- *end++ = '\0';
- port = end;
+ start = buf;
+
+ memset(, 0, sizeof(hints));
+ hints.ai_flags = AI_NUMERICHOST;
+ err = getaddrinfo(start, NULL, , );
+ if (err != 0) {
+ log_debug("%s: %s: %s", __func__, gai_strerror(err),
+ host);
+ return (NULL);
+ }
+ freeaddrinfo(ai);
} else {
- /* Name or address with default port, eg. www.example.com */
- port = NULL;
+ /* Name or address with optional port */
+ if ((end = strchr(start, ':')) != NULL) {
+ *end++ = '\0';
+ port = end;
+ }
+
+ if (!valid_domain(start, )) {
+ log_debug("%s: %s: %s", __func__, errstr, host);
+ return (NULL);
+ }
}
if (port != NULL) {
Re: httpd: use the host name in SERVER_NAME
On 2023/06/30 11:14:51 +0200, Florian Obser wrote:
> On 2023-06-30 10:46 +02, Omar Polo wrote:
> > On 2023/06/29 23:43:25 +0200, Omar Polo wrote:
> >> On 2023/06/29 19:55:52 +0200, Florian Obser wrote:
> >> > I'm worried that we pass un-sanitized input through to fcgi.
> >> > Of course we are passing *a lot* of un-sanitized input through to fcgi,
> >> > so does this matter in the grand scheme of things?
> >> > But I'd like if server_http_parsehost() enforces syntactically valid
> >> > hostnames first.
> >> >
> >> > There is valid_domain() in usr.bin/ssh/misc.c or res_hnok in
> >> > resolv.h. Which is probably fine to use since we don't care too much
> >> > about portable.
> >>
> >> right, httpd is happily accepting any nonsense as Host. Here's an
> >> adaptation of millert' valid_domain() from usr.bin/ssh/misc.c.
> >> Hope noone is relying on using an empty/malformed Host :>
> >
> > actually this is wrong. it breaks when an ipv6 address is used as
> > Host. this seems to be an issue in ssh too, although I haven't
>
> IIRC ssh only uses valid_domain() when it already knows that it's not an
> IPv6 address.
% ssh ssh://[::1]
usage: ssh [-46AaCfGgKkMNnqsTtVvXxYy] [-B bind_interface]
[...]
% curl --head http://[::1]
HTTP/1.1 200 OK
[...]
but maybe I'm holding it wrong. IPv6 addresses in the HostName config
directive work fine though. `ssh ::1' also works.
> > [...]
> > should I make it strictier (not done just because it's a bit long and
> > definitely not fun to validate ipv6 addresses), or there's something
> > else in base that I can steal fom? :)
>
> getaddrinfo(3) with AI_NUMERICHOST.
duh. sorry for the dumb question, it was obvious. Here's a better
diff.
I've made strictier the syntax checks for IPv6 (after the closing ']'
there's the optional port but then nothing else) and joined together
the cases for the host:port or hostname alone to simplify the validity
check.
last thing I'm unsure if whether we should log as-is the original Host
in case of error, as it can contain anything.
Thanks!
diff /usr/src
commit - 9832f5035d799ae12e9f848cff5650481b673260
path + /usr/src
blob - 091cc86fec946a4a9d422c04a48f5cbfae015ce0
file + usr.sbin/httpd/server_http.c
--- usr.sbin/httpd/server_http.c
+++ usr.sbin/httpd/server_http.c
@@ -23,6 +23,7 @@
#include
#include
+#include
#include
#include
@@ -831,11 +832,49 @@ char *
return (buf);
}
+static int
+valid_domain(char *name, const char **errstr)
+{
+ size_t i;
+ unsigned char c, last = '\0';
+
+ *errstr = NULL;
+
+ if (*name == '\0') {
+ *errstr = "empty domain name";
+ return 0;
+ }
+ if (!isalpha((unsigned char)name[0]) &&
+ !isdigit((unsigned char)name[0])) {
+ *errstr = "domain name starts with an invalid character";
+ return 0;
+ }
+ for (i = 0; name[i] != '\0'; ++i) {
+ c = tolower((unsigned char)name[i]);
+ name[i] = c;
+ if (last == '.' && c == '.') {
+ *errstr = "domain name contains consecutive separators";
+ return 0;
+ }
+ if (c != '.' && c != '-' && !isalnum(c) &&
+ c != '_') /* technically invalid, but common */ {
+ *errstr = "domain name contains invalid characters";
+ return 0;
+ }
+ last = c;
+ }
+ if (name[i - 1] == '.')
+ name[i - 1] = '\0';
+ return 1;
+}
+
char *
server_http_parsehost(char *host, char *buf, size_t len, int *portval)
{
+ struct addrinfo hints, *ai;
char*start, *end, *port;
const char *errstr = NULL;
+ int err;
if (strlcpy(buf, host, len) >= len) {
log_debug("%s: host name too long", __func__);
@@ -849,18 +888,37 @@ server_http_parsehost(char *host, char *buf, size_t le
/* Address enclosed in [] with port, eg. [2001:db8::1]:80 */
start++;
*end++ = '\0';
- if ((port = strchr(end, ':')) == NULL || *port == '\0')
- port = NULL;
- else
- port++;
+ if (*end == ':') {
+ *end++ = '\0';
+ port = end;
+ } else if (*end != '\0') {
+ log_debug("%s: gibberish after IPv6 address: %s",
+ __func__, host);
+ return (NULL);
+
Re: httpd: use the host name in SERVER_NAME
On 2023/06/29 23:43:25 +0200, Omar Polo wrote:
> On 2023/06/29 19:55:52 +0200, Florian Obser wrote:
> > I'm worried that we pass un-sanitized input through to fcgi.
> > Of course we are passing *a lot* of un-sanitized input through to fcgi,
> > so does this matter in the grand scheme of things?
> > But I'd like if server_http_parsehost() enforces syntactically valid
> > hostnames first.
> >
> > There is valid_domain() in usr.bin/ssh/misc.c or res_hnok in
> > resolv.h. Which is probably fine to use since we don't care too much
> > about portable.
>
> right, httpd is happily accepting any nonsense as Host. Here's an
> adaptation of millert' valid_domain() from usr.bin/ssh/misc.c.
> Hope noone is relying on using an empty/malformed Host :>
actually this is wrong. it breaks when an ipv6 address is used as
Host. this seems to be an issue in ssh too, although I haven't
checked if the ssh URI specification inherits the authority from
rfc3986 as-is.
here's a slightly revised version, but just for the sake of the
discussion since its ipv6 parsing is too permissive.
should I make it strictier (not done just because it's a bit long and
definitely not fun to validate ipv6 addresses), or there's something
else in base that I can steal fom? :)
(while here I've tweaked slightly the nonsense in
server_http_parsehost wrt ipv6 addresses and port numbers.)
Thanks!
diff /usr/src
commit - 9832f5035d799ae12e9f848cff5650481b673260
path + /usr/src
blob - 091cc86fec946a4a9d422c04a48f5cbfae015ce0
file + usr.sbin/httpd/server_http.c
--- usr.sbin/httpd/server_http.c
+++ usr.sbin/httpd/server_http.c
@@ -831,6 +831,60 @@ char *
return (buf);
}
+static int
+valid_domain(char *name, const char **errstr)
+{
+ size_t i;
+ unsigned char c, last = '\0';
+
+ *errstr = NULL;
+
+ if (*name == '\0') {
+ *errstr = "empty domain name";
+ return 0;
+ }
+
+ if (name[0] == '[') {
+ for (i = 1; name[i] != '\0'; ++i) {
+ if (name[i] == ']')
+ break;
+ if (!isxdigit((unsigned char)name[i]) &&
+ name[i] != ':') {
+ *errstr = "invalid IPv6 address";
+ return 0;
+ }
+ }
+ if (name[i] != ']' || name[i + 1] != '\0') {
+ *errstr = "invalid IPv6 address";
+ return 0;
+ }
+ return 1;
+ }
+
+ if (!isalpha((unsigned char)name[0]) &&
+ !isdigit((unsigned char)name[0])) {
+ *errstr = "domain name starts with an invalid character";
+ return 0;
+ }
+ for (i = 0; name[i] != '\0'; ++i) {
+ c = tolower((unsigned char)name[i]);
+ name[i] = c;
+ if (last == '.' && c == '.') {
+ *errstr = "domain name contains consecutive separators";
+ return 0;
+ }
+ if (c != '.' && c != '-' && !isalnum(c) &&
+ c != '_') /* technically invalid, but common */ {
+ *errstr = "domain name contains invalid characters";
+ return 0;
+ }
+ last = c;
+ }
+ if (name[i - 1] == '.')
+ name[i - 1] = '\0';
+ return 1;
+}
+
char *
server_http_parsehost(char *host, char *buf, size_t len, int *portval)
{
@@ -847,13 +901,11 @@ server_http_parsehost(char *host, char *buf, size_t le
if (*start == '[' && (end = strchr(start, ']')) != NULL) {
/* Address enclosed in [] with port, eg. [2001:db8::1]:80 */
- start++;
- *end++ = '\0';
- if ((port = strchr(end, ':')) == NULL || *port == '\0')
- port = NULL;
- else
- port++;
- memmove(buf, start, strlen(start) + 1);
+ end++;
+ if (*end == ':') {
+ *end++ = '\0';
+ port = end;
+ }
} else if ((end = strchr(start, ':')) != NULL) {
/* Name or address with port, eg. www.example.com:80 */
*end++ = '\0';
@@ -863,6 +915,11 @@ server_http_parsehost(char *host, char *buf, size_t le
port = NULL;
}
+ if (!valid_domain(start, )) {
+ log_debug("%s: %s: %s", __func__, errstr, host);
+ return (NULL);
+ }
+
if (port != NULL) {
/* Save the requested port */
*portval = strtonum(port, 0, 0x, );
Re: httpd: use the host name in SERVER_NAME
On 2023/06/29 19:55:52 +0200, Florian Obser wrote:
> I'm worried that we pass un-sanitized input through to fcgi.
> Of course we are passing *a lot* of un-sanitized input through to fcgi,
> so does this matter in the grand scheme of things?
> But I'd like if server_http_parsehost() enforces syntactically valid
> hostnames first.
>
> There is valid_domain() in usr.bin/ssh/misc.c or res_hnok in
> resolv.h. Which is probably fine to use since we don't care too much
> about portable.
right, httpd is happily accepting any nonsense as Host. Here's an
adaptation of millert' valid_domain() from usr.bin/ssh/misc.c.
Hope noone is relying on using an empty/malformed Host :>
diff /usr/src
commit - 9832f5035d799ae12e9f848cff5650481b673260
path + /usr/src
blob - 091cc86fec946a4a9d422c04a48f5cbfae015ce0
file + usr.sbin/httpd/server_http.c
--- usr.sbin/httpd/server_http.c
+++ usr.sbin/httpd/server_http.c
@@ -831,6 +831,42 @@ char *
return (buf);
}
+static int
+valid_domain(char *name, const char **errstr)
+{
+ size_t i;
+ unsigned char c, last = '\0';
+
+ *errstr = NULL;
+
+ if (*name == '\0') {
+ *errstr = "empty domain name";
+ return 0;
+ }
+ if (!isalpha((unsigned char)name[0]) &&
+ !isdigit((unsigned char)name[0])) {
+ *errstr = "domain name starts with an invalid character";
+ return 0;
+ }
+ for (i = 0; name[i] != '\0'; ++i) {
+ c = tolower((unsigned char)name[i]);
+ name[i] = c;
+ if (last == '.' && c == '.') {
+ *errstr = "domain name contains consecutive separators";
+ return 0;
+ }
+ if (c != '.' && c != '-' && !isalnum(c) &&
+ c != '_') /* technically invalid, but common */ {
+ *errstr = "domain name contains invalid characters";
+ return 0;
+ }
+ last = c;
+ }
+ if (name[i - 1] == '.')
+ name[i - 1] = '\0';
+ return 1;
+}
+
char *
server_http_parsehost(char *host, char *buf, size_t len, int *portval)
{
@@ -863,6 +899,11 @@ server_http_parsehost(char *host, char *buf, size_t le
port = NULL;
}
+ if (!valid_domain(start, )) {
+ log_debug("%s: %s: %s", __func__, errstr, host);
+ return (NULL);
+ }
+
if (port != NULL) {
/* Save the requested port */
*portval = strtonum(port, 0, 0x, );
Re: ftp: drop needless strcspn
On 2023/06/28 18:47:17 +0200, Theo Buehler wrote:
> On Wed, Jun 28, 2023 at 06:37:43PM +0200, Omar Polo wrote:
> > A similar thing could be applied to rpki-client' http.c as well, it
> > should work the same.
>
> Could you please send a diff? The files have diverged, but keeping the
> shared parts in sync is helpful.
here it is, but i can't do more than compile-test it.
The trailing spaces are trimmed in http_get_line(), so the strcspn is
just as redundant.
diff /usr/src
commit - 9832f5035d799ae12e9f848cff5650481b673260
path + /usr/src
blob - 4544eac237147fe77bcb9d0068438827ddd81424
file + usr.sbin/rpki-client/http.c
--- usr.sbin/rpki-client/http.c
+++ usr.sbin/rpki-client/http.c
@@ -1369,7 +1369,6 @@ http_parse_header(struct http_connection *conn, char *
else if (strncasecmp(cp, CONTENTLEN, sizeof(CONTENTLEN) - 1) == 0) {
cp += sizeof(CONTENTLEN) - 1;
cp += strspn(cp, " \t");
- cp[strcspn(cp, " \t")] = '\0';
conn->iosz = strtonum(cp, 0, MAX_CONTENTLEN, );
if (errstr != NULL) {
warnx("Content-Length of %s is %s",
@@ -1422,14 +1421,12 @@ http_parse_header(struct http_connection *conn, char *
sizeof(TRANSFER_ENCODING) - 1) == 0) {
cp += sizeof(TRANSFER_ENCODING) - 1;
cp += strspn(cp, " \t");
- cp[strcspn(cp, " \t")] = '\0';
if (strcasecmp(cp, "chunked") == 0)
conn->chunked = 1;
} else if (strncasecmp(cp, CONTENT_ENCODING,
sizeof(CONTENT_ENCODING) - 1) == 0) {
cp += sizeof(CONTENT_ENCODING) - 1;
cp += strspn(cp, " \t");
- cp[strcspn(cp, " \t")] = '\0';
if (strcasecmp(cp, "gzip") == 0 ||
strcasecmp(cp, "deflate") == 0) {
if (http_inflate_new(conn) == -1)
@@ -1439,7 +1436,6 @@ http_parse_header(struct http_connection *conn, char *
} else if (strncasecmp(cp, CONNECTION, sizeof(CONNECTION) - 1) == 0) {
cp += sizeof(CONNECTION) - 1;
cp += strspn(cp, " \t");
- cp[strcspn(cp, " \t")] = '\0';
if (strcasecmp(cp, "close") == 0)
conn->keep_alive = 0;
else if (strcasecmp(cp, "keep-alive") == 0)
ftp: drop needless strcspn
since fetch.c revision 1.211 ("strip spaces at end of header lines and
in chunked encoding headers") ftp removes trailing whitespaces early
so we don't need to re-do that upon every header we parse.
it can also be misleading since one can wonder why LAST_MODIFIED
doesn't have " \t" but only "\t", or confront it with rpki-client'
http.c and notice that there there is no strcspn() call in
Last-Modified handling.
I've tested it by modifying httpd(8) to append " \t " at the end of
every header (legal per rfc 7230) and observing that ftp still works
fine, including mtime handling.
A similar thing could be applied to rpki-client' http.c as well, it
should work the same.
As a bonus, drop the unused `s' variable in Retry-After handling.
ok?
diff /usr/src
commit - 9832f5035d799ae12e9f848cff5650481b673260
path + /usr/src
blob - eb4c872b72b3db468f782a12756bb7f8fb64784d
file + usr.bin/ftp/fetch.c
--- usr.bin/ftp/fetch.c
+++ usr.bin/ftp/fetch.c
@@ -891,7 +891,6 @@ noslash:
if (strncasecmp(cp, CONTENTLEN, sizeof(CONTENTLEN) - 1) == 0) {
cp += sizeof(CONTENTLEN) - 1;
cp += strspn(cp, " \t");
- cp[strcspn(cp, " \t")] = '\0';
filesize = strtonum(cp, 0, LLONG_MAX, );
if (errstr != NULL)
goto improper;
@@ -964,10 +963,8 @@ noslash:
#define RETRYAFTER "Retry-After:"
} else if (isunavail &&
strncasecmp(cp, RETRYAFTER, sizeof(RETRYAFTER) - 1) == 0) {
- size_t s;
cp += sizeof(RETRYAFTER) - 1;
cp += strspn(cp, " \t");
- cp[strcspn(cp, " \t")] = '\0';
retryafter = strtonum(cp, 0, 0, );
if (errstr != NULL)
retryafter = -1;
@@ -976,7 +973,6 @@ noslash:
sizeof(TRANSFER_ENCODING) - 1) == 0) {
cp += sizeof(TRANSFER_ENCODING) - 1;
cp += strspn(cp, " \t");
- cp[strcspn(cp, " \t")] = '\0';
if (strcasecmp(cp, "chunked") == 0)
chunked = 1;
#ifndef SMALL
@@ -985,7 +981,6 @@ noslash:
sizeof(LAST_MODIFIED) - 1) == 0) {
cp += sizeof(LAST_MODIFIED) - 1;
cp += strspn(cp, " \t");
- cp[strcspn(cp, "\t")] = '\0';
if (strptime(cp, "%a, %d %h %Y %T %Z", ) == NULL)
server_timestamps = 0;
#endif /* !SMALL */
Re: mg.1: add no-tab-mode to set-default-mode description
Hi, On 2023/06/27 18:53:44 -0700, "Simon Branch" wrote: > Hello! > > I've been enjoying the resurrection of no-tab-mode in mg(1). > It is simple to enable by default: > > set-default-mode notab > > The manual page says that "built in modes include fill, indent and > overwrite". This patch makes the list more complete by adding notab, > and adds "can be set globally with set-default-mode" to no-tab-mode's > documentation (since it is there for the other modes). Hopefully this > makes the manual page more useful, obvious, and regular. it's correct, we need to document that for set-default-mode is "notab" and I agree with making the no-tab-mode description coherent with the other modes. Committed, thank you!
smtpd, relayd, iked: drop ssl_init
while talking about a related matter with tb and jsing, jsing noted
that ssl_init() in smtpd is completely useless. All its loading is
already done automatically by libcrypto at runtime, and judging by the
implementation of the called functions there's no need to actually
force the initialization.
There is similar code in relayd and iked, so apply the same treatment.
I've tested smtpd and it works just as fine as before, don't use
relayd but the regression suite is happy. I don't use iked, so some
testing with it is welcomed. Not that I expect any sort of breakage,
this is almost a no-op.
ok?
diff 143f55f5d199bde9c93e92667cd4bfda0a272dd2
d9f7ac73d694ec29760e87d4b21a06e9aa8ef711
commit - 143f55f5d199bde9c93e92667cd4bfda0a272dd2
commit + d9f7ac73d694ec29760e87d4b21a06e9aa8ef711
blob - 7f7c8bee0d371e0ac4537330662bdcc7f20f0cd1
blob + c0ff87dafd5b27ba25fbaac73921cab7488f20ac
--- sbin/iked/ca.c
+++ sbin/iked/ca.c
@@ -33,7 +33,6 @@
#include
#include
-#include
#include
#include
#include
@@ -1959,17 +1958,6 @@ ca_sslinit(void)
}
void
-ca_sslinit(void)
-{
- OpenSSL_add_all_algorithms();
- ERR_load_crypto_strings();
-
- /* Init hardware crypto engines. */
- ENGINE_load_builtin_engines();
- ENGINE_register_all_complete();
-}
-
-void
ca_sslerror(const char *caller)
{
unsigned longerror;
blob - aa824d6f1966034acb591fca3d0710e00796b49c
blob + 360127f73edc1b88f85796e892e8be84829c3477
--- sbin/iked/iked.c
+++ sbin/iked/iked.c
@@ -175,7 +175,6 @@ main(int argc, char *argv[])
if (strlcpy(env->sc_conffile, conffile, PATH_MAX) >= PATH_MAX)
errx(1, "config file exceeds PATH_MAX");
- ca_sslinit();
group_init();
policy_init(env);
blob - 85958e1c2370b0780095e343a58438187a88c3dd
blob + bf83d4799ee8a498cdf9f10ba9d4f57cdfade249
--- sbin/iked/iked.h
+++ sbin/iked/iked.h
@@ -1178,7 +1178,6 @@ void ca_sslinit(void);
voidca_getkey(struct privsep *, struct iked_id *, enum imsg_type);
int ca_privkey_serialize(EVP_PKEY *, struct iked_id *);
int ca_pubkey_serialize(EVP_PKEY *, struct iked_id *);
-voidca_sslinit(void);
voidca_sslerror(const char *);
char *ca_asn1_name(uint8_t *, size_t);
void *ca_x509_name_parse(char *);
blob - a2f1c130d6b45e3082048218c11537dca485998a
blob + a1272319a945a8dc1c859151a9c06c29d10484ab
--- usr.sbin/relayd/config.c
+++ usr.sbin/relayd/config.c
@@ -293,7 +293,6 @@ config_getcfg(struct relayd *env, struct imsg *imsg)
}
if (env->sc_conf.flags & (F_TLS|F_TLSCLIENT)) {
- ssl_init(env);
if (what & CONFIG_CA_ENGINE)
ca_engine_init(env);
}
blob - edc86218960df02cb917606bdf402c776e07206d
blob + 7dd2f856e20f07ea0f9ec6e599da54c3f35ef54e
--- usr.sbin/relayd/relayd.c
+++ usr.sbin/relayd/relayd.c
@@ -255,9 +255,6 @@ main(int argc, char *argv[])
exit(0);
}
- if (env->sc_conf.flags & (F_TLS|F_TLSCLIENT))
- ssl_init(env);
-
/* rekey the TLS tickets before pushing the config */
parent_tls_ticket_rekey(0, 0, env);
if (parent_configure(env) == -1)
blob - 990cec3505fc6bc22b837cc4efb0d58af3614984
blob + 5c4618b9fc4de4867c35f2e09ad0aa8de7ed0290
--- usr.sbin/relayd/relayd.h
+++ usr.sbin/relayd/relayd.h
@@ -1293,7 +1293,6 @@ void ssl_init(struct relayd *);
int script_exec(struct relayd *, struct ctl_script *);
/* ssl.c */
-voidssl_init(struct relayd *);
char *ssl_load_key(struct relayd *, const char *, off_t *, char *);
uint8_t *ssl_update_certificate(const uint8_t *, size_t, EVP_PKEY *,
EVP_PKEY *, X509 *, size_t *);
blob - 0d76f8ba5eba40827760bba8f38a91b4247b0090
blob + 4cb7d81c1e383ec5222a77a74d215d9b13e3ee0d
--- usr.sbin/relayd/ssl.c
+++ usr.sbin/relayd/ssl.c
@@ -27,30 +27,11 @@
#include
#include
-#include
#include "relayd.h"
intssl_password_cb(char *, int, int, void *);
-void
-ssl_init(struct relayd *env)
-{
- static int initialized = 0;
-
- if (initialized)
- return;
-
- SSL_library_init();
- SSL_load_error_strings();
-
- /* Init hardware crypto engines. */
- ENGINE_load_builtin_engines();
- ENGINE_register_all_complete();
-
- initialized = 1;
-}
-
int
ssl_password_cb(char *buf, int size, int rwflag, void *u)
{
@@ -73,9 +54,6 @@ ssl_load_key(struct relayd *env, const char *name, off
long size;
char*data, *buf = NULL;
- /* Initialize SSL library once */
- ssl_init(env);
-
/*
* Read (possibly) encrypted key from file
*/
blob - 9802ee144e84c38ae747c6f25ce9d4957a84e332
blob + 86b3d032501898656da1bec3e757ff6429201b3b
--- usr.sbin/smtpd/ssl.c
+++ usr.sbin/smtpd/ssl.c
@@ -22,7 +22,6 @@
#include
#include
-#include
#include
#include
#include
@@ -31,25 +30,6 @@ void
#include "log.h"
#include "ssl.h"
-void
-ssl_init(void)
-{
-
Re: Error in ex(1) s command when using c option and numbering on
On 2023/06/22 13:18:43 -0600, Todd C. Miller wrote:
> On Tue, 07 Feb 2023 20:35:10 -0700, Todd C. Miller wrote:
>
> > On Tue, 07 Feb 2023 17:17:02 -0700, Todd C. Miller wrote:
> >
> > > Yes, the bug is that the number is not displayed. The following
> > > diff fixes that but there is still a bug because the resulting line
> > > also lacks a line number. In other words, instead of:
> > >
> > > :s/men/MEN/c
> > > 1 Five women came to the party.
> > >^^^[ynq]y
> > > Five woMEN came to the party.
> > >
> > > it should look like this:
> > >
> > > :s/men/MEN/c
> > > 1 Five women came to the party.
> > >^^^[ynq]y
> > > 1 Five woMEN came to the party.
> >
> > Here's an updated diff that prints line numbers when autoprint is
> > set too. This seems to match historic ex behavior and POSIX, but
> > I'd appreciate other eyes on it.
>
> Moving this from busg@ to tech@. I noticed today I still have this
> diff from Feb rotting in my tree. The original thread is:
> https://marc.info/?l=openbsd-bugs=167580085421828=2
>
> OK?
not really using ex, but the diff reads fine and fixes the issue.
ok op@
(midly surprised by LF_INIT, wich is just a long way to set a local
variable :/)
> Index: usr.bin/vi/ex/ex.c
> ===
> RCS file: /cvs/src/usr.bin/vi/ex/ex.c,v
> retrieving revision 1.22
> diff -u -p -u -r1.22 ex.c
> --- usr.bin/vi/ex/ex.c20 Feb 2022 19:45:51 - 1.22
> +++ usr.bin/vi/ex/ex.c8 Feb 2023 03:28:32 -
> @@ -1454,8 +1454,14 @@ addr_verify:
> LF_INIT(FL_ISSET(ecp->iflags, E_C_HASH | E_C_LIST | E_C_PRINT));
> if (!LF_ISSET(E_C_HASH | E_C_LIST | E_C_PRINT | E_NOAUTO) &&
> !F_ISSET(sp, SC_EX_GLOBAL) &&
> - O_ISSET(sp, O_AUTOPRINT) && F_ISSET(ecp, E_AUTOPRINT))
> - LF_INIT(E_C_PRINT);
> + O_ISSET(sp, O_AUTOPRINT) && F_ISSET(ecp, E_AUTOPRINT)) {
> +
> + /* Honor the number option if autoprint is set. */
> + if (F_ISSET(ecp, E_OPTNUM))
> + LF_INIT(E_C_HASH);
> + else
> + LF_INIT(E_C_PRINT);
> + }
>
> if (LF_ISSET(E_C_HASH | E_C_LIST | E_C_PRINT)) {
> cur.lno = sp->lno;
> Index: usr.bin/vi/ex/ex_subst.c
> ===
> RCS file: /cvs/src/usr.bin/vi/ex/ex_subst.c,v
> retrieving revision 1.30
> diff -u -p -u -r1.30 ex_subst.c
> --- usr.bin/vi/ex/ex_subst.c 18 Apr 2017 01:45:35 - 1.30
> +++ usr.bin/vi/ex/ex_subst.c 8 Feb 2023 03:23:27 -
> @@ -633,7 +633,9 @@ nextmatch:match[0].rm_so = offset;
> goto lquit;
> }
> } else {
> - if (ex_print(sp, cmdp, , , 0) ||
> + const int flags =
> + O_ISSET(sp, O_NUMBER) ? E_C_HASH : 0;
> + if (ex_print(sp, cmdp, , , flags) ||
> ex_scprint(sp, , ))
> goto lquit;
> if (ex_txt(sp, , 0, TXT_CR))
smtpd: allow arguments on NOOP
another diff from the -portable repo:
https://github.com/OpenSMTPD/OpenSMTPD/pull/1150
per rfc-5321 § 4.1.1.9 the NOOP command allows optionally one argument
that we SHOULD ignore.
The original diff set the check function in the commands table to NULL
because NOOP is not a phase that can have filters. However, I prefer
to stay on the safe side and add a smtp_check_noop() function.
Alternatively, we could allow a NULL check function in
smtp_session_imsg().
the rfc specifies only one optional string, while here for semplicity
it's relaxed to allow anything.
diff /usr/src
commit - 8def1c1c2777f0b5175283f8116e1eaab1f1962a
path + /usr/src
blob - 1686f03e96deeb5e6ea8b065456e04c27c752c8c
file + usr.sbin/smtpd/smtp_session.c
--- usr.sbin/smtpd/smtp_session.c
+++ usr.sbin/smtpd/smtp_session.c
@@ -212,6 +212,7 @@ static int smtp_check_noparam(struct smtp_session *,
static int smtp_check_mail_from(struct smtp_session *, const char *);
static int smtp_check_rcpt_to(struct smtp_session *, const char *);
static int smtp_check_data(struct smtp_session *, const char *);
+static int smtp_check_noop(struct smtp_session *, const char *);
static int smtp_check_noparam(struct smtp_session *, const char *);
static void smtp_filter_phase(enum filter_phase, struct smtp_session *, const
char *);
@@ -276,7 +277,7 @@ static struct {
{ CMD_DATA, FILTER_DATA,"DATA",
smtp_check_data,smtp_proceed_data },
{ CMD_RSET, FILTER_RSET,"RSET",
smtp_check_rset,smtp_proceed_rset },
{ CMD_QUIT, FILTER_QUIT,"QUIT",
smtp_check_noparam, smtp_proceed_quit },
- { CMD_NOOP, FILTER_NOOP,"NOOP",
smtp_check_noparam, smtp_proceed_noop },
+ { CMD_NOOP, FILTER_NOOP,"NOOP",
smtp_check_noop,smtp_proceed_noop },
{ CMD_HELP, FILTER_HELP,"HELP",
smtp_check_noparam, smtp_proceed_help },
{ CMD_WIZ, FILTER_WIZ, "WIZ",
smtp_check_noparam, smtp_proceed_wiz },
{ CMD_COMMIT, FILTER_COMMIT, ".",
smtp_check_noparam, smtp_proceed_commit },
@@ -1343,8 +1344,8 @@ smtp_command(struct smtp_session *s, char *line)
break;
case CMD_NOOP:
- if (!smtp_check_noparam(s, args))
- break;
+ if (!smtp_check_noop(s, args))
+ break;
smtp_filter_phase(FILTER_NOOP, s, NULL);
break;
@@ -1631,6 +1632,12 @@ smtp_check_noparam(struct smtp_session *s, const char
}
static int
+smtp_check_noop(struct smtp_session *s, const char *args)
+{
+ return 1;
+}
+
+static int
smtp_check_noparam(struct smtp_session *s, const char *args)
{
if (args != NULL) {
Re: avoid truncation of filtered smtpd data lines
On 2023/06/20 14:38:37 -0600, Todd C. Miller wrote:
> > qid = ep+1;
> > - if ((ep = strchr(qid, '|')) == NULL)
> > - fatalx("Missing reqid: %s", line);
> > - ep[0] = '\0';
> > -
>
> This is not a new problem but we really should set errno=0 before
> calling strtoull() for the first time. It is possible for errno
> to already be set to ERANGE which causes problems if strtoull()
> returns ULLONG_MAX and there is not an error. It's fine if you
> don't want to fix that in this commit but I do think it should get
> fixed.
if you don't mind i'll fix it in a separate commit. I've also checked
if there were other places to adjust but this appears to be the only
one instance.
> [...]
>
> It took me a minute to realize that this is OK, but it seems fine.
>
> > if (strcmp(response, "proceed") == 0) {
> > - if (parameter != NULL)
> > - fatalx("Unexpected parameter after proceed: %s", line);
> > filter_protocol_next(token, reqid, 0);
> > return;
yeah it's subtle, i should have pointed it out, sorry. if "response"
contain a parameter, strcmp() return nonzero, so the parameter check
is not needed.
The drawback is that the error messages on protocol violation are a
bit worst. Before something like "...|proceed|foobar" would fail with
"unexpected parameter after proceed: ", now "Invalid directive:
", but I don't think it's a big deal.
> > } else if (strcmp(response, "junk") == 0) {
> > - if (parameter != NULL)
> > - fatalx("Unexpected parameter after junk: %s", line);
> > if (fs->phase == FILTER_COMMIT)
> > fatalx("filter-reponse junk after DATA");
> > filter_result_junk(reqid);
> > @@ -667,11 +648,11 @@ lka_filter_process_response(const char *name, const ch
> > if (parameter == NULL)
> > fatalx("Missing parameter: %s", line);
> >
> > - if (strcmp(response, "rewrite") == 0)
> > + if (strncmp(response, "rewrite|", 8) == 0)
> > filter_result_rewrite(reqid, parameter);
> > - else if (strcmp(response, "reject") == 0)
> > + else if (strncmp(response, "reject|", 7) == 0)
> > filter_result_reject(reqid, parameter);
> > - else if (strcmp(response, "disconnect") == 0)
> > + else if (strncmp(response, "disconnect|", 11) == 0)
> > filter_result_disconnect(reqid, parameter);
> > else
> > fatalx("Invalid directive: %s", line);
> >
diff /usr/src
commit - f47faee0d8945111b03f2db500f23a23f37892bd
path + /usr/src
blob - f0429274168cddb3532c591c6fbc352e0ff7edda
file + usr.sbin/smtpd/lka_filter.c
--- usr.sbin/smtpd/lka_filter.c
+++ usr.sbin/smtpd/lka_filter.c
@@ -605,6 +605,8 @@ lka_filter_process_response(const char *name, const ch
if ((ep = strchr(kind, '|')) == NULL)
fatalx("Missing token: %s", line);
qid = ep+1;
+
+ errno = 0;
reqid = strtoull(qid, , 16);
if (qid[0] == '\0' || *ep != '|')
fatalx("Invalid reqid: %s", line);
Re: [patch] usr.sbin/smtpd filter localhost relays
Hello,
sorry for the delay and thanks for the patch.
On 2023/02/28 12:16:17 +0100, Philipp wrote:
> Hi
>
> On github someone reported an issue[0] regarding localhost MX entries.
> Currently smtpd will just use the localhost relay. This leads to a
> loop. Here a patch filtering localhost and localhost addresses for MX
> requests.
so the report is interesting. due to a typo, a mail was sent to a
wrong host which happen to have "localhost" as MX.
this makes smtpd loop for a while since it connects to localhost to
forward the mail, and connections from localhost are per-config
allowed to send mails to anyone. Rinse and repeat.
after ~2 minutes (not tested myself, the actual number was provided in
the github issue but seems fair) the loop detection (i.e. more than
100 Received headers) kicks in and the mail rejected.
Now, to be honest I don't like the proposed patch. I'm not sure
special-casing "localhost" (and its equivalent spellings) is good.
We're adding code to handle a very fringe case which doesn't really
have bad consequences (one envelope in the queue for a few minutes is
not that of a big deal) and it's not even comprehensive. I don't see
the gain.
Nothing stops me now to change my MX after sending this email to match
the MX of your server and cause you a similar loop when you'll reply.
Or picking an ip in a similarly private blocks like in 192.168.0.0/16
as it's not that uncommon I believe to have a mx that relays mails
from a LAN.
However, I won't object if some other developer think this is good and
wants to commit this or a variation.
Since we're here, some style nits on the diff inlined below.
> As next step you could implement Null-MX (rfc 7505).
This could be worthwile, but wouldn't have helped at all in this case.
It's something the owners of that domain should have used instead of
putting "localhost" as MX.
Patches to support Null-MX are welcome though.
> Philipp
>
> [0] https://github.com/OpenSMTPD/OpenSMTPD/issues/1190
>
> diff --git a/usr.sbin/smtpd/dns.c b/usr.sbin/smtpd/dns.c
> index f7c6b3df..7389efec 100644
> --- a/usr.sbin/smtpd/dns.c
> +++ b/usr.sbin/smtpd/dns.c
> @@ -53,6 +53,7 @@ struct dns_lookup {
> struct dns_session *session;
> char*host;
> int preference;
> + int filter_localhost;
> };
>
> struct dns_session {
> @@ -65,7 +66,7 @@ struct dns_session {
> int refcount;
> };
>
> -static void dns_lookup_host(struct dns_session *, const char *, int);
> +static void dns_lookup_host(struct dns_session *, const char *, int, int);
> static void dns_dispatch_host(struct asr_result *, void *);
> static void dns_dispatch_mx(struct asr_result *, void *);
> static void dns_dispatch_mx_preference(struct asr_result *, void *);
> @@ -139,7 +140,7 @@ dns_imsg(struct mproc *p, struct imsg *imsg)
> case IMSG_MTA_DNS_HOST:
> m_get_string(, );
> m_end();
> - dns_lookup_host(s, host, -1);
> + dns_lookup_host(s, host, -1, 0);
> return;
>
> case IMSG_MTA_DNS_MX:
> @@ -205,6 +206,28 @@ dns_imsg(struct mproc *p, struct imsg *imsg)
> }
> }
>
> +static int
> +is_localhost(struct sockaddr *sa)
> +{
> + struct sockaddr_in6 *ipv6;
> + struct sockaddr_in *ipv4;
> + uint32_t addr;
> +
> + switch (sa->sa_family) {
> + case AF_INET6:
> + // May check also for v6 mapped v4 addresses
please don't use C++-style comments.
> + ipv6 = (struct sockaddr_in6 *)sa;
> + return IN6_IS_ADDR_LOOPBACK(>sin6_addr);
> + case AF_INET:
> + ipv4 = (struct sockaddr_in *)sa;
> + addr = ntohl(ipv4->sin_addr.s_addr);
> + return ((addr >> 24) & 0xff) == 127;
> + default:
> + log_warnx("warn: unknown family in sockaddr");
> + }
> + return 0;
> +}
> +
> static void
> dns_dispatch_host(struct asr_result *ar, void *arg)
> {
> @@ -215,6 +238,10 @@ dns_dispatch_host(struct asr_result *ar, void *arg)
> s = lookup->session;
>
> for (ai = ar->ar_addrinfo; ai; ai = ai->ai_next) {
> + if (lookup->filter_localhost && is_localhost(ai->ai_addr)) {
> + log_warnx("warn: ignore localhost address for host %s",
> lookup->host);
even though is not always respected, please try to keep the lines
under 80 columns.k
> + continue;
> + }
> s->mxfound++;
> m_create(s->p, IMSG_MTA_DNS_HOST, 0, 0, -1);
> m_add_id(s->p, s->reqid);
> @@ -280,14 +307,18 @@ dns_dispatch_mx(struct asr_result *ar, void *arg)
> continue;
> print_dname(rr.rr.mx.exchange, buf, sizeof(buf));
> buf[strlen(buf) - 1] = '\0';
> - dns_lookup_host(s, buf, rr.rr.mx.preference);
> + if (strcasecmp("localhost", buf)==0) {
nit, I'd use spaces around the
avoid truncation of filtered smtpd data lines
hello tech@,
this was reported some time ago on the OpenSMTPD-portable repository[0]
[0]: https://github.com/OpenSMTPD/OpenSMTPD/pull/1192
Filters can register to the data-line event to alter the mail content.
smtpd, when parsing the filter' output it first copies the received
line in a temporary buffer. Here truncation may happen if a line of
the mail is longer than LINE_MAX (minus 50 to be exact).
The proposed diff on github bumps the local buffer to SMTP_LINE_MAX +
256, but I think it's quite wasteful to have a buffer that long on the
stack (SMTP_LINE_MAX is 65535 bytes).
A very simple fix could be:
: if (strcmp(kind, "filter-dataline") == 0) {
: if (fs->phase != FILTER_DATA_LINE)
: fatalx("filter-dataline out of dataline phase");
: - filter_data_next(token, reqid, response);
: + filter_data_next(token, reqid, line + (response - buffer));
: return;
: }
: if (fs->phase == FILTER_DATA_LINE)
since `line' contains the original line and it's guaranteed that the
fields of the reply excluding the last (variable width) fits the
buffer, it works. maybe it's too clever.
Then I realized that we don't need to copy the line at all. We're
already using strtoull to parse the number and the payload is the last
field of the received line, so we can just advance the pointer. The
drawback is that we now need to use a few strncmp, but I think it's
worth it.
disclaimer: i've run this only on localhost so far.
To reproduce, you can use an awk script like the following
#!/usr/bin/awk -f
# filter-dummy: copies the data lines as-is.
BEGIN { FS = "|" }
/^config\|ready$/ {
print "register|filter|smtp-in|data-line"
print "register|ready"
fflush()
}
"filter|smtp-in|data-line" == $1"|"$4"|"$5 {
line = substr($0, length($1$2$3$4$5$6$7) + 8)
printf "filter-dataline|%s|%s|%s\n", $6, $7, line
fflush()
}
with a configuration like:
table aliases file:/etc/mail/aliases
filter "dummy" proc-exec "/path/to/filter-dummy"
listen on lo0 filter "dummy"
action "local" mbox alias
match from local for local action "local"
and then sending a mail with a very long line:
% tr -cd '[:print:]' phase != FILTER_DATA_LINE)
fatalx("filter-dataline out of dataline phase");
filter_data_next(token, reqid, response);
@@ -646,19 +633,13 @@ lka_filter_process_response(const char *name, const ch
if (fs->phase == FILTER_DATA_LINE)
fatalx("filter-result in dataline phase");
- if ((ep = strchr(response, '|'))) {
+ if ((ep = strchr(response, '|')) != NULL)
parameter = ep + 1;
- ep[0] = '\0';
- }
if (strcmp(response, "proceed") == 0) {
- if (parameter != NULL)
- fatalx("Unexpected parameter after proceed: %s", line);
filter_protocol_next(token, reqid, 0);
return;
} else if (strcmp(response, "junk") == 0) {
- if (parameter != NULL)
- fatalx("Unexpected parameter after junk: %s", line);
if (fs->phase == FILTER_COMMIT)
fatalx("filter-reponse junk after DATA");
filter_result_junk(reqid);
@@ -667,11 +648,11 @@ lka_filter_process_response(const char *name, const ch
if (parameter == NULL)
fatalx("Missing parameter: %s", line);
- if (strcmp(response, "rewrite") == 0)
+ if (strncmp(response, "rewrite|", 8) == 0)
filter_result_rewrite(reqid, parameter);
- else if (strcmp(response, "reject") == 0)
+ else if (strncmp(response, "reject|", 7) == 0)
filter_result_reject(reqid, parameter);
- else if (strcmp(response, "disconnect") == 0)
+ else if (strncmp(response, "disconnect|", 11) == 0)
filter_result_disconnect(reqid, parameter);
else
fatalx("Invalid directive: %s", line);
httpd: use the host name in SERVER_NAME
currently httpd uses the name specified in the config `server' block
which is not guaranteed to be a valid hostname.
quoting rfc3875:
The SERVER_NAME variable MUST be set to the name of the server host
to which the client request is directed. It is a case-insensitive
hostname or network address. It forms the host part of the
Script-URI.
[...]
A deployed server can have more than one possible value for this
variable, where several HTTP virtual hosts share the same IP
address. In that case, the server would use the contents of the
request's Host header field to select the correct virtual host.
This is important since `block return' with $SERVER_NAME may be
expanded to something invalid, or a fastcgi application that looks at
SERVER_NAME (e.g. gotwebd or php) may receive stuff like
"*.example.com" instead of a proper host name depending on the
configuration.
This introduces some redundancy with HTTP_HOST (which we have since
all the HTTP headers become a fastcgi param in the form of HTTP_*).
I'm relying on the fact that httpd disallows requests without a Host
header, so that in the fastcgi path desc->http_host is not NULL.
thoughts/ok?
diff /usr/src
commit - 0cb70ced01a203d011a17a85e0c08bbb95ac164d
path + /usr/src
blob - 073ab344079c6ee4ae422c6e7ad01c70a7002055
file + usr.sbin/httpd/server_fcgi.c
--- usr.sbin/httpd/server_fcgi.c
+++ usr.sbin/httpd/server_fcgi.c
@@ -333,7 +333,7 @@ server_fcgi(struct httpd *env, struct client *clt)
goto fail;
}
- if (fcgi_add_param(, "SERVER_NAME", srv_conf->name,
+ if (fcgi_add_param(, "SERVER_NAME", desc->http_host,
clt) == -1) {
errstr = "failed to encode param";
goto fail;
blob - 08deda7d5f097683571264da0ff434553801f8f7
file + usr.sbin/httpd/server_http.c
--- usr.sbin/httpd/server_http.c
+++ usr.sbin/httpd/server_http.c
@@ -1245,9 +1245,10 @@ server_expand_http(struct client *clt, const char *val
return (NULL);
}
if (strstr(val, "$SERVER_NAME") != NULL) {
- if ((str = url_encode(srv_conf->name))
-== NULL)
+ if (desc->http_host == NULL)
return (NULL);
+ if ((str = url_encode(desc->http_host)) == NULL)
+ return (NULL);
ret = expand_string(buf, len, "$SERVER_NAME", str);
free(str);
if (ret != 0)
smtpd: sync imsg_to_str()
some imsg types are missing from the big switch in imsg_to_str(), noticed after a report in m...@opensmtpd.org. Tracing shows: : imsg: lka <- dispatcher: IMSG_??? (139) (len=42) (imsg #139 should be IMSG_REPORT_SMTP_FILTER_RESPONSE if I'm counting right.) Instead of checking one by one (they're a lot!) I just copied over the list from smtpd.h and ran an emacs macro. Some entries changed place, but since the list is long I figured this was the best way to keep everything in sync. ok? diff /usr/src commit - af580bd60cce9d8599fddb1cffa69d16b70ae3a7 path + /usr/src blob - 0bd24de8a65d0655a9866c5d3e66ad82a152959a file + usr.sbin/smtpd/smtpd.c --- usr.sbin/smtpd/smtpd.c +++ usr.sbin/smtpd/smtpd.c @@ -2081,19 +2081,22 @@ imsg_to_str(int type) CASE(IMSG_REPORT_SMTP_LINK_CONNECT); CASE(IMSG_REPORT_SMTP_LINK_DISCONNECT); - CASE(IMSG_REPORT_SMTP_LINK_TLS); CASE(IMSG_REPORT_SMTP_LINK_GREETING); CASE(IMSG_REPORT_SMTP_LINK_IDENTIFY); + CASE(IMSG_REPORT_SMTP_LINK_TLS); CASE(IMSG_REPORT_SMTP_LINK_AUTH); - CASE(IMSG_REPORT_SMTP_TX_RESET); CASE(IMSG_REPORT_SMTP_TX_BEGIN); + CASE(IMSG_REPORT_SMTP_TX_MAIL); + CASE(IMSG_REPORT_SMTP_TX_RCPT); CASE(IMSG_REPORT_SMTP_TX_ENVELOPE); + CASE(IMSG_REPORT_SMTP_TX_DATA); CASE(IMSG_REPORT_SMTP_TX_COMMIT); CASE(IMSG_REPORT_SMTP_TX_ROLLBACK); - CASE(IMSG_REPORT_SMTP_PROTOCOL_CLIENT); CASE(IMSG_REPORT_SMTP_PROTOCOL_SERVER); + CASE(IMSG_REPORT_SMTP_FILTER_RESPONSE); + CASE(IMSG_REPORT_SMTP_TIMEOUT); CASE(IMSG_FILTER_SMTP_BEGIN); CASE(IMSG_FILTER_SMTP_END); @@ -2104,6 +2107,7 @@ imsg_to_str(int type) CASE(IMSG_CA_RSA_PRIVENC); CASE(IMSG_CA_RSA_PRIVDEC); CASE(IMSG_CA_ECDSA_SIGN); + default: (void)snprintf(buf, sizeof(buf), "IMSG_??? (%d)", type);
Re: K -> ANSI functions in config(8)
On 2023/06/16 18:08:17 +0200, Theo Buehler wrote:
> Trivial, but config is important, so I'd rather have an ok.
fwiw ok op@ too, this time I remembered to pay attention to the order :)
> Index: scan.l
> ===
> RCS file: /cvs/src/usr.sbin/config/scan.l,v
> retrieving revision 1.24
> diff -u -p -r1.24 scan.l
> --- scan.l9 Jul 2017 14:04:50 - 1.24
> +++ scan.l1 May 2023 19:36:28 -
> @@ -178,8 +178,7 @@ rmoptions return RMOPTIONS;
> * Open the "main" file (conffile).
> */
> int
> -firstfile(fname)
> - const char *fname;
> +firstfile(const char *fname)
> {
>
> if ((yyin = fopen(fname, "r")) == NULL)
> @@ -197,9 +196,7 @@ firstfile(fname)
> * If ateof == 0 then nothing is inserted.
> */
> int
> -include(fname, ateof)
> - const char *fname;
> - int ateof;
> +include(const char *fname, int ateof)
> {
> FILE *fp;
> struct incl *in;
Re: smtpd-filters: swap link-auth fields
On 2023/06/14 16:34:39 +0200, Omar Polo wrote: > For opensmtpd-filter-rspamd I have a corresponding diff that I'll send > to Gilles as it is off-topic for tech@, but here it is too if you want > to play with it: > > https://paste.omarpolo.com/9jtli2w apologize, this one has a stupid typo. I've opend a PR on github with an updated diff. https://github.com/poolpOrg/filter-rspamd/pull/46 sorry for the noise.
smtpd-filters: swap link-auth fields
Hello,
the `link-auth' event hash the user first and the result of the
operation after; this breaks when a username has a '|' character in
it. Since this is triggered by the `auth login' command, anyone could
send a user with a '|' and, depending on the filter used, make smtpd
exit. (if the filter dies, smtpd does too)
This was reported on the OpenSMTPD-portable github repository with
Gilles' opensmtpd-filter-rspamd:
https://github.com/OpenSMTPD/OpenSMTPD/issues/1213
Diff below is straightforward and includes the documentation changes.
I believe link-auth was forgotten in revision 1.61 of lka_filter.c
when the mail-from/rcpt-to events got their fields swapped.
For opensmtpd-filter-rspamd I have a corresponding diff that I'll send
to Gilles as it is off-topic for tech@, but here it is too if you want
to play with it:
https://paste.omarpolo.com/9jtli2w
To reproduce: (there may be quicker ways, this is just the first i
found)
# pkg_add rspamd opensmtpd-filter-rspamd
# rcctl enable rspamd
# rcctl start rspamd
add the rspamd filter to /etc/mail/smtpd.conf
filter "rspamd" proc-exec "filter-rspamd"
listen on lo0 smtps pki localhost auth filter "rspamd"
and try to do a login:
$ nc -c -Tnoverify localhost 465
helo localhost
auth login
b3xw
MTMyNA==
Thanks,
Omar Polo
diff /usr/src
commit - 66c6b79616659a94b04092c9f103e3aa29809704
path + /usr/src
blob - 0c63657be21352fb1f060505250f7a9ef4fc8d8c
file + usr.sbin/smtpd/lka_filter.c
--- usr.sbin/smtpd/lka_filter.c
+++ usr.sbin/smtpd/lka_filter.c
@@ -24,7 +24,7 @@
#include "smtpd.h"
#include "log.h"
-#definePROTOCOL_VERSION"0.6"
+#definePROTOCOL_VERSION"0.7"
struct filter;
struct filter_session;
@@ -1461,7 +1461,7 @@ lka_report_smtp_link_auth(const char *direction, struc
fs->username = xstrdup(username);
}
report_smtp_broadcast(reqid, direction, tv, "link-auth", "%s|%s\n",
- username, result);
+ result, username);
}
void
blob - 313404c111c77b099b3855f43252c26877874b17
file + usr.sbin/smtpd/smtpd-filters.7
--- usr.sbin/smtpd/smtpd-filters.7
+++ usr.sbin/smtpd/smtpd-filters.7
@@ -271,12 +271,9 @@ This event is generated upon disconnection of the clie
the cipher suite used by the session and the cipher strength in bits.
.It Ic link-disconnect
This event is generated upon disconnection of the client.
-.It Ic link-auth : Ar username result
+.It Ic link-auth : Ar result username
This event is generated upon an authentication attempt by the client.
.Pp
-.Ar username
-contains the username used for the authentication attempt.
-.Pp
.Ar result
contains the string
.Dq pass ,
@@ -284,6 +281,9 @@ depending on the result of the authentication attempt.
or
.Dq error
depending on the result of the authentication attempt.
+.Pp
+.Ar username
+contains the username used for the authentication attempt.
.It Ic tx-reset : Op message-id
This event is generated when a transaction is reset.
.Pp
vmd: relax absolute path requirement for configtest
Hello,
after vmd.c rv 1.142 vmd(8) errors when ran with a non-absolute path;
this makes using -n (configtest) slightly more verbose, as the full
path is needed and not just `vmd -n'.
here's an attempt at relaxing the requirement for the -n case only.
since we're not going to run any vm it should be fine, but apologize
if I've missed something.
Thanks,
Omar Polo
diff /usr/src
commit - 66c6b79616659a94b04092c9f103e3aa29809704
path + /usr/src
blob - 86a5132fe224856a3679f1a1d6863b87b561c9d0
file + usr.sbin/vmd/vmd.c
--- usr.sbin/vmd/vmd.c
+++ usr.sbin/vmd/vmd.c
@@ -874,7 +874,7 @@ main(int argc, char **argv)
log_setverbose(env->vmd_verbose);
/* Re-exec from the vmm child process requires an absolute path. */
- if (proc_id == PROC_PARENT && *argv[0] != '/')
+ if (proc_id == PROC_PARENT && *argv[0] != '/' && !env->vmd_noaction)
fatalx("re-exec requires execution with an absolute path");
env->argv0 = argv[0];
Re: Diff for evaluation (WACOM tablet)
Hello, The patch is mangled. It has CRLFs and various 'fancy' unicode spaces so it doesn't apply. Don't know what mail client you're using, but as a last-resort you may try to attach the diff instead of inlining, some MUAs seems to go really out of their way to mangle the content... Thanks, Omar Polo
Re: libtls, smtpd: switch to EC_KEY_METHOD
On 2023/05/25 19:23:48 +0200, Omar Polo wrote:
> As far as I (and grep) can see, smtpd and the part it needs in libtls
> are the only user of ECDSA_METHOD in tree.
>
> What I've understood talking with tb (and apologizes if I'm making
> mistakes) is that ECDSA_METHOD was replaced with EC_KEY_METHOD. "We"
> inherited the former, it got used in smtpd, and then added the latter
> for openssh. smtpd and libtls were never updated to these new shiny
> APIs.
>
> Diff below is 99% gilles' work on OpenSMTPD-portable. I only had to
> tweak EC_KEY_METHOD_get_compute_key() since the compute key function
> has a different signature in LibreSSL than OpenSSL, and some minor
> style nits.
friendly ping
> While I've tested it (on localhost and between vms), and I'm also
> running it on linux and freebsd with OpenSSL 3.1 and 1.1 respectively
> via OpenSMTPD-portable, additional testing on busier mx is greatly
> appreciated. I don't expect regressions however.
I'm also running with this on my backup mx. I've stopped the main mx
a few times to have all the traffic going through the backup one.
Nothing to report :)
more testing is always apreciated.
> To test:
>
> - apply the diff
> - rebuild and reinstall libtls
> - rebuild, reinstall and restart smtpd
forgot to point out one thing, apologies. You need to have a
certificate with an ec key. For acme-client it means having something
like the following:
domain m.example.com {
# note the `ecdsa' on the following line.
domain key "/etc/ssl/private/m.example.com.key" ecdsa
domain full chain certificate "..."
sign with foobar
}
note also that acme-client won't re-generate the private key, so you
may have to delete it prior requesting a new certificate since it
defaults to rsa.
> It doesn't change the libtls ABI (tls_signer_ecdsa_method is internal)
> and the parts it touches are only used by smtpd AFAIK, so no need to
> rebuild anything else.
>
>
> Thanks!
diff 2b0e5a68588bd522338c80975b42b1e5c355be29
3ea882bd0e5cf2ef7f577629af6affb53aef7775
commit - 2b0e5a68588bd522338c80975b42b1e5c355be29
commit + 3ea882bd0e5cf2ef7f577629af6affb53aef7775
blob - 989339dc033f3c231659a9a37946c453d03509da
blob + f6ba5760737d40ec5250a21c0bdcc7446073111f
--- lib/libtls/tls.c
+++ lib/libtls/tls.c
@@ -389,7 +389,7 @@ tls_keypair_setup_pkey(struct tls *ctx, struct tls_key
tls_keypair_setup_pkey(struct tls *ctx, struct tls_keypair *keypair, EVP_PKEY
*pkey)
{
RSA_METHOD *rsa_method;
- ECDSA_METHOD *ecdsa_method;
+ EC_KEY_METHOD *ecdsa_method;
RSA *rsa = NULL;
EC_KEY *eckey = NULL;
int ret = -1;
@@ -427,15 +427,15 @@ tls_keypair_setup_pkey(struct tls *ctx, struct tls_key
break;
case EVP_PKEY_EC:
if ((eckey = EVP_PKEY_get1_EC_KEY(pkey)) == NULL ||
- ECDSA_set_ex_data(eckey, 0, keypair->pubkey_hash) == 0) {
+ EC_KEY_set_ex_data(eckey, 0, keypair->pubkey_hash) == 0) {
tls_set_errorx(ctx, "EC key setup failure");
goto err;
}
if (ctx->config->sign_cb != NULL) {
ecdsa_method = tls_signer_ecdsa_method();
if (ecdsa_method == NULL ||
- ECDSA_set_ex_data(eckey, 1, ctx->config) == 0 ||
- ECDSA_set_method(eckey, ecdsa_method) == 0) {
+ EC_KEY_set_ex_data(eckey, 1, ctx->config) == 0 ||
+ EC_KEY_set_method(eckey, ecdsa_method) == 0) {
tls_set_errorx(ctx, "failed to setup EC key");
goto err;
}
blob - f4c23f64e67f2f45056467dbdffe84960cdc4e2c
blob + f53b6c800941a746425ba01ffe26daf4c236bc37
--- lib/libtls/tls_internal.h
+++ lib/libtls/tls_internal.h
@@ -298,7 +298,7 @@ ECDSA_METHOD *tls_signer_ecdsa_method(void);
int tls_password_cb(char *_buf, int _size, int _rwflag, void *_u);
RSA_METHOD *tls_signer_rsa_method(void);
-ECDSA_METHOD *tls_signer_ecdsa_method(void);
+EC_KEY_METHOD *tls_signer_ecdsa_method(void);
#define TLS_PADDING_NONE 0
#define TLS_PADDING_RSA_PKCS1 1
blob - f6005d3e07ac6423098c9d35f1f03993cd4dfd88
blob + 93777aa3253fdacbc28abc852f8e660fbd882b01
--- lib/libtls/tls_signer.c
+++ lib/libtls/tls_signer.c
@@ -419,26 +419,21 @@ ECDSA_METHOD *
return (NULL);
}
-ECDSA_METHOD *
+EC_KEY_METHOD *
tls_signer_ecdsa_method(void)
{
- static ECDSA_METHOD *ecdsa_method = NULL;
+ static EC_KEY_METHOD *ecdsa_method = NULL;
pthread_mutex_lock(_method_lock);
if (ecdsa_method != NULL)
goto out;
- ecdsa_method = calloc(1, si
Re: hack game: fix launch without /usr/games in path
On 2023/05/31 18:36:42 +0300, Anton Konyahin wrote: > On 31/05, Omar Polo wrote: > > >Agreed. I prefer the second patch too, which I'm reattaching since it > >was mangled (whitespaces; 'patch -l' is not enough, but 'got patch' > >managed to apply it.) > > My bad, I am still not very comfortable with mailing patches, but I will > learn. no problem :-) You can try mailing the diff to yourself and trying to apply it when in doubt. Some MUA seems to make inlining diffs hard. Attaching one could also work. > >Will wait a bit still in case someone disagrees, but I don't really > >see the point in having hack scraping $PATH for finding itself; the > >format wasn't changed since the initial import so I guess we'll be > >fine :-) Committed, thanks!
switch mail.local to getline()
As per subject. While here I couldn't resist simplifying the "From "
check too, although it is indipendent from the rest of the diff.
(could commit separately if preferred.)
ok?
diff /usr/src
commit - 79631e141468cced94e502d777a484fa0eb1f60f
path + /usr/src
blob - 815fe58323c8b912f5676a7d5a29cc90b5bb434c
file + libexec/mail.local/mail.local.c
--- libexec/mail.local/mail.local.c
+++ libexec/mail.local/mail.local.c
@@ -111,9 +111,10 @@ storemail(char *from)
{
FILE *fp = NULL;
time_t tval;
- int fd, eline;
- size_t len;
- char *line, *tbuf;
+ int fd, eline = 1;
+ char *tbuf, *line = NULL;
+ size_t linesize = 0;
+ ssize_t linelen;
if ((tbuf = strdup(_PATH_LOCTMP)) == NULL)
merr(EX_OSERR, "unable to allocate memory");
@@ -125,23 +126,13 @@ storemail(char *from)
(void)time();
(void)fprintf(fp, "From %s %s", from, ctime());
- for (eline = 1, tbuf = NULL; (line = fgetln(stdin, ));) {
- /* We have to NUL-terminate the line since fgetln does not */
- if (line[len - 1] == '\n')
- line[len - 1] = '\0';
- else {
- /* No trailing newline, so alloc space and copy */
- if ((tbuf = malloc(len + 1)) == NULL)
- merr(EX_OSERR, "unable to allocate memory");
- memcpy(tbuf, line, len);
- tbuf[len] = '\0';
- line = tbuf;
- }
+ while ((linelen = getline(, , stdin)) != -1) {
+ if (line[linelen - 1] == '\n')
+ line[linelen - 1] = '\0';
if (line[0] == '\0')
eline = 1;
else {
- if (eline && line[0] == 'F' && len > 5 &&
- !memcmp(line, "From ", 5))
+ if (eline && !strncmp(line, "From ", 5))
(void)putc('>', fp);
eline = 0;
}
@@ -149,7 +140,7 @@ storemail(char *from)
if (ferror(fp))
break;
}
- free(tbuf);
+ free(line);
/* Output a newline; note, empty messages are allowed. */
(void)putc('\n', fp);
smtpd: add missing time.h include
Another boring diff from opensmtpd-portable. After a report of a build fail with some old gcc on RHEL7 / Centos, I noticed that we're lacking the include time.h for time(3), clock_gettime(3) and localtime(3). Diff below adds it in all the missing files. I'm also including sys/time.h in smtpd.h, as noted in event_init(3), since we're including event.h. It wouldn't be an issue to keep this in -portable, but since the header is genuinely missing I'd prefer to have it fixed in base too instead of relying on some other header to include it. diff /usr/src commit - 79631e141468cced94e502d777a484fa0eb1f60f path + /usr/src blob - 61e7b037bd90d2397e98e52cbb68e2436478b9b2 file + usr.sbin/smtpd/bounce.c --- usr.sbin/smtpd/bounce.c +++ usr.sbin/smtpd/bounce.c @@ -22,6 +22,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - 835ab5520eed8d8bbfcce21e9571f07ae89db97c file + usr.sbin/smtpd/control.c --- usr.sbin/smtpd/control.c +++ usr.sbin/smtpd/control.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - c90b60d2bb3ae7046621a4f576a900fe5557ebfd file + usr.sbin/smtpd/enqueue.c --- usr.sbin/smtpd/enqueue.c +++ usr.sbin/smtpd/enqueue.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - 894bf865a7662ce51138168aa0436fde6c9e7b44 file + usr.sbin/smtpd/mda.c --- usr.sbin/smtpd/mda.c +++ usr.sbin/smtpd/mda.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include blob - 05506da1dbef6fb33f23386727977c8e9118f2a8 file + usr.sbin/smtpd/mta.c --- usr.sbin/smtpd/mta.c +++ usr.sbin/smtpd/mta.c @@ -22,6 +22,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - 92f1ec7705d066698a7e24455b86774b86ccbb9c file + usr.sbin/smtpd/mta_session.c --- usr.sbin/smtpd/mta_session.c +++ usr.sbin/smtpd/mta_session.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include blob - e79e3f06be4fdf53e87596a6e10aa79fbe0ffde8 file + usr.sbin/smtpd/queue.c --- usr.sbin/smtpd/queue.c +++ usr.sbin/smtpd/queue.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - 646cd629879ba9c28d6ecaff8be2adef0cea0b7f file + usr.sbin/smtpd/queue_backend.c --- usr.sbin/smtpd/queue_backend.c +++ usr.sbin/smtpd/queue_backend.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - ec539eb9e1123fef50027467d430b94d688232b4 file + usr.sbin/smtpd/queue_fs.c --- usr.sbin/smtpd/queue_fs.c +++ usr.sbin/smtpd/queue_fs.c @@ -28,6 +28,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - 24ca71ca9ee765bcd6f1c05a24749ec6abce2ca8 file + usr.sbin/smtpd/runq.c --- usr.sbin/smtpd/runq.c +++ usr.sbin/smtpd/runq.c @@ -17,6 +17,7 @@ */ #include +#include #include "smtpd.h" blob - fa3b951bc77242dc73dd56d484e576b0ac6ffe8d file + usr.sbin/smtpd/scheduler_ramqueue.c --- usr.sbin/smtpd/scheduler_ramqueue.c +++ usr.sbin/smtpd/scheduler_ramqueue.c @@ -20,6 +20,7 @@ #include #include #include +#include #include "smtpd.h" #include "log.h" blob - 72e13e8fd8d32d748cb64567953d52612a8140ff file + usr.sbin/smtpd/smtp_session.c --- usr.sbin/smtpd/smtp_session.c +++ usr.sbin/smtpd/smtp_session.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include blob - 84663025648861b28f691f99940938000c17872b file + usr.sbin/smtpd/smtpctl.c --- usr.sbin/smtpd/smtpctl.c +++ usr.sbin/smtpd/smtpctl.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include blob - 5949ce05522f4675aabc72042052ec0ef2025881 file + usr.sbin/smtpd/smtpd.c --- usr.sbin/smtpd/smtpd.c +++ usr.sbin/smtpd/smtpd.c @@ -36,6 +36,7 @@ #include #include #include +#include #include #include blob - 65757e517fdbe27cedea42656835d0ac7ef20c6b file + usr.sbin/smtpd/smtpd.h --- usr.sbin/smtpd/smtpd.h +++ usr.sbin/smtpd/smtpd.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include blob - 4a5c692e9508f763e6a2e83839afd6c9994821e6 file + usr.sbin/smtpd/to.c --- usr.sbin/smtpd/to.c +++ usr.sbin/smtpd/to.c @@ -23,6 +23,7 @@ #include #include #include +#include #if IO_TLS #include #endif
Re: hack game: fix launch without /usr/games in path
On 2023/05/29 08:46:02 +0300, Anton Konyahin wrote:
> I can suggest another (much less) patch, which still allows users to
> play hack without path modification. But all this stuff with checking
> saves creating time doesn't looks actual for me, so I keep original
> patch below.
Agreed. I prefer the second patch too, which I'm reattaching since it
was mangled (whitespaces; 'patch -l' is not enough, but 'got patch'
managed to apply it.)
Will wait a bit still in case someone disagrees, but I don't really
see the point in having hack scraping $PATH for finding itself; the
format wasn't changed since the initial import so I guess we'll be
fine :-)
diff /usr/src
commit - 79631e141468cced94e502d777a484fa0eb1f60f
path + /usr/src
blob - 4abe525065dddaabea09e3a050b3a3db78a10e39
file + games/hack/hack.bones.c
--- games/hack/hack.bones.c
+++ games/hack/hack.bones.c
@@ -139,17 +139,15 @@ getbones(void)
int
getbones(void)
{
- int fd,x,y,ok;
+ int fd,x,y;
if(rn2(3)) return(0); /* only once in three times do we find bones */
bones[6] = '0' + dlevel/10;
bones[7] = '0' + dlevel%10;
if((fd = open(bones, O_RDONLY)) == -1) return(0);
- if((ok = uptodate(fd)) != 0){
- getlev(fd, 0, dlevel);
- for(x = 0; x < COLNO; x++) for(y = 0; y < ROWNO; y++)
- levl[x][y].seen = levl[x][y].new = 0;
- }
+ getlev(fd, 0, dlevel);
+ for(x = 0; x < COLNO; x++) for(y = 0; y < ROWNO; y++)
+ levl[x][y].seen = levl[x][y].new = 0;
(void) close(fd);
#ifdef WIZARD
if(!wizard) /* duvel!frans: don't remove bones while debugging */
@@ -158,5 +156,5 @@ getbones(void)
pline("Cannot unlink %s .", bones);
return(0);
}
- return(ok);
+ return(1);
}
blob - 4fa5eafc297d74a98cd106aadcc3ab67c5783b61
file + games/hack/hack.h
--- games/hack/hack.h
+++ games/hack/hack.h
@@ -681,7 +681,6 @@ int uptodate(int);
int night(void);
int midnight(void);
void gethdate(char *);
-int uptodate(int);
void getlock(void);
#ifdef MAIL
void getmailstatus(void);
blob - 51a066600dbd8313b8fca95f7ef761e08c03e71d
file + games/hack/hack.main.c
--- games/hack/hack.main.c
+++ games/hack/hack.main.c
@@ -103,7 +103,6 @@ main(int argc, char **argv)
int
main(int argc, char **argv)
{
- extern char *__progname;
int fd;
#ifdef CHDIR
char *dir;
@@ -183,15 +182,6 @@ main(int argc, char **argv)
u.ux = FAR; /* prevent nscr() */
(void) signal(SIGHUP, hackhangup);
- /*
-* Find the creation date of this game,
-* so as to avoid restoring outdated savefiles.
-*/
- gethdate(__progname);
-
- /*
-* We cannot do chdir earlier, otherwise gethdate will fail.
-*/
#ifdef CHDIR
chdirx(dir,1);
#endif
@@ -298,8 +288,7 @@ main(int argc, char **argv)
setftty();
(void) snprintf(SAVEF, sizeof SAVEF, "save/%u%s", getuid(), plname);
regularize(SAVEF+5);/* avoid . or / in name */
- if((fd = open(SAVEF, O_RDONLY)) >= 0 &&
- (uptodate(fd) || unlink(SAVEF) == 666)) {
+ if((fd = open(SAVEF, O_RDONLY)) >= 0) {
(void) signal(SIGINT,done1);
pline("Restoring old save file...");
(void) fflush(stdout);
blob - 96a9ca84d2f783477114095339b566d36cb4d17c
file + games/hack/hack.unix.c
--- games/hack/hack.unix.c
+++ games/hack/hack.unix.c
@@ -154,53 +154,8 @@ struct stat buf, hbuf;
return(getlt()->tm_hour == 0);
}
-struct stat buf, hbuf;
+struct stat buf;
-void
-gethdate(char *name)
-{
- char *p, *np, *path;
- char filename[PATH_MAX];
-
- if (strchr(name, '/') != NULL || (p = getenv("PATH")) == NULL)
- p = "";
- np = path = strdup(p); /* Make a copy for strsep. */
- if (path == NULL)
- err(1, NULL);
-
- for (;;) {
- if ((p = strsep(, ":")) == NULL)
- break;
- if (*p == '\0') /* :: */
- (void) strlcpy(filename, name, sizeof filename);
- else
- (void) snprintf(filename, sizeof filename,
- "%s/%s", p, name);
-
- if (stat(filename, ) == 0) {
- free(path);
- return;
- }
- }
- error("Cannot get status of %s.",
- (p = strrchr(name, '/')) ? p+1 : name);
- free(path);
-}
-
-int
-uptodate(int fd)
-{
- if(fstat(fd, )) {
- pline("Cannot get status of saved level? ");
- return(0);
- }
- if(buf.st_mtime < hbuf.st_mtime) {
- pline("Saved level is out of date. ");
- return(0);
- }
- return(1);
-}
-
/* see whether we should throw away this xlock file */
static int
veryold(int fd)
Re: ksh, test: test -t file descriptor isn't optional
sorry for the delay, this is another mail that I meant to take a look
earlier...
On 2023/05/29 16:36:45 +, Lucas wrote:
> Ping.
>
> > Hi tech@,
> >
> > Both test.1 and ksh.1 (under the non-POSIX compatibility flag) state
> > that `test -t` will default to test whether fd 1 is a TTY if the
> > argument is omitted. This isn't the case, and both treat `-t` as the
> > equivalent of `test -n -t`, ie, test if `-t` is a non-empty string,
> > trivially true.
> >
> > It's easy to see it in `test`: it exits right away in switch(argc).
agreed. The "(default 1)" in test.1 is completely wrong.
> > `ksh` is a bit more difficult to follow: builtins eventually call c_test
> > in c_test.c. For `test -t`, c_test will be called with wp being "test",
> > "-t". It'll eventually call test_parse, with the test environment set
> > with
> >
> > te.pos.wp = wp + 1;
> > te.wp_end = wp + argc;
> >
> > For this particular case, argc is 2. Bearing that in mind, test_parse
> > will make a call stack of test_aexpr -> test_oexpr -> test_nexpr ->
> > test_primary. The first 2 ifs are skipped (asuming no TEF_ERROR is set)
> > and the next if, which would handle `test -t` gracefully, isn't entered:
> > one of the && operands, >pos.wp[1] < te->wp_end, is false.
> > Afterwards, `-t` is evaled as TO_SNTZE, ie if it's a non-empty string.
Agree on this too.
> > The following diff amends the manpages, removing the "file descriptor
> > is optional" parts, and removes the unused machinery of bin/ksh/c_test.c
> > to deal with an optional argument in `test -t`,
for what is worth I agree with you. It's a historic behaviour that
has been broken for some time already and noone seems to have noticed,
I think it could go away.
> > together with a small
> > rewrite in test_eval TO_FILTT case, as it was a bit too difficult for me
> > to read: it seems that, for legacy reason (haven't looked at the code
> > history), opnd1 could be NULL for TO_FILTT, the only test with such
> > behaviour. My understanding is that ptest_getopnd avoids that by
> > returning "1". So, opnd1 can't be NULL. bi_getn writes to res, but took
> > me a while to understand that looking only at the conditional call of
> > bi_getn in the if condition. Finally, for some reason, is opnd1 managed
> > to be NULL, isatty(0) is called. I believe that the intention was to do
> >
> > res = opnd1 ? isatty(res) : 0;
> >
> > instead. I think the proposed rewrite is easier to follow.
I think that the rewrite is more complex than needed. I'm attaching a
slightly revised diff that is closer to the current code.
> > Regress is happy, although nothing in there tests `test -t` or `test -t
> > n`. Existing scripts shouldn't break with this change: `test -t` will
> > keep being true, whether fd 1 is a TTY or not. The diff can be further
> > split on man changes and code changes if needed.
> >
> > btw, the easy test for `test -t` being wrong, whether under POSIX compat
> > or not, is
> >
> > $ test -t 1; echo $? # 1 is TTY in an interactive session
> > 0
> > $ test -t 1 >&-; echo $? # 1 isn't a TTY because it's closed
> > 1
> > $ test -t >&-; echo $?
> > 0
bash and ash behave the same out-of-the-box (i.e. without any eventual
posix flag set.)
I'd prefer some more feedback on this before going on.
diff /usr/src
commit - 15eb8637ab039139400e655284e2e2d8ca898a03
path + /usr/src
blob - 7038a52bfa432d515d9683187930407c4d6bd6d5
file + bin/ksh/c_test.c
--- bin/ksh/c_test.c
+++ bin/ksh/c_test.c
@@ -156,12 +156,6 @@ c_test(char **wp)
}
if (argc == 1) {
opnd1 = (*te.getopnd)(, TO_NONOP, 1);
- /* Historically, -t by itself test if fd 1
-* is a file descriptor, but POSIX says its
-* a string test...
-*/
- if (!Flag(FPOSIX) && strcmp(opnd1, "-t") == 0)
- break;
res = (*te.eval)(, TO_STNZE, opnd1,
NULL, 1);
if (invert & 1)
@@ -271,14 +265,11 @@ test_eval(Test_env *te, Test_op op, const char *opnd1,
case TO_FILGZ: /* -s */
return stat(opnd1, ) == 0 && b1.st_size > 0L;
case TO_FILTT: /* -t */
- if (opnd1 && !bi_getn(opnd1, )) {
+ if (!bi_getn(opnd1, )) {
te->flags |= TEF_ERROR;
- res = 0;
- } else {
- /* generate error if in FPOSIX mode? */
- res = isatty(opnd1 ? res : 0);
+ return 0
}
- return res;
+ return isatty(res);
case TO_FILUID: /* -O */
return stat(opnd1, ) == 0 && b1.st_uid == ksheuid;
case TO_FILGID: /* -G */
@@ -527,7 +518,7 @@
Re: Bail out on "id -R user"
On 2023/05/29 16:37:39 +, Lucas wrote:
> Ping.
>
> > According to both usage() and id.1, "id -R" doesn't accept any
> > positional arguments. This diff makes program behave like that.
sorry for the delay, i wanted to take a look earlier.
fwiw I agree. Although it doesn't makes much sense to pass an
argument to id -R, not failing may give the wrong impression.
ok op@ (i'll wait a bit before commiting this in case someone
disagrees)
> diff refs/heads/master refs/heads/id-R-usage
> commit - 55055d619d36cc45f8c6891404c51cd405214e86
> commit + 214ec9c042895b8482378b6ee43530ce4ffe9e21
> blob - 30e2c58e088b69dba98577693e37dd961b4eadc4
> blob + 1f4b678597065fb4243f660b4f66040021c56895
> --- usr.bin/id/id.c
> +++ usr.bin/id/id.c
> @@ -128,6 +128,8 @@ main(int argc, char *argv[])
> usage();
>
> if (Rflag) {
> + if (argc != 0)
> + usage();
> printf("%d\n", getrtable());
> exit(0);
> }
Re: make: slightly better diagnostic
On 2023/05/28 18:09:00 +0200, Marc Espie wrote:
> Here's a slightly more specific diff avoiding useless stat(2)
looks fine to me, and in ports it produces a nice error message:
% chmod 600 Makefile
% make
[...]
make: don't know how to make do-extract
Stop in .
Makefile(s) that couldn't be read:
Makefile: Permission denied
so fwiw ok op@
one comment and a minor style nit below
> [...]
> Index: main.c
> ===
> RCS file: /cvs/src/usr.bin/make/main.c,v
> retrieving revision 1.129
> diff -u -p -r1.129 main.c
> --- main.c17 Jan 2023 13:03:22 - 1.129
> +++ main.c28 May 2023 16:07:45 -
> [...]
> +static FILE *
> +open_makefile(const char *fname)
> +{
> + FILE *stream;
> + struct unreadable *u;
> +
> + stream = fopen(fname, "r");
> + if (stream != NULL)
> + return stream;
> +
> + if (errno != ENOENT) {
> + u = emalloc(sizeof *u);
> + u->fname = strdup(fname);
it's unlikely, but strdup can fail here clobbering errno and leaving
fname NULL. estrdup() should be used instead.
> + u->errcode = errno;
> + Lst_AtEnd(, u);
> + }
> +
> + return NULL;
> +}
> [...]
> @@ -866,7 +910,14 @@ ReadMakefile(void *p, void *q)
> name = Dir_FindFile(fname, userIncludePath);
> if (!name)
> name = Dir_FindFile(fname, systemIncludePath);
> - if (!name || !(stream = fopen(name, "r")))
> + if (!name)
> + return false;
> + /* do not try to open a file we already have, so that
> + * dump_unreadable() yields non-confusing results.
> + */
other multiline comments are spelled as
/*
* do not try to open a file we already have, so that
* dump_unreadable() yields non-confusing results.
*/
and btw thanks for adding this comment, figuring out why this was
needed wouldn't have been straightforward to me.
> + if (strcmp(name, fname) == 0)
> + return false;
> + if ((stream = open_makefile(name)) == NULL)
> return false;
> fname = name;
> /*
Re: make: slightly better diagnostic
On 2023/05/28 13:16:34 +0200, Marc Espie wrote:
> Turns out that, due to the search rules we use, make
> is mostly silent in case it couldn't read a Makefile
>
> The following patch lets it track the makefilenames that
> do exist, but that it wasn't able to open, so that
> a post-mortem can include these.
>
>
> Not sure if other use-cases could also use the post-mortem
>
> The tweak to ReadMakefiles is to avoid pushing the same
> path name twice.
>
> Using Lst_Pop ensures this diagnostic only ever occurs once,
> in case we find other places we might want to stick this.
>
> (and realizing that, I should probably add comments in both
> places instead of explaining this through email)
It would indeed be nice to have a "post-mortem" message with the
Makefiles that couldn't be opened. it would have saved me some
headaches in the past with ports that have too tight permissions.
Diff below reads fine to me, just one question inline, but it doesn't
seem to catch all the situations, consider:
% mkdir /tmp/foo && cd /tmp/foo
/tmp/foo
% touch Makefile
% chmod 000 Makefile
% make
make: no target to make.
% make all
make: don't know how to make all
Stop in /tmp/foo
Makefile(s) that couldn't be read: Makefile
a bare `make' don't print the nice error message, trying with a target
instead does.
> Index: main.c
> ===
> RCS file: /cvs/src/usr.bin/make/main.c,v
> retrieving revision 1.129
> diff -u -p -r1.129 main.c
> --- main.c17 Jan 2023 13:03:22 - 1.129
> +++ main.c28 May 2023 11:12:48 -
> [...]
> +static FILE *
> +open_makefile(const char *fname)
> +{
> + FILE *stream;
> + struct stat buffer;
> +
> + stream = fopen(fname, "r");
> + if (stream != NULL)
> + return stream;
> +
> + if (stat(fname, ) == 0) {
> + Lst_AtEnd(, strdup(fname));
> + }
Why the stat? Can't we just check errno for EACCES instead? EACCES
is also returned when search permission is denied for a component of
the path prefix, which would be a difference but maybe a nice one?
> + return NULL;
> +}
> +
libtls, smtpd: switch to EC_KEY_METHOD
As far as I (and grep) can see, smtpd and the part it needs in libtls
are the only user of ECDSA_METHOD in tree.
What I've understood talking with tb (and apologizes if I'm making
mistakes) is that ECDSA_METHOD was replaced with EC_KEY_METHOD. "We"
inherited the former, it got used in smtpd, and then added the latter
for openssh. smtpd and libtls were never updated to these new shiny
APIs.
Diff below is 99% gilles' work on OpenSMTPD-portable. I only had to
tweak EC_KEY_METHOD_get_compute_key() since the compute key function
has a different signature in LibreSSL than OpenSSL, and some minor
style nits.
While I've tested it (on localhost and between vms), and I'm also
running it on linux and freebsd with OpenSSL 3.1 and 1.1 respectively
via OpenSMTPD-portable, additional testing on busier mx is greatly
appreciated. I don't expect regressions however.
To test:
- apply the diff
- rebuild and reinstall libtls
- rebuild, reinstall and restart smtpd
It doesn't change the libtls ABI (tls_signer_ecdsa_method is internal)
and the parts it touches are only used by smtpd AFAIK, so no need to
rebuild anything else.
Thanks!
diff a6995f7d4e4b475f514b46014b476ba2fb99e6ca
15eb8637ab039139400e655284e2e2d8ca898a03
commit - a6995f7d4e4b475f514b46014b476ba2fb99e6ca
commit + 15eb8637ab039139400e655284e2e2d8ca898a03
blob - 989339dc033f3c231659a9a37946c453d03509da
blob + f6ba5760737d40ec5250a21c0bdcc7446073111f
--- lib/libtls/tls.c
+++ lib/libtls/tls.c
@@ -389,7 +389,7 @@ tls_keypair_setup_pkey(struct tls *ctx, struct tls_key
tls_keypair_setup_pkey(struct tls *ctx, struct tls_keypair *keypair, EVP_PKEY
*pkey)
{
RSA_METHOD *rsa_method;
- ECDSA_METHOD *ecdsa_method;
+ EC_KEY_METHOD *ecdsa_method;
RSA *rsa = NULL;
EC_KEY *eckey = NULL;
int ret = -1;
@@ -427,15 +427,15 @@ tls_keypair_setup_pkey(struct tls *ctx, struct tls_key
break;
case EVP_PKEY_EC:
if ((eckey = EVP_PKEY_get1_EC_KEY(pkey)) == NULL ||
- ECDSA_set_ex_data(eckey, 0, keypair->pubkey_hash) == 0) {
+ EC_KEY_set_ex_data(eckey, 0, keypair->pubkey_hash) == 0) {
tls_set_errorx(ctx, "EC key setup failure");
goto err;
}
if (ctx->config->sign_cb != NULL) {
ecdsa_method = tls_signer_ecdsa_method();
if (ecdsa_method == NULL ||
- ECDSA_set_ex_data(eckey, 1, ctx->config) == 0 ||
- ECDSA_set_method(eckey, ecdsa_method) == 0) {
+ EC_KEY_set_ex_data(eckey, 1, ctx->config) == 0 ||
+ EC_KEY_set_method(eckey, ecdsa_method) == 0) {
tls_set_errorx(ctx, "failed to setup EC key");
goto err;
}
blob - f4c23f64e67f2f45056467dbdffe84960cdc4e2c
blob + f53b6c800941a746425ba01ffe26daf4c236bc37
--- lib/libtls/tls_internal.h
+++ lib/libtls/tls_internal.h
@@ -298,7 +298,7 @@ ECDSA_METHOD *tls_signer_ecdsa_method(void);
int tls_password_cb(char *_buf, int _size, int _rwflag, void *_u);
RSA_METHOD *tls_signer_rsa_method(void);
-ECDSA_METHOD *tls_signer_ecdsa_method(void);
+EC_KEY_METHOD *tls_signer_ecdsa_method(void);
#define TLS_PADDING_NONE 0
#define TLS_PADDING_RSA_PKCS1 1
blob - f6005d3e07ac6423098c9d35f1f03993cd4dfd88
blob + 93777aa3253fdacbc28abc852f8e660fbd882b01
--- lib/libtls/tls_signer.c
+++ lib/libtls/tls_signer.c
@@ -419,26 +419,21 @@ ECDSA_METHOD *
return (NULL);
}
-ECDSA_METHOD *
+EC_KEY_METHOD *
tls_signer_ecdsa_method(void)
{
- static ECDSA_METHOD *ecdsa_method = NULL;
+ static EC_KEY_METHOD *ecdsa_method = NULL;
pthread_mutex_lock(_method_lock);
if (ecdsa_method != NULL)
goto out;
- ecdsa_method = calloc(1, sizeof(*ecdsa_method));
+ ecdsa_method = EC_KEY_METHOD_new(NULL);
if (ecdsa_method == NULL)
goto out;
- ecdsa_method->ecdsa_do_sign = tls_ecdsa_do_sign;
- ecdsa_method->name = strdup("libtls ECDSA method");
- if (ecdsa_method->name == NULL) {
- free(ecdsa_method);
- ecdsa_method = NULL;
- }
+ EC_KEY_METHOD_set_sign(ecdsa_method, NULL, NULL, tls_ecdsa_do_sign);
out:
pthread_mutex_unlock(_method_lock);
blob - c0c918601e741019515da6c83a6874fb9f552a1a
blob + f5d81174b597f8368b45719833d92772e49e78db
--- usr.sbin/smtpd/ca.c
+++ usr.sbin/smtpd/ca.c
@@ -47,10 +47,17 @@ static int rsae_keygen(RSA *, int, BIGNUM *, BN_GENCB
static int rsae_init(RSA *);
static int rsae_finish(RSA *);
static int rsae_keygen(RSA *, int, BIGNUM *, BN_GENCB *);
+static int ecdsae_keygen(EC_KEY *);
+static int ecdsae_compute_key(void *, size_t, const EC_POINT *, EC_KEY *,
+ void *(*)(const void *,
smtpd.h: drop two unused define
both values have been unused for quite some time. last PROC_COUNT use was removed in 'Implement the fork+exec pattern in smtpd' by eric@ in 2016. I've checked the other #defines and they seem to be all used, except these two. ok? diff /usr/src commit - 6f5cff98d90c274a5222db1a9bd17d5c26da7920 path + /usr/src blob - 125a6a5dfbe176065b9fb3835363ed47d32a94ae file + usr.sbin/smtpd/smtpd.h --- usr.sbin/smtpd/smtpd.h +++ usr.sbin/smtpd/smtpd.h @@ -45,10 +45,7 @@ #define CONF_FILE "/etc/mail/smtpd.conf" #define MAILNAME_FILE "/etc/mail/mailname" -#define CA_FILE "/etc/ssl/cert.pem" -#define PROC_COUNT 7 - #define MAX_HOPS_COUNT 100 #defineDEFAULT_MAX_BODY_SIZE (35*1024*1024)
Re: userdel: remove login group for =uid
On 2023/05/19 10:24:58 -0600, Todd C. Miller wrote:
> If /etc/usermgmt.conf has a line like:
>
> group =uid
>
> where a new user's group ID in the passwd file is the same as their
> user ID, remove that group when the user is removed. The group is
> only removed if it matches the login name, has a gid that matches
> the user's uid, and has no other members.
>
> This makes our userdel(8) behave more like the version on other
> systems.
>
> Opinions? This is something that has always bothered me and can
> result in uid/gid mismatches if you remove a user, then re-add them
> without removing the login group first.
I have been bitten by this too and later been (pleasently) surprised
when found that on other systems removing a users removes the group
too. It makes sense to me.
> Thoughts or strong opinions?
fwiw i like the change, ok for me.
However I never did anything "crazy" with usermgmt.conf(5) so don't
know if this could break existing setups, althought I highly doubt:
the worst could be getting some errors from groupdel(8) due to an
already deleted group.
As Aisha pointed out, pkg_delete hints could be updated too.
one minor style nit below, hope you'll excuse my nitpicking.
> [...]
> @@ -1366,6 +1366,15 @@ rm_user_from_groups(char *login_name)
> warnx("Malformed entry `%s'. Skipping", buf);
> continue;
> }
> + if (rm_login_group && strncmp(buf, login_name, login_len) == 0
> + && buf[login_len] == ':') {
> + /* remove login group if empty or user is only member */
> + if (*cp == '\n')
> + continue;
> + if (strncmp(cp, login_name, login_len) == 0 &&
> + cp [login_len] == '\n')
extra space
> + continue;
> + }
sticky(8): mark S_ISVTX as Dv
It makes `man -k any=S_ISVTX' slightly more useful by pointing at sticky(8) too other than strmode(3); may help if someone (like me :-) forgot about sticky(8) files. ok? diff /usr/src commit - beb1c6c70ae1e2a18abd99274d326b36106135ad path + /usr/src blob - 5577ca04b51f728d40b28bafa2d8de891d2fdac0 file + share/man/man8/sticky.8 --- share/man/man8/sticky.8 +++ share/man/man8/sticky.8 @@ -39,7 +39,7 @@ A special file mode, called the .Sh DESCRIPTION A special file mode, called the .Em sticky bit -(mode S_ISVTX), +.Pq mode Dv S_ISVTX , is used to indicate special treatment for files and directories. See .Xr chmod 2
smtpd.conf.5: fix markup for action maildir
it's currently rendered as maildir [pathname [junk]] whereas it really is maildir [pathname] [junk] i.e. both the path and `junk' are optional but indipendently so. it's quite clear from the grammar in parse.y. ok? diff /usr/src commit - beb1c6c70ae1e2a18abd99274d326b36106135ad path + /usr/src blob - c2ee0699e666f6234d54033fceeb4516b608fa65 file + usr.sbin/smtpd/smtpd.conf.5 --- usr.sbin/smtpd/smtpd.conf.5 +++ usr.sbin/smtpd/smtpd.conf.5 @@ -128,7 +128,7 @@ local user in the LMTP session as RCPT TO. might be specified to use the recipient email address (after expansion) instead of the local user in the LMTP session as RCPT TO. -.It Cm maildir Op Ar pathname Op Cm junk +.It Cm maildir Oo Ar pathname Oc Op Cm junk Deliver the message to the maildir in .Ar pathname if specified, or by default to
Re: user: handle paths with whitespace / metacharacters
On 2023/05/18 11:28:58 -0600, Todd C. Miller wrote:
> On Thu, 18 May 2023 18:13:57 +0200, Omar Polo wrote:
> > > + if (ret != 0)
> > > + warnx("[Warning] unable to run `%s'", buf);
> >
> > more than "unable to run" I'd say "failed to run" or "command
> > failed" / "exited with nonzero status".
>
> How about "failed with status N"? It can be useful to have the
> exit status since that may indicate the source of the error (though
> perhaps not so much in this case).
I couldn't have come up with something better.
> Updated diff follows.
ok for me.
Thanks!
Re: user: handle paths with whitespace / metacharacters
On 2023/05/18 09:11:59 -0600, Todd C. Miller wrote:
> The way user(8) runs commands via system(3) is fragile. It does
> not correctly handle paths with whitespace or shell metacharacters.
> Rather than try to quote everything (which is also fragile) I think
> it is safest to just exec the commands directly.
when I saw that the previous change was to remove the only "cd xyz &&
cmd" I hoped that the follow up was to use exec instead of system :)
> OK?
verified that it works as intended. -v is actually nicer now, since
it logs the commands it executes (which is expected as per man page
but not as per previous behaviour.)
for what is worth ok op@ (with or without nits below)
> - todd
>
> Index: usr.sbin/user/user.c
> ===
> RCS file: /cvs/src/usr.sbin/user/user.c,v
> retrieving revision 1.130
> diff -u -p -u -r1.130 user.c
> --- usr.sbin/user/user.c 16 May 2023 21:28:46 - 1.130
> +++ usr.sbin/user/user.c 18 May 2023 15:10:52 -
> @@ -31,6 +31,7 @@
>
> #include
> #include
> +#include
>
> #include
> #include
> @@ -42,7 +43,7 @@
> #include
> #include
> #include
> -#include
> +#include
existing code just used 0 and 1 (e.g. creategid.) lone zeros as
arguments are not really readable, but still I can't make myself
liking stdbool. not that my tastes matters, but it seems to be used
very sparsingly in usr.sbin
> #include
> #include
> #include
> @@ -103,6 +104,10 @@ enum {
> F_ACCTUNLOCK= 0x1
> };
>
> +/* bit flags for runas() */
> +#define RUNAS_DUP_DEVNULL0x01
> +#define RUNAS_IGNORE_EXITVAL 0x02
another nit: the other flags are defined within an enums.
> [...]
> -/* a replacement for system(3) */
> +/* run the given command with optional arguments as the specified uid */
> static int
> -asystem(const char *fmt, ...)
> +runas(const char *path, const char *const argv[], uid_t uid, int flags)
> {
> [...]
> + default:
> + while (waitpid(child, , 0) == -1) {
> + if (errno != EINTR)
> + err(EXIT_FAILURE, "waitpid");
> + }
> + if (WIFSIGNALED(status)) {
> + ret = WTERMSIG(status);
> + warnx("[Warning] `%s' killed by signal %d", buf, ret);
> + ret |= 128;
> + } else {
> + if (!(flags & RUNAS_IGNORE_EXITVAL))
> + ret = WEXITSTATUS(status);
> + if (ret != 0)
> + warnx("[Warning] unable to run `%s'", buf);
more than "unable to run" I'd say "failed to run" or "command
failed" / "exited with nonzero status".
"unable to run" makes me think that it failed to exec the
program, rather than it failed at runtime.
> + }
> + return ret;
> }
> - return ret;
> +}
> +
> +/* run the given command with optional arguments */
> +static int
> +run(const char *path, const char *const argv[])
> +{
> + return runas(path, argv, -1, false);
> }
Re: useradd: use "cp" instead of "pax" to copy dot files
On 2023/05/16 11:39:17 -0600, Todd C. Miller wrote:
> We can just use "cp -a skeldir/. homedir" to copy the skeleton dot
> files to the new user's homedir. There's no good reason to use pax
> when cp will do and this will simplify a future commit of mine.
hard links are handled differently, but I doubt it's an issue here.
The -v output also changes slightly: before was just the list of
copied files, now a series of "source -> destination" lines. don't
think anyone is relying on this however, so don't matter.
fwiw OK for me and +1 for the style(9) nit :)
(i'm curious about the future diff)
> - todd
>
> Index: usr.sbin/user/user.c
> ===
> RCS file: /cvs/src/usr.sbin/user/user.c,v
> retrieving revision 1.129
> diff -u -p -u -r1.129 user.c
> --- usr.sbin/user/user.c 15 May 2023 17:00:24 - 1.129
> +++ usr.sbin/user/user.c 16 May 2023 17:30:18 -
> @@ -171,7 +171,7 @@ enum {
> #define MKDIR"/bin/mkdir"
> #define MV "/bin/mv"
> #define NOLOGIN "/sbin/nologin"
> -#define PAX "/bin/pax"
> +#define CP "/bin/cp"
> #define RM "/bin/rm"
>
> #define UNSET_INACTIVE "Null (unset)"
> @@ -311,8 +311,8 @@ copydotfiles(char *skeldir, char *dir)
> if (n == 0) {
> warnx("No \"dot\" initialisation files found");
> } else {
> - (void) asystem("cd %s && %s -rw -pe %s . %s",
> - skeldir, PAX, (verbose) ? "-v" : "", dir);
> + (void) asystem("%s -a %s %s/. %s",
> + CP, (verbose) ? "-v" : "", skeldir, dir);
> }
> return n;
> }
smtpd: some fatal -> fatalx
while debugging a pebkac in -portable, I noticed that in various
places we use fatal() for libtls failures. errno doesn't generally
contains anything useful after libtls functions, and in most it's
explicitly cleared to avoid misuse.
just to provide a quick example, with `listen on ... ciphers foobar':
% doas smtpd -d
info: OpenSMTPD 7.0.0 starting
dispatcher: no ciphers for 'foobar': No such file or directory
smtpd: process dispatcher socket closed
So change most of them to fatalx which doesn't append errno. While
here I'm also logging the actual error, via tls_config_error() or
tls_error(), that before was missing.
tls_config_new(), tls_server() and tls_client() failures are still
logged with fatal(), which I believe it's correct.
ok?
diff /usr/src
commit - 27d3d13aceb86ba91128d3f12ca9fc893d83fa86
path + /usr/src
blob - dbcf2c0158186f1a3077c25d746c9a8d7a8ad9d0
file + usr.sbin/smtpd/mta.c
--- usr.sbin/smtpd/mta.c
+++ usr.sbin/smtpd/mta.c
@@ -489,38 +489,41 @@ mta_setup_dispatcher(struct dispatcher *dispatcher)
if (remote->tls_ciphers)
ciphers = remote->tls_ciphers;
if (ciphers && tls_config_set_ciphers(config, ciphers) == -1)
- fatal("%s", tls_config_error(config));
+ fatalx("%s", tls_config_error(config));
if (remote->tls_protocols) {
if (tls_config_parse_protocols(,
remote->tls_protocols) == -1)
- fatal("failed to parse protocols \"%s\"",
+ fatalx("failed to parse protocols \"%s\"",
remote->tls_protocols);
if (tls_config_set_protocols(config, protos) == -1)
- fatal("%s", tls_config_error(config));
+ fatalx("%s", tls_config_error(config));
}
if (remote->pki) {
pki = dict_get(env->sc_pki_dict, remote->pki);
if (pki == NULL)
- fatal("client pki \"%s\" not found ", remote->pki);
+ fatalx("client pki \"%s\" not found", remote->pki);
tls_config_set_dheparams(config, dheparams[pki->pki_dhe]);
tls_config_use_fake_private_key(config);
if (tls_config_set_keypair_mem(config, pki->pki_cert,
pki->pki_cert_len, NULL, 0) == -1)
- fatal("tls_config_set_keypair_mem");
+ fatalx("tls_config_set_keypair_mem: %s",
+ tls_config_error(config));
}
if (remote->ca) {
ca = dict_get(env->sc_ca_dict, remote->ca);
if (tls_config_set_ca_mem(config, ca->ca_cert, ca->ca_cert_len)
== -1)
- fatal("tls_config_set_ca_mem");
+ fatalx("tls_config_set_ca_mem: %s",
+ tls_config_error(config));
}
else if (tls_config_set_ca_file(config, tls_default_ca_cert_file())
== -1)
- fatal("tls_config_set_ca_file");
+ fatalx("tls_config_set_ca_file: %s",
+ tls_config_error(config));
if (remote->tls_verify) {
tls_config_verify(config);
blob - a9b7d48c8a5fe3e1af6d32429556f09b7579583b
file + usr.sbin/smtpd/smtp.c
--- usr.sbin/smtpd/smtp.c
+++ usr.sbin/smtpd/smtp.c
@@ -166,14 +166,14 @@ smtp_setup_listener_tls(struct listener *l)
if (l->tls_ciphers)
ciphers = l->tls_ciphers;
if (ciphers && tls_config_set_ciphers(config, ciphers) == -1)
- fatal("%s", tls_config_error(config));
+ fatalx("%s", tls_config_error(config));
if (l->tls_protocols) {
if (tls_config_parse_protocols(, l->tls_protocols) == -1)
- fatal("failed to parse protocols \"%s\"",
+ fatalx("failed to parse protocols \"%s\"",
l->tls_protocols);
if (tls_config_set_protocols(config, protos) == -1)
- fatal("%s", tls_config_error(config));
+ fatalx("%s", tls_config_error(config));
}
pki = l->pki[0];
@@ -181,7 +181,8 @@ smtp_setup_listener_tls(struct listener *l)
fatal("no pki defined");
if (tls_config_set_dheparams(config, dheparams[pki->pki_dhe]) == -1)
- fatal("tls_config_set_dheparams");
+ fatalx("tls_config_set_dheparams: %s",
+ tls_config_error(config));
tls_config_use_fake_private_key(config);
for (i = 0; i < l->pkicount; i++) {
@@ -189,11 +190,13 @@ smtp_setup_listener_tls(struct listener *l)
if (i == 0) {
if (tls_config_set_keypair_mem(config, pki->pki_cert,
pki->pki_cert_len, NULL, 0) == -1)
- fatal("tls_config_set_keypair_mem");
+
Re: smtpd: nits to reduce the diff with -portable
On 2023/05/15 07:34:03 -0600, "Todd C. Miller" wrote: > On Mon, 15 May 2023 13:54:35 +0200, Omar Polo wrote: > > > almost always (cast)var. I've adjusted the spacing in the line I was > > touching, grepping for common types I could only find one instance of > > a '(long long) src' in envelope.c which I'm not addressing here. > > OK millert@. Sorry, as soon as I got an ok offlist from tb@ I went ahead since it was just cosmetic. I noticed I'm too in a hurry recently when committing diffs, so i'll make sure to wait more in the future. > It would be nice to get these changes in portable > as well to avoid gratuitous differences. I'll take care of sync'ing with -portable. Thanks,
Re: smtpd: nits to reduce the diff with -portable
On 2023/05/15 09:40:50 +0200, theo Buehler wrote:
> Do we care that sometimes we (cast)var and sometimes we (cast) var?
> Is this at least consistent per-file?
almost always (cast)var. I've adjusted the spacing in the line I was
touching, grepping for common types I could only find one instance of
a '(long long) src' in envelope.c which I'm not addressing here.
Index: libexec/mail.local/mail.local.c
===
RCS file: /cvs/src/libexec/mail.local/mail.local.c,v
retrieving revision 1.40
diff -u -p -r1.40 mail.local.c
--- libexec/mail.local/mail.local.c 10 May 2023 08:03:49 - 1.40
+++ libexec/mail.local/mail.local.c 15 May 2023 06:59:09 -
@@ -244,7 +244,7 @@ retry:
curoff = lseek(mbfd, 0, SEEK_END);
(void)snprintf(biffmsg, sizeof biffmsg, "%s@%lld\n", name,
- (long long int)curoff);
+ (long long)curoff);
if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
mwarn("temporary file: %s", strerror(errno));
goto bad;
Index: usr.sbin/smtpd/bounce.c
===
RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v
retrieving revision 1.88
diff -u -p -r1.88 bounce.c
--- usr.sbin/smtpd/bounce.c 4 May 2023 12:43:44 - 1.88
+++ usr.sbin/smtpd/bounce.c 15 May 2023 06:59:29 -
@@ -305,7 +305,7 @@ bounce_send(struct bounce_session *s, co
}
static const char *
-bounce_duration(long long int d)
+bounce_duration(long long d)
{
static char buf[32];
Index: usr.sbin/smtpd/lka_filter.c
===
RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
retrieving revision 1.69
diff -u -p -r1.69 lka_filter.c
--- usr.sbin/smtpd/lka_filter.c 10 May 2023 07:20:20 - 1.69
+++ usr.sbin/smtpd/lka_filter.c 15 May 2023 06:59:29 -
@@ -933,13 +933,13 @@ filter_protocol_query(struct filter *fil
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
phase, reqid, token, fs->rdns, param);
else
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
phase, reqid, token, param);
if (n == -1)
fatalx("failed to write to processor");
@@ -957,7 +957,7 @@ filter_data_query(struct filter *filter,
"filter|%s|%lld.%06ld|smtp-in|data-line|"
"%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
reqid, token, line);
if (n == -1)
fatalx("failed to write to processor");
@@ -1374,7 +1374,7 @@ report_smtp_broadcast(uint64_t reqid, co
va_start(ap, format);
if (io_printf(lka_proc_get_io(rp->name),
"report|%s|%lld.%06ld|%s|%s|%016"PRIx64"%s",
- PROTOCOL_VERSION, (long long int)tv->tv_sec, tv->tv_usec,
+ PROTOCOL_VERSION, (long long)tv->tv_sec, tv->tv_usec,
direction, event, reqid,
format[0] != '\n' ? "|" : "") == -1 ||
io_vprintf(lka_proc_get_io(rp->name), format, ap) == -1)
Index: usr.sbin/smtpd/mail.maildir.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mail.maildir.c,v
retrieving revision 1.16
diff -u -p -r1.16 mail.maildir.c
--- usr.sbin/smtpd/mail.maildir.c 10 May 2023 07:19:49 - 1.16
+++ usr.sbin/smtpd/mail.maildir.c 15 May 2023 11:46:03 -
@@ -171,7 +171,7 @@ maildir_engine(const char *dirname, int
(void)strlcpy(hostname, "localhost", sizeof hostname);
(void)snprintf(filename, sizeof filename, "%lld.%08x.%s",
- (long long int) time(NULL),
+ (long long)time(NULL),
arc4random(),
hostname);
Index: usr.sbin/smtpd/mta_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v
retrieving revision 1.147
diff -u -p -r1.147 mta_session.c
--- usr.sbin/smtpd/mta_session.c26 Sep 2022 08:48:52 - 1.147
+++ usr.sbin/smtpd/mta_session.c15 May 2023 06:59:30 -
@@ -1157,7 +1157,7 @@ mta_response(struct mta_session *s, char
s->rcptcount = 0;
if (s->relay->limits->sessdelay_transaction) {
Re: smtpd: nits to reduce the diff with -portable
On 2023/05/15 09:03:47 +0200, Omar Polo wrote:
> On 2023/05/14 18:03:17 -0600, "Theo de Raadt" wrote:
> > Omar Polo wrote:
> > > On 2023/05/10 09:30:12 +0200, Theo Buehler wrote:
> > > > > I forgot to include one off_t cast since it was in a different
> > > > > directory and -even if off topic because it's not in portable- instead
> > > > > of "const"-ify only tz why don't mark as const also the two arrays day
> > > > > and month?
> > > >
> > > > ok.
> > > >
> > > > The previous diff used (long long int) and this one now uses (long
> > > > long).
> > > > Would be nice to be consistent.
> > >
> > > Yes, indeed. smtpd uses `long long int', while for mail.local doesn't
> > > have any. I'll go with `long long int' for consistency, typed `long
> > > long' out of muscular memory.
> >
> > I think it is wrong for smtpd to use "long long int". It is pointless
> > silliness, and there is more value in being idiomatically identical with
> > the greater body of code.
>
> ack (fwiw I prefer long long too). Here's s/long long int/long long/g,
> ok?
let's fix the indentation in smtpctl.c since I have to touch that line
anyway...
Index: libexec/mail.local/mail.local.c
===
RCS file: /cvs/src/libexec/mail.local/mail.local.c,v
retrieving revision 1.40
diff -u -p -r1.40 mail.local.c
--- libexec/mail.local/mail.local.c 10 May 2023 08:03:49 - 1.40
+++ libexec/mail.local/mail.local.c 15 May 2023 06:59:09 -
@@ -244,7 +244,7 @@ retry:
curoff = lseek(mbfd, 0, SEEK_END);
(void)snprintf(biffmsg, sizeof biffmsg, "%s@%lld\n", name,
- (long long int)curoff);
+ (long long)curoff);
if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
mwarn("temporary file: %s", strerror(errno));
goto bad;
Index: usr.sbin/smtpd/bounce.c
===
RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v
retrieving revision 1.88
diff -u -p -r1.88 bounce.c
--- usr.sbin/smtpd/bounce.c 4 May 2023 12:43:44 - 1.88
+++ usr.sbin/smtpd/bounce.c 15 May 2023 06:59:29 -
@@ -305,7 +305,7 @@ bounce_send(struct bounce_session *s, co
}
static const char *
-bounce_duration(long long int d)
+bounce_duration(long long d)
{
static char buf[32];
Index: usr.sbin/smtpd/lka_filter.c
===
RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
retrieving revision 1.69
diff -u -p -r1.69 lka_filter.c
--- usr.sbin/smtpd/lka_filter.c 10 May 2023 07:20:20 - 1.69
+++ usr.sbin/smtpd/lka_filter.c 15 May 2023 06:59:29 -
@@ -933,13 +933,13 @@ filter_protocol_query(struct filter *fil
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
phase, reqid, token, fs->rdns, param);
else
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
phase, reqid, token, param);
if (n == -1)
fatalx("failed to write to processor");
@@ -957,7 +957,7 @@ filter_data_query(struct filter *filter,
"filter|%s|%lld.%06ld|smtp-in|data-line|"
"%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
reqid, token, line);
if (n == -1)
fatalx("failed to write to processor");
@@ -1374,7 +1374,7 @@ report_smtp_broadcast(uint64_t reqid, co
va_start(ap, format);
if (io_printf(lka_proc_get_io(rp->name),
"report|%s|%lld.%06ld|%s|%s|%016"PRIx64"%s",
- PROTOCOL_VERSION, (long long int)tv->tv_sec, tv->tv_usec,
+ PROTOCOL_VERSION, (long long)tv->tv_sec, tv->tv_usec,
direction, event, reqid,
format[0] != '\n' ? "|" : "") == -1 ||
io_vprintf(lka_proc_get_io(rp->name), format, ap) == -1)
Index: usr.sbin/smtpd/
Re: smtpd: nits to reduce the diff with -portable
On 2023/05/14 18:03:17 -0600, "Theo de Raadt" wrote:
> Omar Polo wrote:
> > On 2023/05/10 09:30:12 +0200, Theo Buehler wrote:
> > > > I forgot to include one off_t cast since it was in a different
> > > > directory and -even if off topic because it's not in portable- instead
> > > > of "const"-ify only tz why don't mark as const also the two arrays day
> > > > and month?
> > >
> > > ok.
> > >
> > > The previous diff used (long long int) and this one now uses (long long).
> > > Would be nice to be consistent.
> >
> > Yes, indeed. smtpd uses `long long int', while for mail.local doesn't
> > have any. I'll go with `long long int' for consistency, typed `long
> > long' out of muscular memory.
>
> I think it is wrong for smtpd to use "long long int". It is pointless
> silliness, and there is more value in being idiomatically identical with
> the greater body of code.
ack (fwiw I prefer long long too). Here's s/long long int/long long/g,
ok?
Index: libexec/mail.local/mail.local.c
===
RCS file: /cvs/src/libexec/mail.local/mail.local.c,v
retrieving revision 1.40
diff -u -p -r1.40 mail.local.c
--- libexec/mail.local/mail.local.c 10 May 2023 08:03:49 - 1.40
+++ libexec/mail.local/mail.local.c 15 May 2023 06:59:09 -
@@ -244,7 +244,7 @@ retry:
curoff = lseek(mbfd, 0, SEEK_END);
(void)snprintf(biffmsg, sizeof biffmsg, "%s@%lld\n", name,
- (long long int)curoff);
+ (long long)curoff);
if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
mwarn("temporary file: %s", strerror(errno));
goto bad;
Index: usr.sbin/smtpd/bounce.c
===
RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v
retrieving revision 1.88
diff -u -p -r1.88 bounce.c
--- usr.sbin/smtpd/bounce.c 4 May 2023 12:43:44 - 1.88
+++ usr.sbin/smtpd/bounce.c 15 May 2023 06:59:29 -
@@ -305,7 +305,7 @@ bounce_send(struct bounce_session *s, co
}
static const char *
-bounce_duration(long long int d)
+bounce_duration(long long d)
{
static char buf[32];
Index: usr.sbin/smtpd/lka_filter.c
===
RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
retrieving revision 1.69
diff -u -p -r1.69 lka_filter.c
--- usr.sbin/smtpd/lka_filter.c 10 May 2023 07:20:20 - 1.69
+++ usr.sbin/smtpd/lka_filter.c 15 May 2023 06:59:29 -
@@ -933,13 +933,13 @@ filter_protocol_query(struct filter *fil
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
phase, reqid, token, fs->rdns, param);
else
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
phase, reqid, token, param);
if (n == -1)
fatalx("failed to write to processor");
@@ -957,7 +957,7 @@ filter_data_query(struct filter *filter,
"filter|%s|%lld.%06ld|smtp-in|data-line|"
"%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
reqid, token, line);
if (n == -1)
fatalx("failed to write to processor");
@@ -1374,7 +1374,7 @@ report_smtp_broadcast(uint64_t reqid, co
va_start(ap, format);
if (io_printf(lka_proc_get_io(rp->name),
"report|%s|%lld.%06ld|%s|%s|%016"PRIx64"%s",
- PROTOCOL_VERSION, (long long int)tv->tv_sec, tv->tv_usec,
+ PROTOCOL_VERSION, (long long)tv->tv_sec, tv->tv_usec,
direction, event, reqid,
format[0] != '\n' ? "|" : "") == -1 ||
io_vprintf(lka_proc_get_io(rp->name), format, ap) == -1)
Index: usr.sbin/smtpd/mail.maildir.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mail.maildir.c,v
retrieving revision 1.16
diff -u -p -r1.16 mail.maildir.c
--- usr.sbin/smtpd/mail.maildir.c 10 May 2023 07:19:49 -
Re: smtpd: nits to reduce the diff with -portable
On 2023/05/10 09:30:12 +0200, Theo Buehler wrote: > > I forgot to include one off_t cast since it was in a different > > directory and -even if off topic because it's not in portable- instead > > of "const"-ify only tz why don't mark as const also the two arrays day > > and month? > > ok. > > The previous diff used (long long int) and this one now uses (long long). > Would be nice to be consistent. Yes, indeed. smtpd uses `long long int', while for mail.local doesn't have any. I'll go with `long long int' for consistency, typed `long long' out of muscular memory. thanks!
Re: smtpd: nits to reduce the diff with -portable
On 2023/05/09 19:41:51 -0600, "Todd C. Miller" wrote:
> On Wed, 10 May 2023 00:55:54 +0200, Omar Polo wrote:
>
> > As per subject, here's a few misc nits that would reduce the
> > difference with -portable. There's some printing of time_t via
> > casting to long long, some missing includes (even if in tree it builds
> > nevertheless) and a const for a variable (no idea how it went there in
> > -portable but it's not wrong so including that too.)
>
> OK millert@
thanks!
I forgot to include one off_t cast since it was in a different
directory and -even if off topic because it's not in portable- instead
of "const"-ify only tz why don't mark as const also the two arrays day
and month?
sorry for forgetting these in the previous mail,
diff /usr/src
commit - a2d3cb1e480c37eb6fb14cee9f2946606a0346bc
path + /usr/src
blob - a0d38b1a7c0c7f7016b7d3f69e1c5dad77227e2a
file + libexec/mail.local/mail.local.c
--- libexec/mail.local/mail.local.c
+++ libexec/mail.local/mail.local.c
@@ -243,7 +243,8 @@ retry:
}
curoff = lseek(mbfd, 0, SEEK_END);
- (void)snprintf(biffmsg, sizeof biffmsg, "%s@%lld\n", name, curoff);
+ (void)snprintf(biffmsg, sizeof biffmsg, "%s@%lld\n", name,
+ (long long)curoff);
if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
mwarn("temporary file: %s", strerror(errno));
goto bad;
blob - 6e340ccde1a521ff13805c6f31d38ee77eb5ad50
file + usr.sbin/smtpd/to.c
--- usr.sbin/smtpd/to.c
+++ usr.sbin/smtpd/to.c
@@ -140,10 +140,10 @@ time_to_text(time_t when)
{
struct tm *lt;
static char buf[40];
- char *day[] = {"Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"};
- char *month[] = {"Jan","Feb","Mar","Apr","May","Jun",
+ const char *day[] = {"Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"};
+ const char *month[] = {"Jan","Feb","Mar","Apr","May","Jun",
"Jul","Aug","Sep","Oct","Nov","Dec"};
- char *tz;
+ const char *tz;
long offset;
lt = localtime();
smtpd: nits to reduce the diff with -portable
As per subject, here's a few misc nits that would reduce the
difference with -portable. There's some printing of time_t via
casting to long long, some missing includes (even if in tree it builds
nevertheless) and a const for a variable (no idea how it went there in
-portable but it's not wrong so including that too.)
ok?
diff /usr/src
commit - a2d3cb1e480c37eb6fb14cee9f2946606a0346bc
path + /usr/src
blob - 52924139091915e80409892fbd92dad375ee602c
file + usr.sbin/smtpd/lka_filter.c
--- usr.sbin/smtpd/lka_filter.c
+++ usr.sbin/smtpd/lka_filter.c
@@ -933,13 +933,13 @@ filter_protocol_query(struct filter *filter, uint64_t
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s|%s\n",
PROTOCOL_VERSION,
- tv.tv_sec, tv.tv_usec,
+ (long long int)tv.tv_sec, tv.tv_usec,
phase, reqid, token, fs->rdns, param);
else
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- tv.tv_sec, tv.tv_usec,
+ (long long int)tv.tv_sec, tv.tv_usec,
phase, reqid, token, param);
if (n == -1)
fatalx("failed to write to processor");
@@ -957,7 +957,7 @@ filter_data_query(struct filter *filter, uint64_t toke
"filter|%s|%lld.%06ld|smtp-in|data-line|"
"%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- tv.tv_sec, tv.tv_usec,
+ (long long int)tv.tv_sec, tv.tv_usec,
reqid, token, line);
if (n == -1)
fatalx("failed to write to processor");
@@ -1374,8 +1374,9 @@ report_smtp_broadcast(uint64_t reqid, const char *dire
va_start(ap, format);
if (io_printf(lka_proc_get_io(rp->name),
"report|%s|%lld.%06ld|%s|%s|%016"PRIx64"%s",
- PROTOCOL_VERSION, tv->tv_sec, tv->tv_usec, direction,
- event, reqid, format[0] != '\n' ? "|" : "") == -1 ||
+ PROTOCOL_VERSION, (long long int)tv->tv_sec, tv->tv_usec,
+ direction, event, reqid,
+ format[0] != '\n' ? "|" : "") == -1 ||
io_vprintf(lka_proc_get_io(rp->name), format, ap) == -1)
fatalx("failed to write to processor");
va_end(ap);
blob - c204caaf222db2e13204b09d59c15d7451d69763
file + usr.sbin/smtpd/mail.maildir.c
--- usr.sbin/smtpd/mail.maildir.c
+++ usr.sbin/smtpd/mail.maildir.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include
#defineMAILADDR_ESCAPE "!#$%&'*/?^`{|}~"
blob - edfd988605240338ded5f270ce291739401fa3fb
file + usr.sbin/smtpd/mda.c
--- usr.sbin/smtpd/mda.c
+++ usr.sbin/smtpd/mda.c
@@ -507,7 +507,7 @@ mda_getlastline(int fd, char *dst, size_t dstsz)
size_t sz = 0;
ssize_t len;
int out = 0;
-
+
if (lseek(fd, 0, SEEK_SET) == -1) {
log_warn("warn: mda: lseek");
close(fd);
blob - 4915bf6002c3e68dcf0a090818b521fe45de0a28
file + usr.sbin/smtpd/parse.y
--- usr.sbin/smtpd/parse.y
+++ usr.sbin/smtpd/parse.y
@@ -34,6 +34,8 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
blob - 6e340ccde1a521ff13805c6f31d38ee77eb5ad50
file + usr.sbin/smtpd/to.c
--- usr.sbin/smtpd/to.c
+++ usr.sbin/smtpd/to.c
@@ -143,7 +143,7 @@ time_to_text(time_t when)
char *day[] = {"Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"};
char *month[] = {"Jan","Feb","Mar","Apr","May","Jun",
"Jul","Aug","Sep","Oct","Nov","Dec"};
- char *tz;
+ const char *tz;
long offset;
lt = localtime();
Re: patch: profiling using utrace(2) (compatible with pledge and unveil)
On 2023/04/11 09:28:31 +0200, Sebastien Marie wrote:
> Hi,
>
> After otto@ work on mallocdump using utrace(2), I started to look again at
> profiling (see moncontrol(2)).
>
> The current implementation tries to write a gmon.out file at program exit(3)
> time, which isn't compatible with pledge(2) or unveil(2).
>
> This diff changes the way the runtime profiling information is extracted by
> using utrace(2) (which is permitted while pledged).
>
> The information is collected using ktrace(2), and the gmon.out file could be
> recreated from ktrace.out file post-execution (so without unveil(2)
> restriction).
This is really cool, it makes trivial to profile pledged/unveiled
programs without any source modifications.
> [...]
> Feedback would be appreciated.
I've used this to profile `gotadmin pack' a couple of times. got uses
several "libexec helpers" to parse data on disk, so profiling usually
gets awkward. You'd need to build only the program you're interested
in (e.g. got-read-pack) with PROFILE=1 and collect the data. $PROFDIR
doesn't work out-of-the-box due to unveil (PROFILE builds disable
pledge but leave unveil and permit "gmon.out" rwc.) With gotd it gets
a little more tricky since it's a single binary that gets fork+exec'd
multiple times. The argument popped up recently on the mailing list:
http://marc.gameoftrees.org/mail/1683051301.32966_0.html
With this instead, I can do one single run and collect the data for
all processes, and keep pledge/unveil in place.
To test, I've rebuilt and installed libc (and libz, and libutil -
probably wasn't needed), ran unifdef -m -UPROFILE on got and rebuild
with PROFILE=1 (needed to get -pg -static and the various -l*_p.)
I've uploaded the png obtained with `gprof2dot | dot -Tpng' here:
https://ftp.omarpolo.com/tmp/
There are two directories: one for `gotadmin pack -a' in src.git and
one in got.git.
I've observed that not always the profile data seems fine. Each run,
I have a few processes that report 100% on every function. At first I
thought it was an issue in how PROFILE=1 builds are done, but then I
noticed that "good" and "bad" data can be gathered for different run
of the same executable.
Compare for example
https://ftp.omarpolo.com/tmp/pack-got/profile.got-read-pack.52583.png
(bad)
and
https://ftp.omarpolo.com/tmp/pack-got/profile.got-read-pack.37417.png
(good)
both originating from the same run. I still have the gmon.out files
if needed.
One comment below
> --- lib/libc/gmon/gmon.c
> +++ lib/libc/gmon/gmon.c
> @@ -51,6 +52,32 @@ void
> PROTO_NORMAL(moncontrol);
> PROTO_DEPRECATED(monstartup);
>
> +static void
> +montrace(void *addr, size_t len)
> +{
> + for (;;) {
> + if (len < KTR_USER_MAXLEN) {
should be len <= KTR_USER_MAXLEN ?
> + if (utrace("gmon.out", addr, len) == -1)
> + ERR("error on utrace(), truncated gmon.out");
> + return;
> + }
> + if (utrace("gmon.out", addr, KTR_USER_MAXLEN) == -1)
> + ERR("error on utrace(), truncated gmon.out");
ERR expands to write(2, "..."), so I'd add a return here (very
unlikely for utrace to fail, but if we check for failures we should
also return IMHO)
There's an another existing case of ERR used like this in the file.
> +
> + len -= KTR_USER_MAXLEN;
> + addr += KTR_USER_MAXLEN;
> + }
> +}
Thanks!
Re: mg: rework adjustname() and drop expandtilde()
Please disregard for now; this introduces an issue since in main()
paths are resolved from the working directory of the current buffer so
`mg foo/bar' works (i.e. opens $PWD/foo/bar) but `mg foo/bar foo/baz'
doesn't since it first opens $PWD/foo/bar and then foo/baz relatively
to it, i.e. $PWD/foo/foo/baz.
On 2023/04/23 18:08:18 +0200, Omar Polo wrote:
> adjustname() should canonicalize the given path, but it does not do it
> always, resulting in some bizzare situations.
>
> it calls realpath(3) but when it fails the given path is returned
> as-is. This in practice can happen quite often since it's not usual
> for an editor to visit new files.
>
> A quick way to replicate the issue is to
>
> $ mg foo# assuming `foo' does not exist
> type something then save
> C-x C-f foo RET
>
> surprise, now you're in "foo<2>", a different buffer for the same
> file.
>
> On the other hand, expandtilde() has its own fair share of issues too.
>
> We can instead syntactically clean the path. It loose the "follow
> symlink" part, but it always work and is generally less surprising.
>
> I've checked that all the callers of adjustname() either provide an
> absolute path or are happy with it being resolved via the current
> buffer working directory. getbufcwd() respects the global-wd-mode so
> that case is also handled.
>
> (Some of the calls can/should be bubbled up or dropped, but that's for
> another diff.)
>
> An annoying thing is that adjustname() returns a pointer to a static
> area and it's not uncommon for that path to be fed to adjustname()
> again, hence the temp buffer. One quick way to hit that case is "mg
> ." which will adjustname() in main and then later again in dired_().
>
> ok?
>
>
> Index: cscope.c
> ===
> RCS file: /home/cvs/src/usr.bin/mg/cscope.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 cscope.c
> --- cscope.c 8 Mar 2023 04:43:11 - 1.22
> +++ cscope.c 23 Apr 2023 15:28:25 -
> @@ -338,7 +338,7 @@ jumptomatch(void)
>
> if (curmatch == NULL || currecord == NULL)
> return (FALSE);
> - adjf = adjustname(currecord->filename, TRUE);
> + adjf = adjustname(currecord->filename);
> if (adjf == NULL)
> return (FALSE);
> if ((bp = findbuffer(adjf)) == NULL)
> Index: def.h
> ===
> RCS file: /home/cvs/src/usr.bin/mg/def.h,v
> retrieving revision 1.180
> diff -u -p -r1.180 def.h
> --- def.h 21 Apr 2023 13:39:37 - 1.180
> +++ def.h 23 Apr 2023 15:30:55 -
> @@ -485,7 +485,7 @@ intffclose(FILE *, struct buffer *);
> int ffputbuf(FILE *, struct buffer *, int);
> int ffgetline(FILE *, char *, int, int *);
> int fbackupfile(const char *);
> -char *adjustname(const char *, int);
> +char *adjustname(const char *);
> FILE *startupfile(char *, char *, char *, size_t);
> int copy(char *, char *);
> struct list *make_file_list(char *);
> Index: dir.c
> ===
> RCS file: /home/cvs/src/usr.bin/mg/dir.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 dir.c
> --- dir.c 8 Mar 2023 04:43:11 - 1.33
> +++ dir.c 23 Apr 2023 15:28:25 -
> @@ -117,7 +117,7 @@ do_makedir(char *path)
> mode_t dir_mode, f_mode, oumask;
> char*slash;
>
> - if ((path = adjustname(path, TRUE)) == NULL)
> + if ((path = adjustname(path)) == NULL)
> return (FALSE);
>
> /* Remove trailing slashes */
> Index: dired.c
> ===
> RCS file: /home/cvs/src/usr.bin/mg/dired.c,v
> retrieving revision 1.102
> diff -u -p -r1.102 dired.c
> --- dired.c 8 Mar 2023 04:43:11 - 1.102
> +++ dired.c 23 Apr 2023 15:28:25 -
> @@ -469,7 +469,7 @@ d_copy(int f, int n)
> else if (bufp[0] == '\0')
> return (FALSE);
>
> - topath = adjustname(toname, TRUE);
> + topath = adjustname(toname);
> if (stat(topath, ) == 0) {
> if (S_ISDIR(statbuf.st_mode)) {
> ret = snprintf(toname, sizeof(toname), "%s/%s",
> @@ -479,7 +479,7 @@ d_copy(int f, int n)
> ewprintf("Directory name too long");
> return (FALSE);
> }
> - topath = adjustname(toname, TRUE);
> + topath
Re: mg.1: mark up commands
On 2023/04/25 21:01:21 +0100, Jason McIntyre wrote: > On Tue, Apr 25, 2023 at 03:58:08PM +0200, Omar Polo wrote: > > The commands are sometimes typed as-is, sometimes in single quotes > > 'like-this' and sometime inside Dq. I'd prefer to consistently use Ic > > for commands (which I believe is the appropriate mandoc macro). As a > > by-product, this allows to jump via :t to the description, which is > > very handy. > > > > I haven't touched the two tables with the default keybindings. I > > think that is clear enough that those are commands, and tmux(1) > > doesn't markup commands/variables in tables either. Plus, I think it > > would read weird with all that text in bold. > > > > ok? > > > > hi. > > i think it's fine to do this, if it means it is handled consistently. > i did not read the diff fully, but you should run it though mandoc > -Tlint to find things like missing blanks before punctuation. hint hint. woops :) here's the updated one with the spaces in place, sorry for missing it. I'll go ahead with this then, mandoc -Tlint is happy now. Thanks! Index: mg.1 === RCS file: /cvs/src/usr.bin/mg/mg.1,v retrieving revision 1.132 diff -u -p -r1.132 mg.1 --- mg.125 Apr 2023 13:32:20 - 1.132 +++ mg.125 Apr 2023 20:29:42 - @@ -103,15 +103,20 @@ Backup files have a character appended to the file name and are created in the current working directory by default. Whether to create backup files or not can be toggled with the -make-backup-files command. +.Ic make-backup-files +command. The backup file location can either be in the current working directory, or all backups can be moved to a .Pa ~/.mg.d directory where files retain their path name to retain uniqueness. -Use the backup-to-home-directory to alternate between these two locations. +Use the +.Ic backup-to-home-directory +command to alternate between these two locations. Further, if any application creates backup files in .Pa /tmp , -these can be left with the leave-tmpdir-backups command. +these can be left with the +.Ic leave-tmpdir-backups +command. .Sh TAGS .Nm supports tag files created by @@ -380,294 +385,306 @@ If a positive parameter is supplied, the Otherwise, it is forced to off. .\" .Bl -tag -width x -.It apropos +.It Ic apropos Help Apropos. Prompt the user for a string, open the *help* buffer, and list all .Nm commands that contain that string. -.It audible-bell +.It Ic audible-bell Toggle the audible system bell. -.It auto-execute +.It Ic auto-execute Register an auto-execute hook; that is, specify a filename pattern (conforming to the shell's filename globbing rules) and an associated function to execute when a file matching the specified pattern is read into a buffer. -.It auto-fill-mode +.It Ic auto-fill-mode Toggle auto-fill mode (sometimes called mail-mode) in the current buffer, where text inserted past the fill column is automatically wrapped to a new line. -Can be set globally with set-default-mode. -.It auto-indent-mode +Can be set globally with +.Ic set-default-mode . +.It Ic auto-indent-mode Toggle indent mode in the current buffer, where indentation is preserved after a newline. -Can be set globally with set-default-mode. -.It back-to-indentation +Can be set globally with +.Ic set-default-mode . +.It Ic back-to-indentation Move the dot to the first non-whitespace character on the current line. -.It backup-to-home-directory +.It Ic backup-to-home-directory Save backup copies to a .Pa ~/.mg.d directory instead of working directory. -Requires make-backup-files to be on. -.It backward-char +Requires +.Ic make-backup-files +to be on. +.It Ic backward-char Move cursor backwards one character. -.It backward-kill-word +.It Ic backward-kill-word Kill text backwards by .Va n words. -.It backward-paragraph +.It Ic backward-paragraph Move cursor backwards .Va n paragraphs. Paragraphs are delimited by or or . -.It backward-word +.It Ic backward-word Move cursor backwards by the specified number of words. -.It beginning-of-buffer +.It Ic beginning-of-buffer Move cursor to the top of the buffer. If set, keep mark's position, otherwise set at current position. A numeric argument .Va n will move n/10th of the way from the top. -.It beginning-of-line +.It Ic beginning-of-line Move cursor to the beginning of the line. -.It blink-and-insert +.It Ic blink-and-insert Self-insert a character, then search backwards and blink its matching delimiter. For delimiters other than parenthesis, brackets, and braces, the character itself is used as its own match. -Can be used in the startup file with the global-set-key command. -.It bsmap-mode +Can be used in the startup file with the +.Ic global-set-key +command. +.It Ic bsmap-mode Toggle bsmap mode, where DEL and C-h are swapped. -.It
mg.1: mark up commands
The commands are sometimes typed as-is, sometimes in single quotes 'like-this' and sometime inside Dq. I'd prefer to consistently use Ic for commands (which I believe is the appropriate mandoc macro). As a by-product, this allows to jump via :t to the description, which is very handy. I haven't touched the two tables with the default keybindings. I think that is clear enough that those are commands, and tmux(1) doesn't markup commands/variables in tables either. Plus, I think it would read weird with all that text in bold. ok? Index: mg.1 === RCS file: /cvs/src/usr.bin/mg/mg.1,v retrieving revision 1.132 diff -u -p -r1.132 mg.1 --- mg.125 Apr 2023 13:32:20 - 1.132 +++ mg.125 Apr 2023 13:47:56 - @@ -103,15 +103,20 @@ Backup files have a character appended to the file name and are created in the current working directory by default. Whether to create backup files or not can be toggled with the -make-backup-files command. +.Ic make-backup-files +command. The backup file location can either be in the current working directory, or all backups can be moved to a .Pa ~/.mg.d directory where files retain their path name to retain uniqueness. -Use the backup-to-home-directory to alternate between these two locations. +Use the +.Ic backup-to-home-directory +command to alternate between these two locations. Further, if any application creates backup files in .Pa /tmp , -these can be left with the leave-tmpdir-backups command. +these can be left with the +.Ic leave-tmpdir-backups +command. .Sh TAGS .Nm supports tag files created by @@ -380,294 +385,306 @@ If a positive parameter is supplied, the Otherwise, it is forced to off. .\" .Bl -tag -width x -.It apropos +.It Ic apropos Help Apropos. Prompt the user for a string, open the *help* buffer, and list all .Nm commands that contain that string. -.It audible-bell +.It Ic audible-bell Toggle the audible system bell. -.It auto-execute +.It Ic auto-execute Register an auto-execute hook; that is, specify a filename pattern (conforming to the shell's filename globbing rules) and an associated function to execute when a file matching the specified pattern is read into a buffer. -.It auto-fill-mode +.It Ic auto-fill-mode Toggle auto-fill mode (sometimes called mail-mode) in the current buffer, where text inserted past the fill column is automatically wrapped to a new line. -Can be set globally with set-default-mode. -.It auto-indent-mode +Can be set globally with +.Ic set-default-mode. +.It Ic auto-indent-mode Toggle indent mode in the current buffer, where indentation is preserved after a newline. -Can be set globally with set-default-mode. -.It back-to-indentation +Can be set globally with +.Ic set-default-mode . +.It Ic back-to-indentation Move the dot to the first non-whitespace character on the current line. -.It backup-to-home-directory +.It Ic backup-to-home-directory Save backup copies to a .Pa ~/.mg.d directory instead of working directory. -Requires make-backup-files to be on. -.It backward-char +Requires +.Ic make-backup-files +to be on. +.It Ic backward-char Move cursor backwards one character. -.It backward-kill-word +.It Ic backward-kill-word Kill text backwards by .Va n words. -.It backward-paragraph +.It Ic backward-paragraph Move cursor backwards .Va n paragraphs. Paragraphs are delimited by or or . -.It backward-word +.It Ic backward-word Move cursor backwards by the specified number of words. -.It beginning-of-buffer +.It Ic beginning-of-buffer Move cursor to the top of the buffer. If set, keep mark's position, otherwise set at current position. A numeric argument .Va n will move n/10th of the way from the top. -.It beginning-of-line +.It Ic beginning-of-line Move cursor to the beginning of the line. -.It blink-and-insert +.It Ic blink-and-insert Self-insert a character, then search backwards and blink its matching delimiter. For delimiters other than parenthesis, brackets, and braces, the character itself is used as its own match. -Can be used in the startup file with the global-set-key command. -.It bsmap-mode +Can be used in the startup file with the +.Ic global-set-key +command. +.It Ic bsmap-mode Toggle bsmap mode, where DEL and C-h are swapped. -.It c-mode +.It Ic c-mode Toggle a KNF-compliant mode for editing C program files. -.It call-last-kbd-macro +.It Ic call-last-kbd-macro Invoke the keyboard macro. -.It capitalize-word +.It Ic capitalize-word Capitalize .Va n words; i.e. convert the first character of the word to upper case, and subsequent letters to lower case. -.It cd +.It Ic cd Change the global working directory. See also global-wd-mode. -.It column-number-mode +.It Ic column-number-mode Toggle whether the column number is displayed in the modeline. -.It copy-region-as-kill +.It Ic copy-region-as-kill Copy all of the characters in the region to the kill buffer, clearing
mg: rework adjustname() and drop expandtilde()
adjustname() should canonicalize the given path, but it does not do it
always, resulting in some bizzare situations.
it calls realpath(3) but when it fails the given path is returned
as-is. This in practice can happen quite often since it's not usual
for an editor to visit new files.
A quick way to replicate the issue is to
$ mg foo# assuming `foo' does not exist
type something then save
C-x C-f foo RET
surprise, now you're in "foo<2>", a different buffer for the same
file.
On the other hand, expandtilde() has its own fair share of issues too.
We can instead syntactically clean the path. It loose the "follow
symlink" part, but it always work and is generally less surprising.
I've checked that all the callers of adjustname() either provide an
absolute path or are happy with it being resolved via the current
buffer working directory. getbufcwd() respects the global-wd-mode so
that case is also handled.
(Some of the calls can/should be bubbled up or dropped, but that's for
another diff.)
An annoying thing is that adjustname() returns a pointer to a static
area and it's not uncommon for that path to be fed to adjustname()
again, hence the temp buffer. One quick way to hit that case is "mg
." which will adjustname() in main and then later again in dired_().
ok?
Index: cscope.c
===
RCS file: /home/cvs/src/usr.bin/mg/cscope.c,v
retrieving revision 1.22
diff -u -p -r1.22 cscope.c
--- cscope.c8 Mar 2023 04:43:11 - 1.22
+++ cscope.c23 Apr 2023 15:28:25 -
@@ -338,7 +338,7 @@ jumptomatch(void)
if (curmatch == NULL || currecord == NULL)
return (FALSE);
- adjf = adjustname(currecord->filename, TRUE);
+ adjf = adjustname(currecord->filename);
if (adjf == NULL)
return (FALSE);
if ((bp = findbuffer(adjf)) == NULL)
Index: def.h
===
RCS file: /home/cvs/src/usr.bin/mg/def.h,v
retrieving revision 1.180
diff -u -p -r1.180 def.h
--- def.h 21 Apr 2023 13:39:37 - 1.180
+++ def.h 23 Apr 2023 15:30:55 -
@@ -485,7 +485,7 @@ int ffclose(FILE *, struct buffer *);
int ffputbuf(FILE *, struct buffer *, int);
int ffgetline(FILE *, char *, int, int *);
int fbackupfile(const char *);
-char *adjustname(const char *, int);
+char *adjustname(const char *);
FILE *startupfile(char *, char *, char *, size_t);
int copy(char *, char *);
struct list*make_file_list(char *);
Index: dir.c
===
RCS file: /home/cvs/src/usr.bin/mg/dir.c,v
retrieving revision 1.33
diff -u -p -r1.33 dir.c
--- dir.c 8 Mar 2023 04:43:11 - 1.33
+++ dir.c 23 Apr 2023 15:28:25 -
@@ -117,7 +117,7 @@ do_makedir(char *path)
mode_t dir_mode, f_mode, oumask;
char*slash;
- if ((path = adjustname(path, TRUE)) == NULL)
+ if ((path = adjustname(path)) == NULL)
return (FALSE);
/* Remove trailing slashes */
Index: dired.c
===
RCS file: /home/cvs/src/usr.bin/mg/dired.c,v
retrieving revision 1.102
diff -u -p -r1.102 dired.c
--- dired.c 8 Mar 2023 04:43:11 - 1.102
+++ dired.c 23 Apr 2023 15:28:25 -
@@ -469,7 +469,7 @@ d_copy(int f, int n)
else if (bufp[0] == '\0')
return (FALSE);
- topath = adjustname(toname, TRUE);
+ topath = adjustname(toname);
if (stat(topath, ) == 0) {
if (S_ISDIR(statbuf.st_mode)) {
ret = snprintf(toname, sizeof(toname), "%s/%s",
@@ -479,7 +479,7 @@ d_copy(int f, int n)
ewprintf("Directory name too long");
return (FALSE);
}
- topath = adjustname(toname, TRUE);
+ topath = adjustname(toname);
}
}
if (topath == NULL)
@@ -528,7 +528,7 @@ d_rename(int f, int n)
else if (bufp[0] == '\0')
return (FALSE);
- topath = adjustname(toname, TRUE);
+ topath = adjustname(toname);
if (stat(topath, ) == 0) {
if (S_ISDIR(statbuf.st_mode)) {
ret = snprintf(toname, sizeof(toname), "%s/%s",
@@ -538,7 +538,7 @@ d_rename(int f, int n)
ewprintf("Directory name too long");
return (FALSE);
}
- topath = adjustname(toname, TRUE);
+ topath = adjustname(toname);
}
}
if (topath == NULL)
@@ -901,7 +901,7 @@ dired_(char *dname)
int i;
size_t
mg: allow to change the tab width
This makes the tab width customizable per-buffer. There's a new
function called `set-tab-width' that changes the tab width for the
current buffer or the default one for new buffers if called with a
prefix argument (or from the startup file.)
The default tab width is still 8 column if not changed by the user,
but together with the newly resurrected no-tab-mode this allows mg to
be used for a variety of programming languages and coding styles.
Want to hack on lua? easy:
$ cat <> ~/.mg
set-tab-width 3
auto-execute *.lua no-tab-mode
EOF
You might be tempted to say
auto-execute *.lua set-tab-width 3 # wrong!
but it's currently a syntax error since auto-execute takes a function
without arguments. For the time being at least.
Most of the changes are to replace the common idiom
col |= 0x07;
col++;
with the new helper function ntabstop().
display.c has quite some churn since I'm basically breaking an
abstraction: update() deals with the line to render but knows very
little of the state of the terminal, vtputc() and friends instead
knows nothing of the buffer but know the current physical column. I'm
pushing the mgwin struct down into the vt*() layer so that it knows
how wide a tab is. Previously I did it with a global variable and
while it needed less changes, it was uglier.
util.c may not be the perfect place for ntabstop(). c-mode won't be
happy with a tab width different from eight, still have to think what
to do there. 1 and 16 as min/max value for the tab width are
arbirtrary.
All in all I'm quite happy with how it works. Comments, reviews and
tests appreciated :)
Index: basic.c
===
RCS file: /cvs/src/usr.bin/mg/basic.c,v
retrieving revision 1.53
diff -u -p -r1.53 basic.c
--- basic.c 17 Apr 2023 09:49:04 - 1.53
+++ basic.c 17 Apr 2023 16:32:21 -
@@ -277,8 +277,7 @@ getgoal(struct line *dlp)
for (i = 0; i < llength(dlp); i++) {
c = lgetc(dlp, i);
if (c == '\t') {
- col |= 0x07;
- col++;
+ col = ntabstop(col, curbp->b_tabw);
} else if (ISCTRL(c) != FALSE) {
col += 2;
} else if (isprint(c))
Index: buffer.c
===
RCS file: /cvs/src/usr.bin/mg/buffer.c,v
retrieving revision 1.113
diff -u -p -r1.113 buffer.c
--- buffer.c8 Mar 2023 04:43:11 - 1.113
+++ buffer.c17 Apr 2023 16:32:21 -
@@ -26,9 +26,42 @@ static struct buffer *bnew(const char *)
static int usebufname(const char *);
+/* Default tab width */
+int defb_tabw = 8;
+
/* Flag for global working dir */
extern int globalwd;
+/*
+ * Set the tab width for the current buffer, or the default for new
+ * buffers if called with a prefix argument.
+ */
+int
+settabw(int f, int n)
+{
+ charbuf[8], *bufp;
+ const char *errstr;
+
+ if (f & FFARG) {
+ if (n <= 0 || n > 16)
+ return (FALSE);
+ defb_tabw = n;
+ return (TRUE);
+ }
+
+ if ((bufp = eread("Tab Width: ", buf, sizeof(buf),
+ EFNUL | EFNEW | EFCR)) == NULL)
+ return (ABORT);
+ if (bufp[0] == '\0')
+ return (ABORT);
+ n = strtonum(buf, 1, 16, );
+ if (errstr)
+ return (dobeep_msgs("Tab width", errstr));
+ curbp->b_tabw = n;
+ curwp->w_rflag |= WFFRAME;
+ return (TRUE);
+}
+
int
togglereadonlyall(int f, int n)
{
@@ -588,6 +621,7 @@ bnew(const char *bname)
bp->b_lines = 1;
bp->b_nlseq = "\n"; /* use unix default */
bp->b_nlchr = bp->b_nlseq;
+ bp->b_tabw = defb_tabw;
if ((bp->b_bname = strdup(bname)) == NULL) {
dobeep();
ewprintf("Can't get %d bytes", strlen(bname) + 1);
Index: cmode.c
===
RCS file: /cvs/src/usr.bin/mg/cmode.c,v
retrieving revision 1.21
diff -u -p -r1.21 cmode.c
--- cmode.c 17 Apr 2023 09:49:04 - 1.21
+++ cmode.c 17 Apr 2023 16:32:21 -
@@ -245,10 +245,10 @@ getindent(const struct line *lp, int *cu
for (lo = 0; lo < llength(lp); lo++) {
if (!isspace(c = lgetc(lp, lo)))
break;
- if (c == '\t') {
- nicol |= 0x07;
- }
- nicol++;
+ if (c == '\t')
+ nicol = ntabstop(nicol, curbp->b_tabw);
+ else
+ nicol++;
}
/* If last line was blank, choose 0 */
@@ -411,8 +411,7 @@ findcolpos(const struct buffer *bp, cons
for (i = 0; i < lo; ++i) {
c = lgetc(lp, i);
if (c == '\t') {
- col |= 0x07;
-
mg: fix buffer overflow in displaymatch()
When the match for a character is not in the visible part of the
window, mg shows a copy of that line in the echo area. To do so, it
copies the line to a local buffer, so that tabs and control characters
can be rendered properly, but doesn't check for overflow.
NLINE (the size of `buf') is 256, so if the matching parentesis is on
a line longer than 256 characters that's not visible on the screen, we
start writing out-of-bounds and likely crashing.
I've reproduced the crash by editing (a copy of) /bsd which has very
long lines and verified that diff below fixes it. The kernel is nice
for this because it contains a lot of bytes that mg interprets as
controls and so stresses the rendering of control characters.
ok?
Index: match.c
===
RCS file: /home/cvs/src/usr.bin/mg/match.c,v
retrieving revision 1.23
diff -u -p -r1.23 match.c
--- match.c 17 Apr 2023 09:49:04 - 1.23
+++ match.c 17 Apr 2023 13:59:57 -
@@ -168,17 +168,23 @@ displaymatch(struct line *clp, int cbo)
/* match is not in this window, so display line in echo area */
bufo = 0;
for (cp = 0; cp < llength(clp); cp++) {
+ if (bufo >= sizeof(buf) - 1)
+ break;
+
c = lgetc(clp, cp);
- if (c != '\t')
+ if (c != '\t') {
if (ISCTRL(c)) {
+ if (bufo + 2 >= sizeof(buf) - 1)
+ break;
buf[bufo++] = '^';
buf[bufo++] = CCHR(c);
} else
buf[bufo++] = c;
- else
+ } else {
do {
buf[bufo++] = ' ';
- } while (bufo & 7);
+ } while ((bufo & 7) && bufo < sizeof(buf) - 1);
+ }
}
buf[bufo++] = '\0';
ewprintf("Matches %s", buf);
mg: fix spacing in a few dobeep_msgs() calls
as per subject, when dobeep_msgs() was introduced some places weren't
converted correctly: it already adds a space between the two
arguments, so no need to duplicate.
Index: extend.c
===
RCS file: /cvs/src/usr.bin/mg/extend.c,v
retrieving revision 1.79
diff -u -p -r1.79 extend.c
--- extend.c30 Mar 2023 22:56:47 - 1.79
+++ extend.c17 Apr 2023 10:00:55 -
@@ -493,7 +493,7 @@ redefine_key(int f, int n)
return (FALSE);
(void)strlcat(buf, tmp, sizeof(buf));
if ((mp = name_map(tmp)) == NULL)
- return (dobeep_msgs("Unknown map ", tmp));
+ return (dobeep_msgs("Unknown map", tmp));
if (strlcat(buf, "key: ", sizeof(buf)) >= sizeof(buf))
return (FALSE);
@@ -720,7 +720,7 @@ excline(char *line, int llen, int lnum)
n = (int)nl;
}
if ((fp = name_function(funcp)) == NULL)
- return (dobeep_msgs("Unknown function: ", funcp));
+ return (dobeep_msgs("Unknown function:", funcp));
if (fp == bindtokey || fp == unbindtokey) {
bind = BINDARG;
@@ -847,7 +847,7 @@ excline(char *line, int llen, int lnum)
case BINDNEXT:
lp->l_text[lp->l_used] = '\0';
if ((curmap = name_map(lp->l_text)) == NULL) {
- (void)dobeep_msgs("No such mode: ", lp->l_text);
+ (void)dobeep_msgs("No such mode:", lp->l_text);
status = FALSE;
free(lp);
goto cleanup;
Index: interpreter.c
===
RCS file: /cvs/src/usr.bin/mg/interpreter.c,v
retrieving revision 1.34
diff -u -p -r1.34 interpreter.c
--- interpreter.c 28 Jan 2022 06:18:41 - 1.34
+++ interpreter.c 17 Apr 2023 10:00:55 -
@@ -406,7 +406,7 @@ parsexp(char *begp, const char *par1, co
* If no extant mg command found, just return.
*/
if ((funcp = name_function(cmdp)) == NULL)
- return (dobeep_msgs("Unknown command: ", cmdp));
+ return (dobeep_msgs("Unknown command:", cmdp));
numparams = numparams_function(funcp);
if (numparams == 0)
mg: enable no-tab-mode
The `no-tab-mode' minor mode is currently hidden behind some #ifdef,
and I'd like to enable it since it's really useful. It's the
equivalent of vi' `expandtab', i.e. makes the TAB key inserting spaces
up to the next tab stop.
My plan would be to follow-up with a diff to make the tab width
customizable per-buffer (vi' `shiftwidth'): together, these two
settings could allow mg to be used for a variety of situations with
very little effort.
When re-enabling the no-tab-mode I've initially just un-ifdef'd NOTAB,
but the result is not correct IMHO: even under no-tab-mode mg should
consider a literal TAB character wide until the next tab stop, so in a
few places I've not preserved the un-ifdef'd check. I've also
introduced an helper function doindent() in utils.c to simplify the
task of inserting tabs/spaces until the given column.
ok?
diff /usr/src
commit - 4bc3741a05f858113bfe387089085a77ab225ec5
path + /usr/src
blob - 4a170480432e59db5d6b39096587ffef3a2310e6
file + usr.bin/mg/basic.c
--- usr.bin/mg/basic.c
+++ usr.bin/mg/basic.c
@@ -274,14 +274,9 @@ getgoal(struct line *dlp)
int c, i, col = 0;
char tmp[5];
-
for (i = 0; i < llength(dlp); i++) {
c = lgetc(dlp, i);
- if (c == '\t'
-#ifdef NOTAB
- && !(curbp->b_flag & BFNOTAB)
-#endif
- ) {
+ if (c == '\t') {
col |= 0x07;
col++;
} else if (ISCTRL(c) != FALSE) {
blob - 9f5ffd72f80efd74432dafaa3e48b72acc7b932f
file + usr.bin/mg/cmode.c
--- usr.bin/mg/cmode.c
+++ usr.bin/mg/cmode.c
@@ -245,11 +245,7 @@ getindent(const struct line *lp, int *curi)
for (lo = 0; lo < llength(lp); lo++) {
if (!isspace(c = lgetc(lp, lo)))
break;
- if (c == '\t'
-#ifdef NOTAB
- && !(curbp->b_flag & BFNOTAB)
-#endif /* NOTAB */
- ) {
+ if (c == '\t') {
nicol |= 0x07;
}
nicol++;
@@ -414,11 +410,7 @@ findcolpos(const struct buffer *bp, const struct line
for (i = 0; i < lo; ++i) {
c = lgetc(lp, i);
- if (c == '\t'
-#ifdef NOTAB
- && !(bp->b_flag & BFNOTAB)
-#endif /* NOTAB */
- ) {
+ if (c == '\t') {
col |= 0x07;
col++;
} else if (ISCTRL(c) != FALSE)
blob - 5475b513036334b5bfd5530cf100291c218cddce
file + usr.bin/mg/def.h
--- usr.bin/mg/def.h
+++ usr.bin/mg/def.h
@@ -285,9 +285,7 @@ struct buffer {
#define BFCHG 0x01/* Changed. */
#define BFBAK 0x02/* Need to make a backup.*/
-#ifdef NOTAB
#define BFNOTAB 0x04 /* no tab mode */
-#endif
#define BFOVERWRITE 0x08 /* overwrite mode*/
#define BFREADONLY 0x10 /* read only mode*/
#define BFDIRTY 0x20 /* Buffer was modified elsewhere */
@@ -676,9 +674,7 @@ int fillmode(int, int);
/* modes.c X */
int indentmode(int, int);
int fillmode(int, int);
-#ifdef NOTAB
int notabmode(int, int);
-#endif /* NOTAB */
int overwrite_mode(int, int);
int set_default_mode(int,int);
blob - 1751b5ead137bde915e470e79fe5a9ec6b91b29e
file + usr.bin/mg/display.c
--- usr.bin/mg/display.c
+++ usr.bin/mg/display.c
@@ -317,11 +317,7 @@ vtputc(int c)
vp = vscreen[vtrow];
if (vtcol >= ncol)
vp->v_text[ncol - 1] = '$';
- else if (c == '\t'
-#ifdef NOTAB
- && !(curbp->b_flag & BFNOTAB)
-#endif
- ) {
+ else if (c == '\t') {
do {
vtputc(' ');
} while (vtcol < ncol && (vtcol & 0x07) != 0);
@@ -353,11 +349,7 @@ vtpute(int c)
vp = vscreen[vtrow];
if (vtcol >= ncol)
vp->v_text[ncol - 1] = '$';
- else if (c == '\t'
-#ifdef NOTAB
- && !(curbp->b_flag & BFNOTAB)
-#endif
- ) {
+ else if (c == '\t') {
do {
vtpute(' ');
} while (((vtcol + lbound) & 0x07) != 0 && vtcol < ncol);
@@ -516,11 +508,7 @@ update(int modelinecolor)
i = 0;
while (i < curwp->w_doto) {
c = lgetc(lp, i++);
- if (c == '\t'
-#ifdef NOTAB
- && !(curbp->b_flag & BFNOTAB)
-#endif
- ) {
+ if (c == '\t') {
curcol |= 0x07;
curcol++;
} else if (ISCTRL(c) != FALSE)
blob - e1c59fa0fb43a02c6233288b57d1a6332c5b6282
file + usr.bin/mg/funmap.c
--- usr.bin/mg/funmap.c
+++ usr.bin/mg/funmap.c
@@ -157,9 +157,7 @@ static struct funmap functnames[] = {
{enewline, "newline", 1},
Re: [installer] default answer to "Is the disk partition already mounted?"
On 2023/04/01 16:33:39 +0300, Mikhail wrote:
> Currently default answer for the question is 'yes', which is not true
> for install case, but correct for upgrade one.
>
> Changing default answer depending on the mode of the installer looks
> like a good approach. Idea by Christian (naddy@), implementation and
> testing are mine.
fwiw I like the idea too! Doing the first install it's easy to assume
that the usb stick one is booting from is mounted and so mistakenly
reply "yes". No big deal, but it saves a few back and forths in the
installer (and few mails to misc@ ;-)
I remember being surprised when the default for this was changed from
"no" to "yes".
> diff /usr/src
> commit - c5d97e146b0b84b5aaf0732c7ffb8b845cadd04d
> path + /usr/src
> blob - c5d0d7f129960a410a8c89af1d480135f6a18d09
> file + distrib/miniroot/install.sub
> --- distrib/miniroot/install.sub
> +++ distrib/miniroot/install.sub
> @@ -2042,7 +2042,12 @@ install_disk() {
> # Install sets from disk.
> # Ask for the disk device containing the set files.
> install_disk() {
> - if ! ask_yn "Is the disk partition already mounted?" yes; then
> + local _answer
not very familiar with install.sub, but I'd change _answer to default to yes
> + # In installation mode installer doesn't automount partitions
> + [[ $MODE == install ]] || _answer=yes
and toggle it here to no (or flip the logic); would be clearer IMHO
and would resemble more questions().
This is also the first instance as far as I can see where $MODE would
change the default reply for a question. No idea if this is
important. Also, untested, but it reads fine :)
> + if ! ask_yn "Is the disk partition already mounted?" $_answer; then
> ask_which "disk" "contains the $MODE media" \
> '$(bsort $(get_dkdevs))' \
> '$(get_dkdevs_uninitialized)'
Thanks!
Re: mg: don't access(conffile), just open it
On 2023/03/30 20:09:21 +0200, Theo Buehler wrote:
> > Index: main.c
> [...]
> > @@ -159,8 +162,10 @@ main(int argc, char **argv)
> > update(CMODE);
> >
> > /* user startup file. */
> > - if ((cp = startupfile(NULL, conffile)) != NULL)
> > - (void)load(cp);
> > + if (ffp) {
>
> Could you check this against NULL
>
> if (ffp != NULL) {
>
> Then it's ok.
Thanks!
load now has an unnecessary copy of the file path for no reason, ok to
drop?
Index: extend.c
===
RCS file: /home/cvs/src/usr.bin/mg/extend.c,v
retrieving revision 1.78
diff -u -p -r1.78 extend.c
--- extend.c30 Mar 2023 19:00:02 - 1.78
+++ extend.c30 Mar 2023 20:22:52 -
@@ -649,10 +649,8 @@ load(FILE *ffp, const char *fname)
{
int s = TRUE, line;
int nbytes = 0;
- char excbuf[BUFSIZE], fncpy[NFILEN];
+ char excbuf[BUFSIZE];
- /* keep a note of fname in case of errors in loaded file. */
- (void)strlcpy(fncpy, fname, sizeof(fncpy));
line = 0;
while ((s = ffgetline(ffp, excbuf, sizeof(excbuf) - 1, ))
== FIOSUC) {
@@ -661,7 +659,7 @@ load(FILE *ffp, const char *fname)
if (excline(excbuf, nbytes, line) != TRUE) {
s = FIOERR;
dobeep();
- ewprintf("Error loading file %s at line %d", fncpy,
line);
+ ewprintf("Error loading file %s at line %d", fname,
line);
break;
}
}
Re: mg: don't access(conffile), just open it
On 2023/03/30 18:11:32 +0200, Theo Buehler wrote:
> On Thu, Mar 30, 2023 at 03:47:24PM +0200, Omar Polo wrote:
> > - else if (bufp[0] == '\0')
> > + if (bufp[0] == '\0')
> > return (FALSE);
> > - return (load(fname));
> > + if ((bufp = adjustname(fname, TRUE)) == NULL)
> > + return (FALSE);
> > + ret = ffropen(, bufp, NULL);
>
> Nit: I'd avoid the nesting (I know that you just moved the code):
>
> if (ret == FIODIR)
> (void)ffclose(ffp, NULL);
> if (ret != FIOSUC)
> return (FALSE);
I prefer without the nesting too, changed.
> [...]
> > - if (conffile != NULL && access(conffile, R_OK) != 0) {
> > + if ((cp = startupfile(, NULL, conffile)) != NULL && conffile) {
>
> This doesn't look right. I'd have expected
>
> if ((cp = startupfile(, NULL, conffile)) == NULL && conffile) {
>
> (and indeed, your patch breaks -u).
Arggg... Initially I had startupfile() right before load(), I've moved
it before sending the mail and not re-tested afterwards, sorry!
I've also noticed that since startupfile() returns static memory
errors in -u would report the wrong path (ttykeymapinit() calls
startupfile() itself) so I'm changing it to use a caller provided
buffer for the path. tbf i don't completely like it and it also
causes some churn in the diff in startupfile().
The alternative would be to call ttykeymapinit() after load() in
main.c, and delaying ttykeymapinit(). It'll make the diff smaller but
it feels a bit hackish to keep a pointer to static memory for that
long, and future hackers may be bitten again by this.
(delaying ttykeymapinit() is something on my todo list as well, it
should be called at least after update(CMODE) for errors to be
printed, but it's a separate issue.)
Index: def.h
===
RCS file: /home/cvs/src/usr.bin/mg/def.h,v
retrieving revision 1.177
diff -u -p -r1.177 def.h
--- def.h 20 Oct 2022 18:59:24 - 1.177
+++ def.h 30 Mar 2023 16:34:07 -
@@ -486,7 +486,7 @@ int ffputbuf(FILE *, struct buffer *,
int ffgetline(FILE *, char *, int, int *);
int fbackupfile(const char *);
char *adjustname(const char *, int);
-char *startupfile(char *, char *);
+FILE *startupfile(char *, char *, char *, size_t);
int copy(char *, char *);
struct list*make_file_list(char *);
int fisdir(const char *);
@@ -594,7 +594,7 @@ int extend(int, int);
int evalexpr(int, int);
int evalbuffer(int, int);
int evalfile(int, int);
-int load(const char *);
+int load(FILE *, const char *);
int excline(char *, int, int);
char *skipwhite(char *);
Index: extend.c
===
RCS file: /home/cvs/src/usr.bin/mg/extend.c,v
retrieving revision 1.77
diff -u -p -r1.77 extend.c
--- extend.c8 Mar 2023 04:43:11 - 1.77
+++ extend.c30 Mar 2023 16:22:52 -
@@ -620,37 +620,36 @@ evalbuffer(int f, int n)
int
evalfile(int f, int n)
{
+ FILE*ffp;
char fname[NFILEN], *bufp;
+ int ret;
if ((bufp = eread("Load file: ", fname, NFILEN,
EFNEW | EFCR)) == NULL)
return (ABORT);
- else if (bufp[0] == '\0')
+ if (bufp[0] == '\0')
return (FALSE);
- return (load(fname));
+ if ((bufp = adjustname(fname, TRUE)) == NULL)
+ return (FALSE);
+ ret = ffropen(, bufp, NULL);
+ if (ret == FIODIR)
+ (void)ffclose(ffp, NULL);
+ if (ret != FIOSUC)
+ return (FALSE);
+ ret = load(ffp, bufp);
+ (void)ffclose(ffp, NULL);
+ return (ret);
}
/*
* load - go load the file name we got passed.
*/
int
-load(const char *fname)
+load(FILE *ffp, const char *fname)
{
- int s = TRUE, line, ret;
+ int s = TRUE, line;
int nbytes = 0;
char excbuf[BUFSIZE], fncpy[NFILEN];
- FILE*ffp;
-
- if ((fname = adjustname(fname, TRUE)) == NULL)
- /* just to be careful */
- return (FALSE);
-
- ret = ffropen(, fname, NULL);
- if (ret != FIOSUC) {
- if (ret == FIODIR)
- (void)ffclose(ffp, NULL);
- return (FALSE);
- }
/* keep a note of fname in case of errors in loaded file. */
(void)strlcpy(fncpy, fname, sizeof(fncpy));
@@ -666,7 +665,6 @@ load(const char *fname)
break;
}
}
- (void)ffclose(ffp, NULL);
excbuf[nbytes] = '\0';
if (s != FIOEOF || (nbytes && excline(excbuf, nbytes, ++line) != TRUE)
Re: [patch] /usr.bin/script: Fix process name hardcode for shell subprocesses
On 2023/03/30 22:59:08 +0800, lux wrote: > On Thu, 2023-03-30 at 16:31 +0200, Omar Polo wrote: > > > > Furthermore, the man page explicitly states that script -c runs > > sh(1): > > > > -c command > > Run sh -c command, instead of an interactive shell. > > [...] > > > > So I'd say that script(1) is working as intended. What was exactly > > the issue that you were trying to solve? > > > > > > Sorry, I didn't read the manual carefully. > > I have two types of shell scripts. When selecting different types of > shell to execute script through $0, and did not work as expected. Then > I looked at the source code and could specify the shell through the > $SHELL environment variable, but finally sh was executed, so, I thought > it was a bug. If your scripts are executable script -c ./foo (with foo being your script) should work. sh would exec `foo' which -hopefully- has the correct interpreter in its shebang (i.e. csh, sh, ksh, perl, lua...)
csh.1 markup fix
noticed because it renders oddly: kill %job kill[-s signal_name] pid kill -sig pid ... The Op on its own line becomes part of the item body instead of the item itself. Use an in-line Oo ... Oc instead, ok? diff /usr/src commit - 5e332dd6b4d6aadab29719a5f3abea4d0432c45b path + /usr/src blob - 25b0cec15349ac00ee147cd114b6d7413a65 file + bin/csh/csh.1 --- bin/csh/csh.1 +++ bin/csh/csh.1 @@ -1792,9 +1792,7 @@ option lists process IDs in addition to the normal inf option lists process IDs in addition to the normal information. .Pp .It Ic kill % Ns Ar job -.It Ic kill -.Op Fl s Ar signal_name -.Ar pid +.It Ic kill Oo Fl s Ar signal_name Oc Ar pid .It Ic kill Fl sig Ar pid ... .It Ic kill Fl l Op exit_status Sends either the
Re: [patch] /usr.bin/script: Fix process name hardcode for shell subprocesses
Hello, On 2023/03/29 22:39:52 +0800, lux wrote: > Hi, the name of the shell subprocess is hardcoded in the `script' > command. > > The test is as follows: > > $ echo $SHELL > /bin/ksh > $ script -c 'echo $0' > Script started, output file is typescript > sh < The correct one should be 'ksh' > Script done, output file is typescript > > I fixed. I'm not sure your fix is correct. Unlike the similar issue in mg, $SHELL here is only used in case no -c flag was given and an interactive one is spawned 331 execl(shell, shell, "-i", (char *)NULL); In the other case (script -c) /bin/sh is used *unconditionally* 328 execv(_PATH_BSHELL, argp); If you don't trust my words, here's an example: (running with your patch applied) % SHELL=/bin/bash ./obj/script -c 'echo $0' Script started, output file is typescript bash Script done, output file is typescript % file /bin/bash /bin/bash: cannot stat '/bin/bash' (No such file or directory) There's no /bin/bash but script -c succeeded. Why? Because it always runs /bin/sh when invoked with the -c flag! Furthermore, the man page explicitly states that script -c runs sh(1): -c command Run sh -c command, instead of an interactive shell. [...] So I'd say that script(1) is working as intended. What was exactly the issue that you were trying to solve?
mg: don't access(conffile), just open it
There's this "pattern" in mg where it first access(2) a path, then
does a buch of other things and finally opens it. It can do better.
Diff below removes the access() calls for conffile handling.
startupfile() now does an ffropen() (fopen() wrapper) instead of
access() and returns the file via parameter; callers will pass it to
load() and then close it.
There's one more subtle fix in here that may not be obvious on first
look. I've moved the adjustname() call from load() to evalfile() to
avoid doing a double tilde expansion. All call to load(), except
evalfile(), have a path that's been constructed by mg itself or it has
been passed by the user via -b/-u, thus not need to be "adjusted".
Consider:
% mkdir \~ ; mv ~/.mg \~/ ; mg -u \~/.mg
which admittedly is a very corner case but shows the problem in doing
an extra adjustname().
evalfile() -- aka M-x load -- prompts the user for a path so it's not
unreasonable to call adjustname() on it.
ok?
Index: def.h
===
RCS file: /home/cvs/src/usr.bin/mg/def.h,v
retrieving revision 1.177
diff -u -p -r1.177 def.h
--- def.h 20 Oct 2022 18:59:24 - 1.177
+++ def.h 30 Mar 2023 11:04:59 -
@@ -486,7 +486,7 @@ int ffputbuf(FILE *, struct buffer *,
int ffgetline(FILE *, char *, int, int *);
int fbackupfile(const char *);
char *adjustname(const char *, int);
-char *startupfile(char *, char *);
+char *startupfile(FILE **, char *, char *);
int copy(char *, char *);
struct list*make_file_list(char *);
int fisdir(const char *);
@@ -594,7 +594,7 @@ int extend(int, int);
int evalexpr(int, int);
int evalbuffer(int, int);
int evalfile(int, int);
-int load(const char *);
+int load(FILE *, const char *);
int excline(char *, int, int);
char *skipwhite(char *);
Index: extend.c
===
RCS file: /home/cvs/src/usr.bin/mg/extend.c,v
retrieving revision 1.77
diff -u -p -r1.77 extend.c
--- extend.c8 Mar 2023 04:43:11 - 1.77
+++ extend.c30 Mar 2023 11:04:59 -
@@ -620,37 +620,37 @@ evalbuffer(int f, int n)
int
evalfile(int f, int n)
{
+ FILE*ffp;
char fname[NFILEN], *bufp;
+ int ret;
if ((bufp = eread("Load file: ", fname, NFILEN,
EFNEW | EFCR)) == NULL)
return (ABORT);
- else if (bufp[0] == '\0')
+ if (bufp[0] == '\0')
return (FALSE);
- return (load(fname));
+ if ((bufp = adjustname(fname, TRUE)) == NULL)
+ return (FALSE);
+ ret = ffropen(, bufp, NULL);
+ if (ret != FIOSUC) {
+ if (ret == FIODIR)
+ (void)ffclose(ffp, NULL);
+ return (FALSE);
+ }
+ ret = load(ffp, bufp);
+ (void)ffclose(ffp, NULL);
+ return (ret);
}
/*
* load - go load the file name we got passed.
*/
int
-load(const char *fname)
+load(FILE *ffp, const char *fname)
{
- int s = TRUE, line, ret;
+ int s = TRUE, line;
int nbytes = 0;
char excbuf[BUFSIZE], fncpy[NFILEN];
- FILE*ffp;
-
- if ((fname = adjustname(fname, TRUE)) == NULL)
- /* just to be careful */
- return (FALSE);
-
- ret = ffropen(, fname, NULL);
- if (ret != FIOSUC) {
- if (ret == FIODIR)
- (void)ffclose(ffp, NULL);
- return (FALSE);
- }
/* keep a note of fname in case of errors in loaded file. */
(void)strlcpy(fncpy, fname, sizeof(fncpy));
@@ -666,7 +666,6 @@ load(const char *fname)
break;
}
}
- (void)ffclose(ffp, NULL);
excbuf[nbytes] = '\0';
if (s != FIOEOF || (nbytes && excline(excbuf, nbytes, ++line) != TRUE))
return (FALSE);
Index: fileio.c
===
RCS file: /home/cvs/src/usr.bin/mg/fileio.c,v
retrieving revision 1.110
diff -u -p -r1.110 fileio.c
--- fileio.c30 Mar 2023 07:26:15 - 1.110
+++ fileio.c30 Mar 2023 11:04:59 -
@@ -329,7 +329,7 @@ adjustname(const char *fn, int slashslas
* to the startup file name.
*/
char *
-startupfile(char *suffix, char *conffile)
+startupfile(FILE **ffp, char *suffix, char *conffile)
{
static char file[NFILEN];
char*home;
@@ -350,8 +350,13 @@ startupfile(char *suffix, char *conffile
return (NULL);
}
- if (access(file, R_OK) == 0)
+ ret = ffropen(ffp, file, NULL);
+ if (ret == FIOSUC)
return (file);
+ if (ret == FIODIR)
+ (void)ffclose(*ffp, NULL);
+ *ffp = NULL;
+
mg: drop needless global tagsfn path
mg keeps the path to the last loaded tag file in tagsfn which was used
both for the lazily loading (now removed) and as a flag to know if any
tags are currently loaded. It's redundant, we can just check if the
rb tree is empty instead.
The only difference I can think of is with the corner cases of an
empty tags file (echo -n > tags): with diff below mg would always ask
the user which tag file to load on M-. (find-tag) where currently it
asks only on the first try. I don't think it's an issue.
ok?
Index: tags.c
===
RCS file: /cvs/src/usr.bin/mg/tags.c,v
retrieving revision 1.25
diff -u -p -r1.25 tags.c
--- tags.c 29 Mar 2023 07:29:17 - 1.25
+++ tags.c 29 Mar 2023 08:07:23 -
@@ -38,8 +38,6 @@ static void unloadtags(void
#define DEFAULTFN "tags"
-char *tagsfn = NULL;
-
/* ctags(1) entries are parsed and maintained in a tree. */
struct ctag {
RB_ENTRY(ctag) entry;
@@ -87,42 +85,18 @@ tagsvisit(int f, int n)
if (bufp == NULL)
return (ABORT);
- if (tagsfn == NULL) {
- if (bufp[0] == '\0') {
- if ((tagsfn = strdup(fname)) == NULL) {
- dobeep();
- ewprintf("Out of memory");
- return (FALSE);
- }
- } else {
- /* bufp points to local variable, so duplicate. */
- if ((tagsfn = strdup(bufp)) == NULL) {
- dobeep();
- ewprintf("Out of memory");
- return (FALSE);
- }
- }
- } else {
- if ((temp = strdup(bufp)) == NULL) {
- dobeep();
- ewprintf("Out of memory");
- return (FALSE);
- }
- free(tagsfn);
- tagsfn = temp;
+ if (!RB_EMPTY()) {
if (eyorn("Keep current list of tags table also") == FALSE) {
ewprintf("Starting a new list of tags table");
unloadtags();
}
}
- if (loadtags(tagsfn) == FALSE) {
- free(tagsfn);
- tagsfn = NULL;
- return (FALSE);
- }
+ temp = bufp;
+ if (temp[0] == '\0')
+ temp = fname;
- return (TRUE);
+ return (loadtags(temp));
}
/*
@@ -156,7 +130,7 @@ findtag(int f, int n)
return (FALSE);
}
- if (tagsfn == NULL)
+ if (RB_EMPTY())
if ((ret = tagsvisit(f, n)) != TRUE)
return (ret);
return pushtag(tok);
@@ -328,7 +302,6 @@ closetags(void)
free(s);
}
unloadtags();
- free(tagsfn);
}
/*
Re: mg: fix tagfile parsing
On 2023/03/28 22:25:42 +0200, Theo Buehler wrote:
> Contrary to what I convinced op@ to be the case, duplicate tags may exist
> in legitimate tags files. So we should ignore duplicates rather than
> erroring on them. This fixes parsing the /var/db/libc.tags file.
>
> $ grep -wc ^memcpy /var/db/libc.tags
> 2
oh... good to know that there may be duplicates. it would be nice to
have mg record all of the tags (dups included) and allow the user to
choose where to jump when there are multiple location. Overkill for
now however.
OK for me, thanks!
> Index: tags.c
> ===
> RCS file: /cvs/src/usr.bin/mg/tags.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 tags.c
> --- tags.c22 Mar 2023 22:09:37 - 1.23
> +++ tags.c28 Mar 2023 19:55:17 -
> @@ -388,8 +388,10 @@ addctag(char *s)
> if (*l == '\0')
> goto cleanup;
> t->pat = strip(l, strlen(l));
> - if (RB_INSERT(tagtree, , t) != NULL)
> - goto cleanup;
> + if (RB_INSERT(tagtree, , t) != NULL) {
> + free(t);
> + free(s);
> + }
> return (TRUE);
> cleanup:
> free(t);
mg: don't load tags files lazily
tagsvisit (aka `visit-tags-table') records the path to the tags file
which is lazily opened upon find-tags (aka M-.). visit-tags-table
tries also to be smart and checks that the argument is really a file
using a stat + access dance, and loadtags() which will be called way
later just opens the stored path and parses it, trusting the checks
done before...
Let's make tagsvisit actually loading the tag file instead and
loadtags do the checks instead, which also happens to mirror what GNU
Emacs does.
ok?
Index: mg.1
===
RCS file: /cvs/src/usr.bin/mg/mg.1,v
retrieving revision 1.127
diff -u -p -r1.127 mg.1
--- mg.120 Oct 2022 18:59:24 - 1.127
+++ mg.128 Mar 2023 19:10:41 -
@@ -967,7 +967,7 @@ As it moves, convert any characters to u
Toggle the visible bell.
If this toggle is on, the modeline will flash.
.It visit-tags-table
-Record name of the tags file to be used for subsequent find-tag.
+Load tags file to be used for subsequent find-tag.
.It what-cursor-position
Display a bunch of useful information about the current location of
dot.
Index: tags.c
===
RCS file: /cvs/src/usr.bin/mg/tags.c,v
retrieving revision 1.23
diff -u -p -r1.23 tags.c
--- tags.c 22 Mar 2023 22:09:37 - 1.23
+++ tags.c 28 Mar 2023 19:00:02 -
@@ -39,7 +39,6 @@ static void unloadtags(void
#define DEFAULTFN "tags"
char *tagsfn = NULL;
-int loaded = FALSE;
/* ctags(1) entries are parsed and maintained in a tree. */
struct ctag {
@@ -74,7 +73,6 @@ int
tagsvisit(int f, int n)
{
char fname[NFILEN], *bufp, *temp;
- struct stat sb;
if (getbufcwd(fname, sizeof(fname)) == FALSE)
fname[0] = '\0';
@@ -90,20 +88,6 @@ tagsvisit(int f, int n)
if (bufp == NULL)
return (ABORT);
- if (stat(bufp, ) == -1) {
- dobeep();
- ewprintf("stat: %s", strerror(errno));
- return (FALSE);
- } else if (S_ISREG(sb.st_mode) == 0) {
- dobeep();
- ewprintf("Not a regular file");
- return (FALSE);
- } else if (access(bufp, R_OK) == -1) {
- dobeep();
- ewprintf("Cannot access file %s", bufp);
- return (FALSE);
- }
-
if (tagsfn == NULL) {
if (bufp[0] == '\0') {
if ((tagsfn = strdup(fname)) == NULL) {
@@ -131,8 +115,14 @@ tagsvisit(int f, int n)
ewprintf("Starting a new list of tags table");
unloadtags();
}
- loaded = FALSE;
}
+
+ if (loadtags(tagsfn) == FALSE) {
+ free(tagsfn);
+ tagsfn = NULL;
+ return (FALSE);
+ }
+
return (TRUE);
}
@@ -170,14 +160,6 @@ findtag(int f, int n)
if (tagsfn == NULL)
if ((ret = tagsvisit(f, n)) != TRUE)
return (ret);
- if (!loaded) {
- if (loadtags(tagsfn) == FALSE) {
- free(tagsfn);
- tagsfn = NULL;
- return (FALSE);
- }
- loaded = TRUE;
- }
return pushtag(tok);
}
@@ -300,12 +282,25 @@ poptag(int f, int n)
int
loadtags(const char *fn)
{
+ struct stat sb;
char *l;
FILE *fd;
if ((fd = fopen(fn, "r")) == NULL) {
dobeep();
ewprintf("Unable to open tags file: %s", fn);
+ return (FALSE);
+ }
+ if (fstat(fileno(fd), ) == -1) {
+ dobeep();
+ ewprintf("fstat: %s", strerror(errno));
+ fclose(fd);
+ return (FALSE);
+ }
+ if (!S_ISREG(sb.st_mode)) {
+ dobeep();
+ ewprintf("Not a regular file");
+ fclose(fd);
return (FALSE);
}
while ((l = fparseln(fd, NULL, NULL, "\0",
Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL
On 2023/03/28 10:21:59 +0200, Omar Polo wrote:
> On 2023/03/27 18:58:07 -0600, Todd C. Miller wrote:
> > It might be best to use the basename of the actual shell for argv[0].
> > Our ksh for instance has slightly different behavior when invoked
> > as sh.
>
> like this? :)
>
> (need an up-to-date tree since it builds on the previous, committed,
> diff.)
>
> I've tested with ksh and csh. I guess it's fine to assume the user'
> shell has a -c flag. (vi does the same.)
>
>
> Thanks!
sigh... forgot to advance the pointer after strrchr otherwise argv[0]
would have been /ksh instead of "ksh".
Index: region.c
===
RCS file: /cvs/src/usr.bin/mg/region.c,v
retrieving revision 1.43
diff -u -p -r1.43 region.c
--- region.c28 Mar 2023 08:01:40 - 1.43
+++ region.c28 Mar 2023 14:13:26 -
@@ -34,7 +34,7 @@ staticint iomux(int, char * const, int,
static int preadin(int, struct buffer *);
static voidpwriteout(int, char **, int *);
static int setsize(struct region *, RSIZE);
-static int shellcmdoutput(char * const[], char * const, int);
+static int shellcmdoutput(char * const, char * const, int);
/*
* Kill the region. Ask "getregion" to figure out the bounds of the region.
@@ -415,7 +415,6 @@ piperegion(int f, int n)
struct region region;
int len;
char *cmd, cmdbuf[NFILEN], *text;
- char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
/* C-u M-| is not supported yet */
if (n > 1)
@@ -431,8 +430,6 @@ piperegion(int f, int n)
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
return (ABORT);
- argv[2] = cmd;
-
if (getregion() != TRUE)
return (FALSE);
@@ -446,7 +443,7 @@ piperegion(int f, int n)
region_get_data(, text, len);
- return shellcmdoutput(argv, text, len);
+ return shellcmdoutput(cmd, text, len);
}
/*
@@ -456,7 +453,6 @@ int
shellcommand(int f, int n)
{
char *cmd, cmdbuf[NFILEN];
- char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
if (n > 1)
return (ABORT);
@@ -465,15 +461,14 @@ shellcommand(int f, int n)
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
return (ABORT);
- argv[2] = cmd;
-
- return shellcmdoutput(argv, NULL, 0);
+ return shellcmdoutput(cmd, NULL, 0);
}
int
-shellcmdoutput(char* const argv[], char* const text, int len)
+shellcmdoutput(char* const cmd, char* const text, int len)
{
struct buffer *bp;
+ char*argv[] = {NULL, "-c", cmd, NULL};
char*shellp;
int ret;
@@ -486,6 +481,11 @@ shellcmdoutput(char* const argv[], char*
if ((shellp = getenv("SHELL")) == NULL)
shellp = _PATH_BSHELL;
+
+ if ((argv[0] = strrchr(shellp, '/')) != NULL)
+ argv[0]++;
+ else
+ argv[0] = shellp;
ret = pipeio(shellp, argv, text, len, bp);
Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL
On 2023/03/28 17:02:18 +0800, lux wrote:
> On Mon, 2023-03-27 at 18:58 -0600, Todd C.Miller wrote:
> >
> > > - _exit(1);
> > > - if (path == NULL)
> > > _exit(1);
> > >
>
> Hi, `pipeio' looks like a common function, so maby called in others
> code, checking the path is NULL is a safe check, to prevent writing
> wrong code, I think the condition that path is NULL should not be
> removed.
pipeio() is a common _internal_ function. There are requirements that
callers need to fulfill when calling other functions. Otherwise you'd
have to check also that argv is non-NULL and that it is NULL
terminated, that len is non-negative, that text is a valid pointer if
len is positive, that outbp is non-NULL and a valid pointer etc.
Quite a few checks for a function only called twice and always with
proper parameters :)
% grep 'pipeio(' *.c
buffer.c: ret = pipeio(DIFFTOOL, argv, text, len, bp);
region.c: ret = pipeio(shellp, argv, text, len, bp);
region.c:pipeio(const char* const path, char* const argv[],
Furthermore, path is only looked at in the child process after fork(),
even for the paranoids it won't cause issues in the editor itself.
So I don't think we need to be pedantic and check the path there given
that 1. it is always called with proper arguments and 2. there's no
way it could do something useful with a NULL first argument.
I should have added a note about this in the commit message.
apologies.
Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL
On 2023/03/27 18:58:07 -0600, Todd C. Miller wrote:
> On Mon, 27 Mar 2023 20:06:30 +0200, Omar Polo wrote:
>
> > Is _PATH_BSHELL portable though? I can see a few stuff that uses it
> > (vi among others) but I'm not sure.
>
> The paths.h header is a BSD invention, though it may be present on
> some other systems. I don't think that's a reason not to use it
> though.
>
> > Also, if you look at the callers of shellcmdoutput() they all prepare
> > the argv array as {"sh", "-c", "command provided", NULL} so I'm
> > wondering if we should just ignore $SHELL and always use /bin/sh, or
> > change that "sh" accordingly to $SHELL.
>
> It might be best to use the basename of the actual shell for argv[0].
> Our ksh for instance has slightly different behavior when invoked
> as sh.
like this? :)
(need an up-to-date tree since it builds on the previous, committed,
diff.)
I've tested with ksh and csh. I guess it's fine to assume the user'
shell has a -c flag. (vi does the same.)
Thanks!
Index: region.c
===
RCS file: /cvs/src/usr.bin/mg/region.c,v
retrieving revision 1.43
diff -u -p -r1.43 region.c
--- region.c28 Mar 2023 08:01:40 - 1.43
+++ region.c28 Mar 2023 08:17:30 -
@@ -34,7 +34,7 @@ staticint iomux(int, char * const, int,
static int preadin(int, struct buffer *);
static voidpwriteout(int, char **, int *);
static int setsize(struct region *, RSIZE);
-static int shellcmdoutput(char * const[], char * const, int);
+static int shellcmdoutput(char * const, char * const, int);
/*
* Kill the region. Ask "getregion" to figure out the bounds of the region.
@@ -415,7 +415,6 @@ piperegion(int f, int n)
struct region region;
int len;
char *cmd, cmdbuf[NFILEN], *text;
- char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
/* C-u M-| is not supported yet */
if (n > 1)
@@ -431,8 +430,6 @@ piperegion(int f, int n)
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
return (ABORT);
- argv[2] = cmd;
-
if (getregion() != TRUE)
return (FALSE);
@@ -446,7 +443,7 @@ piperegion(int f, int n)
region_get_data(, text, len);
- return shellcmdoutput(argv, text, len);
+ return shellcmdoutput(cmd, text, len);
}
/*
@@ -456,7 +453,6 @@ int
shellcommand(int f, int n)
{
char *cmd, cmdbuf[NFILEN];
- char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
if (n > 1)
return (ABORT);
@@ -465,16 +461,15 @@ shellcommand(int f, int n)
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
return (ABORT);
- argv[2] = cmd;
-
- return shellcmdoutput(argv, NULL, 0);
+ return shellcmdoutput(cmd, NULL, 0);
}
int
-shellcmdoutput(char* const argv[], char* const text, int len)
+shellcmdoutput(char* const cmd, char* const text, int len)
{
struct buffer *bp;
- char*shellp;
+ char*argv[] = {NULL, "-c", cmd, NULL};
+ char*shellp, *shell;
int ret;
bp = bfind("*Shell Command Output*", TRUE);
@@ -486,6 +481,10 @@ shellcmdoutput(char* const argv[], char*
if ((shellp = getenv("SHELL")) == NULL)
shellp = _PATH_BSHELL;
+
+ if ((shell = strrchr(shellp, '/')) == NULL)
+ shell = shellp;
+ argv[0] = shell;
ret = pipeio(shellp, argv, text, len, bp);
Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL
Hello,
On 2023/03/28 00:53:05 +0800, lux wrote:
> Index: region.c
> ===
> RCS file: /cvs/src/usr.bin/mg/region.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 region.c
> --- region.c 8 Mar 2023 04:43:11 - 1.40
> +++ region.c 27 Mar 2023 16:48:08 -
> @@ -21,6 +21,7 @@
> #include
> #include
> #include
> +#include
>
> #include "def.h"
>
> @@ -486,9 +487,8 @@ shellcmdoutput(char* const argv[], char*
> return (FALSE);
> }
>
> - shellp = getenv("SHELL");
> -
> - ret = pipeio(shellp, argv, text, len, bp);
> + ret = pipeio((shellp = getenv("SHELL")) == NULL ? _PATH_BSHELL
> : shellp,
> + argv, text, len, bp);
>
> if (ret == TRUE) {
> eerase();
Thanks for the diff! I don't think it's a bad idea to fall back to
/bin/sh if $SHELL is (somehow) undefined, but I do have some nits on
the patch itself (which was mangled btw, make sure that your MUA won't
fold lines), but are mostly stylistics.
I'd avoid the ternary operator and choose another way to spell the
same thing that keeps the code more readable. Also, since we're
always going to call pipeio() with a non-NULL path now there's a check
there that becomes unnecessary. See patch below.
Is _PATH_BSHELL portable though? I can see a few stuff that uses it
(vi among others) but I'm not sure.
Also, if you look at the callers of shellcmdoutput() they all prepare
the argv array as {"sh", "-c", "command provided", NULL} so I'm
wondering if we should just ignore $SHELL and always use /bin/sh, or
change that "sh" accordingly to $SHELL.
Index: region.c
===
RCS file: /cvs/src/usr.bin/mg/region.c,v
retrieving revision 1.42
diff -u -p -r1.42 region.c
--- region.c27 Mar 2023 17:54:20 - 1.42
+++ region.c27 Mar 2023 17:58:11 -
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -483,7 +484,8 @@ shellcmdoutput(char* const argv[], char*
return (FALSE);
}
- shellp = getenv("SHELL");
+ if ((shellp = getenv("SHELL")) == NULL)
+ shellp = _PATH_BSHELL;
ret = pipeio(shellp, argv, text, len, bp);
@@ -529,8 +531,6 @@ pipeio(const char* const path, char* con
if (dup2(s[1], STDOUT_FILENO) == -1)
_exit(1);
if (dup2(s[1], STDERR_FILENO) == -1)
- _exit(1);
- if (path == NULL)
_exit(1);
execv(path, argv);
Re: [patch] usr.bin/mg/tags.c: Add free()
I've committed a slightly tweaked version of your diff:
diff /usr/src
commit - 17d31bf3ed9de46ea3449b69914ea1ac97418884
path + /usr/src
blob - f2976f9ac9b98c51cebae458ccd266c6442181bb
file + usr.bin/mg/tags.c
--- usr.bin/mg/tags.c
+++ usr.bin/mg/tags.c
@@ -369,12 +369,12 @@ addctag(char *l)
int
addctag(char *l)
{
- struct ctag *t;
+ struct ctag *t = NULL;
if ((t = malloc(sizeof(struct ctag))) == NULL) {
dobeep();
ewprintf("Out of memory");
- return (FALSE);
+ goto cleanup;
}
t->tag = l;
if ((l = strchr(l, '\t')) == NULL)
Thanks!
Re: [PATCH] cwm: fix transparency of 32 bit client's border
On 2023/03/16 17:01:27 +0100, Omar Polo wrote: > moving to tech@, +cc okan@ > > On 2023/03/16 11:07:26 +0100, Julien Blanchard wrote: > > Hello, > > Here is a patch that fixes a weird semi-transparency issue some apps > > have when using cwm with a compositor. The issue seems to be that the > > highest significant byte of the color is not initialized. > > According to xwininfo alacritty has a depth of 32 and xterm has a > > depth of 24. When using a compositor the windows with a depth of 32 > > don't have the proper border color. > > > > [...] > > > > The same issue happens with all windows that have a depth of 32 > > (firefox, thunderbird...). > > It fixes the borders for firefox and chromium for me with xcompmgr and > doesn't seem to cause issue with the "other" windows (xterm, some gtk3 > stuff, ...), thanks! > > (reattaching Julien' diff) (sorry for the delay) I've committed it with an ok from okan@, thank you! > Index: client.c > === > RCS file: /home/cvs/xenocara/app/cwm/client.c,v > retrieving revision 1.266 > diff -u -p -r1.266 client.c > --- client.c 26 Feb 2022 15:19:18 - 1.266 > +++ client.c 16 Mar 2023 15:43:40 - > @@ -596,7 +596,7 @@ client_draw_border(struct client_ctx *cc > pixel = sc->xftcolor[CWM_COLOR_BORDER_URGENCY].pixel; > > XSetWindowBorderWidth(X_Dpy, cc->win, (unsigned int)cc->bwidth); > - XSetWindowBorder(X_Dpy, cc->win, pixel); > + XSetWindowBorder(X_Dpy, cc->win, pixel | (0xffu << 24)); > } > > static void
Re: smtpd: simplify token name extraction for %{name}
On 2023/03/19 08:11:27 -0600, Todd C. Miller wrote:
> The current code for extracting the token name from %{name} can be
> simplified by computing the token name length. The existing code
> copies "name}" to token[] using memcpy(), then strchr() to find the
> '}' and replace it with a NUL. Using strchr() here is fragile since
> token[] is not yet NUL-terminated. This is currently not a problem
> since there is an earlier check for '}' in the source string but
> it could be dangerous is the code changes further.
>
> I find it much simpler to compute the token name length, verify the
> length, copy the bytes and then explicitly NUL-terminate token.
> This results in less code and is more easily audited.
Agreed, I find it simpler too, and less fragile.
> I've also removed the duplicate check for *(pbuf+1) != '{'.
>
> OK?
(while I still have the details fresh in my mind) ok for me
Re: [PATCH] cwm: fix transparency of 32 bit client's border
moving to tech@, +cc okan@ On 2023/03/16 11:07:26 +0100, Julien Blanchard wrote: > Hello, > Here is a patch that fixes a weird semi-transparency issue some apps > have when using cwm with a compositor. The issue seems to be that the > highest significant byte of the color is not initialized. > According to xwininfo alacritty has a depth of 32 and xterm has a > depth of 24. When using a compositor the windows with a depth of 32 > don't have the proper border color. > > [...] > > The same issue happens with all windows that have a depth of 32 > (firefox, thunderbird...). It fixes the borders for firefox and chromium for me with xcompmgr and doesn't seem to cause issue with the "other" windows (xterm, some gtk3 stuff, ...), thanks! (reattaching Julien' diff) Index: client.c === RCS file: /home/cvs/xenocara/app/cwm/client.c,v retrieving revision 1.266 diff -u -p -r1.266 client.c --- client.c26 Feb 2022 15:19:18 - 1.266 +++ client.c16 Mar 2023 15:43:40 - @@ -596,7 +596,7 @@ client_draw_border(struct client_ctx *cc pixel = sc->xftcolor[CWM_COLOR_BORDER_URGENCY].pixel; XSetWindowBorderWidth(X_Dpy, cc->win, (unsigned int)cc->bwidth); - XSetWindowBorder(X_Dpy, cc->win, pixel); + XSetWindowBorder(X_Dpy, cc->win, pixel | (0xffu << 24)); } static void
Re: cd CDPATH is attempted before dir
On 2023/03/10 11:16:31 +, s...@disroot.org wrote: > I believe since the given directory (argument) is not an absolute path; > it attempts to search in CDPATH before checking if the directory > exists. This will cause any attempts to cd into a directory to fail. > > I also believe that the given directory to cd should be preferred over > CDPATH; what i mean by this is if a directory exists within CDPATH but > also exists within the current working directory; the latter should be > preferred. Agreed. I've been bitten by this too, and I've "fixed" it by explicitly listing "." in front of CDPATH: % echo $CDPATH .:/home/op/w:/usr/ports:/usr/ports/mystuff:/home/op/quicklisp/local-projects To be fair I've not checked how other shells handle CDPATH nor if something is specified by POSIX in this regard, but adding "." in front doesn't seem such a bad workaround. HTH
Re: mg: handle prefix argument in shell-command{,-on-region}
friendly 12 week ping :)
On 2022/12/15 09:19:27 +0100, Omar Polo wrote:
> > > > On 2022/10/13 12:25:00 +0200, Omar Polo wrote:
> > > > > shell-command (M-!) and shell-command-on-region (M-|) works by
> > > > > displaying the output of the command in a new buffer, but in emacs
> > > > > using a prefix argument (C-u) allows to operate on the current buffer.
> > > > >
> > > > > diff belows adds that for mg. I can finally C-u M-! got diff RET when
> > > > > composing mails :)
> > > > >
> > > > > A possible drawback is that now the *Shell Command Output* buffer
> > > > > gains an undo history. linsert is also possibly slower than addline
> > > > > but on the plus side we're no more limited to BUFSIZ long lines.
> > > > >
> > > > > ok/comments/improvements?
> > > >
> > > > Here's a slightly tweaked version that adds a missing parens around a
> > > > return value and uses ssize_t for some vars in preadin. it also changes
> > > > the size read(2) from BUFSIZ-1 to BUFSIZ since we no longer need to NUL
> > > > terminate it.
> > > >
> > > > This has been more useful than I originally expected. I wanted it to
> > > > include diffs and the like more easily, now i'm using it also for all
> > > > sorts of stuff that mg doesn't do out-of-the-box (like using C-u M-|
> > > > sort RET instead of M-x sort-lines.)
> > > >
> > > > If it were for me, M-| and M-! would operate by default on the buffer
> > > > and with C-u on a scratch one, but this is what emacs does and i'm
> > > > probably several decades too late :)
diffstat /home/op/w/mg
M region.c | 50+ 59-
1 file changed, 50 insertions(+), 59 deletions(-)
diff /home/op/w/mg
commit - 4e440c72920f8bfbe8fe8a752bd5eafff5c4c644
path + /home/op/w/mg
blob - 21c5174f52d21103b9cd15942620eb746b5069b2
file + region.c
--- region.c
+++ region.c
@@ -26,14 +26,13 @@ static char leftover[BUFSIZ];
#define TIMEOUT 1
-static char leftover[BUFSIZ];
-
static int getregion(struct region *);
static int iomux(int, char * const, int, struct buffer *);
static int preadin(int, struct buffer *);
static voidpwriteout(int, char **, int *);
static int setsize(struct region *, RSIZE);
-static int shellcmdoutput(char * const[], char * const, int);
+static int shellcmdoutput(char * const[], char * const, int,
+ struct buffer *);
/*
* Kill the region. Ask "getregion" to figure out the bounds of the region.
@@ -419,14 +418,11 @@ piperegion(int f, int n)
piperegion(int f, int n)
{
struct region region;
+ struct buffer *bp = NULL;
int len;
char *cmd, cmdbuf[NFILEN], *text;
char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
- /* C-u M-| is not supported yet */
- if (n > 1)
- return (ABORT);
-
if (curwp->w_markp == NULL) {
dobeep();
ewprintf("The mark is not set now, so there is no region");
@@ -452,7 +448,13 @@ piperegion(int f, int n)
region_get_data(, text, len);
- return shellcmdoutput(argv, text, len);
+ if (n > 1) {
+ bp = curbp;
+ killregion(FFRAND, 1);
+ kdelete();
+ }
+
+ return (shellcmdoutput(argv, text, len, bp));
}
/*
@@ -462,12 +464,12 @@ shellcommand(int f, int n)
int
shellcommand(int f, int n)
{
-
+ struct buffer *bp = NULL;
char *cmd, cmdbuf[NFILEN];
char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
if (n > 1)
- return (ABORT);
+ bp = curbp;
if ((cmd = eread("Shell command: ", cmdbuf, sizeof(cmdbuf),
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
@@ -475,36 +477,43 @@ shellcommand(int f, int n)
argv[2] = cmd;
- return shellcmdoutput(argv, NULL, 0);
+ return (shellcmdoutput(argv, NULL, 0, bp));
}
-
int
-shellcmdoutput(char* const argv[], char* const text, int len)
+shellcmdoutput(char* const argv[], char* const text, int len,
+struct buffer *bp)
{
-
- struct buffer *bp;
+ struct mgwin *wp;
char*shellp;
- int ret;
+ int ret, special = 0;
- bp = bfind("*Shell Command Output*", TRUE);
- bp->b_flag |= BFREADONLY;
- if (bclear(bp) != TRUE) {
- free(text);
- return (FALSE);
+ if (bp == NULL) {
+ special = 1;
+ bp = bfind("*Shell Command Output*", TRUE);
+ bp->b_flag &= ~BFREADONLY; /* di
mg: M-x grep: don't append /dev/null to cmdline
One (midly annoying) feature of mg is that it appends /dev/null to the
grep invocation. I guess this was done so grep is never called with
only one file argument and so it always displays the filename in its
output.
However, this gets a bit annoying when one only types "grep -n -R foo"
and yields no results because only /dev/null was consulted.
My proposal is to add -H to the mix instead.
This poses a problem: it gets too easy to "hang" mg by forgetting the
path to the files: grep would read from stdin and since inherits the
cooked mode ^D and ^C don't work. Diff below works around the problem
in the easier way: redirecting stdin from /dev/null. (a follow-up
patch could rework compile_mode and clean it up a bit, avoiding the
cd'ing back and forth too, by just forking a child and redirect its
stdout with a pipe.)
(note that this issue is already present: M-x grep RET C-u "cat -" RET)
ok?
diff /home/op/w/mg.orig
commit - b83db95072ea94d64c0bb6027c4b3478e3400e5c
path + /home/op/w/mg.orig
blob - 016256f64d06be49304999fa4665018e4f823d31
file + grep.c
--- grep.c
+++ grep.c
@@ -69,14 +69,12 @@ grep(int f, int n)
struct buffer *bp;
struct mgwin*wp;
- (void)strlcpy(cprompt, "grep -n ", sizeof(cprompt));
+ (void)strlcpy(cprompt, "grep -Hn ", sizeof(cprompt));
if ((bufp = eread("Run grep: ", cprompt, NFILEN,
EFDEF | EFNEW | EFCR)) == NULL)
return (ABORT);
else if (bufp[0] == '\0')
return (FALSE);
- if (strlcat(cprompt, " /dev/null", sizeof(cprompt)) >= sizeof(cprompt))
- return (FALSE);
if ((bp = compile_mode("*grep*", cprompt)) == NULL)
return (FALSE);
@@ -189,7 +187,7 @@ compile_mode(const char *name, const char *command)
buf = NULL;
sz = 0;
- n = snprintf(qcmd, sizeof(qcmd), "%s 2>&1", command);
+ n = snprintf(qcmd, sizeof(qcmd), "%s 2>&1 = sizeof(qcmd))
return (NULL);
Re: units(1): support personal library
On 2022/12/24 11:56:37 +0100, Florian Obser wrote:
> This is at least supported by FreeBSD's units(1) as well as by
> systemd/Linux.
>
> [...]
>
> OK?
it's really handy, ok for me! (with s/options/option as mentioned by
Crystal Kolipe.)
the -f '' seems a bit weird to be fair, but it's also possible to do
-f /usr/share/misc/units.lib now at least :)
> diff --git units.1 units.1
> index d7a45f729b3..916d1b03d32 100644
> --- units.1
> +++ units.1
> @@ -79,6 +79,11 @@ The options are as follows:
> .Bl -tag -width Ds
> .It Fl f Ar filename
> Specifies the name of the units data file to load.
> +This options may be specified multiple times.
> +The standard units library is read if
> +.Ar filename
> +is the empty string.
> +This allows extending the standard units library with a personal library.
> .It Fl q
> Suppresses prompting of the user for units and the display of statistics
> about the number of units loaded.
> diff --git units.c units.c
> index 98af5031fb1..488795c78cb 100644
> --- units.c
> +++ units.c
> @@ -100,7 +100,6 @@ readunits(char *userfile)
> int len, linenum, i;
> FILE *unitfile;
>
> - unitcount = 0;
> linenum = 0;
>
> if (userfile) {
> @@ -626,8 +625,7 @@ main(int argc, char **argv)
> struct unittype have, want;
> char havestr[81], wantstr[81];
> int optchar;
> - char *userfile = 0;
> - int quiet = 0;
> + int quiet = 0, units_read = 0;
>
> extern char *optarg;
> extern int optind;
> @@ -638,7 +636,8 @@ main(int argc, char **argv)
> while ((optchar = getopt(argc, argv, "vqf:")) != -1) {
> switch (optchar) {
> case 'f':
> - userfile = optarg;
> + units_read = 1;
> + readunits(*optarg == '\0' ? NULL : optarg);
> break;
> case 'q':
> quiet = 1;
> @@ -662,7 +661,8 @@ main(int argc, char **argv)
> if (argc != 3 && argc != 2 && argc != 0)
> usage();
>
> - readunits(userfile);
> + if (!units_read)
> + readunits(NULL);
>
> if (pledge("stdio", NULL) == -1)
> err(1, "pledge");
Re: [patch] arp.c spaces nested between tabs
On 2022/12/28 12:21:07 -0500, Paul Tagliamonte wrote:
> [2]: {openbsd monks avert your eyes: gnu grep and bash w/ 'shopt -s extglob'}:
> $ grep -P '\t +\t' !(gnu|sys) -ril
ksh actually supports that style of globbing :)
% ls -d !(gnu|sys)
Makefiledistrib/include/regress/usr.bin/
Makefile.cross etc/lib/sbin/ usr.sbin/
bin/games/ libexec/share/
Re: mg: handle prefix argument in shell-command{,-on-region}
ping...
On 2022/11/22 11:53:35 +0100, Omar Polo wrote:
> anyone?
>
> On 2022/11/09 09:00:03 +0100, Omar Polo wrote:
> > bump
> >
> > On 2022/10/25 14:30:51 +0200, Omar Polo wrote:
> > > On 2022/10/13 12:25:00 +0200, Omar Polo wrote:
> > > > shell-command (M-!) and shell-command-on-region (M-|) works by
> > > > displaying the output of the command in a new buffer, but in emacs
> > > > using a prefix argument (C-u) allows to operate on the current buffer.
> > > >
> > > > diff belows adds that for mg. I can finally C-u M-! got diff RET when
> > > > composing mails :)
> > > >
> > > > A possible drawback is that now the *Shell Command Output* buffer
> > > > gains an undo history. linsert is also possibly slower than addline
> > > > but on the plus side we're no more limited to BUFSIZ long lines.
> > > >
> > > > ok/comments/improvements?
> > >
> > > Here's a slightly tweaked version that adds a missing parens around a
> > > return value and uses ssize_t for some vars in preadin. it also changes
> > > the size read(2) from BUFSIZ-1 to BUFSIZ since we no longer need to NUL
> > > terminate it.
> > >
> > > This has been more useful than I originally expected. I wanted it to
> > > include diffs and the like more easily, now i'm using it also for all
> > > sorts of stuff that mg doesn't do out-of-the-box (like using C-u M-|
> > > sort RET instead of M-x sort-lines.)
> > >
> > > If it were for me, M-| and M-! would operate by default on the buffer
> > > and with C-u on a scratch one, but this is what emacs does and i'm
> > > probably several decades too late :)
diff 9c0e33d3f3f118f7aa73cd906c7d12ecfec75a15
ed2bd21549cae0a899f3cfd09f49568210d9430e
commit - 9c0e33d3f3f118f7aa73cd906c7d12ecfec75a15
commit + ed2bd21549cae0a899f3cfd09f49568210d9430e
blob - 21c5174f52d21103b9cd15942620eb746b5069b2
blob + 1bee60b37ceea3f9ec3ceb779727f877a7f15851
--- usr.bin/mg/region.c
+++ usr.bin/mg/region.c
@@ -26,14 +26,13 @@ static char leftover[BUFSIZ];
#define TIMEOUT 1
-static char leftover[BUFSIZ];
-
static int getregion(struct region *);
static int iomux(int, char * const, int, struct buffer *);
static int preadin(int, struct buffer *);
static voidpwriteout(int, char **, int *);
static int setsize(struct region *, RSIZE);
-static int shellcmdoutput(char * const[], char * const, int);
+static int shellcmdoutput(char * const[], char * const, int,
+ struct buffer *);
/*
* Kill the region. Ask "getregion" to figure out the bounds of the region.
@@ -419,14 +418,11 @@ piperegion(int f, int n)
piperegion(int f, int n)
{
struct region region;
+ struct buffer *bp = NULL;
int len;
char *cmd, cmdbuf[NFILEN], *text;
char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
- /* C-u M-| is not supported yet */
- if (n > 1)
- return (ABORT);
-
if (curwp->w_markp == NULL) {
dobeep();
ewprintf("The mark is not set now, so there is no region");
@@ -452,7 +448,13 @@ piperegion(int f, int n)
region_get_data(, text, len);
- return shellcmdoutput(argv, text, len);
+ if (n > 1) {
+ bp = curbp;
+ killregion(FFRAND, 1);
+ kdelete();
+ }
+
+ return (shellcmdoutput(argv, text, len, bp));
}
/*
@@ -462,12 +464,12 @@ shellcommand(int f, int n)
int
shellcommand(int f, int n)
{
-
+ struct buffer *bp = NULL;
char *cmd, cmdbuf[NFILEN];
char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
if (n > 1)
- return (ABORT);
+ bp = curbp;
if ((cmd = eread("Shell command: ", cmdbuf, sizeof(cmdbuf),
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
@@ -475,36 +477,43 @@ shellcommand(int f, int n)
argv[2] = cmd;
- return shellcmdoutput(argv, NULL, 0);
+ return (shellcmdoutput(argv, NULL, 0, bp));
}
-
int
-shellcmdoutput(char* const argv[], char* const text, int len)
+shellcmdoutput(char* const argv[], char* const text, int len,
+struct buffer *bp)
{
-
- struct buffer *bp;
+ struct mgwin *wp;
char*shellp;
- int ret;
+ int ret, special = 0;
- bp = bfind("*Shell Command Output*", TRUE);
- bp->b_flag |= BFREADONLY;
- if (bclear(bp) != TRUE) {
- free(text);
- return (FALSE);
+ if (bp == NULL) {
+ special = 1;
+ bp = bfind("*Shell Command Output*", TRUE);
+
Re: mg: handle prefix argument in shell-command{,-on-region}
anyone?
On 2022/11/09 09:00:03 +0100, Omar Polo wrote:
> bump
>
> On 2022/10/25 14:30:51 +0200, Omar Polo wrote:
> > On 2022/10/13 12:25:00 +0200, Omar Polo wrote:
> > > shell-command (M-!) and shell-command-on-region (M-|) works by
> > > displaying the output of the command in a new buffer, but in emacs
> > > using a prefix argument (C-u) allows to operate on the current buffer.
> > >
> > > diff belows adds that for mg. I can finally C-u M-! got diff RET when
> > > composing mails :)
> > >
> > > A possible drawback is that now the *Shell Command Output* buffer
> > > gains an undo history. linsert is also possibly slower than addline
> > > but on the plus side we're no more limited to BUFSIZ long lines.
> > >
> > > ok/comments/improvements?
> >
> > Here's a slightly tweaked version that adds a missing parens around a
> > return value and uses ssize_t for some vars in preadin. it also changes
> > the size read(2) from BUFSIZ-1 to BUFSIZ since we no longer need to NUL
> > terminate it.
> >
> > This has been more useful than I originally expected. I wanted it to
> > include diffs and the like more easily, now i'm using it also for all
> > sorts of stuff that mg doesn't do out-of-the-box (like using C-u M-|
> > sort RET instead of M-x sort-lines.)
> >
> > If it were for me, M-| and M-! would operate by default on the buffer
> > and with C-u on a scratch one, but this is what emacs does and i'm
> > probably several decades too late :)
diff 214e94d3085276f4e5c6b416bfd54b5d50a7bf91
cde294b6d4634ab98c7926103d373202007e23c2
commit - 214e94d3085276f4e5c6b416bfd54b5d50a7bf91
commit + cde294b6d4634ab98c7926103d373202007e23c2
blob - 21c5174f52d21103b9cd15942620eb746b5069b2
blob + 1bee60b37ceea3f9ec3ceb779727f877a7f15851
--- usr.bin/mg/region.c
+++ usr.bin/mg/region.c
@@ -26,14 +26,13 @@ static char leftover[BUFSIZ];
#define TIMEOUT 1
-static char leftover[BUFSIZ];
-
static int getregion(struct region *);
static int iomux(int, char * const, int, struct buffer *);
static int preadin(int, struct buffer *);
static voidpwriteout(int, char **, int *);
static int setsize(struct region *, RSIZE);
-static int shellcmdoutput(char * const[], char * const, int);
+static int shellcmdoutput(char * const[], char * const, int,
+ struct buffer *);
/*
* Kill the region. Ask "getregion" to figure out the bounds of the region.
@@ -419,14 +418,11 @@ piperegion(int f, int n)
piperegion(int f, int n)
{
struct region region;
+ struct buffer *bp = NULL;
int len;
char *cmd, cmdbuf[NFILEN], *text;
char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
- /* C-u M-| is not supported yet */
- if (n > 1)
- return (ABORT);
-
if (curwp->w_markp == NULL) {
dobeep();
ewprintf("The mark is not set now, so there is no region");
@@ -452,7 +448,13 @@ piperegion(int f, int n)
region_get_data(, text, len);
- return shellcmdoutput(argv, text, len);
+ if (n > 1) {
+ bp = curbp;
+ killregion(FFRAND, 1);
+ kdelete();
+ }
+
+ return (shellcmdoutput(argv, text, len, bp));
}
/*
@@ -462,12 +464,12 @@ shellcommand(int f, int n)
int
shellcommand(int f, int n)
{
-
+ struct buffer *bp = NULL;
char *cmd, cmdbuf[NFILEN];
char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
if (n > 1)
- return (ABORT);
+ bp = curbp;
if ((cmd = eread("Shell command: ", cmdbuf, sizeof(cmdbuf),
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
@@ -475,36 +477,43 @@ shellcommand(int f, int n)
argv[2] = cmd;
- return shellcmdoutput(argv, NULL, 0);
+ return (shellcmdoutput(argv, NULL, 0, bp));
}
-
int
-shellcmdoutput(char* const argv[], char* const text, int len)
+shellcmdoutput(char* const argv[], char* const text, int len,
+struct buffer *bp)
{
-
- struct buffer *bp;
+ struct mgwin *wp;
char*shellp;
- int ret;
+ int ret, special = 0;
- bp = bfind("*Shell Command Output*", TRUE);
- bp->b_flag |= BFREADONLY;
- if (bclear(bp) != TRUE) {
- free(text);
- return (FALSE);
+ if (bp == NULL) {
+ special = 1;
+ bp = bfind("*Shell Command Output*", TRUE);
+ bp->b_flag &= ~BFREADONLY; /* disable read-only */
+ wp = popbuf(bp, WNONE);
+ if (wp == NULL || bclear(bp) != TRUE) {
+ free(text);
+
Re: mg: handle prefix argument in shell-command{,-on-region}
bump
On 2022/10/25 14:30:51 +0200, Omar Polo wrote:
> On 2022/10/13 12:25:00 +0200, Omar Polo wrote:
> > shell-command (M-!) and shell-command-on-region (M-|) works by
> > displaying the output of the command in a new buffer, but in emacs
> > using a prefix argument (C-u) allows to operate on the current buffer.
> >
> > diff belows adds that for mg. I can finally C-u M-! got diff RET when
> > composing mails :)
> >
> > A possible drawback is that now the *Shell Command Output* buffer
> > gains an undo history. linsert is also possibly slower than addline
> > but on the plus side we're no more limited to BUFSIZ long lines.
> >
> > ok/comments/improvements?
>
> Here's a slightly tweaked version that adds a missing parens around a
> return value and uses ssize_t for some vars in preadin. it also changes
> the size read(2) from BUFSIZ-1 to BUFSIZ since we no longer need to NUL
> terminate it.
>
> This has been more useful than I originally expected. I wanted it to
> include diffs and the like more easily, now i'm using it also for all
> sorts of stuff that mg doesn't do out-of-the-box (like using C-u M-|
> sort RET instead of M-x sort-lines.)
>
> If it were for me, M-| and M-! would operate by default on the buffer
> and with C-u on a scratch one, but this is what emacs does and i'm
> probably several decades too late :)
diff 214e94d3085276f4e5c6b416bfd54b5d50a7bf91
cde294b6d4634ab98c7926103d373202007e23c2
commit - 214e94d3085276f4e5c6b416bfd54b5d50a7bf91
commit + cde294b6d4634ab98c7926103d373202007e23c2
blob - 21c5174f52d21103b9cd15942620eb746b5069b2
blob + 1bee60b37ceea3f9ec3ceb779727f877a7f15851
--- usr.bin/mg/region.c
+++ usr.bin/mg/region.c
@@ -26,14 +26,13 @@ static char leftover[BUFSIZ];
#define TIMEOUT 1
-static char leftover[BUFSIZ];
-
static int getregion(struct region *);
static int iomux(int, char * const, int, struct buffer *);
static int preadin(int, struct buffer *);
static voidpwriteout(int, char **, int *);
static int setsize(struct region *, RSIZE);
-static int shellcmdoutput(char * const[], char * const, int);
+static int shellcmdoutput(char * const[], char * const, int,
+ struct buffer *);
/*
* Kill the region. Ask "getregion" to figure out the bounds of the region.
@@ -419,14 +418,11 @@ piperegion(int f, int n)
piperegion(int f, int n)
{
struct region region;
+ struct buffer *bp = NULL;
int len;
char *cmd, cmdbuf[NFILEN], *text;
char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
- /* C-u M-| is not supported yet */
- if (n > 1)
- return (ABORT);
-
if (curwp->w_markp == NULL) {
dobeep();
ewprintf("The mark is not set now, so there is no region");
@@ -452,7 +448,13 @@ piperegion(int f, int n)
region_get_data(, text, len);
- return shellcmdoutput(argv, text, len);
+ if (n > 1) {
+ bp = curbp;
+ killregion(FFRAND, 1);
+ kdelete();
+ }
+
+ return (shellcmdoutput(argv, text, len, bp));
}
/*
@@ -462,12 +464,12 @@ shellcommand(int f, int n)
int
shellcommand(int f, int n)
{
-
+ struct buffer *bp = NULL;
char *cmd, cmdbuf[NFILEN];
char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
if (n > 1)
- return (ABORT);
+ bp = curbp;
if ((cmd = eread("Shell command: ", cmdbuf, sizeof(cmdbuf),
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
@@ -475,36 +477,43 @@ shellcommand(int f, int n)
argv[2] = cmd;
- return shellcmdoutput(argv, NULL, 0);
+ return (shellcmdoutput(argv, NULL, 0, bp));
}
-
int
-shellcmdoutput(char* const argv[], char* const text, int len)
+shellcmdoutput(char* const argv[], char* const text, int len,
+struct buffer *bp)
{
-
- struct buffer *bp;
+ struct mgwin *wp;
char*shellp;
- int ret;
+ int ret, special = 0;
- bp = bfind("*Shell Command Output*", TRUE);
- bp->b_flag |= BFREADONLY;
- if (bclear(bp) != TRUE) {
- free(text);
- return (FALSE);
+ if (bp == NULL) {
+ special = 1;
+ bp = bfind("*Shell Command Output*", TRUE);
+ bp->b_flag &= ~BFREADONLY; /* disable read-only */
+ wp = popbuf(bp, WNONE);
+ if (wp == NULL || bclear(bp) != TRUE) {
+ free(text);
+ return (FALSE);
+ }
+ curbp = bp;
+ curwp = wp;
}
shellp = getenv("SHELL");
ret = pipeio(shellp, argv, text, len, bp)
Re: mg: handle prefix argument in shell-command{,-on-region}
On 2022/10/13 12:25:00 +0200, Omar Polo wrote:
> shell-command (M-!) and shell-command-on-region (M-|) works by
> displaying the output of the command in a new buffer, but in emacs
> using a prefix argument (C-u) allows to operate on the current buffer.
>
> diff belows adds that for mg. I can finally C-u M-! got diff RET when
> composing mails :)
>
> A possible drawback is that now the *Shell Command Output* buffer
> gains an undo history. linsert is also possibly slower than addline
> but on the plus side we're no more limited to BUFSIZ long lines.
>
> ok/comments/improvements?
Here's a slightly tweaked version that adds a missing parens around a
return value and uses ssize_t for some vars in preadin. it also changes
the size read(2) from BUFSIZ-1 to BUFSIZ since we no longer need to NUL
terminate it.
This has been more useful than I originally expected. I wanted it to
include diffs and the like more easily, now i'm using it also for all
sorts of stuff that mg doesn't do out-of-the-box (like using C-u M-|
sort RET instead of M-x sort-lines.)
If it were for me, M-| and M-! would operate by default on the buffer
and with C-u on a scratch one, but this is what emacs does and i'm
probably several decades too late :)
diff 214e94d3085276f4e5c6b416bfd54b5d50a7bf91
e0a2dbb9e0635412548dd6554c3233157b4ad849
commit - 214e94d3085276f4e5c6b416bfd54b5d50a7bf91
commit + e0a2dbb9e0635412548dd6554c3233157b4ad849
blob - 21c5174f52d21103b9cd15942620eb746b5069b2
blob + 67cfa8d581ecc11b4e8b7cdb14db5dd70470ad60
--- usr.bin/mg/region.c
+++ usr.bin/mg/region.c
@@ -26,14 +26,13 @@ static char leftover[BUFSIZ];
#define TIMEOUT 1
-static char leftover[BUFSIZ];
-
static int getregion(struct region *);
static int iomux(int, char * const, int, struct buffer *);
static int preadin(int, struct buffer *);
static voidpwriteout(int, char **, int *);
static int setsize(struct region *, RSIZE);
-static int shellcmdoutput(char * const[], char * const, int);
+static int shellcmdoutput(char * const[], char * const, int,
+ struct buffer *);
/*
* Kill the region. Ask "getregion" to figure out the bounds of the region.
@@ -419,14 +418,11 @@ piperegion(int f, int n)
piperegion(int f, int n)
{
struct region region;
+ struct buffer *bp = NULL;
int len;
char *cmd, cmdbuf[NFILEN], *text;
char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
- /* C-u M-| is not supported yet */
- if (n > 1)
- return (ABORT);
-
if (curwp->w_markp == NULL) {
dobeep();
ewprintf("The mark is not set now, so there is no region");
@@ -452,7 +448,13 @@ piperegion(int f, int n)
region_get_data(, text, len);
- return shellcmdoutput(argv, text, len);
+ if (n > 1) {
+ bp = curbp;
+ killregion(FFRAND, 1);
+ kdelete();
+ }
+
+ return (shellcmdoutput(argv, text, len, bp));
}
/*
@@ -462,12 +464,12 @@ shellcommand(int f, int n)
int
shellcommand(int f, int n)
{
-
+ struct buffer *bp = NULL;
char *cmd, cmdbuf[NFILEN];
char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
if (n > 1)
- return (ABORT);
+ bp = curbp;
if ((cmd = eread("Shell command: ", cmdbuf, sizeof(cmdbuf),
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
@@ -475,36 +477,43 @@ shellcommand(int f, int n)
argv[2] = cmd;
- return shellcmdoutput(argv, NULL, 0);
+ return (shellcmdoutput(argv, NULL, 0, bp));
}
-
int
-shellcmdoutput(char* const argv[], char* const text, int len)
+shellcmdoutput(char* const argv[], char* const text, int len,
+struct buffer *bp)
{
-
- struct buffer *bp;
+ struct mgwin *wp;
char*shellp;
- int ret;
+ int ret, special = 0;
- bp = bfind("*Shell Command Output*", TRUE);
- bp->b_flag |= BFREADONLY;
- if (bclear(bp) != TRUE) {
- free(text);
- return (FALSE);
+ if (bp == NULL) {
+ special = 1;
+ bp = bfind("*Shell Command Output*", TRUE);
+ bp->b_flag &= ~BFREADONLY; /* disable read-only */
+ wp = popbuf(bp, WNONE);
+ if (wp == NULL || bclear(bp) != TRUE) {
+ free(text);
+ return (FALSE);
+ }
+ curbp = bp;
+ curwp = wp;
}
shellp = getenv("SHELL");
ret = pipeio(shellp, argv, text, len, bp);
-
if (ret == TRUE) {
eerase();
- if (lforw(bp->b_headp) == bp->b_headp)
+ if (special && lforw(bp->b_headp) == bp->b_headp
mg: ewprintf("") -> eerase()
as per subject; the effect is almost the same, they both clear the
echo area, but ewprintf("") sets `epresf' to require an additional
eerase() on the next-next input cycle. avoid extra work! :)
ok?
diff /home/op/w/mg.master
commit - 4912f94d4a7e2b653e74201ac73923335fd298aa
path + /home/op/w/mg.master
blob - 810ea3ae3346112842126acfe47e1a934bd9acc0
file + dired.c
--- dired.c
+++ dired.c
@@ -1185,7 +1185,7 @@ gotofile(char *fpth)
ewprintf("File not found %s", fname);
return (FALSE);
} else {
- ewprintf("");
+ eerase();
return (TRUE);
}
}
blob - ced1a2dc4a9541a34a99f9ae96acf9f4e1237f33
file + echo.c
--- echo.c
+++ echo.c
@@ -67,15 +67,15 @@ eyorn(const char *sp)
for (;;) {
s = getkey(FALSE);
if (s == 'y' || s == 'Y' || s == ' ') {
- ewprintf("");
+ eerase();
return (TRUE);
}
if (s == 'n' || s == 'N' || s == CCHR('M')) {
- ewprintf("");
+ eerase();
return (FALSE);
}
if (s == CCHR('G')) {
- ewprintf("");
+ eerase();
return (ctrlg(FFRAND, 1));
}
ewprintf("Please answer y or n. %s? (y or n) ", sp);
@@ -101,19 +101,19 @@ eynorr(const char *sp)
for (;;) {
s = getkey(FALSE);
if (s == 'y' || s == 'Y' || s == ' ') {
- ewprintf("");
+ eerase();
return (TRUE);
}
if (s == 'n' || s == 'N' || s == CCHR('M')) {
- ewprintf("");
+ eerase();
return (FALSE);
}
if (s == 'r' || s == 'R') {
- ewprintf("");
+ eerase();
return (REVERT);
}
if (s == CCHR('G')) {
- ewprintf("");
+ eerase();
return (ctrlg(FFRAND, 1));
}
ewprintf("Please answer y, n or r.");
@@ -137,7 +137,7 @@ eyesno(const char *sp)
EFNUL | EFNEW | EFCR, sp);
for (;;) {
if (rep == NULL) {
- ewprintf("");
+ eerase();
return (ABORT);
}
if (rep[0] != '\0') {
@@ -149,11 +149,11 @@ eyesno(const char *sp)
free(lp);
}
if (strcasecmp(rep, "yes") == 0) {
- ewprintf("");
+ eerase();
return (TRUE);
}
if (strcasecmp(rep, "no") == 0) {
- ewprintf("");
+ eerase();
return (FALSE);
}
}
Re: cwm: do not overlap menu entries
On 2022/10/14 14:11:19 -0400, Okan Demirmen wrote: > the below seems to work: Walter (thanks!) found the spot i missed on > replacing height with ascent+descent. > > please let me know. diff reads fine works for me too, thanks!
Re: cwm: do not overlap menu entries
On 2022/10/13 15:16:47 +, Klemens Nanni wrote: > On Thu, Oct 13, 2022 at 04:39:04PM +0200, Omar Polo wrote: > > On 2022/10/13 13:00:34 +, Klemens Nanni wrote: > > > On Thu, Oct 13, 2022 at 08:28:50AM -0400, Okan Demirmen wrote: > > > > And I keep missing it! I can't reproduce this - can you share the font > > > > you're using maybe? > > > > > > Whatever is the default, I never fiddled with fonts in X, no xorg.conf, > > > `cwm -c/dev/null' shows the glitch for me on a ThinkPad X230. > > > > i can't reproduce it either, not with my normal cwmrc and nor with an > > empty one. However with your patch the selected menu entry seems to > > be... correctly sized? Without your patch the selected item seems to > > be slightly more tall and "touch" the numbers and the parens of the > > items below. > > That as well, at leat to my eye. > > Here are four screen shots from my X230 running `cwm -c /dev/null' > inside Xephyr, taken with `scrot -s -q 100' so I can select the area > without clicking into the window which would make cwm's menu disappear. > > "current-" and "patch-" mean cwm from current and with the +1 patch, > respectively. > > "top-bottom" and "bottom-top" mean that the cursor has been moved across > the menu top to bottom and bottom to top, respectively. > > current-bottom-top.png shows cropped "[]" chars in the second entry, > whereas current-top-bottom.png and patch-*.png do not. Ahhh, now i see that too. it's subtle but your screenshot clearly shows that the selected items hides the top row of fixes from the item belows it. I've never noticed it before 'cause the selected items here has a black background (due to my ~/.Xdefaults.), but it happens for me too. Actually, i get what it looks like a "pixel perfect" result by subtracting two instead of one from descent in menu.c, but i was just playing with it -- need to re-read that part more closely.
Re: cwm: do not overlap menu entries
On 2022/10/13 13:00:34 +, Klemens Nanni wrote: > On Thu, Oct 13, 2022 at 08:28:50AM -0400, Okan Demirmen wrote: > > And I keep missing it! I can't reproduce this - can you share the font > > you're using maybe? > > Whatever is the default, I never fiddled with fonts in X, no xorg.conf, > `cwm -c/dev/null' shows the glitch for me on a ThinkPad X230. i can't reproduce it either, not with my normal cwmrc and nor with an empty one. However with your patch the selected menu entry seems to be... correctly sized? Without your patch the selected item seems to be slightly more tall and "touch" the numbers and the parens of the items below.
