diff mbox

[1/2] osdep: Add runtime OFD lock detection

Message ID 20170721102059.14663-2-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng July 21, 2017, 10:20 a.m. UTC 
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 <famz@redhat.com>
---
 include/qemu/osdep.h |  1 +
 util/osdep.c         | 72 +++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 64 insertions(+), 9 deletions(-)

Comments

Eric Blake July 21, 2017, 12:30 p.m. UTC | #1 
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 <famz@redhat.com>
> ---
>  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.
diff mbox

Patch

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 3b74f6fcb2..6855b94bbf 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -357,6 +357,7 @@  int qemu_dup(int fd);
 int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
 int qemu_unlock_fd(int fd, int64_t start, int64_t len);
 int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
+bool qemu_has_ofd_lock(void);
 
 #if defined(__HAIKU__) && defined(__i386__)
 #define FMT_pid "%ld"
diff --git a/util/osdep.c b/util/osdep.c
index a2863c8e53..b275c6579a 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -38,13 +38,8 @@  extern int madvise(caddr_t, size_t, int);
 #include "qemu/error-report.h"
 #include "monitor/monitor.h"
 
-#ifdef F_OFD_SETLK
-#define QEMU_SETLK F_OFD_SETLK
-#define QEMU_GETLK F_OFD_GETLK
-#else
-#define QEMU_SETLK F_SETLK
-#define QEMU_GETLK F_GETLK
-#endif
+static int fcntl_op_setlk = -1;
+static int fcntl_op_getlk = -1;
 
 static bool fips_enabled = false;
 
@@ -149,6 +144,63 @@  static int qemu_parse_fdset(const char *param)
     return qemu_parse_fd(param);
 }
 
+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,
+        };
+
+        tmpdir = getenv("TMPDIR");
+        if (!tmpdir) {
+            tmpdir = "/var/tmp";
+        }
+        filename = g_strdup_printf("%s/qemu_lock_probe.XXXXXX", tmpdir);
+        fd = mkstemp(filename);
+        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);
+        close(fd);
+        unlink(filename);
+        if (!ret) {
+            fcntl_op_setlk = F_OFD_SETLK;
+            fcntl_op_getlk = F_OFD_GETLK;
+        } else {
+            fcntl_op_setlk = F_SETLK;
+            fcntl_op_getlk = F_GETLK;
+        }
+out:
+        g_free(filename);
+#else
+        fcntl_op_setlk = F_SETLK;
+        fcntl_op_getlk = F_GETLK;
+#endif
+    }
+}
+
+bool qemu_has_ofd_lock(void)
+{
+    qemu_probe_lock_ops();
+#ifdef F_OFD_SETLK
+    return fcntl_op_setlk == F_OFD_SETLK;
+#else
+    return false;
+#endif
+}
+
 static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
 {
     int ret;
@@ -158,7 +210,8 @@  static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
         .l_len    = len,
         .l_type   = fl_type,
     };
-    ret = fcntl(fd, QEMU_SETLK, &fl);
+    qemu_probe_lock_ops();
+    ret = fcntl(fd, fcntl_op_setlk, &fl);
     return ret == -1 ? -errno : 0;
 }
 
@@ -181,7 +234,8 @@  int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
         .l_len    = len,
         .l_type   = exclusive ? F_WRLCK : F_RDLCK,
     };
-    ret = fcntl(fd, QEMU_GETLK, &fl);
+    qemu_probe_lock_ops();
+    ret = fcntl(fd, fcntl_op_getlk, &fl);
     if (ret == -1) {
         return -errno;
     } else {