[libvirt] [PATCH V3] util: Add more virsysfs functions for handling resctrl sysfs
Extended /sys/fs/resctrl sysfs handling such as: Read string/uint. Write string. Create/remove directory. All these operations will be while we are enabled CAT feature later. Signed-off-by: Eli Qiao <liyong.qiao@intel.com> --- src/libvirt_private.syms | 9 +++ src/util/virsysfs.c | 143 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virsysfs.h | 26 +++++++++ src/util/virsysfspriv.h | 1 + 4 files changed, 179 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b551cb8..f6644cb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2620,17 +2620,26 @@ virVasprintfInternal; # util/virsysfs.h +virSysfsCreateResCtrlDir; virSysfsGetCpuValueBitmap; virSysfsGetCpuValueInt; virSysfsGetCpuValueString; virSysfsGetCpuValueUint; virSysfsGetNodeValueBitmap; virSysfsGetNodeValueString; +virSysfsGetResctrlInfoString; +virSysfsGetResctrlInfoUint; +virSysfsGetResctrlPath; +virSysfsGetResctrlString; +virSysfsGetResctrlUint; virSysfsGetSystemPath; virSysfsGetValueBitmap; virSysfsGetValueInt; virSysfsGetValueString; +virSysfsRemoveResCtrlDir; +virSysfsSetResctrlPath; virSysfsSetSystemPath; +virSysfsWriteResctrlString; # util/virsysinfo.h diff --git a/src/util/virsysfs.c b/src/util/virsysfs.c index 7a98b48..a17c391 100644 --- a/src/util/virsysfs.c +++ b/src/util/virsysfs.c @@ -19,6 +19,7 @@ */ #include <config.h> +#include <fcntl.h> #include "internal.h" @@ -36,8 +37,10 @@ VIR_LOG_INIT("util.sysfs"); #define VIR_SYSFS_VALUE_MAXLEN 8192 #define SYSFS_SYSTEM_PATH "/sys/devices/system" +#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl" static const char *sysfs_system_path = SYSFS_SYSTEM_PATH; +static const char *sysfs_resctrl_path = SYSFS_RESCTRL_PATH; void virSysfsSetSystemPath(const char *path) @@ -55,6 +58,20 @@ virSysfsGetSystemPath(void) return sysfs_system_path; } +void virSysfsSetResctrlPath(const char *path) +{ + if (path) + sysfs_resctrl_path = path; + else + sysfs_resctrl_path = SYSFS_RESCTRL_PATH; +} + +const char * +virSysfsGetResctrlPath(void) +{ + return sysfs_resctrl_path; +} + int virSysfsGetValueInt(const char *file, int *value) @@ -227,3 +244,129 @@ virSysfsGetNodeValueBitmap(unsigned int node, VIR_FREE(path); return ret; } + +int +virSysfsGetResctrlString(const char* file, + char **value) +{ + char *path = NULL; + int ret = -1; + if (virAsprintf(&path, "%s/%s", sysfs_resctrl_path, file) < 0) + return -1; + + if (virFileReadAll(path, VIR_SYSFS_VALUE_MAXLEN, value) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(path); + return ret; +} + +int +virSysfsGetResctrlUint(const char *file, + unsigned int *value) +{ + char *path = NULL; + int ret = -1; + if (virAsprintf(&path, "%s/%s", sysfs_resctrl_path, file) < 0) + return -1; + + ret = virFileReadValueUint(path, value); + + VIR_FREE(path); + return ret; +} + +int +virSysfsGetResctrlInfoString(const char* file, + char **value) +{ + char *path = NULL; + int ret = -1; + if (virAsprintf(&path, "%s/info/%s", sysfs_resctrl_path, file) < 0) + return -1; + + if (virFileReadAll(path, VIR_SYSFS_VALUE_MAXLEN, value) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(path); + return ret; +} + +int +virSysfsGetResctrlInfoUint(const char *file, + unsigned int *value) +{ + char *path = NULL; + int ret; + + if (virAsprintf(&path, "%s/info/%s", sysfs_resctrl_path, file) < 0) + return -1; + + ret = virFileReadValueUint(path, value); + + VIR_FREE(path); + return ret; +} + +int +virSysfsWriteResctrlString(const char *file, + const char *content) +{ + char *path = NULL; + int ret = -1; + int writefd; + + if (virAsprintf(&path, "%s/%s", sysfs_resctrl_path, file) < 0) + return -1; + + /* We can not use virFileWriteStr because resctrl requires oflag should be as + O_WRONLY | O_APPEND */ + if ((writefd = open(path, O_WRONLY | O_APPEND, S_IRUSR | S_IWUSR)) < 0) + goto cleanup; + + if (safewrite(writefd, content, strlen(content)) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FORCE_CLOSE(writefd); + VIR_FREE(path); + return ret; +} + +int +virSysfsCreateResCtrlDir(const char *file) +{ + char *path = NULL; + int ret = -1; + + if (virAsprintf(&path, "%s/%s", sysfs_resctrl_path, file) < 0) + return -1; + + ret = virDirCreate(path, 0755, 0, 0, 0); + + VIR_FREE(path); + return ret; +} + +int +virSysfsRemoveResCtrlDir(const char *file) +{ + char *path = NULL; + int ret = -1; + + if (virAsprintf(&path, "%s/%s", sysfs_resctrl_path, file) < 0) + return -1; + + ret = virFileDeleteTree(path); + + VIR_FREE(path); + return ret; +} diff --git a/src/util/virsysfs.h b/src/util/virsysfs.h index cd871ff..60ad444 100644 --- a/src/util/virsysfs.h +++ b/src/util/virsysfs.h @@ -25,6 +25,7 @@ # include "virbitmap.h" const char * virSysfsGetSystemPath(void); +const char * virSysfsGetResctrlPath(void); int virSysfsGetValueInt(const char *file, @@ -67,4 +68,29 @@ virSysfsGetNodeValueBitmap(unsigned int cpu, const char *file, virBitmapPtr *value); +int +virSysfsGetResctrlString(const char* file, + char **value); + +int +virSysfsGetResctrlUint(const char* file, + unsigned int *value); + +int +virSysfsGetResctrlInfoString(const char* file, + char **value); + +int +virSysfsGetResctrlInfoUint(const char *file, + unsigned int *value); + +int +virSysfsWriteResctrlString(const char *file, + const char *content); + +int +virSysfsCreateResCtrlDir(const char *file); + +int +virSysfsRemoveResCtrlDir(const char *file); #endif /* __VIR_SYSFS_H__*/ diff --git a/src/util/virsysfspriv.h b/src/util/virsysfspriv.h index ae9f54a..bf36082 100644 --- a/src/util/virsysfspriv.h +++ b/src/util/virsysfspriv.h @@ -24,5 +24,6 @@ # include "virsysfs.h" void virSysfsSetSystemPath(const char *path); +void virSysfsSetResctrlPath(const char *path); #endif /* __VIR_SYSFS_PRIV_H__*/ -- 1.9.1
#define VIR_SYSFS_VALUE_MAXLEN 8192 #define SYSFS_SYSTEM_PATH "/sys/devices/system" +#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl"
static const char *sysfs_system_path = SYSFS_SYSTEM_PATH; +static const char *sysfs_resctrl_path = SYSFS_RESCTRL_PATH;
void virSysfsSetSystemPath(const char *path) @@ -55,6 +58,20 @@ virSysfsGetSystemPath(void) return sysfs_system_path; }
+void virSysfsSetResctrlPath(const char *path) +{ + if (path) + sysfs_resctrl_path = path; + else + sysfs_resctrl_path = SYSFS_RESCTRL_PATH; +} + +const char * +virSysfsGetResctrlPath(void) +{ + return sysfs_resctrl_path; +} +
NACK This leads to an unnecessary code duplication (applies for most of the functions introduced by this patch). Instead, virsysfs should be made generic enough so it could be reused by any module doing sysfs related tasks, like for example the recently added mediated device framework (otherwise a new sysfs_foo_path = "/sys/bus/mdev/devices/" would need to be created as well). Erik
On Fri, Mar 31, 2017 at 12:28:26PM +0200, Erik Skultety wrote:
#define VIR_SYSFS_VALUE_MAXLEN 8192 #define SYSFS_SYSTEM_PATH "/sys/devices/system" +#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl"
static const char *sysfs_system_path = SYSFS_SYSTEM_PATH; +static const char *sysfs_resctrl_path = SYSFS_RESCTRL_PATH;
void virSysfsSetSystemPath(const char *path) @@ -55,6 +58,20 @@ virSysfsGetSystemPath(void) return sysfs_system_path; }
+void virSysfsSetResctrlPath(const char *path) +{ + if (path) + sysfs_resctrl_path = path; + else + sysfs_resctrl_path = SYSFS_RESCTRL_PATH; +} + +const char * +virSysfsGetResctrlPath(void) +{ + return sysfs_resctrl_path; +} +
NACK
This leads to an unnecessary code duplication (applies for most of the functions introduced by this patch). Instead, virsysfs should be made generic enough so it could be reused by any module doing sysfs related tasks, like for example the recently added mediated device framework (otherwise a new sysfs_foo_path = "/sys/bus/mdev/devices/" would need to be created as well).
I was thinking about this, but let's discuss how to select the proper line between the amount of code duplication (which I didn't like even in my series) and the generality of the code (which at the end leads to worse code in callers). Adding new set of functions for each path prefix is gross, I agree, but how else could we approach this? One of the ideas would be to have a function that registers "path overrides", but that would lead to unnecessary code in production where there are no tests involved. Another approach is to just set the "/sys" path differently, but that would mean we have to have bigger directory structures in the tests. Yet another approach is to ditch virsysfs altogether and just use virFile* functions. We can mock open() and others in tests anyway (like, for example, vircgrouptest does even for the sysfs system paths). However, if I look at the code in the caller functions, the last approach would, over time, end up duplicating more code than we do currently in virsysfs. Also, even though this is highly subjective, the callers are very easy to read and understand. More wrappers, if not overused, of course, lead to cleaner codebase proportionally to the codebase size. I'm not saying that what I did was the best approach, but please be constructive. If we don't have any better solutions for now, we can go with one that's Good Enough™ and refactor it later when we figure out how to approach it. I'm open to suggestions and I know Eli is as well. Martin
On Fri, Mar 31, 2017 at 12:59:33PM +0200, Martin Kletzander wrote:
On Fri, Mar 31, 2017 at 12:28:26PM +0200, Erik Skultety wrote:
#define VIR_SYSFS_VALUE_MAXLEN 8192 #define SYSFS_SYSTEM_PATH "/sys/devices/system" +#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl"
static const char *sysfs_system_path = SYSFS_SYSTEM_PATH; +static const char *sysfs_resctrl_path = SYSFS_RESCTRL_PATH;
void virSysfsSetSystemPath(const char *path) @@ -55,6 +58,20 @@ virSysfsGetSystemPath(void) return sysfs_system_path; }
+void virSysfsSetResctrlPath(const char *path) +{ + if (path) + sysfs_resctrl_path = path; + else + sysfs_resctrl_path = SYSFS_RESCTRL_PATH; +} + +const char * +virSysfsGetResctrlPath(void) +{ + return sysfs_resctrl_path; +} +
NACK
This leads to an unnecessary code duplication (applies for most of the functions introduced by this patch). Instead, virsysfs should be made generic enough so it could be reused by any module doing sysfs related tasks, like for example the recently added mediated device framework (otherwise a new sysfs_foo_path = "/sys/bus/mdev/devices/" would need to be created as well).
I was thinking about this, but let's discuss how to select the proper line between the amount of code duplication (which I didn't like even in my series) and the generality of the code (which at the end leads to worse code in callers).
Adding new set of functions for each path prefix is gross, I agree, but how else could we approach this? One of the ideas would be to have a function that registers "path overrides", but that would lead to unnecessary code in production where there are no tests involved. Another approach is to just set the "/sys" path differently, but that would mean we have to have bigger directory structures in the tests. Yet another approach is to ditch virsysfs altogether and just use virFile* functions. We can mock open() and others in tests anyway (like, for example, vircgrouptest does even for the sysfs system paths). However, if I look at the code in the caller functions, the last approach would, over time, end up duplicating more code than we do currently in virsysfs. Also, even though this is highly subjective, the callers are very easy to read and understand. More wrappers, if not overused, of course, lead to cleaner codebase proportionally to the codebase size.
The thing with code duplication is very subjective, while I say that copying function bodies and enclosing them with a different name, then exporting the symbol via a header and also adding it to the private syms is the kind of duplication I'd like to avoid, I understand you can say the same about my idea (yet to come) about having this in every caller: ... if (!virAsprintf(path, "%s/%s", sysfs_prefix, attr)) goto cleanup; if (virSysfsRead[String,Int,Whatever] < 0) goto cleanup; ... So in my opinion, this kind of duplication is more acceptable than having a ton of symbols exporting the same functionality (aka function bodies). Now, to the idea, to deal with the prefixes, each of the util/virmodule.c would declare a string constant representing the path prefix, like virpci does with PCI_SYSFS and mdev with MDEV_SYSFS_DEVICES, then the burden of constructing the path would be on the caller (as prefaced above). I must admit that I haven't looked at the tests, but I'm afraid you can't get both - generic functions without introducing any duplicates in the code. But as we spoke privately, I think the concept of mocking can be reused to our liking with the sysfs accesses. So, looking at the tests, we will need especially need to figure out how to deal with the amount of files created/present under virhostcpudata. You had a specific idea in mind about this, so feel free to share it here. Erik
I'm not saying that what I did was the best approach, but please be constructive. If we don't have any better solutions for now, we can go with one that's Good Enough™ and refactor it later when we figure out how to approach it. I'm open to suggestions and I know Eli is as well.
Martin
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Apr 03, 2017 at 09:29:37AM +0200, Erik Skultety wrote:
On Fri, Mar 31, 2017 at 12:59:33PM +0200, Martin Kletzander wrote:
On Fri, Mar 31, 2017 at 12:28:26PM +0200, Erik Skultety wrote:
#define VIR_SYSFS_VALUE_MAXLEN 8192 #define SYSFS_SYSTEM_PATH "/sys/devices/system" +#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl"
static const char *sysfs_system_path = SYSFS_SYSTEM_PATH; +static const char *sysfs_resctrl_path = SYSFS_RESCTRL_PATH;
void virSysfsSetSystemPath(const char *path) @@ -55,6 +58,20 @@ virSysfsGetSystemPath(void) return sysfs_system_path; }
+void virSysfsSetResctrlPath(const char *path) +{ + if (path) + sysfs_resctrl_path = path; + else + sysfs_resctrl_path = SYSFS_RESCTRL_PATH; +} + +const char * +virSysfsGetResctrlPath(void) +{ + return sysfs_resctrl_path; +} +
NACK
This leads to an unnecessary code duplication (applies for most of the functions introduced by this patch). Instead, virsysfs should be made generic enough so it could be reused by any module doing sysfs related tasks, like for example the recently added mediated device framework (otherwise a new sysfs_foo_path = "/sys/bus/mdev/devices/" would need to be created as well).
I was thinking about this, but let's discuss how to select the proper line between the amount of code duplication (which I didn't like even in my series) and the generality of the code (which at the end leads to worse code in callers).
Adding new set of functions for each path prefix is gross, I agree, but how else could we approach this? One of the ideas would be to have a function that registers "path overrides", but that would lead to unnecessary code in production where there are no tests involved. Another approach is to just set the "/sys" path differently, but that would mean we have to have bigger directory structures in the tests. Yet another approach is to ditch virsysfs altogether and just use virFile* functions. We can mock open() and others in tests anyway (like, for example, vircgrouptest does even for the sysfs system paths). However, if I look at the code in the caller functions, the last approach would, over time, end up duplicating more code than we do currently in virsysfs. Also, even though this is highly subjective, the callers are very easy to read and understand. More wrappers, if not overused, of course, lead to cleaner codebase proportionally to the codebase size.
The thing with code duplication is very subjective, while I say that copying function bodies and enclosing them with a different name, then exporting the symbol via a header and also adding it to the private syms is the kind of duplication I'd like to avoid, I understand you can say the same about my idea (yet to come) about having this in every caller:
... if (!virAsprintf(path, "%s/%s", sysfs_prefix, attr)) goto cleanup;
if (virSysfsRead[String,Int,Whatever] < 0) goto cleanup; ...
So in my opinion, this kind of duplication is more acceptable than having a ton of symbols exporting the same functionality (aka function bodies). Now, to the idea, to deal with the prefixes, each of the util/virmodule.c would declare a string constant representing the path prefix, like virpci does with PCI_SYSFS and mdev with MDEV_SYSFS_DEVICES, then the burden of constructing the path would be on the caller (as prefaced above). I must admit that I haven't looked at the tests, but I'm afraid you can't get both - generic functions without introducing any duplicates in the code. But as we spoke privately, I think the concept of mocking can be reused to our liking with the sysfs accesses. So, looking at the tests, we will need especially need to figure out how to deal with the amount of files created/present under virhostcpudata. You had a specific idea in mind about this, so feel free to share it here.
Yeah, there's always pros and cons to any solution and since this is pretty subjective, I'd opted for the one I liked the most as I feel like code duplication of *very understandable* parts is better because it speeds up reading the code for its callers. Anyway, I had an idea how to reach pretty good compromise. I'll construct all the paths in the callers, there will be no path masking in a global variable. So only virFileRead* will be used. We might do like a few super wrappers with various number of arguments, for example: virFileReadUint(&temp_uint, "/sys/devices/system/cpu/cpu%d/online", cpu_num) And that'd be it. I was also thinking how not to do one wrapper per type, but I don't think that's possible to do easily with C now. However we can have virFileReadObject() that would call a function on a virObject and for that we need to add interfaces to our classes for which I have an idea how to do as well, but I'm getting too tangled up in that. And I think I have an idea how to do all the testing of this only using mocks in tests, without any effect on the production part of the code. I'll try to do a proof of concept and we'll see how that goes.
Erik
I'm not saying that what I did was the best approach, but please be constructive. If we don't have any better solutions for now, we can go with one that's Good Enough™ and refactor it later when we figure out how to approach it. I'm open to suggestions and I know Eli is as well.
Martin
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Eli Qiao -
Erik Skultety -
Martin Kletzander