Re: [Qemu-devel] [PATCH 1/2] osdep: Add runtime OFD lock detection

[Eric Blake]

On 07/21/2017 05:20 AM, Fam Zheng wrote:
> Build time check of OFD lock is not sufficient and can cause image open
> errors when the runtime environment doesn't support it.
> 
> Add a helper function to probe it at runtime, additionally. Also provide
> a qemu_has_ofd_lock() for callers to check the status.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  include/qemu/osdep.h |  1 +
>  util/osdep.c         | 72 
> +++++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 64 insertions(+), 9 deletions(-)

>  
> +static void qemu_probe_lock_ops(void)
> +{
> +    if (fcntl_op_setlk == -1) {
> +#ifdef F_OFD_SETLK
> +        int fd;
> +        int ret;
> +        const char *tmpdir;
> +        char *filename;
> +        struct flock fl = {
> +            .l_whence = SEEK_SET,
> +            .l_start  = 0,
> +            .l_len    = 0,
> +            .l_type   = F_WRLCK,

Do we actually need to grab a lock over the entire file,...

> +        };
> +
> +        tmpdir = getenv("TMPDIR");
> +        if (!tmpdir) {
> +            tmpdir = "/var/tmp";
> +        }
> +        filename = g_strdup_printf("%s/qemu_lock_probe.XXXXXX", tmpdir);
> +        fd = mkstemp(filename);

Can we skip the temporary file, and just open("/dev/null", O_RDWR), for
slightly less work?

> +        if (fd < 0) {
> +            fprintf(stderr, "Failed to create temporary file '%s': %s\n",
> +                    filename, strerror(errno));
> +            fcntl_op_setlk = F_SETLK;
> +            fcntl_op_getlk = F_GETLK;
> +            goto out;
> +        }
> +        ret = fcntl(fd, F_OFD_SETLK, &fl);

...or should we change this to the much-weaker F_OFD_GETLK?  I guess it
doesn't matter if we use a temporary file, but does if we are trying to
simplify by using /dev/null.

At any rate, the rest of the patch looks sane.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature