tech@mandoc.bsd.lv
 help / color / mirror / Atom feed
* [PATCH makewhatis] add NIXSTOREDIR configuration variable to support NixOS
@ 2021-03-30 19:49 sternenseemann
  2021-03-30 20:30 ` Ingo Schwarze
  0 siblings, 1 reply; 11+ messages in thread
From: sternenseemann @ 2021-03-30 19:49 UTC (permalink / raw)
  To: tech

On NixOS there is a similar situation as with Hombrew: The actual man
pages are installed into custom prefixes for each package under
/nix/store and then symlinked to in the global share/man directory
(which is /run/current-system/sw/share/man for NixOS). This of course
makes makewhatis skip every installed man page when indexing because
/nix/store is considered outside of the man directory.

To fix this issue I've followed the implementation of HOMEBREWDIR. The
drawback of this being that more or less the same code is duplicated for
this purpose. However there may be a (arguably very small) group of
users who would possibly want to use both configuration variables at the
same time as the nix package manager can also be installed on macOS, but
it would probably also not do any harm merging the two variables into one.

This patch is necessary to provide a mandoc-based alternative to the
current documentation.man module in NixOS which uses GNU's man-db
package. I have a working implementation of this (using a cvs version of
mandoc with this patch applied) here:
https://github.com/openlab-aux/vuizvui/blob/master/modules/user/sternenseemann/documentation/mandoc.nix

A side note: During testing I haven't been able to trigger the code path
of the ifdef in filescan. I've added it anyways because HOMEBREWDIR also
has been added there, but didn't seem to be a situation where basedir is
set and filescan is called, but maybe I missed something.

cheers,
lukas


Index: configure
===================================================================
RCS file: /cvs/mandoc/configure,v
retrieving revision 1.77
diff -u -r1.77 configure
--- configure   20 Jul 2020 16:57:30 -0000      1.77
+++ configure   8 Feb 2021 17:51:07 -0000
@@ -108,6 +108,7 @@
 LIBDIR=o				
 MANDIR=
 HOMEBREWDIR=
+NIXSTOREDIR=

 WWWPREFIX="/var/www"
 HTDOCDIR=
@@ -462,6 +463,7 @@
 [ -n "${OSNAME}" ] && echo "#define OSNAME \"${OSNAME}\""
 [ -n "${UTF8_LOCALE}" ] && echo "#define UTF8_LOCALE \"${UTF8_LOCALE}\""
 [ -n "${HOMEBREWDIR}" ] && echo "#define HOMEBREWDIR \"${HOMEBREWDIR}\""
+[ -n "${NIXSTOREDIR}" ] && echo "#define NIXSTOREDIR \"${NIXSTOREDIR}\""
 [ ${HAVE_ATTRIBUTE} -eq 0 ] && echo "#define __attribute__(x)"
 [ ${HAVE_EFTYPE} -eq 0 ] && echo "#define EFTYPE EINVAL"
 [ ${HAVE_O_DIRECTORY} -eq 0 ] && echo "#define O_DIRECTORY 0"
Index: configure.local.example
===================================================================
RCS file: /cvs/mandoc/configure.local.example,v
retrieving revision 1.39
diff -u -r1.39 configure.local.example
--- configure.local.example     20 Jul 2020 16:57:30 -0000      1.39
+++ configure.local.example     8 Feb 2021 17:51:07 -0000
@@ -218,6 +218,16 @@
 PREFIX="/usr/local"
 HOMEBREWDIR="${PREFIX}/Cellar"

+# Similarly, when using the nix package manager or the NixOS linux
+# distribution, man pages are stored in the packages' output store
+# path in /nix/store.  The globally installed man pages under
+# /run/current-system/sw/share/man are thus symlinks to files under
+# /nix/store.  By setting NIXSTOREDIR, mandoc won't ignore these
+# symlinks.
+# This setting can also be used to support Guix which is derived
+# from NixOS, but uses /gnu/store instead of /nix/store.
+NIXSTOREDIR="/nix/store"
+
 # --- user settings for the mandoc(3) library --------------------------

 # By default, libmandoc.a is not installed.  It is almost never needed
Index: mandocdb.c
===================================================================
RCS file: /cvs/mandoc/mandocdb.c,v
retrieving revision 1.267
diff -u -r1.267 mandocdb.c
--- mandocdb.c  3 Apr 2020 11:35:01 -0000       1.267
+++ mandocdb.c  8 Feb 2021 17:51:08 -0000
@@ -615,6 +615,9 @@
 #ifdef HOMEBREWDIR
                            && strncmp(buf, HOMEBREWDIR,
strlen(HOMEBREWDIR))
 #endif
+#ifdef NIXSTOREDIR
+                           && strncmp(buf, NIXSTOREDIR,
strlen(NIXSTOREDIR))
+#endif
                        ) {
                                if (warnings) say("",
                                    "%s: outside base directory", buf);
@@ -825,6 +828,10 @@
                start = usefile + basedir_len;
 #ifdef HOMEBREWDIR
        else if (strncmp(usefile, HOMEBREWDIR, strlen(HOMEBREWDIR)) == 0)
+               start = usefile;
+#endif
+#ifdef NIXSTOREDIR
+       else if (strncmp(usefile, NIXSTOREDIR, strlen(NIXSTOREDIR)) == 0)
                start = usefile;
 #endif
        else {
--
 To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH makewhatis] add NIXSTOREDIR configuration variable to support NixOS
  2021-03-30 19:49 [PATCH makewhatis] add NIXSTOREDIR configuration variable to support NixOS sternenseemann
@ 2021-03-30 20:30 ` Ingo Schwarze
  2021-03-30 21:03   ` sternenseemann
  0 siblings, 1 reply; 11+ messages in thread
From: Ingo Schwarze @ 2021-03-30 20:30 UTC (permalink / raw)
  To: sternenseemann; +Cc: tech

Hi Lukas,

Earendil wrote on Tue, Mar 30, 2021 at 09:49:49PM +0200:

> On NixOS there is a similar situation as with Hombrew: The actual
> man pages are installed into custom prefixes for each package under
> /nix/store and then symlinked to in the global share/man directory
> (which is /run/current-system/sw/share/man for NixOS). This of course
> makes makewhatis skip every installed man page when indexing because
> /nix/store is considered outside of the man directory.

Uh oh.  I must admit i found this "cellar" concept so weird and unusual
that i didn't anticipate any other system doing anything similar, and
hence the overly specific naming of the feature.  Then again, if i
read the commit logs correctly, it did take six years until the second
system (among about two dozen being actively maintained) turned up that
uses such a method...

> To fix this issue I've followed the implementation of HOMEBREWDIR. The
> drawback of this being that more or less the same code is duplicated for
> this purpose. However there may be a (arguably very small) group of
> users who would possibly want to use both configuration variables at the
> same time as the nix package manager can also be installed on macOS, but
> it would probably also not do any harm merging the two variables into one.

Indeed.  Using two package managers on the same machine at the same
time is no doubt a recipe for disaster, and i expect getting the
manual pages indexed will be the least of your worries...

So i'm not really convinced having *two* such variables with identical
functionality makes anything better.

Would simply setting HOMEBREWDIR=/nix/store be OK with you?

We could of course rename the variable to something more generic,
*if* a generic name for this concepts exists or can be invented
in a reasonable way.

> This patch is necessary to provide a mandoc-based alternative to the
> current documentation.man module in NixOS which uses GNU's man-db
> package. I have a working implementation of this (using a cvs version of
> mandoc with this patch applied) here:

Nice.   :-)

> https://github.com/openlab-aux/vuizvui/blob/master/modules/user/sternenseemann/documentation/mandoc.nix

To speed up the reply - i caused enough delay already - i refrained from
studying your code so far.

> A side note: During testing I haven't been able to trigger the code path
> of the ifdef in filescan. I've added it anyways because HOMEBREWDIR also
> has been added there, but didn't seem to be a situation where basedir is
> set and filescan is called, but maybe I missed something.

To reach the filescan() function, you need to call makewhatis with
the -d, -u, or -t option.

Unless i misunderstand my own code (after it required little maintenance
effort for a number of years), using -d or -u will indeed frst call
set_basedir() and then filescan() shortly afterwards, see the
function mandocdb() in the file mandocdb.c.

Yours,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH makewhatis] add NIXSTOREDIR configuration variable to support NixOS
  2021-03-30 20:30 ` Ingo Schwarze
