Page MenuHomeFreeBSD

ldconfig: Remove obsolete -v option and update man-page
ClosedPublic

Authored by se on Feb 28 2024, 11:39 AM.
Tags
None
Referenced Files
F102815945: D44139.diff
Sun, Nov 17, 1:39 PM
Unknown Object (File)
Sat, Nov 2, 9:05 PM
Unknown Object (File)
Tue, Oct 22, 4:13 PM
Unknown Object (File)
Tue, Oct 22, 1:52 AM
Unknown Object (File)
Mon, Oct 21, 9:19 AM
Unknown Object (File)
Oct 2 2024, 11:32 AM
Unknown Object (File)
Oct 1 2024, 9:43 PM
Unknown Object (File)
Sep 27 2024, 2:21 AM
Subscribers

Details

Summary

The -v option has been ignored for a very long time (did only provide information relevant for operations on a.out libraries).
The local variable "verbose" is only set and never read and not made available to any function in ldconfig, it can therefore be removed.
To support the unlikely case that the variable is passed in some script, it can be accepted and ignored (as it already effectively was).
The man-page is updated to no longer mention -v as a valid option.
There are 3 ignored options: "-elf", "-s", and "-v" that are ignored but accepted for historical reason: list these variables at the end of the HISTORY section.
(While here fix the cross-reference to the ld-elf.so.1 man-page by adding the missing section "1".)

Test Plan

Apply patch and verify that ldconfig operates as before.
Verify changes to the man-page result in useful and correct information.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

se requested review of this revision.Feb 28 2024, 11:39 AM

Fix cross-reference to the ld-elf.so.1 man-page - there was no blank between the section number and the comma.

sbin/ldconfig/ldconfig.8
44

Why not just remove?

se marked an inline comment as done.Feb 29 2024, 9:11 AM

My main question regarding this review is whether the comment regarding ignored options should be in the HISTORY section or rather moved to the end of the options section.

I'll update the diff to be based on the actual ldconfig sources after commit of D44093.
And I'll move the text to the end of the DESCRIPTION section, behind the list of options, since I think that it is not of sufficient relevance for the HISTORY section.

sbin/ldconfig/ldconfig.8
44

This was a just a reminder for me that the -elf option was also accepted and ignored - but that it still is in the usage() output.
With the comment added to the HISTORY section this commented outline should be deleted.

161

I have already committed this fix in the previous update that added the -B option.

sbin/ldconfig/ldconfig.c
127

A -B option has been added in a previous commit.

se marked an inline comment as done.
se added a reviewer: imp.

While reading the FILES section I noticed that there was some outdated/wrong information:

  • /var/run/ld-elf32.so.hints is a binary hints file, not a configuration file like /etc/ld-elf.so.conf
  • /etc/ld-elf.so.conf does not exist by default, anymore
  • Files in /usr/local/libdata/ldconfig/ are actually used to initialize the standard hints file when rc.d/ldconfig is executed

The information about ignored options has been moved to the end of the DESCRIPTION section.

kib added inline comments.
sbin/ldconfig/ldconfig.8
161

There is some $LOCALPREFIX knob or such, and user.localbase sysctl, affecting the paths. I believe you was involved with it?

This revision is now accepted and ready to land.Mar 1 2024, 4:23 AM
sbin/ldconfig/ldconfig.8
161

Yes, I was involved in the implementation of the getlocalbase() function, which uses these parameters, and I have implemented the "user.localbase" sysctl variable for this purpose in commit 147eea393fdaf.

Do you want to change how this path is referenced in the man-page and/or a change of rc.d/ldconfig to adapt to a non-standard $LOCALBASE?

This revision was automatically updated to reflect the committed changes.
sbin/ldconfig/ldconfig.8
161

I think that default rc.conf should indeed source the usr.localbase value for _localbase instead of hardcoding /usr/local, and man page for ldconfig should provide some reference to that.

sbin/ldconfig/ldconfig.8
161

See line 58 of /etc/rc on -CURRENT:

_localbase=`/sbin/sysctl -n user.localbase 2> /dev/null`

The assignment of /usr/local in defaults/rc.conf is only a fall-back in case the sysctl variable has not been set.

# Set default value of _localbase if not previously set
: ${_localbase:="/usr/local"}
sbin/ldconfig/ldconfig.8
161

Great. Might be add some hint in rc.conf?

sbin/ldconfig/ldconfig.8
161

I'm not sure about where to put additional information regarding a non-default $LOCALBASE.

It is of course possible to add another line of comments to defaults/rc.conf that explains what "not previously set" means.

Some explanatory text could also be added to the rc.conf man-page.

But there are a lot of other places that refer /usr/local by literally writing this path out (instead of referring to it by e.g. $LOCALPATH), which could be checked whether they cross reference with getlocalbase() or some other man-page that hints at the possibility to build a system with a non-default $LOCALBASE.

But there had been opposition to making users aware of the possibility to change $LOCALBASE on one of my earlier commits in this area (introduction oft the user.localbase sysctl, IIRC).

The reasoning was that this would make it seem a good idea to change this path, while a lot of sources, documentation, and config files hard-code /usr/local (and have to be identified and changed).

My suggestion to better document this specifically for rc.conf:

  1. Add some more text to the setting of _localbase if undefined.
  2. Add a short section to the rc.conf man-page that explains the automatic adjustment that is performed if user.localbase is set to a non-default value (e.g. in /boot/loader.conf).

(BTW: There was an issue that prevented setting of user.localbase in loader.conf, that could lead to a panic - I'm not sure that I have made this a writable tunable after I found out about the possible panic.)

sbin/ldconfig/ldconfig.8
161

My suggestion to better document this specifically for rc.conf:

  1. Add some more text to the setting of _localbase if undefined.

My confusion was due to not knowing that _locabase was already set in /etc/rc. I read the statement in rc.conf as initialization, and not as a fallback.

  1. Add a short section to the rc.conf man-page that explains the automatic adjustment that is performed if user.localbase is set to a non-default value (e.g. in /boot/loader.conf).
sbin/ldconfig/ldconfig.8
161

There are too many programs in the base system that do only support the LOCALBASE directory compiled in during "make buildworld" (e.g. cron, which compiles in that base directory as:
#define LOCALSYSCRONTABS _PATH_LOCALBASE "/etc/cron.d".)
There was some opposition to adding the _localbase variable to defaults/rc.conf, since it could lead to the wrong idea that changing that value was sufficient to make the system use a different LOCALBASE at run-time.

I had converted a number of utilities to use getlocalbase() instead of a compiled in localbase directory, but many more still use the compiled-in value only.

I'll change the comment to better describe the feature and its limitations.
The fall-back was meant to provide a default for systems that don't provide a value in user.localbase - it could be removed, but that would further confiscate the fact where _localbase used in this file originates from.