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