Re: [Qemu-devel] [PATCH v4 08/27] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Daniel P. Berrange]

On Tue, May 10, 2016 at 10:50:40AM +0800, Fam Zheng wrote:
> They are wrappers of POSIX fcntl file locking, with the additional
> interception of open/close (through qemu_open and qemu_close) to offer a
> better semantics that preserves the locks across multiple life cycles of
> different fds on the same file.  The reason to make this semantics
> change over the fcntl locks is to make the API cleaner for QEMU
> subsystems.
> 
> More precisely, we remove this "feature" of fcntl lock:
> 
>     If a process closes any file descriptor referring to a file, then
>     all of the process's locks on that file are released, regardless of
>     the file descriptor(s) on which the locks were obtained.
> 
> as long as the fd is always open/closed through qemu_open and
> qemu_close.

You're not actually really removing that problem - this is just hacking
around it in a manner which is racy & susceptible to silent failure.


> +static int qemu_fd_close(int fd)
> +{
> +    LockEntry *ent, *tmp;
> +    LockRecord *rec;
> +    char *path;
> +    int ret;
> +
> +    assert(fd_to_path);
> +    path = g_hash_table_lookup(fd_to_path, GINT_TO_POINTER(fd));
> +    assert(path);
> +    g_hash_table_remove(fd_to_path, GINT_TO_POINTER(fd));
> +    rec = g_hash_table_lookup(lock_map, path);
> +    ret = close(fd);
> +
> +    if (rec) {
> +        QLIST_FOREACH_SAFE(ent, &rec->records, next, tmp) {
> +            int r;
> +            if (ent->fd == fd) {
> +                QLIST_REMOVE(ent, next);
> +                g_free(ent);
> +                continue;
> +            }
> +            r = qemu_lock_do(ent->fd, ent->start, ent->len,
> +                             ent->readonly ? F_RDLCK : F_WRLCK);
> +            if (r == -1) {
> +                fprintf(stderr, "Failed to acquire lock on fd %d: %s\n",
> +                        ent->fd, strerror(errno));
> +            }

If another app has acquired the lock between the close and this attempt
to re-acquire the lock, then QEMU is silently carrying on without any
lock being held. For something that's intending to provide protection
against concurrent use I think this is not an acceptable failure
scenario.


> +int qemu_open(const char *name, int flags, ...)
> +{
> +    int mode = 0;
> +    int ret;
> +
> +    if (flags & O_CREAT) {
> +        va_list ap;
> +
> +        va_start(ap, flags);
> +        mode = va_arg(ap, int);
> +        va_end(ap);
> +    }
> +    ret = qemu_fd_open(name, flags, mode);
> +    if (ret >= 0) {
> +        qemu_fd_add_record(ret, name);
> +    }
> +    return ret;
> +}

I think the approach you have here is fundamentally not usable with
fcntl locks, because it is still using the pattern

   fd = open(path)
   lock(fd)
   if failed lock
      close(fd)
   ...do stuff.


As mentioned in previous posting I believe the block layer must be
checking whether it already has a lock held against "path" *before*
even attempting to open the file. Once QEMU has the file descriptor
open it is too later, because closing the FD will release pre-existing
locks QEMU has.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|