[libvirt] [PATCH v2 05/12] util: Remove virsysfs and instead enhance virFileReadValue* functions

[Erik Skultety]

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 at 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