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

[Daniel P. Berrange]

On Thu, Mar 02, 2017 at 10:35:57AM +0100, Michal Privoznik wrote:
> On 03/02/2017 10:30 AM, Daniel P. Berrange wrote:
> > On Thu, Mar 02, 2017 at 10:04:24AM +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
> >> commit of f557b3351e0b6d for instance. In fact whole qemu driver serves
> >> as a great example: it's "static int qemuDomain*()" not "static vir
> >> virQEMUDomain*()".
> >>
> >> 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.
> >> The advantage of this approach is that one can immediately tell just
> >> from the name if the function is exported or not.
> > 
> > No, this is a bad rule because it causes us to rename code when we
> > inevitably make static functions non-static. We want a naming rule
> > that is standardized & stable long term. Convering to use 'vir'
> > prefix everywhere gives us that.
> 
> I fail to see why would renaming be a problem. Going from static to
> non-static would require to change all the callers in just one file
> (which is not going to have much callers anyway). Going the opposite
> direction - well, there are no callers in other files anyway. If there
> were we couldn't make the funtion static.

Even though all the code may be in one file, it can still create a huge
amount of code churn. eg if we make qemuDomObjFromDomain non-static,
that'd involve updating 125 lines in the qemu_driver.c file. That's alot
of self-inflicted pain for backporting patches that come afterwards.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|