Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the C plugins mechanism #1867

Merged
merged 2 commits into from Jul 3, 2018
Merged

Conversation

xavierleroy
Copy link
Contributor

@xavierleroy xavierleroy commented Jun 28, 2018

The C plugins mechanism was introduced by @lefessan in #668 to support interception for some system calls in the OCaml runtime system.

Two years and a CVE later, it's unclear that this mechanism is used at all. Moreover, it complicates the runtime system and is not very general (only a few system functions are instrumented). There are other ways to intercept system calls that are more general and require no modification to the source code of the runtime system.

Following discussions with other core Caml developers, this PR proposes to just remove this plugin mechanism. Let the discussion begin!

The mechanism complicates the runtime system and is not very general
(only a few system functions are instrumented).  There are other ways
to intercept system calls that are more general and require no
modification to the source code of the runtime system.
configure Outdated
-no-cplugins|--no-cplugins)
;; # Ignored for backward compatibility
;; # Ignored for backward compatibility; transitional?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, the -with-cplugins and/or -no-cplugins options could be removed altogether, causing a generic error. I have no opinion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I think it’s nicer to have the flag preserved with the warning. Apart from anything else, if anything like it is ever reintroduced, it means care is implied not to re-use the old flag name without considering whether the new flag has the same meaning as the old one (largely academic I expect here, of course)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it's better to keep the error (and even remove the "transitional" comments.

@dra27
Copy link
Member

dra27 commented Jun 28, 2018

Is this running through precheck?

I wholly support this change.

@xavierleroy
Copy link
Contributor Author

xavierleroy commented Jun 29, 2018

This PR went through CI precheck with no errors caused by the changes. (There was an unrelated error on OpenBSD 32, but it's being fixed separately.)

@shindere
Copy link
Contributor

shindere commented Jul 2, 2018 via email

Copy link
Member

@damiendoligez damiendoligez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

configure Outdated
-no-cplugins|--no-cplugins)
;; # Ignored for backward compatibility
;; # Ignored for backward compatibility; transitional?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it's better to keep the error (and even remove the "transitional" comments.

- Clean up configure script as suggested in review
- Added Changes entry
@xavierleroy
Copy link
Contributor Author

Thanks for the feedback. I amended the configure file as suggested and added a Changes entry. Will merge when CI is green.

@xavierleroy xavierleroy merged commit 7e79186 into ocaml:trunk Jul 3, 2018
@xavierleroy xavierleroy deleted the no-cplugins branch July 3, 2018 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants