It is no longer needed thanks to the great virfilemock.c. And this
way we don't have to add a new set of functions for each prefixed
path.
While on that, add two functions that weren't there before, string and
scaled integer reading ones. Also increase the length of the string
being read by one to accompany for the optional newline at the
end (i.e. change INT_STRLEN_BOUND to INT_BUFSIZE_BOUND).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
src/Makefile.am | 2 -
src/conf/capabilities.c | 1 -
src/libvirt_private.syms | 16 +---
src/util/virfile.c | 204 ++++++++++++++++++++++++++++++++++-------
src/util/virfile.h | 14 ++-
src/util/virhostcpu.c | 43 +++++----
src/util/virsysfs.c | 231 -----------------------------------------------
src/util/virsysfs.h | 70 --------------
src/util/virsysfspriv.h | 28 ------
tests/Makefile.am | 5 +-
tests/vircaps2xmltest.c | 6 +-
tests/virhostcputest.c | 8 +-
tests/virnumamock.c | 15 +--
13 files changed, 228 insertions(+), 415 deletions(-)
delete mode 100644 src/util/virsysfs.c
delete mode 100644 src/util/virsysfs.h
delete mode 100644 src/util/virsysfspriv.h
diff --git a/src/Makefile.am b/src/Makefile.am
index 75e4344198c5..f04d262952e8 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -174,7 +174,6 @@ UTIL_SOURCES = \
util/virstorageencryption.c util/virstorageencryption.h \
util/virstoragefile.c util/virstoragefile.h \
util/virstring.h util/virstring.c \
- util/virsysfs.c util/virsysfs.h util/virsysfspriv.h \
util/virsysinfo.c util/virsysinfo.h util/virsysinfopriv.h \
util/virsystemd.c util/virsystemd.h util/virsystemdpriv.h \
util/virthread.c util/virthread.h \
@@ -2586,7 +2585,6 @@ libvirt_setuid_rpc_client_la_SOURCES = \
util/virrandom.c \
util/virsocketaddr.c \
util/virstring.c \
- util/virsysfs.c \
util/virsystemd.c \
util/virtime.c \
util/virthread.c \
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index be95c50cfb67..7ed76e65b1a1 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -45,7 +45,6 @@
#include "virlog.h"
#include "virnuma.h"
#include "virstring.h"
-#include "virsysfs.h"
#include "virtypedparam.h"
#include "viruuid.h"
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 92083e54142d..d399e0dc063a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1632,6 +1632,8 @@ virFileReadLimFD;
virFileReadLink;
virFileReadValueBitmap;
virFileReadValueInt;
+virFileReadValueScaledInt;
+virFileReadValueString;
virFileReadValueUint;
virFileRelLinkPointsTo;
virFileRemove;
@@ -2620,20 +2622,6 @@ virTrimSpaces;
virVasprintfInternal;
-# util/virsysfs.h
-virSysfsGetCpuValueBitmap;
-virSysfsGetCpuValueInt;
-virSysfsGetCpuValueString;
-virSysfsGetCpuValueUint;
-virSysfsGetNodeValueBitmap;
-virSysfsGetNodeValueString;
-virSysfsGetSystemPath;
-virSysfsGetValueBitmap;
-virSysfsGetValueInt;
-virSysfsGetValueString;
-virSysfsSetSystemPath;
-
-
# util/virsysinfo.h
virSysinfoBaseBoardDefClear;
virSysinfoBIOSDefFree;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index cbfa3849d793..c594b5782800 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3799,24 +3799,36 @@ virFileComparePaths(const char *p1, const char *p2)
/**
* virFileReadValueInt:
- * @path: file to read from
* @value: pointer to int to be filled in with the value
+ * @format, ...: file to read from
*
- * Read int from @path and put it into @value.
+ * Read int from @format and put it into @value.
*
* Return -2 for non-existing file, -1 on other errors and 0 if everything went
* fine.
*/
int
-virFileReadValueInt(const char *path, int *value)
+virFileReadValueInt(int *value, const char *format, ...)
{
+ int ret = -1;
char *str = NULL;
+ char *path = NULL;
+ va_list ap;
- if (!virFileExists(path))
- return -2;
+ va_start(ap, format);
+ if (virVasprintf(&path, format, ap) < 0) {
+ va_end(ap);
+ goto cleanup;
+ }
+ va_end(ap);
- if (virFileReadAll(path, INT_STRLEN_BOUND(*value), &str) < 0)
- return -1;
+ if (!virFileExists(path)) {
+ ret = -2;
+ goto cleanup;
+ }
+
+ if (virFileReadAll(path, INT_BUFSIZE_BOUND(*value), &str) < 0)
+ goto cleanup;
virStringTrimOptionalNewline(str);
@@ -3824,83 +3836,205 @@ virFileReadValueInt(const char *path, int *value)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid integer value '%s' in file '%s'"),
str, path);
- return -1;
+ goto cleanup;
}
+ ret = 0;
+ cleanup:
+ VIR_FREE(path);
VIR_FREE(str);
-
- return 0;
+ return ret;
}
/**
* virFileReadValueUint:
- * @path: file to read from
- * @value: pointer to unsigned int to be filled in with the value
+ * @value: pointer to int to be filled in with the value
+ * @format, ...: file to read from
*
- * Read int from @path and put it into @value.
+ * Read unsigned int from @format and put it into @value.
*
* Return -2 for non-existing file, -1 on other errors and 0 if everything went
* fine.
*/
int
-virFileReadValueUint(const char *path, unsigned int *value)
+virFileReadValueUint(unsigned int *value, const char *format, ...)
{
+ int ret = -1;
char *str = NULL;
+ char *path = NULL;
+ va_list ap;
- if (!virFileExists(path))
- return -2;
+ va_start(ap, format);
+ if (virVasprintf(&path, format, ap) < 0) {
+ va_end(ap);
+ goto cleanup;
+ }
+ va_end(ap);
- if (virFileReadAll(path, INT_STRLEN_BOUND(*value), &str) < 0)
- return -1;
+ if (!virFileExists(path)) {
+ ret = -2;
+ goto cleanup;
+ }
+
+ if (virFileReadAll(path, INT_BUFSIZE_BOUND(*value), &str) < 0)
+ goto cleanup;
virStringTrimOptionalNewline(str);
- if (virStrToLong_uip(str, NULL, 10, value)) {
+ if (virStrToLong_uip(str, NULL, 10, value) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid unsigned integer value '%s' in file '%s'"),
str, path);
- return -1;
+ goto cleanup;
}
+ ret = 0;
+ cleanup:
+ VIR_FREE(path);
VIR_FREE(str);
+ return ret;
+}
- return 0;
+
+/* Arbitrarily sized number, feel free to change, but the function should be
+ * used for small, interface-like files, so it should not be huge (subjective) */
+#define VIR_FILE_READ_VALUE_STRING_MAX 4096
+
+/**
+ * virFileReadValueScaledInt:
+ * @value: pointer to unsigned long long int to be filled in with the value
+ * @format, ...: file to read from
+ *
+ * Read unsigned scaled int from @format and put it into @value.
+ *
+ * Return -2 for non-existing file, -1 on other errors and 0 if everything went
+ * fine.
+ */
+int
+virFileReadValueScaledInt(unsigned long long *value, const char *format, ...)
+{
+ int ret = -1;
+ char *str = NULL;
+ char *endp = NULL;
+ char *path = NULL;
+ va_list ap;
+
+ va_start(ap, format);
+ if (virVasprintf(&path, format, ap) < 0) {
+ va_end(ap);
+ goto cleanup;
+ }
+ va_end(ap);
+
+ if (!virFileExists(path)) {
+ ret = -2;
+ goto cleanup;
+ }
+
+ if (virFileReadAll(path, INT_BUFSIZE_BOUND(*value), &str) < 0)
+ goto cleanup;
+
+ virStringTrimOptionalNewline(str);
+
+ if (virStrToLong_ullp(str, &endp, 10, value) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid unsigned scaled integer value '%s' in file '%s'"),
+ str, path);
+ goto cleanup;
+ }
+
+ ret = virScaleInteger(value, endp, 1024, ULLONG_MAX);
+ cleanup:
+ VIR_FREE(path);
+ VIR_FREE(str);
+ return ret;
}
/**
* virFileReadValueBitmap:
- * @path: file to read from
- * @value: double pointer to virBitmap to be allocated and filled in with the
- * value
+ * @value: pointer to virBitmapPtr to be allocated and filled in with the value
+ * @format, ...: file to read from
*
- * Read int from @path and put it into @value.
+ * Read int from @format and put it into @value.
*
* Return -2 for non-existing file, -1 on other errors and 0 if everything went
* fine.
*/
int
-virFileReadValueBitmap(const char *path,
- int maxlen,
- virBitmapPtr *value)
+virFileReadValueBitmap(virBitmapPtr *value, const char *format, ...)
{
- char *buf = NULL;
int ret = -1;
+ char *str = NULL;
+ char *path = NULL;
+ va_list ap;
- if (!virFileExists(path))
- return -2;
+ va_start(ap, format);
+ if (virVasprintf(&path, format, ap) < 0) {
+ va_end(ap);
+ goto cleanup;
+ }
+ va_end(ap);
- if (virFileReadAll(path, maxlen, &buf) < 0)
+ if (!virFileExists(path)) {
+ ret = -2;
goto cleanup;
+ }
- virStringTrimOptionalNewline(buf);
+ if (virFileReadAll(path, VIR_FILE_READ_VALUE_STRING_MAX, &str) < 0)
+ goto cleanup;
- *value = virBitmapParseUnlimited(buf);
+ virStringTrimOptionalNewline(str);
+
+ *value = virBitmapParseUnlimited(str);
if (!*value)
goto cleanup;
ret = 0;
cleanup:
- VIR_FREE(buf);
+ VIR_FREE(path);
+ VIR_FREE(str);
+ return ret;
+}
+
+/**
+ * virFileReadValueString:
+ * @value: pointer to char * to be allocated and filled in with the value
+ * @format, ...: file to read from
+ *
+ * Read string from @format and put it into @value. Don't get this mixed with
+ * virFileReadAll(). This function is a wrapper over it with the behaviour
+ * aligned to other virFileReadValue* functions
+ *
+ * Return -2 for non-existing file, -1 on other errors and 0 if everything went
+ * fine.
+ */
+int
+virFileReadValueString(char **value, const char *format, ...)
+{
+ int ret = -1;
+ char *str = NULL;
+ char *path = NULL;
+ va_list ap;
+
+ va_start(ap, format);
+ if (virVasprintf(&path, format, ap) < 0) {
+ va_end(ap);
+ goto cleanup;
+ }
+ va_end(ap);
+
+ if (!virFileExists(path)) {
+ ret = -2;
+ goto cleanup;
+ }
+
+ ret = virFileReadAll(path, VIR_FILE_READ_VALUE_STRING_MAX, value);
+
+ if (*value)
+ virStringTrimOptionalNewline(*value);
+ cleanup:
+ VIR_FREE(path);
+ VIR_FREE(str);
return ret;
}
diff --git a/src/util/virfile.h b/src/util/virfile.h
index ba1c57c06a8e..6617b9e02407 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -336,8 +336,16 @@ int virFileCopyACLs(const char *src,
int virFileComparePaths(const char *p1, const char *p2);
-int virFileReadValueInt(const char *path, int *value);
-int virFileReadValueUint(const char *path, unsigned int *value);
-int virFileReadValueBitmap(const char *path, int maxlen, virBitmapPtr *value);
+int virFileReadValueInt(int *value, const char *format, ...)
+ ATTRIBUTE_FMT_PRINTF(2, 3);
+int virFileReadValueUint(unsigned int *value, const char *format, ...)
+ ATTRIBUTE_FMT_PRINTF(2, 3);
+int virFileReadValueBitmap(virBitmapPtr *value, const char *format, ...)
+ ATTRIBUTE_FMT_PRINTF(2, 3);
+int virFileReadValueScaledInt(unsigned long long *value, const char *format, ...)
+ ATTRIBUTE_FMT_PRINTF(2, 3);
+int virFileReadValueString(char **value, const char *format, ...)
+ ATTRIBUTE_FMT_PRINTF(2, 3);
+
#endif /* __VIR_FILE_H */
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index a660e3f4dbe5..fd1ac9b8030e 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -56,7 +56,6 @@
#include "virfile.h"
#include "virtypedparam.h"
#include "virstring.h"
-#include "virsysfs.h"
#include "virnuma.h"
#include "virlog.h"
@@ -192,6 +191,7 @@ virHostCPUGetStatsFreeBSD(int cpuNum,
#ifdef __linux__
# define CPUINFO_PATH "/proc/cpuinfo"
# define PROCSTAT_PATH "/proc/stat"
+# define SYSFS_SYSTEM_PATH "/sys/devices/system"
# define VIR_HOST_CPU_MASK_LEN 1024
# define LINUX_NB_CPU_STATS 4
@@ -205,7 +205,10 @@ virHostCPUCountThreadSiblings(unsigned int cpu)
char *str = NULL;
size_t i;
- rv = virSysfsGetCpuValueString(cpu, "topology/thread_siblings", &str);
+ rv = virFileReadValueString(&str,
+ SYSFS_SYSTEM_PATH
+ "/cpu/cpu%u/topology/thread_siblings",
+ cpu);
if (rv == -2) {
ret = 1;
goto cleanup;
@@ -227,9 +230,10 @@ int
virHostCPUGetSocket(unsigned int cpu, unsigned int *socket)
{
int tmp;
- int ret = virSysfsGetCpuValueInt(cpu,
- "topology/physical_package_id",
- &tmp);
+ int ret = virFileReadValueInt(&tmp,
+ SYSFS_SYSTEM_PATH
+ "/cpu/cpu%u/topology/physical_package_id",
+ cpu);
/* If the file is not there, it's 0 */
if (ret == -2)
@@ -251,7 +255,10 @@ virHostCPUGetSocket(unsigned int cpu, unsigned int *socket)
int
virHostCPUGetCore(unsigned int cpu, unsigned int *core)
{
- int ret = virSysfsGetCpuValueUint(cpu, "topology/core_id", core);
+ int ret = virFileReadValueUint(core,
+ SYSFS_SYSTEM_PATH
+ "/cpu/cpu%u/topology/core_id",
+ cpu);
/* If the file is not there, it's 0 */
if (ret == -2)
@@ -268,7 +275,10 @@ virHostCPUGetSiblingsList(unsigned int cpu)
virBitmapPtr ret = NULL;
int rv = -1;
- rv = virSysfsGetCpuValueBitmap(cpu, "topology/thread_siblings_list", &ret);
+ rv = virFileReadValueBitmap(&ret,
+ SYSFS_SYSTEM_PATH
+ "/cpu/cpu%u/topology/thread_siblings_list",
+ cpu);
if (rv == -2) {
/* If the file doesn't exist, the threadis its only sibling */
ret = virBitmapNew(cpu + 1);
@@ -615,7 +625,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
/* OK, we've parsed clock speed out of /proc/cpuinfo. Get the
* core, node, socket, thread and topology information from /sys
*/
- if (virAsprintf(&sysfs_nodedir, "%s/node", virSysfsGetSystemPath()) < 0)
+ if (virAsprintf(&sysfs_nodedir, SYSFS_SYSTEM_PATH "/node") < 0)
goto cleanup;
if (virDirOpenQuiet(&nodedir, sysfs_nodedir) < 0) {
@@ -659,8 +669,8 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
(*nodes)++;
- if (virAsprintf(&sysfs_cpudir, "%s/node/%s",
- virSysfsGetSystemPath(), nodedirent->d_name) < 0)
+ if (virAsprintf(&sysfs_cpudir, SYSFS_SYSTEM_PATH "/node/%s",
+ nodedirent->d_name) < 0)
goto cleanup;
if ((nodecpus = virHostCPUParseNode(sysfs_cpudir, arch,
@@ -694,7 +704,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
fallback:
VIR_FREE(sysfs_cpudir);
- if (virAsprintf(&sysfs_cpudir, "%s/cpu", virSysfsGetSystemPath()) < 0)
+ if (virAsprintf(&sysfs_cpudir, SYSFS_SYSTEM_PATH "/cpu") < 0)
goto cleanup;
if ((nodecpus = virHostCPUParseNode(sysfs_cpudir, arch,
@@ -841,7 +851,7 @@ virHostCPUParseCountLinux(void)
char *tmp;
int ret = -1;
- if (virSysfsGetValueString("cpu/present", &str) < 0)
+ if (virFileReadValueString(&str, SYSFS_SYSTEM_PATH "/cpu/present") < 0)
return -1;
tmp = str;
@@ -866,8 +876,9 @@ int
virHostCPUGetOnline(unsigned int cpu, bool *online)
{
unsigned int tmp = 0;
- int ret = virSysfsGetCpuValueUint(cpu, "online", &tmp);
-
+ int ret = virFileReadValueUint(&tmp,
+ SYSFS_SYSTEM_PATH "/cpu/cpu%u/online",
+ cpu);
/* If the file is not there, it's online (doesn't support offlining) */
if (ret == -2)
@@ -1032,7 +1043,7 @@ virHostCPUGetPresentBitmap(void)
#ifdef __linux__
virBitmapPtr ret = NULL;
- virSysfsGetValueBitmap("cpu/present", &ret);
+ virFileReadValueBitmap(&ret, SYSFS_SYSTEM_PATH "/cpu/present");
return ret;
#else
@@ -1048,7 +1059,7 @@ virHostCPUGetOnlineBitmap(void)
#ifdef __linux__
virBitmapPtr ret = NULL;
- virSysfsGetValueBitmap("cpu/online", &ret);
+ virFileReadValueBitmap(&ret, SYSFS_SYSTEM_PATH "/cpu/online");
return ret;
#else
diff --git a/src/util/virsysfs.c b/src/util/virsysfs.c
deleted file mode 100644
index 6df45a0e36d9..000000000000
--- a/src/util/virsysfs.c
+++ /dev/null
@@ -1,231 +0,0 @@
-/*
- * virsysfs.c: Helper functions for manipulating sysfs files
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library. If not, see
- * <http://www.gnu.org/licenses/>.
- *
- * Author: Martin Kletzander <mkletzan@redhat.com>
- */
-
-#include <config.h>
-
-#include "internal.h"
-
-#include "virsysfspriv.h"
-
-#include "viralloc.h"
-#include "virfile.h"
-#include "virlog.h"
-#include "virstring.h"
-
-#define VIR_FROM_THIS VIR_FROM_NONE
-
-VIR_LOG_INIT("util.sysfs");
-
-
-#define VIR_SYSFS_VALUE_MAXLEN 8192
-#define SYSFS_SYSTEM_PATH "/sys/devices/system"
-
-static const char *sysfs_system_path = SYSFS_SYSTEM_PATH;
-
-
-void virSysfsSetSystemPath(const char *path)
-{
- if (path)
- sysfs_system_path = path;
- else
- sysfs_system_path = SYSFS_SYSTEM_PATH;
-}
-
-
-const char *
-virSysfsGetSystemPath(void)
-{
- return sysfs_system_path;
-}
-
-int
-virSysfsGetValueInt(const char *file,
- int *value)
-{
- char *path = NULL;
- int ret = -1;
-
- if (virAsprintf(&path, "%s/%s", sysfs_system_path, file) < 0)
- return -1;
-
- ret = virFileReadValueInt(path, value);
-
- VIR_FREE(path);
- return ret;
-}
-
-int
-virSysfsGetValueString(const char *file,
- char **value)
-{
- char *path = NULL;
- int ret = -1;
-
- if (virAsprintf(&path, "%s/%s", sysfs_system_path, file) < 0)
- return -1;
-
- if (!virFileExists(path)) {
- ret = -2;
- goto cleanup;
- }
-
- if (virFileReadAll(path, VIR_SYSFS_VALUE_MAXLEN, value) < 0)
- goto cleanup;
-
- virStringTrimOptionalNewline(*value);
-
- ret = 0;
- cleanup:
- VIR_FREE(path);
- return ret;
-}
-
-int
-virSysfsGetValueBitmap(const char *file,
- virBitmapPtr *value)
-{
- char *path = NULL;
- int ret = -1;
-
- if (virAsprintf(&path, "%s/%s", sysfs_system_path, file) < 0)
- return -1;
-
- ret = virFileReadValueBitmap(path, VIR_SYSFS_VALUE_MAXLEN, value);
- VIR_FREE(path);
- return ret;
-}
-
-int
-virSysfsGetCpuValueInt(unsigned int cpu,
- const char *file,
- int *value)
-{
- char *path = NULL;
- int ret = -1;
-
- if (virAsprintf(&path, "%s/cpu/cpu%u/%s", sysfs_system_path, cpu, file) < 0)
- return -1;
-
- ret = virFileReadValueInt(path, value);
-
- VIR_FREE(path);
- return ret;
-}
-
-
-int
-virSysfsGetCpuValueUint(unsigned int cpu,
- const char *file,
- unsigned int *value)
-{
- char *path = NULL;
- int ret = -1;
-
- if (virAsprintf(&path, "%s/cpu/cpu%u/%s", sysfs_system_path, cpu, file) < 0)
- return -1;
-
- ret = virFileReadValueUint(path, value);
-
- VIR_FREE(path);
- return ret;
-}
-
-
-int
-virSysfsGetCpuValueString(unsigned int cpu,
- const char *file,
- char **value)
-{
- char *path = NULL;
- int ret = -1;
-
- if (virAsprintf(&path, "%s/cpu/cpu%u/%s", sysfs_system_path, cpu, file) < 0)
- return -1;
-
- if (!virFileExists(path)) {
- ret = -2;
- goto cleanup;
- }
-
- if (virFileReadAll(path, VIR_SYSFS_VALUE_MAXLEN, value) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- VIR_FREE(path);
- return ret;
-}
-
-int
-virSysfsGetCpuValueBitmap(unsigned int cpu,
- const char *file,
- virBitmapPtr *value)
-{
- char *path = NULL;
- int ret = -1;
-
- if (virAsprintf(&path, "%s/cpu/cpu%u/%s", sysfs_system_path, cpu, file) < 0)
- return -1;
-
- ret = virFileReadValueBitmap(path, VIR_SYSFS_VALUE_MAXLEN, value);
- VIR_FREE(path);
- return ret;
-}
-
-int
-virSysfsGetNodeValueString(unsigned int node,
- const char *file,
- char **value)
-{
- char *path = NULL;
- int ret = -1;
-
- if (virAsprintf(&path, "%s/node/node%u/%s", sysfs_system_path, node, file) < 0)
- return -1;
-
- if (!virFileExists(path)) {
- ret = -2;
- goto cleanup;
- }
-
- if (virFileReadAll(path, VIR_SYSFS_VALUE_MAXLEN, value) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- VIR_FREE(path);
- return ret;
-}
-
-int
-virSysfsGetNodeValueBitmap(unsigned int node,
- const char *file,
- virBitmapPtr *value)
-{
- char *path = NULL;
- int ret = -1;
-
- if (virAsprintf(&path, "%s/node/node%u/%s", sysfs_system_path, node, file) < 0)
- return -1;
-
- ret = virFileReadValueBitmap(path, VIR_SYSFS_VALUE_MAXLEN, value);
- VIR_FREE(path);
- return ret;
-}
diff --git a/src/util/virsysfs.h b/src/util/virsysfs.h
deleted file mode 100644
index cd871ff11dd1..000000000000
--- a/src/util/virsysfs.h
+++ /dev/null
@@ -1,70 +0,0 @@
-/*
- * virsysfs.h: Helper functions for manipulating sysfs files
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library. If not, see
- * <http://www.gnu.org/licenses/>.
- *
- * Author: Martin Kletzander <mkletzan@redhat.com>
- */
-
-#ifndef __VIR_SYSFS_H__
-# define __VIR_SYSFS_H__
-
-# include "internal.h"
-# include "virbitmap.h"
-
-const char * virSysfsGetSystemPath(void);
-
-int
-virSysfsGetValueInt(const char *file,
- int *value);
-
-int
-virSysfsGetValueString(const char *file,
- char **value);
-
-int
-virSysfsGetValueBitmap(const char *file,
- virBitmapPtr *value);
-
-int
-virSysfsGetCpuValueInt(unsigned int cpu,
- const char *file,
- int *value);
-int
-virSysfsGetCpuValueUint(unsigned int cpu,
- const char *file,
- unsigned int *value);
-
-int
-virSysfsGetCpuValueString(unsigned int cpu,
- const char *file,
- char **value);
-
-int
-virSysfsGetCpuValueBitmap(unsigned int cpu,
- const char *file,
- virBitmapPtr *value);
-
-int
-virSysfsGetNodeValueString(unsigned int node,
- const char *file,
- char **value);
-
-int
-virSysfsGetNodeValueBitmap(unsigned int cpu,
- const char *file,
- virBitmapPtr *value);
-
-#endif /* __VIR_SYSFS_H__*/
diff --git a/src/util/virsysfspriv.h b/src/util/virsysfspriv.h
deleted file mode 100644
index ae9f54a40c2a..000000000000
--- a/src/util/virsysfspriv.h
+++ /dev/null
@@ -1,28 +0,0 @@
-/*
- * virsysfspriv.h: Helper functions for manipulating sysfs files
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library. If not, see
- * <http://www.gnu.org/licenses/>.
- *
- * Author: Martin Kletzander <mkletzan@redhat.com>
- */
-
-#ifndef __VIR_SYSFS_PRIV_H__
-# define __VIR_SYSFS_PRIV_H__
-
-# include "virsysfs.h"
-
-void virSysfsSetSystemPath(const char *path);
-
-#endif /* __VIR_SYSFS_PRIV_H__*/
diff --git a/tests/Makefile.am b/tests/Makefile.am
index aa9d2eb3a343..f6306a3c1416 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -928,7 +928,7 @@ virconftest_SOURCES = \
virconftest_LDADD = $(LDADDS)
virhostcputest_SOURCES = \
- virhostcputest.c testutils.h testutils.c
+ virhostcputest.c testutils.h testutils.c virfilemock.c
virhostcputest_LDADD = $(LDADDS)
commandtest_SOURCES = \
@@ -1145,7 +1145,7 @@ virhostcpumock_la_LIBADD = $(MOCKLIBS_LIBS)
if WITH_LINUX
vircaps2xmltest_SOURCES = \
- vircaps2xmltest.c testutils.h testutils.c
+ vircaps2xmltest.c testutils.h testutils.c virfilemock.c
vircaps2xmltest_LDADD = $(LDADDS)
virnumamock_la_SOURCES = \
@@ -1153,6 +1153,7 @@ virnumamock_la_SOURCES = \
virnumamock_la_CFLAGS = $(AM_CFLAGS)
virnumamock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
virnumamock_la_LIBADD = $(MOCKLIBS_LIBS)
+
else ! WITH_LINUX
EXTRA_DIST += vircaps2xmltest.c virnumamock.c
endif ! WITH_LINUX
diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
index 670bb8c375b3..af422238520b 100644
--- a/tests/vircaps2xmltest.c
+++ b/tests/vircaps2xmltest.c
@@ -25,7 +25,7 @@
#include "testutils.h"
#include "capabilities.h"
#include "virbitmap.h"
-#include "virsysfspriv.h"
+#include "virfilemock.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -52,7 +52,7 @@ test_virCapabilities(const void *opaque)
abs_srcdir, data->filename) < 0)
goto cleanup;
- virSysfsSetSystemPath(dir);
+ virFileMockAddPrefix("/sys/devices/system", dir);
caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
if (!caps)
@@ -61,7 +61,7 @@ test_virCapabilities(const void *opaque)
if (virCapabilitiesInitNUMA(caps) < 0)
goto cleanup;
- virSysfsSetSystemPath(NULL);
+ virFileMockClearPrefixes();
if (!(capsXML = virCapabilitiesFormatXML(caps)))
goto cleanup;
diff --git a/tests/virhostcputest.c b/tests/virhostcputest.c
index b415ec5f1b08..6372fefd6f94 100644
--- a/tests/virhostcputest.c
+++ b/tests/virhostcputest.c
@@ -8,12 +8,14 @@
#include "testutils.h"
#include "internal.h"
#include "virhostcpupriv.h"
-#include "virsysfspriv.h"
#include "virfile.h"
#include "virstring.h"
+#include "virfilemock.h"
#define VIR_FROM_THIS VIR_FROM_NONE
+#define SYSFS_SYSTEM_PATH "/sys/devices/system"
+
#if !(defined __linux__)
int
@@ -178,9 +180,9 @@ linuxTestHostCPU(const void *opaque)
goto cleanup;
}
- virSysfsSetSystemPath(sysfs_prefix);
+ virFileMockAddPrefix(SYSFS_SYSTEM_PATH, sysfs_prefix);
result = linuxTestCompareFiles(cpuinfo, data->arch, output);
- virSysfsSetSystemPath(NULL);
+ virFileMockRemovePrefix(SYSFS_SYSTEM_PATH);
cleanup:
VIR_FREE(cpuinfo);
diff --git a/tests/virnumamock.c b/tests/virnumamock.c
index 210d15d6adf0..9c724536e85b 100644
--- a/tests/virnumamock.c
+++ b/tests/virnumamock.c
@@ -1,5 +1,5 @@
/*
- * virnumamock.c: Mock some virNuma functions using virsysfs
+ * virnumamock.c: Mock some virNuma functions using sysfs
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -24,10 +24,11 @@
#include "virfile.h"
#include "viralloc.h"
#include "virstring.h"
-#include "virsysfspriv.h"
#define VIR_FROM_THIS VIR_FROM_NONE
+#define SYSFS_SYSTEM_PATH "/sys/devices/system"
+
static int numa_avail = -1;
@@ -42,7 +43,7 @@ virNumaIsAvailable(void)
if (numa_avail < 0) {
char *sysfs_node_path = NULL;
- if (virAsprintfQuiet(&sysfs_node_path, "%s/node", virSysfsGetSystemPath()) < 0)
+ if (virAsprintfQuiet(&sysfs_node_path, SYSFS_SYSTEM_PATH "/node") < 0)
return false;
numa_avail = virFileExists(sysfs_node_path);
@@ -68,7 +69,7 @@ virNumaGetMaxNode(void)
int ret = -1;
virBitmapPtr map = NULL;
- if (virSysfsGetValueBitmap("node/online", &map) < 0)
+ if (virFileReadValueBitmap(&map, SYSFS_SYSTEM_PATH "/node/online") < 0)
return -1;
ret = virBitmapLastSetBit(map);
@@ -82,7 +83,7 @@ virNumaNodeIsAvailable(int node)
bool ret = false;
virBitmapPtr map = NULL;
- if (virSysfsGetValueBitmap("node/online", &map) < 0)
+ if (virFileReadValueBitmap(&map, SYSFS_SYSTEM_PATH "/node/online") < 0)
return false;
ret = virBitmapIsBitSet(map, node);
@@ -117,7 +118,7 @@ virNumaGetDistances(int node ATTRIBUTE_UNUSED,
}
/*
- * TODO: Adapt virNumaGetHugePageInfo{Path,Dir} to use virsysfs so that the
+ * TODO: Adapt virNumaGetHugePageInfo{Path,Dir} to use sysfs so that the
* paths can be modified and this function can be thrown away and instead we'd
* have copied info from /sys (as we do with /sys/devices/system).
*/
@@ -177,7 +178,7 @@ virNumaGetNodeCPUs(int node, virBitmapPtr *cpus)
int ret = -1;
char *cpulist = NULL;
- if (virSysfsGetNodeValueString(node, "cpulist", &cpulist) < 0)
+ if (virFileReadValueString(&cpulist, SYSFS_SYSTEM_PATH "/node/node%u/cpulist", node) < 0)
return -1;
*cpus = virBitmapParseUnlimited(cpulist);
--
2.12.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Apr 05, 2017 at 04:36:28PM +0200, Martin Kletzander wrote:
> It is no longer needed thanks to the great virfilemock.c. And this
> way we don't have to add a new set of functions for each prefixed
> path.
>
> While on that, add two functions that weren't there before, string and
> scaled integer reading ones. Also increase the length of the string
> being read by one to accompany for the optional newline at the
> end (i.e. change INT_STRLEN_BOUND to INT_BUFSIZE_BOUND).
>
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
> src/Makefile.am | 2 -
> src/conf/capabilities.c | 1 -
> src/libvirt_private.syms | 16 +---
> src/util/virfile.c | 204 ++++++++++++++++++++++++++++++++++-------
> src/util/virfile.h | 14 ++-
> src/util/virhostcpu.c | 43 +++++----
> src/util/virsysfs.c | 231 -----------------------------------------------
> src/util/virsysfs.h | 70 --------------
> src/util/virsysfspriv.h | 28 ------
> tests/Makefile.am | 5 +-
> tests/vircaps2xmltest.c | 6 +-
> tests/virhostcputest.c | 8 +-
> tests/virnumamock.c | 15 +--
> 13 files changed, 228 insertions(+), 415 deletions(-)
> delete mode 100644 src/util/virsysfs.c
> delete mode 100644 src/util/virsysfs.h
> delete mode 100644 src/util/virsysfspriv.h
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 75e4344198c5..f04d262952e8 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -174,7 +174,6 @@ UTIL_SOURCES = \
> util/virstorageencryption.c util/virstorageencryption.h \
> util/virstoragefile.c util/virstoragefile.h \
> util/virstring.h util/virstring.c \
> - util/virsysfs.c util/virsysfs.h util/virsysfspriv.h \
I would split this patch in two, one that introduces the adjustments to
virFile* methods, replacing the virSysfs calls and then another one removing
the virsysfs stuff.
[...]
> /**
> * virFileReadValueInt:
> - * @path: file to read from
> * @value: pointer to int to be filled in with the value
> + * @format, ...: file to read from
> *
> - * Read int from @path and put it into @value.
> + * Read int from @format and put it into @value.
> *
> * Return -2 for non-existing file, -1 on other errors and 0 if everything went
> * fine.
> */
> int
> -virFileReadValueInt(const char *path, int *value)
> +virFileReadValueInt(int *value, const char *format, ...)
I spent a significant amount of time thinking off how this could be done
differently so that everyone likes it (because I don't like passing format
string to a function that in my opinion screams for an argument containing a
path already built...), but haven't come up with anything that everyone would
agree with, so I gave up on that and there's no point in stalling the patch
furthermore and argue about our subjective opinions on the matter (but I just
had to express mine, sorry...).
> {
> + int ret = -1;
> char *str = NULL;
> + char *path = NULL;
> + va_list ap;
>
> - if (!virFileExists(path))
> - return -2;
> + va_start(ap, format);
> + if (virVasprintf(&path, format, ap) < 0) {
> + va_end(ap);
> + goto cleanup;
> + }
> + va_end(ap);
This will relate to the paragraph I wrote above, I know you used ^this bit to
ideally get rid of the 3 lines of code from every caller that needs to read
from a file:
1) declare @path
2) call virAsprintf
3) return -1 on failure of the above
[...]
> +
> +/* Arbitrarily sized number, feel free to change, but the function should be
> + * used for small, interface-like files, so it should not be huge (subjective) */
> +#define VIR_FILE_READ_VALUE_STRING_MAX 4096
either define it on top of the module or define it before the methods that make
use of it, undefining it afterwards (*ScaledInt doesn't use this constant).
> +
> +/**
> + * virFileReadValueScaledInt:
> + * @value: pointer to unsigned long long int to be filled in with the value
> + * @format, ...: file to read from
> + *
> + * Read unsigned scaled int from @format and put it into @value.
> + *
> + * Return -2 for non-existing file, -1 on other errors and 0 if everything went
> + * fine.
> + */
[...]
> - rv = virSysfsGetCpuValueString(cpu, "topology/thread_siblings", &str);
> + rv = virFileReadValueString(&str,
> + SYSFS_SYSTEM_PATH
> + "/cpu/cpu%u/topology/thread_siblings",
> + cpu);
Could we make the string constant part of the formatting string? Because the
way it looks now signals an alert "oh, there's a missing comma delimiting
arguments".
> - if (virAsprintf(&sysfs_nodedir, "%s/node", virSysfsGetSystemPath()) < 0)
> + if (virAsprintf(&sysfs_nodedir, SYSFS_SYSTEM_PATH "/node") < 0)
Same here, could we preserve the explicit formatting string and pass the
constant as an argument to it?
> #define VIR_FROM_THIS VIR_FROM_NONE
> @@ -52,7 +52,7 @@ test_virCapabilities(const void *opaque)
> abs_srcdir, data->filename) < 0)
> goto cleanup;
>
> - virSysfsSetSystemPath(dir);
> + virFileMockAddPrefix("/sys/devices/system", dir);
> caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
>
> if (!caps)
> @@ -61,7 +61,7 @@ test_virCapabilities(const void *opaque)
> if (virCapabilitiesInitNUMA(caps) < 0)
> goto cleanup;
>
> - virSysfsSetSystemPath(NULL);
> + virFileMockClearPrefixes();
RemovePrefix I suppose? It doesn't matter much now, since there's one
occurrence, but from the other change below I figured that we probably want to
remove the one single prefix we just added.
So I had a few nitpicks, but in principle, the patch's fine (even though I will
probably silently disagree with some bits from now on;)), ACK.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Apr 07, 2017 at 09:42:23AM +0200, Erik Skultety wrote:
>On Wed, Apr 05, 2017 at 04:36:28PM +0200, Martin Kletzander wrote:
>> It is no longer needed thanks to the great virfilemock.c. And this
>> way we don't have to add a new set of functions for each prefixed
>> path.
>>
>> While on that, add two functions that weren't there before, string and
>> scaled integer reading ones. Also increase the length of the string
>> being read by one to accompany for the optional newline at the
>> end (i.e. change INT_STRLEN_BOUND to INT_BUFSIZE_BOUND).
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>> src/Makefile.am | 2 -
>> src/conf/capabilities.c | 1 -
>> src/libvirt_private.syms | 16 +---
>> src/util/virfile.c | 204 ++++++++++++++++++++++++++++++++++-------
>> src/util/virfile.h | 14 ++-
>> src/util/virhostcpu.c | 43 +++++----
>> src/util/virsysfs.c | 231 -----------------------------------------------
>> src/util/virsysfs.h | 70 --------------
>> src/util/virsysfspriv.h | 28 ------
>> tests/Makefile.am | 5 +-
>> tests/vircaps2xmltest.c | 6 +-
>> tests/virhostcputest.c | 8 +-
>> tests/virnumamock.c | 15 +--
>> 13 files changed, 228 insertions(+), 415 deletions(-)
>> delete mode 100644 src/util/virsysfs.c
>> delete mode 100644 src/util/virsysfs.h
>> delete mode 100644 src/util/virsysfspriv.h
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 75e4344198c5..f04d262952e8 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -174,7 +174,6 @@ UTIL_SOURCES = \
>> util/virstorageencryption.c util/virstorageencryption.h \
>> util/virstoragefile.c util/virstoragefile.h \
>> util/virstring.h util/virstring.c \
>> - util/virsysfs.c util/virsysfs.h util/virsysfspriv.h \
>
>I would split this patch in two, one that introduces the adjustments to
>virFile* methods, replacing the virSysfs calls and then another one removing
>the virsysfs stuff.
>
I wanted to do that, but if I change the virFileRead* functions, I have
to change the callers in virsysfs.c, file that I will just remove, just
so that the build doesn't fail between those two commits, and that would
result in pointless changes. I don't see how the removal of that file
interferes with the rest of the changes, just skip the removal hunks.
Or you can limit the files for which you see diffs if you look at the
patch outside of the MUA.
>[...]
>
>> /**
>> * virFileReadValueInt:
>> - * @path: file to read from
>> * @value: pointer to int to be filled in with the value
>> + * @format, ...: file to read from
>> *
>> - * Read int from @path and put it into @value.
>> + * Read int from @format and put it into @value.
>> *
>> * Return -2 for non-existing file, -1 on other errors and 0 if everything went
>> * fine.
>> */
>> int
>> -virFileReadValueInt(const char *path, int *value)
>> +virFileReadValueInt(int *value, const char *format, ...)
>
>I spent a significant amount of time thinking off how this could be done
>differently so that everyone likes it (because I don't like passing format
>string to a function that in my opinion screams for an argument containing a
>path already built...), but haven't come up with anything that everyone would
>agree with, so I gave up on that and there's no point in stalling the patch
>furthermore and argue about our subjective opinions on the matter (but I just
>had to express mine, sorry...).
>
Sure, as I said, I would welcome any ideas to make stuff better. But I
couldn't come up with any other one either. That's probably caused by
the fact that I see this one as the cleanest, nicest approach we can
have. If you mention what particular parts you don't like, I can at
least try to meet you half-way. Maybe we'll think of something even
better.
>> {
>> + int ret = -1;
>> char *str = NULL;
>> + char *path = NULL;
>> + va_list ap;
>>
>> - if (!virFileExists(path))
>> - return -2;
>> + va_start(ap, format);
>> + if (virVasprintf(&path, format, ap) < 0) {
>> + va_end(ap);
>> + goto cleanup;
>> + }
>> + va_end(ap);
>
>This will relate to the paragraph I wrote above, I know you used ^this bit to
>ideally get rid of the 3 lines of code from every caller that needs to read
>from a file:
> 1) declare @path
> 2) call virAsprintf
> 3) return -1 on failure of the above
>
And cleanup the path *after* the virFileRead* function is called both if
it failed and if it did not fail, making it harder to call multiple
virFileRead* after each other, effectively making you do 3 or 4 more
lines for each call. Those were the lines that were bothering me, way
more than three you mentioned.
>[...]
>
>> +
>> +/* Arbitrarily sized number, feel free to change, but the function should be
>> + * used for small, interface-like files, so it should not be huge (subjective) */
>> +#define VIR_FILE_READ_VALUE_STRING_MAX 4096
>
>either define it on top of the module or define it before the methods that make
>use of it, undefining it afterwards (*ScaledInt doesn't use this constant).
>
It should've used the symbol, let me check...
... oh, I see I left INT_BUFSIZE_BOUND(unsigned long long) there, which
Ought to be enough for everyone™, I'll move the define elsewhere.
>> +
>> +/**
>> + * virFileReadValueScaledInt:
>> + * @value: pointer to unsigned long long int to be filled in with the value
>> + * @format, ...: file to read from
>> + *
>> + * Read unsigned scaled int from @format and put it into @value.
>> + *
>> + * Return -2 for non-existing file, -1 on other errors and 0 if everything went
>> + * fine.
>> + */
>
>[...]
>
>> - rv = virSysfsGetCpuValueString(cpu, "topology/thread_siblings", &str);
>> + rv = virFileReadValueString(&str,
>> + SYSFS_SYSTEM_PATH
>> + "/cpu/cpu%u/topology/thread_siblings",
>> + cpu);
>
>Could we make the string constant part of the formatting string? Because the
>way it looks now signals an alert "oh, there's a missing comma delimiting
>arguments".
>
>> - if (virAsprintf(&sysfs_nodedir, "%s/node", virSysfsGetSystemPath()) < 0)
>> + if (virAsprintf(&sysfs_nodedir, SYSFS_SYSTEM_PATH "/node") < 0)
>
>Same here, could we preserve the explicit formatting string and pass the
>constant as an argument to it?
>
You mean ("%s/node", SYSFS_SYSTEM_PATH) ? Of course we could. I just
did it this way.
>> #define VIR_FROM_THIS VIR_FROM_NONE
>> @@ -52,7 +52,7 @@ test_virCapabilities(const void *opaque)
>> abs_srcdir, data->filename) < 0)
>> goto cleanup;
>>
>> - virSysfsSetSystemPath(dir);
>> + virFileMockAddPrefix("/sys/devices/system", dir);
>> caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
>>
>> if (!caps)
>> @@ -61,7 +61,7 @@ test_virCapabilities(const void *opaque)
>> if (virCapabilitiesInitNUMA(caps) < 0)
>> goto cleanup;
>>
>> - virSysfsSetSystemPath(NULL);
>> + virFileMockClearPrefixes();
>
>RemovePrefix I suppose? It doesn't matter much now, since there's one
>occurrence, but from the other change below I figured that we probably want to
>remove the one single prefix we just added.
>
Actually, following commits will add multiple overrides (well, at least
one) and this way they won't have to remove each one of them explicitly.
It's also more error-proof, e.g. if someone forgets to remove it.
>So I had a few nitpicks, but in principle, the patch's fine (even though I will
>probably silently disagree with some bits from now on;)), ACK.
>
>Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.