-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework the remaining str_* functions #143
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good to me, except GNU_SOURCE, but is optional so I push my Go.
7acd813
to
5f1d18f
Compare
5f1d18f
to
da136ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
void case_lowerb(s,len) | ||
char *s; | ||
unsigned int len; | ||
void case_lowerb(char *s, unsigned int len) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsigned int len
feels like a size_t
, let's see if the int
-> size_t
commit id up or downstream...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I would like to see it in a different PR on top of this, best once this is merged to avoid duplicate work.
byte_zero.c
Outdated
void byte_zero(s,n) | ||
char *s; | ||
register unsigned int n; | ||
void byte_zero(char *s, unsigned int n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
It looks like there is a hard time getting this not optimized away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
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); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in this PR ;)
da136ed
to
1c3c9bb
Compare
70eb7d6
to
c64752d
Compare
c64752d
to
9018e91
Compare
9018e91
to
d572478
Compare
Same logics would go for the case_*() functions. |
d572478
to
910f8ee
Compare
910f8ee
to
44ac341
Compare
I now also took care of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thankfully DJB introduced functions compatible with the libc, so this gets straightforward: a shallow renaming.
de3f2f8
to
b228fc8
Compare
b228fc8
to
ce9b2ad
Compare
ce9b2ad
to
6e72c06
Compare
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.
6e72c06
to
1111eaf
Compare
This is not really intended to be merged as-is, it should be a point of discussion.