Re: Please test: HZ bump
> And then... can we reduce wakeup latency in general without raising HZ? Other > systems (e.g. DFly) have better wakeup latencies and still have HZ=100. What > are they doing? Can we borrow it? https://frenchfries.net/paul/dfly/nanosleep.html OpenBSD is still adding that one tick which results in a (typical) sleep duration of no less than about 20ms.
smtpd session hang
I had a little debugging session with awolk@ over at #openbsd-daily. His
smtpd would over time end up with hung sessions that never timeout.
The problem is related to the data_io path's congestion control which
may pause the session. In this case the io system will not wait for
read events and as such will not have a chance to timeout until it is
resumed.
If the pause happens when a full message is just about to pass through
the data_io path, the session is never resumed, even though there is
obviously no more congestion and the session should be reading more
input from the client again.
A debug trace excerpt shows the course of events:
mtp: 0xe54baa1e000: IO_DATAIN
debug: smtp: 0xe54baa1e000: filter congestion: pausing session
smtp: 0xe54baa1e000 (data): IO_LOWAT
debug: smtp: 0xe54baa1e000: data io done (259146 bytes)
debug: 0xe54baa1e000: end of message, msgflags=0x
smtp: 0xe54baa1e000: >>> 250 2.0.0: 4f36f9e3 Message accepted for delivery
smtp: 0xe54baa1e000: STATE_BODY -> STATE_HELO
smtp: 0xe54baa1e000: IO_LOWAT
>From this point on, session 0xe54baa1e000 and its io 0xe551d0d5000
(which has the pause_in flag) are never seen again in the trace, and
fstat shows a corresponding connection to smtpd that never goes away.
The proposed fix is to always resume the session if the data_io path
hits the low water mark.
Mr. Wolk tested this diff against smtpd on 6.1 as well as a against
-current version of smtpd (compiled on the same system running 6.1).
Index: usr.sbin/smtpd/smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.303
diff -u -p -r1.303 smtp_session.c
--- usr.sbin/smtpd/smtp_session.c 17 May 2017 14:00:06 - 1.303
+++ usr.sbin/smtpd/smtp_session.c 15 Jun 2017 20:28:12 -
@@ -1474,9 +1474,10 @@ smtp_data_io(struct io *io, int evt, voi
break;
case IO_LOWAT:
- if (s->tx->dataeom && io_queued(s->tx->oev) == 0) {
+ if (s->tx->dataeom && io_queued(s->tx->oev) == 0)
smtp_data_io_done(s);
- } else if (io_paused(s->io, IO_IN)) {
+
+ if (io_paused(s->io, IO_IN)) {
log_debug("debug: smtp: %p: filter congestion over:
resuming session", s);
io_resume(s->io, IO_IN);
}
Re: smtpd session hang
> > Nice catch, the diff reads fine to me, I'll commit later today when I
> > have another ok from eric@
> Yes, this looks correct. But, I would rather move the resume test before
> the EOM test, to avoid touching the session after the transfer has been
> finalized by smtp_data_io_done().
It oughtn't make a difference as long as the duplex control is correct,
but I'm fine with that change.
> > side note, smtpd should not have been able to leak more than 5 fd, it
> > should not have been able to exhaust, is this what you observed ?
For the record, we discussed this with Gilles on irc and while we saw
more than a dozen leaked fds, it's okay as smtpd will allow as many
incoming sessions as the dtable can accommodate (with an fd reserve).
The lower limits are on outgoing connections.
New diff with reordered code. I'll see if I can get Adam to run one more
round of testing..
Index: usr.sbin/smtpd/smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.303
diff -u -p -r1.303 smtp_session.c
--- usr.sbin/smtpd/smtp_session.c 17 May 2017 14:00:06 - 1.303
+++ usr.sbin/smtpd/smtp_session.c 16 Jun 2017 14:56:11 -
@@ -1474,12 +1474,12 @@ smtp_data_io(struct io *io, int evt, voi
break;
case IO_LOWAT:
- if (s->tx->dataeom && io_queued(s->tx->oev) == 0) {
- smtp_data_io_done(s);
- } else if (io_paused(s->io, IO_IN)) {
+ if (io_paused(s->io, IO_IN)) {
log_debug("debug: smtp: %p: filter congestion over:
resuming session", s);
io_resume(s->io, IO_IN);
}
+ if (s->tx->dataeom && io_queued(s->tx->oev) == 0)
+ smtp_data_io_done(s);
break;
default:
Re: use mallocarray in kern
If you look at the diff that went in already, it's using mallocarray. It wouldn't even compile otherwise.
Re: dlopen after dlclose crash
> I might dig deeper once I find the time, but perhaps someone already > > familiar with the code might want to take a look at it before I waste a > > week on it ;-) > > > > The issue is the change in ld.so/library_subr.c rev 1.34. If you back that > change out, the crash disappears. > > The problem is that no one makes changes to the linkages inside ld.so out > of boredom: there was some previous program that crashed without that > change, but the details weren't documented or preserved in a regress/ > program. I've made a couple stabs at reproducing the original program so > that we can be sure to keep it fixed when fixing this, but haven't been > able to pin down a case where the committed change solved the problem. If > you can figure that out, I would gladly buy you a beer or three. Elsewise > we're reaching the point where we back that change out and wait for someone > complain... :-( I managed to come up with a case where a double decrement takes place, when running with the change from 1.34 undone. There are two libraries, l1 and l2. L1 depends on l2; l2 has no deps. Then you do this dance: 1. dlopen l1 2. dlopen l2 3. dlclose l2 4. dlopen l2 5. dlclose l2 6. dlclose l1 So first (1) we load l1, and this also loads l2 as a dep. Now (2) we explicitly open l2, but since it's already loaded, l2 makes a group reference to l1. We (3) close the handle we just opened, and this decrements the grouprefcount on l1, but l1 is still on l2's list of grouprefs (they don't get removed prior to 1.34). L2 also won't be unloaded since it's still needed as a dep by l1. (4) Open l2 again explicitly. Again l2 makes a groupref to l1, so now l1 appears on l2's group list twice. So the next close (5) decrements l1's grouprefcount twice, making it negative... it's around here where l1 gets unloaded. When we try to close it in (6), it's not there. Fwiw, I also saw some segfaults which looked very much like the use after free I had in my SDL2 case. But they seem much more frequent when the change from 1.34 is there. I don't have the time investigate that tonight... I've put together a little test case, see http://guu.fi/ldtest.tgz for the code. -Henri
Re: dlopen after dlclose crash
> > The issue is the change in ld.so/library_subr.c rev 1.34. If you back that > > change out, the crash disappears. > > > > The problem is that no one makes changes to the linkages inside ld.so out > > of boredom: there was some previous program that crashed without that > > change, but the details weren't documented or preserved in a regress/ > > program. I've made a couple stabs at reproducing the original program so > > that we can be sure to keep it fixed when fixing this, but haven't been > > able to pin down a case where the committed change solved the problem. If > > you can figure that out, I would gladly buy you a beer or three. Elsewise > > we're reaching the point where we back that change out and wait for someone > > complain... :-( > > I managed to come up with a case where a double decrement takes place [..] Hello again. I just adjusted the case, trying to make it appropriate for inclusion under src/regress. I also added a (currently failing) regression test for my original problem. The tarball contains two directories, test3 and test4, which ought to go under /usr/src/regress/libexec/ld.so/dlclose. Does it look ok? Don't forget to add the subdirs in the Makefile. http://guu.fi/ldtest-new.tgz Fwiw, I've also come up with a potential fix, but I'll have to test it a bit more. -Henri
Re: chmod.c undefined behavior
> I noticed that chmod.c have uninitialized variable char *ep that was > used. This diff clarify what I mean. It might be a good idea to take a careful look at the man page of strtoul(3). Pay attention to what it does with errno and endptr. Also, take a look at the example.
Re: chmod.c undefined behavior
> Unnecessary goto Or the most straightforward and obvious way to break out of a switch in a loop. > variables defined far away from where they are used, Variables defined predictably at the start of the function, as the convention is in BSD code. Yes, they can be a little far if the function is long. You also always know exactly where to find them. > monster function Two hundred lines is hardly long, and size matters less than readability. If a long function reads neatly from top to bottom, you can split it into a few dozen procedures, requiring the reader to jump back and forth to figure out what the code *actually* does. You then also need to shovel data back and forth between these functions. This is how you turn simple and straightforward into a clusterfuck. > variables are not commented what they do, > variables named well enough If you're familiar with the fts api and know what command line flags are, then half of these variables are immediately obvious. For the rest, the name is a good hint and if you spent a few seconds looking at where and how each variable is used, these are perfectly understandable too. Short names don't get in the way too much. How would you improve these names? It sounds like you're just trying to take snippets of code out of context and declare them bad because you're not reading the rest of the code? Of course you could comment these, but the code is simple enough that such excess verbiage would likely just get in the way. Much in the same manner that splitting the function into a few dozen would only make it harder to see the code for what it is. Mind you, the code isn't written for a first-year comp sci course where the students need annotated twenty-line snippets to help them come to grips with the basic syntax and structure of a computer program. > not all functions are commented what they do.. Again trying to please some quality metric? Usage does not need to be documented. And main() is essentially the whole program, which is extensively documented in a man page. It's not called by another function; there is no internal API to document. What comment would you add there? What value would that comment add? > To me, it looks like that there is no intention to optimize readability and > testability. Instead it looks like there is put effort to minimize amount of > functions or something. It looks like you're not actually reading the code, you're just looking at snippets of it, taken out of context. And then you're applying some cargo- cult pseudo-science to make statements about these snippets. Goto is evil! > Something to aim for: > -Less linearly independent paths in module > -High cohesion > -Low coupling Did you intend to achieve better cohesion and lower coupling by breaking the function into a dozen small functions that appease whatever metric you're using? Of course, I'm a nobody to say how this code should look, but it looks fine to me; and so far as I can tell, it's not broken. Don't fix it if it's not broken. But if you think it can be improved, I'm sure someone here might take a look at the diff once you send it. But if your comments about the current code are any indication of what your ideal version would look like, I'm not sure there are many who would be willing to commit it. But don't let me discourage you; I'm a nobody here. -Henri
Re: chmod.c undefined behavior
> That is good convention. But while the function is so big, it is harder > to read. > > >You then also need to shovel > >data back and forth between these functions. > > Not necessarily. When the program is this tiny, single file source, > it may not so bad thing to use global scope for that shared data. Don't you see the irony in claiming that main is too big and that the variable definitions are too far away, while at the same time saying the program is tiny and proposing to move some variables into file scope? That should say something about paying too much attention to per-function metrics... > I don't say that this code is bad. I think it is good gode, but there is > plenty of room to improve readability. I could bikeshed about it too, plenty. It's harder to make up changes with real substenance into a program that is tiny and already "good code". > > Much in the same manner that splitting the function into > > a few dozen would only make it harder to see the code for what it is. > > And when the lower level details are hidden, it is easier to see what > the code does. This is idea of abstraction. And you can go too far. You could break things into smaller and smaller pieces until you have a set of functions that resemble the instruction set of some virtual CPU. Yes, absraction is a core element of programming, and a critical one at that. However, all abstraction comes at a cost, and sometimes that cost may be greater than the value. A program is the sum of its parts, and by splitting parts of it into smaller pieces the program doesn't always grow simpler or more understandable. On the contrary; all but the best and most generic of abstractions tend to be leaky and flawed, and the programmer needs to keep many details about them in mind in order to underdstand what the code really does. Also, hiding low-level details can be the exact opposite of what you want in order to make it easy to see what the code does. I've seen this pattern with aspiring hackers who learn from contrived textbook examples with excessively annotated & commented code snippets. They learn to give every little thing a name, and call it an abstraction. What they don't learn is idioms known to experienced programmers. So they hand that code to an experienced programmer and after removing layers upon layers of unneeded abstraction (or obfuscation), including all the extra machinery needed to communicate program state between those layers, the experienced programmer finally sees the code for all its bugs. Alternatively, he can try to keep all the made-up abstractions in his head, but that can be quite a burden. He'll end up constantly jumping back and forth between them and re-checking details. Cohesion? You wanted local variables to be nearby so that they are easy to find. That's one detail. Functions hide other details which you might also want to keep nearby. Typedefs and macros make yet another way to hide details that really matter. Try looking for subtle arithmetic overflows when you don't see the operators and don't know the *real* types of the operands. If you have the time, take a look at what kind of layers were removed in LibreSSL. By reading lots and lots of code, you'll start to pick up patterns and idioms. Code becomes easier to read, and you'll spend less time trying to understand specific parts. And because you do not need to explain these parts to yourself, you also won't need to single them out and give them a name to remind yourself of what they do. The result is that you'll find larger functions perfectly readable and understandable. And perhaps you'll start to see why extra layers would actually make it harder to read. That's not to say that small functions are bad, or that larger functions are always better, or that chmod shouldn't ever be split into smaller parts. But the ability to say which abstractions are good and which are useless (or outright harmful) is something that grows with experience. And not all functions are alike; a long function can really read like a list of instructions where you can forget the past steps as soon as they are behind. A small thirty-line function with some twisted logic (add recursion?) could be a real brain teaser. > > Mind you, the code isn't written for a first-year comp sci course > > where the students need annotated twenty-line snippets to help them > > come to grips with the basic syntax and structure of a computer > > program. > > I like to write self documenting code. My practice is to comment ~all > functions with block comments, all parameters (with ranges), return > values, exceptions, external effects, all variable declarations (with > ranges). That sounds like dogma. It's not a terrible attitude to start with, but as you pick up idioms and conventions, you'll find that some things are so obvious that comments add absolutely nothing. And useless comments do not make the code prettier or easier to read. On the other hand, you'll
Re: Stairstep mouse motion
> > Tested on my x230t and will continue to test. No regrssions noticed on > > relative pointing devices. > > > > OK? > > Anyone? > > I appreciate that I am probably the only one using OpenBSD on a tablet, > but a "looks OK" and "no regressions for relative pointing devices" > would be great. What happens when priv->swap_axes is set, and the ax && ay branch is taken along with the wsWheelEmuFilterMotion() branch. Following continue another event is processed and now the axes are swapped again (ax and ay were not reset after use) and then what? Not very likely I know. In hindsight it's possible that wheel emulation has been broken in the previous revision; a glance at wsWheelEmuFilterMotion suggests it doesn't like to handle both x and y axes at once, and will only do the former (resetting the latter) if both are set. I've never used this thing though, and I didn't dive deeper. Other than that I guess your diff is about as reasonable as one can be with this hairy code. I was waiting and hoping someone else with an absolute pointing device could've checked and tested it because I really didn't enjoy working on this file when I did my patch :-)
Re: Stairstep mouse motion
> From: Alexandr Shadchin > > Before (on example pms(4)): > * user move mouse > * pms(4) read state mouse and process it > * pms(4) send dx, dy and buttons in wscons > * wscons generate simple events > * ws(4) reads one event and process it immediately > > After applying diff: > * user move mouse > * pms(4) read state mouse and process it > * pms(4) send dx, dy and buttons in wscons > * wscons generate simple events and adds SYNC event > * ws(4) reads events until it receives SYNC, and only then begins processing > > Tested on mouse. > > Comments ? > > PS: > synaptics(4) is working on a similar basis Absolutely yes for this. This is one of the approaches I originally considered, but then feared it'd be too intrusive. I didn't realize WS_INPUT_SYNC was already a thing and that we're doing this with synaptics. This'll also fix another downside which I mentioned with the previous approach (that it could join two unrelated x & y events if they follow each other). I'm busy crossing the time_t chasm and compiling ports because the packages on mirrors haven't gotten across the libstdc++ bump (damnit). I'll try take a better look at your diff and test it tomorrow.
Re: Stairstep mouse motion
> Alternative solution without extra magic (need rebuild kernel). > > Before (on example pms(4)): > * user move mouse > * pms(4) read state mouse and process it > * pms(4) send dx, dy and buttons in wscons > * wscons generate simple events > * ws(4) reads one event and process it immediately > > After applying diff: > * user move mouse > * pms(4) read state mouse and process it > * pms(4) send dx, dy and buttons in wscons > * wscons generate simple events and adds SYNC event > * ws(4) reads events until it receives SYNC, and only then begins processing > > Tested on mouse. > > Comments ? Testing with usb mouse, so far so good. Mouse wheel emulation seems to be working fine as well. The code reads ok to me, and it's definitely a lot cleaner than the old magic.
Re: mg word wrapping tweak
Matthew Dempsky writes: > Diff below tweaks the logic from "allow double space after /[.?!]/" to > "allow double space after /[.?!]\)?/". I gave it a spin (since Kjell says it's hard to find people to test mg diffs). No problems, the code looks fine, and this behavior certainly makes sense.
Set In-Reply-To when responding in mail(1)
I felt hacking custom scripts to add the In-Reply-To header in a reply
is a bad solution if there's no reason we can't just make mail do it
automagically. So, here comes the diff.
There are a few additional changes here:
- Names starting with underscores are reserved, so I gave
_respond and _Respond names which also document their
behavior a little better. Now they're replyall and
replyorig.
- Ansify replyall
- Fix up whitespace in a couple relevant places.
If someone insists, I might look into adding a knob, though I don't
really see why anyone should want to turn this behavior off.
Index: src/usr.bin/mail//cmd3.c
===
RCS file: /cvs/src/usr.bin/mail/cmd3.c,v
retrieving revision 1.25
diff -u -p -r1.25 cmd3.c
--- src/usr.bin/mail//cmd3.c6 Apr 2011 11:36:26 - 1.25
+++ src/usr.bin/mail//cmd3.c26 Jul 2011 13:15:26 -
@@ -176,9 +176,9 @@ respond(void *v)
int *msgvec = v;
if (value("Replyall") == NULL)
- return(_respond(msgvec));
+ return(replyall(msgvec));
else
- return(_Respond(msgvec));
+ return(replyorig(msgvec));
}
/*
@@ -186,8 +186,7 @@ respond(void *v)
* message header and send them off to mail1()
*/
int
-_respond(msgvec)
- int *msgvec;
+replyall(int *msgvec)
{
struct message *mp;
char *cp, *rcv, *replyto;
@@ -239,6 +238,7 @@ _respond(msgvec)
head.h_cc = np;
} else
head.h_cc = NULL;
+ head.h_inreplyto = hfield("message-id", mp);
head.h_bcc = NULL;
head.h_smopts = NULL;
mail1(&head, 1);
@@ -586,9 +586,9 @@ Respond(void *v)
int *msgvec = v;
if (value("Replyall") == NULL)
- return(_Respond(msgvec));
+ return(replyorig(msgvec));
else
- return(_respond(msgvec));
+ return(replyall(msgvec));
}
/*
@@ -597,7 +597,7 @@ Respond(void *v)
* reply.
*/
int
-_Respond(int *msgvec)
+replyorig(int *msgvec)
{
struct header head;
struct message *mp;
@@ -619,6 +619,7 @@ _Respond(int *msgvec)
if ((head.h_subject = hfield("subject", mp)) == NULL)
head.h_subject = hfield("subj", mp);
head.h_subject = reedit(head.h_subject);
+ head.h_inreplyto = hfield("message-id", mp);
head.h_cc = NULL;
head.h_bcc = NULL;
head.h_smopts = NULL;
Index: src/usr.bin/mail//collect.c
===
RCS file: /cvs/src/usr.bin/mail/collect.c,v
retrieving revision 1.33
diff -u -p -r1.33 collect.c
--- src/usr.bin/mail//collect.c 6 Apr 2011 11:36:26 - 1.33
+++ src/usr.bin/mail//collect.c 26 Jul 2011 13:15:26 -
@@ -328,7 +328,8 @@ cont:
*/
rewind(collf);
puts("---\nMessage contains:");
- puthead(hp, stdout, GTO|GSUBJECT|GCC|GBCC|GNL);
+ puthead(hp, stdout,
+ GTO|GSUBJECT|GCC|GBCC|GINREPLYTO|GNL);
while ((t = getc(collf)) != EOF)
(void)putchar(t);
goto cont;
Index: src/usr.bin/mail//def.h
===
RCS file: /cvs/src/usr.bin/mail/def.h,v
retrieving revision 1.13
diff -u -p -r1.13 def.h
--- src/usr.bin/mail//def.h 25 Jun 2003 15:13:32 - 1.13
+++ src/usr.bin/mail//def.h 26 Jul 2011 13:15:26 -
@@ -155,11 +155,12 @@ struct headline {
char*l_date;/* The entire date string */
};
-#defineGTO 1 /* Grab To: line */
-#defineGSUBJECT 2 /* Likewise, Subject: line */
-#defineGCC 4 /* And the Cc: line */
-#defineGBCC8 /* And also the Bcc: line */
-#defineGMASK (GTO|GSUBJECT|GCC|GBCC)
+#define GTO1 /* Grab To: line */
+#define GSUBJECT 2 /* Likewise, Subject: line */
+#define GCC4 /* And the Cc: line */
+#define GBCC 8 /* And also the Bcc: line */
+#define GINREPLYTO 16 /* In-Reply-To: line */
+#define GMASK (GTO|GSUBJECT|GCC|GBCC|GINREPLYTO)
/* Mask of places from whence */
#defineGNL 16 /* Print blank line after */
@@ -173,6 +174,7 @@ struct headline {
struct header {
struct name *h_to; /* Dynamic "To:" string */
char *h_subject;/* Subject string */
+ char *h_inreplyto; /* In reply to */
struct name *h_cc; /* Carbon copies string */
struct name *h_bcc; /* Blind carbon copies */
struct name *h_smopts; /* Sendmail options */
Index: src/usr.bin/mail//extern.h
=
Re: [PATCH] dired mg patch
> Logan has already tested this. Any other test's/oks?
Is there a good reason to all those if/else blocks which
call d_warpdot twice if it succeeds? I don't see one. At
the very least, I'd remove the redundant call. But we can do
better: just be sane and default to zero on error. In the long
run, the error could be removed entirely by making dired behave
like the rest of mg -- that is, abort somewhere up the stream
if lalloc fails. This way the rest of the functions will never
have to worry about a NULL ltext.
So here's my version. I also removed an unnecessary variable,
added some const in the mix, corrected a minor whitespace nit,
added a comment and renamed some variables (sorry!).
Index: dired.c
===
RCS file: /cvs/src/usr.bin/mg/dired.c,v
retrieving revision 1.48
diff -u -p -r1.48 dired.c
--- dired.c 23 Jan 2011 00:45:03 - 1.48
+++ dired.c 27 Aug 2011 12:32:30 -
@@ -36,6 +36,11 @@ static intd_rename(int, int);
static int d_shell_command(int, int);
static int d_create_directory(int, int);
static int d_makename(struct line *, char *, size_t);
+static int d_warpdot(const char *);
+static int d_forwpage(int, int);
+static int d_backpage(int, int);
+static int d_forwline(int, int);
+static int d_backline(int, int);
static void reaper(int);
extern struct keymap_s helpmap, cXmap, metamap;
@@ -57,15 +62,15 @@ static PF dirednul[] = {
static PF diredcl[] = {
reposition, /* ^L */
d_findfile, /* ^M */
- forwline, /* ^N */
+ d_forwline, /* ^N */
rescan, /* ^O */
- backline, /* ^P */
+ d_backline, /* ^P */
rescan, /* ^Q */
backisearch,/* ^R */
forwisearch,/* ^S */
rescan, /* ^T */
universal_argument, /* ^U */
- forwpage, /* ^V */
+ d_forwpage, /* ^V */
rescan, /* ^W */
NULL/* ^X */
};
@@ -77,7 +82,7 @@ static PF diredcz[] = {
rescan, /* ^] */
rescan, /* ^^ */
rescan, /* ^_ */
- forwline, /* SP */
+ d_forwline, /* SP */
d_shell_command,/* ! */
rescan, /* " */
rescan, /* # */
@@ -99,9 +104,9 @@ static PF diredc[] = {
};
static PF diredn[] = {
- forwline, /* n */
+ d_forwline, /* n */
d_ffotherwindow,/* o */
- backline, /* p */
+ d_backline, /* p */
rescan, /* q */
d_rename, /* r */
rescan, /* s */
@@ -116,13 +121,32 @@ static PF direddl[] = {
d_undelbak /* del */
};
+static PF diredbp[] = {
+ d_backpage /* v */
+};
+
+static PF dirednull[] = {
+ NULL
+};
+
#ifndefDIRED_XMAPS
#defineNDIRED_XMAPS0 /* number of extra map sections */
#endif /* DIRED_XMAPS */
-static struct KEYMAPE (6 + NDIRED_XMAPS + IMAPEXT) diredmap = {
- 6 + NDIRED_XMAPS,
- 6 + NDIRED_XMAPS + IMAPEXT,
+static struct KEYMAPE (1 + IMAPEXT) d_backpagemap = {
+ 1,
+ 1 + IMAPEXT,
+ rescan,
+ {
+ {
+ 'v', 'v', diredbp, NULL
+ }
+ }
+};
+
+static struct KEYMAPE (7 + NDIRED_XMAPS + IMAPEXT) diredmap = {
+ 7 + NDIRED_XMAPS,
+ 7 + NDIRED_XMAPS + IMAPEXT,
rescan,
{
#ifndef NO_HELP
@@ -138,6 +162,10 @@ static struct KEYMAPE (6 + NDIRED_XMAPS
CCHR('L'), CCHR('X'), diredcl, (KEYMAP *) & cXmap
},
{
+ CCHR('['), CCHR('['), dirednull, (KEYMAP *) &
+ d_backpagemap
+ },
+ {
CCHR('Z'), '+', diredcz, (KEYMAP *) & metamap
},
{
@@ -592,6 +620,62 @@ d_makename(struct line *lp, char *fn, si
return ((lgetc(lp, 2) == 'd') ? TRUE : FALSE);
}
+static int
+d_warpdot(const char *l_text)
+{
+ const char *tp = l_text;
+ int field = 0;
+
+ if (tp == NULL)
+ return 0;
+
+ /*
+* Find the byte offset to the (space-delimited) filename
+* field in formatted ls output.
+*/
+ while (*tp != '\0') {
+ if (*tp == ' ') {
+ tp += strspn(tp, " ");
+ if (++field == 9)
+ break;
+ }
+ tp++;
+ }
+ return (tp - l_text);
+}
+
+static int
+d_forwpage(int f, int n)
+{
+ forwpage(f | FFRAND, n);
+
Re: [PATCH] dired mg patch
> In the long run, the error could be removed entirely by making
> dired behave like the rest of mg -- that is, abort somewhere up
> the stream if lalloc fails. This way the rest of the functions
> will never have to worry about a NULL ltext.
Actually, this seems to be the case already. dired calls addline,
which never adds a broken line to the buffer. dired_() on the
other hand has a buffer on the stack, and if that didn't work,
we'd never reach warpdot. The NULL check isn't needed. New version.
Index: dired.c
===
RCS file: /cvs/src/usr.bin/mg/dired.c,v
retrieving revision 1.48
diff -u -p -r1.48 dired.c
--- dired.c 23 Jan 2011 00:45:03 - 1.48
+++ dired.c 27 Aug 2011 13:04:49 -
@@ -36,6 +36,11 @@ static intd_rename(int, int);
static int d_shell_command(int, int);
static int d_create_directory(int, int);
static int d_makename(struct line *, char *, size_t);
+static int d_warpdot(const char *);
+static int d_forwpage(int, int);
+static int d_backpage(int, int);
+static int d_forwline(int, int);
+static int d_backline(int, int);
static void reaper(int);
extern struct keymap_s helpmap, cXmap, metamap;
@@ -57,15 +62,15 @@ static PF dirednul[] = {
static PF diredcl[] = {
reposition, /* ^L */
d_findfile, /* ^M */
- forwline, /* ^N */
+ d_forwline, /* ^N */
rescan, /* ^O */
- backline, /* ^P */
+ d_backline, /* ^P */
rescan, /* ^Q */
backisearch,/* ^R */
forwisearch,/* ^S */
rescan, /* ^T */
universal_argument, /* ^U */
- forwpage, /* ^V */
+ d_forwpage, /* ^V */
rescan, /* ^W */
NULL/* ^X */
};
@@ -77,7 +82,7 @@ static PF diredcz[] = {
rescan, /* ^] */
rescan, /* ^^ */
rescan, /* ^_ */
- forwline, /* SP */
+ d_forwline, /* SP */
d_shell_command,/* ! */
rescan, /* " */
rescan, /* # */
@@ -99,9 +104,9 @@ static PF diredc[] = {
};
static PF diredn[] = {
- forwline, /* n */
+ d_forwline, /* n */
d_ffotherwindow,/* o */
- backline, /* p */
+ d_backline, /* p */
rescan, /* q */
d_rename, /* r */
rescan, /* s */
@@ -116,13 +121,32 @@ static PF direddl[] = {
d_undelbak /* del */
};
+static PF diredbp[] = {
+ d_backpage /* v */
+};
+
+static PF dirednull[] = {
+ NULL
+};
+
#ifndefDIRED_XMAPS
#defineNDIRED_XMAPS0 /* number of extra map sections */
#endif /* DIRED_XMAPS */
-static struct KEYMAPE (6 + NDIRED_XMAPS + IMAPEXT) diredmap = {
- 6 + NDIRED_XMAPS,
- 6 + NDIRED_XMAPS + IMAPEXT,
+static struct KEYMAPE (1 + IMAPEXT) d_backpagemap = {
+ 1,
+ 1 + IMAPEXT,
+ rescan,
+ {
+ {
+ 'v', 'v', diredbp, NULL
+ }
+ }
+};
+
+static struct KEYMAPE (7 + NDIRED_XMAPS + IMAPEXT) diredmap = {
+ 7 + NDIRED_XMAPS,
+ 7 + NDIRED_XMAPS + IMAPEXT,
rescan,
{
#ifndef NO_HELP
@@ -138,6 +162,10 @@ static struct KEYMAPE (6 + NDIRED_XMAPS
CCHR('L'), CCHR('X'), diredcl, (KEYMAP *) & cXmap
},
{
+ CCHR('['), CCHR('['), dirednull, (KEYMAP *) &
+ d_backpagemap
+ },
+ {
CCHR('Z'), '+', diredcz, (KEYMAP *) & metamap
},
{
@@ -592,6 +620,59 @@ d_makename(struct line *lp, char *fn, si
return ((lgetc(lp, 2) == 'd') ? TRUE : FALSE);
}
+static int
+d_warpdot(const char *l_text)
+{
+ const char *tp = l_text;
+ int field = 0;
+
+ /*
+* Find the byte offset to the (space-delimited) filename
+* field in formatted ls output.
+*/
+ while (*tp != '\0') {
+ if (*tp == ' ') {
+ tp += strspn(tp, " ");
+ if (++field == 9)
+ break;
+ }
+ tp++;
+ }
+ return (tp - l_text);
+}
+
+static int
+d_forwpage(int f, int n)
+{
+ forwpage(f | FFRAND, n);
+ curwp->w_doto = d_warpdot(curwp->w_dotp->l_text);
+ return (TRUE);
+}
+
+static int
+d_backpage (int f, int n)
+{
+ backpage(f | FFRAND, n);
+ curwp->w_doto = d_warpdot(curwp->w_dotp->l_text);
+ return (TRUE);
+}
+
+sta
Re: [PATCH] dired mg patch
> The idea of removing the error to behave like the rest > of mg would lead to a brittle design. It's like assuming > errors can only happen once. It makes code faster, but > later changes could cause subtle bugs that could be hard to > track IMHO. Quite the opposite, in fact. The idea is to catch errors where and when they happen (i.e. as early as possible), and handle them appropriately, so they do not propagate. This means there are very few places where error checks are needed, which makes it easier to verify these checks are correct. This is no different from designing a function to return as early as possible if, say, a malloc fails. If you don't do this and instead let the errors propagate, all the basic functions will be littered with checks for brokennes that they shouldn't need to deal with in the first place. With such a litter, it'll be easy to break one of these checks. Why should dired behave different from the rest of mg, when it uses the same buffer handling code anyway?
Re: [PATCH] dired mg patch
> The warpdot() has at least one issue. It leads to > segfaults if you try to open a directory like (BCD). [.. diff ..] > ed > > mg doesn't segfault. Your fix is wrong, and only works by chance. If you craft a directory with enough spaces in its name, it'll segfault again. And you cannot fix this in warpdot, it's the wrong place. The bug is in dired_(), which should abort if the command fails. > Here's a snippet from ntpd.c > if ((pw = getpwnam(NTPD_USER)) == NULL) > errx(1, "unknown user %s", NTPD_USER); > When you're designing functions, you should account for > error values as well. -1 is used by most code written > from scratch in base. The ntpd quote is totally irrelevant. When designing functions, you have to account for errors, if such can happen. In library functions, returning an error value is often the only possible solution, because the library does not know how you want to solve the situation. In other places, there may or may not be a way to handle the error immediately. For a simple application, the developer could decide that it's simply not worth it trying to recover from a failed allocation; in this case, he handles it with a wrapper function which simply makes the program quit instantly. In other cases, you have options such as "do nothing", in which case the operation does not complete, but it doesn't cause a crash either. An example: trying to move the dot past the start or end of the buffer in mg would result in a no-op, and other functions do not have to try and verify that the dot is within the legal range. In the addline case, there is a way to handle the error: don't add a line if you can't allocate it. The error is handled in such a way that most of the other functions do not have to worry about it. There are other places where errors are handled "up the stream" likewise. If you still think this is wrong, I could cook you a 20k line diff that makes every single function check that 1) the character we read from stdin isn't EOF, 2) curwp is not null, 3) curwp->w_dotp is not null, 4) curbp is not null, and so on. By now you should realize how ridiculous this is. These errors are (or should be) accounted for up the stream so that none of the functions that actually have to operate on existing buffer or window contents should worry about it. > Your warpdot() works differently > and doesn't quite conform to style. You're assigning > a value to a variable without checking if this is correct > or not. This style is hard to read according to me. Bullshit. It's not a matter of style; it's a question of whether or not an error can happen, and what to do when it happens. In this case, if there was an error to handle, we could return zero, which will always be a legal value (since we are assigning to an int which is zero if the line is empty). If warpdot was a library function whose callers need to react to the error in different ways, then we'd have to return an error value. Like I said above, the segfault you got has nothing to do with warpdot, and trying to fix it there is wrong.
Re: [PATCH] dired mg patch
> > The warpdot() has at least one issue. It leads to
> > segfaults if you try to open a directory like (BCD).
New diff below. I wanted to make d_warpdot behave exactly
like the rest of mg functions in that it'd take the two standard
int (f, n) arguments, and dereference curwp->* to do its job.
This was sort of a dead end, since curwp is only set after
everything's been done and the various places that call
dired_() have also called showbuffer() (or the alternative).
All of these would've needed an additional function call, or
a lot of restructuring.
Instead I made warpdot take the line pointer (dotp) and
a pointer to doto, giving it all it needs to operate on
the line mostly like the rest of mg. Now it does the usual
end-of-line check by comparing doto to llength(). Looking
for the NUL byte while iterating over the string was never
the right approach for l_text.
These changes mean dired_() can no longer do horrible evil
like computing the dot offset based on an internal char array
which may or may not reflect the reality of what's actually in
the buffer. This is what caused the segfaults you noticed.
It follows that the "first entry after .." logic is no longer
interleaved with a bunch of unrelated stuff. Now it's done
in a separate step after all the ls output has been read.
I also noticed some missing names in the list of keybindings,
and added corresponding funmap_add lines.
Index: dired.c
===
RCS file: /cvs/src/usr.bin/mg/dired.c,v
retrieving revision 1.48
diff -u -p -r1.48 dired.c
--- dired.c 23 Jan 2011 00:45:03 - 1.48
+++ dired.c 27 Aug 2011 22:32:36 -
@@ -36,6 +36,11 @@ static intd_rename(int, int);
static int d_shell_command(int, int);
static int d_create_directory(int, int);
static int d_makename(struct line *, char *, size_t);
+static int d_warpdot(struct line *, int *);
+static int d_forwpage(int, int);
+static int d_backpage(int, int);
+static int d_forwline(int, int);
+static int d_backline(int, int);
static void reaper(int);
extern struct keymap_s helpmap, cXmap, metamap;
@@ -57,15 +62,15 @@ static PF dirednul[] = {
static PF diredcl[] = {
reposition, /* ^L */
d_findfile, /* ^M */
- forwline, /* ^N */
+ d_forwline, /* ^N */
rescan, /* ^O */
- backline, /* ^P */
+ d_backline, /* ^P */
rescan, /* ^Q */
backisearch,/* ^R */
forwisearch,/* ^S */
rescan, /* ^T */
universal_argument, /* ^U */
- forwpage, /* ^V */
+ d_forwpage, /* ^V */
rescan, /* ^W */
NULL/* ^X */
};
@@ -77,7 +82,7 @@ static PF diredcz[] = {
rescan, /* ^] */
rescan, /* ^^ */
rescan, /* ^_ */
- forwline, /* SP */
+ d_forwline, /* SP */
d_shell_command,/* ! */
rescan, /* " */
rescan, /* # */
@@ -99,9 +104,9 @@ static PF diredc[] = {
};
static PF diredn[] = {
- forwline, /* n */
+ d_forwline, /* n */
d_ffotherwindow,/* o */
- backline, /* p */
+ d_backline, /* p */
rescan, /* q */
d_rename, /* r */
rescan, /* s */
@@ -116,13 +121,32 @@ static PF direddl[] = {
d_undelbak /* del */
};
+static PF diredbp[] = {
+ d_backpage /* v */
+};
+
+static PF dirednull[] = {
+ NULL
+};
+
#ifndefDIRED_XMAPS
#defineNDIRED_XMAPS0 /* number of extra map sections */
#endif /* DIRED_XMAPS */
-static struct KEYMAPE (6 + NDIRED_XMAPS + IMAPEXT) diredmap = {
- 6 + NDIRED_XMAPS,
- 6 + NDIRED_XMAPS + IMAPEXT,
+static struct KEYMAPE (1 + IMAPEXT) d_backpagemap = {
+ 1,
+ 1 + IMAPEXT,
+ rescan,
+ {
+ {
+ 'v', 'v', diredbp, NULL
+ }
+ }
+};
+
+static struct KEYMAPE (7 + NDIRED_XMAPS + IMAPEXT) diredmap = {
+ 7 + NDIRED_XMAPS,
+ 7 + NDIRED_XMAPS + IMAPEXT,
rescan,
{
#ifndef NO_HELP
@@ -138,6 +162,10 @@ static struct KEYMAPE (6 + NDIRED_XMAPS
CCHR('L'), CCHR('X'), diredcl, (KEYMAP *) & cXmap
},
{
+ CCHR('['), CCHR('['), dirednull, (KEYMAP *) &
+ d_backpagemap
+ },
+ {
CCHR('Z'), '+', diredcz, (KEYMAP *) & metamap
},
{
@@ -165,8 +193,12 @@ dired_init(void)
f
Re: [PATCH] dired mg patch
> > > The warpdot() has at least one issue. It leads to > > > segfaults if you try to open a directory like (BCD). > > > > [.. diff ..] > > > ed > > > > > > mg doesn't segfault. > > > > Your fix is wrong, and only works by chance. If you craft > > a directory with enough spaces in its name, it'll segfault > > again. And you cannot fix this in warpdot, it's the wrong > > place. The bug is in dired_(), which should abort if the > > command fails. > > > The proper fix is to escape shell metacharacters. That is one (ugly, if you will) way to fix the problem that makes it impossible to open files with funny names. That, however, does not fix the segfaults, because if sh or ls fail for any reason, you'll again adjust the dot offset based on the char array which does not reflect reality. The metacharacter issue is also a bug not related to the warpping we're doing, and is probably best fixed in a separate diff. > There's already strspn(), why write your own > substitute for warpdot() ? Because llength() is the correct way to determine where a line ends in mg. Notably, lines may embed NUL bytes, and they're not required to terminate with one. Checking the length also makes sure you don't try to read an empty line (which may or may be a NULL pointer, I don't care). > You're also returning by supplying the function inside. > This makes the code less readable. I'm just returning (TRUE) because the function can never fail. You could also decide that it's appropriate to return (FALSE) if the 9th field cannot be located, but this doesn't change how the assignment should be done. The actual processing is done through pointers on the stack because we cannot do it via the global pointer (curwp), for the reasons I outlined in my previous message. > Lastly, the changes you made in dired_() seem > overcomplicated to me. If you prefer interleaving unrelated operations, be my guest. After all, it's not my choice. I just hope I needn't touch such code. > You're iterating twice for lforw() and d_warpdot(). If I wasn't already sick of this thread, I'd ask you to quote the exact lines of code to show where I'm iterating twice. Because I don't see it. > In the while() loop, we can already locate the .. and > we just need to lforw() there using the warp variable. > This seems redundant to me. Yes, we could, and I explained why I changed that (which is 1: giving warpdot only a pointer to char array is wrong, and 2: the code was interleaved with other things, namely the reading of ls output, and the closing of the pipe). There's a third reason, which is the misuse of strrchr to extract the filename. So I cleaned up the problematic bunch and fixed the segfault. > > > Your warpdot() works differently > > > and doesn't quite conform to style. You're assigning > > > a value to a variable without checking if this is correct > > > or not. This style is hard to read according to me. > > > > Bullshit. It's not a matter of style; it's a question > > It's a matter of style. The way you're designing the loops > and the functions look needlessly complicated to me, and > it's a bit hard to follow. God. Should forwchar() also return a dot offset and line pointer and let the caller assign them and decide how to handle the exception when they point out of bounds? What if this is a showstopper -- should the caller then return these two values on top of its own return value, and let the caller of the caller decide what to do? We might as propagate all errors to main(). Do you want that 20k line diff? > > of whether or not an error can happen, and what to do when > > it happens. In this case, if there was an error to handle, > > we could return zero, which will always be a legal value (since > > we are assigning to an int which is zero if the line is empty). > > Warpdot() is meant to find the 9th column, if it can't, then something's > wrong. It should return -1 to indicate failure. To be safe, doto > should be set to 0. See above.
Re: [PATCH] dired mg patch
> It fixes the issue that if a filename has a space at the start of > it, the point will stay in the first character column and not jump > to the first non ' ' character in the filename. Yup, this looks good to me. > However, it does seem to expose a bug in dired, that if a filename > has a space at the start of it, mg doesn't find it if you try and > open it. mg gives a message of (New File) and creates and empty > buffer. But in the mean time, I think this change does make this > diff more correct. Indeed. This looks like a flaw in d_makename, which seems to implement its own warpdot() :-) This should be changed to use our new function (and we could make use of NAME_FIELD). That fix, along with the one for dealing with shell metacharacters, should probably be a separate diff though.
Re: [PATCH] dired mg patch
> > However, it does seem to expose a bug in dired, that if a filename
> > has a space at the start of it, mg doesn't find it if you try and
> > open it. mg gives a message of (New File) and creates and empty
> > buffer. But in the mean time, I think this change does make this
> > diff more correct.
>
> Indeed. This looks like a flaw in d_makename, which seems to implement
> its own warpdot() :-) This should be changed to use our new function
> (and we could make use of NAME_FIELD). That fix, along with the one
> for dealing with shell metacharacters, should probably be a separate
> diff though.
And here's that diff. Makename() uses warpdot(). I also refined warpdot
to use NAME_FIELD and return (FALSE) if the field cannot be located (this
is the change I anticipated earlier). This return value should make a
couple conditions easier to understand.
Then there's ls. Trying to escape shell metacharacters is one of the
uglier things a program can do, if the alternative of passing the string
directly to a program is available. Most of this code (fork & exec) is
already in dired.c in shell_command(). I took this code and put it
into a separate function, d_exec(), and added some vararg magic. Using
this function instead of popen() eliminates the issues with metachars
while making dired_() quite a bit simpler.
One bit I'm not particularly fond of is the first parameter of d_exec,
which is used to prefix the lines with spaces (needed so the action chars
like '!' fit on the line in front of ls output). On the principle of YAGNI,
however, I did not implement a bigger hammer that would've let the caller
read & process lines one at a time; this would've involved more functions
and state.
I also added a few relevant comments.
Index: dired.c
===
RCS file: /cvs/src/usr.bin/mg/dired.c,v
retrieving revision 1.49
diff -u -p -r1.49 dired.c
--- dired.c 29 Aug 2011 11:02:06 - 1.49
+++ dired.c 29 Aug 2011 13:54:10 -
@@ -21,6 +21,7 @@
#include
#include
#include
+#include
voiddired_init(void);
static int dired(int, int);
@@ -33,6 +34,7 @@ static int d_expunge(int, int);
static int d_copy(int, int);
static int d_del(int, int);
static int d_rename(int, int);
+static int d_exec(int, struct buffer *, const char *, const char *, ...);
static int d_shell_command(int, int);
static int d_create_directory(int, int);
static int d_makename(struct line *, char *, size_t);
@@ -483,14 +485,10 @@ reaper(int signo __attribute__((unused))
int
d_shell_command(int f, int n)
{
- char command[512], fname[MAXPATHLEN], buf[BUFSIZ], *bufp, *cp;
- int infd, fds[2];
- pid_tpid;
- struct sigaction olda, newa;
+ char command[512], fname[MAXPATHLEN], *bufp;
struct buffer *bp;
struct mgwin*wp;
- FILE*fin;
- char sname[NFILEN];
+ char sname[NFILEN];
bp = bfind("*Shell Command Output*", TRUE);
if (bclear(bp) != TRUE)
@@ -506,11 +504,61 @@ d_shell_command(int f, int n)
bufp = eread("! on %s: ", command, sizeof(command), EFNEW, sname);
if (bufp == NULL)
return (ABORT);
- infd = open(fname, O_RDONLY);
- if (infd == -1) {
+
+ if (d_exec(0, bp, fname, "sh", "-c", command, NULL) != TRUE)
+ return (ABORT);
+
+ if ((wp = popbuf(bp, WNONE)) == NULL)
+ return (ABORT); /* XXX - free the buffer?? */
+ curwp = wp;
+ curbp = wp->w_bufp;
+ return (TRUE);
+}
+
+/*
+ * Pipe input file to cmd and insert the command's output in the
+ * given buffer. Each line will be prefixed with the given
+ * number of spaces.
+ */
+static int
+d_exec(int space, struct buffer *bp, const char *input, const char *cmd, ...)
+{
+ char buf[BUFSIZ];
+ va_list ap;
+ struct sigaction olda, newa;
+ char**argv, *cp;
+ FILE*fin;
+ pid_tpid;
+ int infd, fds[2];
+ int n;
+
+ /* Find the number of arguments. */
+ va_start(ap, cmd);
+ for (n = 2; va_arg(ap, char *) != NULL; n++)
+ ;
+ va_end(ap);
+
+ /* Allocate and build the argv. */
+ if ((argv = calloc(n, sizeof(*argv))) == NULL) {
+ ewprintf("Can't allocate argv : %s", strerror(errno));
+ return (FALSE);
+ }
+
+ n = 1;
+ argv[0] = (char *)cmd;
+ va_start(ap, cmd);
+ while ((argv[n] = va_arg(ap, char *)) != NULL)
+ n++;
+ va_end(ap);
+
+ if (input == NULL)
+ input = "/dev/null";
+
+ if ((infd = open(input, O_RDONLY)) == -1) {
ewprintf("Can't open input file : %s", strerror(errno));
return (FALSE);
}
+
if (pipe(fds) == -1) {
ewprintf("Can't create pipe : %s", s
Re: [PATCH] dired mg patch
> And here's that diff. [..]
It never hurts to re-read a diff before going to bed. Indeed, I
forgot to free argv (execlp uses alloca). The number of things
to clean up after one thing fails is almost getting out of hand,
and at least the fork() case wasn't handled properly. In this new
revision of d_exec, argv and all the fds are checked and freed
(only) at the end of the function, if necessary. That's where we
jump to, should anything fail.
The rest of the diff is as before.
Index: dired.c
===
RCS file: /cvs/src/usr.bin/mg/dired.c,v
retrieving revision 1.49
diff -u -p -r1.49 dired.c
--- dired.c 29 Aug 2011 11:02:06 - 1.49
+++ dired.c 29 Aug 2011 23:27:09 -
@@ -21,6 +21,7 @@
#include
#include
#include
+#include
voiddired_init(void);
static int dired(int, int);
@@ -33,6 +34,7 @@ static int d_expunge(int, int);
static int d_copy(int, int);
static int d_del(int, int);
static int d_rename(int, int);
+static int d_exec(int, struct buffer *, const char *, const char *, ...);
static int d_shell_command(int, int);
static int d_create_directory(int, int);
static int d_makename(struct line *, char *, size_t);
@@ -483,14 +485,10 @@ reaper(int signo __attribute__((unused))
int
d_shell_command(int f, int n)
{
- char command[512], fname[MAXPATHLEN], buf[BUFSIZ], *bufp, *cp;
- int infd, fds[2];
- pid_tpid;
- struct sigaction olda, newa;
+ char command[512], fname[MAXPATHLEN], *bufp;
struct buffer *bp;
struct mgwin*wp;
- FILE*fin;
- char sname[NFILEN];
+ char sname[NFILEN];
bp = bfind("*Shell Command Output*", TRUE);
if (bclear(bp) != TRUE)
@@ -506,68 +504,124 @@ d_shell_command(int f, int n)
bufp = eread("! on %s: ", command, sizeof(command), EFNEW, sname);
if (bufp == NULL)
return (ABORT);
- infd = open(fname, O_RDONLY);
- if (infd == -1) {
+
+ if (d_exec(0, bp, fname, "sh", "-c", command, NULL) != TRUE)
+ return (ABORT);
+
+ if ((wp = popbuf(bp, WNONE)) == NULL)
+ return (ABORT); /* XXX - free the buffer?? */
+ curwp = wp;
+ curbp = wp->w_bufp;
+ return (TRUE);
+}
+
+/*
+ * Pipe input file to cmd and insert the command's output in the
+ * given buffer. Each line will be prefixed with the given
+ * number of spaces.
+ */
+static int
+d_exec(int space, struct buffer *bp, const char *input, const char *cmd, ...)
+{
+ char buf[BUFSIZ];
+ va_list ap;
+ struct sigaction olda, newa;
+ char**argv = NULL, *cp;
+ FILE*fin;
+ int fds[2] = { -1, -1 };
+ int infd = -1;
+ int ret = (ABORT), n;
+ pid_tpid;
+
+ if (sigaction(SIGCHLD, NULL, &olda) == -1)
+ return (ABORT);
+
+ /* Find the number of arguments. */
+ va_start(ap, cmd);
+ for (n = 2; va_arg(ap, char *) != NULL; n++)
+ ;
+ va_end(ap);
+
+ /* Allocate and build the argv. */
+ if ((argv = calloc(n, sizeof(*argv))) == NULL) {
+ ewprintf("Can't allocate argv : %s", strerror(errno));
+ goto out;
+ }
+
+ n = 1;
+ argv[0] = (char *)cmd;
+ va_start(ap, cmd);
+ while ((argv[n] = va_arg(ap, char *)) != NULL)
+ n++;
+ va_end(ap);
+
+ if (input == NULL)
+ input = "/dev/null";
+
+ if ((infd = open(input, O_RDONLY)) == -1) {
ewprintf("Can't open input file : %s", strerror(errno));
- return (FALSE);
+ goto out;
}
+
if (pipe(fds) == -1) {
ewprintf("Can't create pipe : %s", strerror(errno));
- close(infd);
- return (FALSE);
+ goto out;
}
newa.sa_handler = reaper;
newa.sa_flags = 0;
- if (sigaction(SIGCHLD, &newa, &olda) == -1) {
- close(infd);
- close(fds[0]);
- close(fds[1]);
- return (ABORT);
+ if (sigaction(SIGCHLD, &newa, NULL) == -1)
+ goto out;
+
+ if ((pid = fork()) == -1) {
+ ewprintf("Can't fork");
+ goto out;
}
- pid = fork();
+
switch (pid) {
- case -1:
- ewprintf("Can't fork");
- return (ABORT);
- case 0:
+ case 0: /* Child */
close(fds[0]);
dup2(infd, STDIN_FILENO);
dup2(fds[1], STDOUT_FILENO);
dup2(fds[1], STDERR_FILENO);
- execl("/bin/sh", "sh", "-c", bufp, (char *)NULL);
+ execvp(argv[0], argv);
exit(1);
break;
- default:
+ default: /* Parent */
clo
Re: ctags(1) and mg
> Being a little less stupid this time. Rewrote the string handling > part, style(9) formatting and refactored pushtag function. Some thoughts about this. Are you sure you're not going to overflow t in re_tag_conv when you insert escapes? Correct me if I'm wrong, but I think ctags patterns aren't really regexes (which is why you're playing with escapes, and stripping & re-inserting magic) -- wouldn't it be easier to do the matching in a plain loop, checking the start/end offsets against bol/eol if the magic anchors were present? Second, it seems you're allocating a number of strings in addctag. This isn't technically wrong, but one of the upsides of NUL-terminated strings is that you can split one into many (as strsep does) without messing with multiple buffers & allocs & frees. I'd supplement ctagnode with a flag field to tell which magic anchors were present originally. I'd actually strip all the magic and backslashes in addctag, such that pattern pointed to by pat is simply a plain string which must exactly match what's in the buffer, character by character -- making the match loop I proposed above a trivial one. Finally, the choice to use underscores in the name of one function stands out as a little bit odd ;-) That's what I got at a quick glance. I'll try give it another look once this fever and headache fade.
Re: libc malloc poison
> On Thu, Jul 04, 2013 at 05:24:20PM +0200, Mark Kettenis wrote: > > > From: Theo de Raadt > > > Date: Thu, 04 Jul 2013 09:04:54 -0600 > > > > > > I suspect the best approach would be a hybrid value. The upper half > > > of the address should try to land in an unmapped zone, or into the zero > > > page, or into some address space hole, ir into super high memory above > > > the stack which is gauranteed unmapped. > > > > Don't forget strict alignment architectures, where it is beneficial > > to have the lowest bit set to trigger alignment traps. > > You also want the highest bit set. This makes sure signed indexes get > interpreted as negative, which should more often detect problems than > positive ones. There are not only pointers in the heap. A while ago someone hit a bug in my code that I hadn't (and wouldn't ever have) triggered with MALLOC_OPTIONS=S because the Duh pattern always sets the high bit, giving a negative integer which just so happened to be harmless. Back then I wished malloc would've used all random bits instead. If you test run your code more than a couple times (as one obviously should, before going into release), then I'd say randomness (which will likely soon produce a bug-trigger value) is better than a fixed pattern that attempts to be useful in some (common?) scenarios but will reliably fail to make a difference in other cases. I do see the value in having the option to use fixed patterns though, so perhaps the hybrid value with a configurable mask as Theo proposed is a good idea.
Stairstep mouse motion
So I needed to see my thoughts on paper but my desk was so full of stuff
I couldn't make room for pen and paper. Instead I fired up Gimp, and
drawing with the mouse worked fine until I realized it's next to impossible
to draw diagonal lines that look like lines.
Instead of straight lines, I got waves. The faster I draw the mouse, the
deeper the waves. It looked like diagonal mouse motion generated a pair
of pointer motion events, one for the X axis and another for Y. And Gimp
smoothed that stairstep motion, resulting in waves. Xev(1) confirmed my
hypothesis.
It turns out wsmouse(4) is breaking the mouse input into multiple events.
This isn't necessarily a bug, since it allows for a very small and simple
event structure which works without modification as new properties (such
as button states, axes, etc.) are added.
Now wsmouse generates all the events it can in a loop before waking up
the process that waits for these events. So on the receiving side (i.e.
in the xenocara ws(4) driver) we can sum all the consecutive X and Y
deltas from a single read() before issuing a pointer motion event. This
eliminates the stairsteps as long as the events generated by wsmouse fit
in the buffers involved.
Other approaches would be either extending the event structure, or perhaps
adding a new event type that somehow communicates the intended grouping
for events that follow/precede (but ugh...). I felt the ws(4) fix was
the least intrusive, and it appears to work just fine here. What do you
think?
This image (drawn in Gimp) illustrates the problem. The last two lines
were drawn with my diff applied.
http://guu.fi/mousebug.png
Index: xenocara/driver/xf86-input-ws/src/ws.c
===
RCS file: /cvs/xenocara/driver/xf86-input-ws/src/ws.c,v
retrieving revision 1.57
diff -u -p -r1.57 ws.c
--- xenocara/driver/xf86-input-ws/src/ws.c 8 Jul 2012 14:22:03 -
1.57
+++ xenocara/driver/xf86-input-ws/src/ws.c 7 Jul 2013 18:33:57 -
@@ -474,7 +474,7 @@ wsReadInput(InputInfoPtr pInfo)
{
WSDevicePtr priv = (WSDevicePtr)pInfo->private;
static struct wscons_event eventList[NUMEVENTS];
- int n, c;
+ int n, c, dx, dy;
struct wscons_event *event = eventList;
unsigned char *pBuf;
@@ -488,10 +488,11 @@ wsReadInput(InputInfoPtr pInfo)
if (n == 0)
return;
+ dx = dy = 0;
n /= sizeof(struct wscons_event);
while (n--) {
int buttons = priv->lastButtons;
- int dx = 0, dy = 0, dz = 0, dw = 0, ax = 0, ay = 0;
+ int dz = 0, dw = 0, ax = 0, ay = 0;
int zbutton = 0, wbutton = 0;
switch (event->type) {
@@ -506,11 +507,11 @@ wsReadInput(InputInfoPtr pInfo)
buttons));
break;
case WSCONS_EVENT_MOUSE_DELTA_X:
- dx = event->value;
+ dx += event->value;
DBG(4, ErrorF("Relative X %d\n", event->value));
break;
case WSCONS_EVENT_MOUSE_DELTA_Y:
- dy = -event->value;
+ dy -= event->value;
DBG(4, ErrorF("Relative Y %d\n", event->value));
break;
case WSCONS_EVENT_MOUSE_ABSOLUTE_X:
@@ -548,14 +549,18 @@ wsReadInput(InputInfoPtr pInfo)
}
++event;
- if (dx || dy) {
- if (wsWheelEmuFilterMotion(pInfo, dx, dy))
+ if ((dx || dy) && event->type != WSCONS_EVENT_MOUSE_DELTA_X &&
+ event->type != WSCONS_EVENT_MOUSE_DELTA_Y) {
+ int tmpx = dx, tmpy = dy;
+ dx = dy = 0;
+
+ if (wsWheelEmuFilterMotion(pInfo, tmpx, tmpy))
continue;
/* relative motion event */
DBG(3, ErrorF("postMotionEvent dX %d dY %d\n",
- dx, dy));
- xf86PostMotionEvent(pInfo->dev, 0, 0, 2, dx, dy);
+ tmpx, tmpy));
+ xf86PostMotionEvent(pInfo->dev, 0, 0, 2, tmpx, tmpy);
}
if (dz && priv->Z.negative != WS_NOMAP
&& priv->Z.positive != WS_NOMAP) {
@@ -583,9 +588,9 @@ wsReadInput(InputInfoPtr pInfo)
ay = tmp;
}
if (ax) {
- dx = ax - priv->old_ax;
+ int xdelta = ax - priv->old_ax;
priv->old_ax = ax;
- if (wsWheelEmuFilterMotion(pInfo, dx, 0))
+ if (wsWheelEmuFilterMotion(pInfo, xdelta, 0))
continue;
/* absolute position event */
@@ -593,15 +598,24 @@ wsReadInp
Re: Stairstep mouse motion
> The issue that input drivers devices need high refresh frequency to be > able to achieve high-precision freehand drawing is quite well known¹. Yes. But the bug here isn't about that. I think I'll have to elaborate a little. When you move the mouse, it will report its motion in an event with a two-component vector. Naturally both components must always be present, otherwise information is lost. When wsmouse(4) gets such an event, it will break that event into two separate events, one for the X delta and another for Y. This is because the current event structure cannot contain the deltas for both axes. Now ws(4) takes these two events and naturally considers them independent (as there is no metadata to couple them together), and fills in the blanks by setting one or the other delta to zero. The result is two orthogonal vectors which represent a motion your mouse never reported. Their sum is the original vector, but the motion they describe is still wrong. So the problem here is not refresh frequency, precision or anything like that. It's just that our data gets effectively mangled and corrupted between the two layers. My diff attempts to reconstruct the original vector by summing consecutive delta events, but as you noted below, this can also result in data loss. The lossless method would involve extending the ws event structure or adding some metadata (in the form of new events), or perhaps stuffing both deltas into the value field we have now (this is lossless only if they fit in 16 bits).. Interpolation is something I definitely wouldn't apply here :-) > I re-did a few experiments with and without your patch on my laptop > (with the Lenovo track point wihch is a relative device). I could not > reproduce the waves you're getting in Gimp, but without your patch > diagonals show indeed larger staircases effects. Under xfig(1) or > bitmap(1) the difference is much less noticeable. Clearly the > strategy used by applications to trac mouse motion for freehand > drawing are not the same. > > I do fear that with some devices your patch will collapse too many > events and make it harder to follow small radius curves. Right, I did not consider this case. If this is a problem, perhaps the code could be changed to only collapse a pair of DELTA_X and DELTA_Y events, but never more than that. This way it might still incorrectly merge two independent motions along the axes into one diagonal motion, but I would expect that to be rare, and the deltas to be so small that it has very little impact on anything.
Re: Stairstep mouse motion
> > I do fear that with some devices your patch will collapse too many
> > events and make it harder to follow small radius curves.
>
> Right, I did not consider this case. If this is a problem, perhaps
> the code could be changed to only collapse a pair of DELTA_X and
> DELTA_Y events, but never more than that. This way it might still
> incorrectly merge two independent motions along the axes into one
> diagonal motion, but I would expect that to be rare, and the deltas
> to be so small that it has very little impact on anything.
Here's a diff that does just that. If ws receives more than one
delta along an axis, it will not sum these; each will go in a separate
event. But if it gets an X delta followed by an Y delta (or vice versa),
these will be combined into a single event.
Index: xenocara/driver/xf86-input-ws/src/ws.c
===
RCS file: /cvs/xenocara/driver/xf86-input-ws/src/ws.c,v
retrieving revision 1.57
diff -u -p -r1.57 ws.c
--- xenocara/driver/xf86-input-ws/src/ws.c 8 Jul 2012 14:22:03 -
1.57
+++ xenocara/driver/xf86-input-ws/src/ws.c 8 Jul 2013 17:12:27 -
@@ -474,7 +474,7 @@ wsReadInput(InputInfoPtr pInfo)
{
WSDevicePtr priv = (WSDevicePtr)pInfo->private;
static struct wscons_event eventList[NUMEVENTS];
- int n, c;
+ int n, c, dx, dy;
struct wscons_event *event = eventList;
unsigned char *pBuf;
@@ -488,10 +488,11 @@ wsReadInput(InputInfoPtr pInfo)
if (n == 0)
return;
+ dx = dy = 0;
n /= sizeof(struct wscons_event);
while (n--) {
int buttons = priv->lastButtons;
- int dx = 0, dy = 0, dz = 0, dw = 0, ax = 0, ay = 0;
+ int newdx = 0, newdy = 0, dz = 0, dw = 0, ax = 0, ay = 0;
int zbutton = 0, wbutton = 0;
switch (event->type) {
@@ -506,11 +507,17 @@ wsReadInput(InputInfoPtr pInfo)
buttons));
break;
case WSCONS_EVENT_MOUSE_DELTA_X:
- dx = event->value;
+ if (!dx)
+ dx = event->value;
+ else
+ newdx = event->value;
DBG(4, ErrorF("Relative X %d\n", event->value));
break;
case WSCONS_EVENT_MOUSE_DELTA_Y:
- dy = -event->value;
+ if (!dy)
+ dy = -event->value;
+ else
+ newdy = -event->value;
DBG(4, ErrorF("Relative Y %d\n", event->value));
break;
case WSCONS_EVENT_MOUSE_ABSOLUTE_X:
@@ -548,14 +555,20 @@ wsReadInput(InputInfoPtr pInfo)
}
++event;
- if (dx || dy) {
- if (wsWheelEmuFilterMotion(pInfo, dx, dy))
+ if ((newdx || newdy) || ((dx || dy) &&
+ event->type != WSCONS_EVENT_MOUSE_DELTA_X &&
+ event->type != WSCONS_EVENT_MOUSE_DELTA_Y)) {
+ int tmpx = dx, tmpy = dy;
+ dx = newdx;
+ dy = newdy;
+
+ if (wsWheelEmuFilterMotion(pInfo, tmpx, tmpy))
continue;
/* relative motion event */
DBG(3, ErrorF("postMotionEvent dX %d dY %d\n",
- dx, dy));
- xf86PostMotionEvent(pInfo->dev, 0, 0, 2, dx, dy);
+ tmpx, tmpy));
+ xf86PostMotionEvent(pInfo->dev, 0, 0, 2, tmpx, tmpy);
}
if (dz && priv->Z.negative != WS_NOMAP
&& priv->Z.positive != WS_NOMAP) {
@@ -583,9 +596,9 @@ wsReadInput(InputInfoPtr pInfo)
ay = tmp;
}
if (ax) {
- dx = ax - priv->old_ax;
+ int xdelta = ax - priv->old_ax;
priv->old_ax = ax;
- if (wsWheelEmuFilterMotion(pInfo, dx, 0))
+ if (wsWheelEmuFilterMotion(pInfo, xdelta, 0))
continue;
/* absolute position event */
@@ -593,15 +606,24 @@ wsReadInput(InputInfoPtr pInfo)
xf86PostMotionEvent(pInfo->dev, 1, 0, 1, ax);
}
if (ay) {
- dy = ay - priv->old_ay;
+ int ydelta = ay - priv->old_ay;
priv->old_ay = ay;
- if (wsWheelEmuFilterMotion(pInfo, 0, dy))
+ if (wsWheelEmuFilterMotion(pInfo, 0, ydelta))
continue;
/* absolute position event */
Add back-to-indentation (M-m) for mg
This adds the command that moves the dot to the first non-whitespace
character on the line.
Index: src/usr.bin/mg/def.h
===
RCS file: /cvs/src/usr.bin/mg/def.h,v
retrieving revision 1.113
diff -u -p -r1.113 def.h
--- src/usr.bin/mg/def.h30 Jun 2010 19:12:54 - 1.113
+++ src/usr.bin/mg/def.h22 Dec 2010 16:12:15 -
@@ -511,6 +511,7 @@ int indent(int, int);
int forwdel(int, int);
int backdel(int, int);
int space_to_tabstop(int, int);
+int gotoindent(int, int);
/* extend.c X */
int insert(int, int);
Index: src/usr.bin/mg/funmap.c
===
RCS file: /cvs/src/usr.bin/mg/funmap.c,v
retrieving revision 1.32
diff -u -p -r1.32 funmap.c
--- src/usr.bin/mg/funmap.c 15 Sep 2008 16:13:35 - 1.32
+++ src/usr.bin/mg/funmap.c 22 Dec 2010 16:12:15 -
@@ -191,6 +191,7 @@ static struct funmap functnames[] = {
{showcpos, "what-cursor-position",},
{filewrite, "write-file",},
{yank, "yank",},
+ {gotoindent, "back-to-indentation",},
{NULL, NULL,}
};
Index: src/usr.bin/mg/keymap.c
===
RCS file: /cvs/src/usr.bin/mg/keymap.c,v
retrieving revision 1.43
diff -u -p -r1.43 keymap.c
--- src/usr.bin/mg/keymap.c 27 Aug 2008 04:11:52 - 1.43
+++ src/usr.bin/mg/keymap.c 22 Dec 2010 16:12:15 -
@@ -241,7 +241,7 @@ static PF metasqf[] = {
static PF metal[] = {
lowerword, /* l */
- rescan, /* m */
+ gotoindent, /* m */
rescan, /* n */
rescan, /* o */
rescan, /* p */
Index: src/usr.bin/mg/mg.1
===
RCS file: /cvs/src/usr.bin/mg/mg.1,v
retrieving revision 1.47
diff -u -p -r1.47 mg.1
--- src/usr.bin/mg/mg.1 7 Oct 2010 17:08:58 - 1.47
+++ src/usr.bin/mg/mg.1 22 Dec 2010 16:12:15 -
@@ -230,6 +230,8 @@ kill-word
forward-word
.It M-l
downcase-word
+.It M-m
+back-to-indentation
.It M-q
fill-paragraph
.It M-r
@@ -402,6 +404,8 @@ Set all characters in the region to lowe
Set characters to lower case, starting at the dot, and ending
.Va n
words away.
+.It back-to-indentation
+Move the dot to the first non-whitespace character of the line.
.It emacs-version
Return an
.Nm
Index: src/usr.bin/mg/random.c
===
RCS file: /cvs/src/usr.bin/mg/random.c,v
retrieving revision 1.26
diff -u -p -r1.26 random.c
--- src/usr.bin/mg/random.c 15 Sep 2008 16:13:35 - 1.26
+++ src/usr.bin/mg/random.c 22 Dec 2010 16:12:15 -
@@ -440,3 +440,16 @@ space_to_tabstop(int f, int n)
return (linsert((n << 3) - (curwp->w_doto & 7), ' '));
}
#endif /* NOTAB */
+
+/*
+ * Move the dot to the first non-whitespace character of the current line.
+ */
+int
+gotoindent(int f, int n)
+{
+ gotobol(FFRAND, 1);
+ while (curwp->w_doto < llength(curwp->w_dotp) &&
+ (isspace(lgetc(curwp->w_dotp, curwp->w_doto
+ ++curwp->w_doto;
+ return (TRUE);
+}
mg: join-line
Here's another command I always miss while working with mg.
diff -up src/usr.bin/mg.old/def.h src/usr.bin/mg/def.h
--- src/usr.bin/mg.old/def.hFri Jan 14 17:27:17 2011
+++ src/usr.bin/mg/def.hFri Jan 14 17:49:34 2011
@@ -512,6 +512,7 @@ int forwdel(int, int);
int backdel(int, int);
int space_to_tabstop(int, int);
int backtoindent(int, int);
+int joinline(int, int);
/* extend.c X */
int insert(int, int);
diff -up src/usr.bin/mg.old/funmap.c src/usr.bin/mg/funmap.c
--- src/usr.bin/mg.old/funmap.c Fri Jan 14 17:27:17 2011
+++ src/usr.bin/mg/funmap.c Fri Jan 14 17:50:55 2011
@@ -103,6 +103,7 @@ static struct funmap functnames[] = {
{fillword, "insert-with-wrap",},
{backisearch, "isearch-backward",},
{forwisearch, "isearch-forward",},
+ {joinline, "join-line",},
{justone, "just-one-space",},
{ctrlg, "keyboard-quit",},
{killbuffer_cmd, "kill-buffer",},
diff -up src/usr.bin/mg.old/keymap.c src/usr.bin/mg/keymap.c
--- src/usr.bin/mg.old/keymap.c Fri Jan 14 18:41:34 2011
+++ src/usr.bin/mg/keymap.c Fri Jan 14 17:52:04 2011
@@ -228,7 +228,7 @@ static PF metasqf[] = {
NULL, /* [ */
delwhite, /* \ */
rescan, /* ] */
- rescan, /* ^ */
+ joinline, /* ^ */
rescan, /* _ */
rescan, /* ` */
rescan, /* a */
diff -up src/usr.bin/mg.old/mg.1 src/usr.bin/mg/mg.1
--- src/usr.bin/mg.old/mg.1 Fri Jan 14 17:27:17 2011
+++ src/usr.bin/mg/mg.1 Fri Jan 14 18:35:02 2011
@@ -220,6 +220,8 @@ beginning-of-buffer
end-of-buffer
.It M-\e
delete-horizontal-space
+.It M-^
+join-line
.It M-b
backward-word
.It M-c
@@ -510,6 +512,9 @@ Use incremental searching, initially in the forward di
isearch ignores any explicit arguments.
If invoked during macro definition or evaluation, the non-incremental
search-forward is invoked instead.
+.It join-line
+Join the current line to the previous. If called with an argument,
+join the next line to the current one.
.It just-one-space
Delete any whitespace around dot, then insert a space.
.It keyboard-quit
diff -up src/usr.bin/mg.old/random.c src/usr.bin/mg/random.c
--- src/usr.bin/mg.old/random.c Fri Jan 14 17:27:17 2011
+++ src/usr.bin/mg/random.c Fri Jan 14 18:45:16 2011
@@ -453,3 +453,31 @@ backtoindent(int f, int n)
++curwp->w_doto;
return (TRUE);
}
+
+/*
+ * Join the current line to the previous, or with arg, the next line
+ * to the current one. If the former line is not empty, leave exactly
+ * one space at the joint. Otherwise, leave no whitespace.
+ */
+int
+joinline(int f, int n)
+{
+ int doto;
+
+ if (f & FFARG) {
+ gotoeol(FFRAND, 1);
+ forwdel(FFRAND, 1);
+ } else {
+ gotobol(FFRAND, 1);
+ backdel(FFRAND, 1);
+ }
+
+ delwhite(FFRAND, 1);
+
+ if ((doto = curwp->w_doto) > 0) {
+ linsert(1, ' ');
+ curwp->w_doto = doto;
+ }
+
+ return (TRUE);
+}
Re: mg:join-line
> Looks pretty good. I might add an undo boundary
> around the whole thing (I note emacs doesn't do this
> properly, at least on the version I have here)...
I like it. If undo is a concern, then it might be a good idea to
check the other functions while we're here.
I found at least the following offenders in random.c:
twiddle() might return early, leaving the boundaries disabled;
openline() can open many at once, but each gets its own boundary;
justone() makes two changes but doesn't set the boundary;
lfindent() and newline() have the same problem as openline().
There's also indent() which makes two changes, but it's not bound
to a key and is only called via cmode, where the undo boundaries
are in place already.
I'm afraid simply adding the the undo boundary around newline()
will break yank(), which sets its own boundary and calls newline()
among other changes. If we want this undo stuff, then we probably
should add checks such that none of these functions set boundaries
if they were disabled (by some other function) to start with. What
do you think?
The following diff fixes twiddle() and adds boundaries for openline(),
justone(), and lfindent(). I leave newline() intact for now.
--- src/usr.bin/mg.old/random.c Mon Jan 17 15:16:09 2011
+++ src/usr.bin/mg/random.c Mon Jan 17 16:25:21 2011
@@ -119,7 +119,6 @@ twiddle(int f, int n)
dotp = curwp->w_dotp;
doto = curwp->w_doto;
- undo_boundary_enable(FFRAND, 0);
if (doto == llength(dotp)) {
if (--doto <= 0)
return (FALSE);
@@ -129,6 +128,7 @@ twiddle(int f, int n)
if (doto == 0)
return (FALSE);
}
+ undo_boundary_enable(FFRAND, 0);
cr = lgetc(dotp, doto - 1);
(void)backdel(FFRAND, 1);
(void)forwchar(FFRAND, 1);
@@ -158,6 +158,7 @@ openline(int f, int n)
return (TRUE);
/* insert newlines */
+ undo_boundary_enable(FFRAND, 0);
i = n;
do {
s = lnewline();
@@ -166,6 +167,7 @@ openline(int f, int n)
/* then go back up overtop of them all */
if (s == TRUE)
s = backchar(f | FFRAND, n);
+ undo_boundary_enable(FFRAND, 1);
return (s);
}
@@ -223,8 +225,11 @@ deblank(int f, int n)
int
justone(int f, int n)
{
+ undo_boundary_enable(FFRAND, 0);
(void)delwhite(f, n);
- return (linsert(1, ' '));
+ linsert(1, ' ');
+ undo_boundary_enable(FFRAND, 1);
+ return (TRUE);
}
/*
@@ -318,10 +323,12 @@ int
lfindent(int f, int n)
{
int c, i, nicol;
+ int s = TRUE;
if (n < 0)
return (FALSE);
+ undo_boundary_enable(FFRAND, 0);
while (n--) {
nicol = 0;
for (i = 0; i < llength(curwp->w_dotp); ++i) {
@@ -337,10 +344,13 @@ lfindent(int f, int n)
curbp->b_flag & BFNOTAB) ? linsert(nicol, ' ') == FALSE : (
#endif /* NOTAB */
((i = nicol / 8) != 0 && linsert(i, '\t') == FALSE) ||
- ((i = nicol % 8) != 0 && linsert(i, ' ') == FALSE
- return (FALSE);
+ ((i = nicol % 8) != 0 && linsert(i, ' ') == FALSE {
+ s = FALSE;
+ break;
+ }
}
- return (TRUE);
+ undo_boundary_enable(FFRAND, 1);
+ return (s);
}
/*
Re: mg: dirname, basename
> Comments?
Looks quite fine, but I noticed there are no NULL checks for the
newly allocated strings. Aborting a command gracefully might be
better than crashing, should anyone ever be unfortunate enough
to hit this.
Also:
> @@ -415,8 +421,11 @@
> ewprintf("Directory name too long");
> return (FALSE);
> }
> - if ((bufp = eread("Rename %s to: ", toname,
> - sizeof(toname), EFDEF | EFNEW | EFCR, basename(frname))) == NULL)
> + s = xbasename(frname);
> + bufp = eread("Rename %s to: ", toname,
> + sizeof(toname), EFDEF | EFNEW | EFCR, basename(frname));
Re: mg: dirname, basename
> Thus, I propose the following respin: implement xdirname and xbasename
> using a strlcat/strlcpy semantic. This is more natural in most calls anyway.
Good idea. Being used to the strlfoo functions, it looks a bit strange
to make xplen the middlemost argument, but I'm just being me now. :-)
> + if (*dp && dp[0] == '/' && dp[1] == '\0') {
Is the *dp check necessary? I realize it's like this in the old code
too, but it caught my attention now.
In any case, the diff looks good, and I found no problems while
testing it.
route: -iface vs. -interface
I noticed route treats the flags -iface and -interface as synonyms. Almost synonyms. This little diff makes monitor accept -interface as the other commands do. It also makes the manual consistent such that only -iface is used throughout it. Index: src/sbin/route/route.8 === RCS file: /cvs/src/sbin/route/route.8,v retrieving revision 1.66 diff -u -p -r1.66 route.8 --- src/sbin/route/route.8 21 Sep 2010 14:46:40 - 1.66 +++ src/sbin/route/route.8 24 Jan 2011 03:29:47 - @@ -271,7 +271,7 @@ or alternately If the destination is directly reachable via an interface requiring no intermediary system to act as a gateway, the -.Fl interface +.Fl iface modifier should be specified; the gateway given is the address of this host on the common network, indicating the interface to be used for transmission. Index: src/sbin/route/route.c === RCS file: /cvs/src/sbin/route/route.c,v retrieving revision 1.152 diff -u -p -r1.152 route.c --- src/sbin/route/route.c 25 Oct 2010 19:39:55 - 1.152 +++ src/sbin/route/route.c 24 Jan 2011 03:29:47 - @@ -1035,6 +1035,7 @@ monitor(int argc, char *argv[]) af = AF_INET6; break; case K_IFACE: + case K_INTERFACE: filter = ROUTE_FILTER(RTM_IFINFO) | ROUTE_FILTER(RTM_IFANNOUNCE); break;
-w for wsconsctl is not needed, update some manpages
As the subject says: the -w flag is no longer necessary (nor documented) for setting vars with wsconsctl, so don't advise people to use it. Index: src/share/man/man4/akbd.4 === RCS file: /cvs/src/share/man/man4/akbd.4,v retrieving revision 1.3 diff -u -p -r1.3 akbd.4 --- src/share/man/man4/akbd.4 31 May 2007 19:19:49 - 1.3 +++ src/share/man/man4/akbd.4 29 Jan 2011 23:18:09 - @@ -138,7 +138,7 @@ This switches off the To set a German keyboard layout without .Dq dead accents , use -.Ic wsconsctl -w keyboard.encoding=de.nodead . +.Ic wsconsctl keyboard.encoding=de.nodead . To set it at kernel build time, add the following to the kernel configuration file: .Bd -literal -offset indent Index: src/share/man/man4/hilkbd.4 === RCS file: /cvs/src/share/man/man4/hilkbd.4,v retrieving revision 1.12 diff -u -p -r1.12 hilkbd.4 --- src/share/man/man4/hilkbd.4 31 May 2007 19:19:50 - 1.12 +++ src/share/man/man4/hilkbd.4 29 Jan 2011 23:18:09 - @@ -87,7 +87,7 @@ This switches off the .Dq dead accents . .Sh EXAMPLES To set a Swedish keyboard mapping, use -.Ic wsconsctl -w keyboard.encoding=sv . +.Ic wsconsctl keyboard.encoding=sv . To set it at kernel build time, regardless of what keyboard is plugged, add the following to the kernel configuration file: .Bd -literal -offset indent Index: src/share/man/man4/pckbd.4 === RCS file: /cvs/src/share/man/man4/pckbd.4,v retrieving revision 1.37 diff -u -p -r1.37 pckbd.4 --- src/share/man/man4/pckbd.4 7 Dec 2009 19:24:01 - 1.37 +++ src/share/man/man4/pckbd.4 29 Jan 2011 23:18:09 - @@ -200,7 +200,7 @@ To set a German keyboard layout without .Dq dead accents and sending an ESC character before the key symbol if the ALT key is pressed simultaneously, use -.Ic wsconsctl -w keyboard.encoding=de.nodead.metaesc . +.Ic wsconsctl keyboard.encoding=de.nodead.metaesc . To set it at kernel build time, add the following to the kernel configuration file: .Bd -literal -offset indent Index: src/share/man/man4/ukbd.4 === RCS file: /cvs/src/share/man/man4/ukbd.4,v retrieving revision 1.19 diff -u -p -r1.19 ukbd.4 --- src/share/man/man4/ukbd.4 19 Sep 2010 12:52:43 - 1.19 +++ src/share/man/man4/ukbd.4 29 Jan 2011 23:18:09 - @@ -227,7 +227,7 @@ To set a German keyboard layout without .Dq dead accents and sending an ESC character before the key symbol if the ALT key is pressed simultaneously, use -.Ic wsconsctl -w keyboard.encoding=de.nodead.metaesc . +.Ic wsconsctl keyboard.encoding=de.nodead.metaesc . To set it at kernel build time, add the following to the kernel configuration file: .Bd -literal -offset indent Index: src/share/man/man4/man4.sparc/zs.4 === RCS file: /cvs/src/share/man/man4/man4.sparc/zs.4,v retrieving revision 1.23 diff -u -p -r1.23 zs.4 --- src/share/man/man4/man4.sparc/zs.4 10 Jul 2010 19:38:39 - 1.23 +++ src/share/man/man4/man4.sparc/zs.4 29 Jan 2011 23:18:10 - @@ -142,7 +142,7 @@ This switches off the .Dq dead accents . .Sh EXAMPLES To set a German keyboard layout, use -.Ic wsconsctl -w keyboard.encoding=de . +.Ic wsconsctl keyboard.encoding=de . To set it at kernel build time, add the following to the kernel configuration file for a type 4 keyboard: .Bd -literal -offset indent Index: src/share/man/man4/man4.sparc64/zs.4 === RCS file: /cvs/src/share/man/man4/man4.sparc64/zs.4,v retrieving revision 1.16 diff -u -p -r1.16 zs.4 --- src/share/man/man4/man4.sparc64/zs.420 May 2009 20:13:42 - 1.16 @@ -137,7 +137,7 @@ This switches off the .Dq dead accents . .Sh EXAMPLES To set a German keyboard layout, use -.Ic wsconsctl -w keyboard.encoding=de . +.Ic wsconsctl keyboard.encoding=de . To set it at kernel build time, add the following to the kernel configuration file for a type 4 keyboard: .Bd -literal -offset indent Index: src/share/man/man4/man4.vax/lkkbd.4 === RCS file: /cvs/src/share/man/man4/man4.vax/lkkbd.4,v retrieving revision 1.11 diff -u -p -r1.11 lkkbd.4 --- src/share/man/man4/man4.vax/lkkbd.4 31 May 2007 19:19:57 - 1.11 +++ src/share/man/man4/man4.vax/lkkbd.4 29 Jan 2011 23:18:10 - @@ -154,7 +154,7 @@ This switches off the .Dq dead accents . .Sh EXAMPLES To set a French keyboard layout, use -.Ic wsconsctl -w keyboard.encoding=fr . +.Ic wsconsctl keyboard.encoding=fr . To set it at kernel build time, add the following to the kernel configuration file: .Bd -literal -offset indent
