Daemonize after certain possible errors #177

Hritik14 · Jan 14, 2018

else abnormal exit is treated normal.

kernc · Jan 15, 2018

kernc reviewed Jan 15, 2018

View changes

kernc · Jan 15, 2018

src/logkeys.cc

@@ -467,6 +459,16 @@ int main(int argc, char **argv)
  if (!(args.flags & FLAG_NO_DAEMON)) {
    create_PID_file();
  }
+
+  if (!(args.flags & FLAG_NO_DAEMON)) {


Are you sure daemonization comes after PID file creation above? Whose PID does the file then hold?

Hritik14 · Jan 17, 2018

Mind checking now ?

kernc · Jan 18, 2018

kernc requested changes Jan 18, 2018

View changes

I'll admit to opposing the messages-in-a-separate-file change. Given how they are simple error messages, any of them being used exactly once, I see no gain in having them defined so far away from where they're used.

Other than that and the few comments regarding style, this looks fine. 👍

kernc · Jan 18, 2018

src/logkeys.cc

+    error(EXIT_FAILURE, errno, LOGFILE_OPEN_ERROM);
+
+
+  if(access(PID_FILE, F_OK ) != -1) //PID file already exists


I know the code is not pretty nor consistently adhered to any single style, but could you put a space after if and after //. And double-space before //. And remove the space after F_OK. Kind of like the rest of the code is.

Done. I suppose this projects require a contributing.md that mentions these. Would make things a bit faster. Hmm... Maybe that can be automated. Can you list a consistent style you'd like the code to follow ?

kernc · Jan 18, 2018

src/logkeys.cc

+
+
+  if(access(PID_FILE, F_OK ) != -1) //PID file already exists
+	  error(EXIT_FAILURE,errno, ANOTHER_RUNNING_ERROM);


And a space after comma.

kernc · Jan 18, 2018

src/logkeys.cc

+	  error(EXIT_FAILURE,errno, ANOTHER_RUNNING_ERROM);
+
+  if (!(args.flags & FLAG_NO_DAEMON)) {
+	  int nochdir = 0;


Please use space characters only. I know it's not overly consistent everywhere, but that's no justification to introduce further inconsistencies. For some peculiar legacy reason, the indentation level should be represented with two spaces, i.e. __.

I wish this tab vs space war could end.

Hritik14 · Jan 18, 2018

Constant messages in a seperate file always seem good.
Translations become easy to contribute and messages are super easy to modify (if in case)

Hritik14 · Jan 21, 2018

Any update ?

kernc · Jan 21, 2018

kernc reviewed Jan 21, 2018

View changes

Thanks for the nudge. I understand your concerns re messages. But I'll disagree putting them in a single file is always good. Translations become easy to contribute once you have a template file to translate. Gettext, for one ubiquitous translation system, scans any source files you tell it for strings marked with gettext()/_() macro. I prefer stuff defined where they are used, especially when they are used exactly once. And these are error messages meant for advanced users as is the software in general — I don't foresee them being translated into multiple languages, really.

I'll much prefer to see that commit reverted, and let this PR be about what it says in the title? 😄

kernc · Jan 21, 2018

src/logkeys.cc

-    	if(fgets(buffer, 128, pipe) != NULL)
-    		result += buffer;
+    if(fgets(buffer, 128, pipe) != NULL)
+      result += buffer;


if is missing an indent level under while. I guess don't fix (touch) indentation in this PR, other than on the lines you change. 👍

kernc · Jan 21, 2018

src/logkeys.cc

-    if (errno == EEXIST)
-         error(EXIT_FAILURE, errno, "Another process already running? Quitting. (" PID_FILE ")");
-    else error(EXIT_FAILURE, errno, "Error opening PID file '" PID_FILE "'");
+    if (errno == EEXIST) //This should never happen, ever, but error handling is nice.


Two spaces before a comment start (//), one space before comment contents, like so:

if (errno == EEXIST) // This should ...

I believe this is customary with other C-like imperative languages as well.

kernc · Jan 21, 2018

src/logkeys.cc

-
+    error(EXIT_FAILURE, errno, LOGFILE_OPEN_ERROM);
+
+


If you want, can also remove one blank line here, so we're left with a total of one blank line between blocks.

Hritik14 · Jan 22, 2018

I think that's fixed now.

kernc · Jan 22, 2018

kernc reviewed Jan 22, 2018

View changes

kernc · Jan 22, 2018

src/logkeys.cc

+  if (!(args.flags & FLAG_NO_DAEMON)) {
+    int nochdir = 0;
+    if (args.logfile[0] != '/')
+      nochdir = 1;  // don't chdir (logfile specified with relative path)


Just noticed. Since the already open logfile is now inherited by the daemon, nochdir can always be 0. Can remove the variable altogether.

kernc · Jan 22, 2018

kernc reviewed Jan 22, 2018

View changes

kernc · Jan 22, 2018

src/logkeys.cc

+
+  if (!(args.flags & FLAG_NO_DAEMON)) {
+    int noclose = 1;  // don't close streams (stderr used)
+    if (daemon(nochdir, noclose) == -1)  // become daemon


~~If we had CI, it would have caught this~~: nochdir is not defined.

I blindly pushed it. Fixed it soon though. On my local machine it's not there and I force pushed it. Maybe I'd better commit again

Hritik14 · Jan 22, 2018

Fixed

kernc · Jan 22, 2018

kernc approved these changes Jan 22, 2018

View changes

kernc · Jan 22, 2018

Looks good! Thanks for the contribution, and patience. 👍

Hritik14 force-pushed the Hritik14:master branch from bd46d66 to 60828a8 Jan 22, 2018

Daemonize after certain possible errors.

Loading status checks…

4d60a39

Hritik14 force-pushed the Hritik14:master branch from 60828a8 to 4d60a39 Jan 22, 2018

Hritik14 changed the title ~~Daemonize after creating PID files and other stuff~~ Daemonize after certain possible errors Jan 22, 2018

Hritik14 force-pushed the Hritik14:master branch from 98d4d23 to a20c49a Jan 22, 2018

Removed nochdir variable

Loading status checks…

d7112c8

Hritik14 force-pushed the Hritik14:master branch from a20c49a to d7112c8 Jan 22, 2018

kernc merged commit 7a9f19f into kernc:master Jan 22, 2018
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

kernc/logkeys

Join GitHub today

Daemonize after certain possible errors #177

Conversation

Hritik14 commented Jan 14, 2018

kernc reviewed Jan 15, 2018 View changes

This comment has been minimized.

This comment has been minimized.

Hritik14 commented Jan 17, 2018

kernc requested changes Jan 18, 2018 View changes

kernc left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Hritik14 commented Jan 18, 2018

This comment has been minimized.

Hritik14 commented Jan 21, 2018

kernc reviewed Jan 21, 2018 View changes

kernc left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Hritik14 force-pushed the Hritik14:master branch from bd46d66 to 60828a8 Jan 22, 2018

Hritik14 force-pushed the Hritik14:master branch from 60828a8 to 4d60a39 Jan 22, 2018

Hritik14 changed the title Daemonize after creating PID files and other stuff Daemonize after certain possible errors Jan 22, 2018

This comment has been minimized.

Hritik14 commented Jan 22, 2018

kernc reviewed Jan 22, 2018 View changes

This comment has been minimized.

kernc reviewed Jan 22, 2018 View changes

This comment has been minimized.

kernc Jan 22, 2018 • edited

This comment has been minimized.

Hritik14 force-pushed the Hritik14:master branch from 98d4d23 to a20c49a Jan 22, 2018

Hritik14 force-pushed the Hritik14:master branch from a20c49a to d7112c8 Jan 22, 2018

This comment has been minimized.

Hritik14 commented Jan 22, 2018

kernc approved these changes Jan 22, 2018 View changes

This comment has been minimized.

kernc commented Jan 22, 2018

Hide details View details kernc merged commit 7a9f19f into kernc:master Jan 22, 2018 1 check passed

1 check passed

kernc reviewed Jan 15, 2018

View changes

kernc requested changes Jan 18, 2018

View changes

kernc reviewed Jan 21, 2018

View changes

Hritik14 force-pushed the Hritik14:master branch from `bd46d66` to `60828a8` Jan 22, 2018

Hritik14 force-pushed the Hritik14:master branch from `60828a8` to `4d60a39` Jan 22, 2018

kernc reviewed Jan 22, 2018

View changes

kernc reviewed Jan 22, 2018

View changes

kernc Jan 22, 2018 •

edited

Hritik14 force-pushed the Hritik14:master branch from `98d4d23` to `a20c49a` Jan 22, 2018

Hritik14 force-pushed the Hritik14:master branch from `a20c49a` to `d7112c8` Jan 22, 2018

kernc approved these changes Jan 22, 2018

View changes

kernc merged commit `7a9f19f` into kernc:master Jan 22, 2018
1 check passed