- User Since
- Dec 19 2016, 4:11 AM (65 w, 4 d)
Tue, Mar 20
Mon, Mar 19
Wed, Mar 14
Tue, Mar 13
Mon, Mar 12
Let me try again about the net.inet.ip.fw.one_pass. This patch leaves that value alone, that value is 1 by default. The added rules shall reassemble all UDP packets, and since one_pass is set it well at that point PASS THE PACKET. This is a huge hole in the firewall in effect allowing all UDP traffic to pass inward without any port or state being checked. With the added rule that reassembles udp packets it is a MUST that net.inet.ip.fw.one_pass be set to 0 so that the additional checks later in the firewall can be checked. It is also a must that the rule be moved before the check-state.
Sun, Mar 11
I agree that the rule order is wrong here, reass should be done before a check state,
in general you want reass to occur very early, so that other rules are checking the
reassembled packet. reass should occur before any rules that list ports.
Sat, Mar 10
Update bhyve -h usage output to include new cpu topology options.
Pointed out by Roman Bogorodskiy (novel@).
Translate reviewer rgrimes -> bhyve triggered by touching stand/userboot.
Wed, Mar 7
Fix white space nit in usr.sbin/bhyve/bhyve.8
Remove extra comma in usr.sbin/bhyvectl/bhyvectl.c causing build failure
Update per Peter Grehan
Tue, Mar 6
Mon, Mar 5
Wed, Feb 28
Since your all here from the add of bhyve can you look at herald rule H97 https://reviews.freebsd.org/H97 , look for anything I might of missed in the list of file directories.
I am trying to get that upgraded to a global rule with an add non-blocking reviewer bhyve, but that requires higher access than I have.
Tue, Feb 27
If Peter accepts this and says I can go ahead with a commit I would do that, but I need his approval to commit it.
Fri, Feb 23
I dont know why it did that, I tried to simply "accept revision" and it removed all other reviewers, so put them back.
Thu, Feb 22
Feb 17 2018
Correct issues and add man page changes
Feb 16 2018
I missed that, overall I think this man page could use a rework in general form, gather all the syntax up in the SYNOPSIS, but if we keep pecking away at it at least the errors can be removed.
Feb 15 2018
I have all the other comments corrected in my copy, seeking clarification on error check value for vm_set_topology before I correct that. Plan to delay depreciation of old sysctl method to allow simpler merge to stable/11. That is going to require a direct commit to stable/11, and a different commit to ^head
Feb 12 2018
Feb 10 2018
This is missing the associated man page change that changes gateway to [gateway] on the change syntax,
Though perhaps BSD did allow this in the past, the man page never said that the gateway was optional as far as I can recall.
Feb 2 2018
Feb 1 2018
Jan 28 2018
I still find this as a gratuitous difference in FreeBSD that does not exist in NetBSD or OpenBSD, and appears to be BSD behavior back into the 80's, anyone that HAS already scripted to deal with symbolic names well be broken if this change is made.
Jan 25 2018
Only question I have before we remove this, is the tree void of code in this format?
Jan 20 2018
I did a very quick one time scan down the code just to see what was here, these are my comments about it.
None of the functions have block start comments stating what that function does.
Jan 19 2018
Also a one line sample is always good.
Ed suggests do we or do we not use tab to align =, this should be clarified.
Iirc in the places that we do use any initializer in the current code it is spaces on each side of = just as in assignment statements.
Jan 15 2018
Looks good to me, but a future refactor would be nice, #define VMX_GUEST_CLOBBER_INTEL and VMX_GUEST_CLOBBER_AMD and invoke them at the right place. Makes the *_support.S files less different from each other.
Jan 13 2018
Jan 4 2018
On visual inspection this looks good, I do not run pf or stateful so I can not do any real world testing. Please make sure the commit message includes a comment about refactoring common code to a new function ipf_state_seed_alloc.
Dec 31 2017
Two things, first it is not about saving the 100 bytes, it is about not having this function in the kernel at all, I simple do not need it, nor have I needed it for 40 years on any system.
Second, I think we have a POLA problem here, what changes for people that already have SW_WATCHDOG in there kernel config?
Would not this change the behavior of there systems? I think it is a bad idea to take over the existing "option" and changing what it does or how it behaves.
I still think it would just of been simpler to add options SW_WATCHDOG to the GENERIC kernel to solve your desire, that is self documenting more or less as no change is being made to how that behaves, no need to notifiy in RELEASE NOTES (if you push forward with this change as it stands now be certain to mark it for release notes).
Also is not that what you have been doing up to now?
Methods, not policy, your imposing your policy on others.
Also a comparison to the "bloated" generic of 21MB vs a trimmed kernel of 5.5MB is not reasonble to make as a basis for "this is only a tiny bit of code". I agree that it is a tiny bit of code, but it is code most have lived without for decades and should continue to have that option.
This adds the watchdog code to the kernel witn no knob to turn it off or remove it, please make it possible to NOT have this code in the kernel.
Dec 21 2017
Dec 20 2017
I have this patch downloaded, but not applied, on my AMD test system. I'll apply and build after I send this comment. Do we have any more elaborate testing than just do the watchs that jhb did?
Jul 10 2017
In my review of other systems the printing of "default" for this is common and has a long history, changing it at this point in the game would probably be a POLA and possibly break some scripts. I have worked around the issue that caused me to notice this in another way.
Jun 13 2017
Jun 6 2017
As pointed on in the PR is it possible to totally remove all this casting?
It would make the code much cleaner.
May 28 2017
May 27 2017
I think this should do nicely.
*sigh* After further investigation and some discussion I find that there are still problems with this.
If you login to a serial port and resizewin -z runs, it well set the terminal size to what ever terminal size you login in with, if you then logout and resize your terminal and login again resizewin well see non zero values and not do anything leaving us with the same issue that this change is suppose to fix.
Okay, so I should not trust man on freefall.... added May 8th, 2017. This makes this change slightly more palatable as resize/resizewin has been known to eat input if commands are typed over an ssh/telnet session while the login scripts are still running causing both the resize operattion and the typed command to fail, I am still leary of this, but dont object to it being committed.
FreeBSD's resizewin takes no arguments or options, please remove -z from commands.
May 26 2017
Other than the comment expansion request I like this, very clean, very easy to understand, and even easier to expand in the future.
May 25 2017
This is starting to sound like a terminal emulation type vs term= miss match perhaps? My term size was quiet correct, that is what a standard Putty serial terminal size comes up as.
I am having difficulty with this then as my raspberry pie is just fine and dandy over serial when logging into the console:
root@rpi3:~ # stty -a
speed 115200 baud; 24 rows; 80 columns;
FreeBSD rpi3 12.0-CURRENT FreeBSD 12.0-CURRENT #0 r313109M: Thu Feb 2 16:16:39 MST 2017 firstname.lastname@example.org:/usr/home/brd/rpi3/crochet/work/obj/arm64.aarch64/usr/src/sys/GENERIC arm64
root@rpi3:~ # grep ttyu0 /etc/ttys
ttyu0 "/usr/libexec/getty 3wire" vt100 onifconsole secure
May 24 2017
Isnt this caused by a missing entry in /etc/ttys for the serial console? This should be coming from there, not from .dot files.
May 22 2017
I have no problem with this code going in as it is here, it can be evolved with a table post commit.
May 19 2017
Side band communications are not very helpful, that email should of been attached to this review, or at least a note that there was a side band issue raised would of saved a few man cycles.
May 18 2017
Please be sure to flag this for MFC to 11 and 10
May 4 2017
I am happy with this, it moves us forward, and removes the option 150 hack. I am pretty sure that you can actually dole out more than 1 tftp server by careful use of dhcp server configurations, ie the dhcp server has full control over what next-server you are told to load from if your environment is large enough that you need more than one.
Mark as done all the nits Fixed in https://reviews.freebsd.org/D10049 by Anish
May 2 2017
The more I look at PXE and DHCP the sicker I get. Your right, a client well not normally request option 150, for that matter I am not sure it well request 66 either, though I suspect many do, and probably many of them also wrongly interpret that as a dotted quad when it is actually a host name. I have had to fuss with dhcp servers and pxe clients for days if not weeks to get some operating systems booting over the network. Right now I can boot almost any FreeBSD >9 version as long as I am not an a UEFI platform, so our code is not that broken, I do have to use loader code from -head on the older releases, so things have improved. My use case is rather narrow I suppose in that I always chain load to iPXE to use its advanced features vs what most cards have in there bios. I am getting freebsd's pxeboot via tftp, then use that to get the kernel via nfs. Again, can we get input from Bapt who did the addition of opt 150 and how it worked?
Apr 28 2017
Quick glance for 2 minutes says we should probably have a man page change with this?
Apr 26 2017
I have done some more digging, in rfc3492 section 4 the ietf has reclassified options: 4. Reclassifying Options The site-specific option codes 128 to 223 are hereby reclassified as publicly defined options. This leaves 31 site-specific options, 224 to 254.
Interestingly this rfc is by Cisco!
Found the list, these are in the domain of Iana now, so you have to go to https://www.iana.org/assignments/bootp-dhcp-parameters/bootp-dhcp-parameters.xhtml to find them. Which leads you to rfc5859. Reading that RFC is causing me to say something un popular here. We should be using option 150 as an IP addresses, and NOT using option 66 because we can not do hostname resolution. The reasons that Cisco request option 150 was partly based on that.
Does the code even request option 66 from the server? Or do we just expect an sname filled in? I read some of the Intel pxe 2.1 spec and they don't even mention option 66 in there list of options, yet the refer to it several times at the sname line of data structures.
This is a valid concern, We should also ask Bapt why he choose to use option 150 instead of 66 when implementing this?
Apr 25 2017
RFC1048 is obsoleted, first by 1533, then by 2132. Please lets follow current RFC's when seeking information and refering to them. Refering to obsolete RFC's is going to lead to obsolete code. I see at least in the comment of adding vend_end that you refer to RFC2132, it is unclear why you refered to 1048 in the description of the change. RFC2132 names code 66 "TFTP server name". I can not find in the RFC that option 150 is defined. I do find from google searches that this was/is a cisco specific value: "DHCP option 150 provides the IP addresses of a list of TFTP servers. • DHCP option 66 gives the IP address or the hostname of a single TFTP server. Note Cisco IP Phones might also include DHCP option 3 in their requests, which sets the default route. A single request might include both options 150 and 66." This clarifies that it is possible to send both.
Apr 22 2017
Apr 21 2017
I've been swayed that this is a short path to a solution for now, but still would like to see the arc4random eventually updated to allow for the crypto funtion use be pluggable and selectable without a kernel recompile.
Apr 11 2017
I am not sure, but I believe this is pretty much in conflict with what I did in the CPU topology enhancement, D9930
Apr 5 2017
I was going to close this but I see several things from bcr that are not marked as fixed by anish in D10049, can you please review all of bcr's nits and make sure we have those fixed in D10049 before I abandon this?