Hi Minh, You can ignore my comment on pid in temp filename below. /Thanks HansN
-----Original Message----- From: Hans Nordebäck Sent: den 2 maj 2017 15:11 To: Minh Hon Chau <[email protected]>; Anders Widell <[email protected]>; [email protected] Cc: [email protected]; Hans Nordebäck <[email protected]> Subject: Re: [PATCH 1/1] base: Make pid file safe to read by rename it from temporary created file [#2432] 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
