Re: [PATCH v2 1/2] util: Rework virFileFlock() to be unambiguous

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Fri, Jul 10, 2020 at 09:45:25PM +0200, Martin Kletzander wrote:
> The boolean parameters for lock/unlock and read/write (shared/exclusive) caused
> a lot of confusion when reading the callers.  The new approach is explicit and
> unambiguous.
>
> Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> Reviewed-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> ---
> src/util/virfile.c    | 27 ++++++++++++++++++---------
> src/util/virfile.h    |  9 ++++++++-
> src/util/virresctrl.c |  4 ++--
> 3 files changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 213acdbcaa2b..554011eecb25 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -465,23 +465,33 @@ int virFileUnlock(int fd, off_t start, off_t len)
> /**
>  * virFileFlock:
>  * @fd: file descriptor to call flock on
> - * @lock: true for lock, false for unlock
> - * @shared: true if shared, false for exclusive, ignored if `@lock == false`
> + * @op: operation to perform
>  *
>  * This is just a simple wrapper around flock(2) that errors out on unsupported
>  * platforms.
>  *
>  * The lock will be released when @fd is closed or this function is called with
> - * `@lock == false`.
> + * `@op == VIR_FILE_FLOCK_UNLOCK`.
>  *
>  * Returns 0 on success, -1 otherwise (with errno set)
>  */
> -int virFileFlock(int fd, bool lock, bool shared)
> +int virFileFlock(int fd, virFileFlockOperation op)
> {
> -    if (lock)
> -        return flock(fd, shared ? LOCK_SH : LOCK_EX);
> +    int flock_op = -1;
>
> -    return flock(fd, LOCK_UN);
> +    switch (op) {
> +    case VIR_FILE_FLOCK_SHARED:
> +        flock_op = LOCK_SH;
> +        break;
> +    case VIR_FILE_FLOCK_EXCLUSIVE:
> +        flock_op = LOCK_EX;
> +        break;
> +    case VIR_FILE_FLOCK_UNLOCK:
> +        flock_op = LOCK_UN;
> +        break;

Yes, I know I forgot to add the default branch and it complicates matters even
more because it would set an error in a function that just sets errno...  I'll
send a v3.

> +    }
> +
> +    return flock(fd, flock_op);
> }
>
> #else /* WIN32 */
> @@ -505,8 +515,7 @@ int virFileUnlock(int fd G_GNUC_UNUSED,
>
>
> int virFileFlock(int fd G_GNUC_UNUSED,
> -                 bool lock G_GNUC_UNUSED,
> -                 bool shared G_GNUC_UNUSED)
> +                 virFileFlockOperation op G_GNUC_UNUSED)
> {
>     errno = ENOSYS;
>     return -1;
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 7a92364a5c9f..04428727fd3b 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -118,7 +118,14 @@ int virFileLock(int fd, bool shared, off_t start, off_t len, bool waitForLock)
> int virFileUnlock(int fd, off_t start, off_t len)
>     G_GNUC_NO_INLINE;
>
> -int virFileFlock(int fd, bool lock, bool shared);
> +
> +typedef enum {
> +    VIR_FILE_FLOCK_EXCLUSIVE,
> +    VIR_FILE_FLOCK_SHARED,
> +    VIR_FILE_FLOCK_UNLOCK,
> +} virFileFlockOperation;
> +
> +int virFileFlock(int fd, virFileFlockOperation op);
>
> typedef int (*virFileRewriteFunc)(int fd, const void *opaque);
> int virFileRewrite(const char *path,
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 3563fc560db5..b685b8bb6f7b 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -463,7 +463,7 @@ virResctrlLockWrite(void)
>         return -1;
>     }
>
> -    if (virFileFlock(fd, true, true) < 0) {
> +    if (virFileFlock(fd, VIR_FILE_FLOCK_SHARED) < 0) {
>         virReportSystemError(errno, "%s", _("Cannot lock resctrl"));
>         VIR_FORCE_CLOSE(fd);
>         return -1;
> @@ -485,7 +485,7 @@ virResctrlUnlock(int fd)
>         virReportSystemError(errno, "%s", _("Cannot close resctrl"));
>
>         /* Trying to save the already broken */
> -        if (virFileFlock(fd, false, false) < 0)
> +        if (virFileFlock(fd, VIR_FILE_FLOCK_UNLOCK) < 0)
>             virReportSystemError(errno, "%s", _("Cannot unlock resctrl"));
>
>         return -1;
> --
> 2.27.0

Attachment: signature.asc
Description: PGP signature