Add the FreeBSD + ZoL CFT to the news section
ClosedPublic
Actions

Authored by seanc on Apr 27 2019, 9:52 PM.

Details

Reviewers

kmoore
allanjude

Commits

rD52962: share/news: add a link to the CFT for FreeBSD+ZoL

Summary

Another recent newsworthy item.

Diff Detail

Repository

rD FreeBSD doc repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Terse explanation of what this actually means.

Suggested by: allanjude

Send it

This revision is now accepted and ready to land.Apr 27 2019, 9:54 PM

seanc updated this revision to Diff 56760.Apr 27 2019, 9:57 PM

This revision now requires review to proceed.Apr 27 2019, 9:57 PM

This revision was not accepted when it landed; it landed in state Needs Review.Apr 27 2019, 9:58 PM

Closed by commit rD52962: share/news: add a link to the CFT for FreeBSD+ZoL (authored by seanc). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

head/

share/

xml/

news.xml

17 lines

Diff 56761

View Options

head/share/xml/news.xml

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 Lines	maketempfile(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 Lines	main(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

Add the FreeBSD + ZoL CFT to the news sectionClosedPublicActions