Rework the remaining str_* functions #143

DerDakon · 2020-05-24T13:35:04Z

This is not really intended to be merged as-is, it should be a point of discussion.

qmail-send.c

josuah

All good to me, except GNU_SOURCE, but is optional so I push my Go.

str_chr.c.gnu

josuah

In the downstream branch Dakon-1.09, after this merge:8c2f9f9 Merge branch 'Dakon-remove-strchr' into Dakon-1.09

I temprorarily had a undefined j variable in qmail-remote.c: after switching from integers (i, j, ...) to pointers (char *)at, ...), some code was merged that still had integers around.

The merge incorporated an undefined j variable into Dakon-1.09, but after a rebase, I do not see the compilation failure anymore except for dns-oversize : ld: error: undefined symbol: alloc_re. But this is not related to this changeset.

josuah · 2020-06-14T19:08:24Z

case_lowerb.c

-void case_lowerb(s,len)
-char *s;
-unsigned int len;
+void case_lowerb(char *s, unsigned int len)


unsigned int len feels like a size_t, let's see if the int -> size_t commit id up or downstream...

It looks like there are multiple commits that convert int to size_t at multiple places. If this is something we want, would another branch be more appropriate or do we adjust source as we touch it?

Yes, I would like to see it in a different PR on top of this, best once this is merged to avoid duplicate work.

josuah · 2020-06-14T19:15:34Z

byte_zero.c

-void byte_zero(s,n)
-char *s;
-register unsigned int n;
+void byte_zero(char *s, unsigned int n)


Is this not to be replaced by memset so we can remove this file entirely?

#define byte_zero(s, n) memset(s, 0, n)

The other functions above raise this same question for me.

I intentionally kept this so the compiler will not optimize the zeroing aways. I need a lot of logic to get this portable in Qsmtp, and there is still the possibility that it will fallback to not finding one: https://github.com/DerDakon/Qsmtp/blob/master/qsmtpd/auth.c#L29

Would a comment be welcome on the source so that we state out where we want not to edit it? Although maybe byte_zero is a famous enough topic.

libressl/openbsd#5

It looks like there is a hard time getting this not optimized away.

Our luck is that we don't use -flto at the moment. And the other thing is that it is used in that "dangerous" way only in qmail-popup, which will hopefully just go away together with qmail-pop3d.

Cross-referencing #65 which at one point was suggesting

#define byte_zero(s, n) memset((s), 0, (n))

now that I see this conversation here :-)

(Hopefully not derailing this PR: I know in our last release we expressed intent to delete qmail-popup and qmail-pop3d, but I'm not ready to do that yet, even though I don't run them myself. They're a worked example of a particularly qmail-ish design we might want to follow for AUTH and extend to support TLS, so I'd like us to hang onto them at least until we've added those features -- assuming they're relatively cheap to carry with us a bit longer, which I think is true.)

josuah · 2020-06-14T19:18:50Z

byte.h

+extern unsigned int byte_rchr(char *s, unsigned int n, int c);
+extern void byte_copy(char *to, unsigned int n, char *from);
+extern void byte_copyr(char *to, unsigned int n, char *from);
+extern void byte_zero(char *s, unsigned int n);



http://git.skarnet.org/cgi-bin/cgit.cgi/skalibs/tree/src/include/skalibs/bytestr.h

This above provides example on how to convert DJB-style functions to <string.h> ones.

Not in this PR ;)

josuah · 2020-11-14T00:24:03Z

Same logics would go for the case_*() functions.

Makefile

str.h

DerDakon · 2022-07-04T18:27:14Z

I now also took care of byte_copy() and byte_copyr().

josuah

Thankfully DJB introduced functions compatible with the libc, so this gets straightforward: a shallow renaming.

Any type of memory get's passed here, and newer compilers don't like the missing typecast.

While at it change the function API to C89 type and remove register specifications.

No implementation has ever existed in the code.

The compiler will know when this helps or not.

There should be no code that ever comes close to this size, but it will not cost us anything on most platforms.

DerDakon added the development label May 24, 2020

DerDakon requested review from leahneukirchen, schmonz, alanpost, jlmuir, mbhangui, xenotrope and josuah May 24, 2020 13:35

DerDakon marked this pull request as draft May 24, 2020 13:35

josuah reviewed May 24, 2020

View reviewed changes

qmail-send.c Outdated Show resolved Hide resolved

josuah approved these changes May 24, 2020

View reviewed changes

str_chr.c.gnu Outdated Show resolved Hide resolved

DerDakon force-pushed the Dakon-remove-strchr branch from 7acd813 to 5f1d18f Compare June 3, 2020 18:02