@ 2021-03-30 21:03   ` sternenseemann
  2021-03-31 17:34     ` Ingo Schwarze
  0 siblings, 1 reply; 11+ messages in thread
From: sternenseemann @ 2021-03-30 21:03 UTC (permalink / raw)
  To: tech

Hi Ingo,

On 3/30/21 10:30 PM, Ingo Schwarze wrote:
> Indeed.  Using two package managers on the same machine at the same
> time is no doubt a recipe for disaster, and i expect getting the
> manual pages indexed will be the least of your worries...

I'm pretty sure you can use both at the same time since nix wouldn't try
to write anything under /usr in that case, but even if you were using
both at the same time, makewhatis wouldn't be useful for nix, I think
since it heavily operates on a concept of a man dir it can write to.
With NixOS there is enough flexibility to integrate that, but on nix on
another distribution / OS that doesn't work as far as I'm aware.

TL;DR: I don't think this is an use case we need to support.

> So i'm not really convinced having *two* such variables with identical
> functionality makes anything better.
> 
> Would simply setting HOMEBREWDIR=/nix/store be OK with you?

Yes, absolutely! I mainly submitted the patch to get some feedback to
upstream since we would of course like to use mandoc's configuration
options in a way intended by upstream ideally :) If setting HOMEBREWDIR
sounds reasonable for you, I'm absolutely fine with it. We also don't
currently set it for macOS in our package, so this wouldn't cause any
regression.
> We could of course rename the variable to something more generic,
> *if* a generic name for this concepts exists or can be invented
> in a reasonable way.

That would of course be nice. Mayeb something along the lines of
EXTRA_BASEDIR or SYMLINK_TARGET_EXCEPTION? Seems hard to name in a
concise way though.

> To speed up the reply - i caused enough delay already - i refrained from
> studying your code so far.

It's no matter and nothing really too interesting happens there, just
wanted to link it for reference.

> To reach the filescan() function, you need to call makewhatis with
> the -d, -u, or -t option.
> 
> Unless i misunderstand my own code (after it required little maintenance
> effort for a number of years), using -d or -u will indeed frst call
> set_basedir() and then filescan() shortly afterwards, see the
> function mandocdb() in the file mandocdb.c.

