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

[RFC] start converting tmpfiles from shell to C #10

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

vapier
Copy link
Member

@vapier vapier commented May 21, 2019

This is an RFC first off -- I don't intend on merging this directly. I'd like to get feedback about higher level directions before I dive deeper with both documentation and converting code over to compiled code entirely.

For unittesting, I'm writing things in C++ & using gtest. This simplifies test code quite a bit and uses existing common frameworks rather than inventing our own ad-hoc.

I'm assuming people would still prefer to keep the main code in C rather than writing it in C++. Writing it in C++ would simplify resource management, and allow us to use some existing C++ container classes (rather than having to reinvent our own basic ones for C).

Either way, the end result LOC will be higher with the compiled aspect, but it should also be significantly faster, and allow us to address security issues we have now around TOCTOU that are practically impossible to fix in pure shell.

vapier added 3 commits May 19, 2019 09:14
This will make adding a compiled wrapper easier during transition.
We don't compile any code yet.  This is just the framework split
out for easier review.  We'll start compiling things in follow up
commits.
For now, we wrap the existing shell script to maintain full compat.
We'll start moving over functionality in pieces so we can test.
@vapier vapier requested a review from williamh May 21, 2019 16:34
@vapier
Copy link
Member Author

vapier commented May 21, 2019

i'll need to tweak the compiler/CI settings a bit -- looks like Travis defaults to gcc-4.8 which is much too old, and we'll prob need to enable local gtest usage. but neither should matter to the RFC aspects :).

vapier added 5 commits May 21, 2019 22:31
We don't have any tests here yet, but provides the basic framework
for adding them.  The test runner can link & run fine though.

Tests are written using Google Test (gtest) and in C++ to make our
lives easier with more powerful data structures.
These will be used whenever we need to allocate memory and abort if
it fails for any reason.
We work with a few sets, so add a data structure for it to avoid
ad-hoc implementations in other modules.  This is simplify a list
as the sets we're working with tends to be quite small: typically
less than 10.  As long as we don't need to support more, the perf
should be good enough.
The shell script is no longer responsible for parsing the command
line at all.
This is needed for newer gcc versions.
@lu-zero
Copy link

lu-zero commented May 21, 2019

So far looks good to me :) (I should fight the urge to write it in rust for the fun of it, but given the amount of projects still pending it won't happen anytime soon)

@williamh
Copy link
Contributor

The first thing I see is I'd rather use meson as a build system instead of makefiles, so should I start the conversion of that on the master branch?
Also, since we are starting with a new code base, I think I would rather the copyrights just be
"the OpenRC authors" to match OpenRC itself.

@vapier
Copy link
Member Author

vapier commented May 22, 2019

i'm not keen on rust as i plan on having a shared lib interface. it's prob feasible in rust, but seems like it'll add more complexity to the overall runtime usage ...

i have no experience with meson

@lu-zero
Copy link

lu-zero commented May 22, 2019

i'm not keen on rust as i plan on having a shared lib interface. it's prob feasible in rust, but seems like it'll add more complexity to the overall runtime usage ...

Making shared libraries in rust is not hard, and I'm making it even way easier :) If more people have interest we could give it a try, but it can happen at a later time.

I would get the C conversion first.

i have no experience with meson

I have some, it is pretty good for our purposes already and looks like gtest has a ready-made recipe for it.

@vapier
Copy link
Member Author

vapier commented May 27, 2019

does anyone have an opinion against using C++ here ? having access to C++ stdlib would help simplify things further (like std::set).

@lu-zero
Copy link

lu-zero commented May 27, 2019

I'd rather do it in rust then.

@vapier
Copy link
Member Author

vapier commented May 27, 2019

yeah, I'm not doing that

@vapier
Copy link
Member Author

vapier commented Sep 3, 2019

@williamh were you ever able to make progress w/meson here ? or should i just start with makefiles and you can migrate it in parallel ?

@williamh
Copy link
Contributor

williamh commented Jan 3, 2020

Ok, sorry I've not been able to get to this, a couple of questions:

I now have a meson.build on the master branch, but it looks different from what we would need on this branch. How do we go about getting one set up on this branch without affecting the master branch -- @vapier I don't think I can push to your master branch, and I'm not sure that would be a good idea.

Have we made any decision about c, c++ or rust?

@vapier
Copy link
Member Author

vapier commented Jan 4, 2020

I coincidentally picked up rust recently, so I could give it a try. I'm not sure the security aspects matter to the project (as it only ever processes trusted input), but it would include the features that C++ offers over C (i.e. same string handling).

@z3ntu
Copy link

z3ntu commented Jan 4, 2020

I also made a PR a while ago for meson support for @vapier 's branch, see vapier#1

@williamh
Copy link
Contributor

williamh commented Jan 4, 2020

Sigh, we already have Gentoo people claiming that rust is not an acceptable option because of its packaging and static linking.
This pull request isn't the place, but we need to find a way to convince Gentoo people that if we don't accept these modern languages we will eventually be irrelevant.

@lu-zero
Copy link

lu-zero commented Jan 5, 2020

For all we need here, we could even build it using the rustc support provided by meson and no external dependencies.

@zmedico
Copy link

zmedico commented Jan 5, 2020

Consider the overriding blocker for rust on Gentoo to be keywords:

https://packages.gentoo.org/packages/dev-lang/rust

@vapier
Copy link
Member Author

vapier commented Jan 6, 2020

i'll move forward with C++ & meson then, and think about a rust thing in parallel

@lu-zero i wasn't ignoring your meson PR per se, more waiting on the other questions to be settled. since that seems to be done now, i'll try & pick it back up.

@lu-zero
Copy link

lu-zero commented Jan 6, 2020

@vapier it isn't me I think :)

On the other hand, C++ has the lovely problem of bringing in libstdc++ that is way too often ABI-unstable due upstream arguable decisions.

If you want to go on with C++ please make sure we can either avoid linking to the C++ runtime at all or we can linking it statically.

Adding a rust variant for those that prefer it can happen a later time.

@vapier
Copy link
Member Author

vapier commented Jan 7, 2020

I think the ABI instability is a bit overstated. it's not really that big of a problem, and if/when it happens again, this project is probably on the very low end of priority.

it's not really possible to use the useful C++ features without the runtime. static linking of that has always been possible with a dedicated flag. -static-libstdc++ or something.

@lu-zero
Copy link

lu-zero commented Jan 7, 2020

Let see how it goes, probably takes less time to try and see and adapt than keep discussing :)

@lu-zero
Copy link

lu-zero commented Jun 2, 2022

Forgot to mention we did https://github.com/rust-torino/tmpfiles-rs/ as part of the meetup activities.

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.

5 participants