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

[Martin Kletzander]

On Fri, Mar 31, 2017 at 09:26:34AM +0800, Eli Qiao wrote:
>
>> > +# 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 ……
>

I'm not sure I understand the whole sentence (paragraph).  Everyone can
add functionality, but not many people re refactor code.  And if we only
add functionality, the code will become unreadable and unbearable to
work with.  I can skip the trivial patches on top, but I can also just
push them as trivial.  Maybe I misunderstood what you were saying,
please explain what you mean in case I misunderstood.

>- Eli
>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com (mailto:libvir-list at redhat.com)
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>>
>
>
-------------- 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/20170331/6470728f/attachment-0001.sig>