Date: Fri, 05 Apr 2019 19:37:16 +0100 From: Chris Rees <crees@bayofrum.net> To: Konstantin Belousov <kostikbel@gmail.com>, Chris Rees <chris@rees.space> Cc: freebsd-rc@freebsd.org Subject: Re: svn commit: r342389 - head/share/man/man5 Message-ID: <120E994C-79DE-46E2-AAB7-649E4929AFE9@bayofrum.net> In-Reply-To: <20181225074145.GA60291@kib.kiev.ua> References: <9f786428-7fea-4fa4-a29e-ed91997a87fd@email.android.com> <dd115035-34c1-b73a-1ea5-f108407bda8d@rees.space> <20181224133721.GW60291@kib.kiev.ua> <eb8b7db2-d8be-3081-8f76-0291d1fbe3d7@rees.space> <20181224165023.GY60291@kib.kiev.ua> <f7e41992-430a-4e06-9398-7d341353c796@rees.space> <20181225074145.GA60291@kib.kiev.ua>
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 wou= ld 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 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: >> >>>>> >=C2=A0=C2=A0 Clarify kld_list format >> >>>>> >=C2=A0=C2=A0 >> >>>>> >=C2=A0=C2=A0 PR: docs/234248 >> >>>>> >=C2=A0=C2=A0 Submitted by: David Fiander >> >>>>> >=C2=A0=C2=A0 Submitted by: Miroslav Lachman >> >>>>> > >> >>>>> > Modified: >> >>>>> >=C2=A0=C2=A0 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 >> >>>>> >=C2=A0 .Pa /etc/ddb.conf . >> >>>>> >=C2=A0 .It Va kld_list >> >>>>> >=C2=A0 .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.=C2=A0 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.=C2=A0 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. >>=20 >> Yeah, my patch ignores them silently.=C2=A0 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=C2=A0=C2=A0=C2=A0 1 0xffffffff818c3000 40a0=C2=A0=C2=A0=C2=A0=C2=A0 c= use.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=C2=A0=C2=A0=C2=A0 1 0xffffffff818c3000 4c80=C2=A0=C2=A0=C2=A0=C2=A0 c= use4bsd.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=C2=A0=C2=A0=C2=A0 1 0xffffffff818c3000 4c80=C2=A0=C2=A0=C2=A0=C2=A0 c= use4bsd.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.=C2=A0 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 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?120E994C-79DE-46E2-AAB7-649E4929AFE9>