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@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.
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 4096either 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
