Re: [PATCH v3 13/19] avc: create internal avc_init interface

[Nicolas Iooss <nicolas.iooss@m4x.org>]

On Mon, Apr 20, 2020 at 5:46 PM <bill.c.roberts@gmail.com> wrote:
>
> From: William Roberts <william.c.roberts@intel.com>
>
> Now that avc_init is marked deprecated, create an avc_init2 interface
> for internal users.
>
> Signed-off-by: William Roberts <william.c.roberts@intel.com>
> ---
>  libselinux/src/avc.c          | 11 ++++++++++-
>  libselinux/src/avc_internal.h |  5 +++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/libselinux/src/avc.c b/libselinux/src/avc.c
> index ab10b0f9f1cb..505641406995 100644
> --- a/libselinux/src/avc.c
> +++ b/libselinux/src/avc.c
> @@ -157,7 +157,7 @@ int avc_open(struct selinux_opt *opts, unsigned nopts)
>                         break;
>                 }
>
> -       return avc_init("avc", NULL, NULL, NULL, NULL);
> +       return avc_init2("avc", NULL, NULL, NULL, NULL);
>  }
>
>  int avc_init(const char *prefix,
> @@ -165,6 +165,15 @@ int avc_init(const char *prefix,
>              const struct avc_log_callback *log_cb,
>              const struct avc_thread_callback *thread_cb,
>              const struct avc_lock_callback *lock_cb)
> +{
> +       return avc_init2(prefix, mem_cb, log_cb, thread_cb, lock_cb);
> +}
> +
> +int avc_init2(const char *prefix,
> +            const struct avc_memory_callback *mem_cb,
> +            const struct avc_log_callback *log_cb,
> +            const struct avc_thread_callback *thread_cb,
> +            const struct avc_lock_callback *lock_cb)
>  {
>         struct avc_node *new;
>         int i, rc = 0;
> diff --git a/libselinux/src/avc_internal.h b/libselinux/src/avc_internal.h
> index 3f8a6bb1cf84..c8d26a8ae254 100644
> --- a/libselinux/src/avc_internal.h
> +++ b/libselinux/src/avc_internal.h
> @@ -173,4 +173,9 @@ int avc_ss_set_auditdeny(security_id_t ssid, security_id_t tsid,
>  /* netlink kernel message code */
>  extern int avc_netlink_trouble ;
>
> +extern int avc_init2(const char *msgprefix,
> +                   const struct avc_memory_callback *mem_callbacks,
> +                   const struct avc_log_callback *log_callbacks,
> +                   const struct avc_thread_callback *thread_callbacks,
> +                   const struct avc_lock_callback *lock_callbacks);
>  #endif                         /* _SELINUX_AVC_INTERNAL_H_ */
> --
> 2.17.1

Hello,
I do not see the point of having a new avc_init2() "internal
interface". I get that avc_init() is deprecated, that avc_open()
should be used, and that internally a new function (named avc_init2)
is created to make the transition easier. But why is adding
avc_init2() to avc_internal.h necessary? Which internal code is
expected to use it?
If none, I would prefer to make avc_init2() static (changing this
patch to "static init avc_init2(const char*msgprefix,", with a
declaration before avc_open() if you do not want to move the function
in the file).

I have the same question for matchpathcon_fini2(), matchpathcon2(), etc.

Moreover, I do not really like the "...2" naming (this is my own point
of view and I won't block the patch because of it), because it seems
to carry the meaning of "please now use this inferface", which is
untrue. I suggest using avc_init_internal(),
matchpathcon_fini_internal()... that do not carry such a meaning.

Thanks,
Nicolas