2:42 a.m.
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(a)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