Re: svn commit: r368776 - head/usr.bin/login

[Pedro Giffuni]

On 12/18/20 9:23 PM, Pedro F. Giffuni wrote:

Author: pfg
Date: Sat Dec 19 02:23:53 2020
New Revision: 368776
URL: https://svnweb.freebsd.org/changeset/base/368776

Log:
   login(1): when exporting variables check the result of setenv(3)
   
   When exporting a variable we correctly check all the preconditions that

   could make setenv(3) fail. Checking the setenv(3) return value seems
   redundant, but given that login(1) is critical, it doesn't hurt to have
   a post-check.
   
   This change is based on the "Principles of Secure Coding" course by

   Matthew Bishop, PhD., which specifically discusses this code in FreeBSD.
   
   Differential Revision:	https://reviews.freebsd.org/D26966


Modified:
   head/usr.bin/login/login.c

Modified: head/usr.bin/login/login.c
==
--- head/usr.bin/login/login.c  Sat Dec 19 01:46:47 2020(r368775)
+++ head/usr.bin/login/login.c  Sat Dec 19 02:23:53 2020(r368776)
@@ -793,6 +793,7 @@ export(const char *s)
char *p;
const char **pp;
size_t n;
+   int rv;
  
  	if (strlen(s) > 1024 || (p = strchr(s, '=')) == NULL)

return (0);
@@ -804,8 +805,10 @@ export(const char *s)
return (0);
}
*p = '\0';
-   (void)setenv(s, p + 1, 1);
+   rv = setenv(s, p + 1, 1);
*p = '=';
+   if (rv == 1)
+   return (0);
return (1);
  }
  


This is wrong .. it should have been -1.

I'll revert to make the change clean.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r366993 - head/sys/net

[Pedro Giffuni]

On 10/24/20 8:19 PM, Ed Maste wrote:

On Sat, 24 Oct 2020 at 11:27, Warner Losh  wrote:

Given we already have nice .clang-format, that does most of the job, maybe it's 
worth considering looking into tweaking it further to fix this part?
It would be nice if we could finally offload all formatting issues to the tool 
and focus on the actual code :-)

It would be nice if it produced one of the style(9) acceptable formats without 
disrupting things already acceptable.  That's been the big problem with the 
tweaks to date... some things are fixed, others break. It's getting a lot 
closer, though

Upstream clang-format comes with a script that can integrate with git,
adding a `git clang-format` command. It will apply formatting to
modified lines, leaving unchanged ones alone.


I doubt any script can match style(9) perfectly. indent(1) with bde's 
flags came near in some edge cases but generally did a horrible job. 
Also, out style(9) is not mandatory for userland.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r366032 - in head: . share/mk stand sys/conf sys/modules sys/powerpc/include

[Pedro Giffuni]

On 22/09/2020 18:49, Brandon Bergren wrote:

Author: bdragon
Date: Tue Sep 22 23:49:30 2020
New Revision: 366032
URL: https://svnweb.freebsd.org/changeset/base/366032

Log:
   [PowerPC64LE] Set up powerpc.powerpc64le architecture
   
   This is the initial set up for PowerPC64LE.
   
   The current plan is for this arch to remain experimental for FreeBSD 13.
   
   This started as a weekend learning project for me and kinda snowballed from

   there.
   
   (More to follow momentarily.)


Very nice "learning" project ... Huge thanks for taking time on this and 
having fun in the process !


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r365071 - in head/sys: net net/altq net/route net80211 netgraph netgraph/atm netgraph/atm/ccatm netgraph/atm/sscfu netgraph/atm/sscop netgraph/atm/uni netgraph/bluetooth/common netgrap

[Pedro Giffuni]

On 02/09/2020 13:06, Alexey Dokuchaev wrote:

On Wed, Sep 02, 2020 at 10:18:15AM -0500, Pedro Giffuni wrote:

On 01/09/2020 21:05, Alexey Dokuchaev wrote:

...
This is common sense.  I can't count how often I wanted to hack on
something in the base/kernel and was turned away by this atrocious
excessive whitespace mess.

Thank you Mateusz for cleaning this up.

I honestly don't care much, but spaces do no harm and can make the code
more readable. Sort of a silent comment, or what you do in written
language when you start a new paragraph.

Right, but that's the example of appropriate usage of whitespace.  I was
talking about *excessive* whitespace, that is, more than two \n's in a row
if we speak of newlines (subject of these commits).


But how much space is rather subjective so Michael is right in asking 
what rule has been violated.


No one is asking for the change to be reverted: the damage, if any, is 
already done.


Pedro.



./danfe

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r365071 - in head/sys: net net/altq net/route net80211 netgraph netgraph/atm netgraph/atm/ccatm netgraph/atm/sscfu netgraph/atm/sscop netgraph/atm/uni netgraph/bluetooth/common netgrap

[Pedro Giffuni]

On 01/09/2020 21:05, Alexey Dokuchaev wrote:

On Wed, Sep 02, 2020 at 12:41:43AM +0200, Michael Tuexen wrote:

On 1. Sep 2020, at 23:19, Mateusz Guzik  wrote:
Author: mjg
Date: Tue Sep  1 21:19:14 2020
New Revision: 365071
URL: https://svnweb.freebsd.org/changeset/base/365071

Log:
  net: clean up empty lines in .c and .h files

Hi Mateusz,

which rules are enforced? Why?

This is common sense.  I can't count how often I wanted to hack on
something in the base/kernel and was turned away by this atrocious
excessive whitespace mess.

Thank you Mateusz for cleaning this up.


I honestly don't care much, but spaces do no harm and can make the code 
more readable. Sort of a silent comment, or what you do in written 
language when you start a new paragraph.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r362681 - in head: contrib/bc contrib/bc/gen contrib/bc/include contrib/bc/locales contrib/bc/manuals contrib/bc/src contrib/bc/src/bc contrib/bc/src/dc contrib/bc/src/history contrib/

[Pedro Giffuni]

On 29/06/2020 16:11, Stefan Eßer wrote:

Am 29.06.20 um 20:09 schrieb Ed Maste:

On Mon, 29 Jun 2020 at 11:27, John Baldwin  wrote:

I suspect just doing the 'merge --record-only' is the simplest method
assuming Git handles it ok.  I suspect since Git ignores mergeinfo this
is fine, but it would be good for Ed to confirm.  You can always restore
the tests in the future in contrib/bc when you want to add them.

I think a --record-only merge is the best approach; in any case we
have a number of these in the tree already and Git will have to deal
with them.

Thank you for this advice.

There is a new version, which differs only in the man-pages. These
used to mention optional features (of which only NLS support is
actually optional in the version built on FreeBSD, all other are
always compiled in, and I had mentioned to the author that this
might irritate FreeBSD users).


FWIW, since it was rewritten it would have been actually nice to have it 
in modern C++.




I have suggested to the author to add SPDX BSD-2-Clause tags, which
would change a large fraction of the currently committed files.


We only use the SPDX tags for internal software. For stuff from contrib 
it is not necessary.





Is it required to perform the --record-only merge, then?
Since you are going to bring it through the vendor area you should keep 
the merge information yes.

Or would it be OK to import the new release into the vendor branch
and then svn copy that version completely (including the tests and
other files that I had omitted in the initial import) to contrib?

I expect the svn copy without prior svn merge --record-only to
result in the same repository state after the conversion to Git,
but I do not really know ...


It is likely git won't care.

cheers,

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r362488 - in head: contrib/file/magic/Magdir contrib/tcpdump lib/geom/part stand/efi/include stand/i386/boot0 sys/dev/hptmv sys/geom/part usr.bin/fortune/datfiles usr.bin/mkimg usr.sbi

[Pedro Giffuni]

On 22/06/2020 02:58, Alexey Dokuchaev wrote:

On Mon, Jun 22, 2020 at 07:46:25AM +, Baptiste Daroussin wrote:

New Revision: 362488
URL: https://svnweb.freebsd.org/changeset/base/362488

Log:
   Revert r362466
   
   Such change should not have happen without prior discussion and review.

Thank you Baptiste.

./danfe

+1 Thank you
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r362217 - head/stand/common

[Pedro Giffuni]

On 16/06/2020 12:01, Ian Lepore wrote:

On Tue, 2020-06-16 at 07:05 +, Toomas Soome wrote:

Author: tsoome
Date: Tue Jun 16 07:05:03 2020
New Revision: 362217
URL: https://svnweb.freebsd.org/changeset/base/362217

Log:
   loader: variable i is unused without MBR/GPT support built in
   
   Because i is only used as index in for loop, declare it in for

statement.


As much as I prefer doing it this way, style(9) doesn't allow for
variable declarations inside a for() statement (or even inside a local
block, which is just too 1980s for me, but it is still our standard).


Perhaps style(9) needs updating? I think KNF is mandatory for kernel 
code only, and is only suggested for userland.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r361775 - in head/sys: dts/arm64/overlays modules/dtb/rpi

[Pedro Giffuni]

On 03/06/2020 19:59, Oleksandr Tymoshenko wrote:

Rodney W. Grimes (free...@gndrsh.dnsmgr.net) wrote:

[ Charset UTF-8 unsupported, converting... ]

Author: gonzo
Date: Wed Jun  3 22:18:15 2020
New Revision: 361775
URL: https://svnweb.freebsd.org/changeset/base/361775

Log:
   Add spigen overlay for Raspberry Pi 4
   
   Submitted by:	gergely.czu...@harmless.hu


Added:
   head/sys/dts/arm64/overlays/spigen-rpi4.dtso   (contents, props changed)
Modified:
   head/sys/modules/dtb/rpi/Makefile

Added: head/sys/dts/arm64/overlays/spigen-rpi4.dtso
==
--- /dev/null   00:00:00 1970   (empty, because file is newly added)
+++ head/sys/dts/arm64/overlays/spigen-rpi4.dtsoWed Jun  3 22:18:15 
2020(r361775)
@@ -0,0 +1,30 @@
+/* $FreeBSD$ */

This file needs some form of copyright/license.

Whom should I put as a copyright folder, The FreeBSD Project or the
person who submitted the patch?


The person that submitted the patch.

Note that the FreeBSD Project is not an entity and cannot hold 
copyrights (The Foundation can but unless they sponsored it, that 
usually involves paperwork).


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r361136 - head/sys/fs/ext2fs

[Pedro Giffuni]

...

On 17/05/2020 09:52, Fedor Uporov wrote:

Author: fsu
Date: Sun May 17 14:52:54 2020
New Revision: 361136
URL: https://svnweb.freebsd.org/changeset/base/361136

Log:
   Add BE architectures support.
   
   Author of most initial version: pfg (https://reviews.freebsd.org/D23259)
   
   Reviewed by:pfg

   MFC after:  3 months
   
   Differential Revision:https://reviews.freebsd.org/D24685


Thanks so much for finishing off my initial code and for testing in PPC!

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r359005 - head/usr.bin/calendar/calendars

[Pedro Giffuni]

On 15/03/2020 17:12, Conrad Meyer wrote:

Pedro,

It seems like you are attempting to belittle me, and perhaps the
entire USA, by intentionally conflating the calendar(1) program with
the entire concept of written records and education.  Stop it.  I know
you're smart and have been around FreeBSD long enough to understand
that in context, 'calendar(1)' referred to /usr/bin/calendar;
pretending to misunderstand in order to respond to a straw man does
not persuade.


For all I know the calendar(1) utility comes from Berkeley, it was 
included in the CSRG BSD distribution and was written from someone in 
the US.


From my personal point of view, I believe my most important 
contribution to FreeBSD as a whole was r321834, which added the Catholic 
holidays, followed in many countries, to calendar(1). For non-catholics 
it is surely unimportant and a culture/religion uniqueness thing but 
I've always thought FreeBSD had such interest in culture diversity.


Yes, for you calendar is something accidental that shouldn't be there, 
for me it's a differentiating factor.



It's also just not a great way to treat fellow project contributors,
even if you disagree with my opinion.


That was not at all my intention.


Sarcasm doesn't work on the internet.  Especially not among
colleagues.  Please try to speak directly and advocate for your
viewpoints without resorting to it, or sweeping attacks on my person
and/or "culture."


Please excuse me if you felt I meant anything personal against you or 
your way of thinking.


I was trying to show you how someone else, like me, really sees 
somethings different. It is more that just code.


Now, if we can just go on with the regular programming, and leave 
calendar(1) as it is.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r359005 - head/usr.bin/calendar/calendars

[Pedro Giffuni]

On 14/03/2020 21:38, Conrad Meyer wrote:

On Sat, Mar 14, 2020 at 5:49 PM Pedro F. Giffuni  wrote:

Author: pfg
Date: Sun Mar 15 00:49:06 2020
New Revision: 359005
URL: https://svnweb.freebsd.org/changeset/base/359005

Log:
   calendar(1): Updates and corrections for some calendar files.

   These have an educational value and are, no doubt, an integral part of the 
fun
   behind running the BSDs.

Hmm.  I don't believe calendar(1) has education value (or to the
extent it has quantifiable value, N <= 0).  Nor does it have more
subjective "fun" value.  It certainly isn't integral to the system.


Some cultures despise the value of history and things like education and 
maps and degenerate accordingly [1]: this tool helps by hinting 
curiosity. Of course if the database is not populated or people don't 
use calendar(1) it won't be effective at all.



It's a toy of a certain era that has aged poorly.


Some quite long lasting institutions like the Roman Catholic Church, 
which will likely outlive me and you, use it extensively:


http://www.romcal.net/output/2020.htm


It belongs with other fun and non-integral components, like
sopwith(6), in ports.


Ultimately that's one of the reasons why there is interest in a packaged 
base. There are plenty of tools  in base that people don't commonly use; 
we have to find consensus instead of imposing our own limited views.


Pedro.


[1] https://youtu.be/lj3iNxZ8Dww

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r358561 - in head: . share/man/man5 share/man/man7 tools/build/options tools/tools/nanobsd/dhcpd tools/tools/nanobsd/embedded usr.bin usr.bin/calendar usr.bin/calendar/calendars usr.bi

[Pedro Giffuni]

On 02/03/2020 18:37, Conrad Meyer wrote:

Author: cem
Date: Mon Mar  2 23:37:47 2020
New Revision: 358561
URL: https://svnweb.freebsd.org/changeset/base/358561

Log:
   Fix typo in r278616
   
   FreeBSD isn't an encyclopedia.


Wow, that was weird ... thank you for reverting.

Pedro.


Deleted:
   head/tools/build/options/WITHOUT_CALENDAR
   head/usr.bin/calendar/Makefile
   head/usr.bin/calendar/Makefile.depend
   head/usr.bin/calendar/calendar.1
   head/usr.bin/calendar/calendar.c
   head/usr.bin/calendar/calendar.h
   head/usr.bin/calendar/calendars/calendar.all
   head/usr.bin/calendar/calendars/calendar.australia
   head/usr.bin/calendar/calendars/calendar.birthday
   head/usr.bin/calendar/calendars/calendar.brazilian
   head/usr.bin/calendar/calendars/calendar.christian
   head/usr.bin/calendar/calendars/calendar.computer
   head/usr.bin/calendar/calendars/calendar.croatian
   head/usr.bin/calendar/calendars/calendar.dutch
   head/usr.bin/calendar/calendars/calendar.french
   head/usr.bin/calendar/calendars/calendar.german
   head/usr.bin/calendar/calendars/calendar.history
   head/usr.bin/calendar/calendars/calendar.holiday
   head/usr.bin/calendar/calendars/calendar.hungarian
   head/usr.bin/calendar/calendars/calendar.judaic
   head/usr.bin/calendar/calendars/calendar.lotr
   head/usr.bin/calendar/calendars/calendar.music
   head/usr.bin/calendar/calendars/calendar.newzealand
   head/usr.bin/calendar/calendars/calendar.russian
   head/usr.bin/calendar/calendars/calendar.southafrica
   head/usr.bin/calendar/calendars/calendar.ukrainian
   head/usr.bin/calendar/calendars/calendar.usholiday
   head/usr.bin/calendar/calendars/calendar.world
   head/usr.bin/calendar/calendars/de_AT.ISO_8859-15/
   head/usr.bin/calendar/calendars/de_DE.ISO8859-1/
   head/usr.bin/calendar/calendars/fr_FR.ISO8859-1/
   head/usr.bin/calendar/calendars/hr_HR.ISO8859-2/
   head/usr.bin/calendar/calendars/hu_HU.ISO8859-2/
   head/usr.bin/calendar/calendars/pt_BR.ISO8859-1/
   head/usr.bin/calendar/calendars/pt_BR.UTF-8/
   head/usr.bin/calendar/calendars/ru_RU.KOI8-R/
   head/usr.bin/calendar/calendars/ru_RU.UTF-8/
   head/usr.bin/calendar/calendars/uk_UA.KOI8-U/
   head/usr.bin/calendar/dates.c
   head/usr.bin/calendar/day.c
   head/usr.bin/calendar/events.c
   head/usr.bin/calendar/io.c
   head/usr.bin/calendar/locale.c
   head/usr.bin/calendar/ostern.c
   head/usr.bin/calendar/parsedata.c
   head/usr.bin/calendar/paskha.c
   head/usr.bin/calendar/pathnames.h
   head/usr.bin/calendar/pom.c
   head/usr.bin/calendar/sunpos.c
   head/usr.bin/calendar/tests/
   head/usr.sbin/periodic/etc/daily/300.calendar
Modified:
   head/ObsoleteFiles.inc
   head/share/man/man5/periodic.conf.5
   head/share/man/man5/src.conf.5
   head/share/man/man7/hier.7
   head/tools/tools/nanobsd/dhcpd/common
   head/tools/tools/nanobsd/embedded/common
   head/usr.bin/Makefile
   head/usr.bin/leave/leave.1
   head/usr.sbin/periodic/etc/daily/Makefile
   head/usr.sbin/periodic/periodic.conf

Modified: head/ObsoleteFiles.inc
==
--- head/ObsoleteFiles.inc  Mon Mar  2 23:25:02 2020(r358560)
+++ head/ObsoleteFiles.inc  Mon Mar  2 23:37:47 2020(r358561)
@@ -36,6 +36,11 @@
  #   xargs -n1 | sort | uniq -d;
  # done
  
+# 20200302: calendar(1) removed

+OLD_DIRS+=usr/share/calendar
+OLD_FILES+=usr/bin/calendar
+OLD_FILES+=usr/share/man/man1/calendar.1.gz
+
  # 20200301: bktr removed
  OLD_DIRS+=usr/include/dev/bktr
  OLD_FILES+=usr/include/dev/bktr/ioctl_bktr.h

Modified: head/share/man/man5/periodic.conf.5
==
--- head/share/man/man5/periodic.conf.5 Mon Mar  2 23:25:02 2020
(r358560)
+++ head/share/man/man5/periodic.conf.5 Mon Mar  2 23:37:47 2020
(r358561)
@@ -273,13 +273,6 @@ Set to
  if you want the
  .Pa /etc/mail/aliases
  file backed up and modifications to be displayed in your daily output.
-.It Va daily_calendar_enable
-.Pq Vt bool
-Set to
-.Dq Li YES
-if you want to run
-.Nm calendar Fl a
-daily.
  .It Va daily_accounting_enable
  .Pq Vt bool
  Set to
@@ -970,7 +963,6 @@ is shared or distributed.
  .El
  .Sh SEE ALSO
  .Xr apropos 1 ,
-.Xr calendar 1 ,
  .Xr df 1 ,
  .Xr diff 1 ,
  .Xr gzip 1 ,

Modified: head/share/man/man5/src.conf.5
==
--- head/share/man/man5/src.conf.5  Mon Mar  2 23:25:02 2020
(r358560)
+++ head/share/man/man5/src.conf.5  Mon Mar  2 23:37:47 2020
(r358561)
@@ -271,9 +271,6 @@ is set explicitly)
  .El
  .It Va WITHOUT_BZIP2_SUPPORT
  Set to build some programs without optional bzip2 support.
-.It Va WITHOUT_CALENDAR
-Set to not build
-.Xr calendar 1 .
  .It Va WITHOUT_CAPSICUM
  Set to not build Capsicum support into system programs.
  When set, it enforces these options:

Modified: head/share/man/man7/hier.7

Re: svn commit: r358459 - in head/contrib: gcc gcclibs

[Pedro Giffuni]

On 29/02/2020 07:40, Ed Maste wrote:

Author: emaste
Date: Sat Feb 29 12:40:27 2020
New Revision: 358459
URL: https://svnweb.freebsd.org/changeset/base/358459

Log:
   Remove contrib/gcc and contrib/gcclibs
   
   GCC 4.2.1 was disconnected from FreeBSD in r358454.
   
   Sponsored by:	The FreeBSD Foundation


Deleted:
   head/contrib/gcc/
   head/contrib/gcclibs/

And now it's really gone!

While I am no doubt very happy to see this happen, I just have to thank 
the FSF for keeping a competitive compiler for such a long time and 
raising the bar. When the project started and we needed a compiler, they 
were there.


We will also not miss it since we will continue to enjoy it as an 
important package in FreeBSD ports collection :).


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r358348 - in head/lib/libc: . gdtoa gen sparc64 sparc64/fpu sparc64/gen sparc64/sys sys

[Pedro Giffuni]

On 26/02/2020 18:09, Warner Losh wrote:



On Wed, Feb 26, 2020 at 3:47 PM Warner Losh > wrote:




On Wed, Feb 26, 2020 at 12:10 PM Bjoern A. Zeeb
mailto:bzeeb-li...@lists.zabbadoz.net>> wrote:

On 26 Feb 2020, at 18:55, Warner Losh wrote:

> Author: imp
> Date: Wed Feb 26 18:55:09 2020
> New Revision: 358348
> URL: https://svnweb.freebsd.org/changeset/base/358348
>
> Log:
>   Remove sparc64 specific parts of libc.

I have a silly question for which it’s long been too late, but
for the
next time .. why do we need a gazillion of commits to remove
sparc64
rather than 1 “atomic” one (or maybe 2 in case the one missed
a bit)
to remove it all (which would also allow other people to bring
it back
into private trees a lot more easily compared to tracking
changes over
weeks)?


One atomic commit is harder and more work for me.

It's hard to get all the details right before pushing it. It's
hard to develop it as other things in the tree change things which
leads to more conflicts. It can be hard to MFC around (though
these changes won't be MFC'd having too large a commit around them
may generate more conflicts when those things are MFC'd). So I
optimized for my convenience, not others wishing to import it into
their trees. But I'd contend that the delta as far as bringing
back as 10 commits isn't onerous for such a niche demand.


Also, if I screw something up, it's easier to back out smaller commits 
should that be necessary. Backing out huge commits that remove lots of 
things and then reapplying them is tedious and error-prone. Doing it a 
little at a time also allows me to make sure that all the CI stuff 
works before moving on to the next step.


We should/could nevertheless, track the removal process in some PR, or 
in the Wiki, to make life easier to whomever hero wants to resurrect the 
changes (not that I see that happening).


Pedro.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r358153 - head/usr.sbin/services_mkdb

[Pedro Giffuni]

On 22/02/2020 20:09, Steffen Nurpmeso wrote:

Hey, just so, because i posted to such a thing the last time.

Pedro Giffuni wrote in
:
  |On 22/02/2020 11:18, Florian Smeets wrote:
  |> On 20.02.20 04:54, Pedro F. Giffuni wrote:
  |>> Author: pfg
  |>> Date: Thu Feb 20 03:54:07 2020
  |>> New Revision: 358153
  |>> URL: https://svnweb.freebsd.org/changeset/base/358153
  |>>
  |>> Log:
  |>>/etc/services: attempt bring the database to this century.
  |>>
  |>> -smtps 465/tcp#smtp protocol over TLS/SSL (was ssmtp)
  |>> -smtps 465/udp#smtp protocol over TLS/SSL (was ssmtp)
  |> I'm not sure how removals of services have been handled in the past.
  |> This change broke loading of my pf rule set, as I had smtps in there.
  |
  |Excellent!
  |
  |Not that the change broke something but that since we had to revert it
  |we get a second chance to review such things.
  |
  |> I'm not saying that this change is wrong, but I think removing entries
  |> from services can break all kinds of stuff. Not just firewall rule sets,
  |> also scripts and thinking more about it, it will most certainly also
  |> break postfix as it also uses smtps as an alias for port 465 in its
  |> master.cnf
  |
  |According to latest IANA registy:
  ...

   kpasswd   464/udp   # kpasswd (Theodore Ts o)
   urd   465/tcp   # URL Rendezvous Directory for SSM (Toerless 
Eckert)
   submissions   465/tcp   # Message Submission over TLS protocol (IESG, 
IETF Chair, rfc8314) [2017-12-12]
   igmpv3lite465/udp   # IGMP over UDP for SSM (Toerless Eckert)
   digital-vrc   466/tcp   # digital-vrc (Peter Higginson)

Oh yes, they finally managed to overcome the SMTPS problems.
The RFC has a nice reading on that (as i seem to remember), yay IETF.
I am really happy.  (I never understood why POP3S and IMAPS where
done but SMTPS was not.)
Hmm .. I quoted the IANA list but I hadn't read the RFC. Interesting but 
I don't know if it solves Florian's issue.

  |Anything that can be done upstream to sort this out?
  |
  |> I guess this needs to be at least mentioned in the release notes, and
  |> maybe smtps kept as an alias, and check all the others that were removed?
  |
  |For the time being, we can absolutely keep the legacy value with a
  |conflict note. I wish the services list were a bit easier to maintain
  |for such situations.

Doesn't it just search until it finds the string?
Btw. i can only offer the simple awk script that i have for
updating services and protocols again, after the critics last time
i have evolved it from its ArchLinux base, and added a verbose
mode, as you can see above.  (That Theodore Ts'o missspelling is
IANA rooted.)  Whereas it made it more complicated, 139 lines for
download and preparation is not that much.


Interesting. There's also

https://reviews.freebsd.org/D17115

Where I made some comments.

Currently services_mkdb doesn't scale (which is why the patch was 
reverted), but beyond that the real problem is that we shouldn't just 
take the entries blindly. Many people abuse the registry for their 
startups and licensing services and then never de-register them.  In the 
case of NetBSD's services file, it currently has 21838 lines, which is 
bigger that the official IANA file.



Additions could simply be echoed?


I expect we maintain a relatively short list and have people send PRs 
for new entries (assuming they are registered).


BTW, we should probably go ahead and register our lockd in IANA as the 
port number already collides with something else and our use is 
propagating to other OSs (namely illumos).


Pedro.


--steffen
|
|Der Kragenbaer,The moon bear,
|der holt sich munter   he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r358248 - head/sys/vm

[Pedro Giffuni]

On 22/02/2020 14:37, Kyle Evans wrote:

On Sat, Feb 22, 2020 at 1:21 PM Pedro Giffuni  wrote:


On 22/02/2020 14:13, Ian Lepore wrote:

On Sat, 2020-02-22 at 20:01 +0100, Dimitry Andric wrote:

On 22 Feb 2020, at 17:44, Mateusz Guzik  wrote:

On 2/22/20, Kyle Evans  wrote:

On Sat, Feb 22, 2020 at 10:25 AM Ian Lepore 
wrote:

On Sat, 2020-02-22 at 16:20 +, Kyle Evans wrote:

Author: kevans
Date: Sat Feb 22 16:20:04 2020
New Revision: 358248
URL: https://svnweb.freebsd.org/changeset/base/358248

Log:
   vm_radix: prefer __builtin_unreachable() to an unreachable
panic()

   This provides the needed hint to GCC and offers an
annotation for
readers to
   observe that it's in-fact impossible to hit this point.
We'll get hit
with a
   a -Wswitch error if the enum applicable to the switch above
were to
get
   expanded without the new value(s) being handled.

Modified:
   head/sys/vm/vm_radix.c

Modified: head/sys/vm/vm_radix.c
=
=
--- head/sys/vm/vm_radix.cSat Feb 22 13:23:27
2020(r358247)
+++ head/sys/vm/vm_radix.cSat Feb 22 16:20:04
2020(r358248)
@@ -208,8 +208,7 @@ vm_radix_node_load(smrnode_t *p, enum
vm_radix_access
   case SMR:
   return (smr_entered_load(p, vm_radix_smr));
   }
- /* This is unreachable, silence gcc. */
- panic("vm_radix_node_get: Unknown access type");
+ __unreachable();
}

static __inline void

What does __unreachable() do if the code ever becomes
reachable?  Like
if a new enum value is added and this switch doesn't get
updated?


__unreachable doesn't help here, but the compiler will error out
on
the switch() if all enum values aren't addressed and there's no
default: case.

IMO, compilers could/should become smart enough to error if
there's an
explicit __builtin_unreachable() and they can trivially determine
that
all paths will terminate before this, independent of
-Werror=switch*.
___

I think this is way too iffy, check this program:


#include 

int
main(void)
{

 __builtin_unreachable();
 printf("test\n");
}

Neither clang nor gcc warn about this and both stop code generation
past the statement.

Indeed, that is exactly the intent.  See:



https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005funreachable

"If control flow reaches the point of the __builtin_unreachable, the
program is undefined. It is useful in situations where the compiler
cannot deduce the unreachability of the code."

E.g. this is *not* meant as a way to enforce the program to abort at
runtime, if the supposedly unreachable part is actually reached.

For this purpose, one should use an abort() or panic() function call,
with such functions being annotated to never return.

-Dimitry


The problem is, people will see usages such as what Kyle did, where the
code truly is unreachable (due to -Werror=switch), and not realizing
that's why it's valid there, they'll assume it's a type of assert-
unreachable and copy it/use it in other places as if that's what it was
for.

So, IMO, using it should be exceedingly rare and there should be a
comment nearby about why it's valid in that context, or our
__unreachable cover for it should panic on INVARIANTS, as Kyle proposed
in an earlier reply.

No __unreachable() as an attribute is meant as a hint for static
checkers and compiler optimizations. If you are unsure and want a panic,
you can add the panic message after the attribute. The compiler will
then be free to optimize out the panic, but that was the idea anyways.


The current form of __unreachable is only half-useful and apparently
prone to misuse, as has been pointed out earlier in this thread,
without any form of detection that it's been misused. IMO this is of
limited utility, but the review I had included you on will turn it
into a panic under INVARIANTS or into the proper __builtin_unreachable
for stable/release branches (read as "sans debugging"). I also noted
in the review that we didn't have to turn __unreachable into this, but
I had just done so initially, though I'd debate that there's a better
name for what it currently does (e.g. __hint_unreachable) to make it
sound less like an assertion.


The current form of __unreachable() is exactly just the compiler 
attribute on purpose. It indeed never got much use except for cleaning 
some unreachable paths detected by Coverity.


D2536, which was the predecessor of the attribute hinted the idea of a 
more metamorfic call in the lines of your proposal, which I don't think 
I like but I can live with, as the current __unreachable seems to have 
lived its original purpose.


Don't ask me to approve the differential though :)

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r358248 - head/sys/vm

[Pedro Giffuni]

On 22/02/2020 14:13, Ian Lepore wrote:

On Sat, 2020-02-22 at 20:01 +0100, Dimitry Andric wrote:

On 22 Feb 2020, at 17:44, Mateusz Guzik  wrote:

On 2/22/20, Kyle Evans  wrote:

On Sat, Feb 22, 2020 at 10:25 AM Ian Lepore 
wrote:

On Sat, 2020-02-22 at 16:20 +, Kyle Evans wrote:

Author: kevans
Date: Sat Feb 22 16:20:04 2020
New Revision: 358248
URL: https://svnweb.freebsd.org/changeset/base/358248

Log:
  vm_radix: prefer __builtin_unreachable() to an unreachable
panic()

  This provides the needed hint to GCC and offers an
annotation for
readers to
  observe that it's in-fact impossible to hit this point.
We'll get hit
with a
  a -Wswitch error if the enum applicable to the switch above
were to
get
  expanded without the new value(s) being handled.

Modified:
  head/sys/vm/vm_radix.c

Modified: head/sys/vm/vm_radix.c
=
=
--- head/sys/vm/vm_radix.cSat Feb 22 13:23:27
2020(r358247)
+++ head/sys/vm/vm_radix.cSat Feb 22 16:20:04
2020(r358248)
@@ -208,8 +208,7 @@ vm_radix_node_load(smrnode_t *p, enum
vm_radix_access
  case SMR:
  return (smr_entered_load(p, vm_radix_smr));
  }
- /* This is unreachable, silence gcc. */
- panic("vm_radix_node_get: Unknown access type");
+ __unreachable();
}

static __inline void

What does __unreachable() do if the code ever becomes
reachable?  Like
if a new enum value is added and this switch doesn't get
updated?


__unreachable doesn't help here, but the compiler will error out
on
the switch() if all enum values aren't addressed and there's no
default: case.

IMO, compilers could/should become smart enough to error if
there's an
explicit __builtin_unreachable() and they can trivially determine
that
all paths will terminate before this, independent of
-Werror=switch*.
___

I think this is way too iffy, check this program:


#include 

int
main(void)
{

__builtin_unreachable();
printf("test\n");
}

Neither clang nor gcc warn about this and both stop code generation
past the statement.

Indeed, that is exactly the intent.  See:



https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005funreachable

"If control flow reaches the point of the __builtin_unreachable, the
program is undefined. It is useful in situations where the compiler
cannot deduce the unreachability of the code."

E.g. this is *not* meant as a way to enforce the program to abort at
runtime, if the supposedly unreachable part is actually reached.

For this purpose, one should use an abort() or panic() function call,
with such functions being annotated to never return.

-Dimitry


The problem is, people will see usages such as what Kyle did, where the
code truly is unreachable (due to -Werror=switch), and not realizing
that's why it's valid there, they'll assume it's a type of assert-
unreachable and copy it/use it in other places as if that's what it was
for.

So, IMO, using it should be exceedingly rare and there should be a
comment nearby about why it's valid in that context, or our
__unreachable cover for it should panic on INVARIANTS, as Kyle proposed
in an earlier reply.


No __unreachable() as an attribute is meant as a hint for static 
checkers and compiler optimizations. If you are unsure and want a panic, 
you can add the panic message after the attribute. The compiler will 
then be free to optimize out the panic, but that was the idea anyways.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r358152 - head/bin/sh

[Pedro Giffuni]

For the record ...

On 21/02/2020 22:31, Kyle Evans wrote:

On Fri, Feb 21, 2020 at 3:53 PM Li-Wen Hsu  wrote:

On Sat, Feb 22, 2020 at 4:58 AM Antoine Brodin  wrote:

On Thu, Feb 20, 2020 at 4:01 AM Hiroki Sato  wrote:

Author: hrs
Date: Thu Feb 20 03:01:27 2020
New Revision: 358152
URL: https://svnweb.freebsd.org/changeset/base/358152

Log:
   Improve performance of "read" built-in command when using a seekable
   fd.

   The read built-in command calls read(2) with a 1-byte buffer because
   newline characters need to be detected even on a byte stream which
   comes from a non-seekable file descriptor.  Because of this, the
   following script calls >6,000 read(2) to show a 6KiB file:

while read IN; do echo "$IN"; done < /COPYRIGHT

   When the input byte stream is seekable, it is possible to read a data
   block and then reposition the file pointer to where a newline
   character found.  This change adds a small buffer to do this and
   reduces the number of read(2) calls.

   Theoretically, multiple built-in commands reading the same seekable
   byte stream in a single pipe chain can share the buffer.  However,
   this change just makes a single invocation of the read built-in
   allocate a buffer and deallocate it every time for simplicity.
   Although this causes read(2) to read the same regions multiple times,
   the performance penalty should be small compared to the reduction of
   read(2) calls.

   Reviewed by:  jilles
   MFC after:1 week
   Differential Revision:https://reviews.freebsd.org/D23747

This seems to be broken on at least i386.
Please either fix or revert.

Antoine (with hat: portmgr)

Could you provide more detail?  I'm worried because I didn't see
related regression from the recent test results. We may need to add
more test against the breakage you mentioned.


This trivially failed with the example in the commit message; only the
first line would be output. It also triggered a failure of
functional_test:read2 in /usr/tests/bin/sh/builtins on i386 (and all
of the other platforms with a 32-bit size_t), which would exit with a
non-zero status code.

I tested and deployed the fix suggested by cem@ as r358235 by just
making residue an off_t,


This is an example case of why it is important to keep the i386 port 
building and running. If we don't have a 32 bit port that is easy to 
build and test many bugs like these can easily go through.


Cheers,

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r358153 - head/usr.sbin/services_mkdb

[Pedro Giffuni]

On 22/02/2020 11:18, Florian Smeets wrote:

On 20.02.20 04:54, Pedro F. Giffuni wrote:

Author: pfg
Date: Thu Feb 20 03:54:07 2020
New Revision: 358153
URL: https://svnweb.freebsd.org/changeset/base/358153

Log:
   /etc/services: attempt bring the database to this century.
   
-smtps		465/tcp	   #smtp protocol over TLS/SSL (was ssmtp)

-smtps  465/udp#smtp protocol over TLS/SSL (was ssmtp)

I'm not sure how removals of services have been handled in the past.
This change broke loading of my pf rule set, as I had smtps in there.


Excellent!

Not that the change broke something but that since we had to revert it 
we get a second chance to review such things.




I'm not saying that this change is wrong, but I think removing entries
from services can break all kinds of stuff. Not just firewall rule sets,
also scripts and thinking more about it, it will most certainly also
break postfix as it also uses smtps as an alias for port 465 in its
master.cnf


According to latest IANA registy:

urd 465    tcp    URL Rendezvous Directory for 
[Toerless_Eckert] [Toerless_Eckert]

  SSM
submissions 465    tcp    Message Submission over TLS [IESG] 
[IETF_Chair] 2017-12-12    [RFC8314]

  protocol
igmpv3lite  465    udp    IGMP over UDP for SSM 
[Toerless_Eckert] [Toerless_Eckert]


Anything that can be done upstream to sort this out?


I guess this needs to be at least mentioned in the release notes, and
maybe smtps kept as an alias, and check all the others that were removed?


For the time being, we can absolutely keep the legacy value with a 
conflict note. I wish the services list were a bit easier to maintain 
for such situations.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r358153 - head/usr.sbin/services_mkdb

[Pedro Giffuni]

On 20/02/2020 10:09, Ed Maste wrote:

On Thu, 20 Feb 2020 at 09:54, Pedro Giffuni  wrote:

On 2020-02-20 09:46, Ed Maste wrote:

On Thu, 20 Feb 2020 at 04:56, Li-Wen Hsu  wrote:

Please note this is not fixed in ci.freebsd.org yet. The reason is it
directly goes to src/release and perform `make packagesystem` to
generate kernel.txz and base.txz.  If I read release(7) correctly,
this should be performed under a fresh installed environment.  I am
checking how to fix this.

Should we revert the change until a build fix can be committed too?

I am OK with reverting the change, and I am rather surprised such a
simply change could have broken the build.

