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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

> +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() ?

> +    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.

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).

> +    *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.

> +    if (fcntl(*fd_lock, F_SETFD, FD_CLOEXEC) < 0) {

What's wrong with libxl_fd_set_cloexec ?

> +        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.

> +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!

> +    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;


> +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).

> +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.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel