[libvirt] [PATCH 2/3] conf: Clean up nodedev code

[Michal Privoznik]

On 03/02/2017 10:16 AM, Peter Krempa wrote:
> On Thu, Mar 02, 2017 at 10:04:24 +0100, Michal Privoznik wrote:
>> On 03/02/2017 09:58 AM, Peter Krempa wrote:
>>> On Wed, Mar 01, 2017 at 19:27:15 -0500, John Ferlan wrote:
>>>> Alter the static functions from virNodeDev* to just nodeDev* as a visual
>>>> cue to determine which are local or not when reading code.
>>>>
>>>> Cleanup spacing between functions, function defs, etc. to match more modern
>>>> techniques used in libvirt
>>>>
>>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>>> ---
>>>>  src/conf/node_device_conf.c | 476 +++++++++++++++++++++++---------------------
>>>>  src/conf/virnodedeviceobj.c | 128 ++++++------
>>>>  2 files changed, 322 insertions(+), 282 deletions(-)
>>>>
>>>> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
>>>> index bc36527..09e815a 100644
>>>> --- a/src/conf/node_device_conf.c
>>>> +++ b/src/conf/node_device_conf.c
>>>> @@ -72,9 +72,9 @@ VIR_ENUM_IMPL(virNodeDevDRM, VIR_NODE_DEV_DRM_LAST,
>>>>                "render")
>>>>  
>>>>  static int
>>>> -virNodeDevCapsDefParseString(const char *xpath,
>>>> -                             xmlXPathContextPtr ctxt,
>>>> -                             char **string)
>>>> +nodeDevCapsDefParseString(const char *xpath,
>>>> +                          xmlXPathContextPtr ctxt,
>>>> +                          char **string)
>>>
>>> Please don't remove the vir prefix. The coding style tries to converge
>>> to having them everywhere.
>>>
>>
>> Why? If a function is static, we can be sure it's not called from
>> outside of the file. Moreover, I'd direct your attention to recent
> 
> The static function name may still show up in logs and backtraces. It's
> better to see what belongs to libvirt and what does not.

I don't know about your backtraces, but mine show the filename too.
Either actual source file or .so file where the symbol comes from. We
can rely on that.
Or if a static function of ours calls a function from another library,
it's going to be visible too:

#0 virExposedAPI
#1 someStaticFunc
#2 someOtherStaticFunc
#3 glfs_creat
#4 yetAnotherStaticFunc

Frankly, I don't have any difficulties determining the boundaries of
libvirt's part of the stack.

> 
>> commit of f557b3351e0b6d for instance. In fact whole qemu driver serves
>> as a great example: it's "static int qemuDomain*()" not "static vir
>> virQEMUDomain*()".
> 
> Well, if somebody changes the whole src/qemu_driver.c to use the vir
> prefix I'll use the vir prefix. It's called consistency.

No. I'm against that change. qemuDomain*() makes perfect sense. It's in
qemu driver, it's static, it shows up in backtrace, etc.

> 
>> In fact, I'd suggest the opposite rule - use "vir" prefix only if
>> function is shared between modules. For instance  virFileCopyACLs should
>> have the vir prefix because it's exported. virFileRewriteStrHelper
>> should not have the prefix because it's static.
> 
> If we codify this rule, this will mean that once we will export the
> function you'll have to rename it. Also once you unexport it, you'll
> have to rename it. I think that's silly. I'd stick with the prefix
> everywhere.

Yes when you want to make a change you have to make a change. As I
replied to Dan, exporting a func would require change in just one file.
and the change is going to be small anyway.

> 
>> The advantage of this approach is that one can immediately tell just
>> from the name if the function is exported or not.
> 
> I don't think this is very useful. If you are refactoring it you should
> check all callers anyways and other than that the information is not
> very useful.
> 

If a function is static, all you need to check is just that one file.
You don't need to grep for other locations.