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