[libvirt] [PATCH v3 2/8] util: Remove virsysfs and instead enhance virFileReadValue* functions

[Erik Skultety]

On Tue, Apr 25, 2017 at 01:10:26PM +0200, Martin Kletzander wrote:
> It is no longer needed thanks to the great virfilemock.c.  And this

s/mock/wrapper

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

you didn't move the define :).

> +
> +/**
> + * 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;
>  }
>

[...]

>  int
>  virHostCPUGetCore(unsigned int cpu, unsigned int *core)
>  {
> -    int ret = virSysfsGetCpuValueUint(cpu, "topology/core_id", core);
> +    int ret = virFileReadValueUint(core,
> +                                   "%s/cpu/cpu%u/topology/core_id",
> +                                   SYSFS_SYSTEM_PATH, cpu);
>
>      /* If the file is not there, it's 0 */
>      if (ret == -2)
> @@ -268,7 +272,9 @@ virHostCPUGetSiblingsList(unsigned int cpu)
>      virBitmapPtr ret = NULL;
>      int rv = -1;
>
> -    rv = virSysfsGetCpuValueBitmap(cpu, "topology/thread_siblings_list", &ret);
> +    rv = virFileReadValueBitmap(&ret,
> +                                "%s/cpu/cpu%u/topology/thread_siblings_list",
> +                                SYSFS_SYSTEM_PATH, cpu);

So, I like that you put the constant as argument to the formatting string
instead of concatenation since v1...

>      if (rv == -2) {
>          /* If the file doesn't exist, the threadis its only sibling */
>          ret = virBitmapNew(cpu + 1);
> @@ -615,7 +621,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)

In which case we should stay consistent and ^this should be adjusted
accordingly - applies to the rest of the asprintfs below.

ACK with the nits fixed.
Erik