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

[Martin Kletzander]

On Thu, Mar 30, 2017 at 04:07:11PM +0800, Eli Qiao wrote:
>Extended /sys/fs/resctrl sysfs handling.
>
>Signed-off-by: Eli Qiao <liyong.qiao at intel.com>
>---
> src/libvirt_private.syms |  11 ++++-
> src/util/virsysfs.c      | 102 ++++++++++++++++++++++++++++++++++++++++++++++-
> src/util/virsysfs.h      |  16 ++++++++
> src/util/virsysfspriv.h  |   1 +
> 4 files changed, 128 insertions(+), 2 deletions(-)
>
>diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>index b551cb8..b03ad5b 100644
>--- a/src/libvirt_private.syms
>+++ b/src/libvirt_private.syms
>@@ -2388,6 +2388,10 @@ virRandomGenerateWWN;
> virRandomInt;
>
>
>+# 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170330/f3e25754/attachment-0001.sig>