Re: [libvirt] [PATCH v5 10/23] src: rewrite driver name checker in Python

On 11/11/19 9:38 AM, Daniel P. Berrangé wrote: > As part of an goal to eliminate Perl from libvirt build tools, > rewrite the check-drivername.pl tool in Python. > > This was mostly 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. > > In testing though it was discovered the existing code was broken > since it hadn't been updated after driver.h was split into many > files. Since the old code is being thrown away, the fix was done > as part of the rewrite rather than split into a separate commit. > > Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > --- > Makefile.am | 1 + > scripts/check-drivername.py | 114 ++++++++++++++++++++++++++++++++++++ > src/Makefile.am | 18 ++++-- > src/check-drivername.pl | 83 -------------------------- > 4 files changed, 129 insertions(+), 87 deletions(-) > create mode 100644 scripts/check-drivername.py > delete mode 100755 src/check-drivername.pl > > diff --git a/Makefile.am b/Makefile.am > index afed409c94..7155ab6ce9 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -47,6 +47,7 @@ EXTRA_DIST = \ > AUTHORS.in \ > scripts/augeas-gentest.py \ > scripts/check-aclperms.py \ > + scripts/check-drivername.py \ > scripts/check-spacing.py \ > scripts/check-symfile.py \ > scripts/check-symsorting.py \ > diff --git a/scripts/check-drivername.py b/scripts/check-drivername.py > new file mode 100644 > index 0000000000..3226ee7962 > --- /dev/null > +++ b/scripts/check-drivername.py > @@ -0,0 +1,114 @@ > +#!/usr/bin/env python > +# > +# Copyright (C) 2013-2019 Red Hat, Inc. > +# > +# 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 > + > +drvfiles = [] > +symfiles = [] > +for arg in sys.argv: > + if arg.endswith(".h"): > + drvfiles.append(arg) > + else: > + symfiles.append(arg) > + > +symbols = {} > + > +for symfile in symfiles: > + with open(symfile, "r") as fh: > + for line in fh: > + m = re.search(r'''^\s*(vir\w+)\s*;\s*$''', line) > + if m is not None: > + symbols[m.group(1)] = True > + > +status = 0 > +for drvfile in drvfiles: > + with open(drvfile, "r") as fh: > + for line in fh: > + if line.find("virDrvConnectSupportsFeature") != -1: > + continue > + I think this bit is just another mechanism for the skip list below. I can't really figure out what other purpose it could be serving

> + m = re.search(r'''\*(virDrv\w+)\s*\)''', line) > + if m is not None: > + drv = m.group(1) > + > + skip = [ > + "virDrvStateInitialize", > + "virDrvStateCleanup", > + "virDrvStateReload", > + "virDrvStateStop", > + "virDrvConnectURIProbe", > + "virDrvDomainMigratePrepare", > + "virDrvDomainMigratePrepare2", > + "virDrvDomainMigratePrepare3", > + "virDrvDomainMigratePrepare3Params", > + "virDrvDomainMigratePrepareTunnel", > + "virDrvDomainMigratePrepareTunnelParams", > + "virDrvDomainMigratePrepareTunnel3", > + "virDrvDomainMigratePrepareTunnel3Params", > + "virDrvDomainMigratePerform", > + "virDrvDomainMigratePerform3", > + "virDrvDomainMigratePerform3Params", > + "virDrvDomainMigrateConfirm", > + "virDrvDomainMigrateConfirm3", > + "virDrvDomainMigrateConfirm3Params", > + "virDrvDomainMigrateBegin", > + "virDrvDomainMigrateBegin3", > + "virDrvDomainMigrateBegin3Params", > + "virDrvDomainMigrateFinish", > + "virDrvDomainMigrateFinish2", > + "virDrvDomainMigrateFinish3", > + "virDrvDomainMigrateFinish3Params", > + "virDrvStreamInData", > + ] > + if drv in skip: > + continue > + > + sym = drv.replace("virDrv", "vir") > + > + if sym not in symbols: > + print("Driver method name %s doesn't match public API" % > + drv) Missing status = 1 here > + continue > + > + m = re.search(r'''^\*(vir\w+)\s*\)''', line) > + if m is not None: > + name = m.group(1) > + print("Bogus name %s" % name) > + status = 1 > + continue > + Not quite sure what this condition is supposed to be catching. It's trying to match lines that start with '*vir'. I guess it's trying to warning against anything that doesn't start with 'virDrv' and instead plain 'vir', but the anchor usage is messed up. But it looks pre-existing. Anyways, the two other error messages trigger correctly