Page MenuHomeFreeBSD
Differential D19072

Fix initial exec TLS mode for dynamically loaded shared objects.
ClosedPublic
Actions

Authored by kib on Feb 3 2019, 9:11 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 3, 8:16 AM
Unknown Object (File)
Mar 12 2025, 7:05 PM
Unknown Object (File)
Mar 12 2025, 6:33 PM
Unknown Object (File)
Mar 8 2025, 2:31 PM
Unknown Object (File)
Mar 6 2025, 11:57 AM
Unknown Object (File)
Mar 5 2025, 9:35 PM
Unknown Object (File)
Mar 5 2025, 8:23 AM
Unknown Object (File)
Mar 4 2025, 7:02 PM
View All Files
Subscribers
andrew
aryeeteygerald_rogers.com
bdragon
dim
dumbbell
emaste
• ian
imp

Details

Reviewers
None
Commits
rS345703: Fix initial exec TLS mode for dynamically loaded shared objects.
Summary

If the DF_STATIC_TLS dynamic flag is set, rtld tries to allocate TLS in static space. If there is no space left, the dlopen(3) fails. If space if allocated, initial content from PT_TLS segment is distributed to all threads' pcbs.

See https://www.redhat.com/archives/phil-list/2003-February/msg00077.html for plain language explanation.
I am unsure about https://reviews.llvm.org/D33041, I did observed that lld 6.0.1 does not set DF_STATIC_TLS. IMO this is a bug.

The test case is https://github.com/dumbbell/test-tls-initial-exec. I only compiled and somewhat tested patch on amd64, and patch have untested bits for i386.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 22411

Event Timeline

kib created this revision.Feb 3 2019, 9:11 PM
Herald added subscribers: bdragon, imp. · View Herald TranscriptFeb 3 2019, 9:11 PM
Harbormaster completed remote builds in B22333: Diff 53566.Feb 3 2019, 9:11 PM
emaste added a comment.Feb 3 2019, 10:14 PM

Should we encourage lld to revive the https://reviews.llvm.org/D33041 patch?

kib added a comment.Feb 3 2019, 10:25 PM
In D19072#407798, @emaste wrote:

Should we encourage lld to revive the https://reviews.llvm.org/D33041 patch?

Lets discuss it. They did not stated why DF_STATIC_TLS was considered as 'not needed'.

emaste added a comment.Feb 4 2019, 9:56 PM
In D19072#407799, @kib wrote:

Lets discuss it. They did not stated why DF_STATIC_TLS was considered as 'not needed'.

See the ongoing discussion in the LLVM thread - glibc and musl do not use the flag

glibc used to use it, but doesn't since 2002
(2430d57a13f4f10312e13c58962cd9104e6428fd).

As far as I can tell glibc originally used the flag only to signal that the .so should not be opened by dlopen, and the referenced commit changed it to checking explicitly for TLS use. Later changes for dlopen w/ static TLS did not check the flag.

kib updated this revision to Diff 53726.Feb 9 2019, 11:42 AM

Fix quite a bug in the loading of depended objects.
Restructure to avoid coding current thread offset calculation twice.

Harbormaster completed remote builds in B22411: Diff 53726.Feb 9 2019, 11:42 AM
kib updated this revision to Diff 53919.Feb 14 2019, 2:17 PM

Provide implementations for all arches.

Herald added a subscriber: andrew. · View Herald TranscriptFeb 14 2019, 2:17 PM
Harbormaster completed remote builds in B22500: Diff 53919.Feb 14 2019, 2:17 PM
emaste added a comment.Feb 19 2019, 5:45 PM

llvm commit in https://reviews.llvm.org/D57821, @dim reasonable to backport in your opinion?

emaste added a comment.Feb 19 2019, 5:47 PM
In D19072#411770, @emaste wrote:

llvm commit in https://reviews.llvm.org/D57821, @dim reasonable to backport in your opinion?

and https://reviews.llvm.org/D57749

dim added a comment.Feb 19 2019, 9:30 PM
In D19072#411771, @emaste wrote:
In D19072#411770, @emaste wrote:

llvm commit in https://reviews.llvm.org/D57821, @dim reasonable to backport in your opinion?

and https://reviews.llvm.org/D57749

Yes, those should be fine; can be imported independently of this revision.

aryeeteygerald_rogers.com added a subscriber: aryeeteygerald_rogers.com.Mar 28 2019, 1:08 AM
• ian added a subscriber: • ian.Mar 29 2019, 3:13 PM
This comment was removed by • ian.
• ian added a comment.Mar 29 2019, 3:14 PM

I haven't reviewed this change, but I did run-test it (on armv7 only). I updated to r345693 then applied the patch from this review, and ran the test code from https://github.com/emaste/test-tls-initial-exec. The test failed before patching and worked afterwards.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 29 2019, 5:53 PM
Closed by commit rS345703: Fix initial exec TLS mode for dynamically loaded shared objects. (authored by kib). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
lib/
libc/
amd64/
46 lines
gen/
1 line
24 lines
i386/
46 lines
include/
3 lines
sys/
1 line
libthr/
6 lines
arch/
amd64/
include/
46 lines
i386/
include/
46 lines
1 line
thread/
33 lines
2 lines
libexec/
rtld-elf/
2 lines
42 lines

Diff 53726

View Options

lib/libc/amd64/static_tls.h

Show First 20 Lines • Show All 43 Lines • ▼ Show 20 Lines
#include <sys/linker.h> #include <sys/linker.h>
#include <sys/stat.h> #include <sys/stat.h>
#include <sys/module.h> #include <sys/module.h>
#define FREEBSD_ELF #define FREEBSD_ELF
#include <ctype.h> #include <ctype.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h>
#include <fts.h> #include <fts.h>
#include <stdbool.h> #include <stdbool.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include <machine/elf.h> #include <machine/elf.h>
▲ Show 20 Lines • Show All 69 Lines • ▼ Show 20 Lines
* A string is stored as 1-byte length plus data, no padding * A string is stored as 1-byte length plus data, no padding
*/ */
static int static int
record_string(const char *str) record_string(const char *str)
{ {
int error; int error;
size_t len; size_t len;
u_char val; u_char val;
jrtc27Unsubmitted
Done
Inline Actions

Leave this alone

jrtc27: Leave this alone
if (dflag) if (dflag)
return (0); return (0);
val = len = strlen(str); val = len = strlen(str);
if (len > 255) if (len > 255)
errx(1, "string %s too long", str); errx(1, "string %s too long", str);
error = record_buf(&val, sizeof(val)); error = record_buf(&val, sizeof(val));
if (error != 0) if (error != 0)
return (error); return (error);
▲ Show 20 Lines • Show All 506 Lines • ▼ Show 20 Linesmaketempfile(char *dest, const char *root)
fd = mkstemp(dest); fd = mkstemp(dest);
if (fd < 0) if (fd < 0)
return (NULL); return (NULL);
fchmod(fd, 0644); /* nothing secret in the file */ fchmod(fd, 0644); /* nothing secret in the file */
return (fdopen(fd, "w+")); return (fdopen(fd, "w+"));
} }
static int
copyfile(char *src, char *dst) {
jrtc27Unsubmitted
Done
Inline Actions

style(9) requires the opening curly brace for functions to be on its own line

jrtc27: style(9) requires the opening curly brace for functions to be on its own line
jrtc27Unsubmitted
Done
Inline Actions

Not const char *?...

jrtc27: Not `const char *`?...
int retval = 0;
int src_fd, dst_fd;
src_fd = open(src, O_RDONLY);
if (src_fd < 0) {
jrtc27Unsubmitted
Done
Inline Actions

Why is src still a non-const char *?

jrtc27: Why is `src` still a non-const `char *`?
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

Sorry, I rewrote the prototype a dozen times when I was fighting with indent, and I must have hit u one time too many.

mqudsi_neosmart.net: Sorry, I rewrote the prototype a dozen times when I was fighting with indent, and I must have…
warn("can't open %s", src);
return 1;
jrtc27Unsubmitted
Done
Inline Actions

style(9) requires parentheses around all return values

jrtc27: style(9) requires parentheses around all return values
}
jrtc27Unsubmitted
Done
Inline Actions

ival is not a helpful variable name

jrtc27: ival is not a helpful variable name
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

When doing "declarations up front" where a value may be repurposed a few times, I've always noticed a preference for less-specific names (the name is copied from elsewhere in the same file, actually) and I was trying to stick to the conventions I discovered in the existing codebase as I was exposed to them. I'll rename it to be more specific, happy to "be the change you want to see" :)

