Re: uvm_meter: remove wakeup of proc0
The jiffies patch plus the patch in uvm_meter worked for me. It even booted a
little faster than normal.
On Mon, Jul 31, 2023 at 10:04:44PM +0200, Claudio Jeker wrote:
> On Mon, Jul 31, 2023 at 09:49:30PM +0200, Claudio Jeker wrote:
> > On Mon, Jul 31, 2023 at 08:03:41PM +0300, Vitaliy Makkoveev wrote:
> > > This is the culprit:
> > >
> > > schedule_timeout_uninterruptible(long timeout)
> > > {
> > > tsleep(curproc, PWAIT, "schtou", timeout);
> > > return 0;
> > > }
> > >
> >
> > Please give this a try. I think on initialization
> > intel_dp_wait_source_oui() is called before intel_dp->last_oui_write is
> > set and this results in a 10min timeout because our jiffies are set to
> > ULONG_MAX - (10 * 60 * HZ);
>
> After talking with kettenis@ I think the following diff is better.
> Starting with 0 jiffies should fix this issue.
> Unless we want to do the linux madness and set it to
> ((unsigned long)(unsigned int) (-300*HZ))
>
> --
> :wq Claudio
>
> Index: kern_clock.c
> ===
> RCS file: /cvs/src/sys/kern/kern_clock.c,v
> retrieving revision 1.109
> diff -u -p -r1.109 kern_clock.c
> --- kern_clock.c 25 Jul 2023 18:16:19 - 1.109
> +++ kern_clock.c 31 Jul 2023 20:01:57 -
> @@ -84,7 +84,7 @@ int profhz;
> int profprocs;
> int ticks = INT_MAX - (15 * 60 * HZ);
>
> -volatile unsigned long jiffies = ULONG_MAX - (10 * 60 * HZ);
> +volatile unsigned long jiffies;
>
> /*
> * Initialize clock frequencies and start both clocks running.
>
Re: uvm_meter: remove wakeup of proc0
On Sat, Jul 29, 2023 at 11:16:14AM +0200, Claudio Jeker wrote:
> proc0 aka the swapper does not do anything. So there is no need to wake it
> up. Now the problem is that last time this was tried some inteldrm systems
> did hang during bootup because the drm code unexpectedly depended on this
> wakeup.
>
> I think I fixed all possible cases of this in the drm stack and so it is
> time to retry this. People with affected machines please give this a try.
>
> --
> :wq Claudio
>
> Index: uvm/uvm_meter.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_meter.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 uvm_meter.c
> --- uvm/uvm_meter.c 21 Jun 2023 21:16:21 - 1.44
> +++ uvm/uvm_meter.c 29 Jul 2023 07:48:44 -
> @@ -89,8 +89,6 @@ uvm_meter(void)
> {
> if ((gettime() % 5) == 0)
> uvm_loadav();
> - if (proc0.p_slptime > (maxslp / 2))
> - wakeup();
> }
>
> /*
>
I tried this again, but bootup still hung at "root on "
I do have some kernel customizations normally, but this patch, applied to my
custom kernel and to a generic one, causes boot to hang.
Here's the dmesg of my machine:
OpenBSD 7.3-current (NOAMDGPU.MP) #45: Mon Jul 31 08:59:16 MDT 2023
me@sonofthief.private:/home/me/src/sys/arch/amd64/compile/NOAMDGPU.MP
real mem = 8346333184 (7959MB)
avail mem = 8087310336 (7712MB)
random: good seed from bootblocks
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 3.3 @ 0x439de000 (69 entries)
bios0: vendor LENOVO version "FHCN54WW" date 07/05/2021
bios0: LENOVO 82FG
efi0 at bios0: UEFI 2.7
efi0: INSYDE Corp. rev 0x59474054
acpi0 at bios0: ACPI 6.1Undefined scope: \\_SB_.PCI0
acpi0: sleep states S0 S4 S5
acpi0: tables DSDT FACP UEFI SSDT SSDT SSDT SSDT SSDT MSDM NHLT SSDT LPIT WSMT
SSDT SSDT DBGP DBG2 ECDT HPET APIC MCFG SSDT DMAR SSDT SSDT SSDT FPDT PTDT BGRT
acpi0: wakeup devices PEG0(S4) PEGP(S4) PEGA(S4) PEGP(S4) PEGP(S4) XHCI(S4)
XDCI(S4) HDAS(S4) RP01(S4) PXSX(S4) RP02(S4) PXSX(S4) RP03(S4) PXSX(S4)
RP04(S4) PXSX(S4) [...]
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpiec0 at acpi0: not present
acpihpet0 at acpi0: 1920 Hz
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz, 4190.34 MHz, 06-8c-01
cpu0:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,TSC_ADJUST,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,AVX512F,AVX512DQ,RDSEED,ADX,SMAP,AVX512IFMA,CLFLUSHOPT,CLWB,PT,AVX512CD,SHA,AVX512BW,AVX512VL,AVX512VBMI,UMIP,PKU,SRBDS_CTRL,MD_CLEAR,IBT,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,IBRS_ALL,SKIP_L1DFL,MDS_NO,IF_PSCHANGE,MISC_PKG_CT,ENERGY_FILT,DOITM,FBSDP_NO,XSAVEOPT,XSAVEC,XGETBV1,XSAVES
cpu0: 48KB 64b/line 12-way D-cache, 32KB 64b/line 8-way I-cache, 1MB 64b/line
20-way L2 cache, 8MB 64b/line 8-way L3 cache
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges
cpu0: apic clock running at 38MHz
cpu0: mwait min=64, max=64, C-substates=0.2.0.1.2.1.1.1, IBE
cpu1 at mainbus0: apid 2 (application processor)
cpu1: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz, 4190.35 MHz, 06-8c-01
cpu1:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,TSC_ADJUST,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,AVX512F,AVX512DQ,RDSEED,ADX,SMAP,AVX512IFMA,CLFLUSHOPT,CLWB,PT,AVX512CD,SHA,AVX512BW,AVX512VL,AVX512VBMI,UMIP,PKU,SRBDS_CTRL,MD_CLEAR,IBT,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,IBRS_ALL,SKIP_L1DFL,MDS_NO,IF_PSCHANGE,MISC_PKG_CT,ENERGY_FILT,DOITM,FBSDP_NO,XSAVEOPT,XSAVEC,XGETBV1,XSAVES
cpu1: 48KB 64b/line 12-way D-cache, 32KB 64b/line 8-way I-cache, 1MB 64b/line
20-way L2 cache, 8MB 64b/line 8-way L3 cache
cpu1: smt 0, core 1, package 0
cpu2 at mainbus0: apid 4 (application processor)
cpu2: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz, 3791.27 MHz, 06-8c-01
cpu2:
[patch] Remove dead code in uvm_pmr_getpages()
Hello. The diff in this email removes dead code in uvm_pmr_getpages().
This had bothered me for a bit because I had intuitively believed it wasn't
necessary, but had never bothered to fully convince myself. Since I have now,
I'll share the reasoning.
This diff concerns the following bit of code:
if (maxseg == 1 || count == 1) {
start_try = 2;
search[2] = count;
} else if (maxseg >= count && (flags & UVM_PLA_TRYCONTIG) == 0) {
start_try = 2;
search[2] = 1;
} else {
start_try = 0;
search[0] = count;
search[1] = pow2divide(count, maxseg);
search[2] = 1;
if ((flags & UVM_PLA_TRYCONTIG) == 0)
start_try = 1;
- if (search[1] >= search[0]) {
- search[1] = search[0];
- start_try = 1;
- }
if (search[2] >= search[start_try]) {
start_try = 2;
}
}
The lines indicated are the ones to be removed. The reason is that search[1]
will always be less than search[0] at this point in the code.
Proof: From pow2divide() we know that search[1] is the smallest power of 2 that
is >= 1 such that:
search[1]*maxseg >= count == search[0].
This means that if search[1] is > 1 that search[1]*maxseg/2 < search[0]. We
already know that maxseg is at least 2 because of the branch
if (maxseg == 1 || count == 1)
that would have been taken otherwise. Therefore, maxseg/2 is at least 1, so:
search[1] <= search[1]*maxseg/2 < search[0]
when search[1] is at least 2. If search[1] is 1, then search[1] < search[2]
because search[2] == count and count is at least 2 from the branch mentioned
previously. QED.
Index: uvm_pmemrange.c
===
RCS file: /cvs/src/sys/uvm/uvm_pmemrange.c,v
retrieving revision 1.63
diff -u -p -u -p -r1.63 uvm_pmemrange.c
--- uvm_pmemrange.c 10 Apr 2023 04:21:20 - 1.63
+++ uvm_pmemrange.c 10 Jun 2023 16:28:47 -
@@ -909,10 +909,6 @@ uvm_pmr_getpages(psize_t count, paddr_t
search[2] = 1;
if ((flags & UVM_PLA_TRYCONTIG) == 0)
start_try = 1;
- if (search[1] >= search[0]) {
- search[1] = search[0];
- start_try = 1;
- }
if (search[2] >= search[start_try]) {
start_try = 2;
}
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: [acpi] patch to run _INI methods before table devices attach
On Sun, Oct 23, 2022 at 05:51:41PM +0300, Mikhail wrote:
> On Sun, Oct 23, 2022 at 08:32:47AM -0600, Chris Waddey wrote:
> > So I've been following the work on this because I have a lenovo laptop
> > as well and the first patch you sent in I applied and have had
> > everything working that I expect to (battery status/apm all works
> > great, as well as brightness keys and -- surprise -- volume
> > keys). Mark's patch that he suggested instead of this one did not
> > produce any meaningful difference from -current.
> >
> > I'll try the newer patches and let you know how it goes. I also have
> > an HP desktop that I can try it on at home to see if it breaks anything.
>
> > acpiec0 at acpi0: not present
> [...]
> > acpiec1 at acpi0
>
> Based on your dmesg I think you have the same problem as I do - ECDT has
> wrong device name in its EC_ID.
>
> To check this you can acpidump(8) your tables, then decompile AML with
> 'iasl -d DSDT.xxx' and 'iasl -d ECDT.xxx' (iasl is in 'acpica' package),
> after this take a look in ECDT.dsl, you need 'Namepath' field, for me
> it's "\_SB.PC00.LPCB.H_EC", but actual device is "\_SB.PC00.LPCB.EC0" -
> this one you can lookup in DSDT.dsl, try searching for something like
> "Device (EC0)".
>
> If this mismatch of ECDT and DSDT is your case - you can report it to
> Lenovo, as I did, maybe they will fix the BIOS.
It does appear that I have the same mismatch. I have exactly
"\_SB.PC00.LPCB.H_EC" in the namepath field of my ECDT.dsl, but no
mentions of the same in DSDT.dsl. There are, however, many mentions of
"\_SB.PC00.LPCB.EC0" in DSDT.dsl, especially under the "Device (ECO)"
section.
> Also, I'd be appreciated if you come up with the results to tech@, so
> Mark and others can see that it's not only my problem.
CC'ing tech@.
I've applied the following two patches and apm give this output:
Battery state: charging, 57% remaining, 35 minutes recharge time estimate
AC adapter state: connected
Performance adjustment mode: auto (2401 MHz)
or
Battery state: high, 57% remaining, 190 minutes life estimate
AC adapter state: not connected
Performance adjustment mode: auto (400 MHz)
depending on if I'm charging or not. I also have working brightness
and volume keys still.
diff /usr/src
commit - 5cb1d9dce18f152bf9f5d7d4251dafe18a5c0c82
path + /usr/src
blob - 5ef24d5179de52d5321e578b3b73dd9524e7c1de
file + sys/dev/acpi/acpiec.c
--- sys/dev/acpi/acpiec.c
+++ sys/dev/acpi/acpiec.c
@@ -275,14 +275,13 @@ acpiec_attach(struct device *parent, struct device *se
struct acpiec_softc *sc = (struct acpiec_softc *)self;
struct acpi_attach_args *aa = aux;
struct aml_value res;
- int64_t st;
+ int64_t st = 0;
sc->sc_acpi = (struct acpi_softc *)parent;
sc->sc_devnode = aa->aaa_node;
sc->sc_cantburst = 0;
- if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "_STA", 0, NULL, ))
- st = STA_PRESENT | STA_ENABLED | STA_DEV_OK;
+ aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "_STA", 0, NULL, );
if ((st & STA_PRESENT) == 0) {
printf(": not present\n");
return;
diff /usr/src
commit - 3fb2197480c345a19f2098d1b787c28b9c717dcb
path + /usr/src
blob - fc3c1d160ee0ccc7217d77be184c253b29422983
file + sys/dev/acpi/acpi.c
--- sys/dev/acpi/acpi.c
+++ sys/dev/acpi/acpi.c
@@ -1203,6 +1203,9 @@ acpi_attach_common(struct acpi_softc *sc, paddr_t base
}
#endif /* SMALL_KERNEL */
+ /* initialize runtime environment */
+ aml_find_node(sc->sc_root, "_INI", acpi_inidev, sc);
+
/*
* Attach table-defined devices
*/
@@ -1217,9 +1220,6 @@ acpi_attach_common(struct acpi_softc *sc, paddr_t base
config_found_sm(>sc_dev, , acpi_print, acpi_submatch);
}
- /* initialize runtime environment */
- aml_find_node(sc->sc_root, "_INI", acpi_inidev, sc);
-
/* Get PCI mapping */
aml_walknodes(sc->sc_root, AML_WALK_PRE, acpi_getpci, sc);
dmesg:
OpenBSD 7.2-current (GENERIC.MP) #9: Sun Oct 23 09:16:32 MDT 2022
me@thief.private:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 8346333184 (7959MB)
avail mem = 8075952128 (7701MB)
random: good seed from bootblocks
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 3.3 @ 0x439de000 (69 entries)
bios0: vendor LENOVO version "FHCN54WW" date 07/05/2021
bios0: LENOVO 82FG
efi0 at bios0: UEFI 2.7
efi0: INSYDE Corp. rev 0x59474054
acpi0 at bios0: ACPI 6.1Undefined scope: \\_SB_.PCI0
acpi0: sleep states S0 S4 S5
acpi0: tables DSDT FACP UEFI SSDT SSDT SSDT SSDT SSDT MSDM NHLT SSDT LPIT WSMT
SSDT SSDT DBGP DBG2 ECDT HPET APIC MCFG SSDT DMAR SSDT
Re: smtpd bug in Received: header with one recipient
Ping.
On Sun, Oct 09, 2022 at 08:01:01AM +0200, Martijn van Duren wrote:
> Bit focused on other things atm and not familiar with this part of the
> code, but some comments.
>
> On Sat, 2022-10-08 at 12:18 -0600, Chris Waddey wrote:
>> A message with a single successful recipient but with a failed
>> rcpt to: command afterward generates an incorrect Received: header.
...
>> The following patch fixes the problem:
>> Index: smtp_session.c
>> ===
>> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
>> retrieving revision 1.432
>> diff -u -p -r1.432 smtp_session.c
>> --- smtp_session.c 1 Jul 2021 07:42:16 - 1.432
>> +++ smtp_session.c 8 Oct 2022 18:04:51 -
>> @@ -2732,6 +2732,7 @@ static void
>> smtp_message_begin(struct smtp_tx *tx)
>> {
>>struct smtp_session *s;
>> + struct smtp_rcpt *srfp;
>>int (*m_printf)(struct smtp_tx *, const char *, ...);
>>m_printf = smtp_message_printf;
>> @@ -2780,10 +2781,13 @@ smtp_message_begin(struct smtp_tx *tx)
>>}
>>}
>> + /* If we get failed "RCPT TO" commands that modify tx->evp, then
>> +* make sure we use the real recipient for the "for" clause */
> See style(9) how comments should be formatted:
> /*
>* Multi-line comments look like this. Make them real sentences.
>* Fill them so they look like real paragraphs.
>*/
Got it.
>>if (tx->rcptcount == 1) {
>> + srfp = TAILQ_FIRST(>rcpts);
>>m_printf(tx, "\n\tfor <%s@%s>",
>> - tx->evp.rcpt.user,
>> - tx->evp.rcpt.domain);
>> + srfp->maddr.user,
>> + srfp->maddr.domain);
> I don't see anything wrong with this, but does the code still do the correct
> thing with multiple recipients?
> RCPT TO:
> RCPT TO:
> RCPT TO:
> or
> RCPT TO:
> RCPT TO:
> RCPT TO:
For reference, testing the two above scenarios with the original gives
the following Received: header (except date and message id
differences):
Received: by thief.private (OpenSMTPD) with ESMTP id 4f5144f3; Sun, 9 Oct 2022
12:09:22 -0600 (MDT)
With the patch it still gives the following for both scenarios:
Received: by thief.private (OpenSMTPD) with ESMTP id 50bf7075; Sun, 9 Oct 2022
12:14:32 -0600 (MDT)
Modified patch with comment fix (feel free to remove the comment
entirely if you think it's not needed):
Index: smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.432
diff -u -p -r1.432 smtp_session.c
--- smtp_session.c1 Jul 2021 07:42:16 -1.432
+++ smtp_session.c9 Oct 2022 18:15:53 -
@@ -2732,6 +2732,7 @@ static void
smtp_message_begin(struct smtp_tx *tx)
{
struct smtp_session *s;
+struct smtp_rcpt *srfp;
int(*m_printf)(struct smtp_tx *, const char *, ...);
m_printf = smtp_message_printf;
@@ -2780,10 +2781,15 @@ smtp_message_begin(struct smtp_tx *tx)
}
}
+/*
+ * If we get failed "RCPT TO" commands that modify tx->evp, then
+ * make sure we use the real recipient for the "for" clause
+ */
if (tx->rcptcount == 1) {
+srfp = TAILQ_FIRST(>rcpts);
m_printf(tx, "\n\tfor <%s@%s>",
-tx->evp.rcpt.user,
-tx->evp.rcpt.domain);
+srfp->maddr.user,
+srfp->maddr.domain);
}
m_printf(tx, ";\n\t%s\n", time_to_text(time(>time)));
Patch with no comment:
Index: smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.432
diff -u -p -r1.432 smtp_session.c
--- smtp_session.c1 Jul 2021 07:42:16 -1.432
+++ smtp_session.c9 Oct 2022 18:18:44 -
@@ -2732,6 +2732,7 @@ static void
smtp_message_begin(struct smtp_tx *tx)
{
struct smtp_session *s;
+struct smtp_rcpt *srfp;
int(*m_printf)(struct smtp_tx *, const char *, ...);
m_printf = smtp_message_printf;
@@ -2781,9 +2782,10 @@ smtp_message_begin(struct smtp_tx *tx)
}
if (tx->rcptcount == 1) {
+srfp = TAILQ_FIRST(>rcpts);
m_printf(tx, "\n\tfor <%s@%s>",
-tx->evp.rcpt.user,
-tx->evp.rcpt.domain);
+srfp->maddr.user,
+srfp->maddr.domain);
}
m_printf(tx, ";\n\t%s\n", time_to_text(time(>time)));
Re: smtpd bug in Received: header with one recipient
On Sun, Oct 09, 2022 at 08:01:01AM +0200, Martijn van Duren wrote:
> Bit focused on other things atm and not familiar with this part of the
> code, but some comments.
>
> On Sat, 2022-10-08 at 12:18 -0600, Chris Waddey wrote:
>> A message with a single successful recipient but with a failed
>> rcpt to: command afterward generates an incorrect Received: header.
...
>> The following patch fixes the problem:
>> Index: smtp_session.c
>> ===
>> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
>> retrieving revision 1.432
>> diff -u -p -r1.432 smtp_session.c
>> --- smtp_session.c 1 Jul 2021 07:42:16 - 1.432
>> +++ smtp_session.c 8 Oct 2022 18:04:51 -
>> @@ -2732,6 +2732,7 @@ static void
>> smtp_message_begin(struct smtp_tx *tx)
>> {
>>struct smtp_session *s;
>> + struct smtp_rcpt *srfp;
>>int (*m_printf)(struct smtp_tx *, const char *, ...);
>>m_printf = smtp_message_printf;
>> @@ -2780,10 +2781,13 @@ smtp_message_begin(struct smtp_tx *tx)
>>}
>>}
>> + /* If we get failed "RCPT TO" commands that modify tx->evp, then
>> +* make sure we use the real recipient for the "for" clause */
> See style(9) how comments should be formatted:
> /*
>* Multi-line comments look like this. Make them real sentences.
>* Fill them so they look like real paragraphs.
>*/
Got it.
>>if (tx->rcptcount == 1) {
>> + srfp = TAILQ_FIRST(>rcpts);
>>m_printf(tx, "\n\tfor <%s@%s>",
>> - tx->evp.rcpt.user,
>> - tx->evp.rcpt.domain);
>> + srfp->maddr.user,
>> + srfp->maddr.domain);
> I don't see anything wrong with this, but does the code still do the correct
> thing with multiple recipients?
> RCPT TO:
> RCPT TO:
> RCPT TO:
> or
> RCPT TO:
> RCPT TO:
> RCPT TO:
For reference, testing the two above scenarios with the original gives
the following Received: header (except date and message id
differences):
Received: by thief.private (OpenSMTPD) with ESMTP id 4f5144f3; Sun, 9 Oct 2022
12:09:22 -0600 (MDT)
With the patch it still gives the following for both scenarios:
Received: by thief.private (OpenSMTPD) with ESMTP id 50bf7075; Sun, 9 Oct 2022
12:14:32 -0600 (MDT)
Modified patch with comment fix (feel free to remove the comment
entirely if you think it's not needed):
Index: smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.432
diff -u -p -r1.432 smtp_session.c
--- smtp_session.c1 Jul 2021 07:42:16 -1.432
+++ smtp_session.c9 Oct 2022 18:15:53 -
@@ -2732,6 +2732,7 @@ static void
smtp_message_begin(struct smtp_tx *tx)
{
struct smtp_session *s;
+struct smtp_rcpt *srfp;
int(*m_printf)(struct smtp_tx *, const char *, ...);
m_printf = smtp_message_printf;
@@ -2780,10 +2781,15 @@ smtp_message_begin(struct smtp_tx *tx)
}
}
+/*
+ * If we get failed "RCPT TO" commands that modify tx->evp, then
+ * make sure we use the real recipient for the "for" clause
+ */
if (tx->rcptcount == 1) {
+srfp = TAILQ_FIRST(>rcpts);
m_printf(tx, "\n\tfor <%s@%s>",
-tx->evp.rcpt.user,
-tx->evp.rcpt.domain);
+srfp->maddr.user,
+srfp->maddr.domain);
}
m_printf(tx, ";\n\t%s\n", time_to_text(time(>time)));
Patch with no comment:
Index: smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.432
diff -u -p -r1.432 smtp_session.c
--- smtp_session.c1 Jul 2021 07:42:16 -1.432
+++ smtp_session.c9 Oct 2022 18:18:44 -
@@ -2732,6 +2732,7 @@ static void
smtp_message_begin(struct smtp_tx *tx)
{
struct smtp_session *s;
+struct smtp_rcpt *srfp;
int(*m_printf)(struct smtp_tx *, const char *, ...);
m_printf = smtp_message_printf;
@@ -2781,9 +2782,10 @@ smtp_message_begin(struct smtp_tx *tx)
}
if (tx->rcptcount == 1) {
+srfp = TAILQ_FIRST(>rcpts);
m_printf(tx, "\n\tfor <%s@%s>",
-tx->evp.rcpt.user,
-tx->evp.rcpt.domain);
+srfp->maddr.user,
+srfp->maddr.domain);
}
m_printf(tx, ";\n\t%s\n", time_to_text(time(>time)));
smtpd bug in Received: header with one recipient
A message with a single successful recipient but with a failed
rcpt to: command afterward generates an incorrect Received: header.
The following produces the bug:
thief$ nc localhost 25
220 thief.private ESMTP OpenSMTPD
ehlo thief.private^M
250-thief.private Hello thief.private [127.0.0.1], pleased to meet you
250-8BITMIME
250-ENHANCEDSTATUSCODES
250-SIZE 36700160
250-DSN
250 HELP
mail from:^M
250 2.0.0 Ok
rcpt to:^M
250 2.1.5 Destination address valid: Recipient ok
rcpt to:^M
550 Invalid recipient:
data^M
354 Enter mail, end with "." on a line by itself
From:^M
^M
test^M
.^M
250 2.0.0 8f9363cc Message accepted for delivery
quit^M
221 2.0.0 Bye
thief$
This gives the following message (I have the mask-src option set here):
Date: Sat, 8 Oct 2022 12:08:48 -0600 (MDT)
From: me@thief.private
Return-Path:
Delivered-To: me@thief.private
Received: by thief.private (OpenSMTPD) with ESMTP id 8f9363cc for
; Sat, 8 Oct 2022 12:08:48 -0600 (MDT)
Message-ID:
test
The following patch fixes the problem:
Index: smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.432
diff -u -p -r1.432 smtp_session.c
--- smtp_session.c 1 Jul 2021 07:42:16 - 1.432
+++ smtp_session.c 8 Oct 2022 18:04:51 -
@@ -2732,6 +2732,7 @@ static void
smtp_message_begin(struct smtp_tx *tx)
{
struct smtp_session *s;
+ struct smtp_rcpt *srfp;
int (*m_printf)(struct smtp_tx *, const char *, ...);
m_printf = smtp_message_printf;
@@ -2780,10 +2781,13 @@ smtp_message_begin(struct smtp_tx *tx)
}
}
+ /* If we get failed "RCPT TO" commands that modify tx->evp, then
+* make sure we use the real recipient for the "for" clause */
if (tx->rcptcount == 1) {
+ srfp = TAILQ_FIRST(>rcpts);
m_printf(tx, "\n\tfor <%s@%s>",
- tx->evp.rcpt.user,
- tx->evp.rcpt.domain);
+ srfp->maddr.user,
+ srfp->maddr.domain);
}
m_printf(tx, ";\n\t%s\n", time_to_text(time(>time)));
I use OpenSMTPD on OpenBSD for my personal mail, and it's great. Happy
to contribute in any way I can. Thank you for all you do.
Suggestions for /usr/sbin/config
So I recently read most of the source for config (only stuff needed
for kernel compilation), and I'm certainly no expert, but I have a few
suggestions.
The first is that pathnames in gram.y should probably use right
recursion, correct? Unless you want to be limited to pathname lists
of length two. It doesn't matter anywhere yet, but it might someday.
The second is that one could add a precedence to the negation operator
so that you can negate an fexpr instead of just an fatom. I have to
admit I'm a little uncertain about using %nonassoc vs %right, but both
seem to work in toy parsers that I've written to emulate/check this.
Third, you would probably want pathtab to hold all the pathnames, but
it seems that, if we have more than one pathname, not all get put there
during addfile. I don't know how to deal with this exactly, but maybe
just putting it all in a for loop is okay.
Lastly, the emitlocnames function gets some things wrong. (It
sets the i_locnami field according to the i_plocnami field of the
first parent, and because i_plocnami is set according to locators of
its first child found in the packed devi list, things go awry). For
example, my ioconf.c file says that pci* has a sigle locname of "bus",
but because its first parent is mainbus0, and bios0 (with its single
locname of apid) is the first entry in packed with mainbus0 as a
parent, pci has the same index into the locnames array as bios0, which
means it appears to have an "apid" locname.
This last one seems as if it was written correctly 20 years ago and
then changed in the name of optimizing space. I say who cares about
optimizing this much space, keep it simple.
Anyway, that's all I can remember. I've applied the patches below, and
then made a new kernel with it, and everything seems to be working
okay. The locnamp array could definitely be compressed more, but at
least it's correct now.
Thank you,
Chris Waddey
Index: gram.y
===
RCS file: /cvs/src/usr.sbin/config/gram.y,v
retrieving revision 1.24
diff -u -p -r1.24 gram.y
--- gram.y 16 Jan 2015 06:40:16 - 1.24
+++ gram.y 14 Jan 2021 21:48:13 -
@@ -107,6 +107,7 @@ static voidcheck_maxpart(void);
%left '|'
%left '&'
+%nonassoc '!'
%typepathnames
%typefopts fexpr fatom
@@ -169,7 +170,7 @@ dev_eof:
pathnames:
PATHNAME{ $$ = new_nsi($1, NULL, 0); } |
- pathnames '|' PATHNAME { ($$ = $1)->nv_next = new_nsi($3,
NULL, 0); };
+ PATHNAME '|' pathnames { ($$ = new_nsi($1, NULL, 0))->nv_next
= $3; };
/*
* Various nonterminals shared between the grammars.
@@ -187,7 +188,7 @@ fopts:
fexpr:
fatom { $$ = $1; } |
- '!' fatom { $$ = fx_not($2); } |
+ '!' fexpr { $$ = fx_not($2); } |
fexpr '&' fexpr { $$ = fx_and($1, $3); } |
fexpr '|' fexpr { $$ = fx_or($1, $3); } |
'(' fexpr ')' { $$ = $2; };
Index: files.c
===
RCS file: /cvs/src/usr.sbin/config/files.c,v
retrieving revision 1.20
diff -u -p -r1.20 files.c
--- files.c 16 Jan 2015 06:40:16 - 1.20
+++ files.c 14 Jan 2021 21:49:15 -
@@ -143,13 +143,16 @@ addfile(struct nvlist *nvpath, struct nv
* will be used after all.
*/
fi = emalloc(sizeof *fi);
- if (ht_insert(pathtab, path, fi)) {
- free(fi);
- if ((fi = ht_lookup(pathtab, path)) == NULL)
- panic("addfile: ht_lookup(%s)", path);
- error("duplicate file %s", path);
- xerror(fi->fi_srcfile, fi->fi_srcline,
- "here is the original definition");
+ for (nv = nvpath; nv; nv = nv->nv_next) {
+ path = nv->nv_name;
+ if (ht_insert(pathtab, path, fi)) {
+ free(fi);
+ if ((fi = ht_lookup(pathtab, path)) == NULL)
+ panic("addfile: ht_lookup(%s)", path);
+ error("duplicate file %s", path);
+ xerror(fi->fi_srcfile, fi->fi_srcline,
+ "here is the original definition");
+ }
}
memcpy(base, tail, baselen);
base[baselen] = 0;
Index: mkioconf.c
===
RCS file: /cvs/src/usr.sbin/config/mkioconf.c,v
retrieving revision 1.38
diff -u -p -r1.38 mkioconf.c
--- mkioconf.c 28 Jun 2019 13:33:55 - 1.38
+++ mkioconf.c 14 Jan 2021 21:49:57 -
@@ -230,42 +230,29 @@ emitlocnames(FILE *fp)
struct devi **p, *i;
struct nvlist *nv;
struct attr *a;
- int added
Probably useless patch to initial GDT
I doubt this will ever matter, but I think this is correct. I hope this isn't an annoying patch. Just trying to get my feet wet in the kernel. It compiled and ran on my machine for what it's worth. Chris Waddey Index: gidt.S === RCS file: /cvs/src/sys/arch/amd64/stand/libsa/gidt.S,v retrieving revision 1.12 diff -u -p -r1.12 gidt.S --- gidt.S 9 Nov 2019 17:58:46 - 1.12 +++ gidt.S 4 Sep 2020 23:11:23 - @@ -260,14 +260,14 @@ gdt: .byte (LINKADDR >> 16) & 0xff # midbase .byte SDT_MEMERAC | 0 | 0x80 # RXAC, dpl = 0, present .byte 0x0 | 0 | 0 | 0 # hilimit, xx, 16bit, byte granularity - .byte (LINKADDR >> 20) & 0xff # hibase + .byte (LINKADDR >> 24) & 0xff # hibase /* 0x20 : 16 bit data */ .word 0x # lolimit .word (LINKADDR & 0x) # lobase .byte (LINKADDR >> 16) & 0xff # midbase .byte SDT_MEMRWA | 0 | 0x80 # RWA, dpl = 0, present .byte 0x0 | 0 | 0 | 0 # hilimit, xx, 16bit, byte granularity - .byte (LINKADDR >> 20) & 0xff # hibase + .byte (LINKADDR >> 24) & 0xff # hibase .globl Gdtr Gdtr: .word . - gdt - 1
Re: Potential fix for fsck on ext2 rev1 filesystems
Except my the 's in the diff are supposed to be &'s. Index: pass2.c === RCS file: /cvs/src/sbin/fsck_ext2fs/pass2.c,v retrieving revision 1.15 diff -u -p -r1.15 pass2.c --- pass2.c 28 Apr 2016 12:17:15 - 1.15 +++ pass2.c 17 Dec 2019 17:49:51 - @@ -145,7 +145,7 @@ pass2(void) inodirty(); } } - memset(, 0, EXT2_DINODE_SIZE()); + memset(, 0, sizeof(struct ext2fs_dinode)); dino.e2di_mode = htole16(IFDIR); inossize(, inp->i_isize); memcpy(_blocks[0], >i_blks[0], (size_t)inp->i_numblks);
Potential fix for fsck on ext2 rev1 filesystems
Hello all. I while back I put an ext2 rev1 filesystem on my disk (not sure why any more, but oh well). Whenever fsck ran, it stopped during pass2 with the error message "wrong type to dirscan 0". Long story short, it seems that a call to memset was writing zeros over the curino structure because the rev1 inode structure takes up 256 bytes on disk but only 156 in memory. I assume that the patch below (which worked fine on my machine), would also work for anyone with a rev0 ext2 fs since there is only one ext2fs_dinode structure for all of fsck_ext2fs. Does that make sense? I could give more info, but I don't want to be pedantic, and I assume most people looking at this know more than I do. Chris Index: pass2.c === RCS file: /cvs/src/sbin/fsck_ext2fs/pass2.c,v retrieving revision 1.15 diff -u -p -r1.15 pass2.c --- pass2.c 28 Apr 2016 12:17:15 - 1.15 +++ pass2.c 17 Dec 2019 17:49:51 - @@ -145,7 +145,7 @@ pass2(void) inodirty(); } } - memset(dino, 0, EXT2_DINODE_SIZE(sblock)); + memset(dino, 0, sizeof(struct ext2fs_dinode)); dino.e2di_mode = htole16(IFDIR); inossize(dino, inp-i_isize); memcpy(dino.e2di_blocks[0], inp-i_blks[0], (size_t)inp-i_numblks);
diff for sysupgrade.sh in anticipation of release 10.0
Hello all. I know this isn't necessary for quite a while, but in anticipation of OpenBSD 10.0, I suggest the following diff to sysupgrade.sh: Index: sysupgrade.sh === RCS file: /cvs/src/usr.sbin/sysupgrade/sysupgrade.sh,v retrieving revision 1.25 diff -u -p -r1.25 sysupgrade.sh --- sysupgrade.sh 28 Sep 2019 17:30:07 - 1.25 +++ sysupgrade.sh 9 Oct 2019 14:38:20 - @@ -93,8 +93,8 @@ if $RELEASE && $SNAP; then usage fi -set -A _KERNV -- $(sysctl -n kern.version | - sed 's/^OpenBSD \([0-9]\)\.\([0-9]\)\([^ ]*\).*/\1.\2 \3/;q') +set -A _KERNV -- $(sysctl -n kern.version | sed \ + 's/^OpenBSD \([1-9][0-9]*\)\.\([0-9]\)\([^ ]*\).*/\1.\2 \3/;q') shift $(( OPTIND -1 )) ok? Chris
