search

KillingSpark / rustysd

A service manager that is able to run "traditional" systemd services, written in rust
MIT License
506 stars 15 forks source link

RFC: Add support for setting execution environment variables #43

Closed jabedude closed 4 years ago

jabedude commented 4 years ago

Hello! While playing around with rustysd (awesome project :) ), I was tinkering with adding a simple-ish service configuration option and added support for Environment=. Would you be open to upstreaming this?

KillingSpark commented 4 years ago

Hey! Appreciate the interest! I would of course be willing to upstream this. A few things I'd like changed beforehand though:

  1. The environment: Option<EnvVars> in Service seems unused right? This should reside only in the ServiceConfig.
  2. Applying the EnvVars after the fork is a bit trickier than what one would think. The rust stdlib uses a lock in env::set_var() and in the earlier development I hit cases where this was causing the spawned process to deadlock. This could be fixed with newer rust versions but I would like to not rely on that. In fork_child::after_fork_child some other env vars are set, you can try to mimic that behaviour.
  3. Applying the EnvVars should happen somewhere in fork_child::after_fork_child just to keep the match case small.

I am currently rethinking how services should be started in general, see issue #42. That would remove all the weird hacking around restrictions on what can be safely done between fork and exec. But it has it's own problems. I am not sure how to solve this problem in a good manner, but for now what rustysd does currently seems to work fine.

Can I ask how good/bad your experience was, adding functionality to rustysd, like was a pain to find stuff, or was something annoying in the structure? This is my first bigger project so I am curious :)

jabedude commented 4 years ago

Awesome, good to hear!

The environment: Option in Service seems unused right? This should reside only in the ServiceConfig.

Yes definitely. I'll fix that.

fork_child::after_fork_child some other env vars are set, you can try to mimic that behaviour.

Sounds good! I'll push an update now.

I think it was a good experience overall! I think some parts could be refactored a bit to be more idiomatic, but that's the beauty of open source :) We can improve it together!

KillingSpark commented 4 years ago

Hey, that looks good! One last thing, the Environment= setting is part of systemd.exec which is

  1. Parsed in parse_exec_section in unit_parser.rs
  2. And should be stored in the ExecConfig struct together with the other settings from that section

I am sorry that I did not catch that yesterday. If you could make that change I think it is ready to get merged :)

jabedude commented 4 years ago

No problem. Let me know if this looks okay to you :smile:

KillingSpark commented 4 years ago

Yep, looks good. Thank you! :)