Introduce libcaspermock
ClosedPublic

Authored by oshogbo on Dec 11 2016, 11:50 PM.

Details

Summary

Introduce libcaspermock. This library provides API exactly the same as libcasper. The different is that libcaspermock don't provide sandbox-friendly
functions. Adding this library allow us to simplify the code, we don't need check if libcasper is available in the system anymore, instead we just need to
use this library.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
oshogbo retitled this revision from to Introduce libcaspermock.Dec 11 2016, 11:50 PM
oshogbo updated this object.
oshogbo edited the test plan for this revision. (Show Details)
oshogbo added reviewers: cem, allanjude, ed, emaste, bapt.
oshogbo added a project: capsicum.
oshogbo updated this object.Dec 11 2016, 11:54 PM
cem added a comment.Dec 12 2016, 2:35 AM

Why do we need a separate library for this? Why not just turn MK_CASPER=no into the equivalent of cap_enable() -> false?

The behavior of few functions are a little bit different libcasper and libcaspermock but this still could be merged somehow.
The only situation I can think of where you would like to have two libraries is when you would install something from ports which you don't want to use Casper and your base system is using Casper.

cem added a comment.Dec 12 2016, 7:29 AM

The only situation I can think of where you would like to have two libraries is when you would install something from ports which you don't want to use Casper and your base system is using Casper.

That feels a little contrived :-).

bapt added a comment.Dec 15 2016, 10:29 PM

I don't really much like this approach, plus there is a high risk to have libcaspermock and libcasper out of sync

ed added a comment.Dec 16 2016, 9:27 PM

Maybe what I'm about to say is blasphemy in our circles, but it looks like this tries to solve a problem that an object oriented programming language with virtual functions (C++) could easily solve. libcasper's header file would provide declarations for abstract base classes for all sorts of handles. Then there are two implementations of these classes: one that acts as a no-op and one that is actually built on top of Capsicum. That way there is no need to resort to linker tricks.

oshogbo updated this revision to Diff 31884.Aug 10 2017, 5:41 PM

Hi Guys :)

I changed the approach which should make most of you happy :)
With new implementation if we don't use Casper we don't need to link to any external library.
Most of the functions are implemented as macros or inline functions.

I know that this solution is not ideal, but this change block me a lot for further non-related to libcaspermock changes, so I hope we can proceed with that.
I don't thing also that this solutions don't close as any other solutions from implementation.

This approach was also approved and disused with pjd@.

cem added a comment.Aug 15 2017, 6:17 PM

Would you please upload a diff with full context (-U9999)? Thanks.

kdump/Makefile
17 ↗(On Diff #31884)

This change seems kind of gratuitous and makes this diff a lot bigger.

kdump/kdump.c
59 ↗(On Diff #31884)

Shouldn't all of these ifdefs go away with caspermock? I thought that was the whole point of having a mock casper?

oshogbo updated this revision to Diff 32131.Aug 16 2017, 6:40 PM

Update with -U9999.

oshogbo added inline comments.Aug 16 2017, 6:42 PM
kdump/Makefile
17 ↗(On Diff #31884)

Yes sorry, but we need to have the same as in the libcasper.h :(

kdump/kdump.c
59 ↗(On Diff #31884)

Yes it should but I want to simplify all usage of libcasper per application.
This is why I don't remove that yet and want to do it per separate review.

emaste added inline comments.Aug 25 2017, 1:42 PM
lib/libcasper/Makefile
3–4 ↗(On Diff #32131)

why is this needed?

lib/libcasper/Makefile.inc
5–7 ↗(On Diff #32131)

what do you think about switching the sense and making it -DWITHOUT_CASPER or -DCASPER_MOCK or such?

oshogbo updated this revision to Diff 33161.Sep 17 2017, 10:14 AM
oshogbo added inline comments.
lib/libcasper/Makefile
3–4 ↗(On Diff #32131)

Updated. Thanks.

lib/libcasper/Makefile.inc
5–7 ↗(On Diff #32131)

The WITH_CASPER makes more sens because we are adding the shared libraries base on it. Like in ping case.

ed accepted this revision.Sep 21 2017, 2:05 PM
This revision is now accepted and ready to land.Sep 21 2017, 2:05 PM
oshogbo updated this revision to Diff 33266.Sep 21 2017, 2:44 PM

As discussed with emaste@ I commited the changes regarding the stabilization of the ifdefs name in separate commit (r323866). This should make diff a little bit smaller.

This revision now requires review to proceed.Sep 21 2017, 2:44 PM

From in-person working group session, a suggestion to rename libcasper.h to libcasper.h.in and run unifdef on it during install.

% cat example.h.in
// example header file
#ifdef WITH_CASPER
// casper version
#else
// non-casper version
#endif

% unifdef -UWITH_CASPER -o example.h example.h.in
% cat example.h                                  
// example header file
// non-casper version

% unifdef -DWITH_CASPER -o example.h example.h.in
% cat example.h                                  
// example header file
// casper version

From in-person working group session, a suggestion to rename libcasper.h to libcasper.h.in and run unifdef on it during install.

% cat example.h.in
// example header file
#ifdef WITH_CASPER
// casper version
#else
// non-casper version
#endif

% unifdef -UWITH_CASPER -o example.h example.h.in
% cat example.h                                  
// example header file
// non-casper version

% unifdef -DWITH_CASPER -o example.h example.h.in
% cat example.h                                  
// example header file
// casper version

This works for the general build but has a few problems:

  • If a user changes from WITH_CASPER to WITHOUT_CASPER, the headers will not be rebuilt automatically. Doing so would require META_MODE to be enabled to rebuild when the command changes (-DWITHOUT_CASPER vs -DWITH_CASPER). Granted this problem sort of exists if we dont' use unifdef since all objects would need to be rebuilt if the command changes too.
  • The build silently passes regardless of WITH_CASPER or WITHOUT_CASPER or any discrepancy between what is being built and what may already be built. There's probably no way to solve this.
  • This is difficult to solve for the _rescue/_ build to disable casper in them since we need to generate new headers rather than just check in the headers for #if defined(RESCUE) || defined(WITHOUT_CASPER) at compile-time.

I know I'm rehashing an argument here but IMHO we don't gain much from all of this optional logic. Perhaps the reasoning for it needs to be explained here better. Why do we care about a MK_CASPER option? It does not look like a reasonable size argument from the sizes I'm seeing. The first implementation (https://reviews.freebsd.org/D8753?id=22822) of checking function cap_foo() { if (!cap_enable()) return (foo()); ... } seemed much more reasonable and easy to maintain. Perhaps the initial implementation could be improved with better feedback rather than dropping the approach entirely (the multi-lib issue was the biggest concern I saw and not the wrapper behavior). We can place any master override logic in cap_enable for people who don't want the complexity. If we really want these ifdefs I would rather just have something simple (not using unifdef) and just support #if defined(RESCUE) || defined(WITHOUT_CASPER) for the rare cases it is actually needed to be trimmed out and not bother with a default -DWITH_CASPER in the global CFLAGS...

Closed by commit rS325062: Introduce caspermocks. (authored by oshogbo, committed by ). · Explain WhySat, Oct 28, 7:24 PM
This revision was automatically updated to reflect the committed changes.