Page MenuHomeFreeBSD
Differential D7938

[iwm] Use IWM_DEFAULT_SCAN_CHANNELS define as default for sc_capa_n_scan_channels.
ClosedPublic
Actions

Authored by ivadasz on Sep 18 2016, 11:50 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 7, 6:20 PM
Unknown Object (File)
Mon, Apr 7, 3:25 AM
Unknown Object (File)
Sun, Mar 16, 7:43 AM
Unknown Object (File)
Mar 7 2025, 4:01 PM
Unknown Object (File)
Mar 7 2025, 2:36 PM
Unknown Object (File)
Mar 7 2025, 2:21 PM
Unknown Object (File)
Mar 7 2025, 12:34 PM
Unknown Object (File)
Feb 28 2025, 6:26 PM
View All 75 Files
Subscribers
imp

Details

Reviewers
cognet
adrian
Commits
rS306005: [iwm] Use IWM_DEFAULT_SCAN_CHANNELS define as default for…
Summary

Obtained from: Linux iwlwifi

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5205
Build 5341: arc lint + arc unit

Event Timeline

ivadasz updated this revision to Diff 20444.Sep 18 2016, 11:50 AM
ivadasz retitled this revision from to [iwm] Use IWM_DEFAULT_SCAN_CHANNELS define as default for sc_capa_n_scan_channels..
ivadasz updated this object.
ivadasz added reviewers: adrian, cognet.
Herald added a subscriber: imp. · View Herald TranscriptSep 18 2016, 11:50 AM
adrian accepted this revision.Sep 19 2016, 9:37 PM
adrian edited edge metadata.
This revision is now accepted and ready to land.Sep 19 2016, 9:37 PM
Closed by commit rS306005: [iwm] Use IWM_DEFAULT_SCAN_CHANNELS define as default for… (authored by ivadasz). · Explain WhySep 19 2016, 10:24 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
dev/
iwm/
4 lines
3 lines

Diff 20444

View Options

sys/dev/iwm/if_iwm.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

sys/dev/iwm/if_iwmreg.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