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@xxxxxxxxxx> >>>> --- >>>> 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. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
