Re: [libvirt] [PATCH V2 1/2] util: Add more virsysfs functions for handling resctrl sysfs

> +# util/virresctrl.h > +virResCtrlAvailable; > +virResCtrlInit; > + > This has nothing to do in the patch

> # util/virrotatingfile.h > virRotatingFileReaderConsume; > virRotatingFileReaderFree; > @@ -2626,13 +2630,18 @@ virSysfsGetCpuValueString; > virSysfsGetCpuValueUint; > virSysfsGetNodeValueBitmap; > virSysfsGetNodeValueString; > +virSysfsGetResctrlInfoString; > +virSysfsGetResctrlInfoUint; > +virSysfsGetResctrlPath; > +virSysfsGetResctrlString; > +virSysfsGetResctrlUint; > virSysfsGetSystemPath; > virSysfsGetValueBitmap; > virSysfsGetValueInt; > virSysfsGetValueString; > +virSysfsSetResctrlPath; > virSysfsSetSystemPath; > > - Don't remove the line. Every time you are doing a change in the code (and I mean any code), try being consistent. If the code follows some style, don't fight it, but go with it. Not everything can be written in the coding style. As you can see here, all files are separated by two lines. When you remove this, you make the file inconsistent. Also, I mentioned many times before that you should run make check and make syntax-check between patches, and I'm sure our contribution guidelines mention that make check must pass between patches. This change should not pass the checks. It's fine every now and then, we all forget, but I'm trying to run make check and make syntax-check after each patch before sending it. Useful command to use from your topic branch is something along the lines of: git rebase -ix 'make -j9 check && make -j9 syntax-check' master (written by hand from memory, there might be typo, I have a script and bunch of aliases for that)

> # util/virsysinfo.h > virSysinfoBaseBoardDefClear; > virSysinfoBIOSDefFree; > diff --git a/src/util/virsysfs.c b/src/util/virsysfs.c > index 7a98b48..97808be 100644 > --- a/src/util/virsysfs.c > +++ b/src/util/virsysfs.c > [...] > @@ -227,3 +242,88 @@ virSysfsGetNodeValueBitmap(unsigned int node, > VIR_FREE(path); > return ret; > } > + > +int > +virSysfsGetResctrlString(const char* file, > 'char *file' instead of 'char* file' > + char **value) > +{ > + char *path = NULL; > + int ret = -1; > + if (virAsprintf(&path, "%s/%s", sysfs_resctrl_path, file) < 0) > + return -1; > + > + if (!virFileExists(path)) { > + ret = -2; > + goto cleanup; > + } > + > Now the question is; is it possible for some file to be missing there? I mean some file we'd expect? For the system path, the -2 return value is there because we need to work on older systems with older kernels and the files were being added in multiple releases. If there is no need for distinguishing that, then there is no point in explicitly checking for the file to be existing.

And that leads me to another point. Since this patch is by itself, it's not visible how it is used. It looks good (not considering the things pointed out above), but there is no point in merging it until there is a value added. It's just added dead code. But it's something that we'll use, surely.

Martin P.S.: I just sent [1] the next couple of patches (again, first ones are just fixing some bullocks I found out that were left in the code -- that just happens when you're working on a codebase with lots of legacy code), but it adds host cache information to the capabilities. If you have a minute or two, feel free to check it out and let me know what you think.

[1] https://www.redhat.com/archives/libvir-list/2017-March/msg01592.html

-- libvir-list mailing list libvir-list(a)redhat.com (mailto:libvir-list@redhat.com) https://www.redhat.com/mailman/listinfo/libvir-list