Re: [HACKERS] return values of backend sub-main functions

2012-06-19 Thread Peter Eisentraut
On ons, 2012-01-18 at 21:21 +0200, Peter Eisentraut wrote:
 On lör, 2012-01-07 at 16:41 -0500, Tom Lane wrote:
  Peter Eisentraut pete...@gmx.net writes:
   I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(),
   WalSenderMain(), and WalSndLoop() to return void as well.
  
  I agree this code is not very consistent or useful, but one question:
  what should the callers do if one of these functions *does* return?
 
 I was thinking of a two-pronged approach:  First, add
 __attribute__((noreturn)) to the functions.  This will cause a suitable
 compiler to verify on a source-code level that nothing actually returns
 from the function.  And second, at the call site, put an abort();  /*
 not reached */.  Together, this will make the code cleaner and more
 consistent, and will also help the compiler out a bit about the control
 flow.

Patch for 9.3 attached.

diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index c7d48e9..33c5a0a 100644
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -173,7 +173,7 @@
 
 #ifdef EXEC_BACKEND
 	if (argc  1  strncmp(argv[1], --fork, 6) == 0)
-		exit(SubPostmasterMain(argc, argv));
+		SubPostmasterMain(argc, argv); /* does not return */
 #endif
 
 #ifdef WIN32
@@ -189,14 +189,13 @@
 
 	if (argc  1  strcmp(argv[1], --boot) == 0)
 		AuxiliaryProcessMain(argc, argv);		/* does not return */
-
-	if (argc  1  strcmp(argv[1], --describe-config) == 0)
-		exit(GucInfoMain());
-
-	if (argc  1  strcmp(argv[1], --single) == 0)
-		exit(PostgresMain(argc, argv, get_current_username(progname)));
-
-	exit(PostmasterMain(argc, argv));
+	else if (argc  1  strcmp(argv[1], --describe-config) == 0)
+		GucInfoMain();			/* does not return */
+	else if (argc  1  strcmp(argv[1], --single) == 0)
+		PostgresMain(argc, argv, get_current_username(progname)); /* does not return */
+	else
+		PostmasterMain(argc, argv); /* does not return */
+	abort();		/* should not get here */
 }
 
 
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index eeea933..0f8af3a 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -343,8 +343,8 @@ static void LogChildExit(int lev, const char *procname,
 			 int pid, int exitstatus);
 static void PostmasterStateMachine(void);
 static void BackendInitialize(Port *port);
-static int	BackendRun(Port *port);
-static void ExitPostmaster(int status);
+static void	BackendRun(Port *port) __attribute__((noreturn));
+static void ExitPostmaster(int status) __attribute__((noreturn));
 static int	ServerLoop(void);
 static int	BackendStartup(Port *port);
 static int	ProcessStartupPacket(Port *port, bool SSLdone);
@@ -491,7 +491,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
 /*
  * Postmaster main entry point
  */
-int
+void
 PostmasterMain(int argc, char *argv[])
 {
 	int			opt;
@@ -1125,7 +1125,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
 	 */
 	ExitPostmaster(status != STATUS_OK);
 
-	return 0;	/* not reached */
+	abort();	/* not reached */
 }
 
 
@@ -3295,7 +3295,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
 		BackendInitialize(port);
 
 		/* And run the backend */
-		proc_exit(BackendRun(port));
+		BackendRun(port);
 	}
 #endif   /* EXEC_BACKEND */
 
@@ -3539,7 +3539,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
  *		Shouldn't return at all.
  *		If PostgresMain() fails, return status.
  */
-static int
+static void
 BackendRun(Port *port)
 {
 	char	  **av;
@@ -3610,7 +3610,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
 	 */
 	MemoryContextSwitchTo(TopMemoryContext);
 
-	return (PostgresMain(ac, av, port-user_name));
+	PostgresMain(ac, av, port-user_name);
 }
 
 
@@ -3960,7 +3960,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
  * have been inherited by fork() on Unix.  Remaining arguments go to the
  * subprocess FooMain() routine.
  */
-int
+void
 SubPostmasterMain(int argc, char *argv[])
 {
 	Port		port;
@@ -4195,7 +4195,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
 		proc_exit(0);
 	}
 
