Re: [PATCH] New sol10priv module

[Bruno Haible]

Hello David,

Thank you for your module, and your patience regarding the paperwork.

David Bartley wrote:
> The following patches for coreutils and gnulib add a new Solaris 10
> privilege module

Five small remarks about the module:

1) In the module description:

> +lib/sol10priv.h
> +lib/sol10priv.c
...
> +lib_SOURCES += sol10priv-remove.c sol10priv-restore.c

This will not work. You provide the file sol10priv.c and tell the compiler
to compile two other, different files?

You can try to compile your module through the command
  $ ./gnulib-tool --test sol10priv

2) The specification of the functions is ambiguous.

> /* Try to restore priv if it was previously removed from the effective set.  
> */

A specification should allow to write code that invokes a function, without
looking at its implementation. This implies that it should answer the questions:
  - What does the function do? (You have this already answered.)
  - What is the return value?
  - In which ways can the function fail? Does it set errno when it fails?

3)
> +#include <config.h>
> +
> +#if HAVE_GETPPRIV

When a compilation unit is empty after preprocessing, some compilers warn.
There are two possible workarounds:
  - Add some dummy typedef, as in gnulib/lib/canonicalize-lgpl.c,
  - Put the "#if HAVE_GETPPRIV" after #include "sol10priv.h".

4) Would it be possible to add a unit test? Maybe using the 'ppriv'
   command? See [1][2].

5) 
> +2009-04-27  David Bartley  <address@hidden>
> +        New module 'sol10priv'.

It's customary to put a newline after the author line in a ChangeLog entry.

Bruno

[1] http://docs.sun.com/app/docs/doc/816-5175/privileges-5?a=view
[2] http://docs.sun.com/app/docs/doc/816-5165/ppriv-1?a=view