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

29 Jul 2020


      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

    