Re: [libvirt] [PATCH 2/3] conf: Clean up nodedev code
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(a)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.