Re: [PATCH v3 2/3] util: Get rid of virFileFlock()

[Andrea Bolognani <abologna@xxxxxxxxxx>]

On Wed, 2020-07-29 at 13:43 +0200, Martin Kletzander wrote:
> +++ b/src/util/virresctrl.c
> @@ -456,6 +456,8 @@ VIR_ONCE_GLOBAL_INIT(virResctrl);
>  static int
>  virResctrlLockWrite(void)
>  {
> +#ifndef WIN32
> +
>      int fd = open(SYSFS_RESCTRL_PATH, O_RDONLY | O_CLOEXEC);
>  
>      if (fd < 0) {
> @@ -463,13 +465,20 @@ virResctrlLockWrite(void)
>          return -1;
>      }
>  
> -    if (virFileFlock(fd, true, false) < 0) {
> +    if (flock(fd, LOCK_EX) < 0) {
>          virReportSystemError(errno, "%s", _("Cannot lock resctrl"));
>          VIR_FORCE_CLOSE(fd);
>          return -1;
>      }
>  
>      return fd;
> +
> +#else /* WIN32 */
> +
> +    virReportSystemError(ENOSYS, "%s", _("Cannot lock resctrl"));
> +    return -1;
> +
> +#endif
>  }
>  
>  
> @@ -484,10 +493,14 @@ virResctrlUnlock(int fd)
>      if (VIR_CLOSE(fd) < 0) {
>          virReportSystemError(errno, "%s", _("Cannot close resctrl"));
>  
> +#ifndef WIN32
> +
>          /* Trying to save the already broken */
> -        if (virFileFlock(fd, false, false) < 0)
> +        if (flock(fd, LOCK_UN) < 0)
>              virReportSystemError(errno, "%s", _("Cannot unlock resctrl"));
>  
> +#endif
> +
>          return -1;
>      }

So in the end you decided to go for the nuclear option :D

I'm okay with the approach, but I would prefer if you stubbed out the
functions completely, eg.

  #ifndef WIN32
  
  static int
  virResctrlLockWrite(void)
  {
      /* do stuff */
  }
  
  static int
  virResctrlUnlock(int fd)
  {
      /* do stuff */
  }
  
  #else
  
  static int
  virResctrlLockWrite(void)
  {
      virReportSystemError(ENOSYS, "%s",
                           _("resctrl locking is not supported "
                             "on this platform"));
      return -1;
  }
  
  static int
  virResctrlUnlock(int fd)
  {
      virReportSystemError(ENOSYS, "%s",
                           _("resctrl locking is not supported "
                             "on this platform"));
      return -1;
  }
  
  #endif

Also, since AFAIU resctrl is Linux-only, perhaps a better
preprocessor guard would be

  #ifdef __linux__

so that we (correctly) stub the functions out on FreeBSD and macOS
too.

-- 
Andrea Bolognani / Red Hat / Virtualization