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

Add cmdline options -u, -d and -P #1349

Merged
merged 16 commits into from
Apr 9, 2021
Merged

Add cmdline options -u, -d and -P #1349

merged 16 commits into from
Apr 9, 2021

Conversation

flya320
Copy link
Contributor

@flya320 flya320 commented Mar 27, 2021

-u,user: drop to user USER after startup, also changing all groups to those of USER
-d,daemon: daemonize after startup
-P,pidfile: write a pidfile

@flya320
Copy link
Contributor Author

flya320 commented Mar 27, 2021

As far as I can see, the build checks are failing due to the different output of gerbera -h due to the added options.

@flya320
Copy link
Contributor Author

flya320 commented Mar 27, 2021

regarding the lint-clang-format check I would like to keep my formatting, since I find it more readable

@whyman
Copy link
Member

whyman commented Mar 27, 2021

Hey, thanks for the PR 👍

The failing test you can fix by updating the content of fixtures/mock-help.out to match the new output.
The clang-format I agree makes it marginally less readable, but it also makes it consistent with the rest of the project.

@flya320
Copy link
Contributor Author

flya320 commented Mar 27, 2021

As I have no Mac available perhaps someone could check if all uids, all gids are being changed to the requested user and additional groups are being set. I think they should if I understand the mac os manpages correctly.

@whyman
Copy link
Member

whyman commented Mar 27, 2021

Looks good!

The pidfile write can be simplfied using the fmtlib library:

#include <fmt/os.h>

int main() {
  auto out = fmt::output_file("guide.txt");
  out.print("Don't {}", "Panic");
}

src/main.cc Outdated Show resolved Hide resolved
strcat(pidstr, "\n");

// open the pidfile
int pidfd = open(pidfile->c_str(), O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
Copy link
Member

Choose a reason for hiding this comment

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

There is a function writeTextFile in util/tools which does the file work for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will look into it within the next week.

@whyman
Copy link
Member

whyman commented Apr 6, 2021

Can we get this rebased @flya320? Then happy to merge it, we can fix it up once its in with further PRs.

Otherwise keeping up with master branch changes is going to be annoying 😄

@flya320
Copy link
Contributor Author

flya320 commented Apr 6, 2021

Sorry to say, what does rebase mean? What do I have to do?

thorsten

@whyman
Copy link
Member

whyman commented Apr 6, 2021

Your fork was taken at some time in the past, and the main branch has moved on since then (so we now have conflicts).

Rebasing is the process of updating your branch to be taken from the latest code (moving the "base")

The high level steps are:

  1. Pull the latest master code
  2. checkout your branch
  3. git rebase master
  4. Push any conflicts
  5. git rebase --continue
  6. git push --force-with-lease

@whyman
Copy link
Member

whyman commented Apr 6, 2021

Though, if you prefer, we can "squash" these commits into one and merge it without trouble.

@flya320
Copy link
Contributor Author

flya320 commented Apr 8, 2021

What is easier for you?

@whyman
Copy link
Member

whyman commented Apr 8, 2021

Squashing is easiest as I can just press this green button

@flya320
Copy link
Contributor Author

flya320 commented Apr 9, 2021

Lets do squash then! Sorry that I do not have a clue about the inner workings of github.

Thorsten

@whyman whyman merged commit ebc25dd into gerbera:master Apr 9, 2021
@whyman
Copy link
Member

whyman commented Apr 9, 2021

@flya320 Thanks, its merged!

If you are feeling generous a patch for the documentation (/docs) would be great 😄

@flya320
Copy link
Contributor Author

flya320 commented Apr 9, 2021

Yes, I will add a doc patch. May take a few days though...

@flya320
Copy link
Contributor Author

flya320 commented Apr 10, 2021

Do you only need a patch for the website docs or is the manpage also kept up to date?

KarlStraussberger pushed a commit to KarlStraussberger/gerbera that referenced this pull request Apr 11, 2021
* added option -P to write a pidfile
* cleanup of includes needed for new cmdline options
* update formatting of file
* update file formatting
* define _GNU_SOURCE if not defined by buildsystem
* Mac OS X does the setting of gid/uid differently
* Max OS X: setuid only has one argument
* change due to additional cmdline switches
* clang style changes and more comments
* use stat instead of open to test for pidfile
* stat needs to fail due to ENOENT
* Correct wrong variable declaration
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

3 participants