[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv7 04/18] util: Add interface to determine monitor path





On 2018年11月06日 01:19, John Ferlan wrote:

On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Add interface for resctrl monitor to determine the path.

Signed-off-by: Wang Huaqiang <huaqiang wang intel com>
---
  src/libvirt_private.syms |  1 +
  src/util/virresctrl.c    | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
  src/util/virresctrl.h    |  5 ++++-
  3 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d2573c5..5235046 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2680,6 +2680,7 @@ virResctrlInfoGetCache;
  virResctrlInfoGetMonitorPrefix;
  virResctrlInfoMonFree;
  virResctrlInfoNew;
+virResctrlMonitorDeterminePath;
  virResctrlMonitorNew;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 956aca8..1d0eb01 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2462,3 +2462,58 @@ virResctrlMonitorNew(void)
return virObjectNew(virResctrlMonitorClass);
  }
+
+
+/*
+ * virResctrlMonitorDeterminePath
+ *
+ * @monitor: Pointer to a resctrl monitor
+ * @machinename: Name string of the VM
+ *
+ * Determines the directory path that the underlying resctrl group will be
+ * created with.
+ *
+ * A monitor represents a directory under resource control file system,
+ * its directory path could be the same path as @monitor->alloc, could be a
+ * path of directory under 'mon_groups' of @monitor->alloc, or a path of
+ * directory under '/sys/fs/resctrl/mon_groups' if @monitor->alloc is NULL.
+ *
+ * Returns 0 on success, -1 on error.
+ */
+int
+virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
+                               const char *machinename)
+{
+    VIR_AUTOFREE(char *) parentpath = NULL;
+
+    if (!monitor) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Invalid resctrl monitor"));
+        return -1;
+    }
+
+    if (monitor->path) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Resctrl monitor path is already set to '%s'"),
+                       monitor->path);
+        return -1;
+    }
+
+    if (monitor->id && monitor->alloc && monitor->alloc->id) {
+        if (STREQ(monitor->id, monitor->alloc->id)) {
+            if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)
+                return -1;
+            return 0;
+        }
+    }
+
+    if (virAsprintf(&parentpath, "%s/mon_groups", monitor->alloc->path) < 0)
Just ran the changes through Coverity - because of the above
"monitor->alloc" check, Coverity notes right here that monitor->alloc
could be NULL, so I think a check prior to here would be in order,

Yes. Here exists a risk that this line you mentioned could be executed at
the condition that @monitor->alloc is empty pointer, this will cause a crash.

such
as either before or after the monitor->path check:

     if (!monitor->alloc) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Missing resctrl monitor allocation"));
         return -1;
     }

Putting these lines before and after the monitor->path check should be OK for me.
(I don't find the influence made by the order. )


Then the monitor->alloc check wouldn't be necessary. Thus the above
STRDUP becomes:

     if (STREQ_NULLABLE(monitor->id, monitor->alloc->id)) {
         if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)
             return -1;
         return 0;
     }

Agree.
Thanks.


Let me know where you want it.

John

Thanks for review.
Huaqiang


+        return -1;
+
+    monitor->path = virResctrlDeterminePath(parentpath, machinename,
+                                            monitor->id);
+    if (!monitor->path)
+        return -1;
+
+    return 0;
+}
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index eaa077d..baae554 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -191,7 +191,10 @@ virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl,
  typedef struct _virResctrlMonitor virResctrlMonitor;
  typedef virResctrlMonitor *virResctrlMonitorPtr;
-
  virResctrlMonitorPtr
  virResctrlMonitorNew(void);
+
+int
+virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
+                               const char *machinename);
  #endif /*  __VIR_RESCTRL_H__ */



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]