<div><br></div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><blockquote type="cite"><div><div>+# util/virresctrl.h</div><div>+virResCtrlAvailable;</div><div>+virResCtrlInit;</div><div>+</div></div></blockquote><div><br></div><div>This has nothing to do in the patch</div><div><br></div></div></div></span></blockquote><div>Okay, this should be involved by mistake while rebasing. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div></div><blockquote type="cite"><div><div># util/virrotatingfile.h</div><div>virRotatingFileReaderConsume;</div><div>virRotatingFileReaderFree;</div><div>@@ -2626,13 +2630,18 @@ virSysfsGetCpuValueString;</div><div>virSysfsGetCpuValueUint;</div><div>virSysfsGetNodeValueBitmap;</div><div>virSysfsGetNodeValueString;</div><div>+virSysfsGetResctrlInfoString;</div><div>+virSysfsGetResctrlInfoUint;</div><div>+virSysfsGetResctrlPath;</div><div>+virSysfsGetResctrlString;</div><div>+virSysfsGetResctrlUint;</div><div>virSysfsGetSystemPath;</div><div>virSysfsGetValueBitmap;</div><div>virSysfsGetValueInt;</div><div>virSysfsGetValueString;</div><div>+virSysfsSetResctrlPath;</div><div>virSysfsSetSystemPath;</div><div><br></div><div>-</div></div></blockquote><div><br></div><div>Don't remove the line. Every time you are doing a change in the code</div><div>(and I mean any code), try being consistent. If the code follows some</div><div>style, don't fight it, but go with it. Not everything can be written in</div><div>the coding style. As you can see here, all files are separated by two</div><div>lines. When you remove this, you make the file inconsistent.</div><div><br></div><div>Also, I mentioned many times before that you should run make check and</div><div>make syntax-check between patches, and I'm sure our contribution</div><div>guidelines mention that make check must pass between patches. This</div><div>change should not pass the checks. It's fine every now and then, we all</div><div>forget, but I'm trying to run make check and make syntax-check after</div><div>each patch before sending it. Useful command to use from your topic</div><div>branch is something along the lines of:</div><div><br></div><div> git rebase -ix 'make -j9 check && make -j9 syntax-check' master</div><div> (written by hand from memory, there might be typo, I have a script and</div><div> bunch of aliases for that)</div><div><br></div></div></div></span></blockquote><div>Sure, Daniel had reminded me before, I missed it this time, will keep in mind next path. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div></div><blockquote type="cite"><div><div># util/virsysinfo.h</div><div>virSysinfoBaseBoardDefClear;</div><div>virSysinfoBIOSDefFree;</div><div>diff --git a/src/util/virsysfs.c b/src/util/virsysfs.c</div><div>index 7a98b48..97808be 100644</div><div>--- a/src/util/virsysfs.c</div><div>+++ b/src/util/virsysfs.c</div></div></blockquote><div><br></div><div>[...]</div><div><br></div><blockquote type="cite"><div><div>@@ -227,3 +242,88 @@ virSysfsGetNodeValueBitmap(unsigned int node,</div><div> VIR_FREE(path);</div><div> return ret;</div><div>}</div><div>+</div><div>+int</div><div>+virSysfsGetResctrlString(const char* file,</div></div></blockquote><div><br></div><div>'char *file' instead of 'char* file'</div><div><br></div><blockquote type="cite"><div><div>+ char **value)</div><div>+{</div><div>+ char *path = NULL;</div><div>+ int ret = -1;</div><div>+ if (virAsprintf(&path, "%s/%s", sysfs_resctrl_path, file) < 0)</div><div>+ return -1;</div><div>+</div><div>+ if (!virFileExists(path)) {</div><div>+ ret = -2;</div><div>+ goto cleanup;</div><div>+ }</div><div>+</div></div></blockquote><div><br></div><div>Now the question is; is it possible for some file to be missing there?</div><div>I mean some file we'd expect? For the system path, the -2 return value</div><div>is there because we need to work on older systems with older kernels and</div><div>the files were being added in multiple releases. If there is no need</div><div>for distinguishing that, then there is no point in explicitly checking</div><div>for the file to be existing.</div><div><br></div></div></div></span></blockquote><div>No, we won’t expect file doesn’t existed. I will remove this checking </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div></div><div>And that leads me to another point. Since this patch is by itself, it's</div><div>not visible how it is used. It looks good (not considering the things</div><div>pointed out above), but there is no point in merging it until there is a</div><div>value added. It's just added dead code. But it's something that we'll</div><div>use, surely.</div><div><br></div></div></div></span></blockquote><div>Okay, I can add this patch to CAT supporting patch set as the first patch. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div></div><div>Martin</div><div><br></div><div>P.S.: I just sent [1] the next couple of patches (again, first ones are</div><div> just fixing some bullocks I found out that were left in the code</div><div> -- that just happens when you're working on a codebase with lots</div><div> of legacy code), but it adds host cache information to the</div><div> capabilities. If you have a minute or two, feel free to check it</div><div> out and let me know what you think.</div></div></div></span></blockquote><div>Sure, I see you have added fake cache id, that’s good, I can drop it. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div><br></div><div>[1] <a href="https://www.redhat.com/archives/libvir-list/2017-March/msg01592.html">https://www.redhat.com/archives/libvir-list/2017-March/msg01592.html</a></div></div></div></span></blockquote><div><br></div><div>I have one propose, not sure it’s a good or bad one:</div><div><br></div><div>I would like to suggest that let the developer keep focus on adding features,</div><div>then refactor some uitls functions later, developers won’t have all knowledge</div><div>for what’s the function should be go into which utils, and this will bring time</div><div>wasting on keeping rebasing rebasing ……</div><div><br></div><div>- Eli</div><div> </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div>--</div><div>libvir-list mailing list</div><div><a href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a></div><div><a href="https://www.redhat.com/mailman/listinfo/libvir-list">https://www.redhat.com/mailman/listinfo/libvir-list</a></div></div></div></span>
</blockquote>
<div>
<br>
</div>