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
