Re: [PATCH] add support for versions with '-' before a/b/rc
Hello Matija,
Could you please provide a testcase for inclusion in
src/regress/usr.bin/pkg-config too?
Also, is there a particular port or pkg-config file in the wild that you ran
into which exhibits this particular pattern?
Cheers,
Jasper
> On 12 Dec 2019, at 18:28, Matija Skala wrote:
>
> From fa66eb42d0bd2fec7b364644684e6a4cc9ae9baa Mon Sep 17 00:00:00 2001
> From: Matija Skala
> Date: Thu, 28 Nov 2019 19:24:42 +0100
> Subject: [PATCH] add support for versions with '-' before a/b/rc
>
> ---
> usr.bin/pkg-config/pkg-config | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/usr.bin/pkg-config/pkg-config b/usr.bin/pkg-config/pkg-config
> index 6dfbd3224eb..c050e9b058e 100644
> --- a/usr.bin/pkg-config/pkg-config
> +++ b/usr.bin/pkg-config/pkg-config
> @@ -674,13 +674,13 @@ sub compare
> # is there a valid non-numeric suffix to deal with later?
> # accepted are (in order): a(lpha) < b(eta) < rc < ' '.
> # suffix[0] is the 'alpha' part, suffix[1] is the '1' part in 'alpha1'.
> - if ($a =~ s/(rc|beta|b|alpha|a)(\d+)$//) {
> + if ($a =~ s/-?(rc|beta|b|alpha|a)(\d+)$//) {
> say_debug("valid suffix $1$2 found in $a$1$2.");
> $suffix_a[0] = $1;
> $suffix_a[1] = $2;
> }
>
> - if ($b =~ s/(rc|beta|b|alpha|a)(\d+)$//) {
> + if ($b =~ s/-?(rc|beta|b|alpha|a)(\d+)$//) {
> say_debug("valid suffix $1$2 found in $b$1$2.");
> $suffix_b[0] = $1;
> $suffix_b[1] = $2;
>
>
Re: vmm(4) question: unneeded vmclear() in vcpu_readregs_vmx()?
On Tue, Oct 22, 2019 at 06:57:32PM +0900, Iori YONEJI wrote: > On Tue, Oct 22, 2019 at 11:17 AM Mike Larkin wrote: > > > > On Mon, Oct 21, 2019 at 03:52:52AM +0900, Iori YONEJI wrote: > > > Hello tech@, > > > > > > I have a question (or maybe a suggestion) about vmm(4). > > > > > > I'm writing a small additional feature to sys/arch/amd64/amd64/vmm.c > > > and found a seemingly unneeded vmclear() at the end of > > > vcpu_readregs_vmx(). This function didn't seem to affect VM state at > > > first glance because, obviously, its name is _read_. Against this > > > intuition, this function clears VM state, which makes the vmexit > > > handling stop (eg. "advance rip" failure) and the VM die if it is used > > > in there. Possibly this was added as a counterpart of > > > vcpu_reload_vmcs_vmx() at the very beginning of the read function, but > > > I don't think it does undo the reload. > > > > > > This vmclear can be removed in my understanding and also I confirmed > > > that Alpine Linux and OpenBSD guest VMs can run on the vmm(4) without > > > this vmclear call. > > > > > > Or I am just wrong and it may be actually needed in some corner cases. > > > If so, I will restore (vmptrld) vcpu->vc_control_pa every time the VM > > > context continues after calling readregs function or add another flag > > > to indicate whether vmclear is needed here if all of the corner cases > > > are known. > > > > > > Would you mind to tell me I am correct or not, or how it has to be > > > dealt with? > > > > > > > > > Index: sys/arch/amd64/amd64/vmm.c > > > === > > > RCS file: /cvs/src/sys/arch/amd64/amd64/vmm.c,v > > > retrieving revision 1.254 > > > diff -u -p -r1.254 vmm.c > > > --- sys/arch/amd64/amd64/vmm.c22 Sep 2019 08:47:54 -1.254 > > > +++ sys/arch/amd64/amd64/vmm.c20 Oct 2019 18:04:17 - > > > @@ -1602,8 +1602,10 @@ vcpu_readregs_vmx(struct vcpu *vcpu, uin > > > errout: > > > ret = EINVAL; > > > out: > > > +/* XXX Why do we need vmclear here? > > > if (vmclear(>vc_control_pa)) > > > ret = EINVAL; > > > +*/ > > > return (ret); > > > } > > > > > > > The assumption in vcpu_run_vmx is that we will always come in with a cleared > > VMCS state, unless we are staying in the run loop (the "resume" flag in that > > function tracks this). So any ioctl (including things like read/write regs) > > that might possibly manipulate the VMCS, need to clear the VMCS before they > > return to user space. That was the original reason for that clear at the end > > of the function. > > > > If you are adding new functionality that uses read/write regs internally, > > then > > you're right; on return, you'll have a cleared VMCS and subsequent vmreads > > and > > vmwrites will fail. > > > > It looks like there are a couple of other users of readregs/writeregs > > internally - the presently unused GVA translator function as well as the > > in/out > > exit handler structure preparation code. Both of those look like they got > > lucky > > in how they were handling things; they aren't touching the VMCS after > > read/write > > regs or we would have seen them explode like you're saying. > > > > It looks like the start of vcpu_run_vmx *almost* properly handles coming > > in with an arbitrary VMCS. The "almost" part is the bit under #ifdef > > VMM_DEBUG > > that handles triple faults. It seems to assume that the proper VMCS is > > currently > > loaded, which is certainly not true all the time. I'll fix that part. > > > > Based on the fact that the other code paths look clean, I think we can > > remove > > the vmclear from the end of read/write regs, as you suggest. I'll do that as > > well. > > > > Might I ask what you're working on? Just in case I know of someone else > > working > > on the same thing? (There are about a dozen vmm side-projects going on that > > I'm > > aware of and it would be a waste for two people to be working on the same > > thing > > without knowing of the other). Feel free to mail me off-list if you don't > > want > > to share in public. > > > > -ml > > > Thank you. I didn't notice that the VMCS must be cleared before > returning to user space. > > Talking about the use of this function from outside of the run loop, I > worry about that it would overwrite VMCS memory when another physical > CPU that is running the vcpu's run loop has exited and written back its > new state just before read regs clears its VMCS. > If those control_pa(s) always point different memory addresses, this > will never happen. Otherwise, I think it would (very unlikely) happen. > > And thank you to remove the vmclear. > > > Might I ask what you're working on? > I am working on adding MMIO handling. > > Originally, I encountered very same situation as this report: > Booting NetBSD on vmm fails with uvm_fault > https://marc.info/?l=openbsd-bugs=153761926725642=2 > > The address 0xfed40014 is the TPM capability register as
Add sizes for free() in auglx(4)
Hi tech@,
Here is a diff to add sizes for free() in auglx(4).
Similar diff to the ones commited for auvia(4) and autri(4).
Comments? OK?
Index: sys/dev/pci/auglx.c
===
RCS file: /cvs/src/sys/dev/pci/auglx.c,v
retrieving revision 1.16
diff -u -p -r1.16 auglx.c
--- sys/dev/pci/auglx.c 20 Dec 2016 15:45:29 - 1.16
+++ sys/dev/pci/auglx.c 12 Dec 2019 21:38:51 -
@@ -587,7 +587,7 @@ auglx_allocm(void *v, int direction, siz
error = auglx_allocmem(sc, size, PAGE_SIZE, p);
if (error) {
- free(p, pool, 0);
+ free(p, pool, sizeof(*p));
return NULL;
}
@@ -608,7 +608,7 @@ auglx_freem(void *v, void *ptr, int pool
if (p->addr == ptr) {
auglx_freemem(sc, p);
*pp = p->next;
- free(p, pool, 0);
+ free(p, pool, sizeof(*p));
return;
}
}
Add sizes for free() in auacer(4)
Hi tech@,
Here is a diff to add sizes for free() in auacer(4).
Similar diff to the ones commited for auvia(4) and autri(4).
Comments? OK?
Index: sys/dev/pci/auacer.c
===
RCS file: /cvs/src/sys/dev/pci/auacer.c,v
retrieving revision 1.21
diff -u -p -r1.21 auacer.c
--- sys/dev/pci/auacer.c12 Dec 2016 06:43:01 - 1.21
+++ sys/dev/pci/auacer.c12 Dec 2019 21:38:59 -
@@ -603,7 +603,7 @@ auacer_allocm(void *v, int direction, si
error = auacer_allocmem(sc, size, PAGE_SIZE, p);
if (error) {
- free(p, pool, 0);
+ free(p, pool, sizeof(*p));
return (NULL);
}
@@ -623,7 +623,7 @@ auacer_freem(void *v, void *ptr, int poo
if (KERNADDR(p) == ptr) {
auacer_freemem(sc, p);
*pp = p->next;
- free(p, pool, 0);
+ free(p, pool, sizeof(*p));
return;
}
}
Re: pfctl: Do not optimize empty rulesets
Klemens Nanni writes:
> On Wed, Nov 27, 2019 at 08:04:47PM +0100, Klemens Nanni wrote:
>> If an anchor/ruleset contains no rules, there is no point in creating
>> a temporary copy, optimizing and replacing it.
>>
>> Regress passes on amd64.
>>
>> Feedback? OK?
> Anyone?
>
FWIW, it looks good to me. Ok mikeb
> All optimizations work on actual rules; if there are none, we don't
> need to look further, especially not in "profile" mode where existing
> rules are read from the kernel as feedback: an empty ruleset will stay
> empty after optimization is done.
>
> This also does not affect `set' or `table' lines in any way, e.g.
>
> # echo 'table ' | pfctl -o basic -d -nf-
>
> still is an empty ruleset.
>
>
> I came across when debugging anchors, but with -DOPT_DEBUG as well this
> time where `-d' output for multiple anchors wouldn't really be helpful:
>
> $ pfctl -dnf test.pf
> pfctl_optimize_ruleset: optimizing ruleset
> pfctl_optimize_ruleset: optimizing ruleset
> pfctl_optimize_ruleset: optimizing ruleset
>
> So below is an updated diff that also prints the anchor path, letting
> developers know which anchor is being optimized in wha order:
>
> pfctl_optimize_ruleset: optimizing ruleset ""
> pfctl_optimize_ruleset: optimizing ruleset "a1"
> pfctl_optimize_ruleset: optimizing ruleset "_1/a2"
>
> Yes, the main anchor prints as "" but all that is behind compile time
> -DOPT_DEBUG so regular users won't deal with it anyway, so keep the code
> simple instead of adding logging around `rs->anchor->path'.
>
> OK?
>
>
> Index: pfctl_optimize.c
> ===
> RCS file: /cvs/src/sbin/pfctl/pfctl_optimize.c,v
> retrieving revision 1.42
> diff -u -p -r1.42 pfctl_optimize.c
> --- pfctl_optimize.c 28 Jun 2019 13:32:45 - 1.42
> +++ pfctl_optimize.c 12 Dec 2019 20:06:15 -
> @@ -270,7 +270,10 @@ pfctl_optimize_ruleset(struct pfctl *pf,
> struct pf_rule *r;
> struct pf_rulequeue *old_rules;
>
> - DEBUG("optimizing ruleset");
> + if (TAILQ_EMPTY(rs->rules.active.ptr))
> + return (0);
> +
> + DEBUG("optimizing ruleset \"%s\"", rs->anchor->path);
> memset(_buffer, 0, sizeof(table_buffer));
> skip_init();
> TAILQ_INIT(_queue);
Re: pfctl: Do not optimize empty rulesets
On Wed, Nov 27, 2019 at 08:04:47PM +0100, Klemens Nanni wrote:
> If an anchor/ruleset contains no rules, there is no point in creating
> a temporary copy, optimizing and replacing it.
>
> Regress passes on amd64.
>
> Feedback? OK?
Anyone?
All optimizations work on actual rules; if there are none, we don't
need to look further, especially not in "profile" mode where existing
rules are read from the kernel as feedback: an empty ruleset will stay
empty after optimization is done.
This also does not affect `set' or `table' lines in any way, e.g.
# echo 'table ' | pfctl -o basic -d -nf-
still is an empty ruleset.
I came across when debugging anchors, but with -DOPT_DEBUG as well this
time where `-d' output for multiple anchors wouldn't really be helpful:
$ pfctl -dnf test.pf
pfctl_optimize_ruleset: optimizing ruleset
pfctl_optimize_ruleset: optimizing ruleset
pfctl_optimize_ruleset: optimizing ruleset
So below is an updated diff that also prints the anchor path, letting
developers know which anchor is being optimized in wha order:
pfctl_optimize_ruleset: optimizing ruleset ""
pfctl_optimize_ruleset: optimizing ruleset "a1"
pfctl_optimize_ruleset: optimizing ruleset "_1/a2"
Yes, the main anchor prints as "" but all that is behind compile time
-DOPT_DEBUG so regular users won't deal with it anyway, so keep the code
simple instead of adding logging around `rs->anchor->path'.
OK?
Index: pfctl_optimize.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_optimize.c,v
retrieving revision 1.42
diff -u -p -r1.42 pfctl_optimize.c
--- pfctl_optimize.c28 Jun 2019 13:32:45 - 1.42
+++ pfctl_optimize.c12 Dec 2019 20:06:15 -
@@ -270,7 +270,10 @@ pfctl_optimize_ruleset(struct pfctl *pf,
struct pf_rule *r;
struct pf_rulequeue *old_rules;
- DEBUG("optimizing ruleset");
+ if (TAILQ_EMPTY(rs->rules.active.ptr))
+ return (0);
+
+ DEBUG("optimizing ruleset \"%s\"", rs->anchor->path);
memset(_buffer, 0, sizeof(table_buffer));
skip_init();
TAILQ_INIT(_queue);
Re: vm.conf: owner: do not default to root
On Thu, Dec 12, 2019 at 03:03:19PM +0100, Denis Fondras wrote:
> Perhaps vm.conf(5) manual should tell that root:wheel is the default.
For `socket owner' vm.conf(5) already says that, but for actual VMs this
only works outside of templates, ie. the following does not work:
# cat /etc/vm.conf
vm "generic" {
disable
disk "/dev/null"
owner ":wheel"
allow instance {
boot,
disk,
cdrom,
interface,
memory
}
}
# rcctl restart vmd
vmd(ok)
vmd(ok)
# vmctl status
ID PID VCPUS MAXMEM CURMEM TTYOWNERSTATE NAME
1 - 1512M - - :wheel stopped generic
# su -l kn
$ groups
kn wheel wsrc auth wobj
$ vmctl start
$ vmctl start -t generic -b ~/bsd.rd -c test
vmctl: start vm command failed: Operation not permitted
According to documentation, this should work; if at all, I'd like to
amend the manual page when fixing this.
Note that with `owner "kn"' instead, above example does work.
Error in rpki-client
Hi, Here we want the BIRD option, not to change the bind address: --- rpki-client.8.orig 2019-12-12 19:32:25.252139648 +0100 +++ rpki-client.8 2019-12-12 19:32:47.792244479 +0100 @@ -91,7 +91,7 @@ .Xr bgpd 8 compatible input. If the -.Fl b , +.Fl B , .Fl c , and .Fl j Regards, -- Alarig
Re: Question(s) about HID devices
Brad DeMorrow wrote:
> I'm trying to add support for Xbox One Controllers to OpenBSD.
>
> It looks as though I can fairly easily get the device to attach as a uhid
> device, but I believe the device requires some special initialization data
> sent to it before it will actually begin functioning.
>
> That being said, I'm pretty sure that the answer is not to continue adding
> special cases within uhidev.c - it feels weird already that there is Xbox
> 360 controller code inside there to me, to be honest.
>
> My question is this: If I create a new driver that attaches after uhidev,
> will software expecting a standard HID gamepad work, or will they not work
> because they haven't been taught about my new, special driver?
>
> If a new driver isn't the answer, and it must remain a uhidev device - is
> there a standard approach to something like this? Should I just break out
> the Xbox Controller code into a new file and call out to it from the
> uhidev.{match,attach,detach,etc..}?
>
> Should Gamepads be exposed via wsmux like keyboards and mice instead?
there is an extended discussion happening elsewhere about how the ugen
and uhid 'generic' interfaces are very unsafe. If a user wants to use
one specific device, (since everyone expects usb port agility) they must
chmod a+rx /dev/uhid* or /dev/ugen*, depending on the usb device's
capability.
But this means any softwre on the machine may open and talk to any and
all usb devices without drivers 'in-the-raw', creating side effects.
There are a lot of downsides.
My strong preference is that we encourage people to write proper kernel
device drivers, and thereby reduce the incentives which prompt people to
chmod a+rw all the unknown devices on their usb busses.
It is a mess.
[PATCH] add support for versions with '-' before a/b/rc
From fa66eb42d0bd2fec7b364644684e6a4cc9ae9baa Mon Sep 17 00:00:00 2001
From: Matija Skala
Date: Thu, 28 Nov 2019 19:24:42 +0100
Subject: [PATCH] add support for versions with '-' before a/b/rc
---
usr.bin/pkg-config/pkg-config | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/usr.bin/pkg-config/pkg-config b/usr.bin/pkg-config/pkg-config
index 6dfbd3224eb..c050e9b058e 100644
--- a/usr.bin/pkg-config/pkg-config
+++ b/usr.bin/pkg-config/pkg-config
@@ -674,13 +674,13 @@ sub compare
# is there a valid non-numeric suffix to deal with later?
# accepted are (in order): a(lpha) < b(eta) < rc < ' '.
# suffix[0] is the 'alpha' part, suffix[1] is the '1' part in 'alpha1'.
- if ($a =~ s/(rc|beta|b|alpha|a)(\d+)$//) {
+ if ($a =~ s/-?(rc|beta|b|alpha|a)(\d+)$//) {
say_debug("valid suffix $1$2 found in $a$1$2.");
$suffix_a[0] = $1;
$suffix_a[1] = $2;
}
- if ($b =~ s/(rc|beta|b|alpha|a)(\d+)$//) {
+ if ($b =~ s/-?(rc|beta|b|alpha|a)(\d+)$//) {
say_debug("valid suffix $1$2 found in $b$1$2.");
$suffix_b[0] = $1;
$suffix_b[1] = $2;
Question(s) about HID devices
I'm trying to add support for Xbox One Controllers to OpenBSD.
It looks as though I can fairly easily get the device to attach as a uhid
device, but I believe the device requires some special initialization data
sent to it before it will actually begin functioning.
That being said, I'm pretty sure that the answer is not to continue adding
special cases within uhidev.c - it feels weird already that there is Xbox
360 controller code inside there to me, to be honest.
My question is this: If I create a new driver that attaches after uhidev,
will software expecting a standard HID gamepad work, or will they not work
because they haven't been taught about my new, special driver?
If a new driver isn't the answer, and it must remain a uhidev device - is
there a standard approach to something like this? Should I just break out
the Xbox Controller code into a new file and call out to it from the
uhidev.{match,attach,detach,etc..}?
Should Gamepads be exposed via wsmux like keyboards and mice instead?
Thanks,
Re: vm.conf: owner: do not default to root
On Thu, Dec 12, 2019 at 12:52:34PM +0100, Klemens Nanni wrote:
> On Thu, Dec 05, 2019 at 08:46:01PM +0100, Klemens Nanni wrote:
> > vm.conf(5) states it must be `owner user[:group]' or `owner group', not
> > specifying a value is undocumented and ought to be invalid syntax, yet
> > `owner' is treated as `owner root' which is the same as simply omitting
> > the owner line.
> >
> > Diff below causes the following behaviour change:
> >
> > $ cat socket-owner.conf
> > socket owner
> > $ vmd -nf socket-owner.conf
> > configuration OK
> > $ ./obj/vmd -nf socket-owner.conf
> > socket-owner.conf:1: syntax error
> >
> > $ cat vm-owner.conf
> > vm v {
> > disk /dev/null
> > owner
> > }
> > $ vmd -nf vm-owner.conf
> > configuration OK
> > $ ./obj/vmd -nf vm-owner.conf
> > vm-owner.conf:3: syntax error
> >
> > Feedback? OK?
> Ping.
>
Seems legit, OK denis@
Perhaps vm.conf(5) manual should tell that root:wheel is the default.
>
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/vmd/parse.y,v
> retrieving revision 1.53
> diff -u -p -r1.53 parse.y
> --- parse.y 12 Dec 2019 03:53:38 - 1.53
> +++ parse.y 12 Dec 2019 11:51:51 -
> @@ -553,11 +553,7 @@ instance_flags : BOOT { vmc.vmc_insflag
> }
> ;
>
> -owner_id : /* none */{
> - $$.uid = 0;
> - $$.gid = -1;
> - }
> - | NUMBER{
> +owner_id : NUMBER{
> $$.uid = $1;
> $$.gid = -1;
> }
>
Re: massage tcpdump ip and encapsulation output
so this should go in and i can keep improving things in the tree? dlg > On 6 Dec 2019, at 9:45 pm, Claudio Jeker wrote: > > On Fri, Dec 06, 2019 at 12:16:09PM +0100, Sebastian Benoit wrote: >> David Gwynne(da...@gwynne.id.au) on 2019.12.06 15:14:42 +1000: >>> >>> On 5 Dec 2019, at 21:14, Sebastian Benoit wrote: Claudio Jeker(cje...@diehard.n-r-g.com) on 2019.12.05 09:53:49 +0100: > I would suggest to just pack most of the headers into one group of (). > > IPv4 ttl 1 [tos 0x20] 10.0.127.15 > 10.0.127.1 > would become > IPv4 (ttl 1 tos 0x20) 10.0.127.15 > 10.0.127.1 > and > IPv4 ttl 1 [tos 0x20] (id 39958, len 84) 10.0.127.15 > 10.0.127.1 > would become > IPv4 (ttl 1 tos 0x20 id 39958 len 84) 10.0.127.15 > 10.0.127.1 > > Maybe add the commas if that is easy to do. its more readable with commas, i think >>> >>> do you want me to come up with something in this space as part of the >>> large diff, or is the large change generally ok and we can tinker with >>> this stuff afterward? >> >> It was just a comment on the readability of lists like that. >> I like your idea, please proceed whichever way you like. >> >>> >>> there's some concern that what i'm proposing is too radical and will break >>> peoples muscle memory. > > The output of tcpdump depends on the version and OS it is used on. > IMO the important bits that people normally scan for are the IPs, port > numbers, some of the TCP seq numbers or similar protocol specific data. > To make this scanning easier I suggested to reduce the line noise of the > IP header by reducing the amount of different () and [] sequences giving > the eye a way to skip over that chunk quickly. > > I think the new format is better and people need to retrain a bit but > again we should not make it harder den necessary. > > For me your work can go in as long as the further improvements as > discussed here follow. > -- > :wq Claudio
Re: vm.conf: owner: do not default to root
On Thu, Dec 05, 2019 at 08:46:01PM +0100, Klemens Nanni wrote:
> vm.conf(5) states it must be `owner user[:group]' or `owner group', not
> specifying a value is undocumented and ought to be invalid syntax, yet
> `owner' is treated as `owner root' which is the same as simply omitting
> the owner line.
>
> Diff below causes the following behaviour change:
>
> $ cat socket-owner.conf
> socket owner
> $ vmd -nf socket-owner.conf
> configuration OK
> $ ./obj/vmd -nf socket-owner.conf
> socket-owner.conf:1: syntax error
>
> $ cat vm-owner.conf
> vm v {
> disk /dev/null
> owner
> }
> $ vmd -nf vm-owner.conf
> configuration OK
> $ ./obj/vmd -nf vm-owner.conf
> vm-owner.conf:3: syntax error
>
> Feedback? OK?
Ping.
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/vmd/parse.y,v
retrieving revision 1.53
diff -u -p -r1.53 parse.y
--- parse.y 12 Dec 2019 03:53:38 - 1.53
+++ parse.y 12 Dec 2019 11:51:51 -
@@ -553,11 +553,7 @@ instance_flags : BOOT { vmc.vmc_insflag
}
;
-owner_id : /* none */{
- $$.uid = 0;
- $$.gid = -1;
- }
- | NUMBER{
+owner_id : NUMBER{
$$.uid = $1;
$$.gid = -1;
}
Re: patch: make sdiff tty-aware
On 2019/12/12 10:47, Marc Espie wrote: > Well, in the sysmerge use-case, you see the first line showing the > $OpenBSD$ prompt usually. That's often when you realize your terminal is > really small, and you will miss the next lines. The $OpenBSD$ line is > reasonably unambiguous, as the rev number is fairly prominent. I run into similar probably at least half the time when I use sdiff. I don't find it too bad with sysmerge - the merge is usually simple for an individual file, and sysmerge itself reacts when the width is changed between files. (Changing sdiff to fetch the width at startup would have no benefit for the sysmerge case). The problem I have is when running sdiff by hand - I do this fairly often when merging in changes to more complex port config files (postgresql, dovecot, php, etc) - if it isn't apparent that a wider terminal is needed until partway through the file, currently or with "fetch at startup" you'd have to exit and redo the work. On 2019/12/12 03:05, Theo de Raadt wrote: > Rather than writing completely new code from scratch with it's own quirks, > find and copy other utilities which already does it fully, such as > ls, ps, sed, column, w, etc Despite not being full-screen, sdiff is fairly long-running and reacts to user input, from a user's perspective it seems that it should work more like top than ls.
Re: patch: make sdiff tty-aware
I think the diff should do only one thing:
-w width
Print a maximum of width characters on each line. The default is
130 characters.
At startup, do the ioctl to get the width. If -w is specified, it
overrides thae value.
And delete the sentence about 130 from the manual page.
Rather than writing completely new code from scratch with it's own quirks,
find and copy other utilities which already does it fully, such as
ls, ps, sed, column, w, etc
There's no need to be unique snowflake with subtly different behaviour.
It would be a better world if they all behaved precisely the same rather
than some hodgepodge of new code doing it in yet another subtly
different way. The majority of them evaluate the width choices in this
way, and it would be great if there was further convergence towards
this, instead of needless divergence.
if ((p = getenv("COLUMNS")) != NULL)
termwidth = strtonum(p, 1, INT_MAX, NULL);
if (termwidth == 0 && ioctl(STDOUT_FILENO, TIOCGWINSZ, ) == 0 &&
win.ws_col > 0)
termwidth = win.ws_col;
if (termwidth == 0)
termwidth = 80;
Re: patch: make sdiff tty-aware
On Thu, Dec 12, 2019 at 02:51:18AM -0700, Theo de Raadt wrote: > Marc Espie wrote: > > > On Wed, Dec 11, 2019 at 01:15:40PM -0700, Theo de Raadt wrote: > > > I'm not sure I understand the goal of the signal handler. > > > > > > sdiff is moving forward through the file, only. If you are in a pager, > > > you want to increase the width for the later output not yet visible? > > > the normal way one does that in programs which don't backtrack and > > > re-output, is by restarting the program. > > > > > > You don't want a mix of shorter, then longer. If you are using a pager, > > > I suspect at least one of the next lines is already be buffered out. So > > > I don't understand how this will create output which looks correct. > > > > > > I think you should remove the signal handler. > > > > Well, in the sysmerge use-case, you see the first line showing the > > $OpenBSD$ prompt usually. That's often when you realize your terminal is > > really small, and you will miss the next lines. The $OpenBSD$ line is > > reasonably unambiguous, as the rev number is fairly prominent. > > > > The next ones will need the space. > > > > Admittedly, it could be smarter, and react during actual user interaction, > > or offer to redraw the last block with a specific command ? > > That you have to dig for such a narrow example really demonstrates > the pointlessness of the signal handler. I didn't have to dig... it's the specific usecase I have for sdiff3!
Re: patch: make sdiff tty-aware
Marc Espie wrote: > On Wed, Dec 11, 2019 at 01:15:40PM -0700, Theo de Raadt wrote: > > I'm not sure I understand the goal of the signal handler. > > > > sdiff is moving forward through the file, only. If you are in a pager, > > you want to increase the width for the later output not yet visible? > > the normal way one does that in programs which don't backtrack and > > re-output, is by restarting the program. > > > > You don't want a mix of shorter, then longer. If you are using a pager, > > I suspect at least one of the next lines is already be buffered out. So > > I don't understand how this will create output which looks correct. > > > > I think you should remove the signal handler. > > Well, in the sysmerge use-case, you see the first line showing the > $OpenBSD$ prompt usually. That's often when you realize your terminal is > really small, and you will miss the next lines. The $OpenBSD$ line is > reasonably unambiguous, as the rev number is fairly prominent. > > The next ones will need the space. > > Admittedly, it could be smarter, and react during actual user interaction, > or offer to redraw the last block with a specific command ? That you have to dig for such a narrow example really demonstrates the pointlessness of the signal handler.
Re: patch: make sdiff tty-aware
On Wed, Dec 11, 2019 at 01:15:40PM -0700, Theo de Raadt wrote: > I'm not sure I understand the goal of the signal handler. > > sdiff is moving forward through the file, only. If you are in a pager, > you want to increase the width for the later output not yet visible? > the normal way one does that in programs which don't backtrack and > re-output, is by restarting the program. > > You don't want a mix of shorter, then longer. If you are using a pager, > I suspect at least one of the next lines is already be buffered out. So > I don't understand how this will create output which looks correct. > > I think you should remove the signal handler. Well, in the sysmerge use-case, you see the first line showing the $OpenBSD$ prompt usually. That's often when you realize your terminal is really small, and you will miss the next lines. The $OpenBSD$ line is reasonably unambiguous, as the rev number is fairly prominent. The next ones will need the space. Admittedly, it could be smarter, and react during actual user interaction, or offer to redraw the last block with a specific command ?
