Date: Fri, 5 Apr 2019 12:28:06 -0700 (PDT) From: "Rodney W. Grimes" <freebsd-rwg@gndrsh.dnsmgr.net> To: Chris Rees <crees@bayofrum.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: <201904051928.x35JS6G4040777@gndrsh.dnsmgr.net> In-Reply-To: <120E994C-79DE-46E2-AAB7-649E4929AFE9@bayofrum.net>
next in thread | previous in thread | raw e-mail | index | archive | help
> Hello RC people, > > 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. > > Please would someone review and approve? > > 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 Thanks, Rod > Thanks! > > Chris > > 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 > >> >>>>> > > >> >>>>> > >============================================================================== > >> >>>>> > --- 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. E.g. > >we can > >> > silently ignore them. > >> > >> 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. > >> > >> > 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. > > > >> # > >> > >> 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. > >> > >> Can you think of any other behaviour I'd need to check? > >No, for me it looks good enough. > > > >-- > >This message has been scanned for viruses and > >dangerous content by MailScanner, and is > >believed to be clean. > > -- > Sent from my Android device with K-9 Mail. Please excuse my brevity. > -- > This message has been scanned for viruses and > dangerous content by MailScanner, and is > believed to be clean. > > _______________________________________________ > 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" > > -- Rod Grimes rgrimes@freebsd.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201904051928.x35JS6G4040777>