Date: Fri, 05 Apr 2019 21:38:53 +0100 From: Chris Rees <crees@bayofrum.net> To: "Rodney W. Grimes" <freebsd-rwg@gndrsh.dnsmgr.net> Cc: Konstantin Belousov <kostikbel@gmail.com>, Chris Rees <chris@rees.space>, freebsd-rc@freebsd.org Subject: Re: svn commit: r342389 - head/share/man/man5 Message-ID: <1A8A6650-A498-465C-A3EF-F82BAC0B8F21@bayofrum.net> In-Reply-To: <201904051928.x35JS6G4040777@gndrsh.dnsmgr.net> References: <201904051928.x35JS6G4040777@gndrsh.dnsmgr.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Rod, On 5 April 2019 20:28:06 BST, "Rodney W. Grimes" <freebsd-rwg@gndrsh.dnsmgr= .net> wrote: >> Hello RC people, >>=20 >> Konstantin has kindly reviewed the patch to fix load_kld behaviour, >but would prefer that someone more familiar with RC give me approval to >commit. I haven't stripped any of the replies, so the conversation >should be fairly easy to follow, though I'm happy to link to archives >if anyone missed it. >>=20 >> Please would someone review and approve? >>=20 >> https://www.bayofrum.net/~crees/patches/rc-kld_list-extension-2.diff > >Can we please either have this up in phabricator, or have permission >to pull your diff into a phabricator review if you do not want to >do it yourself. > >We do not need the comments and exchange of emails, >just the diffs. > >http://reviews.freebsd.org > https://reviews.freebsd.org/D18670 Appears I'd forgotten it was there- it would be great to have an 'RC' group= to review in phabricator. Cheers, Chris=20 > >> Thanks! >>=20 >> Chris >>=20 >> On 25 December 2018 07:41:45 GMT, Konstantin Belousov ><kostikbel@gmail.com> wrote: >> >On Mon, Dec 24, 2018 at 06:56:40PM +0000, Chris Rees wrote: >> >> On 24/12/2018 16:50, Konstantin Belousov wrote: >> >> > On Mon, Dec 24, 2018 at 03:34:57PM +0000, Chris Rees wrote: >> >> >> On 24/12/2018 13:37, Konstantin Belousov wrote: >> >> >>> On Mon, Dec 24, 2018 at 01:07:54PM +0000, Chris Rees wrote: >> >> >>>> On 24/12/2018 11:23, Chris Rees wrote: >> >> >>>>> On 24 Dec 2018 11:17, Konstantin Belousov ><kostikbel@gmail.com> >> >wrote: >> >> >>>>> >> >> >>>>> On Mon, Dec 24, 2018 at 10:47:48AM +0000, Chris Rees >wrote: >> >> >>>>> > Author: crees (doc,ports committer) >> >> >>>>> > Date: Mon Dec 24 10:47:48 2018 >> >> >>>>> > New Revision: 342389 >> >> >>>>> > URL: https://svnweb.freebsd.org/changeset/base/342389 >> >> >>>>> > >> >> >>>>> > Log: >> >> >>>>> >?? Clarify kld_list format >> >> >>>>> >?? >> >> >>>>> >?? PR: docs/234248 >> >> >>>>> >?? Submitted by: David Fiander >> >> >>>>> >?? Submitted by: Miroslav Lachman >> >> >>>>> > >> >> >>>>> > Modified: >> >> >>>>> >?? head/share/man/man5/rc.conf.5 >> >> >>>>> > >> >> >>>>> > Modified: head/share/man/man5/rc.conf.5 >> >> >>>>> > >> >> >>>>>=20=20=20=20 >> >>=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D >> >> >>>>> > --- head/share/man/man5/rc.conf.5 Mon Dec 24 06:14:32 >> >2018 >> >> >>>>> (r342388) >> >> >>>>> > +++ head/share/man/man5/rc.conf.5 Mon Dec 24 10:47:48 >> >2018 >> >> >>>>> (r342389) >> >> >>>>> > @@ -248,12 +248,14 @@ Default >> >> >>>>> >? .Pa /etc/ddb.conf . >> >> >>>>> >? .It Va kld_list >> >> >>>>> >? .Pq Vt str >> >> >>>>> > -A list of kernel modules to load right after the >local >> >> >>>>> > -disks are mounted. >> >> >>>>> > +A whitespace-separated list of kernel modules to load >> >right after >> >> >>>>> > +the local disks are mounted, without any >> >> >>>>> > +.Pa .ko >> >> >>>>> > +extension or path. >> >> >>>>> I think both extension and path are accepted if >supplied. >> >> >>>>> It is the behaviour described in kldload(8). >> >> >>>>> >> >> >>>>> >> >> >>>>> That's true, but the kld rc script adds .ko, so providing >the >> >> >>>>> extension will probably break, and it checks for existing >> >modules >> >> >>>>> using the provided name as a regex, so that will also fail. >> >> >>>>> >> >> >>>>> I don't think that'd be hard to fix though, so I'll fix that >> >and put a >> >> >>>>> patch up for review later. >> >> >>>> Having looked again, rc.subr uses kldstat -v, so the path is >> >indeed not >> >> >>>> a problem, but the extension is-- removing any extension from >> >_kld will >> >> >>>> ensure that it will always match correctly.? At the moment it >is >> >> >>>> fragile, because it will load correctly the first time but >hit >> >an error >> >> >>>> if the user has put the extension in and the module is >already >> >loaded. >> >> >>>> >> >> >>>> @RC people, does this look acceptable (I'll need approval >> >please)? >> >> >>>> >> >> >>>> >> >https://www.bayofrum.net/~crees/patches/rc-kld_list-extension.diff >> >> >>> I do not quite see a point in the check for the module >presence. >> >> >>> Kernel already rejects already loaded modules (by module >name). >> >> >> True; this code predates the -n option to kldload.? Using that >> >makes the >> >> >> whole checking unnecessary. >> >> >> >> >> >> How about this one? >> >> >> >> >> >> >> >https://www.bayofrum.net/~crees/patches/rc-kld_list-extension-2.diff >> >> > It looks reasonable to me. I am not sure if we want to keep the >> >options >> >> > for load_kld for benefit of the third-party scripts, or not.=20 >E.g. >> >we can >> >> > silently ignore them. >> >>=20 >> >> Yeah, my patch ignores them silently.? It has the added bonus of >not >> >> needing to sweep the ports tree, with all the version issues that >> >> entails as the behaviour has slightly changed if the options are >> >> necessary at that point. >> >>=20 >> >> > How was this tested ? >> >> [crees@pegasus]~/workspace/src/head% sudo sh >> >> # . libexec/rc/rc.subr >> >> # kldstat |grep cuse >> >> # load_kld cuse4bsd >> >> # kldstat |grep cuse >> >> 15??? 1 0xffffffff818c3000 40a0???? cuse.ko >> >> # load_kld cuse4bsd >> >> # load_kld doesntexist >> >> kldload: can't load doesntexist: No such file or directory >> >> sh: WARNING: Unable to load kernel module doesntexist >> >> # kldunload cuse >> >> # load_kld -m nothing -e noop /boot/modules/cuse4bsd.ko >> >> # kldstat |grep cuse >> >> 15??? 1 0xffffffff818c3000 4c80???? cuse4bsd.ko >> >> # load_kld -m nothing -e noop /boot/modules/cuse4bsd.ko >> >> # load_kld -m nothing -e noop /boot/modules/cuse4bsd.ko >> >> # load_kld -m nothing -e noop /boot/modules/cuse4bsd.ko >> >> # kldstat |grep cuse >> >> 15??? 1 0xffffffff818c3000 4c80???? cuse4bsd.ko >> >I suppose escapes are something that your mail agent inserted and >not >> >the >> >actual system output. >> > >> >The script looks fine to me, but I am not the right person to stamp >the >> >approval on the rc changes. >> > >> >> # >> >>=20 >> >> It's rather a curiosity for me that cuse4bsd only loads as itself >if >> >> called by path, but it doesn't happen with any other modules-- >this >> >was >> >> just to prove that full paths and extensions work correctly as >> >> intended.? My machine also boots fine. >> >>=20 >> >> Can you think of any other behaviour I'd need to check? >> >No, for me it looks good enough. >> > >> >--=20 >> >This message has been scanned for viruses and >> >dangerous content by MailScanner, and is >> >believed to be clean. >>=20 >> --=20 >> Sent from my Android device with K-9 Mail. Please excuse my brevity. >> --=20 >> This message has been scanned for viruses and >> dangerous content by MailScanner, and is >> believed to be clean. >>=20 >> _______________________________________________ >> freebsd-rc@freebsd.org mailing list >> https://lists.freebsd.org/mailman/listinfo/freebsd-rc >> To unsubscribe, send any mail to "freebsd-rc-unsubscribe@freebsd.org" >>=20 >>=20 --=20 Sent from my Android device with K-9 Mail. Please excuse my brevity. --=20 This message has been scanned for viruses and dangerous content by MailScanner, and is believed to be clean.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1A8A6650-A498-465C-A3EF-F82BAC0B8F21>