hyperv/vmbus: Don't close unopened channels.
ClosedPublic
Actions

Authored by sepherosa_gmail.com on Nov 18 2016, 6:13 AM.

Details

Reviewers

howard0su_gmail.com
honzhan_microsoft.com
decui_microsoft.com
delphij
royger

Commits

rS309132: hyperv/vmbus: Don't close unopened channels.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

sepherosa_gmail.com updated this revision to Diff 22317.Nov 18 2016, 6:13 AM

sepherosa_gmail.com retitled this revision from to hyperv/vmbus: Don't close unopened channels..

sepherosa_gmail.com updated this object.

sepherosa_gmail.com edited the test plan for this revision. (Show Details)

sepherosa_gmail.com added reviewers: delphij, royger, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com.

Closed by commit rS309132: hyperv/vmbus: Don't close unopened channels. (authored by sephe). · Explain WhyNov 25 2016, 6:12 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

sys/

dev/

hyperv/

vmbus/

vmbus_chan.c

22 lines

Diff 22317

View Options

sys/dev/hyperv/vmbus/vmbus_chan.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 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

hyperv/vmbus: Don't close unopened channels.ClosedPublicActions