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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Nov 15, 2019 at 04:50:57PM -0500, Cole Robinson wrote:
> 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@xxxxxxxxxx>
> > ---
> >  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

Yes, this is just an artifact of the original code. It can be merged
below.

> 
> 
> 
> > +            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

It matches if you define an API without the 'Drv' prefix. I just
need to drop the "^"

eg if i did

diff --git a/src/driver-secret.h b/src/driver-secret.h
index 125238fe7b..ee93eaba68 100644
--- a/src/driver-secret.h
+++ b/src/driver-secret.h
@@ -35,7 +35,7 @@ typedef virSecretPtr
                             const unsigned char *uuid);
 
 typedef virSecretPtr
-(*virDrvSecretLookupByUsage)(virConnectPtr conn,
+(*virFishSecretLookupByUsage)(virConnectPtr conn,
                              int usageType,
                              const char *usageID);


then it would report

  Bogus name *virFishSecretLookupByUsage


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 :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux