diff mbox

[v6,05/22] osdep: Add qemu_lock_fd and qemu_unlock_fd

Message ID 1464943756-14143-6-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng June 3, 2016, 8:48 a.m. UTC 
They are wrappers of POSIX fcntl "file private locking".

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/qemu/osdep.h |  2 ++
 util/osdep.c         | 29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

Comments

Kevin Wolf June 17, 2016, 12:12 p.m. UTC | #1 
Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> They are wrappers of POSIX fcntl "file private locking".
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  include/qemu/osdep.h |  2 ++
>  util/osdep.c         | 29 +++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 6937694..749214a 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -280,6 +280,8 @@ int qemu_madvise(void *addr, size_t len, int advice);
>  
>  int qemu_open(const char *name, int flags, ...);
>  int qemu_close(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);
>  
>  #if defined(__HAIKU__) && defined(__i386__)
>  #define FMT_pid "%ld"
> diff --git a/util/osdep.c b/util/osdep.c
> index 9a7a439..085ed52 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -137,6 +137,35 @@ static int qemu_parse_fdset(const char *param)
>  {
>      return qemu_parse_fd(param);
>  }
> +
> +static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
> +{
> +#ifdef F_OFD_SETLK
> +    int ret;
> +    struct flock fl = {
> +        .l_whence = SEEK_SET,
> +        .l_start  = start,
> +        .l_len    = len,
> +        .l_type   = fl_type,
> +    };
> +    do {
> +        ret = fcntl(fd, F_OFD_SETLK, &fl);
> +    } while (ret == -1 && errno == EINTR);
> +    return ret == -1 ? -errno : 0;
> +#else
> +    return -ENOTSUP;
> +#endif
> +}

This will return -ENOTSUP in the case that the function wasn't available
at build time, but -EINVAL if it was available at build time but the
kernel doesn't support it at runtime. Should we unify this?

Kevin
Fam Zheng June 22, 2016, 7:34 a.m. UTC | #2 
On Fri, 06/17 14:12, Kevin Wolf wrote:
> Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > They are wrappers of POSIX fcntl "file private locking".
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  include/qemu/osdep.h |  2 ++
> >  util/osdep.c         | 29 +++++++++++++++++++++++++++++
> >  2 files changed, 31 insertions(+)
> > 
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 6937694..749214a 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -280,6 +280,8 @@ int qemu_madvise(void *addr, size_t len, int advice);
> >  
> >  int qemu_open(const char *name, int flags, ...);
> >  int qemu_close(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);
> >  
> >  #if defined(__HAIKU__) && defined(__i386__)
> >  #define FMT_pid "%ld"
> > diff --git a/util/osdep.c b/util/osdep.c
> > index 9a7a439..085ed52 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -137,6 +137,35 @@ static int qemu_parse_fdset(const char *param)
> >  {
> >      return qemu_parse_fd(param);
> >  }
> > +
> > +static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
> > +{
> > +#ifdef F_OFD_SETLK
> > +    int ret;
> > +    struct flock fl = {
> > +        .l_whence = SEEK_SET,
> > +        .l_start  = start,
> > +        .l_len    = len,
> > +        .l_type   = fl_type,
> > +    };
> > +    do {
> > +        ret = fcntl(fd, F_OFD_SETLK, &fl);
> > +    } while (ret == -1 && errno == EINTR);
> > +    return ret == -1 ? -errno : 0;
> > +#else
> > +    return -ENOTSUP;
> > +#endif
> > +}
> 
> This will return -ENOTSUP in the case that the function wasn't available
> at build time, but -EINVAL if it was available at build time but the
> kernel doesn't support it at runtime. Should we unify this?

I'm not sure we can reliably distinguish "fcntl cmd not supported" and "fcntl
cmd supported but parameters have invalid value" out of -EINVAL.

Quoting the manual:

> https://www.gnu.org/software/libc/manual/html_node/Open-File-Description-Locks.html
> 
> Macro: int F_OFD_SETL
> 
>     EINVAL
> 
>     Either the lockp argument doesn’t specify valid lock information, the
>     operating system kernel doesn’t support open file description locks, or the
>     file associated with filedes doesn’t support locks.
> 

I'd expect the user to know what he's doing if he is using a kernel that is
much older than the one built QEMU, since this is relatively a very uncommon
thing to do.

Fam
Kevin Wolf June 22, 2016, 8:34 a.m. UTC | #3 
Am 22.06.2016 um 09:34 hat Fam Zheng geschrieben:
> On Fri, 06/17 14:12, Kevin Wolf wrote:
> > Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > > They are wrappers of POSIX fcntl "file private locking".
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  include/qemu/osdep.h |  2 ++
> > >  util/osdep.c         | 29 +++++++++++++++++++++++++++++
> > >  2 files changed, 31 insertions(+)
> > > 
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index 6937694..749214a 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -280,6 +280,8 @@ int qemu_madvise(void *addr, size_t len, int advice);
> > >  
> > >  int qemu_open(const char *name, int flags, ...);
> > >  int qemu_close(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);
> > >  
> > >  #if defined(__HAIKU__) && defined(__i386__)
> > >  #define FMT_pid "%ld"
> > > diff --git a/util/osdep.c b/util/osdep.c
> > > index 9a7a439..085ed52 100644
> > > --- a/util/osdep.c
> > > +++ b/util/osdep.c
> > > @@ -137,6 +137,35 @@ static int qemu_parse_fdset(const char *param)
> > >  {
> > >      return qemu_parse_fd(param);
> > >  }
> > > +
> > > +static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
> > > +{
> > > +#ifdef F_OFD_SETLK
> > > +    int ret;
> > > +    struct flock fl = {
> > > +        .l_whence = SEEK_SET,
> > > +        .l_start  = start,
> > > +        .l_len    = len,
> > > +        .l_type   = fl_type,
> > > +    };
> > > +    do {
> > > +        ret = fcntl(fd, F_OFD_SETLK, &fl);
> > > +    } while (ret == -1 && errno == EINTR);
> > > +    return ret == -1 ? -errno : 0;
> > > +#else
> > > +    return -ENOTSUP;
> > > +#endif
> > > +}
> > 
> > This will return -ENOTSUP in the case that the function wasn't available
> > at build time, but -EINVAL if it was available at build time but the
> > kernel doesn't support it at runtime. Should we unify this?
> 
> I'm not sure we can reliably distinguish "fcntl cmd not supported" and "fcntl
> cmd supported but parameters have invalid value" out of -EINVAL.

Well, the other option is returning -EINVAL instead of -ENOTSUP in the
#else branch.

> Quoting the manual:
> 
> > https://www.gnu.org/software/libc/manual/html_node/Open-File-Description-Locks.html
> > 
> > Macro: int F_OFD_SETL
> > 
> >     EINVAL
> > 
> >     Either the lockp argument doesn’t specify valid lock information, the
> >     operating system kernel doesn’t support open file description locks, or the
> >     file associated with filedes doesn’t support locks.
> > 
> 
> I'd expect the user to know what he's doing if he is using a kernel that is
> much older than the one built QEMU, since this is relatively a very uncommon
> thing to do.

I'm talking about possible bugs if a caller of this function is checking
for -ENOTSUP, it's easy to forget -EINVAL there. Fixing qemu isn't one
of the things that we should expect even from a "user who knows what
he's doing".

Kevin
Fam Zheng June 22, 2016, 9:10 a.m. UTC | #4 
On Wed, 06/22 10:34, Kevin Wolf wrote:
> > > This will return -ENOTSUP in the case that the function wasn't available
> > > at build time, but -EINVAL if it was available at build time but the
> > > kernel doesn't support it at runtime. Should we unify this?
> > 
> > I'm not sure we can reliably distinguish "fcntl cmd not supported" and "fcntl
> > cmd supported but parameters have invalid value" out of -EINVAL.
> 
> Well, the other option is returning -EINVAL instead of -ENOTSUP in the
> #else branch.
> 
> > Quoting the manual:
> > 
> > > https://www.gnu.org/software/libc/manual/html_node/Open-File-Description-Locks.html
> > > 
> > > Macro: int F_OFD_SETL
> > > 
> > >     EINVAL
> > > 
> > >     Either the lockp argument doesn’t specify valid lock information, the
> > >     operating system kernel doesn’t support open file description locks, or the
> > >     file associated with filedes doesn’t support locks.
> > > 
> > 
> > I'd expect the user to know what he's doing if he is using a kernel that is
> > much older than the one built QEMU, since this is relatively a very uncommon
> > thing to do.
> 
> I'm talking about possible bugs if a caller of this function is checking
> for -ENOTSUP, it's easy to forget -EINVAL there. Fixing qemu isn't one
> of the things that we should expect even from a "user who knows what
> he's doing".

Calling function should interprete -ENOTSUP as "not available at build time",
and -EINVAL as any one of the three reasons reported by kernel.

If we return -EINVAL here in the #else branch, the "not available at build
time" is not obvious.  But we intentionally made locking a "nop" if the syscall
is not supported.  Why confuse that with "invalid locking parameter" case?

Fam
diff mbox

Patch

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 6937694..749214a 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -280,6 +280,8 @@  int qemu_madvise(void *addr, size_t len, int advice);
 
 int qemu_open(const char *name, int flags, ...);
 int qemu_close(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);
 
 #if defined(__HAIKU__) && defined(__i386__)
 #define FMT_pid "%ld"
diff --git a/util/osdep.c b/util/osdep.c
index 9a7a439..085ed52 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -137,6 +137,35 @@  static int qemu_parse_fdset(const char *param)
 {
     return qemu_parse_fd(param);
 }
+
+static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
+{
+#ifdef F_OFD_SETLK
+    int ret;
+    struct flock fl = {
+        .l_whence = SEEK_SET,
+        .l_start  = start,
+        .l_len    = len,
+        .l_type   = fl_type,
+    };
+    do {
+        ret = fcntl(fd, F_OFD_SETLK, &fl);
+    } while (ret == -1 && errno == EINTR);
+    return ret == -1 ? -errno : 0;
+#else
+    return -ENOTSUP;
+#endif
+}
+
+int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive)
+{
+    return qemu_lock_fcntl(fd, start, len, exclusive ? F_WRLCK : F_RDLCK);
+}
+
+int qemu_unlock_fd(int fd, int64_t start, int64_t len)
+{
+    return qemu_lock_fcntl(fd, start, len, F_UNLCK);
+}
 #endif
 
 /*