Re: [xenocara] xenodm.man fix
On Fri, Feb 17, 2023 at 11:52:44AM +, Laurence Tratt wrote: > On Thu, Feb 16, 2023 at 09:29:53PM +0300, Mikhail wrote: > > Hello Mikhail, > > > /etc/X11/xenodm/Xsession file has a check for x bit > > Yes, this one caught me out a few years back: > > https://marc.info/?l=openbsd-bugs=162737223625768=2 > > In subsequent discussions with Matthieu and Theo, I *think* the eventual > thought was that it would be better to get rid of the `-x` check. I might be > misremembering that, though. > I'm fine with not runing the script if not executable. I don't remember exactly why I didn't do it when you brought the issue up in 2021. But I prefer to use the fallback session when the script exists and isn't executable rather than letting the session fail immediatly. ok? Index: config/Xsession.in === RCS file: /local/cvs/xenocara/app/xenodm/config/Xsession.in,v retrieving revision 1.2 diff -u -p -u -r1.2 Xsession.in --- config/Xsession.in 1 Jul 2022 20:42:06 - 1.2 +++ config/Xsession.in 18 Feb 2023 06:56:22 - @@ -58,12 +58,8 @@ esac startup=$HOME/.xsession resources=$HOME/.Xresources -if [ -s "$startup" ]; then - if [ -x "$startup" ]; then - "$startup" - else - /bin/sh "$startup" - fi +if [ -s "$startup" -a -x "$startup" ]; then + "$startup" else if [ -f "$resources" ]; then @XRDB_PROGRAM@ -load "$resources" -- Matthieu Herrb
pinsyscall() and ld.so
ld.so now calls pinsyscall(2) pinsyscall(2) is a new system call added to the kernel a few days earlier. So if you are building from source, the usual rule applies strongly: Install a new kernel before doing a build
Potentially unfinished fixes to uvm_share() in uvm_map.c
There was a diff a while back (4 Dec 2019) that fixed a bad offset calculation in this function, but it seems to me that it made a couple other lines of code incorrect as a result. The diff below tweaks the lines so that they have their original intent (based on looking at the diff from when the function was introduced on 5 Jun 2016). diff --git a/sys/uvm/uvm_map.c b/sys/uvm/uvm_map.c index 9ee15eaab94..6507fa66f65 100644 --- a/sys/uvm/uvm_map.c +++ b/sys/uvm/uvm_map.c @@ -3567,7 +3567,7 @@ uvm_share(struct vm_map *dstmap, vaddr_t dstaddr, vm_prot_t prot, /* hole in address space, bail out */ if (psrc_entry != NULL && psrc_entry->end != src_entry->start) break; - if (src_entry->start >= srcaddr + sz) + if (src_entry->start >= srcaddr + n) break; if (UVM_ET_ISSUBMAP(src_entry)) @@ -3603,7 +3603,7 @@ uvm_share(struct vm_map *dstmap, vaddr_t dstaddr, vm_prot_t prot, n -= len; dstva += len; srcaddr += len; - unmap_end = dstva + len; + unmap_end = dstva; if (n == 0) goto exit_unlock; }
Re: acpithinkpad: revert 1.67
> From: Dave Voutila
> Date: Fri, 17 Feb 2023 16:30:46 -0500
>
> joshua stein writes:
>
> > Revision 1.67 made acpithinkpad not take over screen backlight
> > control and let inteldrm or some other driver handle it. This was
> > done to support fine-grained levels of backlight control rather than
> > the dozen steps that the native EC interface supports.
> >
> > Unfortunately this has the drawback of the EC getting confused about
> > what the backlight level actually is and it still tries to change
> > the backlight automatically in response to Fn+F# keys or plug/unplug
> > events. Notably if you have the laptop plugged in with a backlight
> > value of something high, then unplug it and later lower it with
> > xbacklight, when you plug it back in, the EC automatically adjusts
> > the backlight up to that high value.
> >
> > I don't know how to tell the EC to stop adjusting the backlight
> > automatically on power changes, so I would like to revert this
> > change and go back to only having native EC-controlled backlight
> > with xbacklight or wsconsctl.
> >
>
> Odd...this backout makes my X1 Carbon gen10 lost the ability to dim the
> screen to the point of being off. xbacklight and wsconsctl also don't
> stay in sync.
>
> Without the backout (i.e. current tree), it behaves perfectly fine with
> both xbacklight and wsconsctl staying in sync and being able to go fully
> off at 0.0%.
>
> This machine reports v2.0 btw:
>
> acpithinkpad0 at acpi0: version 2.0
Yes, the firmware-based backlight control stuff is broken on many of
the more recent ThinkPad models. And I believe it is no longer used
by modern versions Windows. The limited number of levels supported by
the firmware-based method certainly wasn't the most important reason
to stop using it.
I don't think this backout should go in.
> > patrick@ confirmed that he can still change backlight values on the
> > ThinkPad X395 with this backed out, even though part of the reason
> > for this initial change was to help that machine.
> >
> >
> > Index: acpithinkpad.c
> > ===
> > RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
> > retrieving revision 1.70
> > diff -u -p -u -p -r1.70 acpithinkpad.c
> > --- acpithinkpad.c 6 Apr 2022 18:59:27 - 1.70
> > +++ acpithinkpad.c 17 Feb 2023 14:41:26 -
> > @@ -321,10 +321,8 @@ thinkpad_attach(struct device *parent, s
> > wskbd_set_backlight = thinkpad_set_kbd_backlight;
> > }
> >
> > - /* On version 2 and newer, let *drm or acpivout control brightness */
> > - if (sc->sc_hkey_version == THINKPAD_HKEY_VERSION1 &&
> > - (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
> > - 0, NULL, >sc_brightness) == 0)) {
> > + if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
> > + 0, NULL, >sc_brightness) == 0) {
> > ws_get_param = thinkpad_get_param;
> > ws_set_param = thinkpad_set_param;
> > }
>
>
Re: acpithinkpad: revert 1.67
joshua stein writes:
> Revision 1.67 made acpithinkpad not take over screen backlight
> control and let inteldrm or some other driver handle it. This was
> done to support fine-grained levels of backlight control rather than
> the dozen steps that the native EC interface supports.
>
> Unfortunately this has the drawback of the EC getting confused about
> what the backlight level actually is and it still tries to change
> the backlight automatically in response to Fn+F# keys or plug/unplug
> events. Notably if you have the laptop plugged in with a backlight
> value of something high, then unplug it and later lower it with
> xbacklight, when you plug it back in, the EC automatically adjusts
> the backlight up to that high value.
>
> I don't know how to tell the EC to stop adjusting the backlight
> automatically on power changes, so I would like to revert this
> change and go back to only having native EC-controlled backlight
> with xbacklight or wsconsctl.
>
Odd...this backout makes my X1 Carbon gen10 lost the ability to dim the
screen to the point of being off. xbacklight and wsconsctl also don't
stay in sync.
Without the backout (i.e. current tree), it behaves perfectly fine with
both xbacklight and wsconsctl staying in sync and being able to go fully
off at 0.0%.
This machine reports v2.0 btw:
acpithinkpad0 at acpi0: version 2.0
-dv
> patrick@ confirmed that he can still change backlight values on the
> ThinkPad X395 with this backed out, even though part of the reason
> for this initial change was to help that machine.
>
>
> Index: acpithinkpad.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
> retrieving revision 1.70
> diff -u -p -u -p -r1.70 acpithinkpad.c
> --- acpithinkpad.c6 Apr 2022 18:59:27 - 1.70
> +++ acpithinkpad.c17 Feb 2023 14:41:26 -
> @@ -321,10 +321,8 @@ thinkpad_attach(struct device *parent, s
> wskbd_set_backlight = thinkpad_set_kbd_backlight;
> }
>
> - /* On version 2 and newer, let *drm or acpivout control brightness */
> - if (sc->sc_hkey_version == THINKPAD_HKEY_VERSION1 &&
> - (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
> - 0, NULL, >sc_brightness) == 0)) {
> + if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
> + 0, NULL, >sc_brightness) == 0) {
> ws_get_param = thinkpad_get_param;
> ws_set_param = thinkpad_set_param;
> }
acpithinkpad: revert 1.67
Revision 1.67 made acpithinkpad not take over screen backlight
control and let inteldrm or some other driver handle it. This was
done to support fine-grained levels of backlight control rather than
the dozen steps that the native EC interface supports.
Unfortunately this has the drawback of the EC getting confused about
what the backlight level actually is and it still tries to change
the backlight automatically in response to Fn+F# keys or plug/unplug
events. Notably if you have the laptop plugged in with a backlight
value of something high, then unplug it and later lower it with
xbacklight, when you plug it back in, the EC automatically adjusts
the backlight up to that high value.
I don't know how to tell the EC to stop adjusting the backlight
automatically on power changes, so I would like to revert this
change and go back to only having native EC-controlled backlight
with xbacklight or wsconsctl.
patrick@ confirmed that he can still change backlight values on the
ThinkPad X395 with this backed out, even though part of the reason
for this initial change was to help that machine.
Index: acpithinkpad.c
===
RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
retrieving revision 1.70
diff -u -p -u -p -r1.70 acpithinkpad.c
--- acpithinkpad.c 6 Apr 2022 18:59:27 - 1.70
+++ acpithinkpad.c 17 Feb 2023 14:41:26 -
@@ -321,10 +321,8 @@ thinkpad_attach(struct device *parent, s
wskbd_set_backlight = thinkpad_set_kbd_backlight;
}
- /* On version 2 and newer, let *drm or acpivout control brightness */
- if (sc->sc_hkey_version == THINKPAD_HKEY_VERSION1 &&
- (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
- 0, NULL, >sc_brightness) == 0)) {
+ if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
+ 0, NULL, >sc_brightness) == 0) {
ws_get_param = thinkpad_get_param;
ws_set_param = thinkpad_set_param;
}
msdosfs: Never allocate clusters outside the volume
Hi,
sometimes I see writing to msdosfs fail with EINVAL. This seems to occur
when writing a file reaches the end of the partition. But it only happens
if all bits in the pm_inusemap array are actually used, i.e. if
pm_maxcluster % 32 == 31 . This means that for the number N of data
clusters in the partition the condition N % 32 == 30 is true, as the first
two cluster numbers have special meaning and do not represent data
clusters on disk. For partition sizes that do not fulfill this condition,
there is always a bit set at the end of the pm_inusemap that prevents
chainlength() from overrunning the size of the partition.
Reproducer:
# dd if=/dev/zero bs=1k seek=20019 of=msdos.img count=1
1+0 records in
1+0 records out
1024 bytes transferred in 0.000 secs (52258229 bytes/sec)
# VND=$(vnconfig msdos.img ) && echo $VND
vnd0
# newfs_msdos ${VND}c
/dev/rvnd0c: 39928 sectors in 9982 FAT16 clusters (2048 bytes/cluster)
bps=512 spc=4 res=1 nft=2 rde=512 sec=40040 mid=0xf0 spf=39 spt=63 hds=1 hid=0
# mount_msdos /dev/${VND}c /mnt
# dd if=/dev/zero bs=1k of=/mnt/fff
dd: /mnt/fff: Invalid argument
12829+0 records in
12828+0 records out
13135872 bytes transferred in 1.884 secs (6969515 bytes/sec)
# df -h /mnt
Filesystem SizeUsed Avail Capacity Mounted on
/dev/vnd0c19.5M 12.5M7.0M65%/mnt
# umount /mnt/
# vnconfig -u ${VND}
There is this fix in FreeBSD:
commit 097a1d5fbb7990980f8f806c6878537c964adf32
Author: kib
Date: Fri Oct 28 11:34:32 2016 +
Ensure that cluster allocations never allocate clusters outside the
volume limits. In particular:
- Assert that usemap_alloc() and usemap_free() cluster number argument
is valid.
- In chainlength(), return 0 if cluster start is after the max cluster.
- In chainlength(), cut the calculated cluster chain length at the max
cluster.
- For true paranoia, after the pm_inusemap is calculated in
fillinusemap(), reset all bits in the array for clusters after the
max cluster, as in-use.
However, the for loop added to the end of fillinusemap() by that commit is
a no-op because a) the bits have already been set by the for loop at the
start of the function and b) the boundary conditions in the for loop mix
cluster numbers and indexes to pm_inusemap causing the loop to never be
executed.
Therefore, I omit that for loop alltogether but adapted the rest of the
FreeBSD fix to OpenBSD. This seems to fix the issue.
Are there any heavy users of msdosfs who can give this a test? Or any
comments or oks?
Cheers,
Stefan
diff --git a/sys/msdosfs/msdosfs_fat.c b/sys/msdosfs/msdosfs_fat.c
index c15b6257d43..0ab1463ea67 100644
--- a/sys/msdosfs/msdosfs_fat.c
+++ b/sys/msdosfs/msdosfs_fat.c
@@ -409,6 +409,7 @@ updatefats(struct msdosfsmount *pmp, struct buf *bp,
uint32_t fatbn)
static __inline void
usemap_alloc(struct msdosfsmount *pmp, uint32_t cn)
{
+ KASSERT(cn <= pmp->pm_maxcluster);
pmp->pm_inusemap[cn / N_INUSEBITS] |= 1 << (cn % N_INUSEBITS);
pmp->pm_freeclustercount--;
@@ -417,6 +418,7 @@ usemap_alloc(struct msdosfsmount *pmp, uint32_t cn)
static __inline void
usemap_free(struct msdosfsmount *pmp, uint32_t cn)
{
+ KASSERT(cn <= pmp->pm_maxcluster);
pmp->pm_freeclustercount++;
pmp->pm_inusemap[cn / N_INUSEBITS] &= ~(1 << (cn % N_INUSEBITS));
@@ -644,6 +646,8 @@ chainlength(struct msdosfsmount *pmp, uint32_t start,
uint32_t count)
u_int map;
uint32_t len;
+ if (start > pmp->pm_maxcluster)
+ return (0);
max_idx = pmp->pm_maxcluster / N_INUSEBITS;
idx = start / N_INUSEBITS;
start %= N_INUSEBITS;
@@ -651,11 +655,15 @@ chainlength(struct msdosfsmount *pmp, uint32_t start,
uint32_t count)
map &= ~((1 << start) - 1);
if (map) {
len = ffs(map) - 1 - start;
- return (len > count ? count : len);
+ len = MIN(len, count);
+ len = MIN(len, pmp->pm_maxcluster - start + 1);
+ return (len);
}
len = N_INUSEBITS - start;
- if (len >= count)
- return (count);
+ if (len >= count) {
+ len = MIN(count, pmp->pm_maxcluster - start + 1);
+ return (len);
+ }
while (++idx <= max_idx) {
if (len >= count)
break;
@@ -665,7 +673,9 @@ chainlength(struct msdosfsmount *pmp, uint32_t start,
uint32_t count)
}
len += N_INUSEBITS;
}
- return (len > count ? count : len);
+ len = MIN(len, count);
+ len = MIN(len, pmp->pm_maxcluster - start + 1);
+ return (len);
}
/*
Re: Push solock() down to sosetopt()
On Wed, Feb 15, 2023 at 08:08:42PM +0100, Alexander Bluhm wrote:
> On Mon, Jan 30, 2023 at 05:03:30PM +0300, Vitaliy Makkoveev wrote:
> > It makes sense to push solock() down to sosetopt() too. For a half cases
> > (*pr_ctloutput)() is just null op, so there is no reason to call it.
> > Also, a lot of things could be done without solock() held.
>
> You do a bunch of things together
> - push solock()
> - move code around
> - return error instead of error =
> - set sb to >so_snd or >so_rcv
>
> Some of the refactorings make sense, others change the code just
> to be different in my eyes.
>
> Can we discuss theses things in separate diffs?
>
Does this "set sb to >so_snd or >so_rcv" chunk look reasonable?
Merge sosetopt() SO_SND* with corresponding SO_RCV* code paths. The
difference they have is only the socket buffer.
Index: sys/kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.301
diff -u -p -r1.301 uipc_socket.c
--- sys/kern/uipc_socket.c 10 Feb 2023 14:34:17 - 1.301
+++ sys/kern/uipc_socket.c 17 Feb 2023 12:00:54 -
@@ -1844,6 +1844,9 @@ sosetopt(struct socket *so, int level, i
case SO_SNDLOWAT:
case SO_RCVLOWAT:
{
+ struct sockbuf *sb = ((optname == SO_SNDBUF ||
+ optname == SO_SNDLOWAT) ?
+ >so_snd : >so_rcv);
u_long cnt;
if (m == NULL || m->m_len < sizeof (int))
@@ -1852,34 +1855,21 @@ sosetopt(struct socket *so, int level, i
if ((long)cnt <= 0)
cnt = 1;
switch (optname) {
-
case SO_SNDBUF:
- if (so->so_snd.sb_state & SS_CANTSENDMORE)
- return (EINVAL);
- if (sbcheckreserve(cnt, so->so_snd.sb_wat) ||
- sbreserve(so, >so_snd, cnt))
- return (ENOBUFS);
- so->so_snd.sb_wat = cnt;
- break;
-
case SO_RCVBUF:
- if (so->so_rcv.sb_state & SS_CANTRCVMORE)
+ if (sb->sb_state & (SS_CANTSENDMORE |
+ SS_CANTRCVMORE))
return (EINVAL);
- if (sbcheckreserve(cnt, so->so_rcv.sb_wat) ||
- sbreserve(so, >so_rcv, cnt))
+ if (sbcheckreserve(cnt, sb->sb_wat) ||
+ sbreserve(so, sb, cnt))
return (ENOBUFS);
- so->so_rcv.sb_wat = cnt;
+ sb->sb_wat = cnt;
break;
case SO_SNDLOWAT:
- so->so_snd.sb_lowat =
- (cnt > so->so_snd.sb_hiwat) ?
- so->so_snd.sb_hiwat : cnt;
- break;
case SO_RCVLOWAT:
- so->so_rcv.sb_lowat =
- (cnt > so->so_rcv.sb_hiwat) ?
- so->so_rcv.sb_hiwat : cnt;
+ sb->sb_lowat = (cnt > sb->sb_hiwat) ?
+ sb->sb_hiwat : cnt;
break;
}
break;
@@ -1888,6 +1878,8 @@ sosetopt(struct socket *so, int level, i
case SO_SNDTIMEO:
case SO_RCVTIMEO:
{
+ struct sockbuf *sb = (optname == SO_SNDTIMEO ?
+ >so_snd : >so_rcv);
struct timeval tv;
uint64_t nsecs;
@@ -1901,15 +1893,9 @@ sosetopt(struct socket *so, int level, i
return (EDOM);
if (nsecs == 0)
nsecs = INFSLP;
- switch (optname) {
- case SO_SNDTIMEO:
- so->so_snd.sb_timeo_nsecs = nsecs;
- break;
- case SO_RCVTIMEO:
- so->so_rcv.sb_timeo_nsecs = nsecs;
- break;
- }
+ sb->sb_timeo_nsecs = nsecs;
+
break;
}
Re: [xenocara] xenodm.man fix
On Thu, Feb 16, 2023 at 09:29:53PM +0300, Mikhail wrote: Hello Mikhail, > /etc/X11/xenodm/Xsession file has a check for x bit Yes, this one caught me out a few years back: https://marc.info/?l=openbsd-bugs=162737223625768=2 In subsequent discussions with Matthieu and Theo, I *think* the eventual thought was that it would be better to get rid of the `-x` check. I might be misremembering that, though. Laurie
