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

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Chris Rees-12
Hi again,

On 24/12/2018 11:23, Chris Rees wrote:

> Hi Konstantin,
>
> On 24 Dec 2018 11:17, Konstantin Belousov <[hidden email]> 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

Chris


--
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Konstantin Belousov
On Mon, Dec 24, 2018 at 01:07:54PM +0000, Chris Rees wrote:

> Hi again,
>
> On 24/12/2018 11:23, Chris Rees wrote:
> > Hi Konstantin,
> >
> > On 24 Dec 2018 11:17, Konstantin Belousov <[hidden email]> 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).
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Chris Rees-12
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 <[hidden email]> 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

Chris


--
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Konstantin Belousov
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 <[hidden email]> 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.

How was this tested ?
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Chris Rees-12
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 <[hidden email]> 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
#

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?

Chris

--
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Konstantin Belousov
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 <[hidden email]> 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.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Chris Rees-10
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

Thanks!

Chris

On 25 December 2018 07:41:45 GMT, Konstantin Belousov <[hidden email]> 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 <[hidden email]>
>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.

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Rodney W. Grimes-6
> 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 <[hidden email]> 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 <[hidden email]>
> >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.
>
> _______________________________________________
> [hidden email] mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-rc
> To unsubscribe, send any mail to "[hidden email]"
>
>

--
Rod Grimes                                                 [hidden email]
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Chris Rees-10
Hi Rod,

On 5 April 2019 20:28:06 BST, "Rodney W. Grimes" <[hidden email]> wrote:

>> 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
>

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

>
>> Thanks!
>>
>> Chris
>>
>> On 25 December 2018 07:41:45 GMT, Konstantin Belousov
><[hidden email]> 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
><[hidden email]>
>> >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.
>>
>> _______________________________________________
>> [hidden email] mailing list
>> https://lists.freebsd.org/mailman/listinfo/freebsd-rc
>> To unsubscribe, send any mail to "[hidden email]"
>>
>>

--
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.

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Call for testing, call for src bit approbal - /etc/rc.d kld cleanup

Rodney W. Grimes-6
In reply to this post by Chris Rees-10
> 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.
<stripped the thread, no relevant to my request>
>
> Please would someone review and approve?
>
> https://www.bayofrum.net/~crees/patches/rc-kld_list-extension-2.diff

I would like to ask for a source committer familiar with
/etc/rc* and kldload to review this patch of Chris Rees
and approve it for commit.  I can not approve it due to
mentee status or I would of.

https://reviews.freebsd.org/D18670

Regards,
Rod Grimes

> Thanks!
> Chris
--
Rod Grimes                                                 [hidden email]
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Call for testing, call for src bit approbal - /etc/rc.d kld cleanup

Chris Rees-10
Hi Rod,

On 7 April 2019 05:32:32 BST, "Rodney W. Grimes" <[hidden email]> wrote:

>> 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.
><stripped the thread, no relevant to my request>
>>
>> Please would someone review and approve?
>>
>> https://www.bayofrum.net/~crees/patches/rc-kld_list-extension-2.diff
>
>I would like to ask for a source committer familiar with
>/etc/rc* and kldload to review this patch of Chris Rees
>and approve it for commit.  I can not approve it due to
>mentee status or I would of.
>
>https://reviews.freebsd.org/D18670
>

Thanks for this, but jilles@ approved it yesterday- I'll commit today.

Chris
--
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.

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "[hidden email]"