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

Added support for user / group context switching #25

Open
thecky opened this issue Apr 12, 2020 · 9 comments
Open

Added support for user / group context switching #25

thecky opened this issue Apr 12, 2020 · 9 comments

Comments

@thecky
Copy link

thecky commented Apr 12, 2020

Hi, as a FreeCad User on a systemd system, I was sick off to use the "xhost +" command before starting spacenavd and then FreeCad.
So I implemented a simple user context switching. Now it's possible to start the daemon as a privilege user (root), but the mainpart is executed under an other user or group id.
For this, I added the -u / -g option and also the configfile keys user / group.
Before I submit a PR, I would be glad if you could take a look at the comparison of the branches: master...thecky:daemon_privileges
If the code - and the options - are sensible in your eyes, I would submit a PR.
Thanks and regards, Thomas.

@jtsiomb
Copy link
Contributor

jtsiomb commented Apr 13, 2020

Dropping priviledges wherever root is unnecessary is something I wanted to implement for a long time, but never got around to actually do it. I don't know what this has to do with X authentication on systemd systems, but it's certainly something I want to merge, so thank you for taking the time to do it.

I took a very superficial look just now, and I have a number of things I'd like to do differently. Mostly some structuring simplifications, style things, and naming conventions. but of course you don't have to be involved in that if you don't want. We can go about it in two ways:

  1. you create a pull request or a patch exactly as it is right now, I merge it into a new feature branch, and when I'm done with the changes I want to do, I merge the result into master.

  2. I go through your code adding comments about what I want changed, you do the changes, and I merge directly into master when everything in is the shape I want it.

Let me know which way you prefer. Either way is fine with me.

@thecky
Copy link
Author

thecky commented Apr 13, 2020

Thanks for the reply. I tried to adapt your code space&tab identation (by if/else clauses), but I'm pretty sure I miss a lot of your style things and especially the name conventions ;-)

Well, I can implement your comments into my branch, rebase and then PR against your master branch - but imo it's a also good idea to merge into a new feature branch, make your changes directly and do some additional testings (f.e. it's not tested on a MAC), before it's merged into the master branch.

If you have a style-guide-document, I can even made the style related changes.
The fastest way, would be a feature branch at your side - I think.

@jtsiomb
Copy link
Contributor

jtsiomb commented Apr 13, 2020

I don't have a style document. In general I use tabs for indentation with tabs assumed to be 4 spaces long, K&R style brace placement, and no spaces around parentheses (I mean like if(foo) and for(i...)). But that's not the main thing I care about really, that's easily fixed even later on.

I'll go and examine it in more detail and add comments at some point during the next couple of days. Until then the main things I remember from my first look yesterday were:

  • There's no reason for the priviledge data to be passed from function to function, since there can only be a single instance of those during the whole execution. It should just be global.
  • Some names are exceedingly long, for instance I remember a struct has every field starting with the same redundant prefix.

And the last thing I remember from yesterday is that I wanted to take a closer look at the whole user/priviledge code. My first impression was that it looked more complicated than it has to be. But I can't say for certain until I read it more carefully.

Feel free to move along those lines until I can give you some more substantial feedback, and if at any point you get fed up and don't want to work on this any more, just post a pull request and I'll continue in a feature branch as you said.

@thecky
Copy link
Author

thecky commented Apr 19, 2020

I will change the struct to global, that's not the problem (I prefer to avoid globals, whereever is it possible - but that's only my personally opinion :) ).
With the global struct, I can also revert some changes to not directly affected files (like dummy_usb.c).
To shorten the names inside the struct, shouldn't also be problem.

@jtsiomb
Copy link
Contributor

jtsiomb commented Apr 19, 2020

Thanks, yeah that's the idea, to minimize affected files and functions, and simplify the code. Sorry I didn't do the review yet, been a bit busy these days. If you're tired of waiting just do the changes along the lines I said here, and make a pull request, and I'll handle the rest at a later day.

@thecky
Copy link
Author

thecky commented Apr 26, 2020

Hi, the struct is now global and I also renamed the names inside the struct.

The new comparsion is:
master...thecky:daemon_privileges_cleanup

@jtsiomb
Copy link
Contributor

jtsiomb commented Apr 26, 2020

Excellent, this looks much simpler. Make a pull request, and I'll merge in a branch so that I can go over it for any remaining style issues.

Btw never cast the return value of malloc in C. The cast is not necessary, and it can hide a serious bug if you forget to include stdlib.h, which otherwise would produce a diagnostic (warning about conversion from int to whatever pointer).

@thecky
Copy link
Author

thecky commented Apr 26, 2020

I even created the PR.
Thanks for the hint with the cast - I changed this right now and it's included in the PR.
I also added some text to the README.md and the example-spnavrc.

@jtsiomb
Copy link
Contributor

jtsiomb commented Apr 27, 2020

Thanks, I pulled your changes into a branch named dropsu.

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

No branches or pull requests

2 participants