Re: [PATCH 1/2] selinux-h: new module (from coreutils/gl/)

[Bruno Haible]

Hi Jim,

A few random remarks.

Why is the module name 'selinux-h', not 'selinux'? (We have 'stdlib',
'unistd' etc. for include files.) Is there a separate module 'selinux'
to come later?

> +# define is_selinux_enabled() 0

> +Description:
> +SELinux-related headers for systems that lack them.

Could the description be enhanced to mention
  - what SELinux is or stands for?
  - that this module is only a dummy replacement, not a workalike
    implementation? Currently, it's only obvious after looking into
    the .h files.

Also, could some comment referring to the API be added to the two .in.h
files? Function names like 'getcon' and 'freecon' are not self-explanatory.
Just an URL to the LSB or SELinux web site would be sufficient.

> +AC_DEFUN([gl_HEADERS_SELINUX_CONTEXT_H],

Why the plural here? Why not gl_HEADER_SELINUX_CONTEXT_H? It's only one
header file being replaced by this macro.

> +  AC_CHECK_HEADERS([selinux/selinux.h],
> +                [SELINUX_SELINUX_H=],
> +                [SELINUX_SELINUX_H=selinux/selinux.h])

Is it true that SELinux cannot be installed in another directory than in
--prefix=/usr?

When the header files exist on the system, which library is needed in order
to link with the functions? Is everything already in libc? I'm asking because
the module description does not provide a 'Link:' section.

> +static inline char *context_str (context_t con)
> +  { errno = ENOTSUP; return (void *) 0; }
> +static inline void context_free (context_t c) {}

Hmm, this is not exactly GNU style placement of braces.

> +Files:
> +lib/se-context.in.h
> +lib/se-selinux.in.h

You can use subdirectories, if you want. I.e. you could move
  lib/se-context.in.h   to    lib/selinux/context.in.h
  lib/se-selinux.in.h   to    lib/selinux/selinux.in.h

gnulib-tool allows subdirectories of lib/, and I think you also made
'bootstrap' support them.

> +lib_SOURCES += se-context.in.h se-selinux.in.h

This line is not needed. In this context, lib_SOURCES is equivalent to
EXTRA_DIST, and EXTRA_DIST is synthesized from the 'Files:' list.

Bruno