[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/5] Add some APIs which use locking for crashsafe pidfile handling



On Wed, Aug 10, 2011 at 16:37:27 +0100, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> In daemons using pidfiles to protect against concurrent
> execution there is a possibility that a crash may leave a stale
> pidfile on disk, which then prevents later restart of the daemon.
> 
> To avoid this problem, introduce a pair of APIs which make
> use of virFileLock to ensure crash-safe & race condition-safe
> pidfile acquisition & releae
> 
> * src/libvirt_private.syms, src/util/virpidfile.c,
>   src/util/virpidfile.h: Add virPidFileAcquire and virPidFileRelease
> ---
>  po/POTFILES.in           |    1 +
>  src/libvirt_private.syms |    4 +
>  src/util/virpidfile.c    |  142 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virpidfile.h    |   12 ++++
>  4 files changed, 159 insertions(+), 0 deletions(-)
> 
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 7bb08b0..b200f89 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -123,6 +123,7 @@ src/util/sysinfo.c
>  src/util/util.c
>  src/util/viraudit.c
>  src/util/virfile.c
> +src/util/virpidfile.c
>  src/util/virterror.c
>  src/util/xml.c
>  src/vbox/vbox_MSCOMGlue.c
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index fee3520..64b91ee 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1120,11 +1120,15 @@ virFileFdopen;
>  
>  
>  # virpidfile.h
> +virPidFileAcquire;
> +virPidFileAcquirePath;
>  virPidFileBuildPath;
>  virPidFileRead;
>  virPidFileReadIfAlive;
>  virPidFileReadPath;
>  virPidFileReadPathIfAlive;
> +virPidFileRelease;
> +virPidFileReleasePath;
>  virPidFileWrite;
>  virPidFileWritePath;
>  virPidFileDelete;
> diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
> index dc92868..005c95d 100644
> --- a/src/util/virpidfile.c
> +++ b/src/util/virpidfile.c
> @@ -25,11 +25,15 @@
>  
>  #include <fcntl.h>
>  #include <signal.h>
> +#include <sys/stat.h>
>  
>  #include "virpidfile.h"
>  #include "virfile.h"
>  #include "memory.h"
>  #include "util.h"
> +#include "intprops.h"
> +#include "logging.h"
> +#include "virterror_internal.h"
>  
>  
>  char *virPidFileBuildPath(const char *dir, const char* name)
> @@ -255,3 +259,141 @@ cleanup:
>      VIR_FREE(pidfile);
>      return rc;
>  }
> +
> +
> +
> +int virPidFileAcquirePath(const char *path,
> +                          pid_t pid)
> +{
> +    int fd = -1;
> +    char ebuf[1024];
> +    char pidstr[INT_BUFSIZE_BOUND(pid)];
> +    verify(sizeof(pid_t) <= sizeof(unsigned int));
> +
> +    if (path[0] == '\0')
> +        return 0;
> +
> +    while (1) {
> +        struct stat a, b;
> +        if ((fd = open(path, O_WRONLY|O_CREAT, 0644)) < 0) {
> +            VIR_ERROR(_("Failed to open pid file '%s' : %s"),
> +                      path, virStrerror(errno, ebuf, sizeof ebuf));

Hmm, why not virReportSystemError(errno, ...) everywhere instead of VIR_ERROR
and virStrerror?

Also I think we should call virSetCloseExec on the file descriptor for a
little bit of extra safety.

...

ACK with the comments addressed.

Jirka


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]