Indeed, you still seem to understand it better than me.

cheers,

lukas
--
 To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH makewhatis] add NIXSTOREDIR configuration variable to support NixOS
  2021-03-30 21:03   ` sternenseemann
@ 2021-03-31 17:34     ` Ingo Schwarze
  2021-03-31 19:33       ` sternenseemann
  2021-04-10 15:57       ` [PATCH v2 makewhatis] refactor HOMEBREWDIR into READ_ALLOWED_PATH sternenseemann
  0 siblings, 2 replies; 11+ messages in thread
From: Ingo Schwarze @ 2021-03-31 17:34 UTC (permalink / raw)
  To: sternenseemann; +Cc: tech

Hi Lukas,

Earendil wrote on Tue, Mar 30, 2021 at 11:03:53PM +0200:
> On 3/30/21 10:30 PM, Ingo Schwarze wrote:

>> We could of course rename the variable to something more generic,
>> *if* a generic name for this concepts exists or can be invented
>> in a reasonable way.

> That would of course be nice. Mayeb something along the lines of
> EXTRA_BASEDIR

I don't like that.  The word "basedir" is jargon only used in the source
code in the file mandocdb.c (admittedly, it also appears in an error
message there, so the user may see it, which is probably not good).
The documentation calls more or less the same thing "manpath", see
the manual page man.conf(5), and the related environment variable
MANPATH, see the manual pages man(1) and apropos(1).

But the configuration variable we are trying to name does not point
to a manpath, so its name should not contain the words "manpath"
or "basedir".  A manpath contains man[1-9] and optionally cat[1-9]
subdirecties and nothing else, which is not the case for a homebrew
cellar nor for /nix/store/.  Yes, there may be some manual pages
inside /nix/store/, among many other things, but not in a manpath
structure.

> or SYMLINK_TARGET_EXCEPTION?

A bit better, yes.  But even though already long, it doesn't say
which rule is being flouted, nor what the exception is, and that
symlinks are involved is, in my opinion, not the most relevant part
of this makewhatis(8) feature, even though symlinks certainly are
relevant for the way it is used in Homebrew and NixOS.

> Seems hard to name in a concise way though.

I had one other idea while considering the naming.
Why not allow

  FOO_PATH="/Homebrew/Cellar:/nix/store"

and parse it like so (untested):

	const char	*pb;
	size_t		 len;

	*pb = FOO_PATH;
	while (*pb != '\0') {
		len = strcspn(pb, ":");
		if (strncmp(buf, pb, len) == 0)
			break;
		pb += len;
		pb += strspn(pb, ":");
	}
	if (*pb == '\0' && strncmp(buf, basedir, basedir_len) != 0) {
		say(...);
		continue;
	}

Now, how should that variable be called?

 * The name should end with "_PATH".
 * The makewhatis(8) program is allowed to read any files below
   this path, so the name should include "READ" and some word
   indicating permission.
 * Mentioning makewhatis in the name does not seem important.
   It is more or less clear anyway that man(1) itself can read
   anything given on the command line or found in the database,
   subject to file system permissions, of course.

So, maybe:

  READ_ALLOWED_PATH

What do you think?

> It's no matter and nothing really too interesting happens there, just
> wanted to link it for reference.

That's good.  I should also update the NixOS entries on

  https://mandoc.bsd.lv/ports.html

but maybe wait for the dust to settle first.

