On Tue, Aug 26, 2014 at 08:54:01PM +0200, Lennart Poettering wrote:
> On Mon, 25.08.14 10:02, Michal Sekletar (msekl...@redhat.com) wrote:
>
> > +int label_get_our_label(char **label) {
> > + int r = 0;
> > + char *l = NULL;
> > +
> > +#ifdef HAVE_SELINUX
> > + r = getcon(&l);
> > + if (r < 0)
> > + return r;
> > +
> > + *label = l;
> > +#endif
> > +
> > + return r;
> > +}
>
> Hmm, we shouldn't return success if selinux support is turned off, and
> we don't write anything to *label... This really should return -ENOTSUP
> or so, i figure....
Are you suggesting that we should change other functions in label.c as well?
AFAICT all non-void functions from that module implement same pattern now.
>
> >
> > diff --git a/src/shared/label.h b/src/shared/label.h
> > index 7294820..0df03f7 100644
> > --- a/src/shared/label.h
> > +++ b/src/shared/label.h
> > @@ -25,6 +25,13 @@
> > #include <stdbool.h>
> > #include <sys/socket.h>
> >
> > +#include "util.h"
> > +
> > +#ifdef HAVE_SELINUX
> > +#include <selinux/selinux.h>
> > +#include <selinux/context.h>
> > +#endif
> > +
> > int label_init(const char *prefix);
> > void label_finish(void);
> >
> > @@ -39,6 +46,8 @@ void label_context_clear(void);
> > void label_free(const char *label);
> >
> > int label_get_create_label_from_exe(const char *exe, char **label);
> > +int label_get_our_label(char **label);
> > +int label_get_child_mls_label(int socket_fd, const char *exec, char
> > **label);
> >
> > int label_mkdir(const char *path, mode_t mode);
> >
> > @@ -49,3 +58,11 @@ int label_apply(const char *path, const char *label);
> > int label_write_one_line_file_atomic(const char *fn, const char *line);
> > int label_write_env_file(const char *fname, char **l);
> > int label_fopen_temporary(const char *path, FILE **_f, char **_temp_path);
> > +
> > +#ifdef HAVE_SELINUX
> > +DEFINE_TRIVIAL_CLEANUP_FUNC(security_context_t, freecon);
> > +DEFINE_TRIVIAL_CLEANUP_FUNC(context_t, context_free);
> > +
> > +#define _cleanup_security_context_free_ _cleanup_(freeconp)
> > +#define _cleanup_context_free_ _cleanup_(context_freep)
> > +#endif
>
> Hmm, wouldn't it suffice to have the latter four lines simply in
> label.c, not in the header files? I'd prefer if we didn't have to
> include the selinux headers from the header file...
Quick check revealed that we are using for example security_context_t type in
following modules other than label.c:
src/core/selinux-access.c
src/core/selinux-setup.c
src/journal/journald-server.c
src/journal/journald-stream.c /* now cleanup macros can't be of any use here */
src/nspawn/nspawn.c /* here as well */
Honestly I don't really care either way. Given that use cases in above modules
for new macros are really minimal (1-2 per module) we can just put those four
lines to C file make use of macros there and leave the rest as is.
>
> Otherwise looks great!
Thanks, again, for the review. It is much appreciated.
>
> Lennart
Michal
>
> --
> Lennart Poettering, Red Hat
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
