6:24 a.m.
On Fri, Sep 27, 2019 at 11:42:28AM +0100, Daniel P. Berrangé wrote:
On Fri, Sep 27, 2019 at 10:33:45AM +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 26, 2019 at 06:08:14PM +0200, Ján Tomko wrote:
> > On Tue, Sep 24, 2019 at 03:58:46PM +0100, Daniel P. Berrangé wrote:
> > > As part of an goal to eliminate Perl from libvirt build tools,
> > > rewrite the check-spacing.pl tool in Python.
> > >
> > > This was a straight conversion, manually going line-by-line to
> > > change the syntax from Perl to Python. Thus the overall structure
> > > of the file and approach is the same.
> > >
> > > Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
> > > ---
> > > Makefile.am | 2 +-
> > > build-aux/check-spacing.pl | 198 --------------------------------
> > > build-aux/check-spacing.py | 229 +++++++++++++++++++++++++++++++++++++
> > > cfg.mk | 4 +-
> > > 4 files changed, 232 insertions(+), 201 deletions(-)
> > > delete mode 100755 build-aux/check-spacing.pl
> > > create mode 100755 build-aux/check-spacing.py
> > >
> > > diff --git a/build-aux/check-spacing.py b/build-aux/check-spacing.py
> > > new file mode 100755
> > > index 0000000000..6b9f3ec1ba
> > > --- /dev/null
> > > +++ b/build-aux/check-spacing.py
> > > @@ -0,0 +1,229 @@
> > > +#!/usr/bin/env python
> > > +#
> > > +# Copyright (C) 2012-2019 Red Hat, Inc.
> > > +#
> > > +# check-spacing.pl: Report any usage of 'function (..args..)'
> > > +# Also check for other syntax issues, such as correct use of ';'
> > > +#
> > > +# This library is free software; you can redistribute it and/or
> > > +# modify it under the terms of the GNU Lesser General Public
> > > +# License as published by the Free Software Foundation; either
> > > +# version 2.1 of the License, or (at your option) any later version.
> > > +#
> > > +# This library is distributed in the hope that it will be useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > > +# Lesser General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU Lesser General Public
> > > +# License along with this library. If not, see
> > > +# <http://www.gnu.org/licenses/>;.
> > > +
> > > +from __future__ import print_function
> > > +
> > > +import re
> > > +import sys
> > > +
> > > +
> > > +def check_whitespace(filename):
> > > + errs = False
> > > + with open(filename, 'r') as fh:
> > > + quotedmetaprog =
re.compile(r"""'[";,=]'""")
> > > + quotedstringprog =
re.compile(r'''"(?:[^\\\"]|\\.)*"''')
> > > + commentstartprog =
re.compile(r'''^(.*)/\*.*$''')
> > > + commentendprog =
re.compile(r'''^.*\*/(.*)$''')
> > > + commentprog =
re.compile(r'''^(.*)/\*.*\*/(.*)''')
> > > + funcprog =
re.compile(r'''(\w+)\s\((?!\*)''')
> > > + keywordprog = re.compile(
> > > +
r'''^.*\b(?:if|for|while|switch|return)\(.*$''')
> > > + functypedefprog =
re.compile(r'''^.*\(\*\w+\)\s+\(.*$''')
> > > + whitespaceprog1 =
re.compile(r'''^.*\s\).*$''')
> > > + whitespaceprog2 =
re.compile(r'''^\s+\);?$''')
> > > + whitespaceprog3 =
re.compile(r'''^.*\((?!$)\s.*''')
> > > + commasemiprog1 =
re.compile(r'''.*\s[;,].*''')
> > > + commasemiprog2 = re.compile(r'''.*\S; ;
.*''')
> > > + commasemiprog3 =
re.compile(r'''^\s+;''')
> > > + semicolonprog = re.compile(r'''.*;[^
\\\n;)].*''')
> > > + commaprog = re.compile(r'''.*,[^
\\\n)}].*''')
> > > + assignprog1 = re.compile(r'''[^
]\b[!<>&|\-+*\/%\^=]?=''')
> > > + assignprog2 = re.compile(r'''=[^=
\\\n]''')
> > > + condstartprog =
re.compile(r'''^\s*(if|while|for)\b.*\{$''')
> > > + statementprog =
re.compile(r'''^[^;]*;[^;]*$''')
> > > + condendprog =
re.compile(r'''^\s*}\s*$''')
> > > +
> >
> > I think even with descriptive names for the regexes and the Python
> > rewrite, this script is hard to read and comes too close to be a C
> > parser.
>
> Yeah, trying to parse C code using regular expressions was not
> our most sensible decision.
>
> > Also, the execution time goes from 1.5s to 6.5s, which is longer than
> > all the other checks combined run on my 8-core machine.
>
> Thanks for pointing that out. I'll see if there's any low hanging
> fruit to optimize but I'm doubtful.
>
> I dont mind postponing this patch, as all patches in this series
> are independant of each other.
>
> > Can we get rid of it completely in favor of some proper formatting tool?
>
> I think that would be a good idea because this script only handles
> a tiny subset of formatting rules we like.
>
> > I have played with clang-format to try to match our style, the main
> > problems seem to be:
> > * pre-processor directives are indented by the same offset as code
> > (I see that cppi is specifically intended to add just one space)
>
> That's an interesting approach. I wouldn't object to such indentation
> style myself.
>
> > * function calls sometimes leave an empty opening parenthesis
>
> > * always rewrapping function arguments might create unnecessary churn
> > * parameters wrapping might not be smart enough, e.g. we like to do
> > virReportError(VIR_ERR_CODE, "%s",
> > _("string"));
> > in a lot of places.
>
> Yeah these last two points are the ones I struggled with too when I
> looked at clang-format 6 months back.
Another tool is "uncrustify" which seems to have even more configurability
than clang-format does. On my SSD it takes 30 seconds to run over all the
.c file.
The attached lv.cfg file is an uncrustify config that is reasonably
close to our current style.
To see what it does, run this in your source tree:
find src/ -name '*.c' | xargs uncrustify --replace -c uncrustify.cfg
There's quite a few real mistakes it is correcting for us.
Most of the big stuff is due to us having followed inconsistent
rules in different parts of the source tree. No matter which tool
we pick will suffer this as we have to pick one style
switch/case indents are the big one - sometimes we line up case + switch,
sometimes we don't.
src/access/viraccessdriverpolkit.c | 16 +--
src/admin/admin_server_dispatch.c | 4 +-
src/bhyve/bhyve_capabilities.c | 2 +-
src/bhyve/bhyve_command.c | 66 ++++++-------
src/bhyve/bhyve_driver.c | 14 +--
src/bhyve/bhyve_monitor.c | 4 +-
src/bhyve/bhyve_parse_command.c | 96 +++++++++---------
src/bhyve/bhyve_process.c | 32 +++---
src/conf/capabilities.c | 2 +-
src/conf/checkpoint_conf.c | 2 +-
src/conf/cpu_conf.c | 8 +-
src/conf/device_conf.c | 18 ++--
src/conf/domain_addr.c | 10 +-
src/conf/domain_audit.c | 8 +-
src/conf/domain_capabilities.c | 6 +-
src/conf/domain_conf.c | 414
+++++++++++++++++++++++++++++++++++++++---------------------------------------
src/conf/domain_event.c | 434
+++++++++++++++++++++++++++++++++++++++++-----------------------------------------
src/conf/interface_conf.c | 182 +++++++++++++++++------------------
src/conf/netdev_vlan_conf.c | 2 +-
src/conf/network_conf.c | 60 ++++++------
src/conf/network_event.c | 20 ++--
src/conf/node_device_conf.c | 20 ++--
src/conf/node_device_event.c | 30 +++---
src/conf/node_device_util.c | 2 +-
src/conf/numa_conf.c | 4 +-
src/conf/nwfilter_conf.c | 684
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------
src/conf/nwfilter_ipaddrmap.c | 2 +-
src/conf/nwfilter_params.c | 12 +--
src/conf/secret_event.c | 30 +++---
src/conf/snapshot_conf.c | 8 +-
src/conf/storage_adapter_conf.c | 6 +-
src/conf/storage_conf.c | 76 ++++++---------
src/conf/storage_event.c | 32 +++---
src/conf/virchrdev.c | 32 +++---
src/conf/virinterfaceobj.c | 9 +-
src/conf/virnetworkobj.c | 18 ++--
src/conf/virnetworkportdef.c | 12 +--
src/conf/virnodedeviceobj.c | 21 ++--
src/conf/virnwfilterobj.c | 2 +-
src/conf/virsecretobj.c | 9 +-
src/conf/virstorageobj.c | 24 +++--
src/cpu/cpu_ppc64.c | 8 +-
src/cpu/cpu_s390.c | 8 +-
src/cpu/cpu_x86.c | 76 +++++++--------
src/datatypes.c | 2 +-
src/esx/esx_driver.c | 144 +++++++++++++--------------
src/esx/esx_interface_driver.c | 2 +-
src/esx/esx_network_driver.c | 62 ++++++------
src/esx/esx_storage_backend_iscsi.c | 14 +--
src/esx/esx_storage_backend_vmfs.c | 30 +++---
src/esx/esx_storage_driver.c | 4 +-
src/esx/esx_util.c | 6 +-
src/esx/esx_vi.c | 332
+++++++++++++++++++++++++++++++--------------------------------
src/esx/esx_vi_methods.c | 58 +++++------
src/esx/esx_vi_types.c | 402
++++++++++++++++++++++++++++++++++++++--------------------------------------
src/hyperv/hyperv_driver.c | 84 ++++++++--------
src/hyperv/hyperv_wmi.c | 374
+++++++++++++++++++++++++++++++++++-----------------------------------
src/interface/interface_backend_netcf.c | 80 +++++++--------
src/interface/interface_backend_udev.c | 126 ++++++++++++------------
src/keycodemapdb | 0
src/libvirt-domain.c | 110 ++++++++++-----------
src/libvirt-host.c | 4 +-
src/libvirt-interface.c | 6 +-
src/libvirt-lxc.c | 4 +-
src/libvirt-network.c | 2 +-
src/libvirt-nodedev.c | 4 +-
src/libvirt-storage.c | 10 +-
src/libvirt.c | 18 ++--
src/libxl/libxl_capabilities.c | 11 ++-
src/libxl/libxl_conf.c | 404
++++++++++++++++++++++++++++++++++++++--------------------------------------
src/libxl/libxl_domain.c | 40 ++++----
src/libxl/libxl_driver.c | 636
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------------
src/libxl/libxl_migration.c | 30 +++---
src/libxl/xen_common.c | 38 ++++----
src/libxl/xen_xl.c | 184 +++++++++++++++++------------------
src/libxl/xen_xm.c | 54 +++++------
src/locking/domain_lock.c | 24 ++---
src/locking/lock_daemon.c | 126 ++++++++++++------------
src/locking/lock_manager.c | 4 +-
src/logging/log_daemon.c | 122 +++++++++++------------
src/lxc/lxc_cgroup.c | 5 +-
src/lxc/lxc_container.c | 14 +--
src/lxc/lxc_controller.c | 6 +-
src/lxc/lxc_domain.c | 16 +--
src/lxc/lxc_driver.c | 94 +++++++++---------
src/lxc/lxc_fuse.c | 2 +-
src/lxc/lxc_native.c | 18 ++--
src/lxc/lxc_process.c | 34 +++----
src/network/bridge_driver.c | 52 +++++-----
src/node_device/node_device_hal.c | 6 +-
src/node_device/node_device_udev.c | 6 +-
src/nwfilter/nwfilter_dhcpsnoop.c | 104 ++++++++++----------
src/nwfilter/nwfilter_driver.c | 12 +--
src/nwfilter/nwfilter_ebiptables_driver.c | 160 +++++++++++++++---------------
src/nwfilter/nwfilter_gentech_driver.c | 2 +-
src/nwfilter/nwfilter_learnipaddr.c | 42 ++++----
src/openvz/openvz_conf.c | 10 +-
src/openvz/openvz_driver.c | 25 +++--
src/phyp/phyp_driver.c | 80 +++++++--------
src/qemu/qemu_agent.c | 32 +++---
src/qemu/qemu_alias.c | 4 +-
src/qemu/qemu_block.c | 27 +++---
src/qemu/qemu_blockjob.c | 8 +-
src/qemu/qemu_capabilities.c | 20 ++--
src/qemu/qemu_cgroup.c | 22 ++---
src/qemu/qemu_command.c | 574
++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------
src/qemu/qemu_conf.c | 54 +++++------
src/qemu/qemu_domain.c | 378
+++++++++++++++++++++++++++++++++++------------------------------------
src/qemu/qemu_domain_address.c | 36 +++----
src/qemu/qemu_driver.c | 526
+++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------
src/qemu/qemu_hostdev.c | 2 +-
src/qemu/qemu_hotplug.c | 72 +++++++-------
src/qemu/qemu_interface.c | 14 +--
src/qemu/qemu_migration.c | 104 ++++++++++----------
src/qemu/qemu_migration_cookie.c | 16 +--
src/qemu/qemu_migration_params.c | 4 +-
src/qemu/qemu_monitor.c | 12 +--
src/qemu/qemu_monitor_json.c | 42 ++++----
src/qemu/qemu_monitor_text.c | 6 +-
src/qemu/qemu_process.c | 94 +++++++++---------
src/qemu/qemu_security.c | 8 +-
src/qemu/qemu_tpm.c | 34 +++----
src/remote/remote_daemon.c | 148 ++++++++++++++--------------
src/remote/remote_daemon_config.c | 6 +-
src/remote/remote_daemon_dispatch.c | 22 ++---
src/remote/remote_driver.c | 28 +++---
src/rpc/virnetclient.c | 44 ++++-----
src/rpc/virnetclientstream.c | 8 +-
src/rpc/virnetlibsshsession.c | 4 +-
src/rpc/virnetserverclient.c | 2 +-
src/rpc/virnetserverprogram.c | 2 +-
src/rpc/virnetserverservice.c | 2 +-
src/rpc/virnetsocket.c | 32 +++---
src/rpc/virnetsshsession.c | 8 +-
src/rpc/virnettlscontext.c | 4 +-
src/secret/secret_driver.c | 2 +-
src/secret/secret_util.c | 6 +-
src/security/security_apparmor.c | 28 +++---
src/security/security_dac.c | 8 +-
src/security/security_manager.c | 16 +--
src/security/security_selinux.c | 6 +-
src/security/security_util.c | 4 +-
src/security/virt-aa-helper.c | 204 +++++++++++++++++++--------------------
src/storage/storage_backend_disk.c | 92 +++++++++---------
src/storage/storage_backend_fs.c | 4 +-
src/storage/storage_backend_gluster.c | 5 +-
src/storage/storage_backend_logical.c | 28 +++---
src/storage/storage_backend_mpath.c | 4 +-
src/storage/storage_backend_rbd.c | 42 ++++----
src/storage/storage_driver.c | 58 +++++------
src/storage/storage_util.c | 16 +--
src/test/test_driver.c | 456
+++++++++++++++++++++++++++++++++++++++++++-------------------------------------------
src/util/viralloc.c | 4 +-
src/util/virauthconfig.c | 4 +-
src/util/virbuffer.c | 2 +-
src/util/vircgroup.c | 22 ++---
src/util/vircommand.c | 6 +-
src/util/virconf.c | 84 ++++++++--------
src/util/vircrypto.c | 4 +-
src/util/virdbus.c | 4 +-
src/util/virdnsmasq.c | 6 +-
src/util/virerror.c | 357
+++++++++++++++++++++++++++++++++++++++++++------------------------
src/util/virevent.c | 12 +--
src/util/vireventpoll.c | 4 +-
src/util/virfdstream.c | 8 +-
src/util/virfile.c | 12 +--
src/util/virfirewall.c | 2 +-
src/util/virfirewalld.c | 6 +-
src/util/virhook.c | 28 +++---
src/util/virhostdev.c | 14 +--
src/util/virinitctl.c | 46 ++++-----
src/util/virlease.c | 4 +-
src/util/virlog.c | 38 ++++----
src/util/virmacaddr.c | 2 +-
src/util/virnetdev.c | 140 +++++++++++++--------------
src/util/virnetdevbandwidth.c | 6 +-
src/util/virnetdevbridge.c | 14 +--
src/util/virnetdevip.c | 8 +-
src/util/virnetdevmacvlan.c | 14 +--
src/util/virnetdevvportprofile.c | 14 +--
src/util/virnetlink.c | 14 +--
src/util/virnodesuspend.c | 8 +-
src/util/virnuma.c | 2 +-
src/util/virpci.c | 20 ++--
src/util/virprocess.c | 22 ++---
src/util/virresctrl.c | 2 +-
src/util/virrotatingfile.c | 2 +-
src/util/virscsivhost.c | 2 +-
src/util/virsocketaddr.c | 14 +--
src/util/virstorageencryption.c | 4 +-
src/util/virstoragefile.c | 93 +++++++++---------
src/util/virsysinfo.c | 6 +-
src/util/virsystemd.c | 12 +--
src/util/virtime.c | 10 +-
src/util/virtpm.c | 8 +-
src/util/virtypedparam.c | 18 ++--
src/util/viruri.c | 4 +-
src/util/virusb.c | 8 +-
src/util/virutil.c | 2 +-
src/util/virvhba.c | 14 +--
src/util/virxml.c | 9 +-
src/vbox/vbox_V5_0.c | 2 +-
src/vbox/vbox_V5_1.c | 2 +-
src/vbox/vbox_V5_2.c | 2 +-
src/vbox/vbox_common.c | 250
+++++++++++++++++++++++------------------------
src/vbox/vbox_network.c | 2 +-
src/vbox/vbox_snapshot_conf.c | 106 ++++++++++----------
src/vbox/vbox_storage.c | 2 +-
src/vbox/vbox_tmpl.c | 86 ++++++++---------
src/vmware/vmware_conf.c | 68 ++++++-------
src/vmware/vmware_driver.c | 8 +-
src/vmx/vmx.c | 652
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------
src/vz/vz_driver.c | 64 ++++++------
src/vz/vz_sdk.c | 94 +++++++++---------
src/vz/vz_utils.c | 8 +-
216 files changed, 6371 insertions(+), 6256 deletions(-)
If anyone fancies taking this further feel free. I'm not going to work on
uncrustify right now. I'll just drop this whitespace patch rewrite and
focus on the other things as a priority.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|