| Message ID | 20200420154537.30879-14-william.c.roberts@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,01/19] security_load_booleans: update return comment | expand |
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
On Sun, Apr 26, 2020 at 8:34 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > 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 Its not just transition, its so internal callers can call into an interface that isn't marked deprecated and we can keep the selinux build -Wdeprecated warning enabled. > avc_init2() to avc_internal.h necessary? Which internal code is > expected to use it? Its not, it can be static in the file. > 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. matchpathcon2 > > 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. Thats fine, I just picked one because it was the exact naming convention I used when discussing this with @sds. I didn't want to change that unless someone suggested it. > > Thanks, > Nicolas >
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_ */