Yours,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH makewhatis] add NIXSTOREDIR configuration variable to support NixOS
  2021-03-31 17:34     ` Ingo Schwarze
@ 2021-03-31 19:33       ` sternenseemann
  2021-04-10 15:57       ` [PATCH v2 makewhatis] refactor HOMEBREWDIR into READ_ALLOWED_PATH sternenseemann
  1 sibling, 0 replies; 11+ messages in thread
From: sternenseemann @ 2021-03-31 19:33 UTC (permalink / raw)
  To: tech

Hi Ingo,

On 3/31/21 7:34 PM, Ingo Schwarze wrote:
> I had one other idea while considering the naming.
> Why not allow
> 
>   FOO_PATH="/Homebrew/Cellar:/nix/store"
> 
> and parse it like so (untested):
> 
> 	const char	*pb;
> 	size_t		 len;
> 
> 	*pb = FOO_PATH;
> 	while (*pb != '\0') {
> 		len = strcspn(pb, ":");
> 		if (strncmp(buf, pb, len) == 0)
> 			break;
> 		pb += len;
> 		pb += strspn(pb, ":");
> 	}
> 	if (*pb == '\0' && strncmp(buf, basedir, basedir_len) != 0) {
> 		say(...);
> 		continue;
> 	}
I really like that idea! I initially considered something like this, but
shied away from it as it requires you to make some implementation choice
and having proper arrays in preprocessor macros is never nice.

It adds some extra overhead, I guess, but in comparison to all the IO
makewhatis has to do this should be practically nothing.

> Now, how should that variable be called?
> 
>  * The name should end with "_PATH".
>  * The makewhatis(8) program is allowed to read any files below
>    this path, so the name should include "READ" and some word
>    indicating permission.
>  * Mentioning makewhatis in the name does not seem important.
>    It is more or less clear anyway that man(1) itself can read
>    anything given on the command line or found in the database,
>    subject to file system permissions, of course.
> 
> So, maybe:
> 
>   READ_ALLOWED_PATH
> 
> What do you think?

Seems concise and expressive enough. However I feel like it may hinder
discoverability of this option if it doesn't refer to makewhatis at all.

> That's good.  I should also update the NixOS entries on
> 
>   https://mandoc.bsd.lv/ports.html
> 
> but maybe wait for the dust to settle first.

Sounds like a good idea! If everything goes according to (my) plan, it
should be possible to configure mandoc as default viewer and searcher
just as easily as man-db. I can offer to report back at some point when
the dust has settled, so you don't have to dig through unfamiliar
documentation etc.

Cheers,

Lukas

--
 To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 makewhatis] refactor HOMEBREWDIR into READ_ALLOWED_PATH
  2021-03-31 17:34     ` Ingo Schwarze
  2021-03-31 19:33       ` sternenseemann
@ 2021-04-10 15:57       ` sternenseemann
  2021-04-10 16:15         ` sternenseemann
  1 sibling, 1 reply; 11+ messages in thread
From: sternenseemann @ 2021-04-10 15:57 UTC (permalink / raw)
  To: tech

Hi Ingo,

I went ahead and implemented your suggestion of READ_ALLOWED_PATH. It is
mostly your code with the const char * assignment fixed and a tweak to
the parser, so it doesn't allow every realpath if READ_ALLOWED_PATH has
a leading or trailing ':'.

The previous checks on basedir and HOMEBREWDIR if defined have been
refactored into a function called read_allowed which also tracks the
length of the allowed dir which allows us to at least strip the prefix
from READ_ALLOWED_PATHS in filescan (this wasn't done previously for
HOMEBREWDIR).

HOMEBREWDIR is still supported as a configuration option, but we just
append it to READ_ALLOWED_PATHS in configure. The variable expansion
shenanigans in there are not strictly necessary as the parser would also
support leading colons, but since it works in ksh as well, there seems
to be no harm in having a clean READ_ALLOWED_PATH.

To indicate deprecation, I've removed HOMEBREWDIR from
configure.local.example.

I tested the following cases:

* "/nix/store"
* ":/nix/store"
* "/nix/store:"
* "/nix/store:/usr/local/Cellar"
* "/nix/store:/usr/local/Cellar:"
* ":/nix/store:/usr/local/Cellar"
* "/nix/store:/usr/local/Cellar:/gnu/store"

with a few example paths. Also I've rebuild my system with the new patch
and it works as expected. Did some sanity checks with makewhatis -n as well.

Cheers,

Lukas

PS: Still not sure about the naming, but I don't really care.

Index: configure
===================================================================
RCS file: /cvs/mandoc/configure,v
retrieving revision 1.77
diff -r1.77 configure
464c464,469
< [ -n "${HOMEBREWDIR}" ] && echo "#define HOMEBREWDIR \"${HOMEBREWDIR}\""
---
> if [ -n "${HOMEBREWDIR}" ]; then
>   # support deprecated configuration variable HOMEBREWDIR
>   # by appending it to READ_ALLOWED_PATH
>
READ_ALLOWED_PATH="${READ_ALLOWED_PATH:+$READ_ALLOWED_PATH:}${HOMEBREWDIR}"
> fi
> [ -n "${READ_ALLOWED_PATH}" ] && echo "#define READ_ALLOWED_PATH
\"${READ_ALLOWED_PATH}\""
Index: configure.local.example
===================================================================
RCS file: /cvs/mandoc/configure.local.example,v
retrieving revision 1.39
diff -r1.39 configure.local.example
214,216c214,220
< # into the manual trees.  To allow mandoc to follow such symlinks,
< # you have to specify the physical location of the cellar as returned
< # by realpath(3), for example:
---
> # into the manual trees.  A similar situation arises on Linux
> # distribution such as NixOS and Guix where all man pages are in a
> # so-called “store” directory which are then symlinked into the man
> # basedir. To allow mandoc to follow such symlinks, you have to specify
> # the physical location of the cellar / store directory as returned by
> # realpath(3) like in the following example. You can specify multiple
> # locations by separating them with colons.
219c223
< HOMEBREWDIR="${PREFIX}/Cellar"
---
> READ_ALLOWED_PATH="/nix/store:/gnu/store:${PREFIX}/Cellar"
Index: mandocdb.c
===================================================================
RCS file: /cvs/mandoc/mandocdb.c,v
retrieving revision 1.267
diff -r1.267 mandocdb.c
167a168
> static	ssize_t	 read_allowed(char *);
614,618c615
< 			if (strncmp(buf, basedir, basedir_len) != 0
< #ifdef HOMEBREWDIR
< 			    && strncmp(buf, HOMEBREWDIR, strlen(HOMEBREWDIR))
< #endif
< 			) {
---
> 			if (read_allowed(buf) == -1) {
788a786
> 	ssize_t		 prefix_len;
824,829c822,823
< 	else if (strncmp(usefile, basedir, basedir_len) == 0)
< 		start = usefile + basedir_len;
< #ifdef HOMEBREWDIR
< 	else if (strncmp(usefile, HOMEBREWDIR, strlen(HOMEBREWDIR)) == 0)
< 		start = usefile;
< #endif
---
> 	else if ((prefix_len = read_allowed(usefile)) != -1)
> 		start = usefile + prefix_len;
1947a1942,1980
> }
>
> /*
>  * Checks if we may read from a given realpath when
>  * constructing a database. This checks if the given
>  * path is in the current set basedir or any directory
>  * in READ_ALLOWED_PATH if it is defined.
>  *
>  * Returns -1 if reading is not allowed, the length
>  * of the allowed directory part of the realpath if
>  * reading is allowed. Note that stripping a prefix of
>  * this length is only guaranteed to be a man dir if
>  * the file is in basedir.
>  */
> static ssize_t
> read_allowed(char *realpath)
> {
> 	// if we have no basedir, don't check
> 	if(basedir_len == 0 || basedir == NULL || *basedir == '\0')
> 		return basedir_len;
>
> 	if(strncmp(realpath, basedir, basedir_len) == 0)
> 		return basedir_len;
>
> #ifdef READ_ALLOWED_PATH
> 	const char *pb = READ_ALLOWED_PATH;
>
> 	while (*pb != '\0') {
> 		size_t len = strcspn(pb, ":");
>
> 		if (len > 0 && strncmp(realpath, pb, len) == 0)
> 			return len;
>
> 		pb += len;
> 		pb += strspn(pb, ":");
> 	}
> #endif
>
> 	return -1;

--
 To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 makewhatis] refactor HOMEBREWDIR into READ_ALLOWED_PATH
  2021-04-10 15:57       ` [PATCH v2 makewhatis] refactor HOMEBREWDIR into READ_ALLOWED_PATH sternenseemann
