4:36 a.m.
On Fri, Apr 07, 2017 at 09:42:23AM +0200, Erik Skultety wrote:
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(a)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.
I wanted to do that, but if I change the virFileRead* functions, I have
to change the callers in virsysfs.c, file that I will just remove, just
so that the build doesn't fail between those two commits, and that would
result in pointless changes. I don't see how the removal of that file
interferes with the rest of the changes, just skip the removal hunks.
Or you can limit the files for which you see diffs if you look at the
patch outside of the MUA.
[...]
> /**
> * 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...).
Sure, as I said, I would welcome any ideas to make stuff better. But I
couldn't come up with any other one either. That's probably caused by
the fact that I see this one as the cleanest, nicest approach we can
have. If you mention what particular parts you don't like, I can at
least try to meet you half-way. Maybe we'll think of something even
better.
> {
> + 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
And cleanup the path *after* the virFileRead* function is called both if
it failed and if it did not fail, making it harder to call multiple
virFileRead* after each other, effectively making you do 3 or 4 more
lines for each call. Those were the lines that were bothering me, way
more than three you mentioned.
[...]
> +
> +/* 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).
It should've used the symbol, let me check...
... oh, I see I left INT_BUFSIZE_BOUND(unsigned long long) there, which
Ought to be enough for everyone™, I'll move the define elsewhere.
> +
> +/**
> + * 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?
You mean ("%s/node", SYSFS_SYSTEM_PATH) ? Of course we could. I just
did it this way.
> #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.
Actually, following commits will add multiple overrides (well, at least
one) and this way they won't have to remove each one of them explicitly.
It's also more error-proof, e.g. if someone forgets to remove it.
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