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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