DerDakon force-pushed the Dakon-remove-strchr branch from 5f1d18f to da136ed Compare June 14, 2020 16:29

josuah self-requested a review June 14, 2020 19:01

josuah marked this pull request as ready for review June 14, 2020 19:01

josuah approved these changes Jun 14, 2020

View reviewed changes

josuah reviewed Jun 14, 2020

View reviewed changes

DerDakon force-pushed the Dakon-remove-strchr branch from da136ed to 1c3c9bb Compare June 18, 2020 17:19

DerDakon force-pushed the Dakon-remove-strchr branch 2 times, most recently from 70eb7d6 to c64752d Compare July 8, 2020 17:41

DerDakon force-pushed the Dakon-remove-strchr branch from c64752d to 9018e91 Compare September 26, 2020 11:59

schmonz removed the request for review from alanpost November 3, 2020 15:02

DerDakon force-pushed the Dakon-remove-strchr branch from 9018e91 to d572478 Compare November 9, 2020 17:27

DerDakon force-pushed the Dakon-remove-strchr branch from d572478 to 910f8ee Compare November 14, 2020 08:25

schmonz reviewed Dec 3, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

schmonz reviewed Dec 3, 2020

View reviewed changes

str.h Show resolved Hide resolved

DerDakon force-pushed the Dakon-remove-strchr branch from 910f8ee to 44ac341 Compare December 3, 2020 16:03

DerDakon requested a review from josuah July 4, 2022 18:25

DerDakon mentioned this pull request Jul 4, 2022

fix usage of most POSIX functions #65

Open

mbhangui approved these changes Jul 4, 2022

View reviewed changes

josuah approved these changes Jul 4, 2022

View reviewed changes

DerDakon force-pushed the Dakon-remove-strchr branch from de3f2f8 to b228fc8 Compare October 16, 2022 13:28

DerDakon force-pushed the Dakon-remove-strchr branch from b228fc8 to ce9b2ad Compare August 20, 2023 06:12

DerDakon added 13 commits April 26, 2024 21:22

tests: add test for str_chr()

2017356

replace str_chr() implementation with strchr()

884f450

tests: add test for str_start()

6372459

replace str_start() with strncmp()

e388d12

tests: add tests for str_rchr()

50931ed

replace str_rchr() with strrchr()

00dfaf3

tests: add tests for case_diffs()

fa9357d

replace case_diffs() with strcasecmp()

2e0574d

tests: add tests for byte_copy()

5326555

replace byte_copy() with memcpy()

17ec0c7

tests: add tests for byte_chr()

f754a5b

replace byte_chr() with memchr()

c82c387

use "const void *" for first argument of byte_chr()

7a6c093

DerDakon force-pushed the Dakon-remove-strchr branch from ce9b2ad to 6e72c06 Compare April 26, 2024 20:21

DerDakon added 8 commits April 28, 2024 11:06

byte_zero: take void* as pointer argument

2b7780f

Any type of memory get's passed here, and newer compilers don't like the missing typecast.

add parameters to case.h declarations

9354772

While at it change the function API to C89 type and remove register specifications.

add parameters to byte.h declarations

1b46f6b

remove stale declaration case_startb() from case.h and case.3

b9fa1bf

No implementation has ever existed in the code.

remove manually unrolled loops in byte*.c

2472018

The compiler will know when this helps or not.

make input argument of byte_copyr() const

f301756

replace byte_zero() with explicit_bzero() if available

d8b0800

use size_t for length in API of byte_*()

1111eaf

There should be no code that ever comes close to this size, but it will not cost us anything on most platforms.

DerDakon force-pushed the Dakon-remove-strchr branch from 6e72c06 to 1111eaf Compare April 28, 2024 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework the remaining str_* functions #143

Rework the remaining str_* functions #143

DerDakon commented May 24, 2020

josuah left a comment

josuah left a comment

josuah Jun 14, 2020

josuah Jun 14, 2020

DerDakon Dec 22, 2020

josuah Jun 14, 2020 •

edited

DerDakon Jun 14, 2020

josuah Jun 14, 2020

DerDakon Jun 15, 2020

schmonz Jan 22, 2021

josuah Jun 14, 2020

DerDakon Jun 14, 2020

josuah commented Nov 14, 2020

DerDakon commented Jul 4, 2022

josuah left a comment

Rework the remaining str_* functions #143

Are you sure you want to change the base?

Rework the remaining str_* functions #143

Conversation

DerDakon commented May 24, 2020

josuah left a comment

Choose a reason for hiding this comment

josuah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josuah Jun 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josuah commented Nov 14, 2020

DerDakon commented Jul 4, 2022

josuah left a comment

Choose a reason for hiding this comment

josuah Jun 14, 2020 •

edited