Issue 202 #205

p-alik · 2018-11-20T07:48:33Z

For some reason I can't reopen #203.
The PR do the same:

try to solve make warns: the use of mktemp' is dangerous, better use mkstemp' or `mkdtemp #202.
Additionally it contains few commits from PVS report #114.

esabol · 2018-11-20T14:11:24Z

The only problem with this approach is that mkstemp will actually succeed if you run gearmand or test as root, and temp files will be created in /. I think you should at least unlink the temp file immediately in that scenario.

I think using __mktemp is cleaner, if it's available.

p-alik · 2018-11-20T18:53:02Z

I think using __mktemp is cleaner, if it's available.

mktemp marco as you proposed doesn't work in my environment. (Ubuntu 16.04) That's the reason for mkstemp.

p-alik · 2018-11-21T07:05:42Z

I think you should at least unlink the temp file immediately in that scenario.

It's done in 6ca3f04

esabol · 2018-11-21T07:37:08Z

This is probably fine, but potentially creating a temp file in / still leaves a bad taste. What do you think about using "/tmp/XXXXXXXXX" for the template? You could then use memmove() to remove the /tmp part from lock_name after the mkstemp call, if that's necessary or desirable.

esabol · 2018-11-21T16:39:21Z

util/signal.cc

-  mktemp(lock_name);
+  // random lock_name required
+  if (mkstemp(lock_name) != -1)
+   {


Braces doesn't appear to line up properly here. The open brace should be under the "i" in "if". Double check indentation on the next several lines as well.

mkstemp based approach is removed. The braces also.
Unfortunately Travis CI tests failing with new random string based approach. They are running fine locally.

esabol · 2018-11-22T16:38:27Z

Tests failing... Another option might be to revert the code way back to using unnamed semaphores and only use the named semaphores approach on macOS.

p-alik · 2018-11-22T17:05:49Z

Another option might be to revert the code way back to using unnamed semaphores and only use the named semaphores approach on macOS.

I thought about this also. Actually there is no evidence named semaphores cause the issue. Maybe the content of test-suite.log created during Travis CI build could be helpful in this case. So I'll try to get it.

esabol · 2018-11-22T17:28:21Z

Maybe the content of test-suite.log created during Travis CI build could be helpful in this case. So I'll try to get it.

I've been thinking that it might be useful if Travis CI were to cat test-suite.log after the make test.

… string

according to http://man7.org/linux/man-pages/man3/sem_open.3.html If O_CREAT is specified, then two additional arguments must be supplied

esabol · 2018-11-23T15:46:49Z

Ah, ha! The man page for sem_open says, "If O_CREAT is specified in oflag, then two additional arguments must be supplied." So, as I'm sure you've already figured out, we need the 4-argument version of sem_open...

p-alik · 2018-11-23T16:03:30Z

Yes, I did. We could merge the PR yet.

esabol · 2018-11-23T15:48:00Z

libtest/signal.cc

+  thread_local static std::mt19937 rg{std::random_device{}()};
+  thread_local static std::uniform_int_distribution<std::string::size_type> pick(0, sizeof(chrs) - 2);
+
+  // convenient to named semaphore defination


Misspelled "definition" here.

Also, could you can a blank line after the "break;" for "case SIGQUIT" (right before "case SIGPIPE")? Not in the scope of your changes, but it's trivial.

There is no blank at the end of case SIGQUIT. Do you mean case SIGPIPE?

I was hoping you'd add a blank line after this line:

gearmand/libtest/signal.cc

Line 199 in a333f31

break;

But too late now that we've merged....

esabol · 2018-11-23T15:51:30Z

util/signal.cc

+  std::string s{"/"};
+  s.reserve(len--);
+
+  while(len--)


There should be a space between "while" and "(".

esabol · 2018-11-23T16:02:19Z

libtest/signal.cc

-  lock = sem_open(lock_name, O_CREAT|O_EXCL);
+
+  const char * lock_name = random_lock_name().c_str();
+  lock = sem_open(lock_name, O_CREAT|O_EXCL, 0660, 0);


Instead of 0660, use S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP instead?

I think those S_* constants are defined in sys/stat.h?

#include <sys/stat.h>

esabol · 2018-11-23T16:11:40Z

libtest/signal.cc

+  std::string s{"/"};
+  s.reserve(len--);
+
+  while(len--)


There should be a space between "while" and "(".

esabol · 2018-11-23T16:13:09Z

libtest/signal.cc

+  s.reserve(len--);
+
+  while(len--)
+    s += chrs[pick(rg)];


Something tells me that the coding style for this project is to put braces around single statement loops like this. Please confirm.

I'll add the braces.

esabol · 2018-11-23T16:13:52Z

util/signal.cc

+  s.reserve(len--);
+
+  while(len--)
+    s += chrs[pick(rg)];


Something tells me that the coding style for this project is to put braces around single statement loops like this. Please confirm.

I'll add the braces

esabol · 2018-11-23T16:23:32Z

Also, shouldn't this line be changed?

gearmand/util/signal.cc

Line 230 in ed3d03b

lock = sem_open(lock_name, 0, 0);

I'm not aware of a 3-argument version of sem_open. Only 2 and 4 arguments, right? I'm surprised this even compiles. Don't we want sem_open(lock_name, O_CREAT|O_EXCL, 0660, 0) here?

esabol · 2018-11-23T16:30:05Z

And, not that it matters, but I'm curious why 0660 instead of 0600 for the mode?

p-alik · 2018-11-23T16:41:54Z

And, not that it matters, but I'm curious why 0660 instead of 0600 for the mode?

We'll see, if sem_open(lock_name, O_CREAT|O_EXCL, S_IRUSR|S_IWUSR, 0) works. There is no reason for 0660

esabol · 2018-11-23T16:59:20Z

util/signal.cc

@@ -243,7 +245,7 @@ bool SignalThread::setup()
  }

  const char * lock_name = random_lock_name().c_str();
-  lock = sem_open(lock_name, 0, 0);
+  lock = sem_open(lock_name, O_CREAT, S_IRUSR|S_IWUSR, 0);


No |O_EXCL here?

p-alik mentioned this pull request Nov 20, 2018

Issue 202 #203

Closed

esabol reviewed Nov 21, 2018

View reviewed changes

p-alik force-pushed the issue-202 branch from 6ca3f04 to 4d828e4 Compare November 22, 2018 15:52

p-alik added 5 commits November 23, 2018 07:55

fix SignalThread: complete member initialisation

a0dc022

merely refactoring of getenv check

5a727af

merely refactoring of getenv check

037d9be

replace mktemp based semaphore name assignment by random determinated…

6ef7703

… string

replace mktemp based semaphore name assignment by random determinated…

24015a2

… string

p-alik force-pushed the issue-202 branch from 4d828e4 to 24015a2 Compare November 23, 2018 07:58

p-alik added 5 commits November 23, 2018 08:24

show lock_name on SEM_FAILED

8c6eff6

remove lock_name from SignalThread's constructor

cd643b6

remove lock_name from SignalThread's constructor

f3236f9

show pure errno

3a575ea

pass mode and value parameters to sem_open

046c692

according to http://man7.org/linux/man-pages/man3/sem_open.3.html If O_CREAT is specified, then two additional arguments must be supplied

esabol requested changes Nov 23, 2018

View reviewed changes

set mode parameter of sem_open by named bits

ac5c946

p-alik added 2 commits November 23, 2018 16:44

pass O_CREATE flag and mode to sem_open

4b116c1

while with braces

a2b91f0

p-alik added 2 commits November 23, 2018 16:45

typo

b801034

add O_EXCL to sem_open's flags

d0851c0

esabol reviewed Nov 23, 2018

View reviewed changes

esabol approved these changes Nov 23, 2018

View reviewed changes

p-alik merged commit d0851c0 into gearman:master Nov 23, 2018

p-alik deleted the issue-202 branch November 24, 2018 10:14

p-alik mentioned this pull request Nov 24, 2018

PVS-Report issues p-alik/gearmand#8

Closed

33 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 202 #205

Issue 202 #205

p-alik commented Nov 20, 2018

esabol commented Nov 20, 2018

p-alik commented Nov 20, 2018

p-alik commented Nov 21, 2018

esabol commented Nov 21, 2018

esabol Nov 21, 2018

p-alik Nov 22, 2018

esabol commented Nov 22, 2018

p-alik commented Nov 22, 2018

esabol commented Nov 22, 2018

esabol commented Nov 23, 2018

p-alik commented Nov 23, 2018

esabol Nov 23, 2018

esabol Nov 23, 2018

p-alik Nov 23, 2018

esabol Nov 23, 2018 •

edited

esabol Nov 23, 2018

esabol Nov 23, 2018

p-alik Nov 23, 2018

esabol Nov 23, 2018

esabol Nov 23, 2018

p-alik Nov 23, 2018

esabol Nov 23, 2018

p-alik Nov 23, 2018

esabol commented Nov 23, 2018

esabol commented Nov 23, 2018

p-alik commented Nov 23, 2018

esabol Nov 23, 2018

p-alik Nov 23, 2018

Issue 202 #205

Issue 202 #205

Conversation

p-alik commented Nov 20, 2018

esabol commented Nov 20, 2018

p-alik commented Nov 20, 2018

p-alik commented Nov 21, 2018

esabol commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esabol commented Nov 22, 2018

p-alik commented Nov 22, 2018

esabol commented Nov 22, 2018

esabol commented Nov 23, 2018

p-alik commented Nov 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esabol Nov 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esabol commented Nov 23, 2018

esabol commented Nov 23, 2018

p-alik commented Nov 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esabol Nov 23, 2018 •

edited