@ 2021-04-10 16:15         ` sternenseemann
  2021-08-03 16:42           ` sternenseemann
  0 siblings, 1 reply; 11+ messages in thread
From: sternenseemann @ 2021-04-10 16:15 UTC (permalink / raw)
  To: tech



On 4/10/21 5:57 PM, sternenseemann wrote:
> The previous checks on basedir and HOMEBREWDIR if defined have been
> refactored into a function called read_allowed which also tracks the
> length of the allowed dir which allows us to at least strip the prefix
> from READ_ALLOWED_PATHS in filescan (this wasn't done previously for
> HOMEBREWDIR).

Hm, this is kinda unnecessary, is it? start will be set again for all
files which are symlinks, so this value will never get used right?
--
 To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 makewhatis] refactor HOMEBREWDIR into READ_ALLOWED_PATH
  2021-04-10 16:15         ` sternenseemann
@ 2021-08-03 16:42           ` sternenseemann
  2021-08-05 12:35             ` Ingo Schwarze
  0 siblings, 1 reply; 11+ messages in thread
From: sternenseemann @ 2021-08-03 16:42 UTC (permalink / raw)
  To: tech

Hi Ingo,

just wanted to bump this thread since it is still important to me. I'd
be happy to make any adjustments as well as implement review
suggestions. So if you have time for a review, that'd be great!

On 4/10/21 6:15 PM, sternenseemann wrote:
> Hm, this is kinda unnecessary, is it? start will be set again for all
> files which are symlinks, so this value will never get used right?

Also could you confirm this for me? If true, I think I can make the
patch a bit simpler by making read_allowed just return 0 or 1.

Cheers,

Lukas
--
 To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 makewhatis] refactor HOMEBREWDIR into READ_ALLOWED_PATH
  2021-08-03 16:42           ` sternenseemann
@ 2021-08-05 12:35             ` Ingo Schwarze
  2021-08-07  0:29               ` sternenseemann
  0 siblings, 1 reply; 11+ messages in thread
From: Ingo Schwarze @ 2021-08-05 12:35 UTC (permalink / raw)
  To: sternenseemann; +Cc: tech

Hi Lukas,

sternenseemann wrote on Tue, Aug 03, 2021 at 06:42:13PM +0200:

> just wanted to bump this thread since it is still important to me.

Thats reasonable, thank you, and sorry for forgetting about it.

> I'd be happy to make any adjustments as well as implement review
> suggestions. So if you have time for a review, that'd be great!

Since i desired several minor tweaks that you could hardly anticipate,
i decided to just do them myself, starting from your ideas and patch.

On 4/10/21 6:15 PM, sternenseemann wrote:

> Hm, this is kinda unnecessary, is it? start will be set again for all
> files which are symlinks, so this value will never get used right?

Yes, i think so.  Besides, the start pointer wasn't advanced in the
past with HOMEBREWDIR set, so i don't quite see why the present
change should suddenly make it necessary.  Even if the pointer were
used for something, stripping a directory that is not the basedir
would seem wrong for any purpose because it would lose information.

Other tweaks:
 * No need for backward compatibility.  I expect the number of people
   to be affected by the change to be exactly one, the mandoc
   maintainer for the Homebrew package manager.  They can trivially
   adjust the variable name in their configure.local when updating
   the port.
 * Let's not forget to initialize READ_ALLOWED_PATH in ./configure.
 * Let's keep lines below 80 characters.
 * Rewrite the comment to not start with one particular example
   now that we can finally describe the feature in a generic way.
 * Using the Nix and Guix package managers at the same time does not
   make much sense, or does it?  So let's show multiple examples
   that actually have a chance of being usable as-is in practice.
 * It is critical for maintenance that the code without READ_ALLOWED_PATH
   remains as undististurbed as possible.  This is a portable-only
   feature that will not be committed to the master repository
   at openbsd.org.
   In particular, the basedir test must not be moved into a new
   function.
 * For better or worse, other functions in this file returning int
   return 1 for success or 0 for failure, so do the same here.
 * The argument of read_allowed() can be const char *.
 * With expressive variable names like READ_ALLOWED_PATH and *candidate,
   there is no real need for a long comment.
 * For a better overview, declare all variables at the top of the
   function, as usual in this codebase.
 * The variable name *cp is customary in this codebase for iterating
   over strings.

