bugDatabase-driven manual pager suite - Bugs: bug #24043, get_catpath() sometimes fails on...

 
 

bug #24043: get_catpath() sometimes fails on uncommon locale specifiers (with patch)

Submitter:  Dr. Werner Fink <wfink>
Submitted:  Tue 12 Aug 2008 02:56:12 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 16 Oct 2008 10:22:46 AM UTC, comment #11: 

I'd actually also like man-db's pipeline library to have a non-blocking mode, in which it doesn't need to fork but instead calls a function to decompress whatever data is available: conceptually a coroutine although perhaps in practice it would just keep its state somewhere and you'd call it each time round the loop. That would save a fork() per file at the expense of more complicated internal bookkeeping. I left a TODO comment in pipeline_start about this when I was implementing the COMMAND_FUNCTION stuff. Maybe struct command_function could grow a couple of extra function pointers and a non-blocking flag.

I used to work for a company that produced a (blindingly fast) single-threaded web server, and one of the things I've taken away from that is that all good I/O libraries should include non-blocking modes. :-)

 Colin Watson <cjwatson>
Group administrator
Thu 16 Oct 2008 10:11:24 AM UTC, comment #10: 

I didn't intend to be unfriendly or disparaging - sorry if it came across that way. I meant my comments purely as (perhaps robust) technical debate. As a distribution developer myself, I understand very well that distributions sometimes have needs that diverge from those of the upstream developer, but I also think it's sometimes productive to consider whether substantial patches being carried by distributions are still needed or whether they can be dropped in favour of alternative designs upstream; I've certainly discarded substantial work I've done in favour of an upstream design before now. Please don't take it personally, and I appreciate your comments.

The last time I looked at your patch, I recall that the reason I was reluctant to merge it was that it included quite a bit of special-casing in application code, and I was already concerned about the level of special-casing there. Generally I take cleanliness and readability of code as a goal, and while man-db fell far short of this when I took it over I do honestly think I've improved it; part of this involved rewriting and throwing away spaghetti decompression code. I'd be willing to make libzio a configure option without much question if the code to support it were contained in library code, perhaps spread around lib/pipeline.c and lib/decompress.c. If it made that interface significantly more complicated, I would want to apply a level of thought proportional to the increase in complexity of the interface. :-)

Could you point me to your current complete patch (or SRPM)? I've never been very good at finding my way around SuSE's archive by hand.

 Colin Watson <cjwatson>
Group administrator
Thu 16 Oct 2008 09:57:25 AM UTC, comment #9: 

