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

Eli Qiao <qiaoliyong@xxxxxxxxx> · Fri, 31 Mar 2017 09:26:34 +0800

+# util/virresctrl.h
+virResCtrlAvailable;
+virResCtrlInit;
+

This has nothing to do in the patch

Okay, this should be involved by mistake while rebasing. 
# 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)

Sure, Daniel had reminded me before, I missed it this time, will keep in mind next path. 
# 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.

No, we won’t expect file doesn’t existed. I will remove this checking 
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.

Okay, I can add this patch to CAT supporting patch set as the first patch. 
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.
Sure, I see you have added fake cache id, that’s good, I can drop it. 

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

I have one propose, not sure it’s a good or bad one:

I would like to suggest that let the developer keep focus on adding features,
then refactor some uitls functions later, developers won’t have all knowledge
for what’s the function should be go into which utils, and this will bring time
wasting on keeping rebasing rebasing ……

- Eli
 
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
                 
                 
                 
                 
                
                 
                
                    

                
            --
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



