| Message ID | 20191111115315.1173097-2-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] libselinux,libsemanage: never create -.o in exception.sh | expand |
On 11/11/19 6:53 AM, Nicolas Iooss wrote: > selinuxswig_python_exception.i and semanageswig_python_exception.i need > to be regenerated when either an input header file changes or > exception.sh changes. Add the missing items to the respective Makefiles. > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> Wondering if we ought to be passing the dependencies as arguments to exception.sh and having it use them rather than a hardcoded header file path, but regardless: Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > --- > libselinux/src/Makefile | 2 +- > libsemanage/src/Makefile | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile > index 3b8bad810de0..7f5a5d7418e9 100644 > --- a/libselinux/src/Makefile > +++ b/libselinux/src/Makefile > @@ -151,7 +151,7 @@ $(LIBSO): $(LOBJS) > $(LIBPC): $(LIBPC).in ../VERSION > sed -e 's/@VERSION@/$(VERSION)/; s:@prefix@:$(PREFIX):; s:@libdir@:$(LIBDIR):; s:@includedir@:$(INCLUDEDIR):; s:@PCRE_MODULE@:$(PCRE_MODULE):' < $< > $@ > > -selinuxswig_python_exception.i: ../include/selinux/selinux.h > +selinuxswig_python_exception.i: exception.sh ../include/selinux/selinux.h > bash -e exception.sh > $@ || (rm -f $@ ; false) > > %.o: %.c policy.h > diff --git a/libsemanage/src/Makefile b/libsemanage/src/Makefile > index e029f0988dd8..8a9570c74163 100644 > --- a/libsemanage/src/Makefile > +++ b/libsemanage/src/Makefile > @@ -94,7 +94,7 @@ $(LIBSO): $(LOBJS) > $(LIBPC): $(LIBPC).in ../VERSION > sed -e 's/@VERSION@/$(VERSION)/; s:@prefix@:$(PREFIX):; s:@libdir@:$(LIBDIR):; s:@includedir@:$(INCLUDEDIR):' < $< > $@ > > -semanageswig_python_exception.i: ../include/semanage/semanage.h > +semanageswig_python_exception.i: exception.sh $(wildcard ../include/semanage/*.h) > bash -e exception.sh > $@ || (rm -f $@ ; false) > > conf-scan.c: conf-scan.l conf-parse.h >
On Tue, Nov 12, 2019 at 4:39 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On 11/11/19 6:53 AM, Nicolas Iooss wrote: > > selinuxswig_python_exception.i and semanageswig_python_exception.i need > > to be regenerated when either an input header file changes or > > exception.sh changes. Add the missing items to the respective Makefiles. > > > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> > > Wondering if we ought to be passing the dependencies as arguments to > exception.sh and having it use them rather than a hardcoded header file > path, but regardless: > > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> I merged my three patches. Thanks for your review. In my humble opinion, I find it simpler not to pass any argument to the script, users can regenerate the files by running exception.sh directly. Nevertheless, when I wrote this patch, there is something that surprised me. In libselinux, only functions in selinux.h are considered when adding glue code to raise OSError from errno when a function returns a negative value. Contrary to semanage.h, selinux.h does not include every other libselinux headers. More precisely, "grep '^extern int ' libselinux/include/selinux/*.h" shows some functions in avc.h, label.h and restorecon.h that are not handled. For example avc_netlink_open() documented in its manpage to return -1 and set errno when an error occurs, but is not present in selinuxswig_python_exception.i. Is this a bug? If yes, fixing it requires changing the API of selinux Python module, which could break some applications (a function would raise an exception instead of returning -1). Nicolas
On 11/13/19 3:30 AM, Nicolas Iooss wrote: > On Tue, Nov 12, 2019 at 4:39 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: >> >> On 11/11/19 6:53 AM, Nicolas Iooss wrote: >>> selinuxswig_python_exception.i and semanageswig_python_exception.i need >>> to be regenerated when either an input header file changes or >>> exception.sh changes. Add the missing items to the respective Makefiles. >>> >>> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> >> >> Wondering if we ought to be passing the dependencies as arguments to >> exception.sh and having it use them rather than a hardcoded header file >> path, but regardless: >> >> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > > I merged my three patches. Thanks for your review. > > In my humble opinion, I find it simpler not to pass any argument to > the script, users can regenerate the files by running exception.sh > directly. > > Nevertheless, when I wrote this patch, there is something that > surprised me. In libselinux, only functions in selinux.h are > considered when adding glue code to raise OSError from errno when a > function returns a negative value. Contrary to semanage.h, selinux.h > does not include every other libselinux headers. More precisely, "grep > '^extern int ' libselinux/include/selinux/*.h" shows some functions in > avc.h, label.h and restorecon.h that are not handled. For example > avc_netlink_open() documented in its manpage to return -1 and set > errno when an error occurs, but is not present in > selinuxswig_python_exception.i. Is this a bug? > If yes, fixing it requires changing the API of selinux Python module, > which could break some applications (a function would raise an > exception instead of returning -1). Curiously selinuxswig.i #include's all (or at least many) of the selinux headers in its %module selinux, whereas selinuxswig_python.i only includes selinux.h as you note. I'm unclear on the history here. It seems like a bug, but breaking API wouldn't be good either. I also have doubts that it is a good idea to be exposing all of the libselinux (or libsemanage) interfaces to python rather than a manually curated list. But pruning that back now would also be difficult. We could at least freeze the interface going forward and only add new interfaces explicitly as needed/desired perhaps.
On 11/13/19 9:53 AM, Stephen Smalley wrote: > On 11/13/19 3:30 AM, Nicolas Iooss wrote: >> On Tue, Nov 12, 2019 at 4:39 PM Stephen Smalley <sds@tycho.nsa.gov> >> wrote: >>> >>> On 11/11/19 6:53 AM, Nicolas Iooss wrote: >>>> selinuxswig_python_exception.i and semanageswig_python_exception.i need >>>> to be regenerated when either an input header file changes or >>>> exception.sh changes. Add the missing items to the respective >>>> Makefiles. >>>> >>>> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> >>> >>> Wondering if we ought to be passing the dependencies as arguments to >>> exception.sh and having it use them rather than a hardcoded header file >>> path, but regardless: >>> >>> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> >> >> I merged my three patches. Thanks for your review. >> >> In my humble opinion, I find it simpler not to pass any argument to >> the script, users can regenerate the files by running exception.sh >> directly. >> >> Nevertheless, when I wrote this patch, there is something that >> surprised me. In libselinux, only functions in selinux.h are >> considered when adding glue code to raise OSError from errno when a >> function returns a negative value. Contrary to semanage.h, selinux.h >> does not include every other libselinux headers. More precisely, "grep >> '^extern int ' libselinux/include/selinux/*.h" shows some functions in >> avc.h, label.h and restorecon.h that are not handled. For example >> avc_netlink_open() documented in its manpage to return -1 and set >> errno when an error occurs, but is not present in >> selinuxswig_python_exception.i. Is this a bug? >> If yes, fixing it requires changing the API of selinux Python module, >> which could break some applications (a function would raise an >> exception instead of returning -1). > > Curiously selinuxswig.i #include's all (or at least many) of the selinux > headers in its %module selinux, whereas selinuxswig_python.i only > includes selinux.h as you note. I'm unclear on the history here. It > seems like a bug, but breaking API wouldn't be good either. Sorry, I looked at the wrong thing (#include vs %include); selinuxswig.i %include's all of the headers, and selinuxswig_python.i %include's selinuxswig.i, so it pulls them all in. So we are generating python bindings for all the interfaces but not generating exception handlers for them all. That does seem to be a bug. > > I also have doubts that it is a good idea to be exposing all of the > libselinux (or libsemanage) interfaces to python rather than a manually > curated list. But pruning that back now would also be difficult. We > could at least freeze the interface going forward and only add new > interfaces explicitly as needed/desired perhaps. >
diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile index 3b8bad810de0..7f5a5d7418e9 100644 --- a/libselinux/src/Makefile +++ b/libselinux/src/Makefile @@ -151,7 +151,7 @@ $(LIBSO): $(LOBJS) $(LIBPC): $(LIBPC).in ../VERSION sed -e 's/@VERSION@/$(VERSION)/; s:@prefix@:$(PREFIX):; s:@libdir@:$(LIBDIR):; s:@includedir@:$(INCLUDEDIR):; s:@PCRE_MODULE@:$(PCRE_MODULE):' < $< > $@ -selinuxswig_python_exception.i: ../include/selinux/selinux.h +selinuxswig_python_exception.i: exception.sh ../include/selinux/selinux.h bash -e exception.sh > $@ || (rm -f $@ ; false) %.o: %.c policy.h diff --git a/libsemanage/src/Makefile b/libsemanage/src/Makefile index e029f0988dd8..8a9570c74163 100644 --- a/libsemanage/src/Makefile +++ b/libsemanage/src/Makefile @@ -94,7 +94,7 @@ $(LIBSO): $(LOBJS) $(LIBPC): $(LIBPC).in ../VERSION sed -e 's/@VERSION@/$(VERSION)/; s:@prefix@:$(PREFIX):; s:@libdir@:$(LIBDIR):; s:@includedir@:$(INCLUDEDIR):' < $< > $@ -semanageswig_python_exception.i: ../include/semanage/semanage.h +semanageswig_python_exception.i: exception.sh $(wildcard ../include/semanage/*.h) bash -e exception.sh > $@ || (rm -f $@ ; false) conf-scan.c: conf-scan.l conf-parse.h
selinuxswig_python_exception.i and semanageswig_python_exception.i need to be regenerated when either an input header file changes or exception.sh changes. Add the missing items to the respective Makefiles. Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> --- libselinux/src/Makefile | 2 +- libsemanage/src/Makefile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)