There is no reason to be unfriendly :(
I've never said that man-db's pipeline library is unflexible nor
bad design (IMHO it's perfect design) nevertheless please have a
look on the current libzio before to give such a judgment :(
as I already mentioned, the aim of libzio was to minimize the
runtime of mandb as much as possible. The ansatz of using pipes
I've given also a try but discarded during my measurements.
Even is the execve() can be avoided, the creation of the childs
was more time-consuming then the ansatz of using custom streams
with fopencookie() of the glibc.

Beside there is no reason to throw libzio nor man-db's pipeline library away.  It would be perferct to have an configure option
around to enable the libzio for mandb if available.  This wish
is not and was never intended as disparagement and I expect
also not to become disparaged by anyone else.

Thanks.

 Dr. Werner Fink <wfink>
Wed 15 Oct 2008 05:33:59 PM UTC, comment #8: 

Also, you won't be able to do things like calling manconv and zsoelim as internal functions with libzio. man-db's pipeline library is more flexible since it's relatively generalised rather than being just about decompression. I did look at what you'd done in man-db when I was designing this.

 Colin Watson <cjwatson>
Group administrator
Wed 15 Oct 2008 05:32:03 PM UTC, comment #7: 

I appreciate why you did it to start with, but why bother nowadays? Just use man-db's internal library that does the same thing (at least for all compression types that anyone actually uses in practice for manual pages - why bother using lzma, vanishingly few pages are big enough to make much of a difference and it just slows down decompression on memory-limited systems ...), and that way you won't have to merge a big and complicated patch all the time. It no longer calls an external gunzip if it's built with zlib support.

 Colin Watson <cjwatson>
Group administrator
Wed 15 Oct 2008 05:21:42 PM UTC, comment #6: 

Answer on comment #3:

The libzio provides which can open a FILE stream on .gz, .Z, .bz2, and .lzma files simply with one function fzopen() or fdzopen().
The stream can be handled with fread(), fscanf(), and most
other stream functions and at least it can be closed by fclose().
The idea for this library I had a few years back during patching
the very old source of man-db to avoid the external gunzip
command as this slows down mandb in horrible way (most time
consuming was the fork/execve for the gunzip call) and to be able
to support even the old .Z files in LZW format. See
https://sourceforge.net/projects/libzio/ for more informations.

I'm the maintainer of man-db and texinfo at openSuSE and I
using the libzio library without any problem.  Only programs
using ftell() of fileno() to access the file descriptors will
fail using libzio.

 Dr. Werner Fink <wfink>
Thu 18 Sep 2008 02:11:32 AM UTC, comment #5: 

I've also now done the same internalisation operation on zsoelim, which was the other internal tool that man called while formatting manual pages, although that doesn't affect mandb.

 Colin Watson <cjwatson>
Group administrator
Mon 08 Sep 2008 08:24:44 AM UTC, comment #4: 

Mon Sep  8 09:21:34 BST 2008  Colin Watson  <cjwatson@debian.org>

        Call manconv as an internal function rather than as an external
        process, to improve performance. Indirectly suggested by Dr. Werner
        Fink in Savannah bug #24043.

        * src/Makefile.am (whatis_LDADD): Add $(LIBCOMPRESS).
          (lexgrog_SOURCES, man_SOURCES, mandb_SOURCES, whatis_SOURCES): Add
          manconv.c and manconv.h.
          (manconv_SOURCES): Add manconv.h and manconv_main.c.
        * src/encodings.c (manconv_stdin, free_manconv_codes): New
          functions.
          (add_manconv): Call manconv as an internal function.
        * src/manconv.c (try_iconv): Take the target code as an argument
          rather than using a global variable.
          (manconv): New function, split out of main.
          (split_codes, parse_opt, main): Move to ...
        * src/manconv_main.c: ... here. New file.
        * src/manconv.h: New file.
        * docs/NEWS: Document this.

 Colin Watson <cjwatson>
Group administrator
Mon 08 Sep 2008 05:43:07 AM UTC, comment #3: 

Thanks, I'll have a look at this. In general I don't think that /etc/manpath.config should have to list languages (we don't do that in Debian); it's obviously a losing proposition to have to keep up with all the possible per-locale subdirectories! Perhaps that's why you're seeing this bug when we aren't.

I'm still not sure what you gain from libzio now that man-db has its own decompression layer. It seems like an unnecessary extra layer of abstraction. When man-db was forking gzip, it made sense, but now man-db basically just does gzdopen(); while (remaining) { gzread(); fwrite(); } gzclose(); down the pipe to nroff; you aren't going to make that any better by adding another library to the mix.

I doubt that the performance problems are actually due to creating pipes; that's a very cheap operation. More likely it's due to having to start a new manconv process each time. Instead of having a daemon-like manconv process that can serve several requests, I'd rather link manconv's core code into man and mandb; that way they can call it internally without having to exec an external process, much as they already do with decompression. I'll work on this; thanks.

 Colin Watson <cjwatson>
Group administrator
Mon 18 Aug 2008 04:52:31 PM UTC, comment #2: 

The assert is caused by a custom package, that is that this
foreign package introduce a wrong man path /usr/share/man/jp/
for a Japanese man page. The /etc/manpath.config only reflects
/usr/share/man/ja/, /usr/share/man/ja_JP/, and
/usr/share/man/ja_JP.eucJP/.  IMHO a foreign package should not
cause an assert.  For this reason I've to replace this assert
for openSuSE by a simple warning.

For the wrong naming and the changed coding style be please
accept my sorry. Would it make sence if the mandb could
create the appropiate sub directories for any sub-locales
found beside the main locale.  This would help to reduce
an fatal exit in case of a unknown sub-locale:

  --- mandb.c
  +++ mandb.c       2008-08-18 16:45:48.649940197 +0000
  @@ -418,11 +418,15 @@ static short mandb (const char *catpath,
          short amount;
          char *dbname;
          int fd;
  +       struct stat st;
  
          dbname = mkdbname (catpath);
          sprintf (pid, "%d", getpid ());
          database = appendstr (NULL, catpath, "/", pid, NULL);
  
  +       if (stat(catpath, &st) < 0 && strlen(catpath) > 0 && errno == ENOENT)
  +              mkdir(catpath, S_ISGID|0755);
  +
          /* Just for the case that catpath isn't a system catpath */
          drop_effective_privs ();
          fd = open (database, O_WRONLY | O_CREAT | O_EXCL, 0644);


Beside this the current mandb is very slow even with my changes
for using the libzio.  The pipes opened for every manual page
for execve() the iconv cause a slow down.  As mandb provides
a manconv it could be an idea to use only one pipe for all
manual pages which would also requires that manconv is able
to switch the encoding on the fly with the help of some
magics tags within the input stream.

As I'm on vacation the next two week I can do more about
the requested debug output, as I've no internet the next
two weeks.

 Dr. Werner Fink <wfink>
Sat 16 Aug 2008 11:16:39 PM UTC, comment #1: 

Thanks for your report and patch.

The patch would be smaller if you didn't gratuitously change coding style. Please stick to existing style (and don't mix functional changes with whitespace changes): spaces around operators, no lines longer than 79 columns unless you have a very good excuse (it happens sometimes but I try hard to avoid it even in deeply-indented code), and no space after the '*' in pointer types.

The FILE_CATP and WHATIS_CATP business is obscure to say the least, and sorely needs some comments. After reading it several times I've come to the conclusion that you've used FILE_CATP where the catpath is being used for cat files, and WHATIS_CATP where it's being used for databases. Why would you ever want to specify neither? It seems as if, at worst, that would open the possibility of writing a database that you would never read; at best it's merely very unclear what's going on.

You say "To avoid several different db's for several encodings", but I intentionally designed man-db's per-locale database support to have one database for each per-locale subdirectory of /usr/share/man, even if they share a language code. The reasons for this include (a) some locales share a language code but are materially different, such as pt and pt_BR; and (b) it's quite possible that somebody has unintentionally installed different versions of a page in different per-locale subdirectories, and I'd prefer man-db's databases to track what's actually there rather than having to guess at intent.

The assertion that you removed was in fact correct. If global_manpath is true, then that means that is_global_mandir returned true, which means that get_catpath must find a SYSTEM_CAT entry and return a non-NULL value. I assume that the reason you're hitting the assertion is that you have modified get_catpath to break this assertion. Your problem here is that you're trying to make get_catpath check whether the catpath exists right now (at least in some cases) whereas in fact the catpath should just be created if it doesn't exist yet.

In short, I'm afraid I don't think this patch is taking the correct approach. Can we try to figure out the immediate cause of what's going wrong instead? mandb is supposed to create necessary subdirectories of /var/cache/man for itself. Can you at minimum get the output with the -d switch (which I generally want for all non-trivial bug reports), and perhaps also an strace, so that we can find out why this isn't happening?

 Colin Watson <cjwatson>
Group administrator
Tue 12 Aug 2008 02:56:12 PM UTC, original submission:  

The problem seems to be that within some fully provided locale
the manual page do not exists but nevertheless it is possible
that the main language has a manual page.  This can cause mandb
to fail because it can not create the data base in that case:

 new/man-db-2.5.2> su
 Password:
 boole:man-db-2.5.2 # src/mandb -c
 Processing manual pages under /usr/share/man/cs...
 Updating index cache for path `/usr/share/man/cs/man1'. Wait...
 Checking for stray cats under /usr/share/man/cs...
 Checking for stray cats under /var/cache/man/cs...
 Processing manual pages under /usr/share/man/de...
 Updating index cache for path `/usr/share/man/de/man1'. Wait...
 Updating index cache for path `/usr/share/man/de/man6'. Wait...
 Updating index cache for path `/usr/share/man/de/man7'. Wait...
 Updating index cache for path `/usr/share/man/de/man8'. Wait...
 Updating index cache for path `/usr/share/man/de/man5'. Wait...
 done.
 Checking for stray cats under /usr/share/man/de...
 Checking for stray cats under /var/cache/man/de...
 src/mandb: can't create a temporary filename: No such file or directory

To avoid several different db's for several encodings I've done
a small patch to avoid this.  Also I've seen an assert() which
should not be there.

As ypou can see from the patch I'm using _CATP instead of _CAT
to not interfere with the macros found in libdb/db_storage.h.

 Dr. Werner Fink <wfink>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #16282:  man-db-2.5.2-catp.dif added by wfink (6KiB - video/dv)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by cjwatson (Posted a comment)
  • -email is unavailable- added by wfink (Submitted the item)

    •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follows 1 latest change.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-08-12 wfink Attached File- Added man-db-2.5.2-catp.dif, #16282

    Back to the top

    Copyright © 2024 Free Software Foundation, Inc.
    Verbatim copying and distribution of this entire article is permitted in any medium, provided this notice is preserved.
    The Levitating, Meditating, Flute-playing Gnu logo is a GNU GPL'ed image provided by the Nevrax Design Team.
    Page source code

    Powered by Savane 3.13-461b.
    Corresponding source code