Does this version work for you?

Yours,
  Ingo


Index: configure
===================================================================
RCS file: /home/cvs/mandoc/mandoc/configure,v
retrieving revision 1.77
diff -u -p -r1.77 configure
--- configure	20 Jul 2020 16:57:30 -0000	1.77
+++ configure	5 Aug 2021 11:42:59 -0000
@@ -107,7 +107,7 @@ BIN_FROM_SBIN=
 INCLUDEDIR=
 LIBDIR=
 MANDIR=
-HOMEBREWDIR=
+READ_ALLOWED_PATH=
 
 WWWPREFIX="/var/www"
 HTDOCDIR=
@@ -461,7 +461,8 @@ echo "#define MANPATH_DEFAULT \"${MANPAT
 echo "#define OSENUM ${OSENUM}"
 [ -n "${OSNAME}" ] && echo "#define OSNAME \"${OSNAME}\""
 [ -n "${UTF8_LOCALE}" ] && echo "#define UTF8_LOCALE \"${UTF8_LOCALE}\""
-[ -n "${HOMEBREWDIR}" ] && echo "#define HOMEBREWDIR \"${HOMEBREWDIR}\""
+[ -n "${READ_ALLOWED_PATH}" ] \
+	&& echo "#define READ_ALLOWED_PATH \"${READ_ALLOWED_PATH}\""
 [ ${HAVE_ATTRIBUTE} -eq 0 ] && echo "#define __attribute__(x)"
 [ ${HAVE_EFTYPE} -eq 0 ] && echo "#define EFTYPE EINVAL"
 [ ${HAVE_O_DIRECTORY} -eq 0 ] && echo "#define O_DIRECTORY 0"
Index: configure.local.example
===================================================================
RCS file: /home/cvs/mandoc/mandoc/configure.local.example,v
retrieving revision 1.39
diff -u -p -r1.39 configure.local.example
--- configure.local.example	20 Jul 2020 16:57:30 -0000	1.39
+++ configure.local.example	5 Aug 2021 11:43:00 -0000
@@ -209,14 +209,28 @@ INSTALL_LIB="${INSTALL} -m 0444"
 INSTALL_MAN="${INSTALL} -m 0444"
 INSTALL_DATA="${INSTALL} -m 0444"
 
-# When using the "homebrew" package manager on Mac OS X, the actual
-# manuals are located in a so-called "cellar" and only symlinked
-# into the manual trees.  To allow mandoc to follow such symlinks,
-# you have to specify the physical location of the cellar as returned
-# by realpath(3), for example:
+# By default, makewhatis(8) can only read from the paths passed on the
+# command line or configured in man.conf(5).
+# But some package managers on some operating systems store manual pages
+# in separate "cellar" or "store" directories and only symlink them
+# into the manual trees.
+# To support one or more such package managers, give makewhatis(8)
+# read access to the cellars and stores on your system, in the form
+# of a colon-separated path:
 
+# Homebrow package manager on Mac OS X:
 PREFIX="/usr/local"
-HOMEBREWDIR="${PREFIX}/Cellar"
+READ_ALLOWED_PATH="${PREFIX}/Cellar"
+
+# Nix package manager and/or NixOS Linux distibution:
+READ_ALLOWED_PATH="/nix/store"
+
+# GNU Guix package manager and/or GNU Guix Linux distibution:
+READ_ALLOWED_PATH="/gnu/store"
+
+# If multiple package managers are used concurrently:
+PREFIX="/usr/local"
+READ_ALLOWED_PATH="/nix/store:${PREFIX}/Cellar"
 
 # --- user settings for the mandoc(3) library --------------------------
 
Index: mandocdb.c
===================================================================
RCS file: /home/cvs/mandoc/mandoc/mandocdb.c,v
retrieving revision 1.267
diff -u -p -r1.267 mandocdb.c
--- mandocdb.c	3 Apr 2020 11:35:01 -0000	1.267
+++ mandocdb.c	5 Aug 2021 11:43:00 -0000
@@ -165,6 +165,9 @@ static	void	 putkey(const struct mpage *
 static	void	 putkeys(const struct mpage *, char *, size_t, uint64_t);
 static	void	 putmdockey(const struct mpage *,
 			const struct roff_node *, uint64_t, int);
+#ifdef READ_ALLOWED_PATH
+static	int	 read_allowed(const char *);
+#endif
 static	int	 render_string(char **, size_t *);
 static	void	 say(const char *, const char *, ...)
 			__attribute__((__format__ (__printf__, 2, 3)));
@@ -612,8 +615,8 @@ treescan(void)
 				continue;
 			}
 			if (strncmp(buf, basedir, basedir_len) != 0
-#ifdef HOMEBREWDIR
-			    && strncmp(buf, HOMEBREWDIR, strlen(HOMEBREWDIR))
+#ifdef READ_ALLOWED_PATH
+			    && !read_allowed(buf)
 #endif
 			) {
 				if (warnings) say("",
@@ -823,8 +826,8 @@ filescan(const char *infile)
 		start = usefile;
 	else if (strncmp(usefile, basedir, basedir_len) == 0)
 		start = usefile + basedir_len;
-#ifdef HOMEBREWDIR
-	else if (strncmp(usefile, HOMEBREWDIR, strlen(HOMEBREWDIR)) == 0)
+#ifdef READ_ALLOWED_PATH
+	else if (read_allowed(usefile))
 		start = usefile;
 #endif
 	else {
@@ -2380,6 +2383,25 @@ set_basedir(const char *targetdir, int r
 	}
 	return 1;
 }
+
+#ifdef READ_ALLOWED_PATH
+static int
+read_allowed(const char *candidate)
+{
+	const char	*cp;
+	size_t		 len;
+
+	for (cp = READ_ALLOWED_PATH;; cp += len) {
+		while (*cp == ':')
+			cp++;
+		if (*cp == '\0')
+			return 0;
+		len = strcspn(cp, ":");
+		if (strncmp(candidate, cp, len) == 0)
+			return 1;
+	}
+}
+#endif
 
 static void
 say(const char *file, const char *format, ...)
--
 To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 makewhatis] refactor HOMEBREWDIR into READ_ALLOWED_PATH
  2021-08-05 12:35             ` Ingo Schwarze
@ 2021-08-07  0:29               ` sternenseemann
  2021-08-07 13:17                 ` Ingo Schwarze
  0 siblings, 1 reply; 11+ messages in thread
From: sternenseemann @ 2021-08-07  0:29 UTC (permalink / raw)
  To: tech

Hi Ingo,

On 8/5/21 2:35 PM, Ingo Schwarze wrote:
> Yes, i think so.  Besides, the start pointer wasn't advanced in the
> past with HOMEBREWDIR set, so i don't quite see why the present
> change should suddenly make it necessary.  Even if the pointer were
> used for something, stripping a directory that is not the basedir
> would seem wrong for any purpose because it would lose information.

Agreed.

> Other tweaks:
> …

Sounds very reasonable, thank you for adjusting the patch!

> Does this version work for you?

I did a normal rebuild of my db with this patch and everything seemed to
work fine, so that is a yes overall from my side!

> +# Nix package manager and/or NixOS Linux distibution:

Typo: should be distribution

> +# GNU Guix package manager and/or GNU Guix Linux distibution:

Same here.


Cheers,



  Lukas

--
 To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 makewhatis] refactor HOMEBREWDIR into READ_ALLOWED_PATH
  2021-08-07  0:29               ` sternenseemann
@ 2021-08-07 13:17                 ` Ingo Schwarze
  0 siblings, 0 replies; 11+ messages in thread
From: Ingo Schwarze @ 2021-08-07 13:17 UTC (permalink / raw)
  To: sternenseemann; +Cc: tech

Hi Lukas,

sternenseemann wrote on Sat, Aug 07, 2021 at 02:29:52AM +0200:
> On 8/5/21 2:35 PM, Ingo Schwarze wrote:

>> Does this version work for you?

> I did a normal rebuild of my db with this patch and everything
> seemed to work fine, so that is a yes overall from my side!

Thank you for testing and confirming!
This is very helpful because i have no MacOS X, NixOS, or Guix
systems and i don't like committing half-untested patches.

>> +# Nix package manager and/or NixOS Linux distibution:
> Typo: should be distribution
>> +# GNU Guix package manager and/or GNU Guix Linux distibution:
> Same here.

Thanks for catching that one, fixed in the commit.

Yours,
  Ingo


Log Message:
-----------
Rename the compile-time configuration variable $HOMEBREWDIR to
$READ_ALLOWED_PATH, allow it to contain more than one directory,
and explain how to use it for NixOS and for GNU Guix Linux.

Feature improvement based on observations, input, and earlier patches
from Lukas Epple <sternenseemann at systemli dot org>, and final
patch also tested by Lukas.

Modified Files:
--------------
    mandoc:
        configure
        configure.local.example
        mandocdb.c

Revision Data
-------------
Index: configure
===================================================================
RCS file: /home/cvs/mandoc/mandoc/configure,v
retrieving revision 1.77
retrieving revision 1.78
diff -Lconfigure -Lconfigure -u -p -r1.77 -r1.78
--- configure
+++ configure
@@ -107,7 +107,7 @@ BIN_FROM_SBIN=
 INCLUDEDIR=
 LIBDIR=
 MANDIR=
-HOMEBREWDIR=
+READ_ALLOWED_PATH=
 
 WWWPREFIX="/var/www"
 HTDOCDIR=
@@ -461,7 +461,8 @@ echo "#define MANPATH_DEFAULT \"${MANPAT
 echo "#define OSENUM ${OSENUM}"
 [ -n "${OSNAME}" ] && echo "#define OSNAME \"${OSNAME}\""
 [ -n "${UTF8_LOCALE}" ] && echo "#define UTF8_LOCALE \"${UTF8_LOCALE}\""
-[ -n "${HOMEBREWDIR}" ] && echo "#define HOMEBREWDIR \"${HOMEBREWDIR}\""
+[ -n "${READ_ALLOWED_PATH}" ] \
+	&& echo "#define READ_ALLOWED_PATH \"${READ_ALLOWED_PATH}\""
 [ ${HAVE_ATTRIBUTE} -eq 0 ] && echo "#define __attribute__(x)"
 [ ${HAVE_EFTYPE} -eq 0 ] && echo "#define EFTYPE EINVAL"
 [ ${HAVE_O_DIRECTORY} -eq 0 ] && echo "#define O_DIRECTORY 0"
Index: mandocdb.c
===================================================================
RCS file: /home/cvs/mandoc/mandoc/mandocdb.c,v
retrieving revision 1.267
retrieving revision 1.268
diff -Lmandocdb.c -Lmandocdb.c -u -p -r1.267 -r1.268
--- mandocdb.c
+++ mandocdb.c
@@ -165,6 +165,9 @@ static	void	 putkey(const struct mpage *
 static	void	 putkeys(const struct mpage *, char *, size_t, uint64_t);
 static	void	 putmdockey(const struct mpage *,
 			const struct roff_node *, uint64_t, int);
+#ifdef READ_ALLOWED_PATH
+static	int	 read_allowed(const char *);
+#endif
 static	int	 render_string(char **, size_t *);
 static	void	 say(const char *, const char *, ...)
 			__attribute__((__format__ (__printf__, 2, 3)));
@@ -612,8 +615,8 @@ treescan(void)
 				continue;
 			}
 			if (strncmp(buf, basedir, basedir_len) != 0
-#ifdef HOMEBREWDIR
-			    && strncmp(buf, HOMEBREWDIR, strlen(HOMEBREWDIR))
+#ifdef READ_ALLOWED_PATH
+			    && !read_allowed(buf)
 #endif
 			) {
 				if (warnings) say("",
@@ -823,8 +826,8 @@ filescan(const char *infile)
 		start = usefile;
 	else if (strncmp(usefile, basedir, basedir_len) == 0)
 		start = usefile + basedir_len;
-#ifdef HOMEBREWDIR
-	else if (strncmp(usefile, HOMEBREWDIR, strlen(HOMEBREWDIR)) == 0)
+#ifdef READ_ALLOWED_PATH
+	else if (read_allowed(usefile))
 		start = usefile;
 #endif
 	else {
@@ -2380,6 +2383,25 @@ set_basedir(const char *targetdir, int r
 	}
 	return 1;
 }
+
+#ifdef READ_ALLOWED_PATH
+static int
+read_allowed(const char *candidate)
+{
+	const char	*cp;
+	size_t		 len;
+
+	for (cp = READ_ALLOWED_PATH;; cp += len) {
+		while (*cp == ':')
+			cp++;
+		if (*cp == '\0')
+			return 0;
+		len = strcspn(cp, ":");
+		if (strncmp(candidate, cp, len) == 0)
+			return 1;
+	}
+}
+#endif
 
 static void
 say(const char *file, const char *format, ...)
Index: configure.local.example
===================================================================
RCS file: /home/cvs/mandoc/mandoc/configure.local.example,v
retrieving revision 1.39
retrieving revision 1.40
diff -Lconfigure.local.example -Lconfigure.local.example -u -p -r1.39 -r1.40
--- configure.local.example
+++ configure.local.example
@@ -209,14 +209,28 @@ INSTALL_LIB="${INSTALL} -m 0444"
 INSTALL_MAN="${INSTALL} -m 0444"
 INSTALL_DATA="${INSTALL} -m 0444"
 
-# When using the "homebrew" package manager on Mac OS X, the actual
-# manuals are located in a so-called "cellar" and only symlinked
-# into the manual trees.  To allow mandoc to follow such symlinks,
-# you have to specify the physical location of the cellar as returned
-# by realpath(3), for example:
+# By default, makewhatis(8) can only read from the paths passed on the
+# command line or configured in man.conf(5).
+# But some package managers on some operating systems store manual pages
+# in separate "cellar" or "store" directories and only symlink them
+# into the manual trees.
+# To support one or more such package managers, give makewhatis(8)
+# read access to the cellars and stores on your system, in the form
+# of a colon-separated path:
 
+# Homebrow package manager on Mac OS X:
 PREFIX="/usr/local"
-HOMEBREWDIR="${PREFIX}/Cellar"
+READ_ALLOWED_PATH="${PREFIX}/Cellar"
+
+# Nix package manager and/or NixOS Linux distribution:
+READ_ALLOWED_PATH="/nix/store"
+
+# GNU Guix package manager and/or GNU Guix Linux distribution:
+READ_ALLOWED_PATH="/gnu/store"
+
+# If multiple package managers are used concurrently:
+PREFIX="/usr/local"
+READ_ALLOWED_PATH="/nix/store:${PREFIX}/Cellar"
 
 # --- user settings for the mandoc(3) library --------------------------
 
--
 To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-08-07 13:18 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-30 19:49 [PATCH makewhatis] add NIXSTOREDIR configuration variable to support NixOS sternenseemann
2021-03-30 20:30 ` Ingo Schwarze
2021-03-30 21:03   ` sternenseemann
2021-03-31 17:34     ` Ingo Schwarze
2021-03-31 19:33       ` sternenseemann
2021-04-10 15:57       ` [PATCH v2 makewhatis] refactor HOMEBREWDIR into READ_ALLOWED_PATH sternenseemann
2021-04-10 16:15         ` sternenseemann
2021-08-03 16:42           ` sternenseemann
2021-08-05 12:35             ` Ingo Schwarze
2021-08-07  0:29               ` sternenseemann
2021-08-07 13:17                 ` Ingo Schwarze

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).