Drizzle

Merge lp:~brianaker/drizzle/pid-file-fix into lp:~drizzle-trunk/drizzle/development

  1. pid-file-fix
  2. Merge into development
Proposed by Brian Aker
Status: Merged
Approved by: Brian Aker
Approved revision: 2528
Merged at revision: 2532
Proposed branch: lp:~brianaker/drizzle/pid-file-fix
Merge into: lp:~drizzle-trunk/drizzle/development
Diff against target: 372 lines (+102/-71)
3 files modified
drizzled/drizzled.cc (+88/-30)
drizzled/main.cc (+5/-3)
plugin/signal_handler/signal_handler.cc (+9/-38)
To merge this branch: bzr merge lp:~brianaker/drizzle/pid-file-fix
Reviewer Review Type Date Requested Status
Olaf van der Spek (community) Needs Fixing
Drizzle Merge Team Pending
Review via email: mp+98081@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Olaf van der Spek (olafvdspek) wrote :

Looks like the bug fix is mixed with lots of unrelated changes. Why are you changing not X to X == false?
And why do close(file) twice if the first one fails?

review: Needs Fixing

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'drizzled/drizzled.cc'
2--- drizzled/drizzled.cc 2012-02-12 08:22:19 +0000
3+++ drizzled/drizzled.cc 2012-03-18 01:06:18 +0000
4@@ -23,13 +23,16 @@
5 #include <drizzled/atomics.h>
6 #include <drizzled/data_home.h>
7
8+#include <climits>
9+#include <fcntl.h>
10 #include <netdb.h>
11-#include <sys/types.h>
12+#include <netinet/in.h>
13 #include <netinet/tcp.h>
14-#include <netinet/in.h>
15 #include <signal.h>
16-#include <limits.h>
17 #include <stdexcept>
18+#include <sys/stat.h>
19+#include <sys/types.h>
20+#include <sys/types.h>
21
22 #include <boost/program_options.hpp>
23 #include <drizzled/program_options/config_file.h>
24@@ -383,7 +386,32 @@
25 version from the output of 'drizzled --version', so don't change it!
26 */
27 printf("%s Ver %s for %s-%s on %s (%s)\n", internal::my_progname,
28- PANDORA_RELEASE_VERSION, HOST_VENDOR, HOST_OS, HOST_CPU, COMPILATION_COMMENT);
29+ PANDORA_RELEASE_VERSION, HOST_VENDOR, HOST_OS, HOST_CPU, COMPILATION_COMMENT);
30+}
31+
32+/**
33+ Create file to store pid number.
34+*/
35+static void create_pid_file()
36+{
37+ int file;
38+
39+ if ((file = open(pid_file.file_string().c_str(), O_CREAT|O_WRONLY|O_TRUNC|O_CLOEXEC, S_IRWXU|S_IRGRP|S_IROTH)) > 0)
40+ {
41+ char buff[1024];
42+ int length= snprintf(buff, sizeof(buff), "%ld\n", (long) getpid());
43+
44+ if ((write(file, buff, length)) == length)
45+ {
46+ if (close(file) != -1)
47+ {
48+ return;
49+ }
50+ }
51+ (void)close(file); /* We can ignore the error, since we are going to error anyway at this point */
52+ }
53+
54+ unireg_abort << "Can't start server, was unable to create PID file: " << pid_file.file_string();
55 }
56
57 /****************************************************************************
58@@ -435,8 +463,13 @@
59 }
60 }
61
62- if (session::Cache::count())
63- sleep(2); // Give threads time to die
64+ if (session::Cache::count()) // Give threads time to die
65+ {
66+ struct timespec requested;
67+ requested.tv_sec= 2;
68+ requested.tv_nsec= 0;
69+ nanosleep(&requested, NULL);
70+ }
71
72 /*
73 Force remaining threads to die by closing the connection to the client
74@@ -487,7 +520,9 @@
75 void clean_up(bool print_message)
76 {
77 if (cleanup_done++)
78+ {
79 return;
80+ }
81
82 table_cache_free();
83 free_charsets();
84@@ -503,7 +538,9 @@
85 (void) unlink(pid_file.file_string().c_str()); // This may not always exist
86
87 if (print_message && server_start_time)
88+ {
89 errmsg_printf(drizzled::error::INFO, _(ER(ER_SHUTDOWN_COMPLETE)),internal::my_progname);
90+ }
91
92 session::Cache::shutdownFirst();
93
94@@ -541,38 +578,45 @@
95 unireg_abort << _("drizzled cannot be run as root, use --user to start drizzled up as another user");
96 }
97
98- if (not strcmp(user, "root"))
99+ if (strcmp(user, "root") == 0)
100 {
101 return NULL; // Avoid problem with dynamic libraries
102 }
103
104+ bool failed= false;
105 if ((tmp_user_info= getpwnam(user)) == NULL)
106 {
107 // Allow a numeric uid to be used
108 const char *pos= user;
109 for (; my_charset_utf8_general_ci.isdigit(*pos); pos++)
110- {
111- }
112+ { }
113 if (*pos) // Not numeric id
114- goto err;
115+ {
116+ failed= true;
117+ }
118
119 if ((tmp_user_info= getpwuid(atoi(user))) == NULL)
120- goto err;
121- }
122+ {
123+ failed= true;
124+ }
125+ }
126+
127+ if (failed)
128+ {
129+ unireg_abort << "Fatal error: Can't change to run as user '" << user << "' ; Please check that the user exists!";
130+
131+#ifdef PR_SET_DUMPABLE
132+ if (getDebug().test(debug::CORE_ON_SIGNAL))
133+ {
134+ /* inform kernel that process is dumpable */
135+ (void) prctl(PR_SET_DUMPABLE, 1);
136+ }
137+#endif
138+
139+ return NULL;
140+ }
141+
142 return tmp_user_info;
143-
144-err:
145- unireg_abort << "Fatal error: Can't change to run as user '" << user << "' ; Please check that the user exists!";
146-
147-#ifdef PR_SET_DUMPABLE
148- if (getDebug().test(debug::CORE_ON_SIGNAL))
149- {
150- /* inform kernel that process is dumpable */
151- (void) prctl(PR_SET_DUMPABLE, 1);
152- }
153-#endif
154-
155- return NULL;
156 }
157
158 void set_user(const char *user, passwd *user_info_arg)
159@@ -988,10 +1032,17 @@
160 {
161 string new_unknown_opt("--" + *it);
162 ++it;
163+
164 if (it == file_unknown.end())
165+ {
166 break;
167+ }
168+
169 if (*it != "true")
170+ {
171 new_unknown_opt += "=" + *it;
172+ }
173+
174 unknown_options.push_back(new_unknown_opt);
175 }
176 store(file_parsed, vm);
177@@ -1390,6 +1441,9 @@
178
179 fix_paths();
180
181+ /* Save pid to this process (or thread on Linux) */
182+ create_pid_file();
183+
184 init_time(); /* Init time-functions (read zone) */
185
186 item_create_init();
187@@ -2120,15 +2174,19 @@
188 static void fix_paths()
189 {
190 if (vm.count("help"))
191+ {
192 return;
193+ }
194
195- fs::path pid_file_path(pid_file);
196- if (pid_file_path.root_path().string() == "")
197 {
198- pid_file_path= getDataHome();
199- pid_file_path /= pid_file;
200+ fs::path pid_file_path(pid_file);
201+ if (pid_file_path.root_path().string() == "")
202+ {
203+ pid_file_path= getDataHome();
204+ pid_file_path /= pid_file;
205+ }
206+ pid_file= fs::system_complete(pid_file_path);
207 }
208- pid_file= fs::system_complete(pid_file_path);
209
210 const char *tmp_string= getenv("TMPDIR");
211 struct stat buf;
212
213=== modified file 'drizzled/main.cc'
214--- drizzled/main.cc 2012-02-08 21:33:04 +0000
215+++ drizzled/main.cc 2012-03-18 01:06:18 +0000
216@@ -257,7 +257,7 @@
217 error_handler_hook= my_message_sql;
218
219 /* init_common_variables must get basic settings such as data_home_dir and plugin_load_list. */
220- if (not init_variables_before_daemonizing(argc, argv))
221+ if (init_variables_before_daemonizing(argc, argv) == false)
222 {
223 unireg_abort << "init_variables_before_daemonizing() failed"; // Will do exit
224 }
225@@ -268,18 +268,18 @@
226 {
227 perror("Failed to ignore SIGHUP");
228 }
229+
230 if (daemonize())
231 {
232 unireg_abort << "--daemon failed";
233 }
234 }
235
236- if (not init_variables_after_daemonizing(modules))
237+ if (init_variables_after_daemonizing(modules) == false)
238 {
239 unireg_abort << "init_variables_after_daemonizing() failed"; // Will do exit
240 }
241
242-
243 /*
244 init signals & alarm
245 After this we can't quit by a simple unireg_abort
246@@ -415,7 +415,9 @@
247
248 /* If we error on creation we drop the connection and delete the session. */
249 if (Session::schedule(session))
250+ {
251 Session::unlink(session);
252+ }
253 }
254
255 /* Send server shutdown event */
256
257=== modified file 'plugin/signal_handler/signal_handler.cc'
258--- plugin/signal_handler/signal_handler.cc 2012-01-16 02:37:54 +0000
259+++ plugin/signal_handler/signal_handler.cc 2012-03-18 01:06:18 +0000
260@@ -44,7 +44,6 @@
261 extern int cleanup_done;
262 extern bool volatile abort_loop;
263 extern bool volatile shutdown_in_progress;
264-extern boost::filesystem::path pid_file;
265 /* Prototypes -> all of these should be factored out into a propper shutdown */
266 extern void close_connections(void);
267 }
268@@ -93,33 +92,6 @@
269 clean_up(1);
270 }
271
272-/**
273- Create file to store pid number.
274-*/
275-static void create_pid_file()
276-{
277- int file;
278- char buff[1024];
279-
280- if ((file = open(pid_file.file_string().c_str(), O_CREAT|O_WRONLY|O_TRUNC, S_IRWXU|S_IRGRP|S_IROTH)) > 0)
281- {
282- int length;
283-
284- length= snprintf(buff, 1024, "%ld\n", (long) getpid());
285-
286- if ((write(file, buff, length)) == length)
287- {
288- if (close(file) != -1)
289- return;
290- }
291- (void)close(file); /* We can ignore the error, since we are going to error anyway at this point */
292- }
293- memset(buff, 0, sizeof(buff));
294- snprintf(buff, sizeof(buff)-1, "Can't start server: can't create PID file (%s)", pid_file.file_string().c_str());
295- sql_perror(buff);
296- exit(EXIT_FAILURE);
297-}
298-
299
300 /** This threads handles all signals and alarms. */
301 void signal_hand()
302@@ -141,23 +113,23 @@
303 {
304 std::cerr << "failed setting sigaddset() with SIGQUIT\n";
305 }
306+
307 if (sigaddset(&set,SIGHUP))
308 {
309 std::cerr << "failed setting sigaddset() with SIGHUP\n";
310 }
311 #endif
312+
313 if (sigaddset(&set,SIGTERM))
314 {
315 std::cerr << "failed setting sigaddset() with SIGTERM\n";
316 }
317+
318 if (sigaddset(&set,SIGTSTP))
319 {
320 std::cerr << "failed setting sigaddset() with SIGTSTP\n";
321 }
322
323- /* Save pid to this process (or thread on Linux) */
324- create_pid_file();
325-
326 /*
327 signal to init that we are ready
328 This works by waiting for init to free mutex,
329@@ -181,15 +153,14 @@
330
331 for (;;)
332 {
333- if (shutdown_in_progress && not abort_loop)
334+ if (shutdown_in_progress and abort_loop == false)
335 {
336 sig= SIGTERM;
337 }
338 else
339 {
340 while (sigwait(&set, &sig) == EINTR)
341- {
342- }
343+ { }
344 }
345
346 if (cleanup_done)
347@@ -204,14 +175,14 @@
348 case SIGKILL:
349 case SIGTSTP:
350 /* switch to the old log message processing */
351- if (!abort_loop)
352+ if (abort_loop == false)
353 {
354- abort_loop= 1; // mark abort for threads
355+ abort_loop= true; // mark abort for threads
356 kill_server(sig); // MIT THREAD has a alarm thread
357 }
358 break;
359 case SIGHUP:
360- if (not abort_loop)
361+ if (abort_loop == false)
362 {
363 g_refresh_version++;
364 drizzled::plugin::StorageEngine::flushLogs(NULL);
365@@ -254,7 +225,7 @@
366 * prefer this. -Brian
367 */
368 uint32_t count= 2; // How many times to try join and see if the caller died.
369- while (not completed and count--)
370+ while (completed == false and count--)
371 {
372 int signal= count == 1 ? SIGTSTP : SIGTERM;
373