Indeed, this has uncovered a broken dependency in our build process
(i.e., that the build host's services_mkdb is being used).

IMO we should:

1. Revert the change for now (pfg will you take care of it?)


Yes, Give me some minutes ...



2. MFC the MAX_PROTO increase as soon as possible
3. Try to fix the use of host's services_mkdb
4. Recommit after either #3 is done, or FreeBSD 12.2 and 11.4 are released



Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r358153 - head/usr.sbin/services_mkdb

[Pedro Giffuni]

On 2020-02-20 09:46, Ed Maste wrote:
> On Thu, 20 Feb 2020 at 04:56, Li-Wen Hsu  wrote:
>> Please note this is not fixed in ci.freebsd.org yet. The reason is it
>> directly goes to src/release and perform `make packagesystem` to
>> generate kernel.txz and base.txz.  If I read release(7) correctly,
>> this should be performed under a fresh installed environment.  I am
>> checking how to fix this.
> Should we revert the change until a build fix can be committed too?

I am OK with reverting the change, and I am rather surprised such a
simply change could have broken the build.

This said, it is probably good that the error is reproducible.

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r358153 - head/usr.sbin/services_mkdb

[Pedro Giffuni]

 blockquote, div.yahoo_quoted { margin-left: 0 !important; border-left:1px 
#715FFA solid !important; padding-left:1ex !important; background-color:white 
!important; } 
Oops! Sorry was AFK ...
Thanks!

Pedro.


On Thursday, February 20, 2020, 1:06 AM, Xin Li  wrote:



On 2/19/20 10:01 PM, Yuri Pankov wrote:
> On 20 Feb 2020, at 06:54, Pedro F. Giffuni  wrote:
>>
>> Author: pfg
>> Date: Thu Feb 20 03:54:07 2020
>> New Revision: 358153
>> URL: https://svnweb.freebsd.org/changeset/base/358153
>>
>> Log:
>>  /etc/services: attempt bring the database to this century.
>>
>>  Document better this file, updating the URL to the IANA registry and closely
>>  match the official services.
>>
>>  For system ports (0 to 1023) we now try to follow the registry closely, 
>>noting
>>  some historical differences where applicable.
>>  For the User ports (1024 - 49151) we try to keep some sensible balance only
>>  of services that are likely to be found on FreeBSD/UNIX systems. This 
>>attempts
>>  to strike a balance between complexity and usefulness.
>>
>>  As a side effect: drop references to unofficial Kerberos IV which was EOL'ed
>>  on Oct 2006[1]. While it is conceivable some people may still use it in some
>>  very old FreeBSD machines that can't be replaced easily, the use of it is
>>  considered a security risk. Also drop the unofficial netatalk, which we
>>  supported long ago in the kernel but was dropped long ago.
>>
>>  [1] https://web.mit.edu/kerberos/krb4-end-of-life.html
>>
>>  MFC after:    3 weeks (likely to 12-stable only)
>>  Differential Revision:    https://reviews.freebsd.org/D23621
>>
>> Modified:
>>  head/usr.sbin/services_mkdb/services
> 
> I noticed `mergemaster` failing, and it seems to be not happy with this 
> change:
> 
> # make installconfig
> installing DIRS CONFSDIR
> install  -d -m 0755 -o root  -g wheel  /etc
> install  -C -o root  -g wheel -m 644  
> /usr/src/usr.sbin/services_mkdb/services /etc/services
> services_mkdb -l -q -o /var/db/services.db  /etc/services
> services_mkdb: Ran out of protocols adding `divert'; recompile with larger 
> PROTOMAX
> *** Error code 1
> 
> Stop.
> make: stopped in /usr/src/usr.sbin/services_mkdb

Fixed in r358154.  Please merge the revision with r358153 when MFC'ing.



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r357728 - head/sys/sys

[Pedro Giffuni]

Hi;

On 10/02/2020 08:52, Mateusz Guzik wrote:

Author: mjg
Date: Mon Feb 10 13:52:25 2020
New Revision: 357728
URL: https://svnweb.freebsd.org/changeset/base/357728

Log:
   Tidy up zpcpu_replace*
   
   - only compute the target address once

   - remove spurious type casting, zpcpu_get already return the correct type
   
   While here add missing newlines to other routines.


Modified:
   head/sys/sys/pcpu.h


For the record, this file (and many others), uses a space after #define, 
when we should be using a tab to conform with style(9). One of the many 
lessons from bde.


I have a huge patch to fix those, which I won't commit because it wouldl 
obliterate all VCS annotations.


Pedro.



Modified: head/sys/sys/pcpu.h
==
--- head/sys/sys/pcpu.h Mon Feb 10 13:24:14 2020(r357727)
+++ head/sys/sys/pcpu.h Mon Feb 10 13:52:25 2020(r357728)
@@ -245,32 +245,41 @@ extern struct pcpu *cpuid_to_pcpu[];
   * If you need atomicity use xchg.
   * */
  #define zpcpu_replace(base, val) ({   \
-   __typeof(val) _old = *(__typeof(base))zpcpu_get(base);  \
-   *(__typeof(val) *)zpcpu_get(base) = val;\
+   __typeof(val) *_ptr = zpcpu_get(base);  \
+   __typeof(val) _old; \
+   \
+   _old = *_ptr;   \
+   *_ptr = val;\
_old;   \
  })
  
  #define zpcpu_replace_cpu(base, val, cpu) ({\

-   __typeof(val) _old = *(__typeof(base))zpcpu_get_cpu(base, cpu); \
-   *(__typeof(val) *)zpcpu_get_cpu(base, cpu) = val;   \
+   __typeof(val) *_ptr = zpcpu_get_cpu(base, cpu); \
+   __typeof(val) _old; \
+   \
+   _old = *_ptr;   \
+   *_ptr = val;\
_old;   \
  })
  
  #define zpcpu_set_protected(base, val) ({\

MPASS(curthread->td_critnest > 0);\
__typeof(val) *_ptr = zpcpu_get(base);  \
+   \
*_ptr = (val);  \
  })
  
  #define zpcpu_add_protected(base, val) ({\

MPASS(curthread->td_critnest > 0);\
__typeof(val) *_ptr = zpcpu_get(base);  \
+   \
*_ptr += (val); \
  })
  
  #define zpcpu_sub_protected(base, val) ({\

MPASS(curthread->td_critnest > 0);\
__typeof(val) *_ptr = zpcpu_get(base);  \
+   \
*_ptr -= (val); \
  })
  

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r356758 - in head/usr.sbin/bsdinstall: . scripts

[Pedro Giffuni]

On 15/01/2020 16:41, Ed Maste wrote:

On Wed, 15 Jan 2020 at 16:10, Eugene Grosbein  wrote:

There are multiple scenarios there ZFS may be sub-optimal at least: small i386 
virtual guests
or 32-bit only hardware like AMD Geode, or big amd64 SSD-only systems with 
bhyve and multiple guests
that need lots of memory and should not fight with ZFS for RAM etc.

That may well be the case, but our defaults should represent the
configuration that's desirable to the largest set of users, and IMO
that's ZFS in most cases today.


There is also the policy of not making copyleft code mandatory 
(technically, CDDL is weak copyleft).


If ZFS is disabled in the build, the installer should gracefully disable 
it too.




It might be that we should default to UFS on i386 and ZFS on amd64?


FWIW, I still use ZFS for root, because of the older MBR and the need to 
multiboot.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r356714 - head/sys/ufs/ffs

[Pedro Giffuni]

On 13/01/2020 21:00, Jeff Roberson wrote:

Author: jeff
Date: Tue Jan 14 02:00:24 2020
New Revision: 356714
URL: https://svnweb.freebsd.org/changeset/base/356714

Log:
   Fix a long standing bug in journaled soft-updates.  The dirrem structure
   needs to handle file removal, directory removal, file move, directory move,
   etc.  The code in handle_workitem_remove() needs to propagate any completed
   journal entries to the write that will render the change stable.  In the
   case of a moved directory this means the new parent.  However, for an
   overwrite that frees a directory (DIRCHG) we must move the jsegdep to the
   removed inode to be released when it is stable in the cg bitmap or the
   unlinked inode list.  This case was previously unhandled and caused a
   panic.
   
   Reported by:	mckusick, pho

   Reviewed by: mckusick
   Tested by:   pho


MFC ? I mean if it's a long standing bug + unhandled + causing a panic 
... sounds like it really should be merged.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r356367 - in head: . share/mk

[Pedro Giffuni]

FWIW,

On 04/01/2020 21:47, Ed Maste wrote:

Author: emaste
Date: Sun Jan  5 02:47:56 2020
New Revision: 356367
URL: https://svnweb.freebsd.org/changeset/base/356367

Log:
   Do not build GCC 4.2.1 by default for any CPU architecture



 It's really nice how the compiler warning trend went down.

On FreeBSD-head-mips-build* it went from 7691 warnings to 473 + 83.

Cheers,

Pedro.

*https://ci.freebsd.org/job/FreeBSD-head-mips-build/

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r356185 - in head: lib/geom lib/geom/sched sys/geom sys/geom/sched sys/modules/geom sys/modules/geom/geom_sched sys/sys

[Pedro Giffuni]

On 2019-12-30 06:32, Alexey Dokuchaev wrote:
> On Sun, Dec 29, 2019 at 09:16:04PM +, Alexander Motin wrote:
>> New Revision: 356185
>> URL: https://svnweb.freebsd.org/changeset/base/356185
>>
>> Log:
>>   Remove GEOM_SCHED class and gsched tool.
>>   
>>   This code was not actively maintained since it was introduced 10 years ago.
>>   It lacks support for many later GEOM features, such as direct dispatch,
>>   unmapped I/O, stripesize/stripeoffset, resize, etc.  Plus it is the only
>>   remaining use of GEOM nstart/nend request counters, used there to implement
>>   live insertion/removal, questionable by itself.
> Wow, that was unexpected, I use it on all my machines' HDD drives.


There was a posting by mav@ on the -arch list : " gsched: modernize or
remove?".

It was a short notice (Dec 27), but there was strong agreement from the
author.

> Is there a planned replacement, or I'd better create a port for the
> GEOM_SCHED class and gsched(8) tool?

If you can convince someone to update it with the enhancements suggested
in the -arch post, I guess it could be resurrected, otherwise a port.

Pedro.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r356142 - in head/sys: dev/ofw sys

[Pedro Giffuni]

On 28/12/2019 00:27, Rodney W. Grimes wrote:

[ Charset UTF-8 unsupported, converting... ]

On 2019-12-27 23:24, Rodney W. Grimes wrote:

[ Charset UTF-8 unsupported, converting... ]

On 2019-12-27 22:16, Rodney W. Grimes wrote:

Author: pfg
Date: Sat Dec 28 02:58:30 2019
New Revision: 356142
URL: https://svnweb.freebsd.org/changeset/base/356142

Log:
   SPDX: update some tags with two licenses.

Modified:
   head/sys/dev/ofw/openfirm.h
   head/sys/sys/sched.h

Modified: head/sys/dev/ofw/openfirm.h
==
--- head/sys/dev/ofw/openfirm.h Sat Dec 28 02:11:41 2019(r356141)
+++ head/sys/dev/ofw/openfirm.h Sat Dec 28 02:58:30 2019(r356142)
@@ -1,7 +1,7 @@
  /*$NetBSD: openfirm.h,v 1.1 1998/05/15 10:16:00 tsubai Exp $  */
  
  /*-

- * SPDX-License-Identifier: BSD-4-Clause
+ * SPDX-License-Identifier: (BSD-4-Clause AND BSD-2-Clause-FreeBSD)
   *
   * Copyright (C) 1995, 1996 Wolfgang Solfrank.
   * Copyright (C) 1995, 1996 TooLs GmbH.

Modified: head/sys/sys/sched.h
==
--- head/sys/sys/sched.hSat Dec 28 02:11:41 2019(r356141)
+++ head/sys/sys/sched.hSat Dec 28 02:58:30 2019(r356142)
@@ -1,5 +1,5 @@
  /*-
- * SPDX-License-Identifier: BSD-4-Clause
+ * SPDX-License-Identifier: (BSD-4-Clause AND BSD-2-Clause-FreeBSD)
   *
   * Copyright (c) 1996, 1997
   *  HD Associates, Inc.  All rights reserved.


This situation should not of occured, and leads to an ambigous license state.

It actually happens a lot (I mean two or more licenses in the same
file): SPDX explicitly uses AND (not OR) for cases like this.


What code is under license 2 clause and what under 4 clause?

Anyone redistributing the file has to respect both licenses. If you are
lucky enough to have access to version control you may be able to
discern the author and the corresponding license, otherwise you are
trapped with both.

So the 2 clause add is null, so why have it there?

So that eventually, when the project gets to a point where sufficient
part of the code is rewritten they can opt to change the license to the
simpler form. There are ways to relicense projects gradually, and its
nothing new, in fact it is very much in the BSD spirit to gradually
replace more restricted UNIX code.

The only changing we have done to BSD licenses as in thost cases
that the Regents requested/granted the right to change to lesser
clauses.  Until you get HD & Associtates (in this one case) to
grant that right your walking on a grey edge I would rather not
walk on.



As an independent developer I don't have to adopt on my code 
restrictions that other developers have adopted for their code.




The reference to BSD spirit and replacing more restricted UNIX (tm)
code is way off base in this context.  This is not an AT & T
license we are talking about here.  And again you can not just
modify the existing 4 clause licensed file by slapping a 2 clause
license into it, or the project would of done that everyplace
ages ago.


We are talking about restrictions. You probably missed it, but in the 
late 90's the project opted for removing restrictions, even if that 
meant the code could eventually end up in copyleft codebases. FreeBSD 
has been flexible and pragmatic about it but the other BSDs made 
basically the same decision.



What is done here in this file is a mistake, and should be corrected.
Can you point me to other files that actually have multiple BSD
licenses in them?


I have better things to do in my holidays, but there are plenty.

I will note for reference that this is indeed official project policy:

https://www.freebsd.org/internal/software-license.html

" ... We invite and greatly appreciate the contribution of both changes 
and additions under the two-clause BSD license, and encourage the 
adoption of this license by other open source projects."



It may be a long shot but it has happened on other projects as well:
libdialog (in our tree) was rewritten and relicensed from GPL to LGPL.



It looks to me as if this was done by Jeff Robinson as the 2 clause is
attached to his copyright and we should probably just ask him to relax
that back to the files existing 4 clause license, and or go after Greg
Ansley of HD associtates to get them to relax the 4 clause.


No, Jeff (or anyone else, as I said there are many cases in our tree) is
entitled to choose his own license as long as it is compatible with the
pre-existing licensing.

I was specifically sighting this one file, sys/sys/sched.h.

Actually that might be a grey area, no place does the BSD license grant
you rights to modify the terms of the license, and that is in effect
what adding this second license does.

No one is modifying the original license: it is there and applies to the
original code.



You can choose your own license for original work, sure, but obliterating
parts of an existing 

Re: svn commit: r356142 - in head/sys: dev/ofw sys

[Pedro Giffuni]

On 2019-12-27 23:24, Rodney W. Grimes wrote:
> [ Charset UTF-8 unsupported, converting... ]
>> On 2019-12-27 22:16, Rodney W. Grimes wrote:
 Author: pfg
 Date: Sat Dec 28 02:58:30 2019
 New Revision: 356142
 URL: https://svnweb.freebsd.org/changeset/base/356142

 Log:
   SPDX: update some tags with two licenses.

 Modified:
   head/sys/dev/ofw/openfirm.h
   head/sys/sys/sched.h

 Modified: head/sys/dev/ofw/openfirm.h
 ==
 --- head/sys/dev/ofw/openfirm.hSat Dec 28 02:11:41 2019
 (r356141)
 +++ head/sys/dev/ofw/openfirm.hSat Dec 28 02:58:30 2019
 (r356142)
 @@ -1,7 +1,7 @@
  /*$NetBSD: openfirm.h,v 1.1 1998/05/15 10:16:00 tsubai Exp $  
 */
  
  /*-
 - * SPDX-License-Identifier: BSD-4-Clause
 + * SPDX-License-Identifier: (BSD-4-Clause AND BSD-2-Clause-FreeBSD)
   *
   * Copyright (C) 1995, 1996 Wolfgang Solfrank.
   * Copyright (C) 1995, 1996 TooLs GmbH.

 Modified: head/sys/sys/sched.h
 ==
 --- head/sys/sys/sched.h   Sat Dec 28 02:11:41 2019(r356141)
 +++ head/sys/sys/sched.h   Sat Dec 28 02:58:30 2019(r356142)
 @@ -1,5 +1,5 @@
  /*-
 - * SPDX-License-Identifier: BSD-4-Clause
 + * SPDX-License-Identifier: (BSD-4-Clause AND BSD-2-Clause-FreeBSD)
   *
   * Copyright (c) 1996, 1997
   *  HD Associates, Inc.  All rights reserved.

>>> This situation should not of occured, and leads to an ambigous license 
>>> state.
>> It actually happens a lot (I mean two or more licenses in the same
>> file): SPDX explicitly uses AND (not OR) for cases like this.
>>
>>> What code is under license 2 clause and what under 4 clause? 
>> Anyone redistributing the file has to respect both licenses. If you are
>> lucky enough to have access to version control you may be able to
>> discern the author and the corresponding license, otherwise you are
>> trapped with both.
> So the 2 clause add is null, so why have it there?

So that eventually, when the project gets to a point where sufficient
part of the code is rewritten they can opt to change the license to the
simpler form. There are ways to relicense projects gradually, and its
nothing new, in fact it is very much in the BSD spirit to gradually
replace more restricted UNIX code.

It may be a long shot but it has happened on other projects as well:
libdialog (in our tree) was rewritten and relicensed from GPL to LGPL.


>>> It looks to me as if this was done by Jeff Robinson as the 2 clause is
>>> attached to his copyright and we should probably just ask him to relax
>>> that back to the files existing 4 clause license, and or go after Greg
>>> Ansley of HD associtates to get them to relax the 4 clause.
>>>
>> No, Jeff (or anyone else, as I said there are many cases in our tree) is
>> entitled to choose his own license as long as it is compatible with the
>> pre-existing licensing.
> I was specifically sighting this one file, sys/sys/sched.h.
>
> Actually that might be a grey area, no place does the BSD license grant
> you rights to modify the terms of the license, and that is in effect
> what adding this second license does.

No one is modifying the original license: it is there and applies to the
original code.


> You can choose your own license for original work, sure, but obliterating
> parts of an existing license by applying a second license which is in
> conflict is probably a poor idea.


We don't do that at all: pretty clearly there is no conflict between
both licenses as you can comply with both.

Pedro.



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r356142 - in head/sys: dev/ofw sys

[Pedro Giffuni]

On 2019-12-27 22:16, Rodney W. Grimes wrote:
>> Author: pfg
>> Date: Sat Dec 28 02:58:30 2019
>> New Revision: 356142
>> URL: https://svnweb.freebsd.org/changeset/base/356142
>>
>> Log:
>>   SPDX: update some tags with two licenses.
>>
>> Modified:
>>   head/sys/dev/ofw/openfirm.h
>>   head/sys/sys/sched.h
>>
>> Modified: head/sys/dev/ofw/openfirm.h
>> ==
>> --- head/sys/dev/ofw/openfirm.h  Sat Dec 28 02:11:41 2019
>> (r356141)
>> +++ head/sys/dev/ofw/openfirm.h  Sat Dec 28 02:58:30 2019
>> (r356142)
>> @@ -1,7 +1,7 @@
>>  /*  $NetBSD: openfirm.h,v 1.1 1998/05/15 10:16:00 tsubai Exp $  */
>>  
>>  /*-
>> - * SPDX-License-Identifier: BSD-4-Clause
>> + * SPDX-License-Identifier: (BSD-4-Clause AND BSD-2-Clause-FreeBSD)
>>   *
>>   * Copyright (C) 1995, 1996 Wolfgang Solfrank.
>>   * Copyright (C) 1995, 1996 TooLs GmbH.
>>
>> Modified: head/sys/sys/sched.h
>> ==
>> --- head/sys/sys/sched.h Sat Dec 28 02:11:41 2019(r356141)
>> +++ head/sys/sys/sched.h Sat Dec 28 02:58:30 2019(r356142)
>> @@ -1,5 +1,5 @@
>>  /*-
>> - * SPDX-License-Identifier: BSD-4-Clause
>> + * SPDX-License-Identifier: (BSD-4-Clause AND BSD-2-Clause-FreeBSD)
>>   *
>>   * Copyright (c) 1996, 1997
>>   *  HD Associates, Inc.  All rights reserved.
>>
> This situation should not of occured, and leads to an ambigous license state.

It actually happens a lot (I mean two or more licenses in the same
file): SPDX explicitly uses AND (not OR) for cases like this.

> What code is under license 2 clause and what under 4 clause? 

Anyone redistributing the file has to respect both licenses. If you are
lucky enough to have access to version control you may be able to
discern the author and the corresponding license, otherwise you are
trapped with both.

> It looks to me as if this was done by Jeff Robinson as the 2 clause is
> attached to his copyright and we should probably just ask him to relax
> that back to the files existing 4 clause license, and or go after Greg
> Ansley of HD associtates to get them to relax the 4 clause.
>
No, Jeff (or anyone else, as I said there are many cases in our tree) is
entitled to choose his own license as long as it is compatible with the
pre-existing licensing.

Pedro.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r355828 - head/sys/sys

[Pedro Giffuni]

On 17/12/2019 18:07, Brooks Davis wrote:

On Tue, Dec 17, 2019 at 01:28:20PM -0500, Pedro Giffuni wrote:

On 16/12/2019 23:42, Cy Schubert wrote:

In message <201912162355.xbgntuq6078...@repo.freebsd.org>, "Pedro F.
Giffuni" w
rites:

Author: pfg
Date: Mon Dec 16 23:55:30 2019
New Revision: 355828
URL: https://svnweb.freebsd.org/changeset/base/355828

Log:
Double the size of ARG_MAX on LP64 platforms.

As modern software keeps growing in size, we get requests to update the

value of ARG_MAX in order to link the resulting object files. Other OSs
have much higher values but Increasiong ARG_MAX has a multiplied effect on
KVA, so just bumping this value is dangerous in some archs like ARM32 that
can exhaust KVA rather easily.

While it would be better to have a unique value for all archs, other OSs

(Illumos in partidular) can have different ARG_MAX limits depending on the
platform,  For now we want to be really conservative so we are avoidng
the change on ILP32 and in the alternative case we only double it since tha
t
seems to work well enough for recent Code Aster.

I was planning to bump the _FreeBSD_version but it was bumped recently

(r355798) so we can reuse the 1300068 value for this change.

This doesn't seem right. Each bump should be for a distinct change and
documented as such.

TBH, it is just not worth it: this change will currently benefit only
one port (french/aster) and the update won't be committed until after
the MFC is done.

An MFC is a quite long-term solution.  If merged to 11 and 12 then any
workarounds can't be removed until 11.3 and 12.1 are EOL since we'll be
building packages there until that point.


Yes. I am planning to MFC only to 12-stable as 11-stable may not be 
worth bothering.


Pedro.


-- Brooks

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r355828 - head/sys/sys

[Pedro Giffuni]

On 16/12/2019 23:42, Cy Schubert wrote:

In message <201912162355.xbgntuq6078...@repo.freebsd.org>, "Pedro F.
Giffuni" w
rites:

Author: pfg
Date: Mon Dec 16 23:55:30 2019
New Revision: 355828
URL: https://svnweb.freebsd.org/changeset/base/355828

Log:
   Double the size of ARG_MAX on LP64 platforms.
   
   As modern software keeps growing in size, we get requests to update the

   value of ARG_MAX in order to link the resulting object files. Other OSs
   have much higher values but Increasiong ARG_MAX has a multiplied effect on
   KVA, so just bumping this value is dangerous in some archs like ARM32 that
   can exhaust KVA rather easily.
   
   While it would be better to have a unique value for all archs, other OSs

   (Illumos in partidular) can have different ARG_MAX limits depending on the
   platform,  For now we want to be really conservative so we are avoidng
   the change on ILP32 and in the alternative case we only double it since tha
t
   seems to work well enough for recent Code Aster.
   
   I was planning to bump the _FreeBSD_version but it was bumped recently

   (r355798) so we can reuse the 1300068 value for this change.

This doesn't seem right. Each bump should be for a distinct change and
documented as such.


TBH, it is just not worth it: this change will currently benefit only 
one port (french/aster) and the update won't be committed until after 
the MFC is done.


It is rather more painful that the change only fixes the case for some 
platforms.


Pedro.


Also, it's not like we're saving any build time anyway. sys/syslimits.h
will cause a substantial portion to be rebuilt anyway.

   
   PR:		241710

   MFC after:   5 days

Modified:
   head/sys/sys/syslimits.h

Modified: head/sys/sys/syslimits.h
=
=
--- head/sys/sys/syslimits.hMon Dec 16 23:08:09 2019(r355827)
+++ head/sys/sys/syslimits.hMon Dec 16 23:55:30 2019(r355828)
@@ -48,7 +48,11 @@
   * Do not add any new variables here.  (See the comment at the end of
   * the file for why.)
   */
-#defineARG_MAX 262144  /* max bytes for an exec functi
on */
+#ifndef __ILP32__
+#defineARG_MAX   (2 * 256 * 1024)  /* max bytes for an exec functi
on */
+#else
+#defineARG_MAX   (256 * 1024)  /* max bytes for KVA-starved ar
chs */
+#endif
  #ifndef CHILD_MAX
  #define   CHILD_MAX  40   /* max simultaneous processes *
/
  #endif




___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r355747 - in head: . include lib/libc/stdlib lib/libxo

[Pedro Giffuni]

On 14/12/2019 13:20, Ryan Libby wrote:

On Sat, Dec 14, 2019 at 12:28 AM Conrad Meyer  wrote:

Author: cem
Date: Sat Dec 14 08:28:10 2019
New Revision: 355747
URL: https://svnweb.freebsd.org/changeset/base/355747

Log:
   Deprecate sranddev(3) API

   It serves no useful purpose and wasn't as popular as its equally meritless
   cousin, srandomdev(3).

.. (cut unrelated code)

Modified: head/include/stdlib.h
==
--- head/include/stdlib.h   Sat Dec 14 05:21:56 2019(r355746)
+++ head/include/stdlib.h   Sat Dec 14 08:28:10 2019(r355747)
@@ -309,12 +309,17 @@ intrpmatch(const char *);
  voidsetprogname(const char *);
  int sradixsort(const unsigned char **, int, const unsigned char *,
 unsigned);
-voidsranddev(void);
  voidsrandomdev(void);
  long long
 strtonum(const char *, long long, long long, const char **);

  /* Deprecated interfaces, to be removed. */
+static inline void
+__attribute__((__deprecated__("sranddev to be removed in FreeBSD 13")))
+sranddev(void)
+{
+}
+

This broke some gcc builds in ci.  It looks like older versions of gcc
don't like having an argument to deprecated.


FWIW, the msg argument for __deprecated__ appeared in GCC 4.5.

Pedro.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r354672 - head/lib/libc/secure

[Pedro Giffuni]

On 13/11/2019 13:23, Warner Losh wrote:



On Wed, Nov 13, 2019 at 8:52 AM Pedro Giffuni <mailto:p...@freebsd.org>> wrote:


Hi;

On 12/11/2019 23:44, Warner Losh wrote:



On Tue, Nov 12, 2019 at 9:20 PM Kyle Evans mailto:kev...@freebsd.org>> wrote:



On Tue, Nov 12, 2019, 22:04 Pedro Giffuni mailto:p...@freebsd.org>> wrote:


On 12/11/2019 22:00, Kyle Evans wrote:

Author: kevans
Date: Wed Nov 13 03:00:32 2019
New Revision: 354672
URL:https://svnweb.freebsd.org/changeset/base/354672

Log:
   ssp: rework the logic to use priority=200 on clang builds
   
   The preproc logic was added at the last minute to appease GCC 4.2, and

   kevans@ did clearly not go back and double-check that the logic 
worked out
   for clang builds to use the new variant.
   
   It turns out that clang defines __GNUC__ == 4. Flip it around and check

   __clang__ as well, leaving a note to remove it later.
   

clang reports itself as GCC 4.2, the priority argument
was introduced in GCC 4.3.

   Reported by: cem

Modified:
   head/lib/libc/secure/stack_protector.c

Modified: head/lib/libc/secure/stack_protector.c

==
--- head/lib/libc/secure/stack_protector.c  Wed Nov 13 02:22:00 
2019(r354671)
+++ head/lib/libc/secure/stack_protector.c  Wed Nov 13 03:00:32 
2019(r354672)
@@ -47,13 +47,15 @@ __FBSDID("$FreeBSD$");
   * they're either not usually statically linked or they simply 
don't do things
   * in constructors that would be adversely affected by their 
positioning with
   * respect to this initialization.
+ *
+ * This conditional should be removed when GCC 4.2 is removed.
   */
-#if defined(__GNUC__) && __GNUC__ <= 4
-#define_GUARD_SETUP_CTOR_ATTR  \
-__attribute__((__constructor__, __used__));
-#else
+#if defined(__clang__) || (defined(__GNUC__) && __GNUC__ > 4)
  #define   _GUARD_SETUP_CTOR_ATTR   \
  __attribute__((__constructor__ (200), __used__));
+#else
+#define_GUARD_SETUP_CTOR_ATTR  \
+__attribute__((__constructor__, __used__));
  #endif
  
  extern int __sysctl(const int *name, u_int namelen, void *oldp,


Please fix properly. Assuming clang always supported it,
something like:

#if __GNUC_PREREQ__(4, 3) || __has_attribute(__constructor__)

should work

Cheers,


I considered something of this sort, but searching for
information on the priority argument in the first place was
annoying enough that I had too much search-fatigue to figure
out when GCC actually corrected this, thus assuming that GCC5
(which seemed to be an all-around good release if memory
serves) and later (since I confirmed GCC6) was sufficient.

I'll fix it in the morning (~8 hours) if I receive no further
objections to address.


Soon enough this can be removed entirely... Getting it
pedantically right in the mean time has little value. We don't
really support gcc5 at the moment. gcc6 and later have good
support, but anything new between 4.3 and 6.0 likely is poorly
tagged...



Well, tracking attributes on GCC versions is not easy but I did
spend a good amount of time getting the attributes right on
cdefs.h and while I lost the battle to get support for older GCC
versions deprecated, getting the attributes properly defined in
the GCC 4.2 vs clang vicinity is particularly important.

Not really. We only support 4.2.1 + freebsd hacks and then 
6.. Further refining stuff is useless.


Some people (Panzura I recall) were actually building FreeBSD with 
external compilers including GCC 4.2.1 without FreeBSD hacks. I suspect 
we could build fine with GCC 4.3 and 5.x, although I admit I wouldn't 
see much sense in it.


Refining 4.3 vs 6.0 buys us nothing and distracts our limited 
resources getting correct something we are definitely removing from 
the tree in a couple of months. Going back and refining it gives us no 
practical benefit. While I don't object to the change, per se, I don't 
view it as required given our future plans.


It is not terribly difficult: it is just a matter of getting one number 
right.

We should scrub cdefs.h. We've needed to for a while...


cdefs.h is handy to sort things out in the inprobable case a new 
compiler arrives to the scene. I fully agre

Re: svn commit: r354672 - head/lib/libc/secure

[Pedro Giffuni]

Hi;

On 12/11/2019 23:44, Warner Losh wrote:



On Tue, Nov 12, 2019 at 9:20 PM Kyle Evans <mailto:kev...@freebsd.org>> wrote:




On Tue, Nov 12, 2019, 22:04 Pedro Giffuni mailto:p...@freebsd.org>> wrote:


On 12/11/2019 22:00, Kyle Evans wrote:

Author: kevans
Date: Wed Nov 13 03:00:32 2019
New Revision: 354672
URL:https://svnweb.freebsd.org/changeset/base/354672

Log:
   ssp: rework the logic to use priority=200 on clang builds
   
   The preproc logic was added at the last minute to appease GCC 4.2, and

   kevans@ did clearly not go back and double-check that the logic 
worked out
   for clang builds to use the new variant.
   
   It turns out that clang defines __GNUC__ == 4. Flip it around and check

   __clang__ as well, leaving a note to remove it later.
   

clang reports itself as GCC 4.2, the priority argument was
introduced in GCC 4.3.

   Reported by: cem

Modified:
   head/lib/libc/secure/stack_protector.c

Modified: head/lib/libc/secure/stack_protector.c

==
--- head/lib/libc/secure/stack_protector.c  Wed Nov 13 02:22:00 
2019(r354671)
+++ head/lib/libc/secure/stack_protector.c  Wed Nov 13 03:00:32 
2019(r354672)
@@ -47,13 +47,15 @@ __FBSDID("$FreeBSD$");
   * they're either not usually statically linked or they simply don't 
do things
   * in constructors that would be adversely affected by their 
positioning with
   * respect to this initialization.
+ *
+ * This conditional should be removed when GCC 4.2 is removed.
   */
-#if defined(__GNUC__) && __GNUC__ <= 4
-#define_GUARD_SETUP_CTOR_ATTR  \
-__attribute__((__constructor__, __used__));
-#else
+#if defined(__clang__) || (defined(__GNUC__) && __GNUC__ > 4)
  #define   _GUARD_SETUP_CTOR_ATTR   \
  __attribute__((__constructor__ (200), __used__));
+#else
+#define_GUARD_SETUP_CTOR_ATTR  \
+__attribute__((__constructor__, __used__));
  #endif
  
  extern int __sysctl(const int *name, u_int namelen, void *oldp,


Please fix properly. Assuming clang always supported it,
something like:

#if __GNUC_PREREQ__(4, 3) || __has_attribute(__constructor__)

should work

Cheers,


I considered something of this sort, but searching for information
on the priority argument in the first place was annoying enough
that I had too much search-fatigue to figure out when GCC actually
corrected this, thus assuming that GCC5 (which seemed to be an
all-around good release if memory serves) and later (since I
confirmed GCC6) was sufficient.

I'll fix it in the morning (~8 hours) if I receive no further
objections to address.


Soon enough this can be removed entirely... Getting it pedantically 
right in the mean time has little value. We don't really support gcc5 
at the moment. gcc6 and later have good support, but anything new 
between 4.3 and 6.0 likely is poorly tagged...




Well, tracking attributes on GCC versions is not easy but I did spend a 
good amount of time getting the attributes right on cdefs.h and while I 
lost the battle to get support for older GCC versions deprecated, 
getting the attributes properly defined in the GCC 4.2 vs clang vicinity 
is particularly important.


I particularly dislike the idea of leaving notes of stuff that can be 
removed when an existing compiler is gone. In this case, we can fix this 
without adding more lines of code, and that also helps in case the code 
is MFCd.


Now ... if you want to be pedantic: this code doesn't handle the case 
for non-GCC based compilers, and it probably could be done more generic 
and clean in cdefs.h where it can be reused. But I am not asking for 
that ;).


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r354672 - head/lib/libc/secure

[Pedro Giffuni]

On 12/11/2019 22:00, Kyle Evans wrote:

Author: kevans
Date: Wed Nov 13 03:00:32 2019
New Revision: 354672
URL: https://svnweb.freebsd.org/changeset/base/354672

Log:
   ssp: rework the logic to use priority=200 on clang builds
   
   The preproc logic was added at the last minute to appease GCC 4.2, and

   kevans@ did clearly not go back and double-check that the logic worked out
   for clang builds to use the new variant.
   
   It turns out that clang defines __GNUC__ == 4. Flip it around and check

   __clang__ as well, leaving a note to remove it later.
   
clang reports itself as GCC 4.2, the priority argument was introduced in 
GCC 4.3.

   Reported by: cem

Modified:
   head/lib/libc/secure/stack_protector.c

Modified: head/lib/libc/secure/stack_protector.c
==
--- head/lib/libc/secure/stack_protector.c  Wed Nov 13 02:22:00 2019
(r354671)
+++ head/lib/libc/secure/stack_protector.c  Wed Nov 13 03:00:32 2019
(r354672)
@@ -47,13 +47,15 @@ __FBSDID("$FreeBSD$");
   * they're either not usually statically linked or they simply don't do things
   * in constructors that would be adversely affected by their positioning with
   * respect to this initialization.
+ *
+ * This conditional should be removed when GCC 4.2 is removed.
   */
-#if defined(__GNUC__) && __GNUC__ <= 4
-#define_GUARD_SETUP_CTOR_ATTR  \
-__attribute__((__constructor__, __used__));
-#else
+#if defined(__clang__) || (defined(__GNUC__) && __GNUC__ > 4)
  #define   _GUARD_SETUP_CTOR_ATTR   \
  __attribute__((__constructor__ (200), __used__));
+#else
+#define_GUARD_SETUP_CTOR_ATTR  \
+__attribute__((__constructor__, __used__));
  #endif
  
  extern int __sysctl(const int *name, u_int namelen, void *oldp,


Please fix properly. Assuming clang always supported it, something like:

#if __GNUC_PREREQ__(4, 3) || __has_attribute(__constructor__)

should work

Cheers,

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r346310 - head/share/misc

[Pedro Giffuni]

On 2019-04-17 09:12, Pedro F. Giffuni wrote:

Author: pfg
Date: Wed Apr 17 14:12:11 2019
New Revision: 346310
URL: https://svnweb.freebsd.org/changeset/base/346310

Log:
   Add myself to ports committers.
   
   Approved by: pfg (mentor)


Oops:  I meant thierry (mentor)

yikes!


Pedro.




___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r351659 - in head: contrib/libc++/include contrib/netbsd-tests/lib/libc/ssp gnu/lib/libssp include lib/libc/stdio

[Pedro Giffuni]

On 01/09/2019 22:36, Cy Schubert wrote:

In message 
, Conrad Meyer writes:

Hi Cy,

On Sun, Sep 1, 2019 at 3:23 PM Cy Schubert  wrote:

In message 
c

om>
, Conrad Meyer writes:


Short version: no, we shouldn't [recommend the use of gets_s]. :-)

Longer version:  Annex K functions like gets_s have zero real adoption
(Microsoft's APIs that inspired Annex K are not actually compatible
with the version in the standards); broadly terrible APIs; and in this
particular case and others, unnecessarily duplicate the functionality
of existing long-standing standard C functions (e.g., fgets(3)).

That's not quite true. From the man page:

  The gets_s() function is equivalent to fgets() with a stream of stdin,
  except that the newline character (if any) is not stored in the string

.

I tried to make a distinction earlier that I don't think carried well
over email.  I wrote "unnecessarily duplicate(s) the _functionality_
of existing …" — not "is/are an exact duplicate(s) of …" — because
you're right, gets_s() has (trivial) behavioral differences from
fgets(stdin).

The thing that is important to me is that fgets(3) is portable, super
well understood, and provides a superset of the functionality of
gets_s().  One can easily construct the newline-free version of a line
from one containing a trailing newline.  I don't think this slight
behavioral difference justifies implementing, using, or especially
recommending gets_s().

If Microsoft chooses to ignore or anotherfunctions is their problem.
However in this case, according to the following they do support gets_s().
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/gets-s-getw
s-s?view=vs-2019

Having said all that, glibc is the odd man out here. In that case I'll pull
back my horns. It's sufficient not to not say anything or to highlight both.

BTW. we've had gets_s(3) in our tree for 17 months now. We don't need to
add anything. It's already there.

It is an application developer choice to use one function or another.

As someone who also works on the ports side, the newline is significant
distinction. As gets_s() is closer in function to gets() than fgets() is,
all one
needs to concern oneself with is buffer length. As there are no _other_
differences nothing else needs to be addressed. This is important to ports
maintainers and who must replace gets() with something else. Agreed this
shouldn't be an issue every time but gets_s() is still in our toolbox.


(IMO, it was probably a historical mistake that gets(3) even had
different behavior than fgets(3) to begin with.  gets(3) maybe
predated stdio FILE streams?)

I totally agree.


Some apps may be sensitive to this subtle difference. gets_s() preserves
this behaviour.

Correct conversion of gets()-using programs requires more analysis
than blind replacement with either function.

That's where gets_s() is handy. It requires less analysis. Remember, my
main concern here are our ports maintainers. Upstream developers should
always do analysis. It's not the job of the ports team to perform
significant rewrites of upstream software. IMO, if upsteam software needs
significant rewrite a port maintainer should notify the upstream
maintainer. If the upstream cannot or will not, requiring a maintainer of a
port to make significant changes, DEPRECATED= and EXPIRATION_DATE= are the
best answer. We are not here to rewrite other people's software for them.


Anyway, gets() use is largely behind us so the point is mostly moot —
there are few such programs to convert, and they should be viewed with
an extremely high level of skepticism given they are still using
gets(3) in 2019.

I'm not arguing for keeping gets(3). We already have gets_s(3). Let's use
it where it makes sense. Nor am I saying to use it in exclusion of
fgets(3). It (gets_s()) is in our libc. If it eases the job of maintaining
a port, use it instead.


[Annex K functions] are part of the
standard

They're an optional part of the standard.  Everyone takes the option
of "not."  Literally no one implements Annex K.  It's a bad set of
APIs.

Microsoft and we have chosen to implement some Annex K functions. We
haven't implemented all of them. I don't know if they implemented all _s
functions. Linux glibc has not.

I don't agree that it's a completely bad set of APIs. gets_s() will help
ports maintainers. AFAIK, no ports rely on gets() but that's not to say
some new port might not. Don't forget, my motivation for implementing
gets_s(3) in libc was to ease the pain of deprecating gets() for ports.


and though we support some _s functions it would behoove us to one
day (*) support them all.

If and when the C standard committee adopts Annex K as a required part
of the standard, then I agree we should make every attempt to support
the full standard library.  But in general, I am opposed to the
further adoption of Annex K, and hope the C2x standard committee
finally drops the annex.[1]  (It is weakly defended[2], just to
provide a 

Re: svn commit: r350550 - head/share/mk

[Pedro Giffuni]

On 07/08/2019 15:12, Rodney W. Grimes wrote:

On 07/08/2019 11:00, John Baldwin wrote:

On 8/6/19 9:56 AM, Glen Barber wrote:

On Sat, Aug 03, 2019 at 01:06:18AM +, John Baldwin wrote:

Author: jhb
Date: Sat Aug  3 01:06:17 2019
New Revision: 350550
URL: https://svnweb.freebsd.org/changeset/base/350550

Log:
Flip REPRODUCIBLE_BUILD back to off by default in head.

Having the full uname output can be useful on head even with

unmodified trees or trees that newvers.sh fails to recognize as
modified.

Reviewed by:	emaste

Differential Revision:  https://reviews.freebsd.org/D20895


I would like to request this commit be reverted.  While the original
commit message to enable this knob stated the commit would be reverted
after stable/12 branched, I have seen no public complaints about
enabling REPRODUCIBLE_BUILD by default (and quite honestly, do not see
the benefit of disabling it by default -- why wouldn't we want
reproducibility?).

To me, this feels like a step backwards, with no tangible benefit.
Note, newvers.sh does properly detect a modified tree if it can find
the VCS metadata directory (i.e., .git, .svn) -- I know this because
I personally helped with it.

In my opinion, those that want the non-reproducible metadata included in
output from 'uname -a' should set WITHOUT_REPRODUCIBLE_BUILDS in their
src.conf.  Turning off a sane default for the benefit of what I suspect
is likely a short list of use cases feels like a step in the wrong
direction.

My arguments for flipping this in head (and head only) are that the data
provided in uname -a when this is disabled is useful for development, and
that in head we do tailor settings towards development (e.g. GENERIC in
head vs GENERIC in stable).

The logic to handle modified trees has an inherent assumption that I think
is false, at least for my workflow and I suspect many others.  I do builds
and tests of kernels on separate machines (VMs or bare metal) from where I
use VCS to manage sources so that a kernel crash doesn't toast my source
tree.  The trees are then shared to the build/test machines via NFS.  As
a result, the build/test machines are not always able to detect that the
tree is modified either because a subset of the checkout is exported via
NFS, or the VCS tool isn't installed on the build/test machines because
they are generally barebones systems with only a base installed.  This
does mean that flipping the knob off doesn't provide all of the same info,
but it does provide the path, and the path matters because 'kgdb -n last'
uses it, and because if you use separate directories for separate projects
(e.g. git worktrees), then the path tells you which test kernel you booted.
(It is not uncommon for me to have several test projects in flight on a
single test machine for different branches.)

In the original discussion on arch, we collectively recognized that
developer builds vs release builds were different and needed different
defaults.  The compromise reached at that time was to depend on the VCS
to detect developer builds to choose the policy.  What I have found is that
in practice for at least my workflow that doesn't actually work.  I posit
that the majority of kernels built from head are developer builds, not
releases, and that the default should cater to that.  You could also always
patch release.sh to set WITH_REPRODUCIBLE_BUILD in the environment which I
think would give a more accurate sense of when builds are releases or not.

However, I will yield to whatever the consensus is.

+1 keeping metadata in head.

I am conflicted on this one, and I think there is a reasonable argument
on both sides, but from what I have read here this appears to be mostly
the kernel that is at issue, loss of the meta data from newvers.sh in
the kernel is infact a PITA, even on stable or production release
systems.

I propose a compromise, add 2 knobs:
WITHOUT_REPRODUCIBLE_KERNEL (aka get your metadata in uname)
WITH_REPRODUCIBLE_USERLAND  (aka reproducible userland)

WITH{,OUT}_REPRODUCIBLE_BUILD overrides both, for backwards compat,
and neither should be defined by default.

Too complex IMHO. Either the system is reproducible or it isn't.

Somehow set WITH_REPRODUCIBLE_KERNEL for builds of GENERIC
for releases/snapshots, but do not ship the system with it
set (I can here a growl from Glen on this)  Thus we build
a reproducible kernel and ship it with the system but if
the user builds a kernel it gets meta data to indicate it
is no longer a stock kernel.
FYI, upon finding I could not figure out what kernel I was running
after installing 12.0 release I turnd off REPRODUCIBLE on my kernel
build VM for 12.0.  I do leave it on if I am building userland.

Thoughts?


Among other things, reproducible builds implies that pkg upgrades are 
smaller. I see it makes sense to make releases, and in fact -stable, 
completely reproducible. For -current I am fine with it not being 
reproducible,


All just IMHO.

Pedro.

Re: svn commit: r350550 - head/share/mk

[Pedro Giffuni]

On 07/08/2019 11:00, John Baldwin wrote:

On 8/6/19 9:56 AM, Glen Barber wrote:

On Sat, Aug 03, 2019 at 01:06:18AM +, John Baldwin wrote:

Author: jhb
Date: Sat Aug  3 01:06:17 2019
New Revision: 350550
URL: https://svnweb.freebsd.org/changeset/base/350550

Log:
   Flip REPRODUCIBLE_BUILD back to off by default in head.
   
   Having the full uname output can be useful on head even with

   unmodified trees or trees that newvers.sh fails to recognize as
   modified.
   
   Reviewed by:	emaste

   Differential Revision:   https://reviews.freebsd.org/D20895


I would like to request this commit be reverted.  While the original
commit message to enable this knob stated the commit would be reverted
after stable/12 branched, I have seen no public complaints about
enabling REPRODUCIBLE_BUILD by default (and quite honestly, do not see
the benefit of disabling it by default -- why wouldn't we want
reproducibility?).

To me, this feels like a step backwards, with no tangible benefit.
Note, newvers.sh does properly detect a modified tree if it can find
the VCS metadata directory (i.e., .git, .svn) -- I know this because
I personally helped with it.

In my opinion, those that want the non-reproducible metadata included in
output from 'uname -a' should set WITHOUT_REPRODUCIBLE_BUILDS in their
src.conf.  Turning off a sane default for the benefit of what I suspect
is likely a short list of use cases feels like a step in the wrong
direction.

My arguments for flipping this in head (and head only) are that the data
provided in uname -a when this is disabled is useful for development, and
that in head we do tailor settings towards development (e.g. GENERIC in
head vs GENERIC in stable).

The logic to handle modified trees has an inherent assumption that I think
is false, at least for my workflow and I suspect many others.  I do builds
and tests of kernels on separate machines (VMs or bare metal) from where I
use VCS to manage sources so that a kernel crash doesn't toast my source
tree.  The trees are then shared to the build/test machines via NFS.  As
a result, the build/test machines are not always able to detect that the
tree is modified either because a subset of the checkout is exported via
NFS, or the VCS tool isn't installed on the build/test machines because
they are generally barebones systems with only a base installed.  This
does mean that flipping the knob off doesn't provide all of the same info,
but it does provide the path, and the path matters because 'kgdb -n last'
uses it, and because if you use separate directories for separate projects
(e.g. git worktrees), then the path tells you which test kernel you booted.
(It is not uncommon for me to have several test projects in flight on a
single test machine for different branches.)

In the original discussion on arch, we collectively recognized that
developer builds vs release builds were different and needed different
defaults.  The compromise reached at that time was to depend on the VCS
to detect developer builds to choose the policy.  What I have found is that
in practice for at least my workflow that doesn't actually work.  I posit
that the majority of kernels built from head are developer builds, not
releases, and that the default should cater to that.  You could also always
patch release.sh to set WITH_REPRODUCIBLE_BUILD in the environment which I
think would give a more accurate sense of when builds are releases or not.

However, I will yield to whatever the consensus is.


+1 keeping metadata in head.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r350665 - in head: . etc/mtree sbin/mount_fusefs share/man/man5 sys/fs/fuse sys/sys tests/sys/fs tests/sys/fs/fusefs

[Pedro Giffuni]

Awesome work.

Thanks!

On 06/08/2019 19:38, Alan Somers wrote:

Author: asomers
Date: Wed Aug  7 00:38:26 2019
New Revision: 350665
URL: https://svnweb.freebsd.org/changeset/base/350665

Log:
   fusefs: merge from projects/fuse2
   
   This commit imports the new fusefs driver. It raises the protocol level

   from 7.8 to 7.23, fixes many bugs, adds a test suite for the driver, and
   adds many new features. New features include:
   
   * Optional kernel-side permissions checks (-o default_permissions)

   * Implement VOP_MKNOD, VOP_BMAP, and VOP_ADVLOCK
   * Allow interrupting FUSE operations
   * Support named pipes and unix-domain sockets in fusefs file systems
   * Forward UTIME_NOW during utimensat(2) to the daemon
   * kqueue support for /dev/fuse
   * Allow updating mounts with "mount -u"
   * Allow exporting fusefs file systems over NFS
   * Server-initiated invalidation of the name cache or data cache
   * Respect RLIMIT_FSIZE
   * Try to support servers as old as protocol 7.4
   
   Performance enhancements include:
   
   * Implement FUSE's FOPEN_KEEP_CACHE and FUSE_ASYNC_READ flags

   * Cache file attributes
   * Cache lookup entries, both positive and negative
   * Server-selectable cache modes: writethrough, writeback, or uncached
   * Write clustering
   * Readahead
   * Use counter(9) for statistical reporting
   
   PR:		199934 216391 233783 234581 235773 235774 235775

   PR:  236226 236231 236236 236291 236329 236381 236405
   PR:  236327 236466 236472 236473 236474 236530 236557
   PR:  236560 236844 237052 237181 237588 238565
   Reviewed by: bcr (man pages)
   Reviewed by: cem, ngie, rpokala, glebius, kib, bde, emaste (post-commit
review on project branch)
   MFC after:   3 weeks
   Relnotes:yes
   Sponsored by:The FreeBSD Foundation
   Pull Request:https://reviews.freebsd.org/D21110

Added:
   head/tests/sys/fs/fusefs/
  - copied from r350621, projects/fuse2/tests/sys/fs/fusefs/
Deleted:
   head/sys/fs/fuse/fuse_param.h
Modified:
   head/MAINTAINERS   (contents, props changed)
   head/UPDATING
   head/etc/mtree/BSD.tests.dist
   head/sbin/mount_fusefs/mount_fusefs.8
   head/sbin/mount_fusefs/mount_fusefs.c
   head/share/man/man5/fusefs.5
   head/sys/fs/fuse/fuse.h
   head/sys/fs/fuse/fuse_device.c
   head/sys/fs/fuse/fuse_file.c
   head/sys/fs/fuse/fuse_file.h
   head/sys/fs/fuse/fuse_internal.c
   head/sys/fs/fuse/fuse_internal.h
   head/sys/fs/fuse/fuse_io.c
   head/sys/fs/fuse/fuse_io.h
   head/sys/fs/fuse/fuse_ipc.c
   head/sys/fs/fuse/fuse_ipc.h
   head/sys/fs/fuse/fuse_kernel.h
   head/sys/fs/fuse/fuse_main.c
   head/sys/fs/fuse/fuse_node.c
   head/sys/fs/fuse/fuse_node.h
   head/sys/fs/fuse/fuse_vfsops.c
   head/sys/fs/fuse/fuse_vnops.c
   head/sys/sys/param.h
   head/tests/sys/fs/Makefile
Directory Properties:
   head/   (props changed)

Modified: head/MAINTAINERS
==
--- head/MAINTAINERSTue Aug  6 23:22:25 2019(r350664)
+++ head/MAINTAINERSWed Aug  7 00:38:26 2019(r350665)
@@ -53,6 +53,7 @@ contrib/pjdfstest asomers,ngie,pjd,#test  Pre-commit re
  etc/mail  gshapiroPre-commit review requested.  Keep in sync with 
-STABLE.
  etc/sendmail  gshapiroPre-commit review requested.  Keep in sync with 
-STABLE.
  fetch des Pre-commit review requested, email only.
+fusefs(5)  asomers Pre-commit review requested.
  geli  pjd Pre-commit review requested (both sys/geom/eli/ and 
sbin/geom/class/eli/).
  isci(4)   jimharris   Pre-commit review requested.
  iwm(4)adrian  Pre-commit review requested, send to 
freebsd-wirel...@freebsd.org

Modified: head/UPDATING
==
--- head/UPDATING   Tue Aug  6 23:22:25 2019(r350664)
+++ head/UPDATING   Wed Aug  7 00:38:26 2019(r350665)
@@ -26,6 +26,18 @@ NOTE TO PEOPLE WHO THINK THAT FreeBSD 13.x IS SLOW:
disable the most expensive debugging functionality run
"ln -s 'abort:false,junk:false' /etc/malloc.conf".)
  
+20190727:

+   The vfs.fusefs.sync_unmount and vfs.fusefs.init_backgrounded sysctls
+   and the "-o sync_unmount" and "-o init_backgrounded" mount options have
+   been removed from mount_fusefs(8).  You can safely remove them from
+   your scripts, because they had no effect.
+
+   The vfs.fusefs.fix_broken_io, vfs.fusefs.sync_resize,
+   vfs.fusefs.refresh_size, vfs.fusefs.mmap_enable,
+   vfs.fusefs.reclaim_revoked, and vfs.fusefs.data_cache_invalidate
+   sysctls have been removed.  If you felt the need to set any of them to
+   a non-default value, please tell asom...@freebsd.org why.
+
  20190713:
Default permissions on the /var/account/acct file (and copies of it
rotated by periodic daily scripts) are changed from 0644 to 0640

Re: svn commit: r349802 - head/sys/fs/ext2fs

[Pedro Giffuni]

Hi;

On 2019-07-24 08:21, Ed Maste wrote:

On Sun, 7 Jul 2019 at 04:58, Fedor Uporov  wrote:

Author: fsu
Date: Sun Jul  7 08:58:02 2019
New Revision: 349802
URL: https://svnweb.freebsd.org/changeset/base/349802

Log:
   Add additional check for 'blocks per group' and 'fragments per group' 
superblock fields.

Will you MFC this to stable/12 and stable/11?



After light testing, I committed it to stable/12. stable/11 has diverged 
a lot (the patch doesn't apply cleanly) and since the patch is not 
critical, it don't think it is worth merging.


Cheers,

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r348512 - head/contrib/one-true-awk

[Pedro Giffuni]

On 02/06/2019 11:28, Warner Losh wrote:

Author: imp
Date: Sun Jun  2 16:28:20 2019
New Revision: 348512
URL: https://svnweb.freebsd.org/changeset/base/348512

Log:
   Reapply r301289 by pfg:
   
   |MFV r300961: one-true-awk: replace 0 with NULL for pointers

   |Also remove a redundant semicolon.
   |Also had to rebase on upstream pull.


Thanks so much for looking at those. They were mostly cosmetic but I had 
received an email confirmation from bwk so I falsely assumed they had 
been merged upstream.


Pedro.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r347477 - head/sys/kern

[Pedro Giffuni]

Hi;

On 10/05/2019 23:57, Doug Moore wrote:

With mentor approval, I commit r347469.  I start getting email about
jenkins failure to build for several architectures on account of the
_Generic() construct I introduced in that change.

I whip up a patch to undo that part of r347469, and ask for mentor
approval.  Meanwhile, mentor authorizes me in email to revert r347469.

I try apply applying the fix-patch, and get email that it was rejected
for lack of reviewer.  In retrospect, it seems to have been committed
anyway as r347472.

Thinking that things are still broken, I do what my mentor pre-approved
earlier and revert back to before r347469.  A patch to redo r347469,
without _Generic(), awaits mentor approval.


Ugh...  a rather elegant interaction ;)


I realize that breaking the build and then committing without mentor
approval in my first week as committer isn't a good beginning.   Sorry
about that.


It's probably not official policy but I would think you don't need 
mentor approval to revert a change, assuming things return to the 
pre-commit state, especially if it broke the build.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r346310 - head/share/misc

[Pedro Giffuni]

On 2019-04-17 09:12, Pedro F. Giffuni wrote:

Author: pfg
Date: Wed Apr 17 14:12:11 2019
New Revision: 346310
URL: https://svnweb.freebsd.org/changeset/base/346310

Log:
   Add myself to ports committers.
   
   Approved by: pfg (mentor)


Oops:  I meant thierry (mentor)

yikes!


Pedro.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r345350 - in head: . lib/libjail sbin/mount_fusefs sys/conf sys/fs/fuse sys/modules sys/modules/fuse sys/modules/fusefs

[Pedro Giffuni]

On 20/03/2019 22:13, Rodney W. Grimes wrote:

On Wed, Mar 20, 2019 at 4:01 PM Rodney W. Grimes
 wrote:

Author: asomers
Date: Wed Mar 20 21:48:43 2019
New Revision: 345350
URL: https://svnweb.freebsd.org/changeset/base/345350

Log:
   Rename fuse(4) to fusefs(4)

   This makes it more consistent with other filesystems, which all end in "fs",
   and more consistent with its mount helper, which is already named
   "mount_fusefs".

   Reviewed by:cem, rgrimes

I did not review this code, I made a single comment that
it should be discussed on an applicable mail list (arch@)
which you did do, and I thank you for that.

I would of eventually objected to the "do not rename the source",
as that is one of the sighted reasons we use svn, is it is near
costless to do moves, and this just trades one missmatch for
another, which in my book is a near nop.

Reviews are still not being allowed enough world rotates
before committing.  I am presently abroad, with poor net,
and busy.

Sorry, I didn't realize you weren't done.

What is the current acceptable "wait" time when asking a public
list for a review/response to some operation?


But the other great thing
about SVN is that we can do stuff, and then do more stuff.  Would you
like for me to rename the source files as well?  I could do that.

I suspect now that the code is committed, and that others have
responded, maybe a proper amount of discussion shall occur and
this decision made by more than 3 people.


The
one thing that I won't agree to do is to rename fuse_kernel.h =>
fusefs_kernel.h, because that file comes verbatim from upstream and we
should keep the original name to make it easy to find(1).

If this code comes from upstream we should try to maintain the
same file names, in as many cases as we can.  Did the commit
made increase or decrease that miss match?  Is there only one
file from upstream?

Does it make any since to set this code up as a vendor branch?


No: fuse_kernel.h is the only file with a real upstream.

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r345350 - in head: . lib/libjail sbin/mount_fusefs sys/conf sys/fs/fuse sys/modules sys/modules/fuse sys/modules/fusefs

[Pedro Giffuni]

On 20/03/2019 16:48, Alan Somers wrote:

Author: asomers
Date: Wed Mar 20 21:48:43 2019
New Revision: 345350
URL: https://svnweb.freebsd.org/changeset/base/345350

Log:
   Rename fuse(4) to fusefs(4)
   
   This makes it more consistent with other filesystems, which all end in "fs",

   and more consistent with its mount helper, which is already named
   "mount_fusefs".
   
   Reviewed by:	cem, rgrimes

   MFC after:   2 weeks
   Sponsored by:The FreeBSD Foundation
   Differential Revision:   https://reviews.freebsd.org/D19649

Added:
   head/sys/modules/fusefs/
  - copied from r345349, head/sys/modules/fuse/
Deleted:
   head/sys/modules/fuse/
Modified:
   head/UPDATING
   head/lib/libjail/jail.c
   head/sbin/mount_fusefs/mount_fusefs.c
   head/sys/conf/NOTES
   head/sys/conf/files
   head/sys/conf/options
   head/sys/fs/fuse/fuse.h
   head/sys/fs/fuse/fuse_file.c
   head/sys/fs/fuse/fuse_ipc.c
   head/sys/fs/fuse/fuse_main.c
   head/sys/fs/fuse/fuse_node.c
   head/sys/fs/fuse/fuse_vfsops.c
   head/sys/fs/fuse/fuse_vnops.c
   head/sys/modules/Makefile
   head/sys/modules/fusefs/Makefile



Hmm..

Not that it matters but you renamed the module, shouldn't you rename the 
directory as well?


We have sys/fs/{deadfs|devfs|ext2fs|fdescfs|fifofs|msdosfs| ... etcfs}.

Cheers,

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r345171 - head/usr.sbin/bhyve

[Pedro Giffuni]

On 15/03/2019 11:03, Rodney W. Grimes wrote:

Em sex, 15 de mar de 2019 ?s 22:12, Ian Lepore  escreveu:


On Thu, 2019-03-14 at 19:31 -0700, Rodney W. Grimes wrote:

Author: chuck
Date: Fri Mar 15 02:11:28 2019
New Revision: 345171
URL: https://svnweb.freebsd.org/changeset/base/345171

Log:
   Fix bhyve PCIe capability emulation

   PCIe devices starting with version 1.1 must set the Role-Based
Error
   Reporting bit.

   And while we're in the neighborhood, generalize the code
assigning the
   device type.

   Reviewed by:  imp, araujo, rgrimes
   Approved by:  imp (mentor)
   MFC after:1 week
   Differential Revision: https://reviews.freebsd.org/D19580

This code requires maintainer approval before a commit,
though this was well reviewed that doesnt exclude it
from the MAINTAINERS entry.


Where exactly does it say that in MAINTAINERS?  As another victim of
this sort of drive-by lynching after making a trivial bhyve change I
pretty seriously object to a vague and meaningless entry in MAINTAINERS
being used to pounce on anyone who dares to touch the precious bhyve
code.


There is a new entry on MAINTAINERS:
https://svnweb.freebsd.org/base?view=revision=344631



There is no mention of bhyve in MAINTAINERS, for usr.sbin or elsewhere.
There is an entry for vmm(4), which to me does not say anything about
bhyve, yet somehow everybody is supposed to know what it means and
what-all territory it covers?

IMO, this sort of hyper-proprietary pouncing on everyone who dares
change a single line of code is not productive.  It is HIGHLY de-
motivating.  Large sweeping design changes are one thing, but pouncing
on every tiny minor commit is just not helpful.


+1

I got so frustrated with it recently that I have decided to don't
contribute with bhyve anymore, perhaps even with FreeBSD.
I still have some people under mentorship that I intend to finish and then
probably I will phase out.

Your failure to get reviews, and infact even abandon one that had
negative advice as to the validity of your suggested change and
committing it anyway is more likely the cause here.


I will have to add  to the choir here: getting reviews is not mandatory 
and I hope they will never be.


Reviewed code is likely to have less errors but sometimes we just need 
to get things done.


Look, for example, the case of the automounting daemon amd(8), which we 
have been planning to remove for some years: your unfortunate 
intervention stopped it from getting removed from the system for how 
many months(?) stopping progress there altogether.


Pedro.





You also committed code with no review at all that had to be reverted
after the bugs it caused were found by an external down stream consumers
of the bhyve code.

You had code reverted by core due to a external attribution request,
which had you been attending the bi monthly bhyve calls you would of
known was an issue.

I would suggest these are the reasons your feeling angry, and that
I infact tried to reach out to jhb to discuss some of these earlier
but that reach out was never returned.  I under stand your frustration,
you are just wanting to do with best thing you can for the project
and bhyve, can we try to find a better resolution to this situation
than your exit?
  

-- Ian


Leave it for now, I am sure jhb or thyco are fine with it,
this is just a heads up FYI for future commits.

Bhyve code has been and still is under a fairly tight
MAINTAINER status.


Modified:
   head/usr.sbin/bhyve/pci_emul.c

Modified: head/usr.sbin/bhyve/pci_emul.c
===
===
--- head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:27 2019(r3
45170)
+++ head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:28 2019(r3
45171)
@@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi,
int type)
 bzero(, sizeof(pciecap));

 pciecap.capid = PCIY_EXPRESS;
-   pciecap.pcie_capabilities = PCIECAP_VERSION |
PCIEM_TYPE_ROOT_PORT;
+   pciecap.pcie_capabilities = PCIECAP_VERSION | type;
+   /* Devices starting with version 1.1 must set the RBER bit */
+   if (PCIECAP_VERSION >= 1)
+   pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
 pciecap.link_capabilities = 0x411;  /* gen1, x1 */
 pciecap.link_status = 0x11; /* gen1, x1 */









--

--
Marcelo Araujo(__)ara...@freebsd.org
\\\'',)http://www.FreeBSD.org    \/  \ ^
Power To Server. .\. /_)

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r344487 - in head/sys: conf gnu/gcov

[Pedro Giffuni]

On 26/02/2019 13:15, Rodney W. Grimes wrote:

On Mon, Feb 25, 2019 at 05:11:26PM -0800, K. Macy wrote:

We had a brief discussion of this commit within a subset of core.  This
addition of GPLv2 code is fine as the code is easily removal to a module
(per kmoore@) should the day come that we're read to evict all GPL code.

I don't execute the ctors until coverage is enabled because I have to
manually find the symbols. The linker doesn't actually generate a ctor
section for functions in text.startup in spite of what Juniper's
linker commit would lead one to believe - presumably they have a
private linker script in addition to a private gcov port.  Thus, it
really could just work fine as a module. Nonetheless, everything to be
profiled needs to be compiled with instrumentation, so separating it
out makes very little sense to me. Although, I suppose ctfconvert +
dtrace module is somewhat analogous.


The modest increase in activation energy for that task seems worth it
for the short-term gains of reduced integration cost (this code will
greatly improve our ZFS-on-Linux test coverage.)

Rod rightly points out that we haven't accepted SPDX tags alone as
license statements.  The standard GPL v2.0 boiler plate should be added
to this file along side the tag.

I've copied the full copyright attribution that is in the
corresponding files on Linux. Is there some reason why FreeBSD
requires the files to be inflated with the full license text where the
original lacks it?

We're not asking for the full text, just the standard block:

// This program is free software; you can redistribute it and/or
// modify it under the terms of the GNU General Public License
// as published by the Free Software Foundation; either version 2
// of the License, or (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with this program; if not, write to the Free Software
// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
// 02110-1301, USA.

This is from: https://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html#SEC4

We're not currently using SPDX to include licenses, largely because OSI
completely and utterly botched BSD licenses.  Fixing this is WAY down
the list of things on core's plate.  Unless someone takes ownership
here, I don't see this changing any time soon.

What is it that got botched, and what is it that needs to be done,
some may think I do not support SPDX, I do, in that originally
it was about tagging the files to mark what license applies, and
I agree we should tag the files.  I howerver can not support the
concept that you can replace a copyright and/or license text in a
file with just a SPDX tag.


I think I have mentioned this before although not in public. I was asked 
to give a talk about my experience tagging SPDX files in the FreeBSD, 
ande did discuss about tags-replacing-licenses informally with some 
lawyers working on the linux kernel. They don't all agree on replacing 
the license with the SPDX tag, however when it is done, they are doing 
it in some context:


1) They keep a complete copy of the license text in the tree.

2) The copyright owners were suggested adopt such simplifications, but 
they were asked to do it themselves.


IANAL, but I agree that bringing such files into FreeBSD must involve 
bringing the license text. This could probably be done once for the gnu/ 
branch.



Some sight Linux as a big project using SPDX, but if you dig into
it you find out they did this to recover from there sloppyness of
actually not having anything in some 6000 files in the kernel,
they simply tagged them all GPL 2 and boom they claim problem
solved.


I don't really follow the linux development to know if they followed any 
procedure for licensing all the files or if they assumed some context in 
the process. They did have a huge mess due to finding variants of the 
GPL with different wording and subtle differences, so I understand their 
interest in homogenizing the licensing somehow.


At least in our case we do have a bunch of build artifacts (Makefiles) 
and some headers without a license. In the case of Makefiles and 
manpages we have made no effort to tag those with SPDX, and it probably 
doesn't matter much.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r343030 - in head/sys: cam conf dev/md dev/nvme fs/fuse fs/nfsclient fs/smbfs kern sys ufs/ffs vm

[Pedro Giffuni]

On 1/15/19 11:07 AM, Andrew Gallatin wrote:

On 1/14/19 8:02 PM, Gleb Smirnoff wrote:


Log:
   Allocate pager bufs from UMA instead of 80-ish mutex protected 
linked list.


<...>


   Together with:    gallatin


Thank you so much for carrying this over the finish line!

Drew



It appears to be very impressive! Plans for MFC?

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r341505 - head/share/man/man5

[Pedro Giffuni]

On 12/5/18 10:24 AM, Pedro Giffuni wrote:


On 12/5/18 12:06 AM, Kubilay Kocak wrote:

On 5/12/2018 9:51 am, Pedro F. Giffuni wrote:

Starting with FreeBSD 12 we fully support writing ext4 filesystems

...


Seems minor but I think worth it for discovery/pola/obviousness, and 
a good time (early in the 13.0 cycle).


We get a lot of user questions about ext*fs support on FreeBSD and 
pointing to an ext2fs man page also feels a bit weird.


This has to be "fixed" through documentation. I will admit that I 
haven't been working properly on the documentation, other than trying 
to remember some details in the Wiki page.



Happy to get/organise a !committer contributor to take care of this 
if no-one wants to pick it up.




I will be glad to review/commit manpage changes that make things 
clearer. We should probably even try to document the format, as I 
recall we do for FAT somewhere(?).



I had a first try at making the manpage more precise here:

https://reviews.freebsd.org/D18445

I realize other places like the handbook still needs more love but we 
have to start somewhere.


Pedro.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r341505 - head/share/man/man5

[Pedro Giffuni]

On 05/12/2018 12:41, Conrad Meyer wrote:

On Wed, Dec 5, 2018 at 7:24 AM Pedro Giffuni  wrote:

On 12/5/18 12:06 AM, Kubilay Kocak wrote:

Can we remove '2' from the module/man/etc name if (since) it supports
multiple extXfs versions? Is there anything serious preventing it?

You can currently create plain ext2 filesystems on FreeBSD and add
ext3/4 features on top and it will work just fine. The distinction on
linux about ext2/3/4 is rather accidental: they didn't master Version
Control in time to branch instead of forking the implementation a couple
of times. It also seems like ext3 disappeared.

The Linux model is that the current incarnation of the ext2/3/4 driver
is named "ext4," and that's what Linux users expect.  You can mount
any ext2/3/4 filesystem with the Linux ext4 driver.  For ext4, it was
a result of wanting to keep ext3 stable while developing ext4 in-tree.
(For a while, it was called "ext4dev").  ext4 is long-since stabilized
and ext3 became fully redundant with ext4, so I guess they dropped it.


From a linux user/marketing perspective you are right.

Are we sure there will never be an ext5fs? I would hate to start moving 
a filesystem in the tree every time a new release comes out.



I think we should just follow that convention and rename ext2fs to
ext4fs.   We can mention support for the less-used ancient ext2/3 in a
COMPATIBILITY section or something, if we don't already, but ext4 has
been the go-to basic Linux filesystem for a decade.  (Seriously:  "On
11 October 2008, the patches that mark ext4 as stable code were merged
in the Linux 2.6.28.")  If we support ext4, call it ext4.


We do support enough of ext4 that we could call it ext4 ...

OTOH, the implementation is pretty much UFS1 plus ext2/3/4 extensions.

I like it as it is because people looking at the code will find out 
exactly where it all comes from: we are currently doing no effort keep 
up to date with what ext4 does and we are focusing on compatibility.  
For now I think adding a link in the documentation as others have 
suggested is enough.



My 2¢,
Conrad


Thanks! All feedback is appreciated.

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r341505 - head/share/man/man5

[Pedro Giffuni]

On 12/5/18 12:06 AM, Kubilay Kocak wrote:

On 5/12/2018 9:51 am, Pedro F. Giffuni wrote:

Starting with FreeBSD 12 we fully support writing ext4 filesystems


Can we remove '2' from the module/man/etc name if (since) it supports 
multiple extXfs versions? Is there anything serious preventing it?


Bad idea: neither us or linux support the old extfs format. It is a 
common misconception that ext3 or ext4 are different filesystems: they 
are both extensions over the ext2 format and they were always intended 
to work like that.


You can currently create plain ext2 filesystems on FreeBSD and add 
ext3/4 features on top and it will work just fine. The distinction on 
linux about ext2/3/4 is rather accidental: they didn't master Version 
Control in time to branch instead of forking the implementation a couple 
of times. It also seems like ext3 disappeared.



Seems minor but I think worth it for discovery/pola/obviousness, and a 
good time (early in the 13.0 cycle).


We get a lot of user questions about ext*fs support on FreeBSD and 
pointing to an ext2fs man page also feels a bit weird.


This has to be "fixed" through documentation. I will admit that I 
haven't been working properly on the documentation, other than trying to 
remember some details in the Wiki page.



Happy to get/organise a !committer contributor to take care of this if 
no-one wants to pick it up.




I will be glad to review/commit manpage changes that make things 
clearer. We should probably even try to document the format, as I recall 
we do for FAT somewhere(?).


Pedro.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r337419 - head/usr.bin/sed

[Pedro Giffuni]

On 8/16/2018 10:30 AM, Alan Somers wrote:
On Tue, Aug 7, 2018 at 8:47 AM, Pedro F. Giffuni > wrote:


Author: pfg
Date: Tue Aug  7 14:47:39 2018
New Revision: 337419
URL: https://svnweb.freebsd.org/changeset/base/337419


Log:
  sed(1): partial fix for the case of the regex delimited with '['.

  We don't generally support the weird case of regular expresions
delimited
  by an opening square bracket ('[') but POSIX says that inside
  bracket expressions, escaping is not possible and both '[' and '\'
  represent themselves.

  PR:           230198 (exp-run)
  Obtained from:        OpenBSD

Modified:
  head/usr.bin/sed/compile.c

Modified: head/usr.bin/sed/compile.c

==
--- head/usr.bin/sed/compile.c  Tue Aug  7 14:39:00 2018       
(r337418)
+++ head/usr.bin/sed/compile.c  Tue Aug  7 14:47:39 2018       
(r337419)
@@ -393,11 +393,11 @@ compile_delimited(char *p, char *d, int is_tr)
                        if ((d = compile_ccl(, d)) == NULL)
                                errx(1, "%lu: %s: unbalanced
brackets ([])", linenum, fname);
                        continue;
+               } else if (*p == '\\' && p[1] == c) {
+                       p++;
                } else if (*p == '\\' && p[1] == '[') {
                        *d++ = *p++;
-               } else if (*p == '\\' && p[1] == c)
-                       p++;
-               else if (*p == '\\' && p[1] == 'n') {
+               } else if (*p == '\\' && p[1] == 'n') {
                        *d++ = '\n';
                        p += 2;
                        continue;


This change seems to have caused a regression in multi_test.sh.
https://ci.freebsd.org/job/FreeBSD-head-amd64-test/8630/testReport/usr.bin.sed/multi_test/main/




Seeding /usr/tests/usr.bin/sed/regress.multitest.out/2.23 with current 
result

sed: 1: "s[\[.[X[
": RE error: brackets ([ ]) not balanced
sed: 1: "s[\[.[X\[[
": RE error: brackets ([ ]) not balanced



Thanks for the report. The change is correct but incomplete, we also 
have to fix the first bug reported here:


http://undeadly.org/cgi?action=article;sid=20180728110010

I honestly don't have time for this so I'll revert the bug for the 
second fix for now.


Pedro.



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r337618 - head/usr.bin/printf

[Pedro Giffuni]

Duh!


On 08/11/18 06:13, Jilles Tjoelker wrote:

Author: jilles
Date: Sat Aug 11 11:13:34 2018
New Revision: 337618
URL: https://svnweb.freebsd.org/changeset/base/337618

Log:
   printf: Fix \c in %b in printf builtin exiting the shell after r337458
   
   SVN r337458 erroneously partially reverted r265885.
   
   This is immediately visible when running the Kyua/ATF tests for

   usr.bin/printf, which actually test sh's printf builtin.
   
   PR:		229641


Modified:
   head/usr.bin/printf/printf.c

Modified: head/usr.bin/printf/printf.c
==
--- head/usr.bin/printf/printf.cSat Aug 11 11:05:22 2018
(r337617)
+++ head/usr.bin/printf/printf.cSat Aug 11 11:13:34 2018
(r337618)
@@ -388,7 +388,7 @@ printf_doformat(char *fmt, int *rval)
  
  		free(p);

if (getout)
-   exit(*rval);
+   return (end_fmt);
break;
}
case 'c': {


Thanks for fixing this!
I recall it's the second time I (inadvertently) commit this bug.
So great to have a corresponding test!

Pedro.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r336351 - head/gnu/lib/libstdc++

[Pedro Giffuni]

On 16/07/2018 19:11, Mark Linimon wrote:

On Mon, Jul 16, 2018 at 06:53:28PM +, Pedro F. Giffuni wrote:

   Update libstdc++ configuration.

Will FreeBSD version be incremented for this?

mcl


I thought about it, but then I was not sure anyone will notice.

Nevermind, version bumps are cheap I will do it.

Pedro.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r336113 - head/usr.bin/gzip

[Pedro Giffuni]

On 07/08/18 17:45, Poul-Henning Kamp wrote:


In message <201807082239.w68mdxwd053...@repo.freebsd.org>, "Pedro F. Giffuni" 
writes:


  New version:
  
  usize = buf[4];

  usize |= (unsigned int)buf[5] << 8;
  usize |= (unsigned int)buf[6] << 16;
  usize |= (unsigned int)buf[7] << 24;

Why not use the functions in endian.h ?


Hmm. .. I am just syncing with NetBSD, where this originated from :-/.
But good point.

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r335454 - head/usr.bin/ar

[Pedro Giffuni]

On 20/06/2018 17:42, Rodney W. Grimes wrote:

Author: emaste
Date: Wed Jun 20 18:43:17 2018
New Revision: 335454
URL: https://svnweb.freebsd.org/changeset/base/335454

Log:
   usr.bin/ar: use standard 2-Clause FreeBSD license
   
   Many licenses on ar files contained small variations from the standard

   FreeBSD license text. To avoid license proliferation switch to the usual
   2-clause FreeBSD license after obtaining permission from all copyright
   holders.
   
   Approved by:	jkoshy, kaiw, kientzle

   Sponsored by:The FreeBSD Foundation
   Differential Revision:   https://reviews.freebsd.org/D14561

Modified:
   head/usr.bin/ar/ar.c
   head/usr.bin/ar/read.c
   head/usr.bin/ar/util.c

Modified: head/usr.bin/ar/ar.c
==
--- head/usr.bin/ar/ar.cWed Jun 20 17:37:55 2018(r335453)
+++ head/usr.bin/ar/ar.cWed Jun 20 18:43:17 2018(r335454)
@@ -1,4 +1,6 @@
  /*-
+ * SPDX-License-Identifier: BSD-3-Clause
+ *

I think there may be an error above, commit message says 2 clause,
license below appears to be 2 clause, yet above we have 3?

Look at all the file: there are two licenses there.

Pedro.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r335278 - head/bin/pwd

[Pedro Giffuni]

On 19/06/2018 11:25, John Baldwin wrote:

On 6/18/18 10:26 PM, Eitan Adler wrote:

On 18 June 2018 at 10:57, John Baldwin  wrote:

On 6/16/18 10:14 PM, Eitan Adler wrote:

Author: eadler
Date: Sun Jun 17 05:14:50 2018
New Revision: 335278
URL: https://svnweb.freebsd.org/changeset/base/335278

Log:
   pwd: mark usage as dead

You keep committing changes like this and ignoring e-mails about them.

I replied both the first time and this time. I may have
(accidentally?) ignored similar emails though. The question I have is
other than the mild code churn what's the harm?

It adds clutter.  Also, fixing the tool means you fix all the places at
once rather than slowly adding workarounds one by one.


What broken compiler are you using that doesn't properly inherit __dead2
from the call to exit()?

In this case, scan-build50 was getting annoyed.

Does scan-build from LLVM 6.0 handle this correctly?  If so, I'd say to
just mark this warning as broken (and thus ignore it) for scan-build50
just as we ignore certain warnings from GCC 4.2.1 because they are
broken-as-implemented.

FWIW, clang's scan-build is made to even more false positives and 
general noise than the regular compiler warnings.

It is better to just ignore it unless it finds something real.

Pedro.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r334940 - head/usr.sbin/bhyve

[Pedro Giffuni]

On 06/10/18 21:41, Marcelo Araujo wrote:



2018-06-11 10:25 GMT+08:00 Pedro Giffuni <mailto:p...@freebsd.org>>:




On 10/06/2018 21:09, Marcelo Araujo wrote:

Author: araujo
Date: Mon Jun 11 02:09:20 2018
New Revision: 334940
URL: https://svnweb.freebsd.org/changeset/base/334940
<https://svnweb.freebsd.org/changeset/base/334940>

Log:
   - Add bhyve virtio-scsi storage backend support.
      Example of configuration:
   ctl.conf:
   portal-group pg0 {
           discovery-auth-group no-authentication
           listen 0.0.0.0
           listen [::]
   }
      target iqn.2012-06.com.example:target0 {
           auth-group no-authentication
           portal-group pg0
           port ioctl/5/3
              lun 0 {
                   path /z/test.img
                   size 8G
           }
           lun 1 {
                   path /z/test1.img
                   size 8G
           }
   }
      bhyve <...> -s 4,virtio-scsi,/dev/cam/ctl5.3,iid=3 
      From inside guest:
   root@:~ # zpool status test
     pool: test
    state: ONLINE
     scan: none requested
   config:
              NAME        STATE     READ WRITE CKSUM
           test        ONLINE       0     0     0
             da0       ONLINE       0     0     0
             da1       ONLINE       0     0     0
      dmesg:
   da0 at vtscsi0 bus 0 scbus0 target 0 lun 0
   da0:  Fixed Direct Access SPC-5 SCSI
device
   da0: Serial Number MYSERIAL
   da0: 300.000MB/s transfers
   da0: Command Queueing enabled
   da0: 8192MB (16777216 512 byte sectors)
   da1 at vtscsi0 bus 0 scbus0 target 0 lun 1
   da1:  Fixed Direct Access SPC-5 SCSI
device
   da1: Serial Number MYSERIAL0001
   da1: 300.000MB/s transfers
   da1: Command Queueing enabled
   da1: 8192MB (16777216 512 byte sectors)
      Discussed with:           grehan
   Reviewed by:         mav
   Obtained from:               TrueOS
   Relnotes:            Yes
   Sponsored by:                iXsystems Inc.
   Tested with:         FreeBSD HEAD, Fedora 28 (Workstation) and
                        Ubuntu 18.04.
   Differential Revision: https://reviews.freebsd.org/D15276
<https://reviews.freebsd.org/D15276>

Added:
   head/usr.sbin/bhyve/iov.c   (contents, props changed)
   head/usr.sbin/bhyve/iov.h   (contents, props changed)
   head/usr.sbin/bhyve/pci_virtio_scsi.c  (contents, props
changed)
Modified:
   head/usr.sbin/bhyve/Makefile
   head/usr.sbin/bhyve/bhyve.8
   head/usr.sbin/bhyve/virtio.h

...


Added: head/usr.sbin/bhyve/pci_virtio_scsi.c

==
--- /dev/null   00:00:00 1970   (empty, because file is newly
added)
+++ head/usr.sbin/bhyve/pci_virtio_scsi.c  Mon Jun 11 02:09:20
2018        (r334940)
@@ -0,0 +1,718 @@
+/*-
+ * SPDX-License-Identifier: BSD-2-Clause-FreeBSD
+ *
+ * Copyright (c) 2016 Jakub Klama .
+ * Copyright (c) 2018 Marcelo Araujo .
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
without
+ * modification, are permitted provided that the following
conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above
copyright
+ *    notice, this list of conditions and the following
disclaimer
+ *    in this position and unchanged.
+ * 2. Redistributions in binary form must reproduce the above
copyright
+ *    notice, this list of conditions and the following
disclaimer in the
+ *    documentation and/or other materials provided with the
distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS
``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR
CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY,
OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
INTERRUPTION)
+ * HOWEV

Re: svn commit: r334940 - head/usr.sbin/bhyve

[Pedro Giffuni]

On 10/06/2018 21:41, Marcelo Araujo wrote:



2018-06-11 10:25 GMT+08:00 Pedro Giffuni <mailto:p...@freebsd.org>>:




On 10/06/2018 21:09, Marcelo Araujo wrote:

Author: araujo
Date: Mon Jun 11 02:09:20 2018
New Revision: 334940
URL: https://svnweb.freebsd.org/changeset/base/334940
<https://svnweb.freebsd.org/changeset/base/334940>

Log:
   - Add bhyve virtio-scsi storage backend support.
      Example of configuration:
   ctl.conf:
   portal-group pg0 {
           discovery-auth-group no-authentication
           listen 0.0.0.0
           listen [::]
   }
      target iqn.2012-06.com.example:target0 {
           auth-group no-authentication
           portal-group pg0
           port ioctl/5/3
              lun 0 {
                   path /z/test.img
                   size 8G
           }
           lun 1 {
                   path /z/test1.img
                   size 8G
           }
   }
      bhyve <...> -s 4,virtio-scsi,/dev/cam/ctl5.3,iid=3 
      From inside guest:
   root@:~ # zpool status test
     pool: test
    state: ONLINE
     scan: none requested
   config:
              NAME        STATE     READ WRITE CKSUM
           test        ONLINE       0     0     0
             da0       ONLINE       0     0     0
             da1       ONLINE       0     0     0
      dmesg:
   da0 at vtscsi0 bus 0 scbus0 target 0 lun 0
   da0:  Fixed Direct Access SPC-5 SCSI
device
   da0: Serial Number MYSERIAL
   da0: 300.000MB/s transfers
   da0: Command Queueing enabled
   da0: 8192MB (16777216 512 byte sectors)
   da1 at vtscsi0 bus 0 scbus0 target 0 lun 1
   da1:  Fixed Direct Access SPC-5 SCSI
device
   da1: Serial Number MYSERIAL0001
   da1: 300.000MB/s transfers
   da1: Command Queueing enabled
   da1: 8192MB (16777216 512 byte sectors)
      Discussed with:           grehan
   Reviewed by:         mav
   Obtained from:               TrueOS
   Relnotes:            Yes
   Sponsored by:                iXsystems Inc.
   Tested with:         FreeBSD HEAD, Fedora 28 (Workstation) and
                        Ubuntu 18.04.
   Differential Revision: https://reviews.freebsd.org/D15276
<https://reviews.freebsd.org/D15276>

Added:
   head/usr.sbin/bhyve/iov.c   (contents, props changed)
   head/usr.sbin/bhyve/iov.h   (contents, props changed)
   head/usr.sbin/bhyve/pci_virtio_scsi.c  (contents, props
changed)
Modified:
   head/usr.sbin/bhyve/Makefile
   head/usr.sbin/bhyve/bhyve.8
   head/usr.sbin/bhyve/virtio.h

...


Added: head/usr.sbin/bhyve/pci_virtio_scsi.c

==
--- /dev/null   00:00:00 1970   (empty, because file is newly
added)
+++ head/usr.sbin/bhyve/pci_virtio_scsi.c  Mon Jun 11 02:09:20
2018        (r334940)
@@ -0,0 +1,718 @@
+/*-
+ * SPDX-License-Identifier: BSD-2-Clause-FreeBSD
+ *
+ * Copyright (c) 2016 Jakub Klama .
+ * Copyright (c) 2018 Marcelo Araujo .
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
without
+ * modification, are permitted provided that the following
conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above
copyright
+ *    notice, this list of conditions and the following
disclaimer
+ *    in this position and unchanged.
+ * 2. Redistributions in binary form must reproduce the above
copyright
+ *    notice, this list of conditions and the following
disclaimer in the
+ *    documentation and/or other materials provided with the
distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS
``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR
CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY,
OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
INTERRUPTION)
+ 

Re: svn commit: r334940 - head/usr.sbin/bhyve

[Pedro Giffuni]

On 10/06/2018 21:09, Marcelo Araujo wrote:

Author: araujo
Date: Mon Jun 11 02:09:20 2018
New Revision: 334940
URL: https://svnweb.freebsd.org/changeset/base/334940

Log:
   - Add bhyve virtio-scsi storage backend support.
   
   Example of configuration:

   ctl.conf:
   portal-group pg0 {
   discovery-auth-group no-authentication
   listen 0.0.0.0
   listen [::]
   }
   
   target iqn.2012-06.com.example:target0 {

   auth-group no-authentication
   portal-group pg0
   port ioctl/5/3
   
   lun 0 {

   path /z/test.img
   size 8G
   }
   lun 1 {
   path /z/test1.img
   size 8G
   }
   }
   
   bhyve <...> -s 4,virtio-scsi,/dev/cam/ctl5.3,iid=3 
   
   From inside guest:

   root@:~ # zpool status test
 pool: test
state: ONLINE
 scan: none requested
   config:
   
   NAMESTATE READ WRITE CKSUM

   testONLINE   0 0 0
 da0   ONLINE   0 0 0
 da1   ONLINE   0 0 0
   
   dmesg:

   da0 at vtscsi0 bus 0 scbus0 target 0 lun 0
   da0:  Fixed Direct Access SPC-5 SCSI device
   da0: Serial Number MYSERIAL
   da0: 300.000MB/s transfers
   da0: Command Queueing enabled
   da0: 8192MB (16777216 512 byte sectors)
   da1 at vtscsi0 bus 0 scbus0 target 0 lun 1
   da1:  Fixed Direct Access SPC-5 SCSI device
   da1: Serial Number MYSERIAL0001
   da1: 300.000MB/s transfers
   da1: Command Queueing enabled
   da1: 8192MB (16777216 512 byte sectors)
   
   Discussed with:		grehan

   Reviewed by: mav
   Obtained from:   TrueOS
   Relnotes:Yes
   Sponsored by:iXsystems Inc.
   Tested with: FreeBSD HEAD, Fedora 28 (Workstation) and
Ubuntu 18.04.
   Differential Revision:  https://reviews.freebsd.org/D15276

Added:
   head/usr.sbin/bhyve/iov.c   (contents, props changed)
   head/usr.sbin/bhyve/iov.h   (contents, props changed)
   head/usr.sbin/bhyve/pci_virtio_scsi.c   (contents, props changed)
Modified:
   head/usr.sbin/bhyve/Makefile
   head/usr.sbin/bhyve/bhyve.8
   head/usr.sbin/bhyve/virtio.h

...



Added: head/usr.sbin/bhyve/pci_virtio_scsi.c
==
--- /dev/null   00:00:00 1970   (empty, because file is newly added)
+++ head/usr.sbin/bhyve/pci_virtio_scsi.c   Mon Jun 11 02:09:20 2018
(r334940)
@@ -0,0 +1,718 @@
+/*-
+ * SPDX-License-Identifier: BSD-2-Clause-FreeBSD
+ *
+ * Copyright (c) 2016 Jakub Klama .
+ * Copyright (c) 2018 Marcelo Araujo .
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer
+ *in this position and unchanged.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include 
+__FBSDID("$FreeBSD$");
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "bhyverun.h"
+#include "pci_emul.h"
+#include "virtio.h"
+#include "iov.h"
+
+#define VTSCSI_RINGSZ  64
+#defineVTSCSI_REQUESTQ 1
+#defineVTSCSI_THR_PER_Q16
+#defineVTSCSI_MAXQ (VTSCSI_REQUESTQ + 2)
+#defineVTSCSI_MAXSEG   64
+
+#defineVTSCSI_IN_HEADER_LEN(_sc)   \
+   (sizeof(struct pci_vtscsi_req_cmd_rd) + _sc->vss_config.cdb_size)
+
+#defineVTSCSI_OUT_HEADER_LEN(_sc)  \
+   (sizeof(struct pci_vtscsi_req_cmd_wr) + _sc->vss_config.sense_size)
+
+#define 

Re: svn commit: r333860 - head/sys/kern

[Pedro Giffuni]

On 05/24/18 11:00, Matthew Macy wrote:

On Thu, May 24, 2018 at 8:58 AM, Warner Losh  wrote:


On Thu, May 24, 2018 at 12:53 AM, Matthew Macy  wrote:

On Wed, May 23, 2018 at 11:42 PM, Michael Tuexen
 wrote:

On 24. May 2018, at 08:36, Matthew Macy  wrote:

On Wed, May 23, 2018 at 11:35 PM, Michael Tuexen
 wrote:

On 24. May 2018, at 06:51, Matthew Macy  wrote:

Warnings find bugs PERIOD. Although most are not useful, I've found

Some warnings indicate bugs, some warnings are just wrong. If you
have a "may be used uninitialized" warning being a false positive, you
may silences the warning by just set it to zero in the declaration and
you silence it. Other compilers might then correctly report an
assignment without affect...

I have yet to see a double assignment be flagged as assignment without
effect. If it _does_ occur then we have to disable the warning on the
compiler that we have less faith in.

Have seen it in the past in a difference project... But you miss my
point:

Not all warnings indicate bugs PERIOD. Some warning are just wrong...

Have you read my follow up? _Many_ Many warnings are wrong. Please
respond to that on what the global policy should be. The value of any
one particular instance of a warning does not merit discussion.


The global policy has never been 'fix all warnings no matter what.' It's
been 'Look at the warning. If it's a false positive, use judgement about
whether or not to stifle the compiler.' There are cases I've run into that
it was impossible to silence the warnings (apart form adding command line
stuff) for a particular bit of code. Do it one way gcc 4.2 complains. Do it
another clang complains. appease both and gcc 6 had heart-burn.

So don't gratuitously commit code that fixes warnings on gcc 8. If the
warning points out a legitimate bug, then that's no brainer yes. If it's a
false positive, then it's less clear and often times many factors may need
to be weighed.

Non-actionable warnings are actively detrimental to workflow. They
hide real issues and lead to apathy by developers. If pacifying a
warning is considered undesirable it should be disabled by default
with perhaps a separate mode for enabling it.


False positives are compiler bugs.

It does happen, with GCC more than with clang, that the compiler has too 
many bugs and it's a bad practice to pessimize code to work around 
them.  At very least you should add a comment when adding unnecessary 
initializations, something like /* workaround GCC */, but dropping 
broken warnings is best.


Pedro.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r333880 - head/sys/kern

[Pedro Giffuni]

On 19/05/2018 21:43, Warner Losh wrote:



On Sat, May 19, 2018, 8:40 PM Pedro Giffuni <p...@freebsd.org 
<mailto:p...@freebsd.org>> wrote:



On 19/05/2018 16:02, Warner Losh wrote:



On Sat, May 19, 2018 at 2:32 PM, Poul-Henning Kamp
<p...@phk.freebsd.dk <mailto:p...@phk.freebsd.dk>> wrote:


In message

Re: svn commit: r333880 - head/sys/kern

[Pedro Giffuni]

On 19/05/2018 16:02, Warner Losh wrote:



On Sat, May 19, 2018 at 2:32 PM, Poul-Henning Kamp > wrote:



In message

Re: svn commit: r333745 - in head/sys/contrib/ck: include src

[Pedro Giffuni]

On 17/05/2018 14:35, Matthew Macy wrote:

On Thu, May 17, 2018 at 12:32 PM, Pedro Giffuni <p...@freebsd.org> wrote:


On 17/05/2018 14:27, Emmanuel Vadot wrote:

On Thu, 17 May 2018 14:20:05 -0500
Pedro Giffuni <p...@freebsd.org> wrote:


On 17/05/2018 14:12, Matthew Macy wrote:

How do I avoid problems while allowing timely updates?

-M

On Thu, May 17, 2018 at 11:38 AM, Emmanuel Vadot <m...@bidouilliste.com>
wrote:

Hi Matt,

On Thu, 17 May 2018 18:14:10 + (UTC)
Matt Macy <mm...@freebsd.org> wrote:


Author: mmacy
Date: Thu May 17 18:14:10 2018
New Revision: 333745
URL: https://svnweb.freebsd.org/changeset/base/333745

Log:
 ck: add support for executing callbacks outside of main poll loop

 Pull in change from upstream
deca119d14bfffd440770eb67cbdbeaf7b57eb7b

 |ck_epoch: introduce ck_epoch_deferred
 |
 |Allow for deferral to occur outside epoch poll critical loop
(which may access per-CPU structures).
 |

 Approved by:sbruno

Modified:
 head/sys/contrib/ck/include/ck_epoch.h
 head/sys/contrib/ck/src/ck_epoch.c


CK was imported in vendor-sys/ck, commiting directly into head will
cause some problems in the future.

Actually ... committing to head is fine: some things just have to be
fixed.

   This is not true, it can cause a lot of problems with futures updates
if the change isn't in the vendor repo.

OK ... I meant generally you can. The catch here is that the change was
already upstream so it should have been done in the vendor area and then
merged. It's best to fix this properly now.

Sorry for misguiding.


I'm happy to comply - just point me at the fine manual.



https://www.freebsd.org/doc/en_US.ISO8859-1/articles/committers-guide/subversion-primer.html
Section 5.4.4 (Vendor patches).

Cheers,

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r333745 - in head/sys/contrib/ck: include src

[Pedro Giffuni]

On 17/05/2018 14:27, Emmanuel Vadot wrote:

On Thu, 17 May 2018 14:20:05 -0500
Pedro Giffuni <p...@freebsd.org> wrote:


On 17/05/2018 14:12, Matthew Macy wrote:

How do I avoid problems while allowing timely updates?

-M

On Thu, May 17, 2018 at 11:38 AM, Emmanuel Vadot <m...@bidouilliste.com> wrote:

   Hi Matt,

On Thu, 17 May 2018 18:14:10 + (UTC)
Matt Macy <mm...@freebsd.org> wrote:


Author: mmacy
Date: Thu May 17 18:14:10 2018
New Revision: 333745
URL: https://svnweb.freebsd.org/changeset/base/333745

Log:
ck: add support for executing callbacks outside of main poll loop

Pull in change from upstream deca119d14bfffd440770eb67cbdbeaf7b57eb7b

|ck_epoch: introduce ck_epoch_deferred
|
|Allow for deferral to occur outside epoch poll critical loop (which 
may access per-CPU structures).
|

Approved by:sbruno

Modified:
head/sys/contrib/ck/include/ck_epoch.h
head/sys/contrib/ck/src/ck_epoch.c


   CK was imported in vendor-sys/ck, commiting directly into head will
cause some problems in the future.

Actually ... committing to head is fine: some things just have to be fixed.

  This is not true, it can cause a lot of problems with futures updates
if the change isn't in the vendor repo.
OK ... I meant generally you can. The catch here is that the change was 
already upstream so it should have been done in the vendor area and then 
merged. It's best to fix this properly now.


Sorry for misguiding.

Pedro.


We do ask you try to upstream the change and re-merge things when possible.

  This is already upstream, Matt is just updating CK with a patch he did
( see
https://github.com/concurrencykit/ck/commit/deca119d14bfffd440770eb67cbdbeaf7b57eb7b)


Pedro.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r333745 - in head/sys/contrib/ck: include src

[Pedro Giffuni]

On 17/05/2018 14:12, Matthew Macy wrote:

How do I avoid problems while allowing timely updates?

-M

On Thu, May 17, 2018 at 11:38 AM, Emmanuel Vadot  wrote:

  Hi Matt,

On Thu, 17 May 2018 18:14:10 + (UTC)
Matt Macy  wrote:


Author: mmacy
Date: Thu May 17 18:14:10 2018
New Revision: 333745
URL: https://svnweb.freebsd.org/changeset/base/333745

Log:
   ck: add support for executing callbacks outside of main poll loop

   Pull in change from upstream deca119d14bfffd440770eb67cbdbeaf7b57eb7b

   |ck_epoch: introduce ck_epoch_deferred
   |
   |Allow for deferral to occur outside epoch poll critical loop (which may 
access per-CPU structures).
   |

   Approved by:sbruno

Modified:
   head/sys/contrib/ck/include/ck_epoch.h
   head/sys/contrib/ck/src/ck_epoch.c


  CK was imported in vendor-sys/ck, commiting directly into head will
cause some problems in the future.


Actually ... committing to head is fine: some things just have to be fixed.
We do ask you try to upstream the change and re-merge things when possible.

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r331510 - in head: share/man/man4 sys/conf sys/dev/vmware/vmci sys/modules/vmware sys/modules/vmware/vmci

[Pedro Giffuni]

On 25/03/2018 11:03, Rodney W. Grimes wrote:


On 25/03/2018 06:49, Rodney W. Grimes wrote:

On Sat, Mar 24, 2018 at 6:27 PM, Rodney W. Grimes <
free...@pdx.rh.cn85.dnsmgr.net> wrote:


Author: mp
Date: Sun Mar 25 00:57:00 2018
New Revision: 331510
URL: https://svnweb.freebsd.org/changeset/base/331510

These files do not each contain a usable copyright, though
they seem to contain SPDX tags that indiate they should contain
a BSD 2 clause copyright.

IANAL but I believe you meant "...they should contain a BSD 2 clause
*license*". The files should contain a valid copyright.

A valid, but unusable.  As the copyright is it is a full copyright
held by vmware without any rights to be published or redistributed
any any manner by anyone but vmware.

"Copyright (c) 2018 VMware, Inc. All Rights Reserved."

That is a restrictive copyright, allowing no one to publish, or
in our case, redistribute, without a further license of some form.


The intent of my commit and the author were to use the implied SPDX version
of the licenses without burdening the source code with the more heavyweight
license text. Having seen SPDX in the src tree, I believed
the SPDX-License-Identifier was sufficient. But, to your point, I'm not
sure I have seen a discussion or a decision on it.

SPDX tags are purely to be treated as "advisory" and in no one imply
or create any license agreement.

As happens in economics, different lawyers can have different
interpretations. Our practices were consulted with the SPDX guys but
other projects have different practices.

While the sound practice, especially when you don't own the code, is to
add the SPDX tag in addition to the license text, the linux developers
are encouraging replacing it altogether with the SPDX tag. In their case
they keep a reference to the complete license text elsewhere and they
have some repository log where the copyright owner did the change.

They have grown use to this from the way the GPL is handled, since
the length of the body of that license would be impractical to
include.


For contrib code we just follow upstream. In no case can anyone other
than the copyright owner clarify, or otherwise change, a license.

That does bring a question of why this code is not either on
a vendor import branch, or in contrib?

Can you point to any files in /usr/src that lack a full and complete
standalone license?  Sans perhaps some GPL code that has a pointer
to COPYING and files that can not such as Makefile and .mk's.

There are some. Here is an outstanding example:
usr.sbin/bhyve/bhyvegc.c

FWIW, the one time I did a change I added a copyright disclaimer to the 
commit log  to avoid future issues:

https://svnweb.freebsd.org/base?view=revision=318788

Cheers,

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r331510 - in head: share/man/man4 sys/conf sys/dev/vmware/vmci sys/modules/vmware sys/modules/vmware/vmci

[Pedro Giffuni]

On 25/03/2018 06:49, Rodney W. Grimes wrote:

On Sat, Mar 24, 2018 at 6:27 PM, Rodney W. Grimes <
free...@pdx.rh.cn85.dnsmgr.net> wrote:


Author: mp
Date: Sun Mar 25 00:57:00 2018
New Revision: 331510
URL: https://svnweb.freebsd.org/changeset/base/331510

These files do not each contain a usable copyright, though
they seem to contain SPDX tags that indiate they should contain
a BSD 2 clause copyright.


IANAL but I believe you meant "...they should contain a BSD 2 clause
*license*". The files should contain a valid copyright.

A valid, but unusable.  As the copyright is it is a full copyright
held by vmware without any rights to be published or redistributed
any any manner by anyone but vmware.

"Copyright (c) 2018 VMware, Inc. All Rights Reserved."

That is a restrictive copyright, allowing no one to publish, or
in our case, redistribute, without a further license of some form.


The intent of my commit and the author were to use the implied SPDX version
of the licenses without burdening the source code with the more heavyweight
license text. Having seen SPDX in the src tree, I believed
the SPDX-License-Identifier was sufficient. But, to your point, I'm not
sure I have seen a discussion or a decision on it.

SPDX tags are purely to be treated as "advisory" and in no one imply
or create any license agreement.


As happens in economics, different lawyers can have different 
interpretations. Our practices were consulted with the SPDX guys but 
other projects have different practices.


While the sound practice, especially when you don't own the code, is to 
add the SPDX tag in addition to the license text, the linux developers 
are encouraging replacing it altogether with the SPDX tag. In their case 
they keep a reference to the complete license text elsewhere and they 
have some repository log where the copyright owner did the change.


For contrib code we just follow upstream. In no case can anyone other 
than the copyright owner clarify, or otherwise change, a license.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r331279 - in head: include lib/libc/gen lib/libc/sys lib/libc/tests/gen sys/compat/freebsd32 sys/conf sys/kern sys/sys tests/sys/kern usr.bin/truss

[Pedro Giffuni]

On 20/03/2018 20:40, Ian Lepore wrote:

On Wed, 2018-03-21 at 01:15 +, Conrad Meyer wrote:

Author: cem
Date: Wed Mar 21 01:15:45 2018
New Revision: 331279
URL: https://svnweb.freebsd.org/changeset/base/331279

Log:
   Implement getrandom(2) and getentropy(3)
   
   The general idea here is to provide userspace programs with well-

defined
   sources of entropy, in a fashion that doesn't require opening a new
file
   descriptor (ulimits) or accessing paths (/dev/urandom may be
restricted
   by chroot or capsicum).
   
   getrandom(2) is the more general API, and comes from the Linux

world.
   Since our urandom and random devices are identical, the GRND_RANDOM
flag
   is ignored.
   
   getentropy(3) is added as a compatibility shim for the OpenBSD API.
   
   truss(1) support is included.
   
   Tests for both system calls are provided.  Coverage is believed to

be at
   least as comprehensive as LTP getrandom(2) test
coverage.  Additionally,
   instructions for running the LTP tests directly against FreeBSD are
provided
   in the "Test Plan" section of the Differential revision linked
below.  (They
   pass, of course.)
   
   PR:		194204

   Reported by: David CARLIER 
   Discussed with:  cperciva, delphij, jhb, markj
   Relnotes:maybe
   Differential Revision:   https://reviews.freebsd.org/D14500


A good followup to this might be to switch libc's arc4random seeding to
getrandom(), instead of using a sysctl in a loop.

That appears to be the main use of getentropy() in OpenBSD.
We should now obviate linux_getrandom() as well.

Cheers,

Pedro.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r330601 - head/sys/i386/ibcs2

[Pedro Giffuni]

On 07/03/2018 16:19, Brooks Davis wrote:

On Wed, Mar 07, 2018 at 01:20:14PM -0500, Pedro Giffuni wrote:

FWIW ...

ibcs2 is candidate for future removal.

It is probably time again to see if actual users exist.  ibcs2 has wasted
a few hours of my time over the last few months so keeping it does have
a non-zero cost.


FWIW, I used it long ago on FreeBSD with Unesco's ISIS database software 
but the software is not developed anymore.



We tried to get some vendor interest in it but we failed and given this
is very i386-specific it is probably not worth spending huge efforts on it.

cloudabi seems to be, for all purposes, a better conceptual replacement.

This comment doesn't make much sense. iBCS is the Intel Binary
Compatibility Standard, an obsolete ABI for i386 Unixes such as Xenix,
SCO, and UnixWare.  Cloudabi is, in a sense, taking Capsicum to its
logical extreme and totally unrelated.


It is also a binary format (ELF-based) that can run on several platforms 
including Linux and FreeBSD.
It clearly targets the cloud market, which makes more sense nowadays 
than the i386 use space which were common in the early 90s.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r330601 - head/sys/i386/ibcs2

[Pedro Giffuni]

FWIW ...

ibcs2 is candidate for future removal.

We tried to get some vendor interest in it but we failed and given this 
is very i386-specific it is probably not worth spending huge efforts on it.


cloudabi seems to be, for all purposes, a better conceptual replacement.

Pedro.

On 07/03/2018 09:44, Eitan Adler wrote:

Author: eadler
Date: Wed Mar  7 14:44:32 2018
New Revision: 330601
URL: https://svnweb.freebsd.org/changeset/base/330601

Log:
   sys: Fix a few potential infoleaks in cloudabi
   
   While there is no immediate leak, if the structure changes underneath

   us, there might be in the future.
   
   Submitted by:	Domagoj Stolfa 

   MFC After:   1 month
   Sponsored by:DARPA/AFRL

Modified:
   head/sys/i386/ibcs2/ibcs2_ipc.c

Modified: head/sys/i386/ibcs2/ibcs2_ipc.c
==
--- head/sys/i386/ibcs2/ibcs2_ipc.c Wed Mar  7 14:41:29 2018
(r330600)
+++ head/sys/i386/ibcs2/ibcs2_ipc.c Wed Mar  7 14:44:32 2018
(r330601)
@@ -135,6 +135,8 @@ ibcs2_msgctl(struct thread *td, void *v)
struct msqid_ds bs;
int error;
  
+	memset(, 0, sizeof(is));

+
switch (uap->cmd) {
case IBCS2_IPC_STAT:
error = kern_msgctl(td, uap->msqid, IPC_STAT, );
@@ -317,6 +319,8 @@ ibcs2_semctl(struct thread *td, void *v)
union semun semun;
register_t rval;
int error;
+
+   memset(, 0, sizeof(is));
  
  	switch(uap->cmd) {

case IBCS2_IPC_STAT:



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r330285 - head/sys/sys

[Pedro Giffuni]

On 03/03/2018 05:21, Konstantin Belousov wrote:

On Sat, Mar 03, 2018 at 01:47:41PM +1100, Bruce Evans wrote:

On Fri, 2 Mar 2018, Konstantin Belousov wrote:


On Fri, Mar 02, 2018 at 12:43:34PM -0500, Pedro Giffuni wrote:

...
I think use of _Nonnull attributes in the threading functions may also
be a waste (I introduced them mostly to be compatible with Android).
FWIW, Dragonfly sprinkled some restrict there recently:

http://gitweb.dragonflybsd.org/dragonfly.git/commit/d33005aaee6af52c80428b59b52aee522c002492

Just in case someone is considering more cleanups.

This is not a cleanup for me, but a needed change. Right now x86
copyouts are implemented in asm, so whatever damage is done to the
prototypes, only effect is at the caller side. In my work, i386 copyouts
are done in C, so it starts matter.

That seems slow, especially for small sizes as are common for syscall args
(in 1 of my versions, copyin() of args is optimized to fuword() in a loop,
and fuword() is optimized to not use pcb_onfault, so it is not much more
than 1 memory access.  However, in your i386 version this optimization
would be negative since the slow part is switching the map, so fuword()
should never be used to access multiple words).

Yes. I already explained it in private, the current choice for i386 is
either to be neglected very fast, or to get this change to still be a
Tier 1 32 bit platform. The change is to make 4/4g split for UVA/KVA.
In particular, the change ensures that it is possible to self-host i386
for forthcoming years, which is not practical for armv7 now and would be
less so with clang grow.

In other news, my system already boots single-user on SMP machine and
I have torture tests like setting invalid %ss segment by sigreturn(2),
work.  There is (much) more to come, but I am happy how the patch
progressed so far.

Very nice.


However, copyinstr() and
copystr() should never have been "optimized" by writing them in asm.  On
x86, their asm is badly written so they are slower than simple C versions
except on 8088's and maybe 8086's and maybe on the original i386.  (8088's
were limited mainly by instruction bandwidth and the original i386 wasn't
much better, so short CISC instructions like lodsb and stosb tended to be
faster than larger separate instructions despite their large setup overheads.

Sure, copyinstr() is rewritten in C.  The current version of copyout stuff
is there:
https://kib.kiev.ua/git/gitweb.cgi?p=deviant2.git;a=blob;f=sys/i386/i386/copyout.c;h=9747c06a84d7d2b5faac946f5de57f6a34d96c8c;hb=refs/heads/pcid


Also I looked at the dragonfly commit because I become curious what do you
mean by threading functions.  The first example was
intpthread_attr_getguardsize(const pthread_attr_t * __restrict,
-   size_t *);
+   size_t * __restrict);
POSIX agrees with the dragonfly change, but I do not understand it.
Aliasing rules already disallow the first and second arguments to point
to the same memory, because they have different types.

(1) thread_attr_t is opaque, so the types might be the same.
(2) pthread_attr_t might be a pointer to a struct/union containing a size_t.
(3) perhaps other reasons.  I'm not sure how 'restrict interacts with global
  variables or even it it prevents the interaction in (2).  A previous
  discussion showed that const doesn't make types different enough to
  prevent aliasing.  Similarly for volatile.

Similarly for other pointers to {opaque, struct/union, or even integer} types.
size_t can't be aliased to int, but it can be aliased to any unsigned type
in C and to any unsigned type not smaller than uint16_t in POSIX (POSIX
but not C requires u_char == uint8_t, so size_t can't be u_char in POSIX
but it can be u_char in C).

I can only summarize it as 'there is no use to have restrict on the
pthread_attr_getguardsize() arguments'.



Well, I'll admit I don't understand well the advantages and that's why I 
brought up a pointer to the changes instead of working on them. Usually, 
standards compliance is reason enough for such change though.


Pedro.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r330285 - head/sys/sys

[Pedro Giffuni]

(cc in Eitan as he may be interested in the extra restrict cases)


On 02/03/2018 11:47, Konstantin Belousov wrote:

Author: kib
Date: Fri Mar  2 16:47:02 2018
New Revision: 330285
URL: https://svnweb.freebsd.org/changeset/base/330285

Log:
   Remove _Nonnull attributes from user addresses arguments for
   copyout(9) family.
   
   The addresses are user-controllable, and if the process ABI allows

   mapping at zero, then the zero address is meaningful, contradicting
   the definition of _Nonnull.  In any case, it does not require any
   special code to handle NULL udaddr.
   


FWIW, the _Nonnull attributes didn't do much at all beyond producing a 
warning.

They replaced the GNU __nonnull() attributes which were much more dangerous.
I am OK with seeing both gone here though.


   It is not clear if __restrict makes sense as well, since kaddr and
   udaddr point to different address spaces, so equal numeric values of
   the pointers do not imply aliasing and a legitimate.  But leave it for
   later.
   
   copyinstr(9) does not have its user address argument annotated.


I think use of _Nonnull attributes in the threading functions may also 
be a waste (I introduced them mostly to be compatible with Android).

FWIW, Dragonfly sprinkled some restrict there recently:

http://gitweb.dragonflybsd.org/dragonfly.git/commit/d33005aaee6af52c80428b59b52aee522c002492

Just in case someone is considering more cleanups.

Cheers,

Pedro.


   Sponsored by:The FreeBSD Foundation
   MFC after:   1 week

Modified:
   head/sys/sys/systm.h

Modified: head/sys/sys/systm.h
==
--- head/sys/sys/systm.hFri Mar  2 16:31:23 2018(r330284)
+++ head/sys/sys/systm.hFri Mar  2 16:47:02 2018(r330285)
@@ -277,14 +277,14 @@ int   copystr(const void * _Nonnull __restrict kfaddr,
  int   copyinstr(const void * __restrict udaddr,
void * _Nonnull __restrict kaddr, size_t len,
size_t * __restrict lencopied);
-intcopyin(const void * _Nonnull __restrict udaddr,
+intcopyin(const void * __restrict udaddr,
void * _Nonnull __restrict kaddr, size_t len);
-intcopyin_nofault(const void * _Nonnull __restrict udaddr,
+intcopyin_nofault(const void * __restrict udaddr,
void * _Nonnull __restrict kaddr, size_t len);
  int   copyout(const void * _Nonnull __restrict kaddr,
-   void * _Nonnull __restrict udaddr, size_t len);
+   void * __restrict udaddr, size_t len);
  int   copyout_nofault(const void * _Nonnull __restrict kaddr,
-   void * _Nonnull __restrict udaddr, size_t len);
+   void * __restrict udaddr, size_t len);
  
  int	fubyte(volatile const void *base);

  long  fuword(volatile const void *base);



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r326257 - in head/sys/amd64: acpica amd64 ia32 include include/pc linux32 pci vmm vmm/amd vmm/intel vmm/io

[Pedro Giffuni]

On 01/03/2018 22:26, Ed Maste wrote:

On 27 November 2017 at 10:03, Pedro F. Giffuni  wrote:

Author: pfg
Date: Mon Nov 27 15:03:07 2017
New Revision: 326257
URL: https://svnweb.freebsd.org/changeset/base/326257

Log:
   sys/amd64: further adoption of SPDX licensing ID tags.

...

Modified: head/sys/amd64/linux32/linux32_machdep.c
==
--- head/sys/amd64/linux32/linux32_machdep.cMon Nov 27 15:01:59 2017
(r326256)
+++ head/sys/amd64/linux32/linux32_machdep.cMon Nov 27 15:03:07 2017
(r326257)
@@ -1,4 +1,6 @@
  /*-
+ * SPDX-License-Identifier: BSD-3-Clause
+ *

A few Linuxulator files gained a BSD-3-Clause tag, but they're not
actually 3-Clause BSD; they have a small addition to the first clause,
which ends with "in this position and unchanged." I've been going
through the Linuxulator source files to rationalize the license text
(see review D14210) but am waiting on confirmation from a few
copyright holders before all files can be cleaned up.

Until that happens I think we should just remove the SPDX tags from these files.
Yes, that sounds right. Feel free to drop the SPDX tags from anything 
that looks suspicious.


Pedro.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r329077 - head/usr.bin/tftp

[Pedro Giffuni]

On 02/10/18 11:13, Justin Hibbits wrote:

On Fri, Feb 9, 2018 at 7:22 PM, Ed Maste  wrote:

On 9 February 2018 at 14:46, Conrad Meyer  wrote:

Author: cem
Date: Fri Feb  9 19:46:51 2018
New Revision: 329077
URL: https://svnweb.freebsd.org/changeset/base/329077

Log:
   tftp(1): Fix libedit state corruption involving signals

 From https://ci.freebsd.org/job/FreeBSD-head-mips-build/391/console:

01:15:24 --- all_subdir_usr.bin/tftp ---
01:15:24 --- main.o ---
01:15:24 cc1: warnings being treated as errors
01:15:24 /usr/src/usr.bin/tftp/main.c: In function 'main':
01:15:24 /usr/src/usr.bin/tftp/main.c:182: warning: 'hist' may be used
uninitialized in this function
01:15:24 /usr/src/usr.bin/tftp/main.c:181: warning: 'el' may be used
uninitialized in this function


After initializing these, I see the following interesting error:

/home/chmeee/freebsd/head/usr.bin/tftp/main.c:181: warning: variable
'el' might be clobbered by 'longjmp' or 'vfork'
/home/chmeee/freebsd/head/usr.bin/tftp/main.c:182: warning: variable
'hist' might be clobbered by 'longjmp' or 'vfork'

This appears to be a known bug in gcc, at
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=24239 so I don't know
what the best way to fix it is.


Independent of the compiler bug, it does seem like 'el' and 'hist' can 
be used uninitialized for the non-interactive case.


Untested, but perhaps an only-when-needed initialization like this may 
workaround the compiler bug.


Cheers,

Pedro.
Index: usr.bin/tftp/main.c
===
--- usr.bin/tftp/main.c	(revision 329104)
+++ usr.bin/tftp/main.c	(working copy)
@@ -197,6 +197,9 @@
 		el_set(el, EL_PROMPT, command_prompt);
 		el_set(el, EL_SIGNAL, 1);
 		el_source(el, NULL);
+	} else {	/* Quell GCC */
+	el = NULL;
+	hist = NULL;
 	}
 
 	if (argc > 1) {
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328492 - head/contrib/opie/libopie

[Pedro Giffuni]

On 01/27/18 20:42, Bruce Evans wrote:

On Sat, 27 Jan 2018, Pedro Giffuni wrote:


On 01/27/18 18:21, Bruce Evans wrote:

On Sat, 27 Jan 2018, Dimitry Andric wrote:


On 27 Jan 2018, at 23:20, Ed Schouten <e...@nuxi.nl> wrote:



* [... context lost to corruption of spaces which makes it unreadable]




Wait... This may access utmp.ut_host one byte past the end and no
longer guarantees that host is null-terminated, right?


No, strncpy "copies at most len characters from src into dst".  
However,


No, the change breaks the length so 1 byte past the end is accessed
in implementations where ut_host is not guaranteed to be NUL terminated
and the current instance of ut_host is not NUL terminated.

The main change is in the sizeof(). Regularly you should use the size 
of destination not the source, and apparently GCC8 decided there was 
something to check there.


That is the main breakage.  Using the size of the destination is very 
wrong,

since that size is intentionally 1 larger than the size of the source, to
leave space for appending a NUL.

I am considering reverting the change. Looking at other ways to get rid 
of the warning, please be patient.



...
Looking in detail, upstream (which appears to have disappeared) does 
have the explicit NULL termination in our last import. For 
consistency and given that we already have a strlcpy in that code, we 
should use strlcpy() there. Every modern OS out there has strlcpy(3) 
and if not they can figure out what to do.


strlcpy() still seems to be intentionally left out of glibc.



glibc is not portable. I understand some systems that carry glibc also 
carry libbsd, or they can still use musl:


https://github.com/esmil/musl/blob/master/src/string/strlcpy.c


Pedro.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328492 - head/contrib/opie/libopie

[Pedro Giffuni]

On 01/27/18 18:21, Bruce Evans wrote:

On Sat, 27 Jan 2018, Dimitry Andric wrote:


On 27 Jan 2018, at 23:20, Ed Schouten  wrote:


2018-01-27 23:16 GMT+01:00 Pedro F. Giffuni :

   char host[sizeof(utmp.ut_host) + 1];
   insecure = 1;

-   strncpy(host, utmp.ut_host, sizeof(utmp.ut_host));
-   host[sizeof(utmp.ut_host)] = 0;
+   strncpy(host, utmp.ut_host, sizeof(host));


Wait... This may access utmp.ut_host one byte past the end and no
longer guarantees that host is null-terminated, right?



No, strncpy "copies at most len characters from src into dst".  However,


No, the change breaks the length so 1 byte past the end is accessed
in implementations where ut_host is not guaranteed to be NUL terminated
and the current instance of ut_host is not NUL terminated.

The main change is in the sizeof(). Regularly you should use the size of 
destination not the source, and apparently GCC8 decided there was 
something to check there.



if the length of the source is equal to or greater than len, the
destination is *not* null terminated.  This is likely why the
"host[sizeof(utmp.ut_host)] = 0;" statement was added.


This is why that statement was there.

This change is not even wrong under FreeBSD, since ut_host and several 
other

fields are guaranteed to be NUL terminated in the FreeBSD implementation.
The code was correct and portable and the change just breaks its 
portability.




The change was done for portability to GCC, or at least to fix a warning 
there.



In any case, this is why strlcpy exists. :)


Using strlcpy() in libopie would be another good unportabilization.
contrib/opie never uses strlc*() except in 1 place previously
unportabilized in r208586.  That at least fixed 2 bugs (2 related off
by 1 errors in the code intended to avoid buffer overruns, with the
result that buffer overruns were limited to 1 byte).  It moved the
style bugs by changing hacking on the source string to use of strlcpy().



Looking in detail, upstream (which appears to have disappeared) does 
have the explicit NULL termination in our last import. For consistency 
and given that we already have a strlcpy in that code, we should use 
strlcpy() there. Every modern OS out there has strlcpy(3) and if not 
they can figure out what to do.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328486 - head/usr.bin/fortune/fortune

[Pedro Giffuni]

On 01/27/18 18:08, Ian Lepore wrote:

On Sat, 2018-01-27 at 22:56 +, Conrad Meyer wrote:

I donļæ½t think dragonfly has anything to do with this?ļæ½ļæ½If youļæ½re
converting
bool increments to setting true values in FreeBSD, use
ļæ½true.ļæ½ļæ½ļæ½Thatļæ½s all
there is to it.

Best,
Conrad

style(9) emphasizes internal consistancy in several places. ļæ½The
uppercase TRUE/FALSE is currently the style within that code, so it
makes sense to stick with it. ļæ½If a (somewhat gratuitous) conversion to
the new style is made, it should be a separate commit just for that,
emphasizing that it changes just style and not functionality.


Indeed. I didn't want to mix FALSE with false and TRUE with true so I 
did the change as is to make the diff smaller and still have everything 
look consistent.


Further cleanups (DragonFly already did it) should be done in a 
different commit. I personally don't feel it's something important though.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328486 - head/usr.bin/fortune/fortune

[Pedro Giffuni]

On 01/27/18 17:56, Conrad Meyer wrote:
I don’t think dragonfly has anything to do with this?  If you’re 
converting bool increments to setting true values in FreeBSD, use 
“true.”  That’s all there is to it.




s/TRUE/true/
s/FALSE/false/

Then remove the two #defines for TRUE and FALSE, but I am on my way out, 
so feel free to do the change :).


Pedro.


Best,
Conrad

On Sat, Jan 27, 2018 at 1:44 PM Pedro Giffuni <p...@freebsd.org 
<mailto:p...@freebsd.org>> wrote:


Hi;


On 01/27/18 14:56, Conrad Meyer wrote:
> We can use 'true' and 'false' now.  (style(9) also suggests
using the
> C99 names instead of TRUE/FALSE.)

Yes, I noticed that change in DragonflyBSD but it is conceptually a
different change and it deserves a different commit.

Pedro.

> On Sat, Jan 27, 2018 at 9:43 AM, Pedro F. Giffuni
<p...@freebsd.org <mailto:p...@freebsd.org>> wrote:
>> Author: pfg
>> Date: Sat Jan 27 17:43:09 2018
>> New Revision: 328486
>> URL: https://svnweb.freebsd.org/changeset/base/328486
>>
>> Log:
>>    fortune(6): Fix gcc80 -Wbool-operation warnings.
>>
>>    Hinted by:    Dragonfly (git
4d1086765752f0569497d06460d95117c74f33ac)
>>
>> Modified:
>>    head/usr.bin/fortune/fortune/fortune.c
>>
>> Modified: head/usr.bin/fortune/fortune/fortune.c
>>

==
>> --- head/usr.bin/fortune/fortune/fortune.c      Sat Jan 27
17:24:59 2018        (r328485)
>> +++ head/usr.bin/fortune/fortune/fortune.c      Sat Jan 27
17:43:09 2018        (r328486)
>> @@ -289,35 +289,35 @@ getargs(int argc, char *argv[])
>>   #endif /* DEBUG */
>>                  switch(ch) {
>>                  case 'a':               /* any fortune */
>> -                       All_forts++;
>> +                       All_forts = TRUE;
>>                          break;
>>   #ifdef DEBUG
>>                  case 'D':
>>                          Debug++;
>>                          break;
>>   #endif /* DEBUG */
>> -               case 'e':
>> -                       Equal_probs++;  /* scatter un-allocted
prob equally */
>> +               case 'e':               /* scatter un-allocted
prob equally */
>> +                       Equal_probs = TRUE;
>>                          break;
>>                  case 'f':               /* find fortune files */
>> -                       Find_files++;
>> +                       Find_files = TRUE;
>>                          break;
>>                  case 'l':               /* long ones only */
>> -                       Long_only++;
>> +                       Long_only = TRUE;
>>                          Short_only = FALSE;
>>                          break;
>>                  case 'o':               /* offensive ones only */
>> -                       Offend++;
>> +                       Offend = TRUE;
>>                          break;
>>                  case 's':               /* short ones only */
>> -                       Short_only++;
>> +                       Short_only = TRUE;
>>                          Long_only = FALSE;
>>                          break;
>>                  case 'w':               /* give time to read */
>> -                       Wait++;
>> +                       Wait = TRUE;
>>                          break;
>>                  case 'm':                       /* dump out
the fortunes */
>> -                       Match++;
>> +                       Match = TRUE;
>>                          pat = optarg;
>>                          break;
>>                  case 'i':                       /*
case-insensitive match */
>>



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328493 - head/lib/libthr/thread

[Pedro Giffuni]

For the record ...

On 01/27/18 17:27, Pedro F. Giffuni wrote:

Author: pfg
Date: Sat Jan 27 22:27:55 2018
New Revision: 328493
URL: https://svnweb.freebsd.org/changeset/base/328493

Log:
   libthr: Fix missing break in switch.
  

Hmm..
There is no "missing break" I just copy/pasted the Coverity report (a 
false positive).

Sorry if that confused people.

Pedro.


   This is also a warning in recent GCC with -Wimplicit-fallthrough.
   
   CID:	1356262

   Obtained from:   DragonFly (git 0f037c78 - partial)

Modified:
   head/lib/libthr/thread/thr_printf.c

Modified: head/lib/libthr/thread/thr_printf.c
==
--- head/lib/libthr/thread/thr_printf.c Sat Jan 27 22:16:19 2018
(r328492)
+++ head/lib/libthr/thread/thr_printf.c Sat Jan 27 22:27:55 2018
(r328493)
@@ -95,6 +95,7 @@ next: c = *fmt++;
case 'p':
pstr(fd, "0x");
islong = 1;
+   /* FALLTHROUGH */
case 'd':
case 'u':
case 'x':



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328486 - head/usr.bin/fortune/fortune

[Pedro Giffuni]

Hi;


On 01/27/18 14:56, Conrad Meyer wrote:

We can use 'true' and 'false' now.  (style(9) also suggests using the
C99 names instead of TRUE/FALSE.)


Yes, I noticed that change in DragonflyBSD but it is conceptually a 
different change and it deserves a different commit.


Pedro.


On Sat, Jan 27, 2018 at 9:43 AM, Pedro F. Giffuni  wrote:

Author: pfg
Date: Sat Jan 27 17:43:09 2018
New Revision: 328486
URL: https://svnweb.freebsd.org/changeset/base/328486

Log:
   fortune(6): Fix gcc80 -Wbool-operation warnings.

   Hinted by:Dragonfly (git 4d1086765752f0569497d06460d95117c74f33ac)

Modified:
   head/usr.bin/fortune/fortune/fortune.c

Modified: head/usr.bin/fortune/fortune/fortune.c
==
--- head/usr.bin/fortune/fortune/fortune.c  Sat Jan 27 17:24:59 2018
(r328485)
+++ head/usr.bin/fortune/fortune/fortune.c  Sat Jan 27 17:43:09 2018
(r328486)
@@ -289,35 +289,35 @@ getargs(int argc, char *argv[])
  #endif /* DEBUG */
 switch(ch) {
 case 'a':   /* any fortune */
-   All_forts++;
+   All_forts = TRUE;
 break;
  #ifdef DEBUG
 case 'D':
 Debug++;
 break;
  #endif /* DEBUG */
-   case 'e':
-   Equal_probs++;  /* scatter un-allocted prob equally */
+   case 'e':   /* scatter un-allocted prob equally */
+   Equal_probs = TRUE;
 break;
 case 'f':   /* find fortune files */
-   Find_files++;
+   Find_files = TRUE;
 break;
 case 'l':   /* long ones only */
-   Long_only++;
+   Long_only = TRUE;
 Short_only = FALSE;
 break;
 case 'o':   /* offensive ones only */
-   Offend++;
+   Offend = TRUE;
 break;
 case 's':   /* short ones only */
-   Short_only++;
+   Short_only = TRUE;
 Long_only = FALSE;
 break;
 case 'w':   /* give time to read */
-   Wait++;
+   Wait = TRUE;
 break;
 case 'm':   /* dump out the fortunes */
-   Match++;
+   Match = TRUE;
 pat = optarg;
 break;
 case 'i':   /* case-insensitive match */



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Pedro Giffuni]

On 01/27/18 11:14, Warner Losh wrote:



On Jan 27, 2018 8:17 AM, "Pedro Giffuni" <p...@freebsd.org 
<mailto:p...@freebsd.org>> wrote:




On 01/26/18 06:36, Bruce Evans wrote:

    On Thu, 25 Jan 2018, Pedro Giffuni wrote:

On 25/01/2018 14:24, Bruce Evans wrote:

...
This code only works because (if?) nfs is the only
caller and nfs never
passes insane values.


I am starting to think that we should simply match
uio_resid and set it to ssize_t.
Returning the value to int is certainly not the solution.


Of course using the correct type (int) is part of the solution.

uio_must be checked before it is used for cookies, and after
checking it, it
is small so it fits easily in an int.  It must also checked to
be nonnegative,
so that it doesn't suffer unsigned poisoning when it is
promoted, so it would
also fit in a u_int, but using u_int to store it is silly as
using 1U instead
of 1 for a count of 1.

The bounds checking is something like:

if (ap->uio_resid < 0)
    ap->uio_resid = 0;
if (ap->a_ncookies != NULL) {
    if (ap->uio_resid >= 64 * 1024)
    ap->uio_resid = 64 * 1024;
    ncookies = ap->uio_resid;
}

This checks for negative values for all cases and converts to
0 (EOF) to
preserve historical behaviour for the syscall case and to
avoid overflow
for the cookies case (in case the caller is buggy). The
correct handling
is to return EINVAL, but EOF is good enough.

In the syscall case, uio_resid can be up to SSIZE_MAX, so
don't check it
or corrupt it by assigning it to an int or u_int.

Limit uio_resid from above only in the cookies case.  The
final limit should
be about 128K (whatever nfs uses) or maybe 1M. Don't return
EINVAL above
the limit, since nfs probably wouldn't know how to handle that
(by retrying
with a smaller size).  Test its handling of short counts
instead. It is
expected than nfs asks for 128K and we supply at most 64K. 
The supply is
always reduced at EOF.  Hopefully nfs doesn't treat the short
count as EOF.
It should retry until we supply 0.

Hmm ...

We have never checked the upper bound there, which doesn't mean it
was right.
I found MAXPHYS, which seems a more reasonable limit used in the
kernel for uio_resid.

I am checking the patch compiles and doesn't give surprises.


MAXPHYS is almost the right thing to check. There's per device limits 
for normal I/O, but that doesn't seem to be a strict limit for readdir.




OK... new patch, this time again trying to sanitize only ncookies (which 
can be int or u_int, doesn't matter to me).


Pedro.

Index: sys/fs/ext2fs/ext2_lookup.c
===
--- sys/fs/ext2fs/ext2_lookup.c	(revision 328478)
+++ sys/fs/ext2fs/ext2_lookup.c	(working copy)
@@ -145,7 +145,7 @@
 	off_t offset, startoffset;
 	size_t readcnt, skipcnt;
 	ssize_t startresid;
-	u_int ncookies;
+	int ncookies;
 	int DIRBLKSIZ = VTOI(ap->a_vp)->i_e2fs->e2fs_bsize;
 	int error;
 
@@ -152,7 +152,11 @@
 	if (uio->uio_offset < 0)
 		return (EINVAL);
 	ip = VTOI(vp);
+	if (uio->uio_resid < 0)
+		uio->uio_resid = 0;
 	if (ap->a_ncookies != NULL) {
+		if (uio->uio_resid > MAXPHYS)
+			uio->uio_resid = MAXPHYS;
 		ncookies = uio->uio_resid;
 		if (uio->uio_offset >= ip->i_size)
 			ncookies = 0;
Index: sys/ufs/ufs/ufs_vnops.c
===
--- sys/ufs/ufs/ufs_vnops.c	(revision 328478)
+++ sys/ufs/ufs/ufs_vnops.c	(working copy)
@@ -2170,7 +2170,7 @@
 	off_t offset, startoffset;
 	size_t readcnt, skipcnt;
 	ssize_t startresid;
-	u_int ncookies;
+	int ncookies;
 	int error;
 
 	if (uio->uio_offset < 0)
@@ -2178,7 +2178,11 @@
 	ip = VTOI(vp);
 	if (ip->i_effnlink == 0)
 		return (0);
+	if (uio->uio_resid < 0)
+		uio->uio_resid = 0;
 	if (ap->a_ncookies != NULL) {
+		if (uio->uio_resid > MAXPHYS)
+			uio->uio_resid = MAXPHYS;
 		ncookies = uio->uio_resid;
 		if (uio->uio_offset >= ip->i_size)
 			ncookies = 0;
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328479 - in head/sys: fs/ext2fs ufs/ufs

[Pedro Giffuni]

On 01/27/18 11:03, Konstantin Belousov wrote:

On Sat, Jan 27, 2018 at 03:33:52PM +, Pedro F. Giffuni wrote:

Author: pfg
Date: Sat Jan 27 15:33:52 2018
New Revision: 328479
URL: https://svnweb.freebsd.org/changeset/base/328479

Log:
   {ext2|ufs}_readdir: Set limit on valid ncookies values.
   
   Sanitize the values that will be assigned to ncookies so that we ensure

   they are sane and we can handle them.
   
   Let ncookies signed as it was before r328346. The valid range is such

   that unsigned values are not required and we are not able to avoid at
   least one cast anyways.
   
   Hinted by:	bde


Modified:
   head/sys/fs/ext2fs/ext2_lookup.c
   head/sys/ufs/ufs/ufs_vnops.c

Modified: head/sys/fs/ext2fs/ext2_lookup.c
==
--- head/sys/fs/ext2fs/ext2_lookup.cSat Jan 27 13:46:55 2018
(r328478)
+++ head/sys/fs/ext2fs/ext2_lookup.cSat Jan 27 15:33:52 2018
(r328479)
@@ -145,14 +145,18 @@ ext2_readdir(struct vop_readdir_args *ap)
off_t offset, startoffset;
size_t readcnt, skipcnt;
ssize_t startresid;
-   u_int ncookies;
+   int ncookies;
int DIRBLKSIZ = VTOI(ap->a_vp)->i_e2fs->e2fs_bsize;
int error;
  
  	if (uio->uio_offset < 0)

return (EINVAL);
ip = VTOI(vp);
+   if (uio->uio_resid < 0)
+   uio->uio_resid = 0;
if (ap->a_ncookies != NULL) {
+   if (uio->uio_resid > MAXPHYS)
+   uio->uio_resid = MAXPHYS;
ncookies = uio->uio_resid;
if (uio->uio_offset >= ip->i_size)
ncookies = 0;

Modified: head/sys/ufs/ufs/ufs_vnops.c
==
--- head/sys/ufs/ufs/ufs_vnops.cSat Jan 27 13:46:55 2018
(r328478)
+++ head/sys/ufs/ufs/ufs_vnops.cSat Jan 27 15:33:52 2018
(r328479)
@@ -2170,7 +2170,7 @@ ufs_readdir(ap)
off_t offset, startoffset;
size_t readcnt, skipcnt;
ssize_t startresid;
-   u_int ncookies;
+   int ncookies;
int error;
  
  	if (uio->uio_offset < 0)

@@ -2178,7 +2178,11 @@ ufs_readdir(ap)
ip = VTOI(vp);
if (ip->i_effnlink == 0)
return (0);
+   if (uio->uio_resid < 0)
+   uio->uio_resid = 0;
if (ap->a_ncookies != NULL) {
+   if (uio->uio_resid > MAXPHYS)
+   uio->uio_resid = MAXPHYS;

You just break nfs server.

Look at nfsrvd_readdir() call to VOP_READDIR.  Almost all code which calls
VOP_READDIR() memoize the value of uio_resid before and after the call and
interpret the difference as the amount of data returned.

I said above that only nfs server is broken, because only the server uses
cookies, otherwise your patch would break everything.
Ugh, yes .. I completely missed the fact that uio is a pointer to the 
real thing, not a local copy.

This still should never go off limits but it is certainly wrong.

I reverted it sorry.

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328474 - head/sys/contrib/libnv

[Pedro Giffuni]

On 01/27/18 10:32, Cy Schubert wrote:

In message <201801271258.w0rcwml0078...@repo.freebsd.org>, Mariusz Zaborski
wri
tes:

Author: oshogbo
Date: Sat Jan 27 12:58:21 2018
New Revision: 328474
URL: https://svnweb.freebsd.org/changeset/base/328474

Log:
   Add SPDX tags for nv(9).
   
   MFC after:	2 weeks


Modified:
   head/sys/contrib/libnv/cnvlist.c
   head/sys/contrib/libnv/dnvlist.c
   head/sys/contrib/libnv/nv_impl.h
   head/sys/contrib/libnv/nvlist.c
   head/sys/contrib/libnv/nvlist_impl.h
   head/sys/contrib/libnv/nvpair.c
   head/sys/contrib/libnv/nvpair_impl.h


When was it decided to add SPDX to contrib?




We didn't, it's up to upstream, but being the author (of at least part 
of it) he can do what he wants ;).


Pedro.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Pedro Giffuni]

On 01/26/18 06:36, Bruce Evans wrote:

On Thu, 25 Jan 2018, Pedro Giffuni wrote:


On 25/01/2018 14:24, Bruce Evans wrote:

...
This code only works because (if?) nfs is the only caller and nfs never
passes insane values.



I am starting to think that we should simply match uio_resid and set 
it to ssize_t.

Returning the value to int is certainly not the solution.


Of course using the correct type (int) is part of the solution.

uio_must be checked before it is used for cookies, and after checking 
it, it
is small so it fits easily in an int.  It must also checked to be 
nonnegative,
so that it doesn't suffer unsigned poisoning when it is promoted, so 
it would
also fit in a u_int, but using u_int to store it is silly as using 1U 
instead

of 1 for a count of 1.

The bounds checking is something like:

if (ap->uio_resid < 0)
    ap->uio_resid = 0;
if (ap->a_ncookies != NULL) {
    if (ap->uio_resid >= 64 * 1024)
    ap->uio_resid = 64 * 1024;
    ncookies = ap->uio_resid;
}

This checks for negative values for all cases and converts to 0 (EOF) to
preserve historical behaviour for the syscall case and to avoid overflow
for the cookies case (in case the caller is buggy).  The correct handling
is to return EINVAL, but EOF is good enough.

In the syscall case, uio_resid can be up to SSIZE_MAX, so don't check it
or corrupt it by assigning it to an int or u_int.

Limit uio_resid from above only in the cookies case.  The final limit 
should

be about 128K (whatever nfs uses) or maybe 1M.  Don't return EINVAL above
the limit, since nfs probably wouldn't know how to handle that (by 
retrying

with a smaller size).  Test its handling of short counts instead. It is
expected than nfs asks for 128K and we supply at most 64K.  The supply is
always reduced at EOF.  Hopefully nfs doesn't treat the short count as 
EOF.

It should retry until we supply 0.


Hmm ...

We have never checked the upper bound there, which doesn't mean it was 
right.
I found MAXPHYS, which seems a more reasonable limit used in the kernel 
for uio_resid.


I am checking the patch compiles and doesn't give surprises.

Pedro.


After limiting uio_resid, assign it to the int ncookies.

This doesn't fix the abuse of the ncookies counter to hold the size of 
the

cookies array in bytes for this and the next couple of statements.

Normally the bounds checking should be at the top level, with at most
KASSERT()s at lower levels, but here the levels are mixed, and it isn't
clear if kernel callers have already checked, and it doesn't cost much 
to do much the same checking for the kernel callers as for the syscall 
callers.


Perhaps the 128K limit is good for all cases (this depends on callers not
having buggy short count handling).  Directories of this size are very
rare (don't forget to create very large ones when you test this). Doing
anything with directories of this size tends to be slow anyway, and the
slowness has nothing to do with reading only 128K instead of SSIZE_MAX
bytes at a time.

readdir() in FreeBSD seems to use a read size of only PAGE_SIZE, except
in the unionfs case it seems to try to read the whole direction. It
malloc()s the buffer in both cases.  Blindy malloc()ing or mmap()ing
a buffer large enough for a whole file or directory is no good, since
in theory even directory sizes can be much larger than memory.

Bruce



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Pedro Giffuni]

On 01/26/18 06:36, Bruce Evans wrote:

On Thu, 25 Jan 2018, Pedro Giffuni wrote:


On 25/01/2018 14:24, Bruce Evans wrote:

...
This code only works because (if?) nfs is the only caller and nfs never
passes insane values.



I am starting to think that we should simply match uio_resid and set 
it to ssize_t.

Returning the value to int is certainly not the solution.


Of course using the correct type (int) is part of the solution.


int *was* the correct type, now it doesn't cover all the range.

uio_must be checked before it is used for cookies, and after checking 
it, it
is small so it fits easily in an int.  It must also checked to be 
nonnegative,


Our problem is not really uio_*, our problem is ncookies and we only 
test that it is >0.
I think the attached patch, still keeping the unsigned ncookies, is 
sufficient.




so that it doesn't suffer unsigned poisoning when it is promoted, so 
it would
also fit in a u_int, but using u_int to store it is silly as using 1U 
instead

of 1 for a count of 1.

The bounds checking is something like:

if (ap->uio_resid < 0)
    ap->uio_resid = 0;
if (ap->a_ncookies != NULL) {
    if (ap->uio_resid >= 64 * 1024)
    ap->uio_resid = 64 * 1024;
    ncookies = ap->uio_resid;
}

This checks for negative values for all cases and converts to 0 (EOF) to
preserve historical behaviour for the syscall case and to avoid overflow
for the cookies case (in case the caller is buggy).  The correct handling
is to return EINVAL, but EOF is good enough.

This also touches uio_resid which is probably not good as it is used 
later on.

Our job is not to "fix" the caller but only to apply a reasonable behavior.

I also don't like the magic numbers here.


In the syscall case, uio_resid can be up to SSIZE_MAX, so don't check it
or corrupt it by assigning it to an int or u_int.



The minimal type conversion does not really involve corruption: ncookies 
is local

and the caller will not perceive any change.

Pedro.

Limit uio_resid from above only in the cookies case.  The final limit 
should

be about 128K (whatever nfs uses) or maybe 1M.  Don't return EINVAL above
the limit, since nfs probably wouldn't know how to handle that (by 
retrying

with a smaller size).  Test its handling of short counts instead. It is
expected than nfs asks for 128K and we supply at most 64K.  The supply is
always reduced at EOF.  Hopefully nfs doesn't treat the short count as 
EOF.

It should retry until we supply 0.

After limiting uio_resid, assign it to the int ncookies.

This doesn't fix the abuse of the ncookies counter to hold the size of 
the

cookies array in bytes for this and the next couple of statements.

Normally the bounds checking should be at the top level, with at most
KASSERT()s at lower levels, but here the levels are mixed, and it isn't
clear if kernel callers have already checked, and it doesn't cost much 
to do much the same checking for the kernel callers as for the syscall 
callers.


Perhaps the 128K limit is good for all cases (this depends on callers not
having buggy short count handling).  Directories of this size are very
rare (don't forget to create very large ones when you test this). Doing
anything with directories of this size tends to be slow anyway, and the
slowness has nothing to do with reading only 128K instead of SSIZE_MAX
bytes at a time.

readdir() in FreeBSD seems to use a read size of only PAGE_SIZE, except
in the unionfs case it seems to try to read the whole direction. It
malloc()s the buffer in both cases.  Blindy malloc()ing or mmap()ing
a buffer large enough for a whole file or directory is no good, since
in theory even directory sizes can be much larger than memory.

Bruce


Index: sys/fs/ext2fs/ext2_lookup.c
===
--- sys/fs/ext2fs/ext2_lookup.c	(revision 328443)
+++ sys/fs/ext2fs/ext2_lookup.c	(working copy)
@@ -153,7 +153,10 @@
 		return (EINVAL);
 	ip = VTOI(vp);
 	if (ap->a_ncookies != NULL) {
-		ncookies = uio->uio_resid;
+		if (uio->uio_resid < 0)
+			ncookies = 0;
+		else
+			ncookies = uio->uio_resid;
 		if (uio->uio_offset >= ip->i_size)
 			ncookies = 0;
 		else if (ip->i_size - uio->uio_offset < ncookies)
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Pedro Giffuni]

On 25/01/2018 14:24, Bruce Evans wrote:

On Thu, 25 Jan 2018, Pedro Giffuni wrote:

This is almost unreadable due to hard-coded UTF-8 (mostly for tabs 
corrupted

to spaces) even in previously-literally quoted C code.


Mailer agents ... they all suck :(

On 01/25/18 11:28, Bruce Evans wrote:

On Wed, 24 Jan 2018, Pedro F. Giffuni wrote:


[... Most unreadable lines deleted]


X ncookies = uio->uio_resid;

This has a more serious type error and consequent overflow bugs. 
uio_resid
used to have type int, and cookies had to have type int to match 
that, else

there overflow occurs before the bounds checks.  Now uio_resid has type
ssize_t, which is excessively large on 64-bit arches (64 bits then), 
so the
assignment overflowed when ncookies had type int and uio_resid > 
INT_MAX.
Now it overflows differently when uio_resid > UINT_MAX, and unsign 
extension

causes overflow when uio_resid < 0.  There might be a sanity check on
uio_resid at higher levels, but I can only find a check related to 
EOF in

vfs_read_dirent().

I will argue that none of the code in this function is prepared for 
the eventually of

uio->uio_resid < 0


All of it except the cookies code has to be prepared for that, and seems
to handle it OK, since this userland can set uio_resid.  The other code
is not broken by either the ssize_t expansion or the unsigned bugs, since
it mostly doesn't truncate uio_resid by assigning it to a variable of the
wrong type (it uses uio->uio_resid in-place, except for copying its 
initial
value to startresid, and startresid is not missing the ssize_t 
expansion).

It mostly does comparisons of the form (uio->uio_resid > 0), where it is
0 in uio_resid means EOF, negative is treated as EOF, and strictly 
positive

means more to do.

There is a clear up-front check that uio_offset >= 0 (return EINVAL if
uio_offset < 0).  This is not needed for the trusted nfs caller, but is
needed for syscalls and is done for both.


In that case we would have a rather spectacular failure in malloc.
Unsigning ncookies is a theoretical, although likely impractical, 
improvement here.


No, it increases the bug slightly.  E.g., if uio_resid is -1, ncookies
was -1 / (offsetof(...) + 4) + 1 = 0 + 1 after rounding.  This might even
work (1 cookie at a time, just like if the caller asked for that).  Now
ncookies is -1U / (offsetof(...) + 4) + 1 = a large value. However, if
uio_resid was slightly more negative than -2 * (offsetof(...) + 4), then
ncookies was -1 and in the multiplication this overflows to -1U = a large
value and the result is much the same as for earlier overflow on 
assignment

to u_int ncookies.

This code only works because (if?) nfs is the only caller and nfs never
passes insane values.



I am starting to think that we should simply match uio_resid and set it 
to ssize_t.

Returning the value to int is certainly not the solution.

It is not clear to me that using int or u_int makes a difference 
given it is a local variable
and in this scope the signedness of the variable is basically 
irrelevant.


It is clear to me that overflow bugs occur with both if untrusted 
callers are

allowed.

From a logical point of view .. we can't really have a negative 
number of cookies.


Malicious and buggy callers do illogical things to get through holes in
your logic.

It is also illogical to have a zero number of cookies, but ncookies can
be 0 in various ways.  First, ncookies is 0 in the EOF case (and cookies
are requested).  We depend on 0 not being an invalid size for malloc()
so that we malloc() nothing and later do more nothings in the main loop.
This is a standard use for 0.  If you don't like negative numbers, then
you also shouldn't like 0.  Both exist to make some calculations easier.
Later, ncookies is abused as a residual count, so it becomes 0 at the 
end.

This is another standard use for 0.

Bruce


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Pedro Giffuni]

On 01/25/18 11:28, Bruce Evans wrote:

On Wed, 24 Jan 2018, Pedro F. Giffuni wrote:


Log:
 ext2fs|ufs:Unsign some values related to allocation.

 When allocating memory through malloc(9), we always expect the 
amount of
 memory requested to be unsigned as a negative value would either 
stand for

 an error or an overflow.
 Unsign some values, found when considering the use of 
mallocarray(9), to

 avoid unnecessary casting. Also consider that indexes should be of
 at least the same size/type as the upper limit they pretend to index.


This might not break much, but it adds many more type errors and bogus
(implicit) casts than it fixes.  It actually changes the brokenness of 
the

first variable touched:


Modified: head/sys/fs/ext2fs/ext2_lookup.c
== 

--- head/sys/fs/ext2fs/ext2_lookup.c    Wed Jan 24 17:52:06 2018    
(r328345)
+++ head/sys/fs/ext2fs/ext2_lookup.c    Wed Jan 24 17:58:48 2018    
(r328346)

@@ -145,9 +145,9 @@ ext2_readdir(struct vop_readdir_args *ap)
off_t offset, startoffset;
size_t readcnt, skipcnt;
ssize_t startresid;
-    int ncookies;
int DIRBLKSIZ = VTOI(ap->a_vp)->i_e2fs->e2fs_bsize;
int error;
+    u_int ncookies;


The first bug is only a style bug (unsorting the u_int after the ints 
even

when its variable was accidentally already before the ints.


Yup, my error.



if (uio->uio_offset < 0)
    return (EINVAL);


That is the only change in this file.  No comment on other changes.

ncookies is mostly used in contexts other than for multiplying by it 
before
calling malloc(), and its type is now inconsistent with most other 
places.


The other places are:

X ncookies = uio->uio_resid;

This has a more serious type error and consequent overflow bugs. 
uio_resid
used to have type int, and cookies had to have type int to match that, 
else

there overflow occurs before the bounds checks.  Now uio_resid has type
ssize_t, which is excessively large on 64-bit arches (64 bits then), 
so the

assignment overflowed when ncookies had type int and uio_resid > INT_MAX.
Now it overflows differently when uio_resid > UINT_MAX, and unsign 
extension

causes overflow when uio_resid < 0.  There might be a sanity check on
uio_resid at higher levels, but I can only find a check related to EOF in
vfs_read_dirent().

I will argue that none of the code in this function is prepared for the 
eventually of

uio->uio_resid < 0

In that case we would have a rather spectacular failure in malloc.
Unsigning ncookies is a theoretical, although likely impractical, 
improvement here.


It is not clear to me that using int or u_int makes a difference given 
it is a local variable

and in this scope the signedness of the variable is basically irrelevant.

From a logical point of view .. we can't really have a negative number 
of cookies.


Next, we do some bounds checking which seems to be correct modulo 
previous
overflows, and show the care needed for unsigned variables (i_size is 
unsigned

and must be compared with uio_offset which is signed).

Next, we assign ncookies, to ap_ncookies which still has the correct type
(plain signed int).  If ncookies were actually large enough to need a 
u_int,

or worse a 64-bit ssize_t, then this would overflow.

Later, we KASSERT() that ncookies > 0.  This might cause a compiler 
warning
"unsigned comparison with 0" now that ncookies is unsigned.  We count 
down

ncookies, but the loop termination condition is complicated and we don't
get any benefits from the possible micro-optimization of using 
ncookies as

a loop counter that counts down to 0.

To fix the problem that mallocarray() wanted a size_t arg, ncookies could
be cast to size_t, but that would be silly.  The prototype does the same
cast automatically even for cases with sign mismatches.  Now malloc()
wants a type of size_t (after further breakage to change malloc()s arg
type).  Many conversions are still involved, and casting would at most 
limit

compiler warnings:  the code is now:

X malloc(ncookies * sizeof(*cookies), ...)

First, ncookies and sizeof(...) are promoted to a common type. When 
ncookies
was int, usually it was promoted to size_t and sizeof(...) was not 
promoted.
But on exotic arches with size_t smaller than int, sizeof(...) is 
promoted to

int and ncookies is not promoted.

Next, the type of the result of the multiplication is the common type.

Finally, the prototype used to convert to u_long, but now converts to 
size_t.

When the common type is size_t, then the conversion is now null. When the
common type was int, the conversion was promotion to u_long, but it is 
now

demotion to size_t.

All this only obviously works in practice because all the variables and
the product are small, so they are smaller than all of INT_MAX, SIZE_MAX
and ULONG_MAX on all supported and unsupported arches.  However, it is
hard to write bounds checks that obviously handle all cases, except
by using 

Re: svn commit: r328340 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Pedro Giffuni]

On 25/01/2018 09:42, Bruce Evans wrote:

On Wed, 24 Jan 2018, Pedro F. Giffuni wrote:


Log:
 Revert r327781,  r328093, r328056:
 ufs|ext2fs: Revert uses of mallocarray(9).

 These aren't really useful: drop them.
 Variable unsigning will be brought again later.


Variable "unsigning" (that is, adding unsign extension bugs) is even more
negatively useful than mallocarray(), so should not be brought back.


Any specific case? I already brought those back in r328346.

Unsigning variables used _only_ for array sizes and element counts 
doesn't
cause any new problems (and fixes warnings about converting from 
signed to

unsigned when calling malloc*()), but it is a lot of work to check that
they aren't used for other things where their signedness matters (perhaps
differences or loops that count down to -1 instead of 0).

I did check and had mckusick crosscheck before, but as you say it is 
usually a lot of work and my have missed something.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328218 - in head/sys: amd64/amd64 arm/xscale/ixp425 arm64/arm64 cam cam/ctl compat/ndis dev/aacraid dev/advansys dev/ath dev/beri/virtio dev/bnxt dev/bwn dev/ciss dev/cxgbe/crypto dev

[Pedro Giffuni]

On 24/01/2018 13:40, Hans Petter Selasky wrote:

On 01/24/18 19:28, Warner Losh wrote:

Does mallocarray(10 ,1Gb) panic on i386? It does not. It should.


Hi,

If M_WAITOK is specified, then sleep forever and print a message.
Else return NULL ?



FWIW, I suggested a panic for M_WAITOK and returning NULL for M_NOWAIT, 
but cem didn't like it because it was inconsistent.


I think the current argument is more about the size/trigger than the 
behavior though.


Cheers,
Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328218 - in head/sys: amd64/amd64 arm/xscale/ixp425 arm64/arm64 cam cam/ctl compat/ndis dev/aacraid dev/advansys dev/ath dev/beri/virtio dev/bnxt dev/bwn dev/ciss dev/cxgbe/crypto dev

[Pedro Giffuni]

...
On 24/01/2018 13:30, Conrad Meyer wrote:

...
size_t can handle 10GB, but u_long can't.
This whole argument hinges upon incorrect assumption that size_t is
larger than u_long.


Hmm...

Lets just make it "unsigned long" to be consistent with malloc(9) and 
avoid confusion?


Pedro.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328218 - in head/sys: amd64/amd64 arm/xscale/ixp425 arm64/arm64 cam cam/ctl compat/ndis dev/aacraid dev/advansys dev/ath dev/beri/virtio dev/bnxt dev/bwn dev/ciss dev/cxgbe/crypto dev

[Pedro Giffuni]

On 01/24/18 12:37, Conrad Meyer wrote:

On Tue, Jan 23, 2018 at 11:40 AM, Pedro Giffuni <p...@freebsd.org> wrote:

On 23/01/2018 14:08, Conrad Meyer wrote:

On Sun, Jan 21, 2018 at 7:42 AM, Pedro F. Giffuni <p...@freebsd.org> wrote:

Author: pfg
Date: Sun Jan 21 15:42:36 2018
New Revision: 328218

I'm confused about this change.  Wouldn't it be better to remove the
annotation/attributes from mallocarray() than to remove the protection
against overflow?


Not in my opinion: it would be better to detect such overflows at compile
time (or through a static analyzer) than to have late notification though
panics.

Sure, it would be better, but some situations are only detected at
runtime -- hence mallocarray.  And occasional use of the annotations
on systems with plenty of RAM would keep the source tree free of
compiler-detectable overflows, which I suspect are incredibly
uncommon.


I think the runtime error cases are way more uncommon, specially if the 
checks are unnecessary.
In any case I collected the patch with malloc--> mallocarray changes in 
PR 225197.

Maybe their are useful with fuzzing.


The blind use of mallocarray(9) is probably a mistake also: we
shouldn't use it unless there is some real risk of overflow.

I'm not sure I follow that.


You normally don't get "tainted" values in the kernel. Most of the 
allocations in question were variables with very small number multiplied 
by the size of an int. As Bruce mentioned, we don't do big allocations 
with malloc(9), at least not the size required to get an unsigned number 
overflow. In such cases checking for an overflow is a complete waste of 
time. And then there is also the bug of mallocarray(9) size types not 
matching malloc(9).



(If the compiler is fixed in the future to not use
excessive memory with these attributes, they can be conditionalized on
compiler version, of course.)

All in all, the compiler is not provably wrong: it's just using more swap
space, which is rather inconvenient for small platforms but not necessarily
wrong.

Seems wrong if it's a noticeable amount.  Maybe we could flip the
annotations on or off with a low-ram build knob or something like
that.


There is actually no proof that this was related to the attributes. I 
absolutely dislike the idea of disabling the attributes and even more 
the idea of adding complex machinery to the system headers to account 
for such case.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328218 - in head/sys: amd64/amd64 arm/xscale/ixp425 arm64/arm64 cam cam/ctl compat/ndis dev/aacraid dev/advansys dev/ath dev/beri/virtio dev/bnxt dev/bwn dev/ciss dev/cxgbe/crypto dev

[Pedro Giffuni]

On 01/24/18 03:50, Bruce Evans wrote:

On Tue, 23 Jan 2018, Pedro Giffuni wrote:


On 23/01/2018 14:08, Conrad Meyer wrote:

Hi Pedro,

On Sun, Jan 21, 2018 at 7:42 AM, Pedro F. Giffuni <p...@freebsd.org> 
wrote:

Author: pfg
Date: Sun Jan 21 15:42:36 2018
New Revision: 328218
URL: https://svnweb.freebsd.org/changeset/base/328218

Log:
   Revert r327828, r327949, r327953, r328016-r328026, r328041:
   Uses of mallocarray(9).

   The use of mallocarray(9) has rocketed the required swap to 
build FreeBSD.
   This is likely caused by the allocation size attributes which 
put extra pressure

   on the compiler.

I'm confused about this change.  Wouldn't it be better to remove the
annotation/attributes from mallocarray() than to remove the protection
against overflow?


It would be better to remove mallocarray().

I am fine with that: Its probably better to stop the bug now before it 
propagates.

This said, several drivers, including drm2, have #defines to do the same.

Not in my opinion: it would be better to detect such overflows at 
compile time (or through a static analyzer) than to have late 
notification though panics. The blind use of mallocarray(9) is 
probably a mistake also: we shouldn't use it unless there is some 
real risk of overflow.


Excellent!  There is no real risk of overflow except in cases that are
broken anyway.  The overflow checks in mallocarray() are insufficient 
even

for detecting overflow and don't detect most broken cases when they are
sufficient.  This leaves no cases where even non-blind use of it does any
good.


   (If the compiler is fixed in the future to not use
excessive memory with these attributes, they can be conditionalized on
compiler version, of course.)


All in all, the compiler is not provably wrong: it's just using more 
swap space, which is rather inconvenient for small platforms but not 
necessarily wrong.


I don't know why the compiler uses more swap space, but the general 
brokenness

of mallocarray() is obvious.

Callers must somehow ensure that the allocation size is not too large.
Too large is closer to 64K than SIZE_MAX for most allocations. Some
bloatware allocates a few MB, but malloc() is rarely used for that.
vmstat -m has been broken to not show a summary of sizes at the end,
but on freefall now it seems to shows a maximum malloc() size of 64K,
and vmstat -z confirms this by not showing any malloc() bucket sizes
larger than 64K, and in fact kern_malloc.c only has statically allocatd
bucket sizes up to 64K.  (vmstat -z never had a summary at the end to
break).  Much larger allocations are mostly done using k*alloc(), and
slightly larger allocations are usually done using UMA.  zfs does
zillions of allocations using UMA, and the largest one seems to be 16M.

If the caller doesn't check, this gives a Denial of Service security hole
if the size and count are controlled by userland.  If the size and count
are controlled by the kernel, then the overflow check is not very useful.
It is less useful than most KASSERT()s which are left out of production
kernels.  The caller must keep its allocation sizes not much more than
64K, and once it does that it is unlikely to make them larger than 
SIZE_MAX

sometimes.

The overflow checks in mallocarray have many type errors so they don't
always work:

X Modified: head/share/man/man9/malloc.9
X 
==

X --- head/share/man/man9/malloc.9    Sun Jan  7 10:29:15 2018 (r327673)
X +++ head/share/man/man9/malloc.9    Sun Jan  7 13:21:01 2018 (r327674)
X @@ -45,6 +45,8 @@
X  .In sys/malloc.h
X  .Ft void *
X  .Fn malloc "unsigned long size" "struct malloc_type *type" "int flags"
X +.Ft void *
X +.Fn mallocarray "size_t nmemb" "size_t size" "struct malloc_type 
*type" "int flags"


One type error is already obvious.  malloc(9)'s arg doesn't have type 
size_t
like malloc(3)'s arg.  The arg type is u_long in the kernel. 
mallocarray()'s

types are inconsistent with this.



This would be relatively easy to "fix", at least so that the types match 
malloc(9).

The rest is likely more painful, if it has solution at all.


size_t happens to have the same representation as u_long on all supported
arches, so this doesn't cause many problems now.  On 32-bit arches, 
size_t

hs type u_int.  u_int has smaller rank than u_long, so compilers could
reasonably complain that converting a u_long to a size_t is a downcast.
They shouldn't complain that converting a size_t to a u_long to pass it
to malloc() is an upcast, so there is no problem in typical sloppy code
that uses size_t for sizes and passes these to malloc().  More careful
ciode would use u_long for size to pass to malloc() and compiler's might
complain about downcasting to pass to mallocarray() instead.

Otherwise (on exotic machines with size_t larger than u_long), passing
size_t's to malloc() is a bug unless they valu

Re: svn commit: r328342 - in head/sys: amd64/linux dev/usb/storage

[Pedro Giffuni]

On 01/24/18 12:04, Edward Tomasz Napierala wrote:

Author: trasz
Date: Wed Jan 24 17:04:01 2018
New Revision: 328342
URL: https://svnweb.freebsd.org/changeset/base/328342

Log:
   Add SPDX identifiers to linux_ptrace.c and cfumass.c.
   
   MFC after:	2 weeks



Thanks for all these!

For the record ... I am not considering MFCing SPDX tags.
There is just too much to be done for current to worry about stable at 
all ... but I won't stop anyone from doing so ;).


Pedro.


Modified:
   head/sys/amd64/linux/linux_ptrace.c
   head/sys/dev/usb/storage/cfumass.c

Modified: head/sys/amd64/linux/linux_ptrace.c
==
--- head/sys/amd64/linux/linux_ptrace.c Wed Jan 24 16:58:26 2018
(r328341)
+++ head/sys/amd64/linux/linux_ptrace.c Wed Jan 24 17:04:01 2018
(r328342)
@@ -1,4 +1,6 @@
  /*-
+ * SPDX-License-Identifier: BSD-2-Clause-FreeBSD
+ *
   * Copyright (c) 2017 Edward Tomasz Napierala 
   * All rights reserved.
   *

Modified: head/sys/dev/usb/storage/cfumass.c
==
--- head/sys/dev/usb/storage/cfumass.c  Wed Jan 24 16:58:26 2018
(r328341)
+++ head/sys/dev/usb/storage/cfumass.c  Wed Jan 24 17:04:01 2018
(r328342)
@@ -1,4 +1,6 @@
  /*-
+ * SPDX-License-Identifier: BSD-2-Clause-FreeBSD
+ *
   * Copyright (c) 2016 The FreeBSD Foundation
   * All rights reserved.
   *



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328218 - in head/sys: amd64/amd64 arm/xscale/ixp425 arm64/arm64 cam cam/ctl compat/ndis dev/aacraid dev/advansys dev/ath dev/beri/virtio dev/bnxt dev/bwn dev/ciss dev/cxgbe/crypto dev

[Pedro Giffuni]

Hi;

On 23/01/2018 17:13, Bryan Drewery wrote:

On 1/23/2018 11:40 AM, Pedro Giffuni wrote:

Hi;


On 23/01/2018 14:08, Conrad Meyer wrote:

Hi Pedro,

On Sun, Jan 21, 2018 at 7:42 AM, Pedro F. Giffuni <p...@freebsd.org>
wrote:

Author: pfg
Date: Sun Jan 21 15:42:36 2018
New Revision: 328218
URL: https://svnweb.freebsd.org/changeset/base/328218

Log:
    Revert r327828, r327949, r327953, r328016-r328026, r328041:
    Uses of mallocarray(9).

    The use of mallocarray(9) has rocketed the required swap to build
FreeBSD.
    This is likely caused by the allocation size attributes which put
extra pressure
    on the compiler.

I'm confused about this change.  Wouldn't it be better to remove the
annotation/attributes from mallocarray() than to remove the protection
against overflow?

Not in my opinion: it would be better to detect such overflows at
compile time (or through a static analyzer) than to have late
notification though panics. The blind use of mallocarray(9) is probably
a mistake also: we shouldn't use it unless there is some real risk of
overflow.


    (If the compiler is fixed in the future to not use
excessive memory with these attributes, they can be conditionalized on
compiler version, of course.)

All in all, the compiler is not provably wrong: it's just using more
swap space, which is rather inconvenient for small platforms but not
necessarily wrong.

Pedro.



I haven't dug into this to understand it all, but if mallocarray() is
causing this sort of compilation problem then isn't the problem the
compiler?  Why keep a "dangerous" function around and not actually fix
it?  Is there a bug somewhere to fix the compilation load?



In all honesty .. I don't know what is going on. I theorize it may be 
related to attributes but who knows.


I wouldn't say mallocarray(9) is dangerous, it's just not worth my time 
specially since most of those multiplications have no chance of overflowing.


I will put up a patch with all the malloc --> mallocarray replacements 
in case someone wants to spend time on it.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r328218 - in head/sys: amd64/amd64 arm/xscale/ixp425 arm64/arm64 cam cam/ctl compat/ndis dev/aacraid dev/advansys dev/ath dev/beri/virtio dev/bnxt dev/bwn dev/ciss dev/cxgbe/crypto dev

[Pedro Giffuni]

Hi;


On 23/01/2018 14:08, Conrad Meyer wrote:

Hi Pedro,

On Sun, Jan 21, 2018 at 7:42 AM, Pedro F. Giffuni  wrote:

Author: pfg
Date: Sun Jan 21 15:42:36 2018
New Revision: 328218
URL: https://svnweb.freebsd.org/changeset/base/328218

Log:
   Revert r327828, r327949, r327953, r328016-r328026, r328041:
   Uses of mallocarray(9).

   The use of mallocarray(9) has rocketed the required swap to build FreeBSD.
   This is likely caused by the allocation size attributes which put extra 
pressure
   on the compiler.

I'm confused about this change.  Wouldn't it be better to remove the
annotation/attributes from mallocarray() than to remove the protection
against overflow?


Not in my opinion: it would be better to detect such overflows at 
compile time (or through a static analyzer) than to have late 
notification though panics. The blind use of mallocarray(9) is probably 
a mistake also: we shouldn't use it unless there is some real risk of 
overflow.



   (If the compiler is fixed in the future to not use
excessive memory with these attributes, they can be conditionalized on
compiler version, of course.)


All in all, the compiler is not provably wrong: it's just using more 
swap space, which is rather inconvenient for small platforms but not 
necessarily wrong.


Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r327675 - head/sys/netpfil/pf

[Pedro Giffuni]

> On Jan 17, 2018, at 18:37, Gleb Smirnoff  wrote:
> 
> On Sun, Jan 07, 2018 at 04:44:23PM +0200, Konstantin Belousov wrote:
> K> On Sun, Jan 07, 2018 at 01:35:15PM +, Kristof Provost wrote:
> K> > Author: kp
> K> > Date: Sun Jan  7 13:35:15 2018
> K> > New Revision: 327675
> K> > URL: https://svnweb.freebsd.org/changeset/base/327675
> K> > 
> K> > Log:
> K> >   pf: Avoid integer overflow issues by using mallocarray() iso. malloc()
> K> >   
> K> >   pfioctl() handles several ioctl that takes variable length input, these
> K> >   include:
> K> >   - DIOCRADDTABLES
> K> >   - DIOCRDELTABLES
> K> >   - DIOCRGETTABLES
> K> >   - DIOCRGETTSTATS
> K> >   - DIOCRCLRTSTATS
> K> >   - DIOCRSETTFLAGS
> K> >   
> K> >   All of them take a pfioc_table struct as input from userland. One of
> K> >   its elements (pfrio_size) is used in a buffer length calculation.
> K> >   The calculation contains an integer overflow which if triggered can 
> lead
> K> >   to out of bound reads and writes later on.
> K> So the size of the allocation is controlled directly from the userspace ?
> K> This is an easy DoS, and by itself is perhaps bigger issue than the 
> overflow.
> 
> Yes, this is one of the dirties parts of pf. The whole API to read and 
> configure
> tables from the userland calls to be rewritten from scratch. Conversion from
> malloc to mallocarray really does nothing. Better just put a maximum value
> cap.
> 

FWIW, the associated NULL checks just became no-ops since overflows in 
mallocarray(9) will now cause panics.

Either the flags should be changed to M_NOWAIT or the NULL checks should be 
removed. No idea which makes more sense.

Pedro.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"