You are right, pidfile creation may fail if we use flock in
osaftransportd. I think flock seems pretty useless. I have checked the
source code for start-stop-daemon which is included in Ubuntu, and they
actually use fopen() only, without any call to flock(). So maybe the
previous approach where we create the pidfile atomically is the best one.
I will review the patch you sent out for review.
thanks,
Anders Widell
On 05/04/2017 01:54 PM, minh chau wrote:
> Hi Anders, Hans,
>
> I think there is still a deadlock even with nonblocking flock() in a
> below scenario
> - osaftransportd first calls flock() and the call succeeds
> - osafdtmd calls flock() in __create_pid_file(), and it would fail and
> return -1, __create_pid_file() is not trying to flock() again, thus no
> pid file is created
> - osaftransportd finds the pid file is empty, unlock the lock and try
> again, but then it would not be able to retrieve pid from pid file
> since no pid file is created
>
> It could be solved if we add in the __create_pid_file() a loop of
> lock/unlock similarly as Anders' pseudo code. But there's another
> problem.
> It is another method of FileNotify::WaitForFileClose() as we are
> introducing, this WaitForFileClose() belongs to /base code so it could
> be used by any services to wait for any kinds of file creation. It
> should work for pid file because of flock, but it's not guaranteed for
> the other non-pid file (not using flock) that data in file is always
> ready to read. The WaitForFileClose() may return while the non-pid
> file is being written. That's probably the reason we didn't like
> IN_CLOSE_WRITE and went for atomic temporary file.
>
> The problem could be easy if we fix it at osaftransportd, but it would
> be a bit harder if we fix at /base code, which's solution supposes to
> be more generic.
> The problem in this ticket is at osaftransportd which is reading pid
> file, specifically. We know how pid file is created and there's just
> pid in file.
> If the fix is at transportd, we may retry to read pid file, or retry
> to take the flock until there's pid in file (as pseudo code). If the
> fix is at base/file_notify, it is supposed to work for all kinds of
> file creation.
>
> It's my first thought for now, I could be wrong and still learning
> Ander's pseudo code ...
>
> thanks,
> Minh
>
> On 03/05/17 19:55, Anders Widell wrote:
>> Pseudocode could be something like this:
>>
>>
>> CREATE inotify watch for IN_CLOSE_WRITE on the file
>>
>> REPEAT
>>
>> Open the file
>>
>> IF Open was successful THEN
>>
>> Lock the file nonblocking
>>
>> IF Lock was successful THEN
>>
>> READ the file
>>
>> IF Read was successful and file size was non-zero THEN
>>
>> UNLOCK the file (maybe not needed - done by
>> close?)
>>
>> CLOSE the file
>>
>> REMOVE inotify watch
>>
>> RETURN file contents
>>
>> ENDIF
>>
>> UNLOCK the file (maybe not needed - done by close?)
>>
>> ENDIF
>>
>> CLOSE the file
>>
>> ENDIF
>>
>> WAIT for inotify event
>>
>> END REPEAT
>>
>> regards,
>> Anders Widell
>>
>>
>> On 05/03/2017 10:46 AM, Anders Widell wrote:
>>> I think osaftransportd must do this sequence:
>>>
>>> * Open the file
>>>
>>> * Lock the file
>>>
>>> * Check if the file has a non-zero size - if the size is zero, close
>>> the
>>> file and treat it as non-existing.
>>>
>>> Though your worry about deadlock is important from a security
>>> perspective. Locking shall be non-blocking as Hans says - otherwise
>>> there can be a risk for denial of service.
>>>
>>> regards,
>>>
>>> Anders Widell
>>>
>>> On 05/03/2017 08:52 AM, Hans Nordebäck wrote:
>>>> Hi Minh,
>>>>
>>>> the flock is used in non blocking mode, not sure but as I mentioned
>>>> earlier why not add a new function
>>>>
>>>> FileNotify::WaitForFileClose and change the return codes in
>>>> ProcessEvents to differentiate the events?
>>>>
>>>> /Thanks HansN
>>>>
>>>>
>>>> On 05/03/2017 08:43 AM, minh chau wrote:
>>>>> Hi Anders, Hans,
>>>>>
>>>>> Using flock() when reading pid file in osaftransportd, it may be
>>>>> causing a deadlock. When open() is called in __create_pidfile(), that
>>>>> will generate IN_CREATE towards osaftransportd. Right after that if
>>>>> osaftransportd takes the lock before __create_pidfile(), I think it
>>>>> will cause a deadlock?
>>>>>
>>>>> There might be two ways we can do:
>>>>>
>>>>> (1) It's a little bit change in the temporary approach, we first
>>>>> create original pid file as normally, at the end of
>>>>> __create_pidfile(), create a symlink temporary pid file targeting the
>>>>> original pid file. Then, osaftransportd now watches the temporary pid
>>>>> file, and FileNotify can continue using IN_CREATE (which is created
>>>>> by both symlink() and open(), don't have to add IN_MOVED_TO)
>>>>>
>>>>> (2) The other simple one is the retry of reading pid as sent out in
>>>>> V1, it does retry as similar as NID is trying to open fifo file after
>>>>> call WaitForFileCreation(). We can extend the waiting time of retries
>>>>> for osaftransportd as similar to NID, and the pid should be ready to
>>>>> read by that period of waiting time.
>>>>>
>>>>> Or (3)...?
>>>>>
>>>>> thanks,
>>>>> Minh
>>>>>
>>>>> On 03/05/17 00:43, Anders Widell wrote:
>>>>>> Yes, when reading this patch I realize that the pidfile creation
>>>>>> already uses flock() for atomicity. Maybe this is standard practice
>>>>>> when creating pid files, and if so we probably shouldn't do it in
>>>>>> another way because then we can have problems interacting with other
>>>>>> programs that operate on the pid files (Ubuntu start-stop-daemon and
>>>>>> LSB start_daemon). Maybe the solution is as simple as using flock()
>>>>>> when reading the pid file?
>>>>>>
>>>>>> regards,
>>>>>>
>>>>>> Anders Widell
>>>>>>
>>>>>>
>>>>>> On 05/02/2017 03:10 PM, Hans Nordebäck wrote:
>>>>>>> Hi Minh,
>>>>>>>
>>>>>>> some minor comments below, I discussed with Anders about using the
>>>>>>> existing flock instead of the rename.
>>>>>>>
>>>>>>> /BR HansN
>>>>>>>
>>>>>>>
>>>>>>> On 04/28/2017 11:36 AM, Minh Chau wrote:
>>>>>>>> At startup, osaftransportd waits for osafdtmd.pid file creation
>>>>>>>> and then reads dtm pid. If osafdtmd.pid has not been completedly
>>>>>>>> created but osaftransportd still receives IN_CREATE,
>>>>>>>> osaftransported
>>>>>>>> will fail to read pid of dtmd. That results in a node reboot with
>>>>>>>> a reason as "osafdtmd failed to start".
>>>>>>>>
>>>>>>>> The patch implements an approach suggested by Anders Widell, which
>>>>>>>> creates a completed temporary pid file first, then renames it to
>>>>>>>> correct pid file name. Whenever osaftransportd is notified to read
>>>>>>>> dtmd's pid, the data in pid file should be always safe to read. In
>>>>>>>> addition to this, FileNotify needs to introduce IN_MOVED_TO event.
>>>>>>>> ---
>>>>>>>> src/base/daemon.c | 27 ++++++++++++++++++---------
>>>>>>>> src/base/file_notify.cc | 10 +++++++++-
>>>>>>>> 2 files changed, 27 insertions(+), 10 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/src/base/daemon.c b/src/base/daemon.c
>>>>>>>> index 77a869561..2fd161ce0 100644
>>>>>>>> --- a/src/base/daemon.c
>>>>>>>> +++ b/src/base/daemon.c
>>>>>>>> @@ -88,40 +88,49 @@ static int __create_pidfile(const char
>>>>>>>> *pidfile)
>>>>>>>> {
>>>>>>>> FILE *file = NULL;
>>>>>>>> int fd, pid, rc = 0;
>>>>>>>> + char pidfiletmp[256] = {0};
>>>>>>> [HansN] use NAME_MAX instead of 256
>>>>>>>> +
>>>>>>>> + sprintf(pidfiletmp, "%s.tmp", pidfile);
>>>>>>> [HansN] use snprintf and shouldn't pid be included in temp name?
>>>>>>>> /* open the file and associate a stream with it */
>>>>>>>> - if (((fd = open(pidfile, O_RDWR | O_CREAT, 0644)) == -1) ||
>>>>>>>> + if (((fd = open(pidfiletmp, O_RDWR | O_CREAT, 0644)) ==
>>>>>>>> -1) ||
>>>>>>>> ((file = fdopen(fd, "r+")) == NULL)) {
>>>>>>>> - syslog(LOG_ERR, "open failed, pidfile=%s, errno=%s",
>>>>>>>> pidfile,
>>>>>>>> - strerror(errno));
>>>>>>>> + syslog(LOG_ERR, "open failed, pidfiletmp=%s, errno=%s",
>>>>>>>> + pidfiletmp, strerror(errno));
>>>>>>> [HansN] close(fd) is missing
>>>>>>>> return -1;
>>>>>>>> }
>>>>>>>> /* Lock the file */
>>>>>>>> if (flock(fd, LOCK_EX | LOCK_NB) == -1) {
>>>>>>>> - syslog(LOG_ERR, "flock failed, pidfile=%s, errno=%s",
>>>>>>>> pidfile,
>>>>>>>> - strerror(errno));
>>>>>>>> + syslog(LOG_ERR, "flock failed, pidfiletmp=%s, errno=%s",
>>>>>>>> + pidfiletmp, strerror(errno));
>>>>>>>> fclose(file);
>>>>>>>> return -1;
>>>>>>>> }
>>>>>>>> pid = getpid();
>>>>>>>> if (!fprintf(file, "%d\n", pid)) {
>>>>>>>> - syslog(LOG_ERR, "fprintf failed, pidfile=%s, errno=%s",
>>>>>>>> pidfile,
>>>>>>>> - strerror(errno));
>>>>>>>> + syslog(LOG_ERR, "fprintf failed, pidfiletmp=%s,
>>>>>>>> errno=%s",
>>>>>>>> + pidfiletmp, strerror(errno));
>>>>>>>> fclose(file);
>>>>>>>> return -1;
>>>>>>>> }
>>>>>>>> fflush(file);
>>>>>>>> if (flock(fd, LOCK_UN) == -1) {
>>>>>>>> - syslog(LOG_ERR, "flock failed, pidfile=%s, errno=%s",
>>>>>>>> pidfile,
>>>>>>>> - strerror(errno));
>>>>>>>> + syslog(LOG_ERR, "flock failed, pidfiletmp=%s, errno=%s",
>>>>>>>> + pidfiletmp, strerror(errno));
>>>>>>>> fclose(file);
>>>>>>>> return -1;
>>>>>>>> }
>>>>>>>> fclose(file);
>>>>>>>> + if (rename(pidfiletmp, pidfile) != 0) {
>>>>>>>> + syslog(LOG_ERR, "rename failed, old=%s new=%s, error:%s",
>>>>>>>> + pidfiletmp, pidfile, strerror(errno));
>>>>>>>> + return -1;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> return rc;
>>>>>>>> }
>>>>>>>> diff --git a/src/base/file_notify.cc b/src/base/file_notify.cc
>>>>>>>> index e96be8b4a..80d1b7d54 100644
>>>>>>>> --- a/src/base/file_notify.cc
>>>>>>>> +++ b/src/base/file_notify.cc
>>>>>>>> @@ -50,7 +50,7 @@ FileNotify::FileNotifyErrors
>>>>>>>> FileNotify::WaitForFileCreation(
>>>>>>>> SplitFileName(file_name);
>>>>>>>> if ((inotify_wd_ = inotify_add_watch(inotify_fd_,
>>>>>>>> file_path_.c_str(),
>>>>>>>> - IN_CREATE)) == -1) {
>>>>>>>> + IN_CREATE | IN_MOVED_TO))
>>>>>>>> == -1) {
>>>>>>>> LOG_NO("inotify_add_watch failed: %s", strerror(errno));
>>>>>>>> return FileNotifyErrors::kError;
>>>>>>>> }
>>>>>>>> @@ -144,6 +144,14 @@ FileNotify::FileNotifyErrors
>>>>>>>> FileNotify::ProcessEvents(
>>>>>>>> return FileNotifyErrors::kOK;
>>>>>>>> }
>>>>>>>> }
>>>>>>>> + if (event->mask & IN_MOVED_TO) {
>>>>>>>> + if (file_name_ == event->name) {
>>>>>>>> + TRACE("file name: %s moved to %s",
>>>>>>>> file_name_.c_str(),
>>>>>>>> + file_path_.c_str());
>>>>>>>> + delete[] fds;
>>>>>>>> + return FileNotifyErrors::kOK;
>>>>>>>> + }
>>>>>>>> + }
>>>>>>>> if (event->mask & IN_IGNORED) {
>>>>>>>> TRACE("IN_IGNORE received, (ignored)");
>>>>>>>> }
>>>>>>
>>>>>>
>>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>>
>>> Check out the vibrant tech community on one of the world's most
>>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>>> _______________________________________________
>>> Opensaf-devel mailing list
>>> [email protected]
>>> https://lists.sourceforge.net/lists/listinfo/opensaf-devel
>>>
>>
>>
>>
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