mqudsi_neosmart.net: When doing "declarations up front" where a value may be repurposed a few times, I've always…
jrtc27Unsubmitted
Done
Inline Actions

ival just tells you it's an integer. ret and err are often used to hint that they're return values/error returns from something, which at least tells you more than the type. The only place ival is used in this file is for writing the version integer, 1, as the first integer in the hints file.

jrtc27: `ival` just tells you it's an integer. `ret` and `err` are often used to hint that they're…
dst_fd = open(dst, O_WRONLY | O_CREAT | O_TRUNC);
if (dst_fd < 0) {
warn("can't create %s", dst);
close(src_fd);
return 1;
jrtc27Unsubmitted
Done
Inline Actions

Now you've changed to bool these are the wrong way round

jrtc27: Now you've changed to bool these are the wrong way round
}
if (copy_file_range(src_fd, NULL, dst_fd, NULL, SSIZE_MAX, 0) < 0) {
jrtc27Unsubmitted
Done
Inline Actions

If it succeeds, the call returns the number of bytes copied, which can be
fewer than len. Returning fewer bytes than len does not necessarily in-
dicate that EOF was reached. However, a return of zero for a non-zero
len argument indicates that the offset for infd is at or beyond EOF.
copy_file_range() should be used in a loop until copying of the desired
byte range has been completed. If an error has occurred, a -1 is re-
turned and the error code is placed in the global variable errno.

(https://www.freebsd.org/cgi/man.cgi?query=copy_file_range)

jrtc27: > If it succeeds, the call returns the number of bytes copied, which can be > fewer than len.
warn("can't copy %s to %s", src, dst);
retval = 1;
}
close(src_fd);
close(dst_fd);
return retval;
jrtc27Unsubmitted
Done
Inline Actions

This is now a 12 space indent, not a 4 space one

jrtc27: This is now a 12 space indent, not a 4 space one
}
jrtc27Unsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

jrtc27: style(9) is 4 spaces indent for continuation lines
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

You're right. I thought indent(1) respected style(9)? There were a few other incorrect changes it made I had to fix by hand, but I missed this one.

mqudsi_neosmart.net: > style(9) is 4 spaces indent for continuation lines You're right. I thought indent(1)…
jrtc27Unsubmitted
Done
Inline Actions

indent(1) isn't perfect, no. In this case, it defaults to -lp, whereas style(9) wants -nlp.

jrtc27: indent(1) isn't perfect, no. In this case, it defaults to -lp, whereas style(9) wants -nlp.
emasteUnsubmitted
Done
Inline Actions

You can also give clang-format a try. It's not able to exactly match style(9) but gets pretty close.

I usually redirect the formatted output to a temp file, diff the input and formatted files, and iteratively edit.

emaste: You can also give //clang-format// a try. It's not able to exactly match style(9) but gets…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

You can also give clang-format a try. It's not able to exactly match style(9) but gets pretty close.

Hi Ed! Yeah, I just discovered that FreeBSD has an (experimental?) .clang-format file in the project root that gave me much more sane results than ident(1) (with -nlp), but clang-format was responsible for the 12-space continuation indent jrtc27@ just pointed out (and it seems to have an unreasonably short hard limit to the line length?). Getting there!

mqudsi_neosmart.net: > You can also give //clang-format// a try. It's not able to exactly match style(9) but gets…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

mqudsi_neosmart.net: > style(9) is 4 spaces indent for continuation lines
jrtc27Unsubmitted
Done
Inline Actions

If you change this to > 0 then you'll exit the loop for < 0

jrtc27: If you change this to > 0 then you'll exit the loop for < 0
static char xrefname[MAXPATHLEN], tempname[MAXPATHLEN]; static char xrefname[MAXPATHLEN], tempname[MAXPATHLEN];
static void static void
usage(void) usage(void)
{ {
fprintf(stderr, "%s\n", fprintf(stderr, "%s\n",
"usage: kldxref [-Rdv] [-f hintsfile] path ..." "usage: kldxref [-Rdv] [-f hintsfile] path ..."
); );
jrtc27Unsubmitted
Done
Inline Actions

... and then this could just be return (ival_renamed == 0), if you keep the return value at all, or

if (new_ival_name < 0) {
        warn(...);
        return (false);
}

return (true);
jrtc27: ... and then this could just be `return (ival_renamed == 0)`, if you keep the return value at…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

I really prefer to keep error handling as close as possible to the code that triggered it, both to make it easier to review and to prevent refactoring/maintenance issues down the line. If I were to move the warn to the end, someone could potentially introduce another syscall that would clobber errno. Actually, I think that's already the case since this would come after the close(2) calls. But just checking the renamed variable to bubble up the return code would be fine.

mqudsi_neosmart.net: I really prefer to keep error handling as close as possible to the code that triggered it, both…
jrtc27Unsubmitted
Done
Inline Actions

Oh, right, warn implicitly checks errno. You can still put the if < 0 warn directly after the loop though and avoid the nesting.

jrtc27: Oh, right, warn implicitly checks errno. You can still put the `if < 0 warn` directly after the…
exit(1); exit(1);
} }
static int static int
compare(const FTSENT *const *a, const FTSENT *const *b) compare(const FTSENT *const *a, const FTSENT *const *b)
{ {
if ((*a)->fts_info == FTS_D && (*b)->fts_info != FTS_D) if ((*a)->fts_info == FTS_D && (*b)->fts_info != FTS_D)
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Linesmain(int argc, char *argv[])
for (;;) { for (;;) {
p = fts_read(ftsp); p = fts_read(ftsp);
if ((p == NULL || p->fts_info == FTS_D) && fxref) { if ((p == NULL || p->fts_info == FTS_D) && fxref) {
/* close and rename the current hint file */ /* close and rename the current hint file */
fclose(fxref); fclose(fxref);
fxref = NULL; fxref = NULL;
if (reccnt != 0) { if (reccnt != 0) {
rename(tempname, xrefname); if (rename(tempname, xrefname) != 0) {
/* handle temp file being on a different filesystem */
if (errno == EXDEV) {
/* copyfile reports its own errors */
copyfile(tempname, xrefname);
jrtc27Unsubmitted
Done
Inline Actions

Having a return value for a static function that's always ignored is a bit odd

jrtc27: Having a return value for a static function that's always ignored is a bit odd
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

I agree, which is why I made sure to document why it's ignored here. But the return value is there to be more generic/useful (and it doesn't hurt besides looking odd without that comment to explain). If an operation needed to abort after a failed file copy, the return code would then be used to abort but that's not the case right here.

mqudsi_neosmart.net: I agree, which is why I made sure to document why it's ignored here. But the return value is…
jrtc27Unsubmitted
Done
Inline Actions

Ok, but that's not quite what your comment says. Your comment says it reports its own errors, but that would also be true of a void-returning copyfile.

jrtc27: Ok, but that's not quite what your comment says. Your comment says it reports its own errors…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

If you start with the assumption that the only reason to check the return code would be to either report an error or perform some sort of control flow based off the result, if the comment says it does its own error checking then the fact that there's no control flow logic here indicates that we're just going to continue. I'll make it void if that's what you prefer, though.

mqudsi_neosmart.net: If you start with the assumption that the only reason to check the return code would be to…
emasteUnsubmitted
Done
Inline Actions

If you start with the assumption that the only reason to check the return code would be to either report an error or perform some sort of control flow based off the result, if the comment says it does its own error checking then the fact that there's no control flow logic here indicates that we're just going to continue. I'll make it void if that's what you prefer, though.

emaste: > If you start with the assumption that the only reason to check the return code would be to…
emasteUnsubmitted
Done
Inline Actions

Oops, did not mean to add that.

emaste: Oops, did not mean to add that.
jrtc27Unsubmitted
Done
Inline Actions

I still think this approach is a bad design

jrtc27: I still think this approach is a bad design
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

As I mentioned, if you just want the static function to be void, I'm happy to change it. Would that work for you?

mqudsi_neosmart.net: As I mentioned, if you just want the static function to be void, I'm happy to change it. Would…
jrtc27Unsubmitted
Done
Inline Actions

Yes, that would be ok

jrtc27: Yes, that would be ok
unlink(tempname);
} else {
unlink(tempname);
jrtc27Unsubmitted
Done
Inline Actions

If you unlink after warning then it can be hoisted

jrtc27: If you unlink after warning then it can be hoisted
warn("can't rename %s to %s", xrefname, tempname);
}
}
} else { } else {
/* didn't find any entry, ignore this file */ /* didn't find any entry, ignore this file */
unlink(tempname); unlink(tempname);
unlink(xrefname); unlink(xrefname);
} }
} }
if (p == NULL) if (p == NULL)
break; break;
Show All 25 Lines
View Options

lib/libc/gen/Symbol.map

Show First 20 Lines • Show All 43 Lines • ▼ Show 20 Lines
#include <sys/linker.h> #include <sys/linker.h>
#include <sys/stat.h> #include <sys/stat.h>
#include <sys/module.h> #include <sys/module.h>
#define FREEBSD_ELF #define FREEBSD_ELF
#include <ctype.h> #include <ctype.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h>
#include <fts.h> #include <fts.h>
#include <stdbool.h> #include <stdbool.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include <machine/elf.h> #include <machine/elf.h>
▲ Show 20 Lines • Show All 69 Lines • ▼ Show 20 Lines
* A string is stored as 1-byte length plus data, no padding * A string is stored as 1-byte length plus data, no padding
*/ */
static int static int
record_string(const char *str) record_string(const char *str)
{ {
int error; int error;
size_t len; size_t len;
u_char val; u_char val;
jrtc27Unsubmitted
Done
Inline Actions

Leave this alone

jrtc27: Leave this alone
if (dflag) if (dflag)
return (0); return (0);
val = len = strlen(str); val = len = strlen(str);
if (len > 255) if (len > 255)
errx(1, "string %s too long", str); errx(1, "string %s too long", str);
error = record_buf(&val, sizeof(val)); error = record_buf(&val, sizeof(val));
if (error != 0) if (error != 0)
return (error); return (error);
▲ Show 20 Lines • Show All 506 Lines • ▼ Show 20 Linesmaketempfile(char *dest, const char *root)
fd = mkstemp(dest); fd = mkstemp(dest);
if (fd < 0) if (fd < 0)
return (NULL); return (NULL);
fchmod(fd, 0644); /* nothing secret in the file */ fchmod(fd, 0644); /* nothing secret in the file */
return (fdopen(fd, "w+")); return (fdopen(fd, "w+"));
} }
static int
copyfile(char *src, char *dst) {
jrtc27Unsubmitted
Done
Inline Actions

style(9) requires the opening curly brace for functions to be on its own line

jrtc27: style(9) requires the opening curly brace for functions to be on its own line
jrtc27Unsubmitted
Done
Inline Actions

Not const char *?...

jrtc27: Not `const char *`?...
int retval = 0;
int src_fd, dst_fd;
src_fd = open(src, O_RDONLY);
if (src_fd < 0) {
jrtc27Unsubmitted
Done
Inline Actions

Why is src still a non-const char *?

jrtc27: Why is `src` still a non-const `char *`?
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

Sorry, I rewrote the prototype a dozen times when I was fighting with indent, and I must have hit u one time too many.

mqudsi_neosmart.net: Sorry, I rewrote the prototype a dozen times when I was fighting with indent, and I must have…
warn("can't open %s", src);
return 1;
jrtc27Unsubmitted
Done
Inline Actions

style(9) requires parentheses around all return values

jrtc27: style(9) requires parentheses around all return values
}
jrtc27Unsubmitted
Done
Inline Actions

ival is not a helpful variable name

jrtc27: ival is not a helpful variable name
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

When doing "declarations up front" where a value may be repurposed a few times, I've always noticed a preference for less-specific names (the name is copied from elsewhere in the same file, actually) and I was trying to stick to the conventions I discovered in the existing codebase as I was exposed to them. I'll rename it to be more specific, happy to "be the change you want to see" :)

mqudsi_neosmart.net: When doing "declarations up front" where a value may be repurposed a few times, I've always…
jrtc27Unsubmitted
Done
Inline Actions

ival just tells you it's an integer. ret and err are often used to hint that they're return values/error returns from something, which at least tells you more than the type. The only place ival is used in this file is for writing the version integer, 1, as the first integer in the hints file.

jrtc27: `ival` just tells you it's an integer. `ret` and `err` are often used to hint that they're…
dst_fd = open(dst, O_WRONLY | O_CREAT | O_TRUNC);
if (dst_fd < 0) {
warn("can't create %s", dst);
close(src_fd);
return 1;
jrtc27Unsubmitted
Done
Inline Actions

Now you've changed to bool these are the wrong way round

jrtc27: Now you've changed to bool these are the wrong way round
}
if (copy_file_range(src_fd, NULL, dst_fd, NULL, SSIZE_MAX, 0) < 0) {
jrtc27Unsubmitted
Done
Inline Actions

If it succeeds, the call returns the number of bytes copied, which can be
fewer than len. Returning fewer bytes than len does not necessarily in-
dicate that EOF was reached. However, a return of zero for a non-zero
len argument indicates that the offset for infd is at or beyond EOF.
copy_file_range() should be used in a loop until copying of the desired
byte range has been completed. If an error has occurred, a -1 is re-
turned and the error code is placed in the global variable errno.

(https://www.freebsd.org/cgi/man.cgi?query=copy_file_range)

jrtc27: > If it succeeds, the call returns the number of bytes copied, which can be > fewer than len.
warn("can't copy %s to %s", src, dst);
retval = 1;
}
close(src_fd);
close(dst_fd);
return retval;
jrtc27Unsubmitted
Done
Inline Actions

This is now a 12 space indent, not a 4 space one

jrtc27: This is now a 12 space indent, not a 4 space one
}
jrtc27Unsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

jrtc27: style(9) is 4 spaces indent for continuation lines
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

You're right. I thought indent(1) respected style(9)? There were a few other incorrect changes it made I had to fix by hand, but I missed this one.

mqudsi_neosmart.net: > style(9) is 4 spaces indent for continuation lines You're right. I thought indent(1)…
jrtc27Unsubmitted
Done
Inline Actions

indent(1) isn't perfect, no. In this case, it defaults to -lp, whereas style(9) wants -nlp.

jrtc27: indent(1) isn't perfect, no. In this case, it defaults to -lp, whereas style(9) wants -nlp.
emasteUnsubmitted
Done
Inline Actions

You can also give clang-format a try. It's not able to exactly match style(9) but gets pretty close.

I usually redirect the formatted output to a temp file, diff the input and formatted files, and iteratively edit.

emaste: You can also give //clang-format// a try. It's not able to exactly match style(9) but gets…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

You can also give clang-format a try. It's not able to exactly match style(9) but gets pretty close.

Hi Ed! Yeah, I just discovered that FreeBSD has an (experimental?) .clang-format file in the project root that gave me much more sane results than ident(1) (with -nlp), but clang-format was responsible for the 12-space continuation indent jrtc27@ just pointed out (and it seems to have an unreasonably short hard limit to the line length?). Getting there!

mqudsi_neosmart.net: > You can also give //clang-format// a try. It's not able to exactly match style(9) but gets…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

mqudsi_neosmart.net: > style(9) is 4 spaces indent for continuation lines
jrtc27Unsubmitted
Done
Inline Actions

If you change this to > 0 then you'll exit the loop for < 0

jrtc27: If you change this to > 0 then you'll exit the loop for < 0
static char xrefname[MAXPATHLEN], tempname[MAXPATHLEN]; static char xrefname[MAXPATHLEN], tempname[MAXPATHLEN];
static void static void
usage(void) usage(void)
{ {
fprintf(stderr, "%s\n", fprintf(stderr, "%s\n",
"usage: kldxref [-Rdv] [-f hintsfile] path ..." "usage: kldxref [-Rdv] [-f hintsfile] path ..."
); );
jrtc27Unsubmitted
Done
Inline Actions

... and then this could just be return (ival_renamed == 0), if you keep the return value at all, or

if (new_ival_name < 0) {
        warn(...);
        return (false);
}

return (true);
jrtc27: ... and then this could just be `return (ival_renamed == 0)`, if you keep the return value at…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

I really prefer to keep error handling as close as possible to the code that triggered it, both to make it easier to review and to prevent refactoring/maintenance issues down the line. If I were to move the warn to the end, someone could potentially introduce another syscall that would clobber errno. Actually, I think that's already the case since this would come after the close(2) calls. But just checking the renamed variable to bubble up the return code would be fine.

mqudsi_neosmart.net: I really prefer to keep error handling as close as possible to the code that triggered it, both…
jrtc27Unsubmitted
Done
Inline Actions

Oh, right, warn implicitly checks errno. You can still put the if < 0 warn directly after the loop though and avoid the nesting.

jrtc27: Oh, right, warn implicitly checks errno. You can still put the `if < 0 warn` directly after the…
exit(1); exit(1);
} }
static int static int
compare(const FTSENT *const *a, const FTSENT *const *b) compare(const FTSENT *const *a, const FTSENT *const *b)
{ {
if ((*a)->fts_info == FTS_D && (*b)->fts_info != FTS_D) if ((*a)->fts_info == FTS_D && (*b)->fts_info != FTS_D)
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Linesmain(int argc, char *argv[])
for (;;) { for (;;) {
p = fts_read(ftsp); p = fts_read(ftsp);
if ((p == NULL || p->fts_info == FTS_D) && fxref) { if ((p == NULL || p->fts_info == FTS_D) && fxref) {
/* close and rename the current hint file */ /* close and rename the current hint file */
fclose(fxref); fclose(fxref);
fxref = NULL; fxref = NULL;
if (reccnt != 0) { if (reccnt != 0) {
rename(tempname, xrefname); if (rename(tempname, xrefname) != 0) {
/* handle temp file being on a different filesystem */
if (errno == EXDEV) {
/* copyfile reports its own errors */
copyfile(tempname, xrefname);
jrtc27Unsubmitted
Done
Inline Actions

Having a return value for a static function that's always ignored is a bit odd

jrtc27: Having a return value for a static function that's always ignored is a bit odd
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

I agree, which is why I made sure to document why it's ignored here. But the return value is there to be more generic/useful (and it doesn't hurt besides looking odd without that comment to explain). If an operation needed to abort after a failed file copy, the return code would then be used to abort but that's not the case right here.

mqudsi_neosmart.net: I agree, which is why I made sure to document why it's ignored here. But the return value is…
jrtc27Unsubmitted
Done
Inline Actions

Ok, but that's not quite what your comment says. Your comment says it reports its own errors, but that would also be true of a void-returning copyfile.

jrtc27: Ok, but that's not quite what your comment says. Your comment says it reports its own errors…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

If you start with the assumption that the only reason to check the return code would be to either report an error or perform some sort of control flow based off the result, if the comment says it does its own error checking then the fact that there's no control flow logic here indicates that we're just going to continue. I'll make it void if that's what you prefer, though.

mqudsi_neosmart.net: If you start with the assumption that the only reason to check the return code would be to…
emasteUnsubmitted
Done
Inline Actions

If you start with the assumption that the only reason to check the return code would be to either report an error or perform some sort of control flow based off the result, if the comment says it does its own error checking then the fact that there's no control flow logic here indicates that we're just going to continue. I'll make it void if that's what you prefer, though.

emaste: > If you start with the assumption that the only reason to check the return code would be to…
emasteUnsubmitted
Done
Inline Actions

Oops, did not mean to add that.

emaste: Oops, did not mean to add that.
jrtc27Unsubmitted
Done
Inline Actions

I still think this approach is a bad design

jrtc27: I still think this approach is a bad design
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

As I mentioned, if you just want the static function to be void, I'm happy to change it. Would that work for you?

mqudsi_neosmart.net: As I mentioned, if you just want the static function to be void, I'm happy to change it. Would…
jrtc27Unsubmitted
Done
Inline Actions

Yes, that would be ok

jrtc27: Yes, that would be ok
unlink(tempname);
} else {
unlink(tempname);
jrtc27Unsubmitted
Done
Inline Actions

If you unlink after warning then it can be hoisted

jrtc27: If you unlink after warning then it can be hoisted
warn("can't rename %s to %s", xrefname, tempname);
}
}
} else { } else {
/* didn't find any entry, ignore this file */ /* didn't find any entry, ignore this file */
unlink(tempname); unlink(tempname);
unlink(xrefname); unlink(xrefname);
} }
} }
if (p == NULL) if (p == NULL)
break; break;
Show All 25 Lines
View Options

lib/libc/gen/elf_utils.c

Show First 20 Lines • Show All 43 Lines • ▼ Show 20 Lines
#include <sys/linker.h> #include <sys/linker.h>
#include <sys/stat.h> #include <sys/stat.h>
#include <sys/module.h> #include <sys/module.h>
#define FREEBSD_ELF #define FREEBSD_ELF
#include <ctype.h> #include <ctype.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h>
#include <fts.h> #include <fts.h>
#include <stdbool.h> #include <stdbool.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include <machine/elf.h> #include <machine/elf.h>
▲ Show 20 Lines • Show All 69 Lines • ▼ Show 20 Lines
* A string is stored as 1-byte length plus data, no padding * A string is stored as 1-byte length plus data, no padding
*/ */
static int static int
record_string(const char *str) record_string(const char *str)
{ {
int error; int error;
size_t len; size_t len;
u_char val; u_char val;
jrtc27Unsubmitted
Done
Inline Actions

Leave this alone

jrtc27: Leave this alone
if (dflag) if (dflag)
return (0); return (0);
val = len = strlen(str); val = len = strlen(str);
if (len > 255) if (len > 255)
errx(1, "string %s too long", str); errx(1, "string %s too long", str);
error = record_buf(&val, sizeof(val)); error = record_buf(&val, sizeof(val));
if (error != 0) if (error != 0)
return (error); return (error);
▲ Show 20 Lines • Show All 506 Lines • ▼ Show 20 Linesmaketempfile(char *dest, const char *root)
fd = mkstemp(dest); fd = mkstemp(dest);
if (fd < 0) if (fd < 0)
return (NULL); return (NULL);
fchmod(fd, 0644); /* nothing secret in the file */ fchmod(fd, 0644); /* nothing secret in the file */
return (fdopen(fd, "w+")); return (fdopen(fd, "w+"));
} }
static int
copyfile(char *src, char *dst) {
jrtc27Unsubmitted
Done
Inline Actions

style(9) requires the opening curly brace for functions to be on its own line

jrtc27: style(9) requires the opening curly brace for functions to be on its own line
jrtc27Unsubmitted
Done
Inline Actions

Not const char *?...

jrtc27: Not `const char *`?...
int retval = 0;
int src_fd, dst_fd;
src_fd = open(src, O_RDONLY);
if (src_fd < 0) {
jrtc27Unsubmitted
Done
Inline Actions

Why is src still a non-const char *?

jrtc27: Why is `src` still a non-const `char *`?
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

Sorry, I rewrote the prototype a dozen times when I was fighting with indent, and I must have hit u one time too many.

mqudsi_neosmart.net: Sorry, I rewrote the prototype a dozen times when I was fighting with indent, and I must have…
warn("can't open %s", src);
return 1;
jrtc27Unsubmitted
Done
Inline Actions

style(9) requires parentheses around all return values

jrtc27: style(9) requires parentheses around all return values
}
jrtc27Unsubmitted
Done
Inline Actions

ival is not a helpful variable name

jrtc27: ival is not a helpful variable name
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

When doing "declarations up front" where a value may be repurposed a few times, I've always noticed a preference for less-specific names (the name is copied from elsewhere in the same file, actually) and I was trying to stick to the conventions I discovered in the existing codebase as I was exposed to them. I'll rename it to be more specific, happy to "be the change you want to see" :)

mqudsi_neosmart.net: When doing "declarations up front" where a value may be repurposed a few times, I've always…
jrtc27Unsubmitted
Done
Inline Actions

ival just tells you it's an integer. ret and err are often used to hint that they're return values/error returns from something, which at least tells you more than the type. The only place ival is used in this file is for writing the version integer, 1, as the first integer in the hints file.

jrtc27: `ival` just tells you it's an integer. `ret` and `err` are often used to hint that they're…
dst_fd = open(dst, O_WRONLY | O_CREAT | O_TRUNC);
if (dst_fd < 0) {
warn("can't create %s", dst);
close(src_fd);
return 1;
jrtc27Unsubmitted
Done
Inline Actions

Now you've changed to bool these are the wrong way round

jrtc27: Now you've changed to bool these are the wrong way round
}
if (copy_file_range(src_fd, NULL, dst_fd, NULL, SSIZE_MAX, 0) < 0) {
jrtc27Unsubmitted
Done
Inline Actions

If it succeeds, the call returns the number of bytes copied, which can be
fewer than len. Returning fewer bytes than len does not necessarily in-
dicate that EOF was reached. However, a return of zero for a non-zero
len argument indicates that the offset for infd is at or beyond EOF.
copy_file_range() should be used in a loop until copying of the desired
byte range has been completed. If an error has occurred, a -1 is re-
turned and the error code is placed in the global variable errno.

(https://www.freebsd.org/cgi/man.cgi?query=copy_file_range)

jrtc27: > If it succeeds, the call returns the number of bytes copied, which can be > fewer than len.
warn("can't copy %s to %s", src, dst);
retval = 1;
}
close(src_fd);
close(dst_fd);
return retval;
jrtc27Unsubmitted
Done
Inline Actions

This is now a 12 space indent, not a 4 space one

jrtc27: This is now a 12 space indent, not a 4 space one
}
jrtc27Unsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

jrtc27: style(9) is 4 spaces indent for continuation lines
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

You're right. I thought indent(1) respected style(9)? There were a few other incorrect changes it made I had to fix by hand, but I missed this one.

mqudsi_neosmart.net: > style(9) is 4 spaces indent for continuation lines You're right. I thought indent(1)…
jrtc27Unsubmitted
Done
Inline Actions

indent(1) isn't perfect, no. In this case, it defaults to -lp, whereas style(9) wants -nlp.

jrtc27: indent(1) isn't perfect, no. In this case, it defaults to -lp, whereas style(9) wants -nlp.
emasteUnsubmitted
Done
Inline Actions

You can also give clang-format a try. It's not able to exactly match style(9) but gets pretty close.

I usually redirect the formatted output to a temp file, diff the input and formatted files, and iteratively edit.

emaste: You can also give //clang-format// a try. It's not able to exactly match style(9) but gets…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

You can also give clang-format a try. It's not able to exactly match style(9) but gets pretty close.

Hi Ed! Yeah, I just discovered that FreeBSD has an (experimental?) .clang-format file in the project root that gave me much more sane results than ident(1) (with -nlp), but clang-format was responsible for the 12-space continuation indent jrtc27@ just pointed out (and it seems to have an unreasonably short hard limit to the line length?). Getting there!

mqudsi_neosmart.net: > You can also give //clang-format// a try. It's not able to exactly match style(9) but gets…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

mqudsi_neosmart.net: > style(9) is 4 spaces indent for continuation lines
jrtc27Unsubmitted
Done
Inline Actions

If you change this to > 0 then you'll exit the loop for < 0

jrtc27: If you change this to > 0 then you'll exit the loop for < 0
static char xrefname[MAXPATHLEN], tempname[MAXPATHLEN]; static char xrefname[MAXPATHLEN], tempname[MAXPATHLEN];
static void static void
usage(void) usage(void)
{ {
fprintf(stderr, "%s\n", fprintf(stderr, "%s\n",
"usage: kldxref [-Rdv] [-f hintsfile] path ..." "usage: kldxref [-Rdv] [-f hintsfile] path ..."
); );
jrtc27Unsubmitted
Done
Inline Actions

... and then this could just be return (ival_renamed == 0), if you keep the return value at all, or

if (new_ival_name < 0) {
        warn(...);
        return (false);
}

return (true);
jrtc27: ... and then this could just be `return (ival_renamed == 0)`, if you keep the return value at…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

I really prefer to keep error handling as close as possible to the code that triggered it, both to make it easier to review and to prevent refactoring/maintenance issues down the line. If I were to move the warn to the end, someone could potentially introduce another syscall that would clobber errno. Actually, I think that's already the case since this would come after the close(2) calls. But just checking the renamed variable to bubble up the return code would be fine.

mqudsi_neosmart.net: I really prefer to keep error handling as close as possible to the code that triggered it, both…
jrtc27Unsubmitted
Done
Inline Actions

Oh, right, warn implicitly checks errno. You can still put the if < 0 warn directly after the loop though and avoid the nesting.

jrtc27: Oh, right, warn implicitly checks errno. You can still put the `if < 0 warn` directly after the…
exit(1); exit(1);
} }
static int static int
compare(const FTSENT *const *a, const FTSENT *const *b) compare(const FTSENT *const *a, const FTSENT *const *b)
{ {
if ((*a)->fts_info == FTS_D && (*b)->fts_info != FTS_D) if ((*a)->fts_info == FTS_D && (*b)->fts_info != FTS_D)
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Linesmain(int argc, char *argv[])
for (;;) { for (;;) {
p = fts_read(ftsp); p = fts_read(ftsp);
if ((p == NULL || p->fts_info == FTS_D) && fxref) { if ((p == NULL || p->fts_info == FTS_D) && fxref) {
/* close and rename the current hint file */ /* close and rename the current hint file */
fclose(fxref); fclose(fxref);
fxref = NULL; fxref = NULL;
if (reccnt != 0) { if (reccnt != 0) {
rename(tempname, xrefname); if (rename(tempname, xrefname) != 0) {
/* handle temp file being on a different filesystem */
if (errno == EXDEV) {
/* copyfile reports its own errors */
copyfile(tempname, xrefname);
jrtc27Unsubmitted
Done
Inline Actions

Having a return value for a static function that's always ignored is a bit odd

jrtc27: Having a return value for a static function that's always ignored is a bit odd
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

I agree, which is why I made sure to document why it's ignored here. But the return value is there to be more generic/useful (and it doesn't hurt besides looking odd without that comment to explain). If an operation needed to abort after a failed file copy, the return code would then be used to abort but that's not the case right here.

mqudsi_neosmart.net: I agree, which is why I made sure to document why it's ignored here. But the return value is…
jrtc27Unsubmitted
Done
Inline Actions

Ok, but that's not quite what your comment says. Your comment says it reports its own errors, but that would also be true of a void-returning copyfile.

jrtc27: Ok, but that's not quite what your comment says. Your comment says it reports its own errors…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

If you start with the assumption that the only reason to check the return code would be to either report an error or perform some sort of control flow based off the result, if the comment says it does its own error checking then the fact that there's no control flow logic here indicates that we're just going to continue. I'll make it void if that's what you prefer, though.

mqudsi_neosmart.net: If you start with the assumption that the only reason to check the return code would be to…
emasteUnsubmitted
Done
Inline Actions

If you start with the assumption that the only reason to check the return code would be to either report an error or perform some sort of control flow based off the result, if the comment says it does its own error checking then the fact that there's no control flow logic here indicates that we're just going to continue. I'll make it void if that's what you prefer, though.

emaste: > If you start with the assumption that the only reason to check the return code would be to…
emasteUnsubmitted
Done
Inline Actions

Oops, did not mean to add that.

emaste: Oops, did not mean to add that.
jrtc27Unsubmitted
Done
Inline Actions

I still think this approach is a bad design

jrtc27: I still think this approach is a bad design
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

As I mentioned, if you just want the static function to be void, I'm happy to change it. Would that work for you?

mqudsi_neosmart.net: As I mentioned, if you just want the static function to be void, I'm happy to change it. Would…
jrtc27Unsubmitted
Done
Inline Actions

Yes, that would be ok

jrtc27: Yes, that would be ok
unlink(tempname);
} else {
unlink(tempname);
jrtc27Unsubmitted
Done
Inline Actions

If you unlink after warning then it can be hoisted

jrtc27: If you unlink after warning then it can be hoisted
warn("can't rename %s to %s", xrefname, tempname);
}
}
} else { } else {
/* didn't find any entry, ignore this file */ /* didn't find any entry, ignore this file */
unlink(tempname); unlink(tempname);
unlink(xrefname); unlink(xrefname);
} }
} }
if (p == NULL) if (p == NULL)
break; break;
Show All 25 Lines
View Options

lib/libc/i386/static_tls.h

Show First 20 Lines • Show All 43 Lines • ▼ Show 20 Lines
#include <sys/linker.h> #include <sys/linker.h>
#include <sys/stat.h> #include <sys/stat.h>
#include <sys/module.h> #include <sys/module.h>
#define FREEBSD_ELF #define FREEBSD_ELF
#include <ctype.h> #include <ctype.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h>
#include <fts.h> #include <fts.h>
#include <stdbool.h> #include <stdbool.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include <machine/elf.h> #include <machine/elf.h>
▲ Show 20 Lines • Show All 69 Lines • ▼ Show 20 Lines
* A string is stored as 1-byte length plus data, no padding * A string is stored as 1-byte length plus data, no padding
*/ */
static int static int
record_string(const char *str) record_string(const char *str)
{ {
int error; int error;
size_t len; size_t len;
u_char val; u_char val;
jrtc27Unsubmitted
Done
Inline Actions

Leave this alone

jrtc27: Leave this alone
if (dflag) if (dflag)
return (0); return (0);
val = len = strlen(str); val = len = strlen(str);
if (len > 255) if (len > 255)
errx(1, "string %s too long", str); errx(1, "string %s too long", str);
error = record_buf(&val, sizeof(val)); error = record_buf(&val, sizeof(val));
if (error != 0) if (error != 0)
return (error); return (error);
▲ Show 20 Lines • Show All 506 Lines • ▼ Show 20 Linesmaketempfile(char *dest, const char *root)
fd = mkstemp(dest); fd = mkstemp(dest);
if (fd < 0) if (fd < 0)
return (NULL); return (NULL);
fchmod(fd, 0644); /* nothing secret in the file */ fchmod(fd, 0644); /* nothing secret in the file */
return (fdopen(fd, "w+")); return (fdopen(fd, "w+"));
} }
static int
copyfile(char *src, char *dst) {
jrtc27Unsubmitted
Done
Inline Actions

style(9) requires the opening curly brace for functions to be on its own line

jrtc27: style(9) requires the opening curly brace for functions to be on its own line
jrtc27Unsubmitted
Done
Inline Actions

Not const char *?...

jrtc27: Not `const char *`?...
int retval = 0;
int src_fd, dst_fd;
src_fd = open(src, O_RDONLY);
if (src_fd < 0) {
jrtc27Unsubmitted
Done
Inline Actions

Why is src still a non-const char *?

jrtc27: Why is `src` still a non-const `char *`?
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

Sorry, I rewrote the prototype a dozen times when I was fighting with indent, and I must have hit u one time too many.

mqudsi_neosmart.net: Sorry, I rewrote the prototype a dozen times when I was fighting with indent, and I must have…
warn("can't open %s", src);
return 1;
jrtc27Unsubmitted
Done
Inline Actions

style(9) requires parentheses around all return values

jrtc27: style(9) requires parentheses around all return values
}
jrtc27Unsubmitted
Done
Inline Actions

ival is not a helpful variable name

jrtc27: ival is not a helpful variable name
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

When doing "declarations up front" where a value may be repurposed a few times, I've always noticed a preference for less-specific names (the name is copied from elsewhere in the same file, actually) and I was trying to stick to the conventions I discovered in the existing codebase as I was exposed to them. I'll rename it to be more specific, happy to "be the change you want to see" :)

mqudsi_neosmart.net: When doing "declarations up front" where a value may be repurposed a few times, I've always…
jrtc27Unsubmitted
Done
Inline Actions

ival just tells you it's an integer. ret and err are often used to hint that they're return values/error returns from something, which at least tells you more than the type. The only place ival is used in this file is for writing the version integer, 1, as the first integer in the hints file.

jrtc27: `ival` just tells you it's an integer. `ret` and `err` are often used to hint that they're…
dst_fd = open(dst, O_WRONLY | O_CREAT | O_TRUNC);
if (dst_fd < 0) {
warn("can't create %s", dst);
close(src_fd);
return 1;
jrtc27Unsubmitted
Done
Inline Actions

Now you've changed to bool these are the wrong way round

jrtc27: Now you've changed to bool these are the wrong way round
}
if (copy_file_range(src_fd, NULL, dst_fd, NULL, SSIZE_MAX, 0) < 0) {
jrtc27Unsubmitted
Done
Inline Actions

If it succeeds, the call returns the number of bytes copied, which can be
fewer than len. Returning fewer bytes than len does not necessarily in-
dicate that EOF was reached. However, a return of zero for a non-zero
len argument indicates that the offset for infd is at or beyond EOF.
copy_file_range() should be used in a loop until copying of the desired
byte range has been completed. If an error has occurred, a -1 is re-
turned and the error code is placed in the global variable errno.

(https://www.freebsd.org/cgi/man.cgi?query=copy_file_range)

jrtc27: > If it succeeds, the call returns the number of bytes copied, which can be > fewer than len.
warn("can't copy %s to %s", src, dst);
retval = 1;
}
close(src_fd);
close(dst_fd);
return retval;
jrtc27Unsubmitted
Done
Inline Actions

This is now a 12 space indent, not a 4 space one

jrtc27: This is now a 12 space indent, not a 4 space one
}
jrtc27Unsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

jrtc27: style(9) is 4 spaces indent for continuation lines
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

You're right. I thought indent(1) respected style(9)? There were a few other incorrect changes it made I had to fix by hand, but I missed this one.

mqudsi_neosmart.net: > style(9) is 4 spaces indent for continuation lines You're right. I thought indent(1)…
jrtc27Unsubmitted
Done
Inline Actions

indent(1) isn't perfect, no. In this case, it defaults to -lp, whereas style(9) wants -nlp.

jrtc27: indent(1) isn't perfect, no. In this case, it defaults to -lp, whereas style(9) wants -nlp.
emasteUnsubmitted
Done
Inline Actions

You can also give clang-format a try. It's not able to exactly match style(9) but gets pretty close.

I usually redirect the formatted output to a temp file, diff the input and formatted files, and iteratively edit.

emaste: You can also give //clang-format// a try. It's not able to exactly match style(9) but gets…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

You can also give clang-format a try. It's not able to exactly match style(9) but gets pretty close.

Hi Ed! Yeah, I just discovered that FreeBSD has an (experimental?) .clang-format file in the project root that gave me much more sane results than ident(1) (with -nlp), but clang-format was responsible for the 12-space continuation indent jrtc27@ just pointed out (and it seems to have an unreasonably short hard limit to the line length?). Getting there!

mqudsi_neosmart.net: > You can also give //clang-format// a try. It's not able to exactly match style(9) but gets…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

mqudsi_neosmart.net: > style(9) is 4 spaces indent for continuation lines
jrtc27Unsubmitted
Done
Inline Actions

If you change this to > 0 then you'll exit the loop for < 0

jrtc27: If you change this to > 0 then you'll exit the loop for < 0
static char xrefname[MAXPATHLEN], tempname[MAXPATHLEN]; static char xrefname[MAXPATHLEN], tempname[MAXPATHLEN];
static void static void
usage(void) usage(void)
{ {
fprintf(stderr, "%s\n", fprintf(stderr, "%s\n",
"usage: kldxref [-Rdv] [-f hintsfile] path ..." "usage: kldxref [-Rdv] [-f hintsfile] path ..."
); );
jrtc27Unsubmitted
Done
Inline Actions

... and then this could just be return (ival_renamed == 0), if you keep the return value at all, or

if (new_ival_name < 0) {
        warn(...);
        return (false);
}

return (true);
jrtc27: ... and then this could just be `return (ival_renamed == 0)`, if you keep the return value at…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

I really prefer to keep error handling as close as possible to the code that triggered it, both to make it easier to review and to prevent refactoring/maintenance issues down the line. If I were to move the warn to the end, someone could potentially introduce another syscall that would clobber errno. Actually, I think that's already the case since this would come after the close(2) calls. But just checking the renamed variable to bubble up the return code would be fine.

mqudsi_neosmart.net: I really prefer to keep error handling as close as possible to the code that triggered it, both…
jrtc27Unsubmitted
Done
Inline Actions

Oh, right, warn implicitly checks errno. You can still put the if < 0 warn directly after the loop though and avoid the nesting.

jrtc27: Oh, right, warn implicitly checks errno. You can still put the `if < 0 warn` directly after the…
exit(1); exit(1);
} }
static int static int
compare(const FTSENT *const *a, const FTSENT *const *b) compare(const FTSENT *const *a, const FTSENT *const *b)
{ {
if ((*a)->fts_info == FTS_D && (*b)->fts_info != FTS_D) if ((*a)->fts_info == FTS_D && (*b)->fts_info != FTS_D)
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Linesmain(int argc, char *argv[])
for (;;) { for (;;) {
p = fts_read(ftsp); p = fts_read(ftsp);
if ((p == NULL || p->fts_info == FTS_D) && fxref) { if ((p == NULL || p->fts_info == FTS_D) && fxref) {
/* close and rename the current hint file */ /* close and rename the current hint file */
fclose(fxref); fclose(fxref);
fxref = NULL; fxref = NULL;
if (reccnt != 0) { if (reccnt != 0) {
rename(tempname, xrefname); if (rename(tempname, xrefname) != 0) {
/* handle temp file being on a different filesystem */
if (errno == EXDEV) {
/* copyfile reports its own errors */
copyfile(tempname, xrefname);
jrtc27Unsubmitted
Done
Inline Actions

Having a return value for a static function that's always ignored is a bit odd

jrtc27: Having a return value for a static function that's always ignored is a bit odd
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

I agree, which is why I made sure to document why it's ignored here. But the return value is there to be more generic/useful (and it doesn't hurt besides looking odd without that comment to explain). If an operation needed to abort after a failed file copy, the return code would then be used to abort but that's not the case right here.

mqudsi_neosmart.net: I agree, which is why I made sure to document why it's ignored here. But the return value is…
jrtc27Unsubmitted
Done
Inline Actions

Ok, but that's not quite what your comment says. Your comment says it reports its own errors, but that would also be true of a void-returning copyfile.

jrtc27: Ok, but that's not quite what your comment says. Your comment says it reports its own errors…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

If you start with the assumption that the only reason to check the return code would be to either report an error or perform some sort of control flow based off the result, if the comment says it does its own error checking then the fact that there's no control flow logic here indicates that we're just going to continue. I'll make it void if that's what you prefer, though.

mqudsi_neosmart.net: If you start with the assumption that the only reason to check the return code would be to…
emasteUnsubmitted
Done
Inline Actions

If you start with the assumption that the only reason to check the return code would be to either report an error or perform some sort of control flow based off the result, if the comment says it does its own error checking then the fact that there's no control flow logic here indicates that we're just going to continue. I'll make it void if that's what you prefer, though.

emaste: > If you start with the assumption that the only reason to check the return code would be to…
emasteUnsubmitted
Done
Inline Actions

Oops, did not mean to add that.

emaste: Oops, did not mean to add that.
jrtc27Unsubmitted
Done
Inline Actions

I still think this approach is a bad design

jrtc27: I still think this approach is a bad design
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

As I mentioned, if you just want the static function to be void, I'm happy to change it. Would that work for you?

mqudsi_neosmart.net: As I mentioned, if you just want the static function to be void, I'm happy to change it. Would…
jrtc27Unsubmitted
Done
Inline Actions

Yes, that would be ok

jrtc27: Yes, that would be ok
unlink(tempname);
} else {
unlink(tempname);
jrtc27Unsubmitted
Done
Inline Actions

If you unlink after warning then it can be hoisted

jrtc27: If you unlink after warning then it can be hoisted
warn("can't rename %s to %s", xrefname, tempname);
}
}
} else { } else {
/* didn't find any entry, ignore this file */ /* didn't find any entry, ignore this file */
unlink(tempname); unlink(tempname);
unlink(xrefname); unlink(xrefname);
} }
} }
if (p == NULL) if (p == NULL)
break; break;
Show All 25 Lines
View Options

lib/libc/include/libc_private.h

Show First 20 Lines • Show All 43 Lines • ▼ Show 20 Lines
#include <sys/linker.h> #include <sys/linker.h>
#include <sys/stat.h> #include <sys/stat.h>
#include <sys/module.h> #include <sys/module.h>
#define FREEBSD_ELF #define FREEBSD_ELF
#include <ctype.h> #include <ctype.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h>
#include <fts.h> #include <fts.h>
#include <stdbool.h> #include <stdbool.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include <machine/elf.h> #include <machine/elf.h>
▲ Show 20 Lines • Show All 69 Lines • ▼ Show 20 Lines
* A string is stored as 1-byte length plus data, no padding * A string is stored as 1-byte length plus data, no padding
*/ */
static int static int
record_string(const char *str) record_string(const char *str)
{ {
int error; int error;
size_t len; size_t len;
u_char val; u_char val;
jrtc27Unsubmitted
Done
Inline Actions

Leave this alone

jrtc27: Leave this alone
if (dflag) if (dflag)
return (0); return (0);
val = len = strlen(str); val = len = strlen(str);
if (len > 255) if (len > 255)
errx(1, "string %s too long", str); errx(1, "string %s too long", str);
error = record_buf(&val, sizeof(val)); error = record_buf(&val, sizeof(val));
if (error != 0) if (error != 0)
return (error); return (error);
▲ Show 20 Lines • Show All 506 Lines • ▼ Show 20 Linesmaketempfile(char *dest, const char *root)
fd = mkstemp(dest); fd = mkstemp(dest);
if (fd < 0) if (fd < 0)
return (NULL); return (NULL);
fchmod(fd, 0644); /* nothing secret in the file */ fchmod(fd, 0644); /* nothing secret in the file */
return (fdopen(fd, "w+")); return (fdopen(fd, "w+"));
} }
static int
copyfile(char *src, char *dst) {
jrtc27Unsubmitted
Done
Inline Actions

style(9) requires the opening curly brace for functions to be on its own line

jrtc27: style(9) requires the opening curly brace for functions to be on its own line
jrtc27Unsubmitted
Done
Inline Actions

Not const char *?...

jrtc27: Not `const char *`?...
int retval = 0;
int src_fd, dst_fd;
src_fd = open(src, O_RDONLY);
if (src_fd < 0) {
jrtc27Unsubmitted
Done
Inline Actions

Why is src still a non-const char *?

jrtc27: Why is `src` still a non-const `char *`?
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

Sorry, I rewrote the prototype a dozen times when I was fighting with indent, and I must have hit u one time too many.

mqudsi_neosmart.net: Sorry, I rewrote the prototype a dozen times when I was fighting with indent, and I must have…
warn("can't open %s", src);
return 1;
jrtc27Unsubmitted
Done
Inline Actions

style(9) requires parentheses around all return values

jrtc27: style(9) requires parentheses around all return values
}
jrtc27Unsubmitted
Done
Inline Actions

ival is not a helpful variable name

jrtc27: ival is not a helpful variable name
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

When doing "declarations up front" where a value may be repurposed a few times, I've always noticed a preference for less-specific names (the name is copied from elsewhere in the same file, actually) and I was trying to stick to the conventions I discovered in the existing codebase as I was exposed to them. I'll rename it to be more specific, happy to "be the change you want to see" :)

mqudsi_neosmart.net: When doing "declarations up front" where a value may be repurposed a few times, I've always…
jrtc27Unsubmitted
Done
Inline Actions

ival just tells you it's an integer. ret and err are often used to hint that they're return values/error returns from something, which at least tells you more than the type. The only place ival is used in this file is for writing the version integer, 1, as the first integer in the hints file.

jrtc27: `ival` just tells you it's an integer. `ret` and `err` are often used to hint that they're…
dst_fd = open(dst, O_WRONLY | O_CREAT | O_TRUNC);
if (dst_fd < 0) {
warn("can't create %s", dst);
close(src_fd);
return 1;
jrtc27Unsubmitted
Done
Inline Actions

Now you've changed to bool these are the wrong way round

jrtc27: Now you've changed to bool these are the wrong way round
}
if (copy_file_range(src_fd, NULL, dst_fd, NULL, SSIZE_MAX, 0) < 0) {
jrtc27Unsubmitted
Done
Inline Actions

If it succeeds, the call returns the number of bytes copied, which can be
fewer than len. Returning fewer bytes than len does not necessarily in-
dicate that EOF was reached. However, a return of zero for a non-zero
len argument indicates that the offset for infd is at or beyond EOF.
copy_file_range() should be used in a loop until copying of the desired
byte range has been completed. If an error has occurred, a -1 is re-
turned and the error code is placed in the global variable errno.

(https://www.freebsd.org/cgi/man.cgi?query=copy_file_range)

jrtc27: > If it succeeds, the call returns the number of bytes copied, which can be > fewer than len.
warn("can't copy %s to %s", src, dst);
retval = 1;
}
close(src_fd);
close(dst_fd);
return retval;
jrtc27Unsubmitted
Done
Inline Actions

This is now a 12 space indent, not a 4 space one

jrtc27: This is now a 12 space indent, not a 4 space one
}
jrtc27Unsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

jrtc27: style(9) is 4 spaces indent for continuation lines
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

You're right. I thought indent(1) respected style(9)? There were a few other incorrect changes it made I had to fix by hand, but I missed this one.

mqudsi_neosmart.net: > style(9) is 4 spaces indent for continuation lines You're right. I thought indent(1)…
jrtc27Unsubmitted
Done
Inline Actions

indent(1) isn't perfect, no. In this case, it defaults to -lp, whereas style(9) wants -nlp.

jrtc27: indent(1) isn't perfect, no. In this case, it defaults to -lp, whereas style(9) wants -nlp.
emasteUnsubmitted
Done
Inline Actions

You can also give clang-format a try. It's not able to exactly match style(9) but gets pretty close.

I usually redirect the formatted output to a temp file, diff the input and formatted files, and iteratively edit.

emaste: You can also give //clang-format// a try. It's not able to exactly match style(9) but gets…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

You can also give clang-format a try. It's not able to exactly match style(9) but gets pretty close.

Hi Ed! Yeah, I just discovered that FreeBSD has an (experimental?) .clang-format file in the project root that gave me much more sane results than ident(1) (with -nlp), but clang-format was responsible for the 12-space continuation indent jrtc27@ just pointed out (and it seems to have an unreasonably short hard limit to the line length?). Getting there!

mqudsi_neosmart.net: > You can also give //clang-format// a try. It's not able to exactly match style(9) but gets…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

style(9) is 4 spaces indent for continuation lines

mqudsi_neosmart.net: > style(9) is 4 spaces indent for continuation lines
jrtc27Unsubmitted
Done
Inline Actions

If you change this to > 0 then you'll exit the loop for < 0

jrtc27: If you change this to > 0 then you'll exit the loop for < 0
static char xrefname[MAXPATHLEN], tempname[MAXPATHLEN]; static char xrefname[MAXPATHLEN], tempname[MAXPATHLEN];
static void static void
usage(void) usage(void)
{ {
fprintf(stderr, "%s\n", fprintf(stderr, "%s\n",
"usage: kldxref [-Rdv] [-f hintsfile] path ..." "usage: kldxref [-Rdv] [-f hintsfile] path ..."
); );
jrtc27Unsubmitted
Done
Inline Actions

... and then this could just be return (ival_renamed == 0), if you keep the return value at all, or

if (new_ival_name < 0) {
        warn(...);
        return (false);
}

return (true);
jrtc27: ... and then this could just be `return (ival_renamed == 0)`, if you keep the return value at…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

I really prefer to keep error handling as close as possible to the code that triggered it, both to make it easier to review and to prevent refactoring/maintenance issues down the line. If I were to move the warn to the end, someone could potentially introduce another syscall that would clobber errno. Actually, I think that's already the case since this would come after the close(2) calls. But just checking the renamed variable to bubble up the return code would be fine.

mqudsi_neosmart.net: I really prefer to keep error handling as close as possible to the code that triggered it, both…
jrtc27Unsubmitted
Done
Inline Actions

Oh, right, warn implicitly checks errno. You can still put the if < 0 warn directly after the loop though and avoid the nesting.

jrtc27: Oh, right, warn implicitly checks errno. You can still put the `if < 0 warn` directly after the…
exit(1); exit(1);
} }
static int static int
compare(const FTSENT *const *a, const FTSENT *const *b) compare(const FTSENT *const *a, const FTSENT *const *b)
{ {
if ((*a)->fts_info == FTS_D && (*b)->fts_info != FTS_D) if ((*a)->fts_info == FTS_D && (*b)->fts_info != FTS_D)
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Linesmain(int argc, char *argv[])
for (;;) { for (;;) {
p = fts_read(ftsp); p = fts_read(ftsp);
if ((p == NULL || p->fts_info == FTS_D) && fxref) { if ((p == NULL || p->fts_info == FTS_D) && fxref) {
/* close and rename the current hint file */ /* close and rename the current hint file */
fclose(fxref); fclose(fxref);
fxref = NULL; fxref = NULL;
if (reccnt != 0) { if (reccnt != 0) {
rename(tempname, xrefname); if (rename(tempname, xrefname) != 0) {
/* handle temp file being on a different filesystem */
if (errno == EXDEV) {
/* copyfile reports its own errors */
copyfile(tempname, xrefname);
jrtc27Unsubmitted
Done
Inline Actions

Having a return value for a static function that's always ignored is a bit odd

jrtc27: Having a return value for a static function that's always ignored is a bit odd
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

I agree, which is why I made sure to document why it's ignored here. But the return value is there to be more generic/useful (and it doesn't hurt besides looking odd without that comment to explain). If an operation needed to abort after a failed file copy, the return code would then be used to abort but that's not the case right here.

mqudsi_neosmart.net: I agree, which is why I made sure to document why it's ignored here. But the return value is…
jrtc27Unsubmitted
Done
Inline Actions

Ok, but that's not quite what your comment says. Your comment says it reports its own errors, but that would also be true of a void-returning copyfile.

jrtc27: Ok, but that's not quite what your comment says. Your comment says it reports its own errors…
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

If you start with the assumption that the only reason to check the return code would be to either report an error or perform some sort of control flow based off the result, if the comment says it does its own error checking then the fact that there's no control flow logic here indicates that we're just going to continue. I'll make it void if that's what you prefer, though.

mqudsi_neosmart.net: If you start with the assumption that the only reason to check the return code would be to…
emasteUnsubmitted
Done
Inline Actions

If you start with the assumption that the only reason to check the return code would be to either report an error or perform some sort of control flow based off the result, if the comment says it does its own error checking then the fact that there's no control flow logic here indicates that we're just going to continue. I'll make it void if that's what you prefer, though.

emaste: > If you start with the assumption that the only reason to check the return code would be to…
emasteUnsubmitted
Done
Inline Actions

Oops, did not mean to add that.

emaste: Oops, did not mean to add that.
jrtc27Unsubmitted
Done
Inline Actions

I still think this approach is a bad design

jrtc27: I still think this approach is a bad design
mqudsi_neosmart.netAuthorUnsubmitted
Done
Inline Actions

As I mentioned, if you just want the static function to be void, I'm happy to change it. Would that work for you?

mqudsi_neosmart.net: As I mentioned, if you just want the static function to be void, I'm happy to change it. Would…
jrtc27Unsubmitted
Done
Inline Actions

Yes, that would be ok

jrtc27: Yes, that would be ok
unlink(tempname);
} else {
unlink(tempname);
jrtc27Unsubmitted
Done
Inline Actions

If you unlink after warning then it can be hoisted

jrtc27: If you unlink after warning then it can be hoisted
warn("can't rename %s to %s", xrefname, tempname);
}
}
} else { } else {
/* didn't find any entry, ignore this file */ /* didn't find any entry, ignore this file */
unlink(tempname); unlink(tempname);
unlink(xrefname); unlink(xrefname);
} }
} }
if (p == NULL) if (p == NULL)
break; break;
Show All 25 Lines
View Options

lib/libc/sys/interposing_table.c

Loading...
View Options

lib/libthr/Makefile

Loading...
View Options

lib/libthr/arch/amd64/include/pthread_tls.h

Loading...
View Options

lib/libthr/arch/i386/include/pthread_tls.h

Loading...
View Options

lib/libthr/pthread.map

Loading...
View Options

lib/libthr/thread/thr_list.c

Loading...
View Options

lib/libthr/thread/thr_private.h

Loading...
View Options

libexec/rtld-elf/rtld.h

Loading...
View Options

libexec/rtld-elf/rtld.c

Loading...