-	return 1;	/* shouldn't get here */
+	abort();	/* shouldn't get here */
 }
 #endif   /* EXEC_BACKEND */
 
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 45a3b2e..a9a8689 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -121,7 +121,7 @@
 
 /* Prototypes for private functions */
 static bool HandleReplicationCommand(const char *cmd_string);
-static int	WalSndLoop(void);
+static void WalSndLoop(void) __attribute__((noreturn));
 static void InitWalSnd(void);
 static void WalSndHandshake(void);
 static void WalSndKill(int code, Datum arg);
@@ -136,7 +136,7 @@
 
 
 /* Main entry point for walsender process */
-int
+void
 WalSenderMain(void)
 {
 	MemoryContext 

Re: [HACKERS] return values of backend sub-main functions

2012-06-19 Thread Robert Haas
On Tue, Jun 19, 2012 at 7:31 AM, Peter Eisentraut pete...@gmx.net wrote:
 On ons, 2012-01-18 at 21:21 +0200, Peter Eisentraut wrote:
 On lör, 2012-01-07 at 16:41 -0500, Tom Lane wrote:
  Peter Eisentraut pete...@gmx.net writes:
   I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(),
   WalSenderMain(), and WalSndLoop() to return void as well.
 
  I agree this code is not very consistent or useful, but one question:
  what should the callers do if one of these functions *does* return?

 I was thinking of a two-pronged approach:  First, add
 __attribute__((noreturn)) to the functions.  This will cause a suitable
 compiler to verify on a source-code level that nothing actually returns
 from the function.  And second, at the call site, put an abort();  /*
 not reached */.  Together, this will make the code cleaner and more
 consistent, and will also help the compiler out a bit about the control
 flow.

 Patch for 9.3 attached.

Seems reasonable on a quick read-through.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] return values of backend sub-main functions

2012-06-19 Thread Josh Kupershmidt
On Tue, Jun 19, 2012 at 4:31 AM, Peter Eisentraut pete...@gmx.net wrote:
 On ons, 2012-01-18 at 21:21 +0200, Peter Eisentraut wrote:
 On lör, 2012-01-07 at 16:41 -0500, Tom Lane wrote:
  Peter Eisentraut pete...@gmx.net writes:
   I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(),
   WalSenderMain(), and WalSndLoop() to return void as well.
 
  I agree this code is not very consistent or useful, but one question:
  what should the callers do if one of these functions *does* return?

 I was thinking of a two-pronged approach:  First, add
 __attribute__((noreturn)) to the functions.  This will cause a suitable
 compiler to verify on a source-code level that nothing actually returns
 from the function.  And second, at the call site, put an abort();  /*
 not reached */.  Together, this will make the code cleaner and more
 consistent, and will also help the compiler out a bit about the control
 flow.

 Patch for 9.3 attached.

+1. Should this call around line 4114 of postmaster.c not bother with
proc_exit() anymore:

/* And run the backend */
proc_exit(BackendRun(port));

I was hoping that some of the clang static analyzer complaints would
go away with these changes, though it looks like only one[1] did. I
would be interested to see the similar elog/ereport patch you
mentioned previously, perhaps that will eliminate a bunch of warnings.

Josh

[1] this one goes away with the patch:
http://kupershmidt.org/pg/scan-build-2012-06-19-master/report-E2cUqJ.html#EndPath

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] return values of backend sub-main functions

2012-01-18 Thread Peter Eisentraut
On lör, 2012-01-07 at 16:41 -0500, Tom Lane wrote:
 Peter Eisentraut pete...@gmx.net writes:
  I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(),
  WalSenderMain(), and WalSndLoop() to return void as well.
 
 I agree this code is not very consistent or useful, but one question:
 what should the callers do if one of these functions *does* return?

I was thinking of a two-pronged approach:  First, add
__attribute__((noreturn)) to the functions.  This will cause a suitable
compiler to verify on a source-code level that nothing actually returns
from the function.  And second, at the call site, put an abort();  /*
not reached */.  Together, this will make the code cleaner and more
consistent, and will also help the compiler out a bit about the control
flow.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] return values of backend sub-main functions

2012-01-07 Thread Tom Lane
Peter Eisentraut pete...@gmx.net writes:
 I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(),
 WalSenderMain(), and WalSndLoop() to return void as well.

I agree this code is not very consistent or useful, but one question:
what should the callers do if one of these functions *does* return?

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers