Re: [PATCH v1 02/10] libxl_internal: functions to lock / unlock domain configuration

[Wei Liu <wei.liu2@citrix.com>]

On Thu, Jul 24, 2014 at 07:24:10PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH v1 02/10] libxl_internal: functions to lock / unlock domain configuration"):
> > Simple file lock taken from xl to serialise access to "libxl-json" file.
> > If a thread cannot get hold of the lock it waits due to F_SETLKW.
> 
> Right.
> 
> > In order to generate lock file name, rename userdata_path to
> > libxl__userdata_path and declare it in libxl_internal.h
> 
> I don't mind it in such a small patch but in general it is easier to
> review things if non-functional changes like this are split out into a
> separate patch.
> 

Ack.

> > +int libxl__lock_domain_configuration(libxl__gc *gc, uint32_t domid,
> > +                                     int *fd_lock)
> > +{
> ...
> > +    int rc;
> > +    struct flock fl;
> > +    const char *lockfile;
> > +
> > +    if (*fd_lock >= 0)
> > +        return ERROR_INVAL;
> 
> Why not assert() ?
> 

Ack.

> > +    lockfile = libxl__userdata_path(gc, domid, "libxl-json.lock", "d");
> 
> Perhaps lockfile = ...(, "libxl-json", "l") ?  I think users of
> libxl__userdata_path are entitled to invent their own `wh' values.
> 

Good to know.

> Otherwise you have to document "libxl-json.lock" as a reserved
> userdata name (which is a bit daft because no-one would use it, but it
> is, formally speaking, wrong to use it here).
> 

I think I will go with the "l" approach.

> > +    *fd_lock = open(lockfile, O_WRONLY|O_CREAT, S_IWUSR);
> > +    if (*fd_lock < 0) {
> > +        LOGE(ERROR, "cannot open lockfile %s errno=%d\n", lockfile, errno);
> 
> LOGE's message should not contain \n.
> 

Fixed.

> > +    if (fcntl(*fd_lock, F_SETFD, FD_CLOEXEC) < 0) {
> 
> What's wrong with libxl_fd_set_cloexec ?
> 

Will use this one.

This implementation was basically a copy of the one in xl, I didn't
check if there's some helper function to do that.

> > +        close(*fd_lock);
> 
> Please use the idempotent `goto out' error handling style to deal with
> closing the fd on error.  Your failure to do so has resulted in
> error-case fd leak in this function.
> 

Ack.

> > +get_lock:
> > +    fl.l_type = F_WRLCK;
> > +    fl.l_whence = SEEK_SET;
> > +    fl.l_start = 0;
> > +    fl.l_len = 0;
> > +    rc = fcntl(*fd_lock, F_SETLKW, &fl);
> > +    if (rc < 0 && errno == EINTR)
> > +        goto get_lock;
> 
> Please, no more of these `goto'-based loops!
> 

Ack.

> > +    if (rc < 0) {
> > +        LOGE(ERROR, "cannot acquire lock %s errno=%d\n", lockfile, errno);
> > +        rc = ERROR_FAIL;
> 
> goto out.
> 
> > +    } else
> > +        rc = 0;
> 
> No, not like that.  Like this:
> 
>      rc = 0;
>      return rc;
> 
>    out:
>      if (*fd_lock >= 0) { close(*fd_lock); *fd_lock = -1; }
>      return rc;
> 
> 

Will fix this.

> > +int libxl__unlock_domain_configuration(libxl__gc *gc, uint32_t domid,
> > +                                       int *fd_lock)
> > +{
> 
> Closing the fd is sufficient.  I'm not even sure why you need a whole
> function for this; the caller could just call close().  The caller can
> can ignore any errors (which I think are impossible anyway) since
> after close the fd is gone anyway.
> 
> > +/*
> > + * Lock / unlock domain configuration in libxl private data store.
> > + * fd_lock contains the file descriptor pointing to the lock file.
> > + */
> > +int libxl__lock_domain_configuration(libxl__gc *gc, uint32_t domid,
> > +                                     int *fd_lock);
> 
> You need to explain the lifetime semantics of *fd_lock.  Your code
> demands that the caller set it to -1 beforehand (which is fine).
> 

Ack.

> > +int libxl__unlock_domain_configuration(libxl__gc *gc, uint32_t domid,
> > +                                       int *fd_lock);
> 
> If you do retain this as a separate function, it should return void.
> I can think of nothing useful that the caller could do with an error
> from it.
> 

I will retain this as a separate function so that it looks symmetric to
__lock.

Wei.

> Ian.